Re: [v2,3/3] dt-bindings: ata: ahci_tegra: Add tegra210 AHCI

Re: [v2,3/3] dt-bindings: ata: ahci_tegra: Add tegra210 AHCI

[Mikko Perttunen]

On 24.11.2016 09:43, PREETHAM RAMACHANDRA wrote:
> From: Preetham Chandru R <pchandru-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> Signed-off-by: Preetham Chandru R <pchandru-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  .../bindings/ata/nvidia,tegra124-ahci.txt          | 48 ++++++++++++++++------
>  1 file changed, 36 insertions(+), 12 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt b/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
> index 66c83c3..446214f 100644
> --- a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
> +++ b/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
> @@ -1,9 +1,9 @@
> -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.
> +- compatible : Must be one of:
> +  - Tegra124 : "nvidia,tegra124-ahci"
> +  - Tegra210 : "nvidia,tegra210-ahci"
>  - reg : Should contain 2 entries:
>    - AHCI register set (SATA BAR5)
>    - SATA register set

I think you added a new set in the first patch, AUX; you should add it 
here as well.

> @@ -13,8 +13,6 @@ Required properties :
>  - 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 +22,35 @@ 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
> +- For T210:
> +  - l0-hvddio-sata-supply : Defines the SATA HVDDIO regulator
> +  - l0-dvddio-sata-supply : Defines the SATA DVDDIO regulator
> +  - hvdd-pex-pll-e-supply : Defines the PEX PLL_E regulator
> +  - dvdd-sata-pll-supply  : Defines the SATA PLL regulator
> +  - hvdd-sata-supply      : Defines the SATA HVDD regulator
> +- nvidia,disable-features : Must include the following entries:
> +  - devslp
> +  - dipm
> +
> +Optional properties:
> +- clock-names :
> +  - cml1 :
> +    cml1 clock is required by phy so it is optional to define
> +    here as phy driver will be enabling this clock.

The current phy driver does not seem to be enabling this clock. For 
PCIE, the PCIE driver itself enables cml0. It might be true that the phy 
driver should be enabling this. Thierry?

> +  - pll_e :
> +    pll_e is the parent of cml1 clock so it is optional to define
> +    here as phy driver will be enabling this clock.
> +- nvidia,disable-features :
> +  - hipm
> +  - ncq
> +  - partial
> +  - slumber
>
>

Thanks,
Mikko.

Re: [v2,3/3] dt-bindings: ata: ahci_tegra: Add tegra210 AHCI

[Jon Hunter]

On 28/11/16 13:05, Mikko Perttunen wrote:
> On 24.11.2016 09:43, PREETHAM RAMACHANDRA wrote:
>> From: Preetham Chandru R <pchandru-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

I did not receive the original (please CC linux-tegra as well if you did
not originally), but there should be some description here.

>> Signed-off-by: Preetham Chandru R <pchandru-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>  .../bindings/ata/nvidia,tegra124-ahci.txt          | 48
>> ++++++++++++++++------
>>  1 file changed, 36 insertions(+), 12 deletions(-)
>>
>> diff --git
>> a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
>> b/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
>> index 66c83c3..446214f 100644
>> --- a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
>> +++ b/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
>> @@ -1,9 +1,9 @@
>> -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.
>> +- compatible : Must be one of:
>> +  - Tegra124 : "nvidia,tegra124-ahci"
>> +  - Tegra210 : "nvidia,tegra210-ahci"
>>  - reg : Should contain 2 entries:
>>    - AHCI register set (SATA BAR5)
>>    - SATA register set
> 
> I think you added a new set in the first patch, AUX; you should add it
> here as well.
> 
>> @@ -13,8 +13,6 @@ Required properties :
>>  - 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 +22,35 @@ 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
>> +- For T210:
>> +  - l0-hvddio-sata-supply : Defines the SATA HVDDIO regulator
>> +  - l0-dvddio-sata-supply : Defines the SATA DVDDIO regulator
>> +  - hvdd-pex-pll-e-supply : Defines the PEX PLL_E regulator
>> +  - dvdd-sata-pll-supply  : Defines the SATA PLL regulator
>> +  - hvdd-sata-supply      : Defines the SATA HVDD regulator
>> +- nvidia,disable-features : Must include the following entries:
>> +  - devslp
>> +  - dipm

My understanding is that the AHCI controller requires the SATA powergate
to be enabled. Now we have support for powergates via the genpd
framework we should add the 'power-domains' property for this device.

Do you know if there is any sequencing requirement with regard to
powering on the above rails and the powergate? If not we should check as
we should ensure that we have the proper sequencing.

Cheers
Jon

-- 
nvpublic

RE: [v2,3/3] dt-bindings: ata: ahci_tegra: Add tegra210 AHCI

[Preetham Chandru]

