[PATCH v2 0/3] Fix SATA disk hotplug for Armada 370/XP SoCs

[PATCH v2 0/3] Fix SATA disk hotplug for Armada 370/XP SoCs

[Simon Guinot]

Hello,

This patch series fixes a SATA disk hotplug issue for the Armada 370/XP
SoCs: once a disk is removed from a SATA port, then the re-plug events
are not detected by the sata_mv driver.

This should be applied to the -stable kernels 3.10 and onwards. 

Changes since v1:
- Introduces a new compatible string for the SATA IP found in Armada
  370/XP SoCs.
- Only fix the LP_PHY_CTL register (0x58) if the compatible string
  "marvell,armada-370-xp-sata" is found in the SATA DT node.
- Update compatible string in DT SATA node for Armada 370/XP SoCs. 

Thanks,

Simon

Lior Amsalem (1):
  ata: sata_mv: fix disk hotplug for Armada 370/XP SoCs

Simon Guinot (2):
  ata: sata_mv: introduce compatible string "marvell,armada-370-xp-sata"
  ARM: mvebu: update the SATA compatible string for Armada 370/XP

 Documentation/devicetree/bindings/ata/marvell.txt |  2 +-
 arch/arm/boot/dts/armada-370-xp.dtsi              |  2 +-
 drivers/ata/sata_mv.c                             | 25 +++++++++++++++++++++++
 3 files changed, 27 insertions(+), 2 deletions(-)

-- 
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Gregory Clement <gregory.clement@free-electrons.com>
Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Cc: Lior Amsalem <alior@marvell.com>
Cc: stable at vger.kernel.org
-- 
1.8.5.2

[PATCH v2 1/3] ata: sata_mv: introduce compatible string "marvell, armada-370-xp-sata"

[Simon Guinot]

The sata_mv driver supports the SATA IP found in several Marvell SoCs.
As some new SATA registers have been introduced with the Armada 370/XP
SoCs, a way to identify them is needed.

This patch introduces a new compatible string for the SATA IP found in
Armada 370/XP SoCs.

Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Gregory Clement <gregory.clement@free-electrons.com>
Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Cc: Lior Amsalem <alior@marvell.com>
Cc: stable at vger.kernel.org
---
 Documentation/devicetree/bindings/ata/marvell.txt | 2 +-
 drivers/ata/sata_mv.c                             | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/ata/marvell.txt b/Documentation/devicetree/bindings/ata/marvell.txt
index b5cdd20cde9c..a52a5d018042 100644
--- a/Documentation/devicetree/bindings/ata/marvell.txt
+++ b/Documentation/devicetree/bindings/ata/marvell.txt
@@ -1,7 +1,7 @@
 * Marvell Orion SATA
 
 Required Properties:
-- compatibility : "marvell,orion-sata"
+- compatibility : "marvell,orion-sata" or "marvell,armada-370-xp-sata"
 - reg           : Address range of controller
 - interrupts    : Interrupt controller is using
 - nr-ports      : Number of SATA ports in use.
diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index 56be31819897..df35c521a141 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -4209,6 +4209,7 @@ static int mv_platform_resume(struct platform_device *pdev)
 
 #ifdef CONFIG_OF
 static struct of_device_id mv_sata_dt_ids[] = {
+	{ .compatible = "marvell,armada-370-xp-sata", },
 	{ .compatible = "marvell,orion-sata", },
 	{},
 };
-- 
1.8.5.2

[PATCH v2 2/3] ata: sata_mv: fix disk hotplug for Armada 370/XP SoCs

[Simon Guinot]

From: Lior Amsalem <alior@marvell.com>

On Armada 370/XP SoCs, once a disk is removed from a SATA port, then the
re-plug events are not detected by the sata_mv driver. This patch fixes
the issue by updating the PHY speed in the LP_PHY_CTL register (0x58)
according to the SControl speed.

Note that this fix is only applied if the compatible string
"marvell,armada-370-xp-sata" is found in the SATA DT node.

Signed-off-by: Lior Amsalem <alior@marvell.com>
Signed-off-by: Nadav Haklai <nadavh@marvell.com>
Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Gregory Clement <gregory.clement@free-electrons.com>
Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Cc: stable at vger.kernel.org
---
Changes since v1:
- Only fix the LP_PHY_CTL register (0x58) if the compatible string
  "marvell,armada-370-xp-sata" is found in the SATA DT node.
