Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1581481 > unrolled thread

Re: [PATCH v4 3/3] PCI: imx6: Add code to support i.MX7D

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 3/3] PCI: imx6: Add code to support i.MX7D Rob Herring <robh@kernel.org> - 2017-02-15 18:20 +0100
    Re: [PATCH v4 3/3] PCI: imx6: Add code to support i.MX7D Bjorn Helgaas <helgaas@kernel.org> - 2017-02-15 18:40 +0100
      Re: [PATCH v4 3/3] PCI: imx6: Add code to support i.MX7D Rob Herring <robh@kernel.org> - 2017-02-15 22:30 +0100
        Re: [PATCH v4 3/3] PCI: imx6: Add code to support i.MX7D Bjorn Helgaas <helgaas@kernel.org> - 2017-02-15 23:00 +0100
          Re: [PATCH v4 3/3] PCI: imx6: Add code to support i.MX7D Lucas Stach <l.stach@pengutronix.de> - 2017-02-16 10:20 +0100
    Re: [PATCH v4 3/3] PCI: imx6: Add code to support i.MX7D Andrey Smirnov <andrew.smirnov@gmail.com> - 2017-02-16 07:10 +0100
    Re: [PATCH v4 3/3] PCI: imx6: Add code to support i.MX7D Lucas Stach <l.stach@pengutronix.de> - 2017-02-16 10:20 +0100
      Re: [PATCH v4 3/3] PCI: imx6: Add code to support i.MX7D Rob Herring <robh@kernel.org> - 2017-02-21 17:40 +0100
        Re: [PATCH v4 3/3] PCI: imx6: Add code to support i.MX7D Lucas Stach <l.stach@pengutronix.de> - 2017-02-21 17:50 +0100

#1581481 — Re: [PATCH v4 3/3] PCI: imx6: Add code to support i.MX7D

On Tue, Feb 07, 2017 at 07:50:27AM -0800, Andrey Smirnov wrote:
> Add various bits of code needed to support i.MX7D variant of the IP.
> 
> Cc: yurovsky@gmail.com
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: devicetree@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>  .../devicetree/bindings/pci/fsl,imx6q-pcie.txt     |  13 ++-
>  drivers/pci/host/pci-imx6.c                        | 121 ++++++++++++++++-----
>  include/linux/mfd/syscon/imx7-iomuxc-gpr.h         |   4 +
>  3 files changed, 112 insertions(+), 26 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> index 83aeb1f..11db2ab 100644
> --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> @@ -4,7 +4,11 @@ This PCIe host controller is based on the Synopsis Designware PCIe IP
>  and thus inherits all the common properties defined in designware-pcie.txt.
>  
>  Required properties:
> -- compatible: "fsl,imx6q-pcie", "fsl,imx6sx-pcie", "fsl,imx6qp-pcie"
> +- compatible:
> +	- "fsl,imx6q-pcie"
> +	- "fsl,imx6sx-pcie",
> +	- "fsl,imx6qp-pcie"
> +	- "fsl,imx7d-pcie"
>  - reg: base address and length of the PCIe controller
>  - interrupts: A list of interrupt outputs of the controller. Must contain an
>    entry for each entry in the interrupt-names property.
> @@ -34,6 +38,13 @@ Additional required properties for imx6sx-pcie:
>  - clock names: Must include the following additional entries:
>  	- "pcie_inbound_axi"
>  
> +Additional required properties for imx7d-pcie:
> +- power-domains: Must be set to a phandle pointing to PCIE_PHY power domain

This domain is just the PHY? Seems like this needs a separate PHY 
driver.

> +- resets: Must contain phandles to PCIE related reset lines exposed by SRC IP block
> +- reset-names: Must contain the following entires:
> +  	       - "pciephy"

And for this too.

