Patchwork [RFC,15/15] arm: dts: dra7: add sata node

login
register
mail settings
Submitter Roger Quadros
Date Sept. 19, 2013, 1:24 p.m.
Message ID <1379597059-15405-1-git-send-email-rogerq@ti.com>
Download mbox | patch
Permalink /patch/275964/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Roger Quadros - Sept. 19, 2013, 1:24 p.m.
From: Balaji T K <balajitk@ti.com>

Add support for sata controller.

[Roger Q] Clean up.

CC: Benoit Cousson <bcousson@baylibre.com>
Signed-off-by: Balaji T K <balajitk@ti.com>
Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 arch/arm/boot/dts/dra7.dtsi |   49 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 49 insertions(+), 0 deletions(-)
Sergei Shtylyov - Sept. 19, 2013, 2:11 p.m.
Hello.

On 09/19/2013 05:24 PM, Roger Quadros wrote:

> From: Balaji T K <balajitk@ti.com>

> Add support for sata controller.

> [Roger Q] Clean up.

> CC: Benoit Cousson <bcousson@baylibre.com>
> Signed-off-by: Balaji T K <balajitk@ti.com>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>   arch/arm/boot/dts/dra7.dtsi |   49 +++++++++++++++++++++++++++++++++++++++++++
>   1 files changed, 49 insertions(+), 0 deletions(-)

> diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
> index ce9a0f0..545545d 100644
> --- a/arch/arm/boot/dts/dra7.dtsi
> +++ b/arch/arm/boot/dts/dra7.dtsi
> @@ -426,6 +426,55 @@
>   			dma-names = "tx", "rx";
>   		};
>
> +		omap_control_sata: control-phy@4a002374 {
> +			compatible = "ti,control-phy-pipe3";
> +			reg = <0x4a002374 0x4>;
> +			reg-names = "power";
> +			clocks = <&sys_clkin1>;
> +			clock-names = "sysclk";
> +		};
> +
> +		ocp2scp3: ocp2scp3@4a090000 {
> +			compatible = "ti,omap-ocp2scp";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;
> +			ti,hwmods = "ocp2scp3";
> +			reg = <0x4a090000 0x1f>; /* ocp2scp3 */
> +			reg-names = "ocp2scp3";
> +			sata_phy: sataphy@4A096000 {

    It's better to name the PHY nodes uniformly after already standard 
"ethernet-phy" and your "control-phy".

> +				compatible = "ti,phy-pipe3-sata";
> +				reg = <0x4A096000 0x80>, /* phy_rx */
> +				      <0x4A096400 0x64>, /* phy_tx */
> +				      <0x4A096800 0x40>; /* pll_ctrl */
> +				reg-names = "phy_rx", "phy_tx", "pll_ctrl";
> +				ctrl-module = <&omap_control_sata>;
> +				clocks = <&sata_ref_clk>,
> +					 <&sys_clkin1>;
> +				clock-names = "optclk", "sysclk";
> +				#phy-cells = <0>;
> +			};
> +		};
> +
> +		sata: sata@4a141100 {
> +			compatible = "ti,sata";
> +			ti,hwmods = "sata";
> +			reg = <0x4a141100 0x7>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;
> +			dwc-ahci@4a140000 {

    Hm, ePAPR spec. [1] says that "the name of a node should be somewhat 
generic, reflecting the function of the device and not its precise programming 
model", so it looks like the name should be "sata" as well. I'm a bit at a 
loss here, not sure why you had to use the nested device nodes.

> +				  compatible = "snps,dwc-ahci";
> +				  reg = <0x4a140000 0x1100>;
> +				  interrupts = <0 54 0x4>;
> +				  phys = <&sata_phy>;

    Hm, it's the third PHY related generic property I'm encountering. First, 
there was "phy-handle", then "phy", now "phys"... Seems like a bit too much. :-)

> +				  phy-names = "sata-phy";
> +				  clocks = <&sata_ref_clk>;
> +				  clock-names = "optclk";
> +			};
> +		};

[1] http://www.power.org/resources/downloads/Power_ePAPR_APPROVED_v1.0.pdf

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Roger Quadros - Sept. 20, 2013, 10:19 a.m.
Hi,

