[PATCH 0/3] Enable ahci_st for stih407 and fix hang

[PATCH 0/3] Enable ahci_st for stih407 and fix hang

[Peter Griffin]

Hi,

Now that miphy28lp phy is present upstream we can add sata DT nodes for
stih407 family silicon. As part of testing this I found a hang
with the current driver implmentation which does some register writes
before enabling the IP clock. Presumably on stih416 devices the sata
clock must have already been enabled by the targetpack/bootloader.

This series also tidys up some inaccuracies in the DT documentation
where the doc hasn't kept aligned with the driver. In addtion to these
fixes I've also added an example for stih407 silicon.

Peter Griffin (3):
  ahci: st: Update the ahci_st DT documentation
  ARM: DT: STi: STiH407: Add sata DT nodes.
  ahci: st: st_configure_oob must be called after IP is clocked.

 Documentation/devicetree/bindings/ata/ahci-st.txt | 45 ++++++++++++++++-------
 arch/arm/boot/dts/stih407-family.dtsi             | 44 ++++++++++++++++++++++
 drivers/ata/ahci_st.c                             |  6 ++-
 3 files changed, 80 insertions(+), 15 deletions(-)

-- 
1.9.1

[PATCH 1/3] ahci: st: Update the ahci_st DT documentation

[Peter Griffin]

As part of testing ahci_st driver working on stih407 I noticed
several things wrong in the DT documentation: -

1) Compatible string doesn't match the driver code
2) pwr-rst reset isn't documented (but exists in the driver)
3) some whitespace issues (spaces not tabs)

Also add in a stih407 family example into the doc.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
 Documentation/devicetree/bindings/ata/ahci-st.txt | 45 ++++++++++++++++-------
 1 file changed, 32 insertions(+), 13 deletions(-)

diff --git a/Documentation/devicetree/bindings/ata/ahci-st.txt b/Documentation/devicetree/bindings/ata/ahci-st.txt
index 1331202..e1d01df 100644
--- a/Documentation/devicetree/bindings/ata/ahci-st.txt
+++ b/Documentation/devicetree/bindings/ata/ahci-st.txt
@@ -3,29 +3,48 @@ STMicroelectronics STi SATA controller
 This binding describes a SATA device.
 
 Required properties:
- - compatible	   : Must be "st,sti-ahci"
+ - 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 port
  - phy-names	   :   Associated name must be; "ahci_phy"
 
+Optional properties:
+ - resets	   : The power-down, soft-reset and power-reset lines of SATA IP
+ - reset-names	   :   Associated names must be; "pwr-dwn", "sw-rst" and "pwr-rst"
+
 Example:
 
+	/* Example for stih416 */
 	sata0: sata@fe380000 {
-		compatible      = "st,sti-ahci";
-		reg             = <0xfe380000 0x1000>;
-		interrupts      = <GIC_SPI 157 IRQ_TYPE_NONE>;
-		interrupt-names = "hostc";
-		phys	        = <&phy_port0 PHY_TYPE_SATA>;
-		phy-names       = "ahci_phy";
-		resets	        = <&powerdown STIH416_SATA0_POWERDOWN>,
+		compatible	= "st,ahci";
+		reg		= <0xfe380000 0x1000>;
+		interrupts	= <GIC_SPI 157 IRQ_TYPE_NONE>;
+		interrupt-names	= "hostc";
+		phys		= <&phy_port0 PHY_TYPE_SATA>;
+		phy-names	= "ahci_phy";
+		resets		= <&powerdown STIH416_SATA0_POWERDOWN>,
 				  <&softreset STIH416_SATA0_SOFTRESET>;
-		reset-names     = "pwr-dwn", "sw-rst";
-		clocks	        = <&clk_s_a0_ls CLK_ICN_REG>;
-		clock-names     = "ahci_clk";
+		reset-names	= "pwr-dwn", "sw-rst";
+		clocks		= <&clk_s_a0_ls CLK_ICN_REG>;
+		clock-names	= "ahci_clk";
+	};
+
+	/* Example for stih407 family silicon */
+	sata0: sata@9b20000 {
+		compatible	= "st,ahci";
+		reg		= <0x9b20000 0x1000>;
+		interrupts	= <GIC_SPI 159 IRQ_TYPE_NONE>;
+		interrupt-names	= "hostc";
+		phys		= <&phy_port0 PHY_TYPE_SATA>;
+		phy-names	= "ahci_phy";
+		resets		= <&powerdown STIH407_SATA0_POWERDOWN>,
+				  <&softreset STIH407_SATA0_SOFTRESET>,
+				  <&softreset STIH407_SATA0_PWR_SOFTRESET>;
+		reset-names	= "pwr-dwn", "sw-rst", "pwr-rst";
+		clocks		= <&clk_s_c0_flexgen CLK_ICN_REG>;
+		clock-names	= "ahci_clk";
 	};