>-----Original Message-----
>From: Jonathan Hunter
>Sent: Monday, November 28, 2016 7:22 PM
>To: Mikko Perttunen; Preetham Chandru; tj@kernel.org;
>swarren@wwwdotorg.org; thierry.reding@gmail.com;
>preetham260@gmail.com
>Cc: Laxman Dewangan; linux-ide@vger.kernel.org; Venu Byravarasu; Pavan
>Kunapuli; linux-tegra@vger.kernel.org
>Subject: Re: [v2,3/3] dt-bindings: ata: ahci_tegra: Add tegra210 AHCI
>
>
>On 28/11/16 13:05, Mikko Perttunen wrote:
>> On 24.11.2016 09:43, PREETHAM RAMACHANDRA wrote:
>>> From: Preetham Chandru R <pchandru@nvidia.com>
>
>I did not receive the original (please CC linux-tegra as well if you did not
>originally), but there should be some description here.
>
>>> Signed-off-by: Preetham Chandru R <pchandru@nvidia.com>
>>> ---
>>>  .../bindings/ata/nvidia,tegra124-ahci.txt          | 48
>>> ++++++++++++++++------
>>>  1 file changed, 36 insertions(+), 12 deletions(-)
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
>>> b/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
>>> index 66c83c3..446214f 100644
>>> --- a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
>>> +++ b/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
>>> @@ -1,9 +1,9 @@
>>> -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.
>>> +- compatible : Must be one of:
>>> +  - Tegra124 : "nvidia,tegra124-ahci"
>>> +  - Tegra210 : "nvidia,tegra210-ahci"
>>>  - reg : Should contain 2 entries:
>>>    - AHCI register set (SATA BAR5)
>>>    - SATA register set
>>
>> I think you added a new set in the first patch, AUX; you should add it
>> here as well.
>>
>>> @@ -13,8 +13,6 @@ Required properties :
>>>  - 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 +22,35 @@ 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
>>> +- For T210:
>>> +  - l0-hvddio-sata-supply : Defines the SATA HVDDIO regulator
>>> +  - l0-dvddio-sata-supply : Defines the SATA DVDDIO regulator
>>> +  - hvdd-pex-pll-e-supply : Defines the PEX PLL_E regulator
>>> +  - dvdd-sata-pll-supply  : Defines the SATA PLL regulator
>>> +  - hvdd-sata-supply      : Defines the SATA HVDD regulator
>>> +- nvidia,disable-features : Must include the following entries:
>>> +  - devslp
>>> +  - dipm
>
>My understanding is that the AHCI controller requires the SATA powergate
>to be enabled. Now we have support for powergates via the genpd
>framework we should add the 'power-domains' property for this device.
>
>Do you know if there is any sequencing requirement with regard to powering
>on the above rails and the powergate? If not we should check as we should
>ensure that we have the proper sequencing.
>

All the regulators mentioned under T210 section are related to phy and they need to be enabled first.  
I will add the power-domain support for AHCI in a different patch. 

>Cheers
>Jon
>
>--
>nvpublic

RE: [v2,3/3] dt-bindings: ata: ahci_tegra: Add tegra210 AHCI

[Preetham Chandru]

>-----Original Message-----
>From: Mikko Perttunen
>Sent: Monday, November 28, 2016 6:35 PM
>To: Preetham Chandru; tj@kernel.org; swarren@wwwdotorg.org;
>thierry.reding@gmail.com; preetham260@gmail.com
>Cc: Laxman Dewangan; linux-ide@vger.kernel.org; Venu Byravarasu; Pavan
>Kunapuli; linux-tegra@vger.kernel.org
>Subject: Re: [v2,3/3] dt-bindings: ata: ahci_tegra: Add tegra210 AHCI
>
>On 24.11.2016 09:43, PREETHAM RAMACHANDRA wrote:
>> From: Preetham Chandru R <pchandru@nvidia.com>
>>
>> Signed-off-by: Preetham Chandru R <pchandru@nvidia.com>
>> ---
>>  .../bindings/ata/nvidia,tegra124-ahci.txt          | 48 ++++++++++++++++----
>--
>>  1 file changed, 36 insertions(+), 12 deletions(-)
>>
>> diff --git
>> a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
>> b/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
>> index 66c83c3..446214f 100644
>> --- a/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
>> +++ b/Documentation/devicetree/bindings/ata/nvidia,tegra124-ahci.txt
>> @@ -1,9 +1,9 @@
>> -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.
>> +- compatible : Must be one of:
>> +  - Tegra124 : "nvidia,tegra124-ahci"
>> +  - Tegra210 : "nvidia,tegra210-ahci"
>>  - reg : Should contain 2 entries:
>>    - AHCI register set (SATA BAR5)
>>    - SATA register set
>
>I think you added a new set in the first patch, AUX; you should add it here as
>well.
>

ok

>> @@ -13,8 +13,6 @@ Required properties :
>>  - 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 +22,35 @@ 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
>> +- For T210:
>> +  - l0-hvddio-sata-supply : Defines the SATA HVDDIO regulator
>> +  - l0-dvddio-sata-supply : Defines the SATA DVDDIO regulator
>> +  - hvdd-pex-pll-e-supply : Defines the PEX PLL_E regulator
>> +  - dvdd-sata-pll-supply  : Defines the SATA PLL regulator
>> +  - hvdd-sata-supply      : Defines the SATA HVDD regulator
>> +- nvidia,disable-features : Must include the following entries:
>> +  - devslp
>> +  - dipm
>> +
>> +Optional properties:
>> +- clock-names :
>> +  - cml1 :
>> +    cml1 clock is required by phy so it is optional to define
>> +    here as phy driver will be enabling this clock.
>
>The current phy driver does not seem to be enabling this clock. For PCIE, the
>PCIE driver itself enables cml0. It might be true that the phy driver should be
>enabling this. Thierry?
>

Yes, this should really be done in phy driver. I cross checked and I see phy driver is not enabling cml1 clk.
I will check with Thierry regarding this.  Depending on where we decide to enable cml1 clock  I will either
remove the optional properties or move it must include entries.

>> +  - pll_e :
>> +    pll_e is the parent of cml1 clock so it is optional to define
>> +    here as phy driver will be enabling this clock.
>> +- nvidia,disable-features :
>> +  - hipm
>> +  - ncq
>> +  - partial
>> +  - slumber
>>
>>
>
>Thanks,
>Mikko.

Thanks,
Preetham.