[V7,1/7] dt-bindings: ahci-tegra: add binding documentation

Message ID 1518456406-21564-2-git-send-email-pchandru@nvidia.com
State Changes Requested
Headers show
Series
  • Refactor and add AHCI support for tegra210
Related show

Commit Message

Preetham Chandru Ramchandra Feb. 12, 2018, 5:26 p.m.
From: Preetham Ramchandra <pchandru@nvidia.com>

This adds bindings documentation for the
AHCI controller on Tegra210

Signed-off-by: Preetham Chandru R <pchandru@nvidia.com>
---
v7:
* For Aux register set drop the Tegra210 since this register
  set also works on Tegra124
* rephrase the sentence for cml1 clock
* change the commit subject to include ahci-tegra
* drop pll_e since CCF handles it automatically as
  CML1 is a child clock of it.
v4:
* changed the commit message
* changed 'sata-cold' reset to mandatory for t210 and t124
* Removed the regulators for T210 since these regulators
  will be enabled in phy driver.
v3:
* Add AUX register.
v2:
* change cml1, pll_e and phy regulators as optional
  for T210.
---
 .../bindings/ata/nvidia,tegra124-ahci.txt          | 35 ++++++++++++++--------
 1 file changed, 22 insertions(+), 13 deletions(-)

Comments

Rob Herring Feb. 19, 2018, 2:46 a.m. | #1
On Mon, Feb 12, 2018 at 10:56:40PM +0530, Preetham Chandru Ramchandra wrote:
> From: Preetham Ramchandra <pchandru@nvidia.com>
> 
> This adds bindings documentation for the
> AHCI controller on Tegra210
> 
> Signed-off-by: Preetham Chandru R <pchandru@nvidia.com>
> ---
> v7:
> * For Aux register set drop the Tegra210 since this register
>   set also works on Tegra124
> * rephrase the sentence for cml1 clock
> * change the commit subject to include ahci-tegra
> * drop pll_e since CCF handles it automatically as
>   CML1 is a child clock of it.
> v4:
> * changed the commit message
> * changed 'sata-cold' reset to mandatory for t210 and t124
> * Removed the regulators for T210 since these regulators
>   will be enabled in phy driver.
> v3:
> * Add AUX register.
> v2:
> * change cml1, pll_e and phy regulators as optional
>   for T210.
> ---
>  .../bindings/ata/nvidia,tegra124-ahci.txt          | 35 ++++++++++++++--------
>  1 file changed, 22 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt b/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
> index 66c83c3e8915..0f4520a00716 100644
> --- a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
> +++ b/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
> @@ -1,20 +1,19 @@
> -Tegra124 SoC SATA AHCI controller
> +Tegra SoC SATA AHCI controller
>  
>  Required properties :
> -- compatible : For Tegra124, must contain "nvidia,tegra124-ahci".  Otherwise,
> -  must contain '"nvidia,<chip>-ahci", "nvidia,tegra124-ahci"', where <chip>
> -  is tegra132.
> -- reg : Should contain 2 entries:
> +- compatible : Must be one of:
> +  - Tegra124 : "nvidia,tegra124-ahci"
> +  - Tegra210 : "nvidia,tegra210-ahci"

Are you dropping T132?

> +- reg : Should contain 3 entries:

You can't just add more entries to existing compatibles. Does this apply 
to T124?

>    - AHCI register set (SATA BAR5)
>    - SATA register set
> +  - AUX register set
>  - interrupts : Defines the interrupt used by SATA
>  - clocks : Must contain an entry for each entry in clock-names.
>    See ../clocks/clock-bindings.txt for details.
>  - clock-names : Must include the following entries:
>    - sata
>    - sata-oob
> -  - cml1
> -  - pll_e
>  - resets : Must contain an entry for each entry in reset-names.
>    See ../reset/reset.txt for details.
>  - reset-names : Must include the following entries:
> @@ -24,9 +23,19 @@ Required properties :
>  - phys : Must contain an entry for each entry in phy-names.
>    See ../phy/phy-bindings.txt for details.
>  - phy-names : Must include the following entries:
> -  - sata-phy : XUSB PADCTL SATA PHY
> -- hvdd-supply : Defines the SATA HVDD regulator
> -- vddio-supply : Defines the SATA VDDIO regulator
> -- avdd-supply : Defines the SATA AVDD regulator
> -- target-5v-supply : Defines the SATA 5V power regulator
> -- target-12v-supply : Defines the SATA 12V power regulator
> +  - For T124:
> +    - sata-phy : XUSB PADCTL SATA PHY
> +  - For T210:
> +    - sata-0

