diff mbox

[v2,1/3] ata: ahci_st: Provide DT bindings for ST's SATA implementation

Message ID 1393328481-7251-1-git-send-email-lee.jones@linaro.org
State New
Headers show

Commit Message

Lee Jones Feb. 25, 2014, 11:41 a.m. UTC
Cc: devicetree@vger.kernel.org
Cc: Srinivas Kandagatla <srinivas.kandagatla@st.com>
Acked-by: Alexandre Torgue <alexandre.torgue@st.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 Documentation/devicetree/bindings/ata/ahci-st.txt | 31 +++++++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/ata/ahci-st.txt

Comments

Mark Rutland March 12, 2014, 11:42 a.m. UTC | #1
On Tue, Feb 25, 2014 at 11:41:19AM +0000, Lee Jones wrote:
> Cc: devicetree@vger.kernel.org
> Cc: Srinivas Kandagatla <srinivas.kandagatla@st.com>
> Acked-by: Alexandre Torgue <alexandre.torgue@st.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  Documentation/devicetree/bindings/ata/ahci-st.txt | 31 +++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/ata/ahci-st.txt
> 
> diff --git a/Documentation/devicetree/bindings/ata/ahci-st.txt b/Documentation/devicetree/bindings/ata/ahci-st.txt
> new file mode 100644
> index 0000000..1b69fa9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/ata/ahci-st.txt
> @@ -0,0 +1,31 @@
> +STMicroelectronics STi SATA controller

Is that a family name or a particular device?

> +
> +This binding describes a SATA device.
> +
> +Required properties:
> + - compatible	   : Must be "st,ahci"

Is there not a more specific name? This is awfully generic.

> + - reg		   : Physical base addresses and length of register sets
> + - interrupts	   : Interrupt associated with the SATA device
> + - interrupt-names :   Associated name must be; "hostc"
> + - resets	   : The power-down and soft-reset lines of SATA IP
> + - reset-names	   :   Associated names must be; "pwr-dwn" and "sw-rst"

Get rid of the semicolon, it makes this harder to read. Same for the
other -names proeprties.

> + - clocks	   : The phandle for the clock
> + - clock-names	   :   Associated name must be; "ahci_clk"
> + - phys		   : The phandle for the PHY device
> + - phy-names	   :   Associated name must be; "ahci_phy"

This looks sane, but I'd like to see a more specific description and
compatible string.

Cheers,
Mark.

> +
> +Example:
> +
> +	sata0: sata@fe380000 {
> +		compatible      = "st,ahci";
> +		reg             = <0xfe380000 0x1000>;
> +		interrupts      = <GIC_SPI 157 IRQ_TYPE_NONE>;
> +		interrupt-names = "hostc";
> +		phys	        = <&miphy365x_phy MIPHY_PORT_0 MIPHY_TYPE_SATA>;
> +		phy-names       = "ahci_phy";
> +		resets	        = <&powerdown STIH416_SATA0_POWERDOWN>,
> +				  <&softreset STIH416_SATA0_SOFTRESET>;
> +		reset-names     = "pwr-dwn", "sw-rst";
> +		clocks	        = <&CLK_S_ICN_REG_0>;
> +		clock-names     = "ahci_clk";
> +	};
> -- 
> 1.8.3.2
> 
> --
> 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
> 
--
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
Lee Jones March 12, 2014, 12:48 p.m. UTC | #2
> > +This binding describes a SATA device.
> > +
> > +Required properties:
> > + - compatible	   : Must be "st,ahci"
> 
> Is there not a more specific name? This is awfully generic.

There isn't one. ST only have one AHCI driver. The driver is named as
generically.

> > + - reg		   : Physical base addresses and length of register sets
> > + - interrupts	   : Interrupt associated with the SATA device
> > + - interrupt-names :   Associated name must be; "hostc"
> > + - resets	   : The power-down and soft-reset lines of SATA IP
> > + - reset-names	   :   Associated names must be; "pwr-dwn" and "sw-rst"
> 
> Get rid of the semicolon, it makes this harder to read. Same for the
> other -names proeprties.

Sure.

> > + - clocks	   : The phandle for the clock
> > + - clock-names	   :   Associated name must be; "ahci_clk"
> > + - phys		   : The phandle for the PHY device
> > + - phy-names	   :   Associated name must be; "ahci_phy"
> 
> This looks sane, but I'd like to see a more specific description and
> compatible string.

Specific description in what sense?

"This binding describes a SATA device."

to

"This binding describes ST's SATA device." ?

