Patchwork [2/6] ARM: EXYNOS5: DT Support for SATA and SATA PHY

login
register
mail settings
Submitter Vasanth Ananthan
Date Oct. 9, 2012, 11:48 a.m.
Message ID <1349783332-18467-3-git-send-email-vasanthananthan@gmail.com>
Download mbox | patch
Permalink /patch/190285/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Vasanth Ananthan - Oct. 9, 2012, 11:48 a.m.
This patch adds Device Nodes for SATA and SATA PHY device.

Signed-off-by: Vasanth Ananthan <vasanth.a@samsung.com>
---
 arch/arm/boot/dts/exynos5250-smdk5250.dts |   11 +++++++++++
 arch/arm/boot/dts/exynos5250.dtsi         |   20 ++++++++++++++++++++
 arch/arm/mach-exynos/include/mach/map.h   |    7 +++++++
 arch/arm/mach-exynos/mach-exynos5-dt.c    |    6 ++++++
 4 files changed, 44 insertions(+), 0 deletions(-)
Olof Johansson - Oct. 9, 2012, 6:28 p.m.
Hi,


On Tue, Oct 09, 2012 at 05:18:48PM +0530, Vasanth Ananthan wrote:
> This patch adds Device Nodes for SATA and SATA PHY device.
> 
> Signed-off-by: Vasanth Ananthan <vasanth.a@samsung.com>
> ---
>  arch/arm/boot/dts/exynos5250-smdk5250.dts |   11 +++++++++++
>  arch/arm/boot/dts/exynos5250.dtsi         |   20 ++++++++++++++++++++
>  arch/arm/mach-exynos/include/mach/map.h   |    7 +++++++
>  arch/arm/mach-exynos/mach-exynos5-dt.c    |    6 ++++++
>  4 files changed, 44 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/exynos5250-smdk5250.dts b/arch/arm/boot/dts/exynos5250-smdk5250.dts
> index 8a5e348..bb262ce 100644
> --- a/arch/arm/boot/dts/exynos5250-smdk5250.dts
> +++ b/arch/arm/boot/dts/exynos5250-smdk5250.dts
> @@ -48,6 +48,17 @@
>  		};
>  	};
>  
> +	i2c@121D0000 {
> +		samsung,i2c-sda-delay = <100>;
> +                samsung,i2c-max-bus-freq = <40000>;
> +		samsung,i2c-slave-addr = <0x38>;

Whitespace is off above.

> +
> +		sataphy@70 {

sata-phy would be a more conventional name.

> +			compatible = "samsung,i2c-phy";

i2c-phy? Seems like an odd choice of name. What is this device?

> +			reg = <0x38>;

70 is unit address but here it's 0x38? One of them is wrong. No need for a unit
address if it's a unique name, by the way.


> +		};
> +	};
> +
>  	i2c@12C80000 {
>  		status = "disabled";
>  	};
> diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
> index 004aaa8..5a47a8f 100644
> --- a/arch/arm/boot/dts/exynos5250.dtsi
> +++ b/arch/arm/boot/dts/exynos5250.dtsi
> @@ -88,6 +88,18 @@
>  		interrupts = <0 54 0>;
>  	};
>  
> +	sata@122F0000 {
> +                compatible = "samsung,exynos-sata-ahci";
> +                reg = <0x122F0000 0x1ff>;
> +		interrupts = <0 115 0>;

More whitespace damage. And need binding.

> +        };
> +
> +        sata-phy@12170000 {
> +                compatible = "samsung,exynos-sata-phy";
> +                reg = <0x12170000 0x1ff>;
> +        };

Should have binding too. How does this relate to the i2c device above.

> +
>  	i2c@12C60000 {
>  		compatible = "samsung,s3c2440-i2c";
>  		reg = <0x12C60000 0x100>;
> @@ -152,6 +164,13 @@
>  		#size-cells = <0>;
>  	};
>  
> +	i2c@121D0000 {
> +                compatible = "samsung,s3c2440-sataphy-i2c";

Is this a unique i2c controller, or is it just another one like the others on
the chip? If it's the latter, it should use the regular compatible string.

> +                reg = <0x121D0000 0x100>;
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +        };
> +
>  	spi_0: spi@12d20000 {
>  		compatible = "samsung,exynos4210-spi";
>  		reg = <0x12d20000 0x100>;
> @@ -460,4 +479,5 @@
>  			#gpio-cells = <4>;
>  		};
>  	};
> +

Stray whitespace change.

