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

[PATCH RESEND 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.

Changes in RESEND:
- Rebased on v4.0-rc6 (Pete)
- Applied Acks (Pete)

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>
Acked-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Maxime Coquelin <maxime.coquelin@st.com>
---
 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>
Acked-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Maxime Coquelin <maxime.coquelin@st.com>
---
 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>
Acked-by: Lee Jones <lee.jones@linaro.org>
Acked-by: Maxime Coquelin <maxime.coquelin@st.com>
---
 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 3/3] ahci: st: st_configure_oob must be called after IP is clocked.

[Tejun Heo]

On Tue, Mar 31, 2015 at 08:35:09AM +0100, 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>
> Acked-by: Lee Jones <lee.jones@linaro.org>
> Acked-by: Maxime Coquelin <maxime.coquelin@st.com>

How should this patch be routed?  Should I pick this up and apply to
libata/for-4.1 or should it go together with the other patches?  If
the latter, please feel free to add my acked-by.

Thanks.

-- 
tejun

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

[Maxime Coquelin]

Hello Tejun,

On 03/31/2015 05:29 PM, Tejun Heo wrote:
> On Tue, Mar 31, 2015 at 08:35:09AM +0100, 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>
>> Acked-by: Lee Jones <lee.jones@linaro.org>
>> Acked-by: Maxime Coquelin <maxime.coquelin@st.com>
> How should this patch be routed?  Should I pick this up and apply to
> libata/for-4.1 or should it go together with the other patches?  If
> the latter, please feel free to add my acked-by.

I think you can pick patches 1 & 3.
I will apply patch 2 to STi DT branch.

Thanks,
Maxime
>
> Thanks.
>

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

[Peter Griffin]

Hi Tejun,

On Tue, 31 Mar 2015, Tejun Heo wrote:

> On Tue, Mar 31, 2015 at 08:35:09AM +0100, 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>
> > Acked-by: Lee Jones <lee.jones@linaro.org>
> > Acked-by: Maxime Coquelin <maxime.coquelin@st.com>
> 
> How should this patch be routed?  Should I pick this up and apply to
> libata/for-4.1 or should it go together with the other patches?  If
> the latter, please feel free to add my acked-by.

This patch (and also the DT documentation change) can go via your tree. Maxime
will take the stih407 DT patch.

regards,

Peter.

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

[Tejun Heo]

On Tue, Mar 31, 2015 at 04:36:27PM +0100, Peter Griffin wrote:
> This patch (and also the DT documentation change) can go via your tree. Maxime
> will take the stih407 DT patch.

Hmmm... The first patch doesn't apply to libata/for-4.1?

Thanks.

-- 
tejun

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

[Peter Griffin]

Hi Tejun,

On Tue, 31 Mar 2015, Tejun Heo wrote:

> On Tue, Mar 31, 2015 at 04:36:27PM +0100, Peter Griffin wrote:
> > This patch (and also the DT documentation change) can go via your tree. Maxime
> > will take the stih407 DT patch.
> 
> Hmmm... The first patch doesn't apply to libata/for-4.1?

I was just investigating this, and you don't have the patch I sent on 25th February
entitled "ahci: st: Update the DT example for how to obtain the PHY."

I also did a re-send yesterday with ack's collected here https://lkml.org/lkml/2015/3/30/404.

However both times it seems I didn't add you to the email :-( which certainly explains
why you don't have it.

If I re-send that patch with you on CC can you take and apply it as well? That would
preserve the ack's.

regards,

Peter.

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

[Tejun Heo]

On Tue, Mar 31, 2015 at 08:35:09AM +0100, 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>
> Acked-by: Lee Jones <lee.jones@linaro.org>
> Acked-by: Maxime Coquelin <maxime.coquelin@st.com>

Applied 1, 3 to libata/for-4.1.

Thanks.

-- 
tejun

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

[Maxime Coquelin]

On 03/31/2015 09:35 AM, 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>
> Acked-by: Lee Jones <lee.jones@linaro.org>
> Acked-by: Maxime Coquelin <maxime.coquelin@st.com>
> ---
>   arch/arm/boot/dts/stih407-family.dtsi | 44 +++++++++++++++++++++++++++++++++++
>   1 file changed, 44 insertions(+)
>
>
Applied in sti-dt-for-v4.2.

Thanks!
Maxime

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

[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