- Update the commit message.

 drivers/ata/sata_mv.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index df35c521a141..780bcc808b51 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -304,6 +304,7 @@ enum {
 	MV5_LTMODE		= 0x30,
 	MV5_PHY_CTL		= 0x0C,
 	SATA_IFCFG		= 0x050,
+	LP_PHY_CTL		= 0x058,
 
 	MV_M2_PREAMP_MASK	= 0x7e0,
 
@@ -431,6 +432,7 @@ enum {
 	MV_HP_CUT_THROUGH	= (1 << 10),	/* can use EDMA cut-through */
 	MV_HP_FLAG_SOC		= (1 << 11),	/* SystemOnChip, no PCI */
 	MV_HP_QUIRK_LED_BLINK_EN = (1 << 12),	/* is led blinking enabled? */
+	MV_HP_FIX_LP_PHY_CTL	= (1 << 13),	/* fix speed in LP_PHY_CTL ? */
 
 	/* Port private flags (pp_flags) */
 	MV_PP_FLAG_EDMA_EN	= (1 << 0),	/* is EDMA engine enabled? */
@@ -1358,6 +1360,7 @@ static int mv_scr_write(struct ata_link *link, unsigned int sc_reg_in, u32 val)
 
 	if (ofs != 0xffffffffU) {
 		void __iomem *addr = mv_ap_base(link->ap) + ofs;
+		struct mv_host_priv *hpriv = link->ap->host->private_data;
 		if (sc_reg_in == SCR_CONTROL) {
 			/*
 			 * Workaround for 88SX60x1 FEr SATA#26:
@@ -1374,6 +1377,18 @@ static int mv_scr_write(struct ata_link *link, unsigned int sc_reg_in, u32 val)
 			 */
 			if ((val & 0xf) == 1 || (readl(addr) & 0xf) == 1)
 				val |= 0xf000;
+
+			if (hpriv->hp_flags & MV_HP_FIX_LP_PHY_CTL) {
+				void __iomem *lp_phy_addr =
+					mv_ap_base(link->ap) + LP_PHY_CTL;
+				/*
+				 * Set PHY speed according to SControl speed.
+				 */
+				if ((val & 0xf0) == 0x10)
+					writelfl(0x7, lp_phy_addr);
+				else
+					writelfl(0x227, lp_phy_addr);
+			}
 		}
 		writelfl(val, addr);
 		return 0;
@@ -4110,6 +4125,15 @@ static int mv_platform_probe(struct platform_device *pdev)
 	if (rc)
 		goto err;
 
+	/*
+	 * To allow disk hotplug on Armada 370/XP SoCs, the PHY speed must be
+	 * updated in the LP_PHY_CTL register.
+	 */
+	if (pdev->dev.of_node &&
+		of_device_is_compatible(pdev->dev.of_node,
+					"marvell,armada-370-xp-sata"))
+		hpriv->hp_flags |= MV_HP_FIX_LP_PHY_CTL;
+
 	/* initialize adapter */
 	rc = mv_init_host(host);
 	if (rc)
-- 
1.8.5.2

[PATCH v2 3/3] ARM: mvebu: update the SATA compatible string for Armada 370/XP

[Simon Guinot]

This patch updates the Armada 370/XP SATA node with the new compatible
string "marvell,armada-370-xp-sata".

Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Gregory Clement <gregory.clement@free-electrons.com>
Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Cc: Lior Amsalem <alior@marvell.com>
Cc: stable at vger.kernel.org
---
 arch/arm/boot/dts/armada-370-xp.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 7f10f627ae5b..b5def2416800 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -152,7 +152,7 @@
 			};
 
 			sata at a0000 {
-				compatible = "marvell,orion-sata";
+				compatible = "marvell,armada-370-xp-sata";
 				reg = <0xa0000 0x5000>;
 				interrupts = <55>;
 				clocks = <&gateclk 15>, <&gateclk 30>;
-- 
1.8.5.2

[PATCH v2 3/3] ARM: mvebu: update the SATA compatible string for Armada 370/XP

[Gregory CLEMENT]

Hi Simon,

On 14/01/2014 15:50, Simon Guinot wrote:
> This patch updates the Armada 370/XP SATA node with the new compatible
> string "marvell,armada-370-xp-sata".

Usually we try to use the name of the first SoC instead of a family name.
So here it should be "marvell,armada-370-sata"

Thanks,

Gregory



> 
> Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Gregory Clement <gregory.clement@free-electrons.com>
> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Cc: Lior Amsalem <alior@marvell.com>
> Cc: stable at vger.kernel.org
> ---
>  arch/arm/boot/dts/armada-370-xp.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
> index 7f10f627ae5b..b5def2416800 100644
> --- a/arch/arm/boot/dts/armada-370-xp.dtsi
> +++ b/arch/arm/boot/dts/armada-370-xp.dtsi
> @@ -152,7 +152,7 @@
>  			};
>  
>  			sata at a0000 {
> -				compatible = "marvell,orion-sata";
> +				compatible = "marvell,armada-370-xp-sata";
>  				reg = <0xa0000 0x5000>;
>  				interrupts = <55>;
>  				clocks = <&gateclk 15>, <&gateclk 30>;
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH v2 1/3] ata: sata_mv: introduce compatible string "marvell, armada-370-xp-sata"

[Gregory CLEMENT]

Hi Simon, Jeff, Tejun,

On 14/01/2014 15:50, Simon Guinot wrote:
> The sata_mv driver supports the SATA IP found in several Marvell SoCs.
> As some new SATA registers have been introduced with the Armada 370/XP
> SoCs, a way to identify them is needed.
> 
> This patch introduces a new compatible string for the SATA IP found in
> Armada 370/XP SoCs.
> 
> Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Gregory Clement <gregory.clement@free-electrons.com>
> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Cc: Lior Amsalem <alior@marvell.com>
> Cc: stable at vger.kernel.org
> ---
>  Documentation/devicetree/bindings/ata/marvell.txt | 2 +-
>  drivers/ata/sata_mv.c                             | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/ata/marvell.txt b/Documentation/devicetree/bindings/ata/marvell.txt
> index b5cdd20cde9c..a52a5d018042 100644
> --- a/Documentation/devicetree/bindings/ata/marvell.txt
> +++ b/Documentation/devicetree/bindings/ata/marvell.txt
> @@ -1,7 +1,7 @@
>  * Marvell Orion SATA
>  
>  Required Properties:
> -- compatibility : "marvell,orion-sata"
> +- compatibility : "marvell,orion-sata" or "marvell,armada-370-xp-sata"

Usually we try to use the name of the first SoC instead of a family name.
So here it should be "marvell,armada-370-sata"

Thanks,

Gregory


>  - reg           : Address range of controller
>  - interrupts    : Interrupt controller is using
>  - nr-ports      : Number of SATA ports in use.
> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index 56be31819897..df35c521a141 100644
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c
> @@ -4209,6 +4209,7 @@ static int mv_platform_resume(struct platform_device *pdev)
>  
>  #ifdef CONFIG_OF
>  static struct of_device_id mv_sata_dt_ids[] = {
> +	{ .compatible = "marvell,armada-370-xp-sata", },
>  	{ .compatible = "marvell,orion-sata", },
>  	{},
>  };
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH v2 0/3] Fix SATA disk hotplug for Armada 370/XP SoCs

[Jason Cooper]

Simon,

On Tue, Jan 14, 2014 at 03:50:04PM +0100, Simon Guinot wrote:
> Hello,
> 
> This patch series fixes a SATA disk hotplug issue for the Armada 370/XP
> SoCs: once a disk is removed from a SATA port, then the re-plug events
> are not detected by the sata_mv driver.
> 
> This should be applied to the -stable kernels 3.10 and onwards. 

Please add a 'Fixes: <commit-hash> (oneline)' tag below the Cc: stable
tags.  It looks like the sata_mv binding and the Armada 370 DT booting
were both introduced in v3.6, so I would probably use:

Fixes: 9ae6f740b49f (arm: mach-mvebu: add support for Armada 370 and Armada XP with DT)

Is there a specific reason you say v3.10?

Once you've incorporated Gregory's comments,

Acked-by: Jason Cooper <jason@lakedaemon.net>

> Lior Amsalem (1):
>   ata: sata_mv: fix disk hotplug for Armada 370/XP SoCs
> 
> Simon Guinot (2):
>   ata: sata_mv: introduce compatible string "marvell,armada-370-xp-sata"
>   ARM: mvebu: update the SATA compatible string for Armada 370/XP
> 
>  Documentation/devicetree/bindings/ata/marvell.txt |  2 +-
>  arch/arm/boot/dts/armada-370-xp.dtsi              |  2 +-
>  drivers/ata/sata_mv.c                             | 25 +++++++++++++++++++++++
>  3 files changed, 27 insertions(+), 2 deletions(-)

Tejun,

I see no point to breaking up this series only to have both pieces end
up in stable.  Are you ok with taking the whole thing?

thx,

Jason.

[PATCH v2 0/3] Fix SATA disk hotplug for Armada 370/XP SoCs

[Tejun Heo]

Hey, Jason.

On Tue, Jan 14, 2014 at 10:14:04AM -0500, Jason Cooper wrote:
> I see no point to breaking up this series only to have both pieces end
> up in stable.  Are you ok with taking the whole thing?

Yeah, sure.  Gregory thought the device id could be better.  Is that
not the case?

Thanks.

-- 
tejun

[PATCH v2 0/3] Fix SATA disk hotplug for Armada 370/XP SoCs

[Tejun Heo]

On Tue, Jan 14, 2014 at 10:21:12AM -0500, Tejun Heo wrote:
> Hey, Jason.
> 
> On Tue, Jan 14, 2014 at 10:14:04AM -0500, Jason Cooper wrote:
> > I see no point to breaking up this series only to have both pieces end
> > up in stable.  Are you ok with taking the whole thing?
> 
> Yeah, sure.  Gregory thought the device id could be better.  Is that
> not the case?

Gees, please disregard.  You explicitly commmented about Gregory's
point.  I'll apply the patches once Simon posts the updated version.

Thanks.

-- 
tejun

[PATCH v2 0/3] Fix SATA disk hotplug for Armada 370/XP SoCs

[Russell King - ARM Linux]

On Tue, Jan 14, 2014 at 10:14:04AM -0500, Jason Cooper wrote:
> Simon,
> 
> On Tue, Jan 14, 2014 at 03:50:04PM +0100, Simon Guinot wrote:
> > Hello,
> > 
> > This patch series fixes a SATA disk hotplug issue for the Armada 370/XP
> > SoCs: once a disk is removed from a SATA port, then the re-plug events
> > are not detected by the sata_mv driver.
> > 
> > This should be applied to the -stable kernels 3.10 and onwards. 
> 
> Please add a 'Fixes: <commit-hash> (oneline)' tag below the Cc: stable
> tags.  It looks like the sata_mv binding and the Armada 370 DT booting
> were both introduced in v3.6, so I would probably use:
> 
> Fixes: 9ae6f740b49f (arm: mach-mvebu: add support for Armada 370 and Armada XP with DT)

Correction - it's format is:

Fixes: <commit-hash> ("oneline")

Double-quotes inside the parens.  (Don't ask me why, that's just how it
was decided.)

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[PATCH v2 0/3] Fix SATA disk hotplug for Armada 370/XP SoCs

[Simon Guinot]

On Tue, Jan 14, 2014 at 10:14:04AM -0500, Jason Cooper wrote:
> Simon,
> 
> On Tue, Jan 14, 2014 at 03:50:04PM +0100, Simon Guinot wrote:
> > Hello,
> > 
> > This patch series fixes a SATA disk hotplug issue for the Armada 370/XP
> > SoCs: once a disk is removed from a SATA port, then the re-plug events
> > are not detected by the sata_mv driver.
> > 
> > This should be applied to the -stable kernels 3.10 and onwards. 
> 
> Please add a 'Fixes: <commit-hash> (oneline)' tag below the Cc: stable
> tags.  It looks like the sata_mv binding and the Armada 370 DT booting
> were both introduced in v3.6, so I would probably use:
> 
> Fixes: 9ae6f740b49f (arm: mach-mvebu: add support for Armada 370 and Armada XP with DT)

Hi Jason,

I am not comfortable with this "Fixes: commit_id" notation. It states
that the patch fixes a regression introduced by a given commit. This
hardly makes sense to me because obviously the hotplug issue has not
been introduced by:

"arm: mach-mvebu: add support for Armada 370 and Armada XP with DT".

I know that the purpose is purely administrative, but it is kind of
weird...

> 
> Is there a specific reason you say v3.10?

I could have said nothing as well.

As stable kernel older than 3.10 are no longer maintained, I think
we don't need to point out a specific commit as a -stable target.
Simply Cc'ing -stable without any extra informations should be good
enough.

> 
> Once you've incorporated Gregory's comments,
> 
> Acked-by: Jason Cooper <jason@lakedaemon.net>

OK.

Thanks,

Simon

> 
> > Lior Amsalem (1):
> >   ata: sata_mv: fix disk hotplug for Armada 370/XP SoCs
> > 
> > Simon Guinot (2):
> >   ata: sata_mv: introduce compatible string "marvell,armada-370-xp-sata"
> >   ARM: mvebu: update the SATA compatible string for Armada 370/XP
> > 
> >  Documentation/devicetree/bindings/ata/marvell.txt |  2 +-
> >  arch/arm/boot/dts/armada-370-xp.dtsi              |  2 +-
> >  drivers/ata/sata_mv.c                             | 25 +++++++++++++++++++++++
> >  3 files changed, 27 insertions(+), 2 deletions(-)
> 
> Tejun,
> 
> I see no point to breaking up this series only to have both pieces end
> up in stable.  Are you ok with taking the whole thing?
> 
> thx,
> 
> Jason.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140114/e6b03fc5/attachment.sig>

[PATCH v2 0/3] Fix SATA disk hotplug for Armada 370/XP SoCs

[Andrew Lunn]

> As stable kernel older than 3.10 are no longer maintained

Please take a look at www.kernel.org

Both 3.4 and 3.2 have had stable updated this year.

     Andrew

[PATCH v2 0/3] Fix SATA disk hotplug for Armada 370/XP SoCs

[Tejun Heo]

On Tue, Jan 14, 2014 at 04:48:34PM +0100, Simon Guinot wrote:
> As stable kernel older than 3.10 are no longer maintained, I think
> we don't need to point out a specific commit as a -stable target.
> Simply Cc'ing -stable without any extra informations should be good
> enough.

Well, somebody eventually has to figure it out.  If you know it's
applicable to 3.6+ just do

 Cc: stable at vger.kernel.org # v3.6+

Thanks.

-- 
tejun

[PATCH v2 0/3] Fix SATA disk hotplug for Armada 370/XP SoCs

[Jason Cooper]

Simon,

On Tue, Jan 14, 2014 at 04:48:34PM +0100, Simon Guinot wrote:
> On Tue, Jan 14, 2014 at 10:14:04AM -0500, Jason Cooper wrote:
> > Simon,
> > 
> > On Tue, Jan 14, 2014 at 03:50:04PM +0100, Simon Guinot wrote:
> > > Hello,
> > > 
> > > This patch series fixes a SATA disk hotplug issue for the Armada 370/XP
> > > SoCs: once a disk is removed from a SATA port, then the re-plug events
> > > are not detected by the sata_mv driver.
> > > 
> > > This should be applied to the -stable kernels 3.10 and onwards. 
> > 
> > Please add a 'Fixes: <commit-hash> (oneline)' tag below the Cc: stable
> > tags.  It looks like the sata_mv binding and the Armada 370 DT booting
> > were both introduced in v3.6, so I would probably use:
> > 
> > Fixes: 9ae6f740b49f (arm: mach-mvebu: add support for Armada 370 and Armada XP with DT)
> 
> Hi Jason,
> 
> I am not comfortable with this "Fixes: commit_id" notation. It states
> that the patch fixes a regression introduced by a given commit. This
> hardly makes sense to me because obviously the hotplug issue has not
> been introduced by:
> 
> "arm: mach-mvebu: add support for Armada 370 and Armada XP with DT".
> 
> I know that the purpose is purely administrative, but it is kind of
> weird...

True, the wording may not be the best, but the goal is to make it easier
to do a 'tag --contains' search.  We could call it

Repair-Something-Broken-Since-Introduced-by:

in this case ;-)