>  };
> diff --git a/arch/arm/mach-exynos/include/mach/map.h b/arch/arm/mach-exynos/include/mach/map.h
> index c72b675..6827190 100644
> --- a/arch/arm/mach-exynos/include/mach/map.h
> +++ b/arch/arm/mach-exynos/include/mach/map.h
> @@ -177,9 +177,16 @@
>  #define EXYNOS4_PA_HSOTG		0x12480000
>  #define EXYNOS4_PA_USB_HSPHY		0x125B0000
>  
> +#ifdef CONFIG_ARCH_EXYNOS4

No need to ifdef since namespace isn't overlapped.

>  #define EXYNOS4_PA_SATA			0x12560000
>  #define EXYNOS4_PA_SATAPHY		0x125D0000
>  #define EXYNOS4_PA_SATAPHY_CTRL		0x126B0000
> +#endif
> +#ifdef CONFIG_ARCH_EXYNOS5

Same here.

> +#define EXYNOS5_PA_SATA_PHY_CTRL	0x12170000
> +#define EXYNOS5_PA_SATA_PHY_I2C		0x121D0000
> +#define EXYNOS5_PA_SATA_BASE		0x122F0000
> +#endif


-Olof
--
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
Vasanth Ananthan - Oct. 10, 2012, 9:20 a.m.
Hi,

On Tue, Oct 9, 2012 at 11:58 PM, Olof Johansson <olof@lixom.net> wrote:
>
> Hi,
>
>
> On Tue, Oct 09, 2012 at 05:18:48PM +0530, Vasanth Ananthan wrote:
> > This patch adds Device Nodes for SATA and SATA PHY device.
> >
> > Signed-off-by: Vasanth Ananthan <vasanth.a@samsung.com>
> > ---
> >  arch/arm/boot/dts/exynos5250-smdk5250.dts |   11 +++++++++++
> >  arch/arm/boot/dts/exynos5250.dtsi         |   20 ++++++++++++++++++++
> >  arch/arm/mach-exynos/include/mach/map.h   |    7 +++++++
> >  arch/arm/mach-exynos/mach-exynos5-dt.c    |    6 ++++++
> >  4 files changed, 44 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/arm/boot/dts/exynos5250-smdk5250.dts b/arch/arm/boot/dts/exynos5250-smdk5250.dts
> > index 8a5e348..bb262ce 100644
> > --- a/arch/arm/boot/dts/exynos5250-smdk5250.dts
> > +++ b/arch/arm/boot/dts/exynos5250-smdk5250.dts
> > @@ -48,6 +48,17 @@
> >               };
> >       };
> >
> > +     i2c@121D0000 {
> > +             samsung,i2c-sda-delay = <100>;
> > +                samsung,i2c-max-bus-freq = <40000>;
> > +             samsung,i2c-slave-addr = <0x38>;
>
> Whitespace is off above.
>
> > +
> > +             sataphy@70 {
>
> sata-phy would be a more conventional name.
>
> > +                     compatible = "samsung,i2c-phy";
>
> i2c-phy? Seems like an odd choice of name. What is this device?


The SATA physical layer controller is both a platform device and a i2c
slave device.
This compatible string is for the i2c client driver. Hence I have used
i2c-phy here.

>
>
> > +                     reg = <0x38>;
>
> 70 is unit address but here it's 0x38? One of them is wrong. No need for a unit
> address if it's a unique name, by the way.
>
>
> > +             };
> > +     };
> > +
> >       i2c@12C80000 {
> >               status = "disabled";
> >       };
> > diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
> > index 004aaa8..5a47a8f 100644
> > --- a/arch/arm/boot/dts/exynos5250.dtsi
> > +++ b/arch/arm/boot/dts/exynos5250.dtsi
> > @@ -88,6 +88,18 @@
> >               interrupts = <0 54 0>;
> >       };
> >
> > +     sata@122F0000 {
> > +                compatible = "samsung,exynos-sata-ahci";
> > +                reg = <0x122F0000 0x1ff>;
> > +             interrupts = <0 115 0>;
>
> More whitespace damage. And need binding.
>
> > +        };
> > +
> > +        sata-phy@12170000 {
> > +                compatible = "samsung,exynos-sata-phy";
> > +                reg = <0x12170000 0x1ff>;
> > +        };
>
> Should have binding too. How does this relate to the i2c device above.