On 09/19/2013 05:11 PM, Sergei Shtylyov wrote:
> Hello.
> 
> On 09/19/2013 05:24 PM, Roger Quadros wrote:
> 
>> From: Balaji T K <balajitk@ti.com>
> 
>> Add support for sata controller.
> 
>> [Roger Q] Clean up.
> 
>> CC: Benoit Cousson <bcousson@baylibre.com>
>> Signed-off-by: Balaji T K <balajitk@ti.com>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>>   arch/arm/boot/dts/dra7.dtsi |   49 +++++++++++++++++++++++++++++++++++++++++++
>>   1 files changed, 49 insertions(+), 0 deletions(-)
> 
>> diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
>> index ce9a0f0..545545d 100644
>> --- a/arch/arm/boot/dts/dra7.dtsi
>> +++ b/arch/arm/boot/dts/dra7.dtsi
>> @@ -426,6 +426,55 @@
>>               dma-names = "tx", "rx";
>>           };
>>
>> +        omap_control_sata: control-phy@4a002374 {
>> +            compatible = "ti,control-phy-pipe3";
>> +            reg = <0x4a002374 0x4>;
>> +            reg-names = "power";
>> +            clocks = <&sys_clkin1>;
>> +            clock-names = "sysclk";
>> +        };
>> +
>> +        ocp2scp3: ocp2scp3@4a090000 {
>> +            compatible = "ti,omap-ocp2scp";
>> +            #address-cells = <1>;
>> +            #size-cells = <1>;
>> +            ranges;
>> +            ti,hwmods = "ocp2scp3";
>> +            reg = <0x4a090000 0x1f>; /* ocp2scp3 */
>> +            reg-names = "ocp2scp3";
>> +            sata_phy: sataphy@4A096000 {
> 
>    It's better to name the PHY nodes uniformly after already standard "ethernet-phy" and your "control-phy".

OK. will fix it to sata-phy.

> 
>> +                compatible = "ti,phy-pipe3-sata";
>> +                reg = <0x4A096000 0x80>, /* phy_rx */
>> +                      <0x4A096400 0x64>, /* phy_tx */
>> +                      <0x4A096800 0x40>; /* pll_ctrl */
>> +                reg-names = "phy_rx", "phy_tx", "pll_ctrl";
>> +                ctrl-module = <&omap_control_sata>;
>> +                clocks = <&sata_ref_clk>,
>> +                     <&sys_clkin1>;
>> +                clock-names = "optclk", "sysclk";
>> +                #phy-cells = <0>;
>> +            };
>> +        };
>> +
>> +        sata: sata@4a141100 {
>> +            compatible = "ti,sata";
>> +            ti,hwmods = "sata";
>> +            reg = <0x4a141100 0x7>;
>> +            #address-cells = <1>;
>> +            #size-cells = <1>;
>> +            ranges;
>> +            dwc-ahci@4a140000 {
> 
>    Hm, ePAPR spec. [1] says that "the name of a node should be somewhat generic, reflecting the function of the device and not its precise programming model", so it looks like the name should be "sata" as well. I'm a bit at a loss here, not sure why you had to use the nested device nodes.
> 

ok. will fix it to sata.
I've nested it because the wrapper registers are not part of the AHCI sata controller.
They are TI specific registers for power management. 
Similar setup is on the USB controller. Please see omap_dwc3 node.

But if you have better idea, please let me know.

>> +                  compatible = "snps,dwc-ahci";
>> +                  reg = <0x4a140000 0x1100>;
>> +                  interrupts = <0 54 0x4>;
>> +                  phys = <&sata_phy>;
> 
>    Hm, it's the third PHY related generic property I'm encountering. First, there was "phy-handle", then "phy", now "phys"... Seems like a bit too much. :-)

I'm afraid but this is how the designers have made it.

1) control-phy-pipe3 is that part of the PHY which sits in control module space and is different
from the sata-phy space and hence needs a different node. If it were to me, I would just put this
resource in sata-phy node, but there was a discussion about this earlier to do it otherwise [1].

2) sata-phy (sataphy) is the actual SATA PHY device.

3) phys is just a reference to the sata_phy and is used via the generic PHY framework.
It is upto the sata driver to power up/down the phy.

> 
>> +                  phy-names = "sata-phy";
>> +                  clocks = <&sata_ref_clk>;
>> +                  clock-names = "optclk";
>> +            };
>> +        };
> 
> [1] http://www.power.org/resources/downloads/Power_ePAPR_APPROVED_v1.0.pdf
> 

cheers,
-roger

[1] - https://lkml.org/lkml/2012/9/10/399
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov - Sept. 22, 2013, 6:45 p.m.
On 09/20/2013 02:19 PM, Roger Quadros wrote:

>>> From: Balaji T K <balajitk@ti.com>

>>> Add support for sata controller.

>>> [Roger Q] Clean up.

>>> CC: Benoit Cousson <bcousson@baylibre.com>
>>> Signed-off-by: Balaji T K <balajitk@ti.com>
>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>> ---
>>>    arch/arm/boot/dts/dra7.dtsi |   49 +++++++++++++++++++++++++++++++++++++++++++
>>>    1 files changed, 49 insertions(+), 0 deletions(-)