-names is kind of pointless for a single entry. Differing names is even 
more pointless.

> +- For T124:
> +  - hvdd-supply : Defines the SATA HVDD regulator
> +  - vddio-supply : Defines the SATA VDDIO regulator
> +  - avdd-supply : Defines the SATA AVDD regulator
> +  - target-5v-supply : Defines the SATA 5V power regulator
> +  - target-12v-supply : Defines the SATA 12V power regulator

No supplies on T210? The last 2 sound like board level supplies to the 
drive.

> +
> +Optional properties:
> +- clock-names :
> +  - cml1 :
> +    cml1 clock should be defined here if the PHY driver
> +    doesn't manage them. If it does, they should not be.
> -- 
> 2.7.4
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Thierry Reding Feb. 19, 2018, 2:14 p.m. | #2
On Sun, Feb 18, 2018 at 08:46:35PM -0600, Rob Herring wrote:
> On Mon, Feb 12, 2018 at 10:56:40PM +0530, Preetham Chandru Ramchandra wrote:
> > From: Preetham Ramchandra <pchandru@nvidia.com>
> > 
> > This adds bindings documentation for the
> > AHCI controller on Tegra210
> > 
> > Signed-off-by: Preetham Chandru R <pchandru@nvidia.com>
> > ---
> > v7:
> > * For Aux register set drop the Tegra210 since this register
> >   set also works on Tegra124
> > * rephrase the sentence for cml1 clock
> > * change the commit subject to include ahci-tegra
> > * drop pll_e since CCF handles it automatically as
> >   CML1 is a child clock of it.
> > v4:
> > * changed the commit message
> > * changed 'sata-cold' reset to mandatory for t210 and t124
> > * Removed the regulators for T210 since these regulators
> >   will be enabled in phy driver.
> > v3:
> > * Add AUX register.
> > v2:
> > * change cml1, pll_e and phy regulators as optional
> >   for T210.
> > ---
> >  .../bindings/ata/nvidia,tegra124-ahci.txt          | 35 ++++++++++++++--------
> >  1 file changed, 22 insertions(+), 13 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt b/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
> > index 66c83c3e8915..0f4520a00716 100644
> > --- a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
> > +++ b/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
> > @@ -1,20 +1,19 @@
> > -Tegra124 SoC SATA AHCI controller
> > +Tegra SoC SATA AHCI controller
> >  
> >  Required properties :
> > -- compatible : For Tegra124, must contain "nvidia,tegra124-ahci".  Otherwise,
> > -  must contain '"nvidia,<chip>-ahci", "nvidia,tegra124-ahci"', where <chip>
> > -  is tegra132.
> > -- reg : Should contain 2 entries:
> > +- compatible : Must be one of:
> > +  - Tegra124 : "nvidia,tegra124-ahci"
> > +  - Tegra210 : "nvidia,tegra210-ahci"
> 
> Are you dropping T132?

Tegra132 is identical to Tegra124 except for the CPU cores. That said,
we've erred on the side of caution in the past and required that the
compatible properties for Tegra132 contain both "nvidia,tegra132-*" and
"nvidia,tegra124-*". The reason was that we'd get support from drivers
that support Tegra124 automatically while at the same time having a chip
specific compatible string that would allow us to add Tegra132 specific
quirks should there be any.

So I think it'd be best if AHCI followed that and we add an entry for
Tegra132 here:

  - Tegra132: "nvidia,tegra132-ahci", "nvidia,tegra124-ahci"