> > Is there a specific reason you say v3.10?
> 
> I could have said nothing as well.
> 
> As stable kernel older than 3.10 are no longer maintained, I think
> we don't need to point out a specific commit as a -stable target.
> Simply Cc'ing -stable without any extra informations should be good
> enough.

The reason I ask is that I'd like to know if, in debugging this issue,
you discovered that the fix was not needed before v3.10 (hypothetically,
say because sata hotplug was added in v3.10).

The linux-stable team isn't the only people who find this information
useful.  Distros maintaining older kernels would find it very helpful
when going through -stable patches to know easily if a patch should be
backported further, say to v3.8.

thx,

Jason.

[PATCH v2 0/3] Fix SATA disk hotplug for Armada 370/XP SoCs

[Jason Cooper]

On Tue, Jan 14, 2014 at 10:59:45AM -0500, Jason Cooper wrote:
...
> The linux-stable team isn't the only people who find this information
> useful.  Distros maintaining older kernels would find it very helpful
> when going through -stable patches to know easily if a patch should be
> backported further, say to v3.8.

And yes, I'm aware that -stable is actively supporting farther back than
what we are discussing here.  I was refuting the specific argument as
opposed to correcting wrong assumptions.

In general, it's better to be accurate regarding 'Fixes:' commits than
assuming no one cares about anything older than the oldest -stable
kernel.