-- 
1.9.1

[PATCH 2/3] ARM: DT: STi: STiH407: Add sata DT nodes.

[Peter Griffin]

Now that the miphy28lp is upstream, we can add the sata dt nodes
for stih407 family silicon. This has been tested on b2120 board
J4 (sata0 channel). These nodes are disabled by default as a
special mini pci-e to sata daughter board is required which
isn't shipped with the board.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
 arch/arm/boot/dts/stih407-family.dtsi | 44 +++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
index a57c06e..d526921 100644
--- a/arch/arm/boot/dts/stih407-family.dtsi
+++ b/arch/arm/boot/dts/stih407-family.dtsi
@@ -338,6 +338,50 @@
 			};
 		};
 
+		sata0: sata@9b20000 {
+			compatible = "st,ahci";
+			reg = <0x9b20000 0x1000>;
+
+			interrupts = <GIC_SPI 159 IRQ_TYPE_NONE>;
+			interrupt-names = "hostc";
+
+			phys = <&phy_port0 PHY_TYPE_SATA>;
+			phy-names = "ahci_phy";
+
+			resets = <&powerdown STIH407_SATA0_POWERDOWN>,
+				 <&softreset STIH407_SATA0_SOFTRESET>,
+				 <&softreset STIH407_SATA0_PWR_SOFTRESET>;
+			reset-names = "pwr-dwn", "sw-rst", "pwr-rst";
+
+			clock-names = "ahci_clk";
+			clocks = <&clk_s_c0_flexgen CLK_ICN_REG>;
+
+			status = "disabled";
+		};
+
+		sata1: sata@9b28000 {
+			compatible = "st,ahci";
+			reg = <0x9b28000 0x1000>;
+
+			interrupts = <GIC_SPI 170 IRQ_TYPE_NONE>;
+			interrupt-names = "hostc";
+
+			phys = <&phy_port1 PHY_TYPE_SATA>;
+			phy-names = "ahci_phy";
+
+			resets = <&powerdown STIH407_SATA1_POWERDOWN>,
+				 <&softreset STIH407_SATA1_SOFTRESET>,
+				 <&softreset STIH407_SATA1_PWR_SOFTRESET>;
+			reset-names = "pwr-dwn",
+				      "sw-rst",
+				      "pwr-rst";
+
+			clock-names = "ahci_clk";
+			clocks = <&clk_s_c0_flexgen CLK_ICN_REG>;
+
+			status = "disabled";
+		};
+
 		st_dwc3: dwc3@8f94000 {
 			compatible	= "st,stih407-dwc3";
 			reg		= <0x08f94000 0x1000>, <0x110 0x4>;
-- 
1.9.1

[PATCH 3/3] ahci: st: st_configure_oob must be called after IP is clocked.

[Peter Griffin]

Currently the ahci_st driver will hang the system on probe, as the
st_configure_oob function does some register writes before the IP
is clocked. This patch moves the function call to after
ahci_platform_enable_resources (which enables the IP clock), and
resolves the hang.

Addtionally st_ahci_configure_oob should be called in the st_ahci_resume
function, so we also rectify that ensuring it is also called after
the IP clock has been enabled.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
 drivers/ata/ahci_st.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/ahci_st.c b/drivers/ata/ahci_st.c
index bc971af..ea0ff00 100644
--- a/drivers/ata/ahci_st.c
+++ b/drivers/ata/ahci_st.c
@@ -68,8 +68,6 @@ static int st_ahci_deassert_resets(struct device *dev)
 		}
 	}
 