Thierry
Thierry Reding Feb. 19, 2018, 2:24 p.m. | #3
On Sun, Feb 18, 2018 at 08:46:35PM -0600, Rob Herring wrote:
> On Mon, Feb 12, 2018 at 10:56:40PM +0530, Preetham Chandru Ramchandra wrote:
> > From: Preetham Ramchandra <pchandru@nvidia.com>
> > 
> > This adds bindings documentation for the
> > AHCI controller on Tegra210
> > 
> > Signed-off-by: Preetham Chandru R <pchandru@nvidia.com>
> > ---
> > v7:
> > * For Aux register set drop the Tegra210 since this register
> >   set also works on Tegra124
> > * rephrase the sentence for cml1 clock
> > * change the commit subject to include ahci-tegra
> > * drop pll_e since CCF handles it automatically as
> >   CML1 is a child clock of it.
> > v4:
> > * changed the commit message
> > * changed 'sata-cold' reset to mandatory for t210 and t124
> > * Removed the regulators for T210 since these regulators
> >   will be enabled in phy driver.
> > v3:
> > * Add AUX register.
> > v2:
> > * change cml1, pll_e and phy regulators as optional
> >   for T210.
> > ---
> >  .../bindings/ata/nvidia,tegra124-ahci.txt          | 35 ++++++++++++++--------
> >  1 file changed, 22 insertions(+), 13 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt b/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
> > index 66c83c3e8915..0f4520a00716 100644
> > --- a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
> > +++ b/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
> > @@ -1,20 +1,19 @@
> > -Tegra124 SoC SATA AHCI controller
> > +Tegra SoC SATA AHCI controller
> >  
> >  Required properties :
> > -- compatible : For Tegra124, must contain "nvidia,tegra124-ahci".  Otherwise,
> > -  must contain '"nvidia,<chip>-ahci", "nvidia,tegra124-ahci"', where <chip>
> > -  is tegra132.
> > -- reg : Should contain 2 entries:
> > +- compatible : Must be one of:
> > +  - Tegra124 : "nvidia,tegra124-ahci"
> > +  - Tegra210 : "nvidia,tegra210-ahci"
> 
> Are you dropping T132?
> 
> > +- reg : Should contain 3 entries:
> 
> You can't just add more entries to existing compatibles. Does this apply 
> to T124?

I'd consider this a bug in existing DTSs. The SATA AUX registers exist
as far back as Tegra30. The reason why they were never included in the
DTS in because the driver never programmed those registers.

However, the driver change in patch 5/7 which uses this has a comment
that AUX registers are optional, so perhaps we can just add that fact to
the bindings as well.

Thierry
Thierry Reding Feb. 19, 2018, 2:27 p.m. | #4
On Mon, Feb 12, 2018 at 10:56:40PM +0530, Preetham Chandru Ramchandra wrote:
> From: Preetham Ramchandra <pchandru@nvidia.com>
> 
> This adds bindings documentation for the
> AHCI controller on Tegra210

Please avoid lines that are too short. They shouldn't exceed 72
characters, but you're supposed to make fully use of those. Also the
above is a sentence, so should end with a '.'.

> diff --git a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt b/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
> index 66c83c3e8915..0f4520a00716 100644
> --- a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
> +++ b/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
> @@ -1,20 +1,19 @@
> -Tegra124 SoC SATA AHCI controller
> +Tegra SoC SATA AHCI controller
>  
>  Required properties :
> -- compatible : For Tegra124, must contain "nvidia,tegra124-ahci".  Otherwise,
> -  must contain '"nvidia,<chip>-ahci", "nvidia,tegra124-ahci"', where <chip>
> -  is tegra132.
> -- reg : Should contain 2 entries:
> +- compatible : Must be one of:
> +  - Tegra124 : "nvidia,tegra124-ahci"
> +  - Tegra210 : "nvidia,tegra210-ahci"
> +- reg : Should contain 3 entries:
>    - AHCI register set (SATA BAR5)
>    - SATA register set
> +  - AUX register set
>  - interrupts : Defines the interrupt used by SATA
>  - clocks : Must contain an entry for each entry in clock-names.
>    See ../clocks/clock-bindings.txt for details.
>  - clock-names : Must include the following entries:
>    - sata
>    - sata-oob
> -  - cml1
> -  - pll_e
>  - resets : Must contain an entry for each entry in reset-names.
>    See ../reset/reset.txt for details.
>  - reset-names : Must include the following entries:
> @@ -24,9 +23,19 @@ Required properties :
>  - phys : Must contain an entry for each entry in phy-names.
>    See ../phy/phy-bindings.txt for details.
>  - phy-names : Must include the following entries:
> -  - sata-phy : XUSB PADCTL SATA PHY
> -- hvdd-supply : Defines the SATA HVDD regulator
> -- vddio-supply : Defines the SATA VDDIO regulator
> -- avdd-supply : Defines the SATA AVDD regulator
> -- target-5v-supply : Defines the SATA 5V power regulator
> -- target-12v-supply : Defines the SATA 12V power regulator
> +  - For T124:
> +    - sata-phy : XUSB PADCTL SATA PHY
> +  - For T210:
> +    - sata-0
> +- For T124:

Can we please use TegraXYZ here? I find it useful to be consistent here
because those will show up if I grep for "tegra", whereas TXYZ won't.

Thierry
Thierry Reding Feb. 19, 2018, 2:28 p.m. | #5
On Mon, Feb 12, 2018 at 10:56:40PM +0530, Preetham Chandru Ramchandra wrote:
> From: Preetham Ramchandra <pchandru@nvidia.com>
> 
> This adds bindings documentation for the
> AHCI controller on Tegra210
> 
> Signed-off-by: Preetham Chandru R <pchandru@nvidia.com>

Oh, and you might want to mention Tegra210 in the patch subject.

Thierry

Patch

diff --git a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt b/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
index 66c83c3e8915..0f4520a00716 100644
--- a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
+++ b/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
@@ -1,20 +1,19 @@ 
-Tegra124 SoC SATA AHCI controller
+Tegra SoC SATA AHCI controller
 
 Required properties :
-- compatible : For Tegra124, must contain "nvidia,tegra124-ahci".  Otherwise,
-  must contain '"nvidia,<chip>-ahci", "nvidia,tegra124-ahci"', where <chip>
-  is tegra132.
-- reg : Should contain 2 entries:
+- compatible : Must be one of:
+  - Tegra124 : "nvidia,tegra124-ahci"
+  - Tegra210 : "nvidia,tegra210-ahci"
+- reg : Should contain 3 entries:
   - AHCI register set (SATA BAR5)
   - SATA register set
+  - AUX register set
 - interrupts : Defines the interrupt used by SATA
 - clocks : Must contain an entry for each entry in clock-names.
   See ../clocks/clock-bindings.txt for details.
 - clock-names : Must include the following entries:
   - sata
   - sata-oob
-  - cml1
-  - pll_e
 - resets : Must contain an entry for each entry in reset-names.
   See ../reset/reset.txt for details.
 - reset-names : Must include the following entries:
@@ -24,9 +23,19 @@  Required properties :
 - phys : Must contain an entry for each entry in phy-names.
   See ../phy/phy-bindings.txt for details.
 - phy-names : Must include the following entries:
-  - sata-phy : XUSB PADCTL SATA PHY
-- hvdd-supply : Defines the SATA HVDD regulator
-- vddio-supply : Defines the SATA VDDIO regulator
-- avdd-supply : Defines the SATA AVDD regulator
-- target-5v-supply : Defines the SATA 5V power regulator
-- target-12v-supply : Defines the SATA 12V power regulator
+  - For T124:
+    - sata-phy : XUSB PADCTL SATA PHY
+  - For T210:
+    - sata-0
+- For T124:
+  - hvdd-supply : Defines the SATA HVDD regulator
+  - vddio-supply : Defines the SATA VDDIO regulator
+  - avdd-supply : Defines the SATA AVDD regulator
+  - target-5v-supply : Defines the SATA 5V power regulator
+  - target-12v-supply : Defines the SATA 12V power regulator
+
+Optional properties:
+- clock-names :
+  - cml1 :
+    cml1 clock should be defined here if the PHY driver
+    doesn't manage them. If it does, they should not be.