thx,

Jason.

[PATCH v2 0/3] Fix SATA disk hotplug for Armada 370/XP SoCs

[Luis Henriques]

On Tue, Jan 14, 2014 at 10:59:45AM -0500, Jason Cooper wrote:
> Simon,
> 
> On Tue, Jan 14, 2014 at 04:48:34PM +0100, Simon Guinot wrote:
> > On Tue, Jan 14, 2014 at 10:14:04AM -0500, Jason Cooper wrote:
> > > Simon,
> > > 
> > > On Tue, Jan 14, 2014 at 03:50:04PM +0100, Simon Guinot wrote:
> > > > Hello,
> > > > 
> > > > This patch series fixes a SATA disk hotplug issue for the Armada 370/XP
> > > > SoCs: once a disk is removed from a SATA port, then the re-plug events
> > > > are not detected by the sata_mv driver.
> > > > 
> > > > This should be applied to the -stable kernels 3.10 and onwards. 
> > > 
> > > Please add a 'Fixes: <commit-hash> (oneline)' tag below the Cc: stable
> > > tags.  It looks like the sata_mv binding and the Armada 370 DT booting
> > > were both introduced in v3.6, so I would probably use:
> > > 
> > > Fixes: 9ae6f740b49f (arm: mach-mvebu: add support for Armada 370 and Armada XP with DT)
> > 
> > Hi Jason,
> > 
> > I am not comfortable with this "Fixes: commit_id" notation. It states
> > that the patch fixes a regression introduced by a given commit. This
> > hardly makes sense to me because obviously the hotplug issue has not
> > been introduced by:
> > 
> > "arm: mach-mvebu: add support for Armada 370 and Armada XP with DT".
> > 
> > I know that the purpose is purely administrative, but it is kind of
> > weird...
> 
> True, the wording may not be the best, but the goal is to make it easier
> to do a 'tag --contains' search.  We could call it
> 
> Repair-Something-Broken-Since-Introduced-by:
> 
> in this case ;-)
> 
> > > Is there a specific reason you say v3.10?
> > 
> > I could have said nothing as well.
> > 
> > As stable kernel older than 3.10 are no longer maintained, I think
> > we don't need to point out a specific commit as a -stable target.
> > Simply Cc'ing -stable without any extra informations should be good
> > enough.
> 
> The reason I ask is that I'd like to know if, in debugging this issue,
> you discovered that the fix was not needed before v3.10 (hypothetically,
> say because sata hotplug was added in v3.10).
> 
> The linux-stable team isn't the only people who find this information
> useful.  Distros maintaining older kernels would find it very helpful
> when going through -stable patches to know easily if a patch should be
> backported further, say to v3.8.