-	st_ahci_configure_oob(drv_data->hpriv->mmio);
-
 	if (drv_data->sw_rst) {
 		err = reset_control_deassert(drv_data->sw_rst);
 		if (err) {
@@ -172,6 +170,8 @@ static int st_ahci_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
+	st_ahci_configure_oob(drv_data->hpriv->mmio);
+
 	err = ahci_platform_init_host(pdev, hpriv, &st_ahci_port_info,
 				      &ahci_platform_sht);
 	if (err) {
@@ -222,6 +222,8 @@ static int st_ahci_resume(struct device *dev)
 		return err;
 	}
 
+	st_ahci_configure_oob(drv_data->hpriv->mmio);
+
 	return ahci_platform_resume_host(dev);
 }
 #endif
-- 
1.9.1

Re: [PATCH 1/3] ahci: st: Update the ahci_st DT documentation

[Lee Jones]

On Mon, 02 Mar 2015, Peter Griffin wrote:

> As part of testing ahci_st driver working on stih407 I noticed
> several things wrong in the DT documentation: -
> 
> 1) Compatible string doesn't match the driver code
> 2) pwr-rst reset isn't documented (but exists in the driver)
> 3) some whitespace issues (spaces not tabs)
> 
> Also add in a stih407 family example into the doc.
> 
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
>  Documentation/devicetree/bindings/ata/ahci-st.txt | 45 ++++++++++++++++-------
>  1 file changed, 32 insertions(+), 13 deletions(-)

Acked-by: Lee Jones <lee.jones@linaro.org>

> diff --git a/Documentation/devicetree/bindings/ata/ahci-st.txt b/Documentation/devicetree/bindings/ata/ahci-st.txt
> index 1331202..e1d01df 100644
> --- a/Documentation/devicetree/bindings/ata/ahci-st.txt
> +++ b/Documentation/devicetree/bindings/ata/ahci-st.txt
> @@ -3,29 +3,48 @@ STMicroelectronics STi SATA controller
>  This binding describes a SATA device.
>  
>  Required properties:
> - - compatible	   : Must be "st,sti-ahci"
> + - 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 port
>   - phy-names	   :   Associated name must be; "ahci_phy"
>  
> +Optional properties:
> + - resets	   : The power-down, soft-reset and power-reset lines of SATA IP
> + - reset-names	   :   Associated names must be; "pwr-dwn", "sw-rst" and "pwr-rst"
> +
>  Example:
>  
> +	/* Example for stih416 */
>  	sata0: sata@fe380000 {
> -		compatible      = "st,sti-ahci";
> -		reg             = <0xfe380000 0x1000>;
> -		interrupts      = <GIC_SPI 157 IRQ_TYPE_NONE>;
> -		interrupt-names = "hostc";
> -		phys	        = <&phy_port0 PHY_TYPE_SATA>;
> -		phy-names       = "ahci_phy";
> -		resets	        = <&powerdown STIH416_SATA0_POWERDOWN>,
> +		compatible	= "st,ahci";
> +		reg		= <0xfe380000 0x1000>;
> +		interrupts	= <GIC_SPI 157 IRQ_TYPE_NONE>;
> +		interrupt-names	= "hostc";
> +		phys		= <&phy_port0 PHY_TYPE_SATA>;
> +		phy-names	= "ahci_phy";
> +		resets		= <&powerdown STIH416_SATA0_POWERDOWN>,
>  				  <&softreset STIH416_SATA0_SOFTRESET>;
> -		reset-names     = "pwr-dwn", "sw-rst";
> -		clocks	        = <&clk_s_a0_ls CLK_ICN_REG>;
> -		clock-names     = "ahci_clk";
> +		reset-names	= "pwr-dwn", "sw-rst";
> +		clocks		= <&clk_s_a0_ls CLK_ICN_REG>;
> +		clock-names	= "ahci_clk";
> +	};
> +
> +	/* Example for stih407 family silicon */
> +	sata0: sata@9b20000 {
> +		compatible	= "st,ahci";
> +		reg		= <0x9b20000 0x1000>;
> +		interrupts	= <GIC_SPI 159 IRQ_TYPE_NONE>;
> +		interrupt-names	= "hostc";
> +		phys		= <&phy_port0 PHY_TYPE_SATA>;
> +		phy-names	= "ahci_phy";
> +		resets		= <&powerdown STIH407_SATA0_POWERDOWN>,
> +				  <&softreset STIH407_SATA0_SOFTRESET>,
> +				  <&softreset STIH407_SATA0_PWR_SOFTRESET>;
> +		reset-names	= "pwr-dwn", "sw-rst", "pwr-rst";
> +		clocks		= <&clk_s_c0_flexgen CLK_ICN_REG>;
> +		clock-names	= "ahci_clk";
>  	};

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/3] ARM: DT: STi: STiH407: Add sata DT nodes.