> > +Example:
> > +
> > +	sata0: sata@fe380000 {
> > +		compatible      = "st,ahci";
> > +		reg             = <0xfe380000 0x1000>;
> > +		interrupts      = <GIC_SPI 157 IRQ_TYPE_NONE>;
> > +		interrupt-names = "hostc";
> > +		phys	        = <&miphy365x_phy MIPHY_PORT_0 MIPHY_TYPE_SATA>;
> > +		phy-names       = "ahci_phy";
> > +		resets	        = <&powerdown STIH416_SATA0_POWERDOWN>,
> > +				  <&softreset STIH416_SATA0_SOFTRESET>;
> > +		reset-names     = "pwr-dwn", "sw-rst";
> > +		clocks	        = <&CLK_S_ICN_REG_0>;
> > +		clock-names     = "ahci_clk";
> > +	};
Arnd Bergmann March 12, 2014, 4:06 p.m. UTC | #3
On Wednesday 12 March 2014 12:48:35 Lee Jones wrote:
> > > +This binding describes a SATA device.
> > > +
> > > +Required properties:
> > > + - compatible         : Must be "st,ahci"
> > 
> > Is there not a more specific name? This is awfully generic.
> 
> There isn't one. ST only have one AHCI driver. The driver is named as
> generically.

If it is this generic, can't you use the completely generic
ahci-platform driver and add a "generic-ahci" compatible string
to that?

	Arnd
--
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
Lee Jones March 12, 2014, 5:14 p.m. UTC | #4
> > > > +This binding describes a SATA device.
> > > > +
> > > > +Required properties:
> > > > + - compatible         : Must be "st,ahci"
> > > 
> > > Is there not a more specific name? This is awfully generic.
> > 
> > There isn't one. ST only have one AHCI driver. The driver is named as
> > generically.
> 
> If it is this generic, can't you use the completely generic
> ahci-platform driver and add a "generic-ahci" compatible string
> to that?

It pretty much does. The only 'special' functionality we handle in
the driver are resets and configuring the OOB. However, both are
required.
Arnd Bergmann March 12, 2014, 6:59 p.m. UTC | #5
On Wednesday 12 March 2014 17:14:52 Lee Jones wrote:
> > > > > +This binding describes a SATA device.
> > > > > +
> > > > > +Required properties:
> > > > > + - compatible         : Must be "st,ahci"
> > > > 
> > > > Is there not a more specific name? This is awfully generic.
> > > 
> > > There isn't one. ST only have one AHCI driver. The driver is named as
> > > generically.
> > 
> > If it is this generic, can't you use the completely generic
> > ahci-platform driver and add a "generic-ahci" compatible string
> > to that?
> 
> It pretty much does. The only 'special' functionality we handle in
> the driver are resets and configuring the OOB. However, both are
> required.

The reset handling should be easy to add to the generic driver
usign as an optional property if I'm not mistaken.

What is OOB?

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/ata/ahci-st.txt b/Documentation/devicetree/bindings/ata/ahci-st.txt
new file mode 100644
index 0000000..1b69fa9
--- /dev/null
+++ b/Documentation/devicetree/bindings/ata/ahci-st.txt
@@ -0,0 +1,31 @@ 
+STMicroelectronics STi SATA controller
+
+This binding describes a SATA device.
+
+Required properties:
+ - compatible	   : Must be "st,ahci"
+ - reg		   : Physical base addresses and length of register sets
+ - interrupts	   : Interrupt associated with the SATA device
+ - interrupt-names :   Associated name must be; "hostc"
+ - resets	   : The power-down and soft-reset lines of SATA IP
+ - reset-names	   :   Associated names must be; "pwr-dwn" and "sw-rst"
+ - clocks	   : The phandle for the clock
+ - clock-names	   :   Associated name must be; "ahci_clk"
+ - phys		   : The phandle for the PHY device
+ - phy-names	   :   Associated name must be; "ahci_phy"
+
+Example:
+
+	sata0: sata@fe380000 {
+		compatible      = "st,ahci";
+		reg             = <0xfe380000 0x1000>;
+		interrupts      = <GIC_SPI 157 IRQ_TYPE_NONE>;
+		interrupt-names = "hostc";
+		phys	        = <&miphy365x_phy MIPHY_PORT_0 MIPHY_TYPE_SATA>;
+		phy-names       = "ahci_phy";
+		resets	        = <&powerdown STIH416_SATA0_POWERDOWN>,
+				  <&softreset STIH416_SATA0_SOFTRESET>;
+		reset-names     = "pwr-dwn", "sw-rst";
+		clocks	        = <&CLK_S_ICN_REG_0>;
+		clock-names     = "ahci_clk";
+	};