Obviously, I really appreciate when people add this sort of information
into a commit.  This additional information is quite useful for me as a
maintainer of extended (aka 'non-official') stable kernels.

Cheers,
--
Luis

> thx,
> 
> Jason.
> --
> To unsubscribe from this list: send the line "unsubscribe stable" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 0/3] Fix SATA disk hotplug for Armada 370/XP SoCs

[Simon Guinot]

On Tue, Jan 14, 2014 at 10:59:45AM -0500, Jason Cooper wrote:
> Simon,
> 
> On Tue, Jan 14, 2014 at 04:48:34PM +0100, Simon Guinot wrote:
> > On Tue, Jan 14, 2014 at 10:14:04AM -0500, Jason Cooper wrote:
> > > Simon,
> > > 
> > > On Tue, Jan 14, 2014 at 03:50:04PM +0100, Simon Guinot wrote:
> > > > Hello,
> > > > 
> > > > This patch series fixes a SATA disk hotplug issue for the Armada 370/XP
> > > > SoCs: once a disk is removed from a SATA port, then the re-plug events
> > > > are not detected by the sata_mv driver.
> > > > 
> > > > This should be applied to the -stable kernels 3.10 and onwards. 
> > > 
> > > Please add a 'Fixes: <commit-hash> (oneline)' tag below the Cc: stable
> > > tags.  It looks like the sata_mv binding and the Armada 370 DT booting
> > > were both introduced in v3.6, so I would probably use:
> > > 
> > > Fixes: 9ae6f740b49f (arm: mach-mvebu: add support for Armada 370 and Armada XP with DT)
> > 
> > Hi Jason,
> > 
> > I am not comfortable with this "Fixes: commit_id" notation. It states
> > that the patch fixes a regression introduced by a given commit. This
> > hardly makes sense to me because obviously the hotplug issue has not
> > been introduced by:
> > 
> > "arm: mach-mvebu: add support for Armada 370 and Armada XP with DT".
> > 
> > I know that the purpose is purely administrative, but it is kind of
> > weird...
> 
> True, the wording may not be the best, but the goal is to make it easier
> to do a 'tag --contains' search.  We could call it
> 
> Repair-Something-Broken-Since-Introduced-by:
> 
> in this case ;-)
> 
> > > Is there a specific reason you say v3.10?
> > 
> > I could have said nothing as well.
> > 
> > As stable kernel older than 3.10 are no longer maintained, I think
> > we don't need to point out a specific commit as a -stable target.
> > Simply Cc'ing -stable without any extra informations should be good
> > enough.
> 
> The reason I ask is that I'd like to know if, in debugging this issue,
> you discovered that the fix was not needed before v3.10 (hypothetically,
> say because sata hotplug was added in v3.10).
> 
> The linux-stable team isn't the only people who find this information
> useful.  Distros maintaining older kernels would find it very helpful
> when going through -stable patches to know easily if a patch should be
> backported further, say to v3.8.