[Lee Jones]

On Mon, 02 Mar 2015, Peter Griffin wrote:

> Now that the miphy28lp is upstream, we can add the sata dt nodes
> for stih407 family silicon. This has been tested on b2120 board
> J4 (sata0 channel). These nodes are disabled by default as a
> special mini pci-e to sata daughter board is required which
> isn't shipped with the board.
> 
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
>  arch/arm/boot/dts/stih407-family.dtsi | 44 +++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)

Acked-by: Lee Jones <lee.jones@linaro.org>

> diff --git a/arch/arm/boot/dts/stih407-family.dtsi b/arch/arm/boot/dts/stih407-family.dtsi
> index a57c06e..d526921 100644
> --- a/arch/arm/boot/dts/stih407-family.dtsi
> +++ b/arch/arm/boot/dts/stih407-family.dtsi
> @@ -338,6 +338,50 @@
>  			};
>  		};
>  
> +		sata0: sata@9b20000 {
> +			compatible = "st,ahci";
> +			reg = <0x9b20000 0x1000>;
> +
> +			interrupts = <GIC_SPI 159 IRQ_TYPE_NONE>;
> +			interrupt-names = "hostc";
> +
> +			phys = <&phy_port0 PHY_TYPE_SATA>;
> +			phy-names = "ahci_phy";
> +
> +			resets = <&powerdown STIH407_SATA0_POWERDOWN>,
> +				 <&softreset STIH407_SATA0_SOFTRESET>,
> +				 <&softreset STIH407_SATA0_PWR_SOFTRESET>;
> +			reset-names = "pwr-dwn", "sw-rst", "pwr-rst";
> +
> +			clock-names = "ahci_clk";
> +			clocks = <&clk_s_c0_flexgen CLK_ICN_REG>;
> +
> +			status = "disabled";
> +		};
> +
> +		sata1: sata@9b28000 {
> +			compatible = "st,ahci";
> +			reg = <0x9b28000 0x1000>;
> +
> +			interrupts = <GIC_SPI 170 IRQ_TYPE_NONE>;
> +			interrupt-names = "hostc";
> +
> +			phys = <&phy_port1 PHY_TYPE_SATA>;
> +			phy-names = "ahci_phy";
> +
> +			resets = <&powerdown STIH407_SATA1_POWERDOWN>,
> +				 <&softreset STIH407_SATA1_SOFTRESET>,
> +				 <&softreset STIH407_SATA1_PWR_SOFTRESET>;
> +			reset-names = "pwr-dwn",
> +				      "sw-rst",
> +				      "pwr-rst";
> +
> +			clock-names = "ahci_clk";
> +			clocks = <&clk_s_c0_flexgen CLK_ICN_REG>;
> +
> +			status = "disabled";
> +		};
> +
>  		st_dwc3: dwc3@8f94000 {
>  			compatible	= "st,stih407-dwc3";
>  			reg		= <0x08f94000 0x1000>, <0x110 0x4>;

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 3/3] ahci: st: st_configure_oob must be called after IP is clocked.