As mentioned earlier SATA physical layer controller is both a platform
device and also an i2c slave device.
This Node is for the SATA physical layer platform device, the previous
node is for i2c slave device.
Certain initialization settings done directly and other settings has
to be done through i2c.

>
> > +
> >       i2c@12C60000 {
> >               compatible = "samsung,s3c2440-i2c";
> >               reg = <0x12C60000 0x100>;
> > @@ -152,6 +164,13 @@
> >               #size-cells = <0>;
> >       };
> >
> > +     i2c@121D0000 {
> > +                compatible = "samsung,s3c2440-sataphy-i2c";
>
> Is this a unique i2c controller, or is it just another one like the others on
> the chip? If it's the latter, it should use the regular compatible string.


Yes, its a unique i2c controller which lacks an interrupt line while
others are interrupt driven.
Hence I have used a distinct compatible string for the driver to
distinguish the controller.

>
> > +                reg = <0x121D0000 0x100>;
> > +                #address-cells = <1>;
> > +                #size-cells = <0>;
> > +        };
> > +
> >       spi_0: spi@12d20000 {
> >               compatible = "samsung,exynos4210-spi";
> >               reg = <0x12d20000 0x100>;
> > @@ -460,4 +479,5 @@
> >                       #gpio-cells = <4>;
> >               };
> >       };
> > +
>
> Stray whitespace change.
>
> >  };
> > diff --git a/arch/arm/mach-exynos/include/mach/map.h b/arch/arm/mach-exynos/include/mach/map.h
> > index c72b675..6827190 100644
> > --- a/arch/arm/mach-exynos/include/mach/map.h
> > +++ b/arch/arm/mach-exynos/include/mach/map.h
> > @@ -177,9 +177,16 @@
> >  #define EXYNOS4_PA_HSOTG             0x12480000
> >  #define EXYNOS4_PA_USB_HSPHY         0x125B0000
> >
> > +#ifdef CONFIG_ARCH_EXYNOS4
>
> No need to ifdef since namespace isn't overlapped.
>
> >  #define EXYNOS4_PA_SATA                      0x12560000
> >  #define EXYNOS4_PA_SATAPHY           0x125D0000
> >  #define EXYNOS4_PA_SATAPHY_CTRL              0x126B0000
> > +#endif
> > +#ifdef CONFIG_ARCH_EXYNOS5
>
> Same here.
>
> > +#define EXYNOS5_PA_SATA_PHY_CTRL     0x12170000
> > +#define EXYNOS5_PA_SATA_PHY_I2C              0x121D0000
> > +#define EXYNOS5_PA_SATA_BASE         0x122F0000
> > +#endif
>
>
> -Olof


I will incorporate the other comments and resubmit the patch. Thanks.

--
Vasanth Ananthan.
--
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
Olof Johansson - Oct. 10, 2012, 4:31 p.m.
Hi,

On Wed, Oct 10, 2012 at 02:08:31PM +0530, Vasanth Ananthan wrote:

> > > +
> > > +             sataphy@70 {
> >
> > sata-phy would be a more conventional name.
> >
> > > +                     compatible = "samsung,i2c-phy";
> >
> > i2c-phy? Seems like an odd choice of name. What is this device?
> >
> 
> The SATA physical layer controller is both a platform device and a i2c
> slave device.
> This compatible string is for the i2c client driver. Hence I have used
> i2c-phy here.

Ok, but samsung,i2c-phy is much too generic. Maybe something like
samsung,exynos5-sata-phy  (and rename the MMIO-controlled phy controls
to ...sata-phy-controller below).


> > > +        };
> > > +
> > > +        sata-phy@12170000 {
> > > +                compatible = "samsung,exynos-sata-phy";
> > > +                reg = <0x12170000 0x1ff>;
> > > +        };
> >
> > Should have binding too. How does this relate to the i2c device above.
> >
> 
> As mentioned earlier SATA physical layer controller is both a platform
> device and also an i2c slave device.
> This Node is for the SATA physical layer platform device, the previous node
> is for i2c slave device.
> Certain initialization settings done directly and other settings has to be
> done through i2c.

Wow, that's quite awkward. What needs to be done over i2c? I think I have
seen use of SATA without touching the i2c side but it might have been for
a simple setup.