The patches may effectively apply against a 3.6 kernel but who is going
to embed a such kernel on an Armada-based board ?

IMO, stabilize the Armada support for kernels older than 3.10 is a waste
of time, for you, for the -stable maintainers and for me. Some important
features (for Armada SoCs) are missing on this kernels. Then I don't
think that someone is going to use a kernel older than 3.10 with an
Armada-based board.

But anyway, it is not up to me and I have hopefully added all the needed
-stable informations to the v3 patch series.

Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140115/12e03481/attachment-0001.sig>

[PATCH v2 2/3] ata: sata_mv: fix disk hotplug for Armada 370/XP SoCs

[Thomas Petazzoni]

Dear Simon Guinot,

On Tue, 14 Jan 2014 15:50:06 +0100, Simon Guinot wrote:

> +			if (hpriv->hp_flags & MV_HP_FIX_LP_PHY_CTL) {
> +				void __iomem *lp_phy_addr =
> +					mv_ap_base(link->ap) + LP_PHY_CTL;
> +				/*
> +				 * Set PHY speed according to SControl speed.
> +				 */
> +				if ((val & 0xf0) == 0x10)
> +					writelfl(0x7, lp_phy_addr);
> +				else
> +					writelfl(0x227, lp_phy_addr);
> +			}

I think we could do a little bit better than these magical values.

