Re: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
Am Mittwoch, den 28.11.2018, 10:55 + schrieb Leonard Crestez:
> On 11/27/18 11:15 PM, Andrey Smirnov wrote:
> > On Tue, Nov 27, 2018 at 2:46 AM Leonard Crestez
> > wrote:
> > > On 11/27/18 12:06 PM, Lucas Stach wrote:
> > > > Am Montag, den 26.11.2018, 10:24 -0800 schrieb Andrey Smirnov:
> > > > > On Tue, Nov 20, 2018 at 2:49 AM Leonard Crestez
> > > > > wrote:
> > > > > > On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
> > > > > > > @@ -921,7 +1004,28 @@ static int imx6_pcie_probe(struct
> > > > > > > platform_device *pdev)
> > > > > > > - case IMX7D:
> > > > > > > + case IMX8MQ:
> > > > > > > + if (of_property_read_u32(node, "fsl,iomux-gpr1x",
> > > > > > > + _pcie->gpr1x)) {
> > > > > > > + dev_err(dev, "Failed to get GPR1x
> > > > > > > address\n");
> > > > > > > + return -EINVAL;
> > > > > > > + }
> > > > > >
> > > > > > This is for distinguishing multiple controllers on the SOC but other
> > > > > > registers and bits might differ. Isn't it preferable to have a
> > > > > > property
> > > > > > for controller id instead of adding many registers to DT?
> > > > >
> > > > > I liked encoding necessary info in DT directly slightly better than
> > > > > encoding abstract ID and then decoding it further in the driver code.
> > > > > OTOH, I am not really attached to that path. Lucas, can you comment on
> > > > > this please?
> > > > Yes, after rereading the patch with this in mind I agree that having
> > > > the GPR offset on DT directly is IMO the better approach than an
> > > > abstract ID.
> > >
> > > But it's not a single offset, for example the device_type (EP/RC) has
> > > bits for the two controllers side-by-side in GPR12.
> > >
> >
> > Playing devil's advocate for a bit:
> >
> > More specifically, currently the following per-controller bits need to
> > be configured:
> >
> > - Location of the "device type" field within GPR12
> > - GPR register to use to control PCIn_CLKREQ_B_OVERRIDE_EN and
> > PCIn_CLKREQ_B_OVERRIDE_EN (GPR14 vs GPR16)
> > - Now that Philip spoke against PCIE_CTRL_APPS_CLK_REQ being exposed
> > via reset controller driver, we need to know which SRC register to use
> > to control that bit (SRC_PCIEPHY_RCR vs. SRC_PCIE2_RCR)
>
> I looked a bit through bindings and there some instances of syscon-$BLH
> properties which include detailed offsets or bitmasks for $BLAH relative
> to the target syscon node.
>
> If you're going the route of adding properties points to IOMUXC/SRC bits
> it would sense to ensure that they're also usable on other SOCs,
> otherwise you're just making 8mq more complicated. But that's hard.
>
> But I think it's easier to deal with such SOC-specific details behind a
> compat string. Maybe the DT list has some opinion on this?
I agree with this. The number of bits and offsets is too large to keep
it in DT properties, without running into backwards compat issues later
on.
> I wonder if of_alias_get_id would be a reasonable way to distinguish
> between pcie0 and pcie1 instead of adding an ctrl-id property?
No, the alias is a arbitrary number that can change from board to
board. It can, but doesn't need to, correspond to any real hardware ID.
If we need a hardware controller ID, then there is no way around adding
a property to the PCIe controller node for this.
Regards,
Lucas
Re: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
Am Mittwoch, den 28.11.2018, 10:55 + schrieb Leonard Crestez:
> On 11/27/18 11:15 PM, Andrey Smirnov wrote:
> > On Tue, Nov 27, 2018 at 2:46 AM Leonard Crestez
> > wrote:
> > > On 11/27/18 12:06 PM, Lucas Stach wrote:
> > > > Am Montag, den 26.11.2018, 10:24 -0800 schrieb Andrey Smirnov:
> > > > > On Tue, Nov 20, 2018 at 2:49 AM Leonard Crestez
> > > > > wrote:
> > > > > > On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
> > > > > > > @@ -921,7 +1004,28 @@ static int imx6_pcie_probe(struct
> > > > > > > platform_device *pdev)
> > > > > > > - case IMX7D:
> > > > > > > + case IMX8MQ:
> > > > > > > + if (of_property_read_u32(node, "fsl,iomux-gpr1x",
> > > > > > > + _pcie->gpr1x)) {
> > > > > > > + dev_err(dev, "Failed to get GPR1x
> > > > > > > address\n");
> > > > > > > + return -EINVAL;
> > > > > > > + }
> > > > > >
> > > > > > This is for distinguishing multiple controllers on the SOC but other
> > > > > > registers and bits might differ. Isn't it preferable to have a
> > > > > > property
> > > > > > for controller id instead of adding many registers to DT?
> > > > >
> > > > > I liked encoding necessary info in DT directly slightly better than
> > > > > encoding abstract ID and then decoding it further in the driver code.
> > > > > OTOH, I am not really attached to that path. Lucas, can you comment on
> > > > > this please?
> > > > Yes, after rereading the patch with this in mind I agree that having
> > > > the GPR offset on DT directly is IMO the better approach than an
> > > > abstract ID.
> > >
> > > But it's not a single offset, for example the device_type (EP/RC) has
> > > bits for the two controllers side-by-side in GPR12.
> > >
> >
> > Playing devil's advocate for a bit:
> >
> > More specifically, currently the following per-controller bits need to
> > be configured:
> >
> > - Location of the "device type" field within GPR12
> > - GPR register to use to control PCIn_CLKREQ_B_OVERRIDE_EN and
> > PCIn_CLKREQ_B_OVERRIDE_EN (GPR14 vs GPR16)
> > - Now that Philip spoke against PCIE_CTRL_APPS_CLK_REQ being exposed
> > via reset controller driver, we need to know which SRC register to use
> > to control that bit (SRC_PCIEPHY_RCR vs. SRC_PCIE2_RCR)
>
> I looked a bit through bindings and there some instances of syscon-$BLH
> properties which include detailed offsets or bitmasks for $BLAH relative
> to the target syscon node.
>
> If you're going the route of adding properties points to IOMUXC/SRC bits
> it would sense to ensure that they're also usable on other SOCs,
> otherwise you're just making 8mq more complicated. But that's hard.
>
> But I think it's easier to deal with such SOC-specific details behind a
> compat string. Maybe the DT list has some opinion on this?
I agree with this. The number of bits and offsets is too large to keep
it in DT properties, without running into backwards compat issues later
on.
> I wonder if of_alias_get_id would be a reasonable way to distinguish
> between pcie0 and pcie1 instead of adding an ctrl-id property?
No, the alias is a arbitrary number that can change from board to
board. It can, but doesn't need to, correspond to any real hardware ID.
If we need a hardware controller ID, then there is no way around adding
a property to the PCIe controller node for this.
Regards,
Lucas
Re: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
Am Dienstag, den 27.11.2018, 13:14 -0800 schrieb Andrey Smirnov:
> On Tue, Nov 27, 2018 at 2:46 AM Leonard Crestez
> wrote:
> >
> > On 11/27/18 12:06 PM, Lucas Stach wrote:
> > > Hi Andrey,
> > >
> > > Am Montag, den 26.11.2018, 10:24 -0800 schrieb Andrey Smirnov:
> > > > On Tue, Nov 20, 2018 at 2:49 AM Leonard Crestez
> > > > wrote:
> > > > >
> > > > > On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
> > > > > > @@ -921,7 +1004,28 @@ static int imx6_pcie_probe(struct
> > > > > > platform_device *pdev)
> > > > > > - case IMX7D:
> > > > > > + case IMX8MQ:
> > > > > > + if (of_property_read_u32(node, "fsl,iomux-gpr1x",
> > > > > > + _pcie->gpr1x)) {
> > > > > > + dev_err(dev, "Failed to get GPR1x address\n");
> > > > > > + return -EINVAL;
> > > > > > + }
> > > > >
> > > > > This is for distinguishing multiple controllers on the SOC but other
> > > > > registers and bits might differ. Isn't it preferable to have a
> > > > > property
> > > > > for controller id instead of adding many registers to DT?
> > > >
> > > > I liked encoding necessary info in DT directly slightly better than
> > > > encoding abstract ID and then decoding it further in the driver code.
> > > > OTOH, I am not really attached to that path. Lucas, can you comment on
> > > > this please?
> > >
> > > Yes, after rereading the patch with this in mind I agree that having
> > > the GPR offset on DT directly is IMO the better approach than an
> > > abstract ID.
> >
> > But it's not a single offset, for example the device_type (EP/RC) has
> > bits for the two controllers side-by-side in GPR12.
> >
>
> Playing devil's advocate for a bit:
>
> More specifically, currently the following per-controller bits need to
> be configured:
>
> - Location of the "device type" field within GPR12
> - GPR register to use to control PCIn_CLKREQ_B_OVERRIDE_EN and
> PCIn_CLKREQ_B_OVERRIDE_EN (GPR14 vs GPR16)
> - Now that Philip spoke against PCIE_CTRL_APPS_CLK_REQ being exposed
> via reset controller driver, we need to know which SRC register to use
> to control that bit (SRC_PCIEPHY_RCR vs. SRC_PCIE2_RCR)
If if we need a lot of different GPR offsets, I revise my earlier
opinion. We certainly don't want lots of bit offsets and masks in the
DT. The whole GPR thing is ugly, but I would rather hide this in the
driver code, keyed by compatible and a controller id property, where we
can change things without any worries about DT backward compatibility.
Regards,
Lucas
Re: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
Am Dienstag, den 27.11.2018, 13:14 -0800 schrieb Andrey Smirnov:
> On Tue, Nov 27, 2018 at 2:46 AM Leonard Crestez
> wrote:
> >
> > On 11/27/18 12:06 PM, Lucas Stach wrote:
> > > Hi Andrey,
> > >
> > > Am Montag, den 26.11.2018, 10:24 -0800 schrieb Andrey Smirnov:
> > > > On Tue, Nov 20, 2018 at 2:49 AM Leonard Crestez
> > > > wrote:
> > > > >
> > > > > On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
> > > > > > @@ -921,7 +1004,28 @@ static int imx6_pcie_probe(struct
> > > > > > platform_device *pdev)
> > > > > > - case IMX7D:
> > > > > > + case IMX8MQ:
> > > > > > + if (of_property_read_u32(node, "fsl,iomux-gpr1x",
> > > > > > + _pcie->gpr1x)) {
> > > > > > + dev_err(dev, "Failed to get GPR1x address\n");
> > > > > > + return -EINVAL;
> > > > > > + }
> > > > >
> > > > > This is for distinguishing multiple controllers on the SOC but other
> > > > > registers and bits might differ. Isn't it preferable to have a
> > > > > property
> > > > > for controller id instead of adding many registers to DT?
> > > >
> > > > I liked encoding necessary info in DT directly slightly better than
> > > > encoding abstract ID and then decoding it further in the driver code.
> > > > OTOH, I am not really attached to that path. Lucas, can you comment on
> > > > this please?
> > >
> > > Yes, after rereading the patch with this in mind I agree that having
> > > the GPR offset on DT directly is IMO the better approach than an
> > > abstract ID.
> >
> > But it's not a single offset, for example the device_type (EP/RC) has
> > bits for the two controllers side-by-side in GPR12.
> >
>
> Playing devil's advocate for a bit:
>
> More specifically, currently the following per-controller bits need to
> be configured:
>
> - Location of the "device type" field within GPR12
> - GPR register to use to control PCIn_CLKREQ_B_OVERRIDE_EN and
> PCIn_CLKREQ_B_OVERRIDE_EN (GPR14 vs GPR16)
> - Now that Philip spoke against PCIE_CTRL_APPS_CLK_REQ being exposed
> via reset controller driver, we need to know which SRC register to use
> to control that bit (SRC_PCIEPHY_RCR vs. SRC_PCIE2_RCR)
If if we need a lot of different GPR offsets, I revise my earlier
opinion. We certainly don't want lots of bit offsets and masks in the
DT. The whole GPR thing is ugly, but I would rather hide this in the
driver code, keyed by compatible and a controller id property, where we
can change things without any worries about DT backward compatibility.
Regards,
Lucas
RE: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
> -Original Message-
> From: Andrey Smirnov [mailto:andrew.smir...@gmail.com]
> Sent: 2018年11月27日 2:10
> To: Richard Zhu
> Cc: linux-kernel ; Bjorn Helgaas
> ; Fabio Estevam ; Chris
> Healy ; Lucas Stach ;
> Leonard Crestez ; Aisheng DONG
> ; dl-linux-imx ;
> linux-arm-kernel ;
> linux-...@vger.kernel.org
> Subject: Re: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
>
> On Sun, Nov 18, 2018 at 11:07 PM Richard Zhu
> wrote:
> >
> > Hi Andrey:
> > Thanks for your patch-set.
> > I have comment about the L1SS implementation below.
> > It's better to figure out one method to fix it.
> >
> > BR
> > Richard
> >
> > > -Original Message-
> > > From: Andrey Smirnov [mailto:andrew.smir...@gmail.com]
> > > Sent: 2018年11月18日 2:12
> > > To: linux-kernel@vger.kernel.org
> > > Cc: Andrey Smirnov ;
> bhelg...@google.com;
> > > Fabio Estevam ; cphe...@gmail.com;
> > > l.st...@pengutronix.de; Leonard Crestez ;
> > > Aisheng DONG ; Richard Zhu
> > > ; dl-linux-imx ;
> > > linux-arm-ker...@lists.infradead.org; linux-...@vger.kernel.org
> > > Subject: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
> > >
> > > Cc: bhelg...@google.com
> > > Cc: Fabio Estevam
> > > Cc: cphe...@gmail.com
> > > Cc: l.st...@pengutronix.de
> > > Cc: Leonard Crestez
> > > Cc: "A.s. Dong"
> > > Cc: Richard Zhu
> > > Cc: linux-...@nxp.com
> > > Cc: linux-arm-ker...@lists.infradead.org
> > > Cc: linux-kernel@vger.kernel.org
> > > Cc: linux-...@vger.kernel.org
> > > Signed-off-by: Andrey Smirnov
> > > ---
> > > drivers/pci/controller/dwc/Kconfig| 2 +-
> > > drivers/pci/controller/dwc/pci-imx6.c | 117
> > > --
> > > 2 files changed, 113 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/Kconfig
> > > b/drivers/pci/controller/dwc/Kconfig
> > > index 91b0194240a5..2b139acccf32 100644
> > > --- a/drivers/pci/controller/dwc/Kconfig
> > > +++ b/drivers/pci/controller/dwc/Kconfig
> > > @@ -90,7 +90,7 @@ config PCI_EXYNOS
> > >
> > > config PCI_IMX6
> > > bool "Freescale i.MX6 PCIe controller"
> > > - depends on SOC_IMX6Q || (ARM && COMPILE_TEST)
> > > + depends on SOC_IMX8MQ || SOC_IMX6Q || (ARM &&
> COMPILE_TEST)
> > > depends on PCI_MSI_IRQ_DOMAIN
> > > select PCIE_DW_HOST
> > >
> > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > > b/drivers/pci/controller/dwc/pci-imx6.c
> > > index 3c3002861d25..8d1f310e41a6 100644
> > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > @@ -8,6 +8,7 @@
> > > * Author: Sean Cross
> > > */
> > >
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -30,6 +31,14 @@
> > >
> > > #include "pcie-designware.h"
> > >
> > > +#define IMX8MQ_PCIE_LINK_CAP_REG_OFFSET 0x7C
> > > +#define IMX8MQ_PCIE_LINK_CAP_L1EL_64US (0x6 <<
> 15)
> > > +
> > > +#define IMX8MQ_GPR_PCIE_REF_USE_PAD BIT(9)
> > > +#define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN BIT(10)
> > > +#define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE BIT(11)
> > > +
> > > +
> > > #define to_imx6_pcie(x) dev_get_drvdata((x)->dev)
> > >
> > > enum imx6_pcie_variants {
> > > @@ -37,6 +46,7 @@ enum imx6_pcie_variants {
> > > IMX6SX,
> > > IMX6QP,
> > > IMX7D,
> > > + IMX8MQ,
> > > };
> > >
> > > struct imx6_pcie {
> > > @@ -48,8 +58,10 @@ struct imx6_pcie {
> > > struct clk *pcie_inbound_axi;
> > > struct clk *pcie;
> > > struct regmap *iomuxc_gpr;
> > > + u32 gpr1x;
> > > struct reset_control*pciephy_reset;
> > > struct reset_control*apps_reset;
> > > + struct reset_control*apps_clk_req;
> > > struct reset_control*turnoff_reset;
> > > enum imx6_pcie_variants variant;
> > > u32 tx_deemph_gen1;
> > > @@ -59,6 +71,7 @@ struct imx6_pcie {
> > > u32 tx_swing_low;
> > &g
RE: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
> -Original Message-
> From: Andrey Smirnov [mailto:andrew.smir...@gmail.com]
> Sent: 2018年11月27日 2:10
> To: Richard Zhu
> Cc: linux-kernel ; Bjorn Helgaas
> ; Fabio Estevam ; Chris
> Healy ; Lucas Stach ;
> Leonard Crestez ; Aisheng DONG
> ; dl-linux-imx ;
> linux-arm-kernel ;
> linux-...@vger.kernel.org
> Subject: Re: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
>
> On Sun, Nov 18, 2018 at 11:07 PM Richard Zhu
> wrote:
> >
> > Hi Andrey:
> > Thanks for your patch-set.
> > I have comment about the L1SS implementation below.
> > It's better to figure out one method to fix it.
> >
> > BR
> > Richard
> >
> > > -Original Message-
> > > From: Andrey Smirnov [mailto:andrew.smir...@gmail.com]
> > > Sent: 2018年11月18日 2:12
> > > To: linux-kernel@vger.kernel.org
> > > Cc: Andrey Smirnov ;
> bhelg...@google.com;
> > > Fabio Estevam ; cphe...@gmail.com;
> > > l.st...@pengutronix.de; Leonard Crestez ;
> > > Aisheng DONG ; Richard Zhu
> > > ; dl-linux-imx ;
> > > linux-arm-ker...@lists.infradead.org; linux-...@vger.kernel.org
> > > Subject: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
> > >
> > > Cc: bhelg...@google.com
> > > Cc: Fabio Estevam
> > > Cc: cphe...@gmail.com
> > > Cc: l.st...@pengutronix.de
> > > Cc: Leonard Crestez
> > > Cc: "A.s. Dong"
> > > Cc: Richard Zhu
> > > Cc: linux-...@nxp.com
> > > Cc: linux-arm-ker...@lists.infradead.org
> > > Cc: linux-kernel@vger.kernel.org
> > > Cc: linux-...@vger.kernel.org
> > > Signed-off-by: Andrey Smirnov
> > > ---
> > > drivers/pci/controller/dwc/Kconfig| 2 +-
> > > drivers/pci/controller/dwc/pci-imx6.c | 117
> > > --
> > > 2 files changed, 113 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/Kconfig
> > > b/drivers/pci/controller/dwc/Kconfig
> > > index 91b0194240a5..2b139acccf32 100644
> > > --- a/drivers/pci/controller/dwc/Kconfig
> > > +++ b/drivers/pci/controller/dwc/Kconfig
> > > @@ -90,7 +90,7 @@ config PCI_EXYNOS
> > >
> > > config PCI_IMX6
> > > bool "Freescale i.MX6 PCIe controller"
> > > - depends on SOC_IMX6Q || (ARM && COMPILE_TEST)
> > > + depends on SOC_IMX8MQ || SOC_IMX6Q || (ARM &&
> COMPILE_TEST)
> > > depends on PCI_MSI_IRQ_DOMAIN
> > > select PCIE_DW_HOST
> > >
> > > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > > b/drivers/pci/controller/dwc/pci-imx6.c
> > > index 3c3002861d25..8d1f310e41a6 100644
> > > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > > @@ -8,6 +8,7 @@
> > > * Author: Sean Cross
> > > */
> > >
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -30,6 +31,14 @@
> > >
> > > #include "pcie-designware.h"
> > >
> > > +#define IMX8MQ_PCIE_LINK_CAP_REG_OFFSET 0x7C
> > > +#define IMX8MQ_PCIE_LINK_CAP_L1EL_64US (0x6 <<
> 15)
> > > +
> > > +#define IMX8MQ_GPR_PCIE_REF_USE_PAD BIT(9)
> > > +#define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN BIT(10)
> > > +#define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE BIT(11)
> > > +
> > > +
> > > #define to_imx6_pcie(x) dev_get_drvdata((x)->dev)
> > >
> > > enum imx6_pcie_variants {
> > > @@ -37,6 +46,7 @@ enum imx6_pcie_variants {
> > > IMX6SX,
> > > IMX6QP,
> > > IMX7D,
> > > + IMX8MQ,
> > > };
> > >
> > > struct imx6_pcie {
> > > @@ -48,8 +58,10 @@ struct imx6_pcie {
> > > struct clk *pcie_inbound_axi;
> > > struct clk *pcie;
> > > struct regmap *iomuxc_gpr;
> > > + u32 gpr1x;
> > > struct reset_control*pciephy_reset;
> > > struct reset_control*apps_reset;
> > > + struct reset_control*apps_clk_req;
> > > struct reset_control*turnoff_reset;
> > > enum imx6_pcie_variants variant;
> > > u32 tx_deemph_gen1;
> > > @@ -59,6 +71,7 @@ struct imx6_pcie {
> > > u32 tx_swing_low;
> > &g
Re: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
On 11/27/18 11:15 PM, Andrey Smirnov wrote:
> On Tue, Nov 27, 2018 at 2:46 AM Leonard Crestez
> wrote:
>> On 11/27/18 12:06 PM, Lucas Stach wrote:
>>> Am Montag, den 26.11.2018, 10:24 -0800 schrieb Andrey Smirnov:
On Tue, Nov 20, 2018 at 2:49 AM Leonard Crestez
wrote:
> On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
>> @@ -921,7 +1004,28 @@ static int imx6_pcie_probe(struct platform_device
>> *pdev)
>> - case IMX7D:
>> + case IMX8MQ:
>> + if (of_property_read_u32(node, "fsl,iomux-gpr1x",
>> + _pcie->gpr1x)) {
>> + dev_err(dev, "Failed to get GPR1x address\n");
>> + return -EINVAL;
>> + }
>
> This is for distinguishing multiple controllers on the SOC but other
> registers and bits might differ. Isn't it preferable to have a property
> for controller id instead of adding many registers to DT?
I liked encoding necessary info in DT directly slightly better than
encoding abstract ID and then decoding it further in the driver code.
OTOH, I am not really attached to that path. Lucas, can you comment on
this please?
>>> Yes, after rereading the patch with this in mind I agree that having
>>> the GPR offset on DT directly is IMO the better approach than an
>>> abstract ID.
>>
>> But it's not a single offset, for example the device_type (EP/RC) has
>> bits for the two controllers side-by-side in GPR12.
>>
>
> Playing devil's advocate for a bit:
>
> More specifically, currently the following per-controller bits need to
> be configured:
>
> - Location of the "device type" field within GPR12
> - GPR register to use to control PCIn_CLKREQ_B_OVERRIDE_EN and
> PCIn_CLKREQ_B_OVERRIDE_EN (GPR14 vs GPR16)
> - Now that Philip spoke against PCIE_CTRL_APPS_CLK_REQ being exposed
> via reset controller driver, we need to know which SRC register to use
> to control that bit (SRC_PCIEPHY_RCR vs. SRC_PCIE2_RCR)
I looked a bit through bindings and there some instances of syscon-$BLH
properties which include detailed offsets or bitmasks for $BLAH relative
to the target syscon node.
If you're going the route of adding properties points to IOMUXC/SRC bits
it would sense to ensure that they're also usable on other SOCs,
otherwise you're just making 8mq more complicated. But that's hard.
But I think it's easier to deal with such SOC-specific details behind a
compat string. Maybe the DT list has some opinion on this?
I wonder if of_alias_get_id would be a reasonable way to distinguish
between pcie0 and pcie1 instead of adding an ctrl-id property?
--
Regards,
Leonard
Re: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
On 11/27/18 11:15 PM, Andrey Smirnov wrote:
> On Tue, Nov 27, 2018 at 2:46 AM Leonard Crestez
> wrote:
>> On 11/27/18 12:06 PM, Lucas Stach wrote:
>>> Am Montag, den 26.11.2018, 10:24 -0800 schrieb Andrey Smirnov:
On Tue, Nov 20, 2018 at 2:49 AM Leonard Crestez
wrote:
> On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
>> @@ -921,7 +1004,28 @@ static int imx6_pcie_probe(struct platform_device
>> *pdev)
>> - case IMX7D:
>> + case IMX8MQ:
>> + if (of_property_read_u32(node, "fsl,iomux-gpr1x",
>> + _pcie->gpr1x)) {
>> + dev_err(dev, "Failed to get GPR1x address\n");
>> + return -EINVAL;
>> + }
>
> This is for distinguishing multiple controllers on the SOC but other
> registers and bits might differ. Isn't it preferable to have a property
> for controller id instead of adding many registers to DT?
I liked encoding necessary info in DT directly slightly better than
encoding abstract ID and then decoding it further in the driver code.
OTOH, I am not really attached to that path. Lucas, can you comment on
this please?
>>> Yes, after rereading the patch with this in mind I agree that having
>>> the GPR offset on DT directly is IMO the better approach than an
>>> abstract ID.
>>
>> But it's not a single offset, for example the device_type (EP/RC) has
>> bits for the two controllers side-by-side in GPR12.
>>
>
> Playing devil's advocate for a bit:
>
> More specifically, currently the following per-controller bits need to
> be configured:
>
> - Location of the "device type" field within GPR12
> - GPR register to use to control PCIn_CLKREQ_B_OVERRIDE_EN and
> PCIn_CLKREQ_B_OVERRIDE_EN (GPR14 vs GPR16)
> - Now that Philip spoke against PCIE_CTRL_APPS_CLK_REQ being exposed
> via reset controller driver, we need to know which SRC register to use
> to control that bit (SRC_PCIEPHY_RCR vs. SRC_PCIE2_RCR)
I looked a bit through bindings and there some instances of syscon-$BLH
properties which include detailed offsets or bitmasks for $BLAH relative
to the target syscon node.
If you're going the route of adding properties points to IOMUXC/SRC bits
it would sense to ensure that they're also usable on other SOCs,
otherwise you're just making 8mq more complicated. But that's hard.
But I think it's easier to deal with such SOC-specific details behind a
compat string. Maybe the DT list has some opinion on this?
I wonder if of_alias_get_id would be a reasonable way to distinguish
between pcie0 and pcie1 instead of adding an ctrl-id property?
--
Regards,
Leonard
Re: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
On Tue, Nov 27, 2018 at 2:46 AM Leonard Crestez wrote:
>
> On 11/27/18 12:06 PM, Lucas Stach wrote:
> > Hi Andrey,
> >
> > Am Montag, den 26.11.2018, 10:24 -0800 schrieb Andrey Smirnov:
> >> On Tue, Nov 20, 2018 at 2:49 AM Leonard Crestez
> >> wrote:
> >>>
> >>> On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
> @@ -921,7 +1004,28 @@ static int imx6_pcie_probe(struct platform_device
> *pdev)
> - case IMX7D:
> + case IMX8MQ:
> + if (of_property_read_u32(node, "fsl,iomux-gpr1x",
> + _pcie->gpr1x)) {
> + dev_err(dev, "Failed to get GPR1x address\n");
> + return -EINVAL;
> + }
> >>>
> >>> This is for distinguishing multiple controllers on the SOC but other
> >>> registers and bits might differ. Isn't it preferable to have a property
> >>> for controller id instead of adding many registers to DT?
> >>
> >> I liked encoding necessary info in DT directly slightly better than
> >> encoding abstract ID and then decoding it further in the driver code.
> >> OTOH, I am not really attached to that path. Lucas, can you comment on
> >> this please?
> >
> > Yes, after rereading the patch with this in mind I agree that having
> > the GPR offset on DT directly is IMO the better approach than an
> > abstract ID.
>
> But it's not a single offset, for example the device_type (EP/RC) has
> bits for the two controllers side-by-side in GPR12.
>
Playing devil's advocate for a bit:
More specifically, currently the following per-controller bits need to
be configured:
- Location of the "device type" field within GPR12
- GPR register to use to control PCIn_CLKREQ_B_OVERRIDE_EN and
PCIn_CLKREQ_B_OVERRIDE_EN (GPR14 vs GPR16)
- Now that Philip spoke against PCIE_CTRL_APPS_CLK_REQ being exposed
via reset controller driver, we need to know which SRC register to use
to control that bit (SRC_PCIEPHY_RCR vs. SRC_PCIE2_RCR)
Thanks,
Andrey Smirnov
Re: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
On Tue, Nov 27, 2018 at 2:46 AM Leonard Crestez wrote:
>
> On 11/27/18 12:06 PM, Lucas Stach wrote:
> > Hi Andrey,
> >
> > Am Montag, den 26.11.2018, 10:24 -0800 schrieb Andrey Smirnov:
> >> On Tue, Nov 20, 2018 at 2:49 AM Leonard Crestez
> >> wrote:
> >>>
> >>> On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
> @@ -921,7 +1004,28 @@ static int imx6_pcie_probe(struct platform_device
> *pdev)
> - case IMX7D:
> + case IMX8MQ:
> + if (of_property_read_u32(node, "fsl,iomux-gpr1x",
> + _pcie->gpr1x)) {
> + dev_err(dev, "Failed to get GPR1x address\n");
> + return -EINVAL;
> + }
> >>>
> >>> This is for distinguishing multiple controllers on the SOC but other
> >>> registers and bits might differ. Isn't it preferable to have a property
> >>> for controller id instead of adding many registers to DT?
> >>
> >> I liked encoding necessary info in DT directly slightly better than
> >> encoding abstract ID and then decoding it further in the driver code.
> >> OTOH, I am not really attached to that path. Lucas, can you comment on
> >> this please?
> >
> > Yes, after rereading the patch with this in mind I agree that having
> > the GPR offset on DT directly is IMO the better approach than an
> > abstract ID.
>
> But it's not a single offset, for example the device_type (EP/RC) has
> bits for the two controllers side-by-side in GPR12.
>
Playing devil's advocate for a bit:
More specifically, currently the following per-controller bits need to
be configured:
- Location of the "device type" field within GPR12
- GPR register to use to control PCIn_CLKREQ_B_OVERRIDE_EN and
PCIn_CLKREQ_B_OVERRIDE_EN (GPR14 vs GPR16)
- Now that Philip spoke against PCIE_CTRL_APPS_CLK_REQ being exposed
via reset controller driver, we need to know which SRC register to use
to control that bit (SRC_PCIEPHY_RCR vs. SRC_PCIE2_RCR)
Thanks,
Andrey Smirnov
Re: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
On Tue, Nov 27, 2018 at 2:16 AM Leonard Crestez wrote:
>
> On 11/26/18 8:24 PM, Andrey Smirnov wrote:
> > On Tue, Nov 20, 2018 at 2:49 AM Leonard Crestez
> > wrote:
> >> On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
>
> >>> + if (of_property_read_u32_array(
> >>> + node, "fsl,gpr12-device-type",
> >>> + imx6_pcie->device_type,
> >>> + ARRAY_SIZE(imx6_pcie->device_type))) {
> >>> + dev_err(dev, "Failed to get device type
> >>> mask/value\n");
> >>> + return -EINVAL;
> >>> + }
> >>
> >> The device type can be set on multiple SOCs, why are you adding a
> >> mandatory property only for 8m?
> >
> > My thinking was that other SoCs don't really have two controllers, so
> > they don't really need to have that property, but, more importantly,
> > not forcing them to have this property should preserve backwards
> > compatibility with old DTBs.
>
> The "device_type" registers determine Root Complex versus End Point
> mode. This is not yet supported on imx in upstream but it can be done on
> many soc versions.
>
Yeah, I am aware, I wasn't really trying to make it possible to
configure IP block to be a EP, that is just an unavoidable consequence
of the approach taken.
> Looking at other pcie controllers (and dwc core) this is normally done
> with a different compatible string with an "-ep" suffix. It feels wrong
> to expose bitmasks and values directly in DT instead.
>
> For this patch you should probably just hardcode RC mode.
Hmm, given how the mask/value have to be different for each
controller, the only way to hardcode it that I can think of would be
to change the property pass a bit offset instead of mask/value pair.
Is that what you are suggesting?
Thanks,
Andrey Smirnov
Re: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
On Tue, Nov 27, 2018 at 2:16 AM Leonard Crestez wrote:
>
> On 11/26/18 8:24 PM, Andrey Smirnov wrote:
> > On Tue, Nov 20, 2018 at 2:49 AM Leonard Crestez
> > wrote:
> >> On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
>
> >>> + if (of_property_read_u32_array(
> >>> + node, "fsl,gpr12-device-type",
> >>> + imx6_pcie->device_type,
> >>> + ARRAY_SIZE(imx6_pcie->device_type))) {
> >>> + dev_err(dev, "Failed to get device type
> >>> mask/value\n");
> >>> + return -EINVAL;
> >>> + }
> >>
> >> The device type can be set on multiple SOCs, why are you adding a
> >> mandatory property only for 8m?
> >
> > My thinking was that other SoCs don't really have two controllers, so
> > they don't really need to have that property, but, more importantly,
> > not forcing them to have this property should preserve backwards
> > compatibility with old DTBs.
>
> The "device_type" registers determine Root Complex versus End Point
> mode. This is not yet supported on imx in upstream but it can be done on
> many soc versions.
>
Yeah, I am aware, I wasn't really trying to make it possible to
configure IP block to be a EP, that is just an unavoidable consequence
of the approach taken.
> Looking at other pcie controllers (and dwc core) this is normally done
> with a different compatible string with an "-ep" suffix. It feels wrong
> to expose bitmasks and values directly in DT instead.
>
> For this patch you should probably just hardcode RC mode.
Hmm, given how the mask/value have to be different for each
controller, the only way to hardcode it that I can think of would be
to change the property pass a bit offset instead of mask/value pair.
Is that what you are suggesting?
Thanks,
Andrey Smirnov
Re: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
On 11/27/18 12:06 PM, Lucas Stach wrote:
> Hi Andrey,
>
> Am Montag, den 26.11.2018, 10:24 -0800 schrieb Andrey Smirnov:
>> On Tue, Nov 20, 2018 at 2:49 AM Leonard Crestez
>> wrote:
>>>
>>> On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
@@ -921,7 +1004,28 @@ static int imx6_pcie_probe(struct platform_device
*pdev)
- case IMX7D:
+ case IMX8MQ:
+ if (of_property_read_u32(node, "fsl,iomux-gpr1x",
+ _pcie->gpr1x)) {
+ dev_err(dev, "Failed to get GPR1x address\n");
+ return -EINVAL;
+ }
>>>
>>> This is for distinguishing multiple controllers on the SOC but other
>>> registers and bits might differ. Isn't it preferable to have a property
>>> for controller id instead of adding many registers to DT?
>>
>> I liked encoding necessary info in DT directly slightly better than
>> encoding abstract ID and then decoding it further in the driver code.
>> OTOH, I am not really attached to that path. Lucas, can you comment on
>> this please?
>
> Yes, after rereading the patch with this in mind I agree that having
> the GPR offset on DT directly is IMO the better approach than an
> abstract ID.
But it's not a single offset, for example the device_type (EP/RC) has
bits for the two controllers side-by-side in GPR12.
Adding a "ctrl-id" property would fully describe the hardware from the
start. If we add per-register information instead then when other
registers are used we'll get DT compatibility headaches.
It's worth noting that 8qm/8qxp also have multiple controllers with a
different set of bits placed differently in iomuxc.
Re: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
On 11/27/18 12:06 PM, Lucas Stach wrote:
> Hi Andrey,
>
> Am Montag, den 26.11.2018, 10:24 -0800 schrieb Andrey Smirnov:
>> On Tue, Nov 20, 2018 at 2:49 AM Leonard Crestez
>> wrote:
>>>
>>> On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
@@ -921,7 +1004,28 @@ static int imx6_pcie_probe(struct platform_device
*pdev)
- case IMX7D:
+ case IMX8MQ:
+ if (of_property_read_u32(node, "fsl,iomux-gpr1x",
+ _pcie->gpr1x)) {
+ dev_err(dev, "Failed to get GPR1x address\n");
+ return -EINVAL;
+ }
>>>
>>> This is for distinguishing multiple controllers on the SOC but other
>>> registers and bits might differ. Isn't it preferable to have a property
>>> for controller id instead of adding many registers to DT?
>>
>> I liked encoding necessary info in DT directly slightly better than
>> encoding abstract ID and then decoding it further in the driver code.
>> OTOH, I am not really attached to that path. Lucas, can you comment on
>> this please?
>
> Yes, after rereading the patch with this in mind I agree that having
> the GPR offset on DT directly is IMO the better approach than an
> abstract ID.
But it's not a single offset, for example the device_type (EP/RC) has
bits for the two controllers side-by-side in GPR12.
Adding a "ctrl-id" property would fully describe the hardware from the
start. If we add per-register information instead then when other
registers are used we'll get DT compatibility headaches.
It's worth noting that 8qm/8qxp also have multiple controllers with a
different set of bits placed differently in iomuxc.
Re: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
On 11/26/18 8:24 PM, Andrey Smirnov wrote:
> On Tue, Nov 20, 2018 at 2:49 AM Leonard Crestez
> wrote:
>> On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
>>> + if (of_property_read_u32_array(
>>> + node, "fsl,gpr12-device-type",
>>> + imx6_pcie->device_type,
>>> + ARRAY_SIZE(imx6_pcie->device_type))) {
>>> + dev_err(dev, "Failed to get device type
>>> mask/value\n");
>>> + return -EINVAL;
>>> + }
>>
>> The device type can be set on multiple SOCs, why are you adding a
>> mandatory property only for 8m?
>
> My thinking was that other SoCs don't really have two controllers, so
> they don't really need to have that property, but, more importantly,
> not forcing them to have this property should preserve backwards
> compatibility with old DTBs.
The "device_type" registers determine Root Complex versus End Point
mode. This is not yet supported on imx in upstream but it can be done on
many soc versions.
Looking at other pcie controllers (and dwc core) this is normally done
with a different compatible string with an "-ep" suffix. It feels wrong
to expose bitmasks and values directly in DT instead.
For this patch you should probably just hardcode RC mode.
Re: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
On 11/26/18 8:24 PM, Andrey Smirnov wrote:
> On Tue, Nov 20, 2018 at 2:49 AM Leonard Crestez
> wrote:
>> On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
>>> + if (of_property_read_u32_array(
>>> + node, "fsl,gpr12-device-type",
>>> + imx6_pcie->device_type,
>>> + ARRAY_SIZE(imx6_pcie->device_type))) {
>>> + dev_err(dev, "Failed to get device type
>>> mask/value\n");
>>> + return -EINVAL;
>>> + }
>>
>> The device type can be set on multiple SOCs, why are you adding a
>> mandatory property only for 8m?
>
> My thinking was that other SoCs don't really have two controllers, so
> they don't really need to have that property, but, more importantly,
> not forcing them to have this property should preserve backwards
> compatibility with old DTBs.
The "device_type" registers determine Root Complex versus End Point
mode. This is not yet supported on imx in upstream but it can be done on
many soc versions.
Looking at other pcie controllers (and dwc core) this is normally done
with a different compatible string with an "-ep" suffix. It feels wrong
to expose bitmasks and values directly in DT instead.
For this patch you should probably just hardcode RC mode.
Re: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
Hi Andrey,
Am Montag, den 26.11.2018, 10:24 -0800 schrieb Andrey Smirnov:
> On Tue, Nov 20, 2018 at 2:49 AM Leonard Crestez
> wrote:
> >
> > On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
> > > @@ -921,7 +1004,28 @@ static int imx6_pcie_probe(struct platform_device
> > > *pdev)
> > > - case IMX7D:
> > > + case IMX8MQ:
> > > + if (of_property_read_u32(node, "fsl,iomux-gpr1x",
> > > + _pcie->gpr1x)) {
> > > + dev_err(dev, "Failed to get GPR1x address\n");
> > > + return -EINVAL;
> > > + }
> >
> > This is for distinguishing multiple controllers on the SOC but other
> > registers and bits might differ. Isn't it preferable to have a property
> > for controller id instead of adding many registers to DT?
> >
>
> I liked encoding necessary info in DT directly slightly better than
> encoding abstract ID and then decoding it further in the driver code.
> OTOH, I am not really attached to that path. Lucas, can you comment on
> this please?
Yes, after rereading the patch with this in mind I agree that having
the GPR offset on DT directly is IMO the better approach than an
abstract ID.
One other thing I noticed is that we probably need some property to
encode if the clock is supplied by an external clkgen, or if the clock
is provided by the i.MX. Hardcoding this in the driver will lead to DT
backward compatibility headaches later on if someone decides to build a
board with the clock provided from the i.MX.
Regards,
Lucas
Re: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
Hi Andrey,
Am Montag, den 26.11.2018, 10:24 -0800 schrieb Andrey Smirnov:
> On Tue, Nov 20, 2018 at 2:49 AM Leonard Crestez
> wrote:
> >
> > On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
> > > @@ -921,7 +1004,28 @@ static int imx6_pcie_probe(struct platform_device
> > > *pdev)
> > > - case IMX7D:
> > > + case IMX8MQ:
> > > + if (of_property_read_u32(node, "fsl,iomux-gpr1x",
> > > + _pcie->gpr1x)) {
> > > + dev_err(dev, "Failed to get GPR1x address\n");
> > > + return -EINVAL;
> > > + }
> >
> > This is for distinguishing multiple controllers on the SOC but other
> > registers and bits might differ. Isn't it preferable to have a property
> > for controller id instead of adding many registers to DT?
> >
>
> I liked encoding necessary info in DT directly slightly better than
> encoding abstract ID and then decoding it further in the driver code.
> OTOH, I am not really attached to that path. Lucas, can you comment on
> this please?
Yes, after rereading the patch with this in mind I agree that having
the GPR offset on DT directly is IMO the better approach than an
abstract ID.
One other thing I noticed is that we probably need some property to
encode if the clock is supplied by an external clkgen, or if the clock
is provided by the i.MX. Hardcoding this in the driver will lead to DT
backward compatibility headaches later on if someone decides to build a
board with the clock provided from the i.MX.
Regards,
Lucas
Re: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
On Tue, Nov 20, 2018 at 2:49 AM Leonard Crestez wrote:
>
> On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
> > @@ -921,7 +1004,28 @@ static int imx6_pcie_probe(struct platform_device
> > *pdev)
> > - case IMX7D:
> > + case IMX8MQ:
> > + if (of_property_read_u32(node, "fsl,iomux-gpr1x",
> > + _pcie->gpr1x)) {
> > + dev_err(dev, "Failed to get GPR1x address\n");
> > + return -EINVAL;
> > + }
>
> This is for distinguishing multiple controllers on the SOC but other
> registers and bits might differ. Isn't it preferable to have a property
> for controller id instead of adding many registers to DT?
>
I liked encoding necessary info in DT directly slightly better than
encoding abstract ID and then decoding it further in the driver code.
OTOH, I am not really attached to that path. Lucas, can you comment on
this please?
> > +
> > + if (of_property_read_u32_array(
> > + node, "fsl,gpr12-device-type",
> > + imx6_pcie->device_type,
> > + ARRAY_SIZE(imx6_pcie->device_type))) {
> > + dev_err(dev, "Failed to get device type
> > mask/value\n");
> > + return -EINVAL;
> > + }
>
> The device type can be set on multiple SOCs, why are you adding a
> mandatory property only for 8m?
My thinking was that other SoCs don't really have two controllers, so
they don't really need to have that property, but, more importantly,
not forcing them to have this property should preserve backwards
compatibility with old DTBs.
>
> There should probably be a separate patch with documented DT bindings.
>
Yes, definitely, I just wanted to come up with a set of bindings
agreed on by the driver maintainers first.
Thanks,
Andrey Smirnov
Re: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
On Tue, Nov 20, 2018 at 2:49 AM Leonard Crestez wrote:
>
> On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
> > @@ -921,7 +1004,28 @@ static int imx6_pcie_probe(struct platform_device
> > *pdev)
> > - case IMX7D:
> > + case IMX8MQ:
> > + if (of_property_read_u32(node, "fsl,iomux-gpr1x",
> > + _pcie->gpr1x)) {
> > + dev_err(dev, "Failed to get GPR1x address\n");
> > + return -EINVAL;
> > + }
>
> This is for distinguishing multiple controllers on the SOC but other
> registers and bits might differ. Isn't it preferable to have a property
> for controller id instead of adding many registers to DT?
>
I liked encoding necessary info in DT directly slightly better than
encoding abstract ID and then decoding it further in the driver code.
OTOH, I am not really attached to that path. Lucas, can you comment on
this please?
> > +
> > + if (of_property_read_u32_array(
> > + node, "fsl,gpr12-device-type",
> > + imx6_pcie->device_type,
> > + ARRAY_SIZE(imx6_pcie->device_type))) {
> > + dev_err(dev, "Failed to get device type
> > mask/value\n");
> > + return -EINVAL;
> > + }
>
> The device type can be set on multiple SOCs, why are you adding a
> mandatory property only for 8m?
My thinking was that other SoCs don't really have two controllers, so
they don't really need to have that property, but, more importantly,
not forcing them to have this property should preserve backwards
compatibility with old DTBs.
>
> There should probably be a separate patch with documented DT bindings.
>
Yes, definitely, I just wanted to come up with a set of bindings
agreed on by the driver maintainers first.
Thanks,
Andrey Smirnov
Re: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
On Sun, Nov 18, 2018 at 11:07 PM Richard Zhu wrote:
>
> Hi Andrey:
> Thanks for your patch-set.
> I have comment about the L1SS implementation below.
> It's better to figure out one method to fix it.
>
> BR
> Richard
>
> > -Original Message-
> > From: Andrey Smirnov [mailto:andrew.smir...@gmail.com]
> > Sent: 2018年11月18日 2:12
> > To: linux-kernel@vger.kernel.org
> > Cc: Andrey Smirnov ; bhelg...@google.com;
> > Fabio Estevam ; cphe...@gmail.com;
> > l.st...@pengutronix.de; Leonard Crestez ;
> > Aisheng DONG ; Richard Zhu
> > ; dl-linux-imx ;
> > linux-arm-ker...@lists.infradead.org; linux-...@vger.kernel.org
> > Subject: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
> >
> > Cc: bhelg...@google.com
> > Cc: Fabio Estevam
> > Cc: cphe...@gmail.com
> > Cc: l.st...@pengutronix.de
> > Cc: Leonard Crestez
> > Cc: "A.s. Dong"
> > Cc: Richard Zhu
> > Cc: linux-...@nxp.com
> > Cc: linux-arm-ker...@lists.infradead.org
> > Cc: linux-kernel@vger.kernel.org
> > Cc: linux-...@vger.kernel.org
> > Signed-off-by: Andrey Smirnov
> > ---
> > drivers/pci/controller/dwc/Kconfig| 2 +-
> > drivers/pci/controller/dwc/pci-imx6.c | 117 --
> > 2 files changed, 113 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/Kconfig
> > b/drivers/pci/controller/dwc/Kconfig
> > index 91b0194240a5..2b139acccf32 100644
> > --- a/drivers/pci/controller/dwc/Kconfig
> > +++ b/drivers/pci/controller/dwc/Kconfig
> > @@ -90,7 +90,7 @@ config PCI_EXYNOS
> >
> > config PCI_IMX6
> > bool "Freescale i.MX6 PCIe controller"
> > - depends on SOC_IMX6Q || (ARM && COMPILE_TEST)
> > + depends on SOC_IMX8MQ || SOC_IMX6Q || (ARM && COMPILE_TEST)
> > depends on PCI_MSI_IRQ_DOMAIN
> > select PCIE_DW_HOST
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 3c3002861d25..8d1f310e41a6 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -8,6 +8,7 @@
> > * Author: Sean Cross
> > */
> >
> > +#include
> > #include
> > #include
> > #include
> > @@ -30,6 +31,14 @@
> >
> > #include "pcie-designware.h"
> >
> > +#define IMX8MQ_PCIE_LINK_CAP_REG_OFFSET 0x7C
> > +#define IMX8MQ_PCIE_LINK_CAP_L1EL_64US (0x6 << 15)
> > +
> > +#define IMX8MQ_GPR_PCIE_REF_USE_PAD BIT(9)
> > +#define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN BIT(10)
> > +#define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE BIT(11)
> > +
> > +
> > #define to_imx6_pcie(x) dev_get_drvdata((x)->dev)
> >
> > enum imx6_pcie_variants {
> > @@ -37,6 +46,7 @@ enum imx6_pcie_variants {
> > IMX6SX,
> > IMX6QP,
> > IMX7D,
> > + IMX8MQ,
> > };
> >
> > struct imx6_pcie {
> > @@ -48,8 +58,10 @@ struct imx6_pcie {
> > struct clk *pcie_inbound_axi;
> > struct clk *pcie;
> > struct regmap *iomuxc_gpr;
> > + u32 gpr1x;
> > struct reset_control*pciephy_reset;
> > struct reset_control*apps_reset;
> > + struct reset_control*apps_clk_req;
> > struct reset_control*turnoff_reset;
> > enum imx6_pcie_variants variant;
> > u32 tx_deemph_gen1;
> > @@ -59,6 +71,7 @@ struct imx6_pcie {
> > u32 tx_swing_low;
> > int link_gen;
> > struct regulator*vpcie;
> > + u32 device_type[2];
> > };
> >
> > /* Parameters for the waiting for PCIe PHY PLL to lock on i.MX7 */ @@
> > -245,7 +258,8 @@ static void imx6_pcie_reset_phy(struct imx6_pcie
> > *imx6_pcie) {
> > u32 tmp;
> >
> > - if (imx6_pcie->variant == IMX7D)
> > + if (imx6_pcie->variant == IMX7D ||
> > + imx6_pcie->variant == IMX8MQ)
> > return;
> >
> > pcie_phy_read(imx6_pcie, PHY_RX_OVRD_IN_LO, ); @@ -261,6
> > +275,7 @@ static void imx6_pcie_reset_phy(struct imx6_pcie *imx6_pcie)
> > pcie_phy_write(imx6_pcie, PHY_RX_OVRD_IN_LO, tmp); }
> >
> > +#ifdef CONFIG_ARM
> > /* Added for PCI abort handling */
> > static int imx6q_pcie_abort_handler(unsigned long addr,
> > unsigned int fsr, struct pt_regs *regs) @@ -294,6 +309,7 @@
> > static
> > int imx6q_pcie_abort_handler(unsigned long addr,
> >
> > return 1;
> > }
> > +#endif
> >
> > static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> > { @@ -301,6 +317,7 @@ static void imx6_pcie_assert_core_reset(struct
> > imx6_pcie *imx6_pcie)
> >
> > switch (imx6_pcie->variant) {
> > case IMX7D:
> > + case IMX8MQ: /* FALLTHROUGH */
> > reset_control_assert(imx6_pcie->pciephy_reset);
> > reset_control_assert(imx6_pcie->apps_reset);
> > break;
> > @@ -369,6 +386,18 @@ static int imx6_pcie_enable_ref_clk(struct
> > imx6_pcie *imx6_pcie)
> > break;
> > case IMX7D:
> >
Re: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
On Sun, Nov 18, 2018 at 11:07 PM Richard Zhu wrote:
>
> Hi Andrey:
> Thanks for your patch-set.
> I have comment about the L1SS implementation below.
> It's better to figure out one method to fix it.
>
> BR
> Richard
>
> > -Original Message-
> > From: Andrey Smirnov [mailto:andrew.smir...@gmail.com]
> > Sent: 2018年11月18日 2:12
> > To: linux-kernel@vger.kernel.org
> > Cc: Andrey Smirnov ; bhelg...@google.com;
> > Fabio Estevam ; cphe...@gmail.com;
> > l.st...@pengutronix.de; Leonard Crestez ;
> > Aisheng DONG ; Richard Zhu
> > ; dl-linux-imx ;
> > linux-arm-ker...@lists.infradead.org; linux-...@vger.kernel.org
> > Subject: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
> >
> > Cc: bhelg...@google.com
> > Cc: Fabio Estevam
> > Cc: cphe...@gmail.com
> > Cc: l.st...@pengutronix.de
> > Cc: Leonard Crestez
> > Cc: "A.s. Dong"
> > Cc: Richard Zhu
> > Cc: linux-...@nxp.com
> > Cc: linux-arm-ker...@lists.infradead.org
> > Cc: linux-kernel@vger.kernel.org
> > Cc: linux-...@vger.kernel.org
> > Signed-off-by: Andrey Smirnov
> > ---
> > drivers/pci/controller/dwc/Kconfig| 2 +-
> > drivers/pci/controller/dwc/pci-imx6.c | 117 --
> > 2 files changed, 113 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/Kconfig
> > b/drivers/pci/controller/dwc/Kconfig
> > index 91b0194240a5..2b139acccf32 100644
> > --- a/drivers/pci/controller/dwc/Kconfig
> > +++ b/drivers/pci/controller/dwc/Kconfig
> > @@ -90,7 +90,7 @@ config PCI_EXYNOS
> >
> > config PCI_IMX6
> > bool "Freescale i.MX6 PCIe controller"
> > - depends on SOC_IMX6Q || (ARM && COMPILE_TEST)
> > + depends on SOC_IMX8MQ || SOC_IMX6Q || (ARM && COMPILE_TEST)
> > depends on PCI_MSI_IRQ_DOMAIN
> > select PCIE_DW_HOST
> >
> > diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> > b/drivers/pci/controller/dwc/pci-imx6.c
> > index 3c3002861d25..8d1f310e41a6 100644
> > --- a/drivers/pci/controller/dwc/pci-imx6.c
> > +++ b/drivers/pci/controller/dwc/pci-imx6.c
> > @@ -8,6 +8,7 @@
> > * Author: Sean Cross
> > */
> >
> > +#include
> > #include
> > #include
> > #include
> > @@ -30,6 +31,14 @@
> >
> > #include "pcie-designware.h"
> >
> > +#define IMX8MQ_PCIE_LINK_CAP_REG_OFFSET 0x7C
> > +#define IMX8MQ_PCIE_LINK_CAP_L1EL_64US (0x6 << 15)
> > +
> > +#define IMX8MQ_GPR_PCIE_REF_USE_PAD BIT(9)
> > +#define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN BIT(10)
> > +#define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE BIT(11)
> > +
> > +
> > #define to_imx6_pcie(x) dev_get_drvdata((x)->dev)
> >
> > enum imx6_pcie_variants {
> > @@ -37,6 +46,7 @@ enum imx6_pcie_variants {
> > IMX6SX,
> > IMX6QP,
> > IMX7D,
> > + IMX8MQ,
> > };
> >
> > struct imx6_pcie {
> > @@ -48,8 +58,10 @@ struct imx6_pcie {
> > struct clk *pcie_inbound_axi;
> > struct clk *pcie;
> > struct regmap *iomuxc_gpr;
> > + u32 gpr1x;
> > struct reset_control*pciephy_reset;
> > struct reset_control*apps_reset;
> > + struct reset_control*apps_clk_req;
> > struct reset_control*turnoff_reset;
> > enum imx6_pcie_variants variant;
> > u32 tx_deemph_gen1;
> > @@ -59,6 +71,7 @@ struct imx6_pcie {
> > u32 tx_swing_low;
> > int link_gen;
> > struct regulator*vpcie;
> > + u32 device_type[2];
> > };
> >
> > /* Parameters for the waiting for PCIe PHY PLL to lock on i.MX7 */ @@
> > -245,7 +258,8 @@ static void imx6_pcie_reset_phy(struct imx6_pcie
> > *imx6_pcie) {
> > u32 tmp;
> >
> > - if (imx6_pcie->variant == IMX7D)
> > + if (imx6_pcie->variant == IMX7D ||
> > + imx6_pcie->variant == IMX8MQ)
> > return;
> >
> > pcie_phy_read(imx6_pcie, PHY_RX_OVRD_IN_LO, ); @@ -261,6
> > +275,7 @@ static void imx6_pcie_reset_phy(struct imx6_pcie *imx6_pcie)
> > pcie_phy_write(imx6_pcie, PHY_RX_OVRD_IN_LO, tmp); }
> >
> > +#ifdef CONFIG_ARM
> > /* Added for PCI abort handling */
> > static int imx6q_pcie_abort_handler(unsigned long addr,
> > unsigned int fsr, struct pt_regs *regs) @@ -294,6 +309,7 @@
> > static
> > int imx6q_pcie_abort_handler(unsigned long addr,
> >
> > return 1;
> > }
> > +#endif
> >
> > static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> > { @@ -301,6 +317,7 @@ static void imx6_pcie_assert_core_reset(struct
> > imx6_pcie *imx6_pcie)
> >
> > switch (imx6_pcie->variant) {
> > case IMX7D:
> > + case IMX8MQ: /* FALLTHROUGH */
> > reset_control_assert(imx6_pcie->pciephy_reset);
> > reset_control_assert(imx6_pcie->apps_reset);
> > break;
> > @@ -369,6 +386,18 @@ static int imx6_pcie_enable_ref_clk(struct
> > imx6_pcie *imx6_pcie)
> > break;
> > case IMX7D:
> >
Re: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
> @@ -921,7 +1004,28 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> - case IMX7D:
> + case IMX8MQ:
> + if (of_property_read_u32(node, "fsl,iomux-gpr1x",
> + _pcie->gpr1x)) {
> + dev_err(dev, "Failed to get GPR1x address\n");
> + return -EINVAL;
> + }
This is for distinguishing multiple controllers on the SOC but other
registers and bits might differ. Isn't it preferable to have a property
for controller id instead of adding many registers to DT?
> +
> + if (of_property_read_u32_array(
> + node, "fsl,gpr12-device-type",
> + imx6_pcie->device_type,
> + ARRAY_SIZE(imx6_pcie->device_type))) {
> + dev_err(dev, "Failed to get device type
> mask/value\n");
> + return -EINVAL;
> + }
The device type can be set on multiple SOCs, why are you adding a
mandatory property only for 8m?
There should probably be a separate patch with documented DT bindings.
--
Regards,
Leonard
Re: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
On Sat, 2018-11-17 at 10:12 -0800, Andrey Smirnov wrote:
> @@ -921,7 +1004,28 @@ static int imx6_pcie_probe(struct platform_device *pdev)
> - case IMX7D:
> + case IMX8MQ:
> + if (of_property_read_u32(node, "fsl,iomux-gpr1x",
> + _pcie->gpr1x)) {
> + dev_err(dev, "Failed to get GPR1x address\n");
> + return -EINVAL;
> + }
This is for distinguishing multiple controllers on the SOC but other
registers and bits might differ. Isn't it preferable to have a property
for controller id instead of adding many registers to DT?
> +
> + if (of_property_read_u32_array(
> + node, "fsl,gpr12-device-type",
> + imx6_pcie->device_type,
> + ARRAY_SIZE(imx6_pcie->device_type))) {
> + dev_err(dev, "Failed to get device type
> mask/value\n");
> + return -EINVAL;
> + }
The device type can be set on multiple SOCs, why are you adding a
mandatory property only for 8m?
There should probably be a separate patch with documented DT bindings.
--
Regards,
Leonard
RE: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
Hi Andrey:
Thanks for your patch-set.
I have comment about the L1SS implementation below.
It's better to figure out one method to fix it.
BR
Richard
> -Original Message-
> From: Andrey Smirnov [mailto:andrew.smir...@gmail.com]
> Sent: 2018年11月18日 2:12
> To: linux-kernel@vger.kernel.org
> Cc: Andrey Smirnov ; bhelg...@google.com;
> Fabio Estevam ; cphe...@gmail.com;
> l.st...@pengutronix.de; Leonard Crestez ;
> Aisheng DONG ; Richard Zhu
> ; dl-linux-imx ;
> linux-arm-ker...@lists.infradead.org; linux-...@vger.kernel.org
> Subject: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
>
> Cc: bhelg...@google.com
> Cc: Fabio Estevam
> Cc: cphe...@gmail.com
> Cc: l.st...@pengutronix.de
> Cc: Leonard Crestez
> Cc: "A.s. Dong"
> Cc: Richard Zhu
> Cc: linux-...@nxp.com
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-...@vger.kernel.org
> Signed-off-by: Andrey Smirnov
> ---
> drivers/pci/controller/dwc/Kconfig| 2 +-
> drivers/pci/controller/dwc/pci-imx6.c | 117 --
> 2 files changed, 113 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/Kconfig
> b/drivers/pci/controller/dwc/Kconfig
> index 91b0194240a5..2b139acccf32 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -90,7 +90,7 @@ config PCI_EXYNOS
>
> config PCI_IMX6
> bool "Freescale i.MX6 PCIe controller"
> - depends on SOC_IMX6Q || (ARM && COMPILE_TEST)
> + depends on SOC_IMX8MQ || SOC_IMX6Q || (ARM && COMPILE_TEST)
> depends on PCI_MSI_IRQ_DOMAIN
> select PCIE_DW_HOST
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> b/drivers/pci/controller/dwc/pci-imx6.c
> index 3c3002861d25..8d1f310e41a6 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -8,6 +8,7 @@
> * Author: Sean Cross
> */
>
> +#include
> #include
> #include
> #include
> @@ -30,6 +31,14 @@
>
> #include "pcie-designware.h"
>
> +#define IMX8MQ_PCIE_LINK_CAP_REG_OFFSET 0x7C
> +#define IMX8MQ_PCIE_LINK_CAP_L1EL_64US (0x6 << 15)
> +
> +#define IMX8MQ_GPR_PCIE_REF_USE_PAD BIT(9)
> +#define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN BIT(10)
> +#define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE BIT(11)
> +
> +
> #define to_imx6_pcie(x) dev_get_drvdata((x)->dev)
>
> enum imx6_pcie_variants {
> @@ -37,6 +46,7 @@ enum imx6_pcie_variants {
> IMX6SX,
> IMX6QP,
> IMX7D,
> + IMX8MQ,
> };
>
> struct imx6_pcie {
> @@ -48,8 +58,10 @@ struct imx6_pcie {
> struct clk *pcie_inbound_axi;
> struct clk *pcie;
> struct regmap *iomuxc_gpr;
> + u32 gpr1x;
> struct reset_control*pciephy_reset;
> struct reset_control*apps_reset;
> + struct reset_control*apps_clk_req;
> struct reset_control*turnoff_reset;
> enum imx6_pcie_variants variant;
> u32 tx_deemph_gen1;
> @@ -59,6 +71,7 @@ struct imx6_pcie {
> u32 tx_swing_low;
> int link_gen;
> struct regulator*vpcie;
> + u32 device_type[2];
> };
>
> /* Parameters for the waiting for PCIe PHY PLL to lock on i.MX7 */ @@
> -245,7 +258,8 @@ static void imx6_pcie_reset_phy(struct imx6_pcie
> *imx6_pcie) {
> u32 tmp;
>
> - if (imx6_pcie->variant == IMX7D)
> + if (imx6_pcie->variant == IMX7D ||
> + imx6_pcie->variant == IMX8MQ)
> return;
>
> pcie_phy_read(imx6_pcie, PHY_RX_OVRD_IN_LO, ); @@ -261,6
> +275,7 @@ static void imx6_pcie_reset_phy(struct imx6_pcie *imx6_pcie)
> pcie_phy_write(imx6_pcie, PHY_RX_OVRD_IN_LO, tmp); }
>
> +#ifdef CONFIG_ARM
> /* Added for PCI abort handling */
> static int imx6q_pcie_abort_handler(unsigned long addr,
> unsigned int fsr, struct pt_regs *regs) @@ -294,6 +309,7 @@
> static
> int imx6q_pcie_abort_handler(unsigned long addr,
>
> return 1;
> }
> +#endif
>
> static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> { @@ -301,6 +317,7 @@ static void imx6_pcie_assert_core_reset(struct
> imx6_pcie *imx6_pcie)
>
> switch (imx6_pcie->variant) {
> case IMX7D:
> + case IMX8MQ: /* FALLTHROUGH */
> reset_control_assert(imx6_pcie->pciephy_reset);
> reset_control_assert(imx6_pcie->apps_reset);
> break;
> @@ -369,6 +386,18 @@ static int imx6_pcie_enable_ref_clk(struct
> imx6_pcie *imx6_pcie)
> break;
> case IMX7D:
> break;
> + case IMX8MQ:
> + /*
> + * Set the over ride low and enabled
> + * make sure that REF_CLK is turned on.
> + */
> + regmap_update_bits(imx6_pcie->iomuxc_gpr, imx6_pcie->gpr1x,
> +IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE,
> +
RE: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
Hi Andrey:
Thanks for your patch-set.
I have comment about the L1SS implementation below.
It's better to figure out one method to fix it.
BR
Richard
> -Original Message-
> From: Andrey Smirnov [mailto:andrew.smir...@gmail.com]
> Sent: 2018年11月18日 2:12
> To: linux-kernel@vger.kernel.org
> Cc: Andrey Smirnov ; bhelg...@google.com;
> Fabio Estevam ; cphe...@gmail.com;
> l.st...@pengutronix.de; Leonard Crestez ;
> Aisheng DONG ; Richard Zhu
> ; dl-linux-imx ;
> linux-arm-ker...@lists.infradead.org; linux-...@vger.kernel.org
> Subject: [PATCH 3/3] PCI: imx: Add support for i.MX8MQ
>
> Cc: bhelg...@google.com
> Cc: Fabio Estevam
> Cc: cphe...@gmail.com
> Cc: l.st...@pengutronix.de
> Cc: Leonard Crestez
> Cc: "A.s. Dong"
> Cc: Richard Zhu
> Cc: linux-...@nxp.com
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-...@vger.kernel.org
> Signed-off-by: Andrey Smirnov
> ---
> drivers/pci/controller/dwc/Kconfig| 2 +-
> drivers/pci/controller/dwc/pci-imx6.c | 117 --
> 2 files changed, 113 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/Kconfig
> b/drivers/pci/controller/dwc/Kconfig
> index 91b0194240a5..2b139acccf32 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -90,7 +90,7 @@ config PCI_EXYNOS
>
> config PCI_IMX6
> bool "Freescale i.MX6 PCIe controller"
> - depends on SOC_IMX6Q || (ARM && COMPILE_TEST)
> + depends on SOC_IMX8MQ || SOC_IMX6Q || (ARM && COMPILE_TEST)
> depends on PCI_MSI_IRQ_DOMAIN
> select PCIE_DW_HOST
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c
> b/drivers/pci/controller/dwc/pci-imx6.c
> index 3c3002861d25..8d1f310e41a6 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -8,6 +8,7 @@
> * Author: Sean Cross
> */
>
> +#include
> #include
> #include
> #include
> @@ -30,6 +31,14 @@
>
> #include "pcie-designware.h"
>
> +#define IMX8MQ_PCIE_LINK_CAP_REG_OFFSET 0x7C
> +#define IMX8MQ_PCIE_LINK_CAP_L1EL_64US (0x6 << 15)
> +
> +#define IMX8MQ_GPR_PCIE_REF_USE_PAD BIT(9)
> +#define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE_EN BIT(10)
> +#define IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE BIT(11)
> +
> +
> #define to_imx6_pcie(x) dev_get_drvdata((x)->dev)
>
> enum imx6_pcie_variants {
> @@ -37,6 +46,7 @@ enum imx6_pcie_variants {
> IMX6SX,
> IMX6QP,
> IMX7D,
> + IMX8MQ,
> };
>
> struct imx6_pcie {
> @@ -48,8 +58,10 @@ struct imx6_pcie {
> struct clk *pcie_inbound_axi;
> struct clk *pcie;
> struct regmap *iomuxc_gpr;
> + u32 gpr1x;
> struct reset_control*pciephy_reset;
> struct reset_control*apps_reset;
> + struct reset_control*apps_clk_req;
> struct reset_control*turnoff_reset;
> enum imx6_pcie_variants variant;
> u32 tx_deemph_gen1;
> @@ -59,6 +71,7 @@ struct imx6_pcie {
> u32 tx_swing_low;
> int link_gen;
> struct regulator*vpcie;
> + u32 device_type[2];
> };
>
> /* Parameters for the waiting for PCIe PHY PLL to lock on i.MX7 */ @@
> -245,7 +258,8 @@ static void imx6_pcie_reset_phy(struct imx6_pcie
> *imx6_pcie) {
> u32 tmp;
>
> - if (imx6_pcie->variant == IMX7D)
> + if (imx6_pcie->variant == IMX7D ||
> + imx6_pcie->variant == IMX8MQ)
> return;
>
> pcie_phy_read(imx6_pcie, PHY_RX_OVRD_IN_LO, ); @@ -261,6
> +275,7 @@ static void imx6_pcie_reset_phy(struct imx6_pcie *imx6_pcie)
> pcie_phy_write(imx6_pcie, PHY_RX_OVRD_IN_LO, tmp); }
>
> +#ifdef CONFIG_ARM
> /* Added for PCI abort handling */
> static int imx6q_pcie_abort_handler(unsigned long addr,
> unsigned int fsr, struct pt_regs *regs) @@ -294,6 +309,7 @@
> static
> int imx6q_pcie_abort_handler(unsigned long addr,
>
> return 1;
> }
> +#endif
>
> static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> { @@ -301,6 +317,7 @@ static void imx6_pcie_assert_core_reset(struct
> imx6_pcie *imx6_pcie)
>
> switch (imx6_pcie->variant) {
> case IMX7D:
> + case IMX8MQ: /* FALLTHROUGH */
> reset_control_assert(imx6_pcie->pciephy_reset);
> reset_control_assert(imx6_pcie->apps_reset);
> break;
> @@ -369,6 +386,18 @@ static int imx6_pcie_enable_ref_clk(struct
> imx6_pcie *imx6_pcie)
> break;
> case IMX7D:
> break;
> + case IMX8MQ:
> + /*
> + * Set the over ride low and enabled
> + * make sure that REF_CLK is turned on.
> + */
> + regmap_update_bits(imx6_pcie->iomuxc_gpr, imx6_pcie->gpr1x,
> +IMX8MQ_GPR_PCIE_CLK_REQ_OVERRIDE,
> +
