Am Mittwoch, den 24.09.2014, 07:09 +0000 schrieb
Hong-Xing.Zhu@xxxxxxxxxxxxx:
[...]
> > > + if (is_imx6sx_pcie(imx6_pcie)) {
> > > + /* Force PCIe PHY reset */
> > > + regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR5,
> > > + IMX6SX_GPR5_PCIE_BTNRST,
> > > + IMX6SX_GPR5_PCIE_BTNRST);
> > > +
> > > + regmap_update_bits(imx6_pcie->gpc_ips_reg, 0, 1 << 7, 1 << 7);
> >
> > Magic values here. Also this is the only time we need to access gpc_ips_reg.
> > So if this is a prerequisite to enabling the ANATOP regulator, I would argue
> > it should be done in the regulator driver.
> [Richard]Magic values would be replaced.
> Yes, this is the only time we need to access gpc_ips_reg.
> It's a little complex to add the GPC manipulations in
> ANATOP/regulator framework/driver codes.
> Since ANATOP regulator is common framework and driver, it's hard to manipulate
> GPC bits in ANATOP/regulator driver.
> In order to be easier, I add the GPC bits manipulation here directly.
> How do you think about that?
I still think it would be better to handle this in the regulator driver.
But as I don't yet have a reference manual for the imx6sx: can you
please describe what this bit does exactly? Maybe this helps me to
understand where the call should be placed.
> >
> > > + /* Power up PCIe PHY, ANATOP_REG_CORE offset 0x140, bit13-9 */
> >
> > Oh so this is actually a PHY regulator, not feeding the whole core, but just
> > the PHY? You could remove the comment it is clear what you are doing from the
> > code and the offsets are of no interest in the PCIe driver.
> [Richard] yes, it is a PHY regulator, not used to feed the whole core.
Ok, so I expect this to be called "pcie-phy-supply" in the binding.
> >
> > > + regulator_set_voltage(imx6_pcie->pcie_regulator,
> > > + 1100000, 1100000);
> > > + ret = regulator_enable(imx6_pcie->pcie_regulator);
> > > + if (ret)
> > > + dev_info(pp->dev, "failed to enable pcie regulator.\n");
> > > + regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > > + IMX6SX_GPR12_RX_EQ_MASK, IMX6SX_GPR12_RX_EQ_2);
> > > + }
> > >
> > > regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > > - IMX6Q_GPR12_PCIE_CTL_2, 0 << 10);
> > > + IMX6Q_GPR12_PCIE_CTL_2,
> > > + IMX6Q_GPR12_PCIE_CTL_2_CLR);
> > >
> > > /* configure constant input signal to the pcie ctrl and phy */
> > > regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > > IMX6Q_GPR12_DEVICE_TYPE, PCI_EXP_TYPE_ROOT_PORT << 12);
> > > regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > > - IMX6Q_GPR12_LOS_LEVEL, 9 << 4);
> > > + IMX6Q_GPR12_LOS_LEVEL, IMX6Q_GPR12_LOS_LEVEL_9);
> > >
> > > regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR8,
> > > IMX6Q_GPR8_TX_DEEMPH_GEN1, 0 << 0); @@ -370,7 +435,8 @@ static
[...]
> > > +static void pci_imx_resume(void)
> > > +{
> > > + struct pcie_port *pp = &imx6_pcie->pp;
> > > +
> > > + if (is_imx6sx_pcie(imx6_pcie)) {
> > > + /* reset iMX6SX PCIe */
> > > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > + IOMUXC_GPR5, BIT(18), 1 << 18);
> > > +
> > > + regmap_update_bits(imx6_pcie->iomuxc_gpr,
> > > + IOMUXC_GPR5, BIT(18), 0 << 18);
> > > +
> >
> > Again magic numbers here. Please add defines for those.
> [Richard] Ok.
> >
> > > + /*
> > > + * controller maybe turn off, re-configure again
> > > + * Set the CLASS_REV of RC CFG header to
> > > + * PCI_CLASS_BRIDGE_PCI
> > > + */
> > > + writel(readl(pp->dbi_base + PCI_CLASS_REVISION)
> > > + | (PCI_CLASS_BRIDGE_PCI << 16),
> > > + pp->dbi_base + PCI_CLASS_REVISION);
> > > +
> >
> > Can't we just move the call to set this from dw_pcie_host_init() to
> > dw_pcie_setup_rc() so we don't need to do this ourselves? It seems to be the
> > more logical change.
> [Richard]dw_pcie_host_init contains the whole re-initialization and re-link-up
> again of the pcie module. It's not proper to re-call dw_pcie_host_init().
> I find that the msi init should be re-configured again after resume back.
> So, the definitions of the "PCIE_MSI_ADDR_LO" and "PCIE_MSI_ADDR_HI" would be used
> here.
>
I seems you misunderstood my comment here. I'm not saying we should call
dw_pcie_host_init() here, which would be clearly wrong.
What I'm saying is that the call to set up the CLASS_REV register is
currently done in dw_pcie_host_init(), but from a quick look at the code
I think it is safe to move this call to dw_pcie_setup_rc().
If you move it to this function there would no need to do it explicitly
from this resume hook again.
> BTW, do you know why the "/* Synopsis specific PCIE configuration registers */"
> is not defined in pcie-designware.h, but in pcie-designware.c?
>
Most probably because the setup for the DW PCIe core should be handled
through pcie-designware.c and not through the individual SoC drivers.
> >
> > > + dw_pcie_setup_rc(pp);
> > > + }
> > > +}
> > > +
> > > +static struct syscore_ops pci_imx_syscore_ops = {
> > > + .suspend = pci_imx_suspend,
> > > + .resume = pci_imx_resume,
> > > +};
> > > +#endif
> > > +
> >
> > Why does this need to be syscore_ops instead of dev_pm_ops?
> [Richard] PM_TURN_OFF msg should be sent out at the end of the suspend of pcie subsystem.
> Resume and re-configure of rc controller should be done before the resume of pcie subsystem.
> So, syscore_ops is used here.
Ok, makes sense.
Regards,
Lucas
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