The datasheet says:

 * bits 12:9, PIN_PHY_GEN_RX. Value 0x0 => 1.5 Gbps, value 0x1 => 3 Gbps
 * bits 8:5, PIN_PHY_GEN_TX. Value 0x0 => 1.5 Gbps, value 0x1 => 3 Gbps
 * bit 2, PIN_PU_TX. Value 0x0 => Power down, value 0x1 => Power up.
 * bit 1, PIN_PU_RX. Value 0x0 => Power down, value 0x1 => Power up.
 * bit 0, PIN_PU_PLL. Value 0x0 => Power down, value 0x1 => Power up.

So maybe something like:

#define PIN_PHY_GEN_1_5		0
#define PIN_PHY_GEN_3		1

#define PIN_PHY_GEN_RX(gen)	((gen) << 9)
#define PIN_PHY_GEN_TX(gen)	((gen) << 5)
#define PIN_PU_TX		BIT(2)
#define PIN_PU_RX		BIT(1)
#define PIN_PU_PLL		BIT(0)


		u32 sata_gen;

		if ((val & 0xf0) == 0x10)
			sata_gen = PIN_PHY_GEN_1_5;
		else
			sata_gen = PIN_PHY_GEN_3;

		writelfl(PIN_PHY_GEN_RX(sata_gen) |
			 PIN_PHY_GEN_TX(sata_gen) |
			 PIN_PU_TX | PIN_PU_RX | PIN_PU_PLL,
			 lp_phy_addr);


> +	/*
> +	 * To allow disk hotplug on Armada 370/XP SoCs, the PHY speed must be
> +	 * updated in the LP_PHY_CTL register.
> +	 */
> +	if (pdev->dev.of_node &&
> +		of_device_is_compatible(pdev->dev.of_node,
> +					"marvell,armada-370-xp-sata"))

Testing whether pdev->dev.of_node is not NULL does not seems to be
useful. A quick read of of_device_is_compatible() and the function it's
calling seem to indicate that of_device_is_compatible will return false
if the passed struct device_node * is NULL.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH v2 2/3] ata: sata_mv: fix disk hotplug for Armada 370/XP SoCs