> > > +
> > >       i2c@12C60000 {
> > >               compatible = "samsung,s3c2440-i2c";
> > >               reg = <0x12C60000 0x100>;
> > > @@ -152,6 +164,13 @@
> > >               #size-cells = <0>;
> > >       };
> > >
> > > +     i2c@121D0000 {
> > > +                compatible = "samsung,s3c2440-sataphy-i2c";
> >
> > Is this a unique i2c controller, or is it just another one like the others
> > on
> > the chip? If it's the latter, it should use the regular compatible string.
> >
> 
> Yes, its a unique i2c controller which lacks an interrupt line while others
> are interrupt driven.
> Hence I have used a distinct compatible string for the driver to
> distinguish the controller.

It would be better to just make the i2c bus driver handle the case where there
is no interrupt specifier and just use polling in those cases, especially if
the rest of the device is identical and doesn't need special handling.

As a matter of fact, if that had been done for the hdmi phy, then you could
have done this patch without modifying the driver at all, just device tree
contents. And the same would go for the next time down the road when
a "special" i2c bus is added.


-Olof
--
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
Vasanth Ananthan - Oct. 11, 2012, 6:49 a.m.
Hi,

On Wed, Oct 10, 2012 at 10:01 PM, Olof Johansson <olof@lixom.net> wrote:
> Hi,
>
> On Wed, Oct 10, 2012 at 02:08:31PM +0530, Vasanth Ananthan wrote:
>
>> > > +
>> > > +             sataphy@70 {
>> >
>> > sata-phy would be a more conventional name.
>> >
>> > > +                     compatible = "samsung,i2c-phy";
>> >
>> > i2c-phy? Seems like an odd choice of name. What is this device?
>> >
>>
>> The SATA physical layer controller is both a platform device and a i2c
>> slave device.
>> This compatible string is for the i2c client driver. Hence I have used
>> i2c-phy here.
>
> Ok, but samsung,i2c-phy is much too generic. Maybe something like
> samsung,exynos5-sata-phy  (and rename the MMIO-controlled phy controls
> to ...sata-phy-controller below).
>

I'll do so..

>
>> > > +        };
>> > > +
>> > > +        sata-phy@12170000 {
>> > > +                compatible = "samsung,exynos-sata-phy";
>> > > +                reg = <0x12170000 0x1ff>;
>> > > +        };
>> >
>> > Should have binding too. How does this relate to the i2c device above.
>> >
>>
>> As mentioned earlier SATA physical layer controller is both a platform
>> device and also an i2c slave device.
>> This Node is for the SATA physical layer platform device, the previous node
>> is for i2c slave device.
>> Certain initialization settings done directly and other settings has to be
>> done through i2c.
>
> Wow, that's quite awkward. What needs to be done over i2c? I think I have
> seen use of SATA without touching the i2c side but it might have been for
> a simple setup.

40 bit Interface setting is done through i2c. Internal setting address
0x3A and data 0x0B.

>
>> > > +
>> > >       i2c@12C60000 {
>> > >               compatible = "samsung,s3c2440-i2c";
>> > >               reg = <0x12C60000 0x100>;
>> > > @@ -152,6 +164,13 @@
>> > >               #size-cells = <0>;
>> > >       };
>> > >
>> > > +     i2c@121D0000 {
>> > > +                compatible = "samsung,s3c2440-sataphy-i2c";
>> >
>> > Is this a unique i2c controller, or is it just another one like the others
>> > on
>> > the chip? If it's the latter, it should use the regular compatible string.
>> >
>>
>> Yes, its a unique i2c controller which lacks an interrupt line while others
>> are interrupt driven.
>> Hence I have used a distinct compatible string for the driver to
>> distinguish the controller.
>
> It would be better to just make the i2c bus driver handle the case where there
> is no interrupt specifier and just use polling in those cases, especially if
> the rest of the device is identical and doesn't need special handling.
>
> As a matter of fact, if that had been done for the hdmi phy, then you could
> have done this patch without modifying the driver at all, just device tree
> contents. And the same would go for the next time down the road when
> a "special" i2c bus is added.
>


Yes, It is the i2c bus driver thats handles this case. There is a
subsequent patch
that provides the polling support to the driver. As far as I know, the
i2c controller for
HDMI PHY is also interrupt driven.

>
> -Olof

Patch

diff --git a/arch/arm/boot/dts/exynos5250-smdk5250.dts b/arch/arm/boot/dts/exynos5250-smdk5250.dts
index 8a5e348..bb262ce 100644
--- a/arch/arm/boot/dts/exynos5250-smdk5250.dts
+++ b/arch/arm/boot/dts/exynos5250-smdk5250.dts
@@ -48,6 +48,17 @@ 
 		};
 	};
 