>>> diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
>>> index ce9a0f0..545545d 100644
>>> --- a/arch/arm/boot/dts/dra7.dtsi
>>> +++ b/arch/arm/boot/dts/dra7.dtsi
>>> @@ -426,6 +426,55 @@
[...]

>>> +        sata: sata@4a141100 {
>>> +            compatible = "ti,sata";
>>> +            ti,hwmods = "sata";
>>> +            reg = <0x4a141100 0x7>;

    Not 0x8 BTW?

>>> +            #address-cells = <1>;
>>> +            #size-cells = <1>;
>>> +            ranges;
>>> +            dwc-ahci@4a140000 {

>>     Hm, ePAPR spec. [1] says that "the name of a node should be somewhat generic, reflecting the function of the device and not its precise programming model", so it looks like the name should be "sata" as well. I'm a bit at a loss here, not sure why you had to use the nested device nodes.

> ok. will fix it to sata.
> I've nested it because the wrapper registers are not part of the AHCI sata controller.
> They are TI specific registers for power management.
> Similar setup is on the USB controller. Please see omap_dwc3 node.

> But if you have better idea, please let me know.

     Don't know, it seems to me that you're over-complicating it by using the 
nested nodes. You could just have AHCI regs as a first tuple of the "regs" 
prop, and PM regs as a second tuple.

>>> +                  compatible = "snps,dwc-ahci";
>>> +                  reg = <0x4a140000 0x1100>;
>>> +                  interrupts = <0 54 0x4>;
>>> +                  phys = <&sata_phy>;

>>     Hm, it's the third PHY related generic property I'm encountering. First, there was "phy-handle", then "phy", now "phys"... Seems like a bit too much. :-)

> I'm afraid but this is how the designers have made it.

> 1) control-phy-pipe3 is that part of the PHY which sits in control module space and is different
> from the sata-phy space and hence needs a different node. If it were to me, I would just put this
> resource in sata-phy node, but there was a discussion about this earlier to do it otherwise [1].

> 2) sata-phy (sataphy) is the actual SATA PHY device.

> 3) phys is just a reference to the sata_phy and is used via the generic PHY framework.
> It is upto the sata driver to power up/down the phy.

    I understand that it's a reference but why have 3 variants of such phandle 
containing prop? Is it really possible for a device to have multiple PHYs? 
Well, remembering our customer's USB, it's indeed possible, however, there 2 
PHYs out of 3 are not software controllable...

>>> +                  phy-names = "sata-phy";
>>> +                  clocks = <&sata_ref_clk>;
>>> +                  clock-names = "optclk";
>>> +            };
>>> +        };

>> [1] http://www.power.org/resources/downloads/Power_ePAPR_APPROVED_v1.0.pdf

> cheers,
> -roger

> [1] - https://lkml.org/lkml/2012/9/10/399

WBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Roger Quadros - Sept. 23, 2013, 8:24 a.m.
On 09/22/2013 09:45 PM, Sergei Shtylyov wrote:
> On 09/20/2013 02:19 PM, Roger Quadros wrote:
> 
>>>> From: Balaji T K <balajitk@ti.com>
> 
>>>> Add support for sata controller.
> 
>>>> [Roger Q] Clean up.
> 
>>>> CC: Benoit Cousson <bcousson@baylibre.com>
>>>> Signed-off-by: Balaji T K <balajitk@ti.com>
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>> ---
>>>>    arch/arm/boot/dts/dra7.dtsi |   49 +++++++++++++++++++++++++++++++++++++++++++
>>>>    1 files changed, 49 insertions(+), 0 deletions(-)
> 
>>>> diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
>>>> index ce9a0f0..545545d 100644
>>>> --- a/arch/arm/boot/dts/dra7.dtsi
>>>> +++ b/arch/arm/boot/dts/dra7.dtsi
>>>> @@ -426,6 +426,55 @@
> [...]
> 
>>>> +        sata: sata@4a141100 {
>>>> +            compatible = "ti,sata";
>>>> +            ti,hwmods = "sata";
>>>> +            reg = <0x4a141100 0x7>;
> 
>    Not 0x8 BTW?

You are right. Two 32 bit registers are used.
However, in the instance summary, the reference manual says 256 bytes.
So I think we should use 0x100.

> 
>>>> +            #address-cells = <1>;
>>>> +            #size-cells = <1>;
>>>> +            ranges;
>>>> +            dwc-ahci@4a140000 {
> 
>>>     Hm, ePAPR spec. [1] says that "the name of a node should be somewhat generic, reflecting the function of the device and not its precise programming model", so it looks like the name should be "sata" as well. I'm a bit at a loss here, not sure why you had to use the nested device nodes.
> 
>> ok. will fix it to sata.
>> I've nested it because the wrapper registers are not part of the AHCI sata controller.
>> They are TI specific registers for power management.
>> Similar setup is on the USB controller. Please see omap_dwc3 node.
> 
>> But if you have better idea, please let me know.
> 
>     Don't know, it seems to me that you're over-complicating it by using the nested nodes. You could just have AHCI regs as a first tuple of the "regs" prop, and PM regs as a second tuple.
> 

Yes that is possible, and in fact it was that way in Balaji's original code.
However in that case, won't TI specific handling be need to be done in the ahci_platform driver?

As of now, that is limited to using pm_runtime to enable/disable the hardware module so it is generic enough.
However in the future it would mean reading/writing to the TI wrapper register. If this can be done in ahci_platform
driver then I don't see any issue and can combine the 2 nodes.

>>>> +                  compatible = "snps,dwc-ahci";
>>>> +                  reg = <0x4a140000 0x1100>;
>>>> +                  interrupts = <0 54 0x4>;
>>>> +                  phys = <&sata_phy>;
> 
>>>     Hm, it's the third PHY related generic property I'm encountering. First, there was "phy-handle", then "phy", now "phys"... Seems like a bit too much. :-)
> 
>> I'm afraid but this is how the designers have made it.
> 
>> 1) control-phy-pipe3 is that part of the PHY which sits in control module space and is different
>> from the sata-phy space and hence needs a different node. If it were to me, I would just put this
>> resource in sata-phy node, but there was a discussion about this earlier to do it otherwise [1].
> 
>> 2) sata-phy (sataphy) is the actual SATA PHY device.
> 
>> 3) phys is just a reference to the sata_phy and is used via the generic PHY framework.
>> It is upto the sata driver to power up/down the phy.
> 
>    I understand that it's a reference but why have 3 variants of such phandle containing prop? Is it really possible for a device to have multiple PHYs? Well, remembering our customer's USB, it's indeed possible, however, there 2 PHYs out of 3 are not software controllable...
> 

If I understand right, are you asking that we don't need "phy-names" property if there can be only one PHY?
I too think it is redundant. Maybe the PHY framework should be modified to allow users to use phy_get() whithout
any phy name string. In such case it should return the first PHY. 
Kishon, any thoughts?

>>>> +                  phy-names = "sata-phy";
>>>> +                  clocks = <&sata_ref_clk>;
>>>> +                  clock-names = "optclk";
>>>> +            };
>>>> +        };
> 
>>> [1] http://www.power.org/resources/downloads/Power_ePAPR_APPROVED_v1.0.pdf
> 

cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
index ce9a0f0..545545d 100644
--- a/arch/arm/boot/dts/dra7.dtsi
+++ b/arch/arm/boot/dts/dra7.dtsi
@@ -426,6 +426,55 @@ 
 			dma-names = "tx", "rx";
 		};
 
+		omap_control_sata: control-phy@4a002374 {
+			compatible = "ti,control-phy-pipe3";
+			reg = <0x4a002374 0x4>;
+			reg-names = "power";
+			clocks = <&sys_clkin1>;
+			clock-names = "sysclk";
+		};
+
+		ocp2scp3: ocp2scp3@4a090000 {
+			compatible = "ti,omap-ocp2scp";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges;
+			ti,hwmods = "ocp2scp3";
+			reg = <0x4a090000 0x1f>; /* ocp2scp3 */
+			reg-names = "ocp2scp3";
+			sata_phy: sataphy@4A096000 {
+				compatible = "ti,phy-pipe3-sata";
+				reg = <0x4A096000 0x80>, /* phy_rx */
+				      <0x4A096400 0x64>, /* phy_tx */
+				      <0x4A096800 0x40>; /* pll_ctrl */
+				reg-names = "phy_rx", "phy_tx", "pll_ctrl";
+				ctrl-module = <&omap_control_sata>;
+				clocks = <&sata_ref_clk>,
+					 <&sys_clkin1>;
+				clock-names = "optclk", "sysclk";
+				#phy-cells = <0>;
+			};
+		};
+
+		sata: sata@4a141100 {
+			compatible = "ti,sata";
+			ti,hwmods = "sata";
+			reg = <0x4a141100 0x7>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			ranges;
+			dwc-ahci@4a140000 {
+				  compatible = "snps,dwc-ahci";
+				  reg = <0x4a140000 0x1100>;
+				  interrupts = <0 54 0x4>;
+				  phys = <&sata_phy>;
+				  phy-names = "sata-phy";
+				  clocks = <&sata_ref_clk>;
+				  clock-names = "optclk";
+			};
+		};
+
+
 		mcspi1: spi@48098000 {
 			compatible = "ti,omap4-mcspi";
 			reg = <0x48098000 0x200>;