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

[Preetham Chandru <pchandru-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

>-----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.