On Mon, Aug 11, 2014 at 1:24 AM, Lucas Stach <l.stach@xxxxxxxxxxxxxx> wrote:
> Hi Tim,
>
> Am Samstag, den 09.08.2014, 10:49 -0700 schrieb Tim Harvey:
> [...]
>
>> i/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
>> > index a568efaa331c..1be607360988 100644
>> > --- a/drivers/pci/host/pci-imx6.c
>> > +++ b/drivers/pci/host/pci-imx6.c
>> > @@ -49,6 +49,9 @@ struct imx6_pcie {
>> >
>> > /* PCIe Port Logic registers (memory-mapped) */
>> > #define PL_OFFSET 0x700
>> > +#define PCIE_PL_PFLR (PL_OFFSET + 0x08)
>> > +#define PCIE_PL_PFLR_LINK_STATE_MASK (0x3f << 16)
>> > +#define PCIE_PL_PFLR_FORCE_LINK (1 << 15)
>> > #define PCIE_PHY_DEBUG_R0 (PL_OFFSET + 0x28)
>> > #define PCIE_PHY_DEBUG_R1 (PL_OFFSET + 0x2c)
>> > #define PCIE_PHY_DEBUG_R1_XMLH_LINK_IN_TRAINING (1 << 29)
>> > @@ -214,6 +217,31 @@ static int imx6q_pcie_abort_handler(unsigned long addr,
>> > static int imx6_pcie_assert_core_reset(struct pcie_port *pp)
>> > {
>> > struct imx6_pcie *imx6_pcie = to_imx6_pcie(pp);
>> > + u32 val, gpr1, gpr12;
>> > +
>> > + /*
>> > + * If the bootloader already enabled the link we need some special
>> > + * handling to get the core back into a state where it is safe to
>> > + * touch it for configuration. As there is no dedicated reset signal
>> > + * wired up for MX6QDL, we need to manually force LTSSM into "detect"
>> > + * state before completely disabling LTSSM, which is a prerequisite
>> > + * for core configuration.
>> > + * If both LTSSM_ENABLE and REF_SSP_ENABLE are active we have a strong
>> > + * indication that the bootloader activated the link.
>> > + */
>> > + regmap_read(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1, &gpr1);
>> > + regmap_read(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12, &gpr12);
>> > +
>> > + if ((gpr1 & IMX6Q_GPR1_PCIE_REF_CLK_EN) &&
>> > + (gpr12 & IMX6Q_GPR12_PCIE_CTL_2)) {
>> > + val = readl(pp->dbi_base + PCIE_PL_PFLR);
>> > + val &= ~PCIE_PL_PFLR_LINK_STATE_MASK;
>> > + val |= PCIE_PL_PFLR_FORCE_LINK;
>> > + writel(val, pp->dbi_base + PCIE_PL_PFLR);
>>
>> Lucas,
>>
>> I hate to delay this patch any longer as I believe its critical that
>> this gets in the current kernel. I did however encounter an issue when
>> I backported this to another (older 3.10 kernel I use). The access
>> above to dbi_base to force the link down assumes that PCIe clock is
>> enabled (or it will hang the system). This obviously must be the case
>> for the current kernel (the bootloader enables it and I guess the
>> clock setup for imx6 doesn't disturb it), but on the particular kernel
>> I was backporting to there must be enough differences in the clock
>> tree that the clock setup disabled the PCIe clocks. Thus I had to
>> enable them early, right before the access to dbi_base above in that
>> kernel.
>>
>> I wonder if we should at least put a comment before the dbi_base
>> access above about the assumption that the bootloader also enabled
>> PCIe clock and the clock config left it as such so we don't get burned
>> later on if the clock config changes and causes a hang here.
>>
>
> What clock are we talking about here exactly? If it's just the register
> clock "pcie" we may as well enable it early to make things robust. Other
> clocks may be a bit more problematic.
>
Lucas,
I'm not really sure, I just duplicated what was being done in the
regular clock setup to resolve my issue on a 3.10 vendor kernel where
I have backported the pci driver from mainline. I'm not clear yet why
the clocks still wouldn't be enabled from the bootloader, but I assume
its because of something else that is happening in that vendor kernel.
I don't feel there is anything wrong with your patch and have already
acked it - it solves a real issue with the current driver. Its just
worth realizing that clocks do have to be setup before you access
dbi_base which currently is true, but in the future may depend on
changes that occur outside of the pci driver.
Tim
--
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