+	i2c@121D0000 {
+		samsung,i2c-sda-delay = <100>;
+                samsung,i2c-max-bus-freq = <40000>;
+		samsung,i2c-slave-addr = <0x38>;
+
+		sataphy@70 {
+			compatible = "samsung,i2c-phy";
+			reg = <0x38>;
+		};
+	};
+
 	i2c@12C80000 {
 		status = "disabled";
 	};
diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
index 004aaa8..5a47a8f 100644
--- a/arch/arm/boot/dts/exynos5250.dtsi
+++ b/arch/arm/boot/dts/exynos5250.dtsi
@@ -88,6 +88,18 @@ 
 		interrupts = <0 54 0>;
 	};
 
+	sata@122F0000 {
+                compatible = "samsung,exynos-sata-ahci";
+                reg = <0x122F0000 0x1ff>;
+		interrupts = <0 115 0>;
+        };
+
+        sata-phy@12170000 {
+                compatible = "samsung,exynos-sata-phy";
+                reg = <0x12170000 0x1ff>;
+        };
+
+
 	i2c@12C60000 {
 		compatible = "samsung,s3c2440-i2c";
 		reg = <0x12C60000 0x100>;
@@ -152,6 +164,13 @@ 
 		#size-cells = <0>;
 	};
 
+	i2c@121D0000 {
+                compatible = "samsung,s3c2440-sataphy-i2c";
+                reg = <0x121D0000 0x100>;
+                #address-cells = <1>;
+                #size-cells = <0>;
+        };
+
 	spi_0: spi@12d20000 {
 		compatible = "samsung,exynos4210-spi";
 		reg = <0x12d20000 0x100>;
@@ -460,4 +479,5 @@ 
 			#gpio-cells = <4>;
 		};
 	};
+
 };
diff --git a/arch/arm/mach-exynos/include/mach/map.h b/arch/arm/mach-exynos/include/mach/map.h
index c72b675..6827190 100644
--- a/arch/arm/mach-exynos/include/mach/map.h
+++ b/arch/arm/mach-exynos/include/mach/map.h
@@ -177,9 +177,16 @@ 
 #define EXYNOS4_PA_HSOTG		0x12480000
 #define EXYNOS4_PA_USB_HSPHY		0x125B0000
 
+#ifdef CONFIG_ARCH_EXYNOS4
 #define EXYNOS4_PA_SATA			0x12560000
 #define EXYNOS4_PA_SATAPHY		0x125D0000
 #define EXYNOS4_PA_SATAPHY_CTRL		0x126B0000
+#endif
+#ifdef CONFIG_ARCH_EXYNOS5
+#define EXYNOS5_PA_SATA_PHY_CTRL	0x12170000
+#define EXYNOS5_PA_SATA_PHY_I2C		0x121D0000
+#define EXYNOS5_PA_SATA_BASE		0x122F0000
+#endif
 
 #define EXYNOS4_PA_SROMC		0x12570000
 #define EXYNOS5_PA_SROMC		0x12250000
diff --git a/arch/arm/mach-exynos/mach-exynos5-dt.c b/arch/arm/mach-exynos/mach-exynos5-dt.c
index ef770bc..c931c6c 100644
--- a/arch/arm/mach-exynos/mach-exynos5-dt.c
+++ b/arch/arm/mach-exynos/mach-exynos5-dt.c
@@ -47,6 +47,12 @@  static const struct of_dev_auxdata exynos5250_auxdata_lookup[] __initconst = {
 				"s3c2440-i2c.0", NULL),
 	OF_DEV_AUXDATA("samsung,s3c2440-i2c", EXYNOS5_PA_IIC(1),
 				"s3c2440-i2c.1", NULL),
+	OF_DEV_AUXDATA("samsung,exynos-sata-ahci", EXYNOS5_PA_SATA_BASE,
+				"sata_exynos", NULL),
+	OF_DEV_AUXDATA("samsung,exynos-sata-phy", EXYNOS5_PA_SATA_PHY_CTRL,
+				"sata_phy_exynos", NULL),
+	OF_DEV_AUXDATA("samsung,s3c2440-sataphy-i2c", EXYNOS5_PA_SATA_PHY_I2C,
+				"sata-phy-i2c", NULL),
 	OF_DEV_AUXDATA("samsung,exynos4210-spi", EXYNOS5_PA_SPI0,
 				"exynos4210-spi.0", NULL),
 	OF_DEV_AUXDATA("samsung,exynos4210-spi", EXYNOS5_PA_SPI1,