> +	       - "apps"
> +
>  Example:
>  
>  	pcie@0x01000000 {

[...]

> @@ -251,6 +261,10 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
>  	u32 val, gpr1, gpr12;
>  
>  	switch (imx6_pcie->variant) {
> +	case IMX7D:
> +		reset_control_assert(imx6_pcie->pciephy_reset);
> +		reset_control_assert(imx6_pcie->apps_reset);
> +		break;
>  	case IMX6SX:
>  		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
>  				   IMX6SX_GPR12_PCIE_TEST_POWERDOWN,

So the difference with i.MX7D is not really that it has a reset or not, 
but some platforms use a reset driver and some do not. The latter should 
be fixed.

Rob

[toc] | [next] | [standalone]

#1581492

On Wed, Feb 15, 2017 at 11:17:00AM -0600, Rob Herring wrote:
> On Tue, Feb 07, 2017 at 07:50:27AM -0800, Andrey Smirnov wrote:
> > Add various bits of code needed to support i.MX7D variant of the IP.
> > 
> > Cc: yurovsky@gmail.com
> > Cc: Lucas Stach <l.stach@pengutronix.de>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Lee Jones <lee.jones@linaro.org>
> > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: devicetree@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > ---
> >  .../devicetree/bindings/pci/fsl,imx6q-pcie.txt     |  13 ++-
> >  drivers/pci/host/pci-imx6.c                        | 121 ++++++++++++++++-----
> >  include/linux/mfd/syscon/imx7-iomuxc-gpr.h         |   4 +
> >  3 files changed, 112 insertions(+), 26 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> > index 83aeb1f..11db2ab 100644
> > --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> > +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> > @@ -4,7 +4,11 @@ This PCIe host controller is based on the Synopsis Designware PCIe IP
> >  and thus inherits all the common properties defined in designware-pcie.txt.
> >  
> >  Required properties:
> > -- compatible: "fsl,imx6q-pcie", "fsl,imx6sx-pcie", "fsl,imx6qp-pcie"
> > +- compatible:
> > +	- "fsl,imx6q-pcie"
> > +	- "fsl,imx6sx-pcie",
> > +	- "fsl,imx6qp-pcie"
> > +	- "fsl,imx7d-pcie"
> >  - reg: base address and length of the PCIe controller
> >  - interrupts: A list of interrupt outputs of the controller. Must contain an
> >    entry for each entry in the interrupt-names property.
> > @@ -34,6 +38,13 @@ Additional required properties for imx6sx-pcie:
> >  - clock names: Must include the following additional entries:
> >  	- "pcie_inbound_axi"
> >  
> > +Additional required properties for imx7d-pcie:
> > +- power-domains: Must be set to a phandle pointing to PCIE_PHY power domain
> 
> This domain is just the PHY? Seems like this needs a separate PHY 
> driver.
> 
> > +- resets: Must contain phandles to PCIE related reset lines exposed by SRC IP block
> > +- reset-names: Must contain the following entires:
> > +  	       - "pciephy"
> 
> And for this too.
> 
> > +	       - "apps"
> > +
> >  Example:
> >  
> >  	pcie@0x01000000 {
> 
> [...]
> 
> > @@ -251,6 +261,10 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> >  	u32 val, gpr1, gpr12;
> >  
> >  	switch (imx6_pcie->variant) {
> > +	case IMX7D:
> > +		reset_control_assert(imx6_pcie->pciephy_reset);
> > +		reset_control_assert(imx6_pcie->apps_reset);
> > +		break;
> >  	case IMX6SX:
> >  		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> >  				   IMX6SX_GPR12_PCIE_TEST_POWERDOWN,
> 
> So the difference with i.MX7D is not really that it has a reset or not, 
> but some platforms use a reset driver and some do not. The latter should 
> be fixed.

I have this patch queued for v4.11.  Are these things that should be
fixed first?  If so, I can drop this.

Bjorn

[toc] | [prev] | [next] | [standalone]

#1581676

On Wed, Feb 15, 2017 at 11:38:50AM -0600, Bjorn Helgaas wrote:
> On Wed, Feb 15, 2017 at 11:17:00AM -0600, Rob Herring wrote:
> > On Tue, Feb 07, 2017 at 07:50:27AM -0800, Andrey Smirnov wrote:
> > > Add various bits of code needed to support i.MX7D variant of the IP.

> > 
> > [...]
> > 
> > > @@ -251,6 +261,10 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> > >  	u32 val, gpr1, gpr12;
> > >  
> > >  	switch (imx6_pcie->variant) {
> > > +	case IMX7D:
> > > +		reset_control_assert(imx6_pcie->pciephy_reset);
> > > +		reset_control_assert(imx6_pcie->apps_reset);
> > > +		break;
> > >  	case IMX6SX:
> > >  		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > >  				   IMX6SX_GPR12_PCIE_TEST_POWERDOWN,
> > 
> > So the difference with i.MX7D is not really that it has a reset or not, 
> > but some platforms use a reset driver and some do not. The latter should 
> > be fixed.
> 
> I have this patch queued for v4.11.  Are these things that should be
> fixed first?  If so, I can drop this.

Well, depends if you trust things will get fixed later and if the PHY 
in fact should be separate as that affects the binding. It would affect 
how the driver changes are done as instead of "if (IMX7D) ...", you'd 
have "if (imx6_pcie->apps_reset) ..." for example. That part depends on
how much churn you want there.

Rob

[toc] | [prev] | [next] | [standalone]

#1581703

On Wed, Feb 15, 2017 at 03:26:24PM -0600, Rob Herring wrote:
> On Wed, Feb 15, 2017 at 11:38:50AM -0600, Bjorn Helgaas wrote:
> > On Wed, Feb 15, 2017 at 11:17:00AM -0600, Rob Herring wrote:
> > > On Tue, Feb 07, 2017 at 07:50:27AM -0800, Andrey Smirnov wrote:
> > > > Add various bits of code needed to support i.MX7D variant of the IP.
> 
> > > 
> > > [...]
> > > 
> > > > @@ -251,6 +261,10 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> > > >  	u32 val, gpr1, gpr12;
> > > >  
> > > >  	switch (imx6_pcie->variant) {
> > > > +	case IMX7D:
> > > > +		reset_control_assert(imx6_pcie->pciephy_reset);
> > > > +		reset_control_assert(imx6_pcie->apps_reset);
> > > > +		break;
> > > >  	case IMX6SX:
> > > >  		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > > >  				   IMX6SX_GPR12_PCIE_TEST_POWERDOWN,
> > > 
> > > So the difference with i.MX7D is not really that it has a reset or not, 
> > > but some platforms use a reset driver and some do not. The latter should 
> > > be fixed.
> > 
> > I have this patch queued for v4.11.  Are these things that should be
> > fixed first?  If so, I can drop this.
> 
> Well, depends if you trust things will get fixed later and if the PHY 
> in fact should be separate as that affects the binding. It would affect 
> how the driver changes are done as instead of "if (IMX7D) ...", you'd 
> have "if (imx6_pcie->apps_reset) ..." for example. That part depends on
> how much churn you want there.

I dropped it for now, not that I don't trust it will get fixed, but it
sounds like not completely trivial changes and will affect the binding
as well, so the intermediate state sounds a little messy.

Bjorn

[toc] | [prev] | [next] | [standalone]

#1582406

Am Mittwoch, den 15.02.2017, 15:57 -0600 schrieb Bjorn Helgaas:
> On Wed, Feb 15, 2017 at 03:26:24PM -0600, Rob Herring wrote:
> > On Wed, Feb 15, 2017 at 11:38:50AM -0600, Bjorn Helgaas wrote:
> > > On Wed, Feb 15, 2017 at 11:17:00AM -0600, Rob Herring wrote:
> > > > On Tue, Feb 07, 2017 at 07:50:27AM -0800, Andrey Smirnov wrote:
> > > > > Add various bits of code needed to support i.MX7D variant of the IP.
> > 
> > > > 
> > > > [...]
> > > > 
> > > > > @@ -251,6 +261,10 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> > > > >  	u32 val, gpr1, gpr12;
> > > > >  
> > > > >  	switch (imx6_pcie->variant) {
> > > > > +	case IMX7D:
> > > > > +		reset_control_assert(imx6_pcie->pciephy_reset);
> > > > > +		reset_control_assert(imx6_pcie->apps_reset);
> > > > > +		break;
> > > > >  	case IMX6SX:
> > > > >  		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> > > > >  				   IMX6SX_GPR12_PCIE_TEST_POWERDOWN,
> > > > 
> > > > So the difference with i.MX7D is not really that it has a reset or not, 
> > > > but some platforms use a reset driver and some do not. The latter should 
> > > > be fixed.
> > > 
> > > I have this patch queued for v4.11.  Are these things that should be
> > > fixed first?  If so, I can drop this.
> > 
> > Well, depends if you trust things will get fixed later and if the PHY 
> > in fact should be separate as that affects the binding. It would affect 
> > how the driver changes are done as instead of "if (IMX7D) ...", you'd 
> > have "if (imx6_pcie->apps_reset) ..." for example. That part depends on
> > how much churn you want there.
> 
> I dropped it for now, not that I don't trust it will get fixed, but it
> sounds like not completely trivial changes and will affect the binding
> as well, so the intermediate state sounds a little messy.

As I pointed out in direct reply to Rob, I honestly think the binding is
fine as is and properly reflects the hardware. But I guess he'll comment
on that, so JFYI.

Regards,
Lucas

[toc] | [prev] | [next] | [standalone]

#1582347

On Wed, Feb 15, 2017 at 9:17 AM, Rob Herring <robh@kernel.org> wrote:
> On Tue, Feb 07, 2017 at 07:50:27AM -0800, Andrey Smirnov wrote:
>> Add various bits of code needed to support i.MX7D variant of the IP.
>>
>> Cc: yurovsky@gmail.com
>> Cc: Lucas Stach <l.stach@pengutronix.de>
>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Lee Jones <lee.jones@linaro.org>
>> Cc: Fabio Estevam <fabio.estevam@nxp.com>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: devicetree@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> ---
>>  .../devicetree/bindings/pci/fsl,imx6q-pcie.txt     |  13 ++-
>>  drivers/pci/host/pci-imx6.c                        | 121 ++++++++++++++++-----
>>  include/linux/mfd/syscon/imx7-iomuxc-gpr.h         |   4 +
>>  3 files changed, 112 insertions(+), 26 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
>> index 83aeb1f..11db2ab 100644
>> --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
>> +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
>> @@ -4,7 +4,11 @@ This PCIe host controller is based on the Synopsis Designware PCIe IP
>>  and thus inherits all the common properties defined in designware-pcie.txt.
>>
>>  Required properties:
>> -- compatible: "fsl,imx6q-pcie", "fsl,imx6sx-pcie", "fsl,imx6qp-pcie"
>> +- compatible:
>> +     - "fsl,imx6q-pcie"
>> +     - "fsl,imx6sx-pcie",
>> +     - "fsl,imx6qp-pcie"
>> +     - "fsl,imx7d-pcie"
>>  - reg: base address and length of the PCIe controller
>>  - interrupts: A list of interrupt outputs of the controller. Must contain an
>>    entry for each entry in the interrupt-names property.
>> @@ -34,6 +38,13 @@ Additional required properties for imx6sx-pcie:
>>  - clock names: Must include the following additional entries:
>>       - "pcie_inbound_axi"
>>
>> +Additional required properties for imx7d-pcie:
>> +- power-domains: Must be set to a phandle pointing to PCIE_PHY power domain
>
> This domain is just the PHY? Seems like this needs a separate PHY
> driver.

PCIE_PHY is the name of the power domain corresponding to PGC_PCIE
(which is what that property is expected to point to) as per
Frescale/NXP datasheet (p. 822 in v0.1 of i.MX7 Application Processors
Manual). I was never able to find any clear language indicating what
parts of DesignWare's IP core and Freescale's/NXP's PCIE PHY it powers
in the manual. However, experiments with hardware show that when that
domain remains non-powered any attempt to access registers of DW's IP
block result in system hanging, so it seemed to me that the two are
not independent of each other enough to be represented as individual
DT nodes.

>
>> +- resets: Must contain phandles to PCIE related reset lines exposed by SRC IP block
>> +- reset-names: Must contain the following entires:
>> +            - "pciephy"
>
> And for this too.
>
>> +            - "apps"
>> +
>>  Example:
>>
>>       pcie@0x01000000 {
>
> [...]
>
>> @@ -251,6 +261,10 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
>>       u32 val, gpr1, gpr12;
>>
>>       switch (imx6_pcie->variant) {
>> +     case IMX7D:
>> +             reset_control_assert(imx6_pcie->pciephy_reset);
>> +             reset_control_assert(imx6_pcie->apps_reset);
>> +             break;
>>       case IMX6SX:
>>               regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
>>                                  IMX6SX_GPR12_PCIE_TEST_POWERDOWN,
>
> So the difference with i.MX7D is not really that it has a reset or not,
> but some platforms use a reset driver and some do not. The latter should
> be fixed.

That depends on what variant of the SoC you are comparing it to. 6QP,
6SX do have reset and helper signals wire to bits in registers in
IOMUX, 6Q howerver doesn't have a reset line wire and have to do some
trickery as per comment in the driver several lines below:

"... 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 memory serves me well part of that 6Q trickery code is the reason
for driver using hook_fault_code().

That is not to say that all of this code could not be encapsulated as
a reset controller, and I agree, doing so might make the driver
better. At the same time I don't have the hardware to test all of
those platforms and I am hoping we can agree this kind of change to be
out of scope of this series.

Thanks,
Andrey Smironv

[toc] | [prev] | [next] | [standalone]

#1582403

Am Mittwoch, den 15.02.2017, 11:17 -0600 schrieb Rob Herring:
> On Tue, Feb 07, 2017 at 07:50:27AM -0800, Andrey Smirnov wrote:
> > Add various bits of code needed to support i.MX7D variant of the IP.
> > 
> > Cc: yurovsky@gmail.com
> > Cc: Lucas Stach <l.stach@pengutronix.de>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Lee Jones <lee.jones@linaro.org>
> > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: devicetree@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > ---
> >  .../devicetree/bindings/pci/fsl,imx6q-pcie.txt     |  13 ++-
> >  drivers/pci/host/pci-imx6.c                        | 121 ++++++++++++++++-----
> >  include/linux/mfd/syscon/imx7-iomuxc-gpr.h         |   4 +
> >  3 files changed, 112 insertions(+), 26 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> > index 83aeb1f..11db2ab 100644
> > --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> > +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> > @@ -4,7 +4,11 @@ This PCIe host controller is based on the Synopsis Designware PCIe IP
> >  and thus inherits all the common properties defined in designware-pcie.txt.
> >  
> >  Required properties:
> > -- compatible: "fsl,imx6q-pcie", "fsl,imx6sx-pcie", "fsl,imx6qp-pcie"
> > +- compatible:
> > +	- "fsl,imx6q-pcie"
> > +	- "fsl,imx6sx-pcie",
> > +	- "fsl,imx6qp-pcie"
> > +	- "fsl,imx7d-pcie"
> >  - reg: base address and length of the PCIe controller
> >  - interrupts: A list of interrupt outputs of the controller. Must contain an
> >    entry for each entry in the interrupt-names property.
> > @@ -34,6 +38,13 @@ Additional required properties for imx6sx-pcie:
> >  - clock names: Must include the following additional entries:
> >  	- "pcie_inbound_axi"
> >  
> > +Additional required properties for imx7d-pcie:
> > +- power-domains: Must be set to a phandle pointing to PCIE_PHY power domain
> 
> This domain is just the PHY? Seems like this needs a separate PHY 
> driver.
> 
No, it's called the PHY power domain, as that is probably the part that
draws the most power, but the PCIe core also looses it's state when this
domain is powered down. So it's probably the complete core that is
inside this domain.

> > +- resets: Must contain phandles to PCIE related reset lines exposed by SRC IP block
> > +- reset-names: Must contain the following entires:
> > +  	       - "pciephy"
> 
> And for this too.
> 
> > +	       - "apps"
> > +
> >  Example:
> >  
> >  	pcie@0x01000000 {
> 
> [...]
> 
> > @@ -251,6 +261,10 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
> >  	u32 val, gpr1, gpr12;
> >  
> >  	switch (imx6_pcie->variant) {
> > +	case IMX7D:
> > +		reset_control_assert(imx6_pcie->pciephy_reset);
> > +		reset_control_assert(imx6_pcie->apps_reset);
> > +		break;
> >  	case IMX6SX:
> >  		regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
> >  				   IMX6SX_GPR12_PCIE_TEST_POWERDOWN,
> 
> So the difference with i.MX7D is not really that it has a reset or not, 
> but some platforms use a reset driver and some do not. The latter should 
> be fixed.

The resets on anything before i.MX7 are not in a separate reset driver,
but are just some signals from the PCIe core wired into a syscon (IOMUX
GPR) area. While we could invent a reset controller for those, I don't
see how this would improve things. Especially as the reset on i.MX6
seems to be some side-effect of the "power-down" signal of the core, so
not strictly a reset.

Also I don't see why we should change the binding for the driver with a
long history of deployed DTs. That seems like a total waste of manpower.

Regards,
Lucas

[toc] | [prev] | [next] | [standalone]

#1585517

On Thu, Feb 16, 2017 at 3:12 AM, Lucas Stach <l.stach@pengutronix.de> wrote:
> Am Mittwoch, den 15.02.2017, 11:17 -0600 schrieb Rob Herring:
>> On Tue, Feb 07, 2017 at 07:50:27AM -0800, Andrey Smirnov wrote:
>> > Add various bits of code needed to support i.MX7D variant of the IP.
>> >
>> > Cc: yurovsky@gmail.com
>> > Cc: Lucas Stach <l.stach@pengutronix.de>
>> > Cc: Bjorn Helgaas <bhelgaas@google.com>
>> > Cc: Rob Herring <robh+dt@kernel.org>
>> > Cc: Mark Rutland <mark.rutland@arm.com>
>> > Cc: Lee Jones <lee.jones@linaro.org>
>> > Cc: Fabio Estevam <fabio.estevam@nxp.com>
>> > Cc: linux-arm-kernel@lists.infradead.org
>> > Cc: devicetree@vger.kernel.org
>> > Cc: linux-kernel@vger.kernel.org
>> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> > ---
>> >  .../devicetree/bindings/pci/fsl,imx6q-pcie.txt     |  13 ++-
>> >  drivers/pci/host/pci-imx6.c                        | 121 ++++++++++++++++-----
>> >  include/linux/mfd/syscon/imx7-iomuxc-gpr.h         |   4 +
>> >  3 files changed, 112 insertions(+), 26 deletions(-)
>> >
>> > diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
>> > index 83aeb1f..11db2ab 100644
>> > --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
>> > +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
>> > @@ -4,7 +4,11 @@ This PCIe host controller is based on the Synopsis Designware PCIe IP
>> >  and thus inherits all the common properties defined in designware-pcie.txt.
>> >
>> >  Required properties:
>> > -- compatible: "fsl,imx6q-pcie", "fsl,imx6sx-pcie", "fsl,imx6qp-pcie"
>> > +- compatible:
>> > +   - "fsl,imx6q-pcie"
>> > +   - "fsl,imx6sx-pcie",
>> > +   - "fsl,imx6qp-pcie"
>> > +   - "fsl,imx7d-pcie"
>> >  - reg: base address and length of the PCIe controller
>> >  - interrupts: A list of interrupt outputs of the controller. Must contain an
>> >    entry for each entry in the interrupt-names property.
>> > @@ -34,6 +38,13 @@ Additional required properties for imx6sx-pcie:
>> >  - clock names: Must include the following additional entries:
>> >     - "pcie_inbound_axi"
>> >
>> > +Additional required properties for imx7d-pcie:
>> > +- power-domains: Must be set to a phandle pointing to PCIE_PHY power domain
>>
>> This domain is just the PHY? Seems like this needs a separate PHY
>> driver.
>>
> No, it's called the PHY power domain, as that is probably the part that
> draws the most power, but the PCIe core also looses it's state when this
> domain is powered down. So it's probably the complete core that is
> inside this domain.

A shared domain doesn't mean the phy and core should be 1 node. It is
the separate reset and clock for the PHY that tell me they should be
separate. And I'm pretty sure the DW block and PHY are separate. If
the PHY registers were part of the same register range, then I'd say
they should be one.

>> > +- resets: Must contain phandles to PCIE related reset lines exposed by SRC IP block
>> > +- reset-names: Must contain the following entires:
>> > +          - "pciephy"
>>
>> And for this too.
>>
>> > +          - "apps"
>> > +
>> >  Example:
>> >
>> >     pcie@0x01000000 {
>>
>> [...]
>>
>> > @@ -251,6 +261,10 @@ static void imx6_pcie_assert_core_reset(struct imx6_pcie *imx6_pcie)
>> >     u32 val, gpr1, gpr12;
>> >
>> >     switch (imx6_pcie->variant) {
>> > +   case IMX7D:
>> > +           reset_control_assert(imx6_pcie->pciephy_reset);
>> > +           reset_control_assert(imx6_pcie->apps_reset);
>> > +           break;
>> >     case IMX6SX:
>> >             regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR12,
>> >                                IMX6SX_GPR12_PCIE_TEST_POWERDOWN,
>>
>> So the difference with i.MX7D is not really that it has a reset or not,
>> but some platforms use a reset driver and some do not. The latter should
>> be fixed.
>
> The resets on anything before i.MX7 are not in a separate reset driver,
> but are just some signals from the PCIe core wired into a syscon (IOMUX
> GPR) area. While we could invent a reset controller for those, I don't
> see how this would improve things. Especially as the reset on i.MX6
> seems to be some side-effect of the "power-down" signal of the core, so
> not strictly a reset.
>
> Also I don't see why we should change the binding for the driver with a
> long history of deployed DTs. That seems like a total waste of manpower.

We can debate whether or not we change existing platforms. Maybe that
doesn't make sense now. But best practices should be considered when
adding new bindings rather than just extending existing bindings. We
didn't split out PHYs at one time and now we generally do. I'm not so
concerned with just adding i.MX7D, but really the next chip (and the
next).

Rob

[toc] | [prev] | [next] | [standalone]

#1585523

Am Dienstag, den 21.02.2017, 10:38 -0600 schrieb Rob Herring:
> On Thu, Feb 16, 2017 at 3:12 AM, Lucas Stach <l.stach@pengutronix.de> wrote:
> > Am Mittwoch, den 15.02.2017, 11:17 -0600 schrieb Rob Herring:
> >> On Tue, Feb 07, 2017 at 07:50:27AM -0800, Andrey Smirnov wrote:
> >> > Add various bits of code needed to support i.MX7D variant of the IP.
> >> >
> >> > Cc: yurovsky@gmail.com
> >> > Cc: Lucas Stach <l.stach@pengutronix.de>
> >> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> >> > Cc: Rob Herring <robh+dt@kernel.org>
> >> > Cc: Mark Rutland <mark.rutland@arm.com>
> >> > Cc: Lee Jones <lee.jones@linaro.org>
> >> > Cc: Fabio Estevam <fabio.estevam@nxp.com>
> >> > Cc: linux-arm-kernel@lists.infradead.org
> >> > Cc: devicetree@vger.kernel.org
> >> > Cc: linux-kernel@vger.kernel.org
> >> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> >> > ---
> >> >  .../devicetree/bindings/pci/fsl,imx6q-pcie.txt     |  13 ++-
> >> >  drivers/pci/host/pci-imx6.c                        | 121 ++++++++++++++++-----
> >> >  include/linux/mfd/syscon/imx7-iomuxc-gpr.h         |   4 +
> >> >  3 files changed, 112 insertions(+), 26 deletions(-)
> >> >
> >> > diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> >> > index 83aeb1f..11db2ab 100644
> >> > --- a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> >> > +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt
> >> > @@ -4,7 +4,11 @@ This PCIe host controller is based on the Synopsis Designware PCIe IP
> >> >  and thus inherits all the common properties defined in designware-pcie.txt.
> >> >
> >> >  Required properties:
> >> > -- compatible: "fsl,imx6q-pcie", "fsl,imx6sx-pcie", "fsl,imx6qp-pcie"
> >> > +- compatible:
> >> > +   - "fsl,imx6q-pcie"
> >> > +   - "fsl,imx6sx-pcie",
> >> > +   - "fsl,imx6qp-pcie"
> >> > +   - "fsl,imx7d-pcie"
> >> >  - reg: base address and length of the PCIe controller
> >> >  - interrupts: A list of interrupt outputs of the controller. Must contain an
> >> >    entry for each entry in the interrupt-names property.
> >> > @@ -34,6 +38,13 @@ Additional required properties for imx6sx-pcie:
> >> >  - clock names: Must include the following additional entries:
> >> >     - "pcie_inbound_axi"
> >> >
> >> > +Additional required properties for imx7d-pcie:
> >> > +- power-domains: Must be set to a phandle pointing to PCIE_PHY power domain
> >>
> >> This domain is just the PHY? Seems like this needs a separate PHY
> >> driver.
> >>
> > No, it's called the PHY power domain, as that is probably the part that
> > draws the most power, but the PCIe core also looses it's state when this
> > domain is powered down. So it's probably the complete core that is
> > inside this domain.
> 
> A shared domain doesn't mean the phy and core should be 1 node. It is
> the separate reset and clock for the PHY that tell me they should be
> separate. And I'm pretty sure the DW block and PHY are separate. If
> the PHY registers were part of the same register range, then I'd say
> they should be one.

Then we are on the same page of _not_ splitting out the PHY. :)
The DW PCIe PHY has no separate register range on i.MX. In fact the PHY
registers are only accessible through a indirection register in the PCIe
host controller register range.

Regards,
Lucas

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web