[Simon Guinot]

On Sun, Jan 26, 2014 at 08:32:10AM +0100, Thomas Petazzoni wrote:
> Dear Simon Guinot,

Hi Thomas,

> 
> On Tue, 14 Jan 2014 15:50:06 +0100, Simon Guinot wrote:
> 
> > +			if (hpriv->hp_flags & MV_HP_FIX_LP_PHY_CTL) {
> > +				void __iomem *lp_phy_addr =
> > +					mv_ap_base(link->ap) + LP_PHY_CTL;
> > +				/*
> > +				 * Set PHY speed according to SControl speed.
> > +				 */
> > +				if ((val & 0xf0) == 0x10)
> > +					writelfl(0x7, lp_phy_addr);
> > +				else
> > +					writelfl(0x227, lp_phy_addr);
> > +			}
> 
> I think we could do a little bit better than these magical values.
> 
> The datasheet says:
> 
>  * bits 12:9, PIN_PHY_GEN_RX. Value 0x0 => 1.5 Gbps, value 0x1 => 3 Gbps
>  * bits 8:5, PIN_PHY_GEN_TX. Value 0x0 => 1.5 Gbps, value 0x1 => 3 Gbps
>  * bit 2, PIN_PU_TX. Value 0x0 => Power down, value 0x1 => Power up.
>  * bit 1, PIN_PU_RX. Value 0x0 => Power down, value 0x1 => Power up.
>  * bit 0, PIN_PU_PLL. Value 0x0 => Power down, value 0x1 => Power up.

I missed this section in the datasheet...

> 
> So maybe something like:
> 
> #define PIN_PHY_GEN_1_5		0
> #define PIN_PHY_GEN_3		1
> 
> #define PIN_PHY_GEN_RX(gen)	((gen) << 9)
> #define PIN_PHY_GEN_TX(gen)	((gen) << 5)
> #define PIN_PU_TX		BIT(2)
> #define PIN_PU_RX		BIT(1)
> #define PIN_PU_PLL		BIT(0)
> 
> 
> 		u32 sata_gen;
> 
> 		if ((val & 0xf0) == 0x10)
> 			sata_gen = PIN_PHY_GEN_1_5;
> 		else
> 			sata_gen = PIN_PHY_GEN_3;
> 
> 		writelfl(PIN_PHY_GEN_RX(sata_gen) |
> 			 PIN_PHY_GEN_TX(sata_gen) |
> 			 PIN_PU_TX | PIN_PU_RX | PIN_PU_PLL,
> 			 lp_phy_addr);

Yes, it is much more understandable.

> 
> 
> > +	/*
> > +	 * To allow disk hotplug on Armada 370/XP SoCs, the PHY speed must be
> > +	 * updated in the LP_PHY_CTL register.
> > +	 */
> > +	if (pdev->dev.of_node &&
> > +		of_device_is_compatible(pdev->dev.of_node,
> > +					"marvell,armada-370-xp-sata"))
> 
> Testing whether pdev->dev.of_node is not NULL does not seems to be
> useful. A quick read of of_device_is_compatible() and the function it's
> calling seem to indicate that of_device_is_compatible will return false
> if the passed struct device_node * is NULL.

I see.

It seems to me, you already have a patch ready to send. Isn't it ?
Or do you want me to do the update ?

Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140131/1e4dc2f0/attachment.sig>

[PATCH v2 2/3] ata: sata_mv: fix disk hotplug for Armada 370/XP SoCs

[Thomas Petazzoni]

Dear Simon Guinot,

On Fri, 31 Jan 2014 11:46:44 +0100, Simon Guinot wrote:

> > > +	/*
> > > +	 * To allow disk hotplug on Armada 370/XP SoCs, the PHY speed must be
> > > +	 * updated in the LP_PHY_CTL register.
> > > +	 */
> > > +	if (pdev->dev.of_node &&
> > > +		of_device_is_compatible(pdev->dev.of_node,
> > > +					"marvell,armada-370-xp-sata"))
> > 
> > Testing whether pdev->dev.of_node is not NULL does not seems to be
> > useful. A quick read of of_device_is_compatible() and the function it's
> > calling seem to indicate that of_device_is_compatible will return false
> > if the passed struct device_node * is NULL.
> 
> I see.
> 
> It seems to me, you already have a patch ready to send. Isn't it ?
> Or do you want me to do the update ?

I see that your patches have been merged, so I will send followup fixes.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com