[Lee Jones]

On Mon, 02 Mar 2015, Peter Griffin wrote:

> Currently the ahci_st driver will hang the system on probe, as the
> st_configure_oob function does some register writes before the IP
> is clocked. This patch moves the function call to after
> ahci_platform_enable_resources (which enables the IP clock), and
> resolves the hang.
> 
> Addtionally st_ahci_configure_oob should be called in the st_ahci_resume
> function, so we also rectify that ensuring it is also called after
> the IP clock has been enabled.
> 
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
>  drivers/ata/ahci_st.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

I suspect we might find a few issues like this now we're not using
'clk_ignore_unused'.

Acked-by: Lee Jones <lee.jones@linaro.org>
 
> diff --git a/drivers/ata/ahci_st.c b/drivers/ata/ahci_st.c
> index bc971af..ea0ff00 100644
> --- a/drivers/ata/ahci_st.c
> +++ b/drivers/ata/ahci_st.c
> @@ -68,8 +68,6 @@ static int st_ahci_deassert_resets(struct device *dev)
>  		}
>  	}
>  
> -	st_ahci_configure_oob(drv_data->hpriv->mmio);
> -
>  	if (drv_data->sw_rst) {
>  		err = reset_control_deassert(drv_data->sw_rst);
>  		if (err) {
> @@ -172,6 +170,8 @@ static int st_ahci_probe(struct platform_device *pdev)
>  	if (err)
>  		return err;
>  
> +	st_ahci_configure_oob(drv_data->hpriv->mmio);
> +
>  	err = ahci_platform_init_host(pdev, hpriv, &st_ahci_port_info,
>  				      &ahci_platform_sht);
>  	if (err) {
> @@ -222,6 +222,8 @@ static int st_ahci_resume(struct device *dev)
>  		return err;
>  	}
>  
> +	st_ahci_configure_oob(drv_data->hpriv->mmio);
> +
>  	return ahci_platform_resume_host(dev);
>  }
>  #endif

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 0/3] Enable ahci_st for stih407 and fix hang

[Maxime Coquelin]

Hi Peter,

On 03/02/2015 01:48 PM, Peter Griffin wrote:
> Hi,
>
> Now that miphy28lp phy is present upstream we can add sata DT nodes for
> stih407 family silicon. As part of testing this I found a hang
> with the current driver implmentation which does some register writes
> before enabling the IP clock. Presumably on stih416 devices the sata
> clock must have already been enabled by the targetpack/bootloader.
>
> This series also tidys up some inaccuracies in the DT documentation
> where the doc hasn't kept aligned with the driver. In addtion to these
> fixes I've also added an example for stih407 silicon.
>
> Peter Griffin (3):
>    ahci: st: Update the ahci_st DT documentation
>    ARM: DT: STi: STiH407: Add sata DT nodes.
>    ahci: st: st_configure_oob must be called after IP is clocked.
>
>   Documentation/devicetree/bindings/ata/ahci-st.txt | 45 ++++++++++++++++-------
>   arch/arm/boot/dts/stih407-family.dtsi             | 44 ++++++++++++++++++++++
>   drivers/ata/ahci_st.c                             |  6 ++-
>   3 files changed, 80 insertions(+), 15 deletions(-)
>

For the series, you can add my:
Acked-by: Maxime Coquelin <maxime.coquelin@st.com>

Thanks,
Maxime