Fix SATA disk hotplug for Armada 370/XP SoCs

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 <[email protected]>

Cc Thomas Petazzoni <[email protected]>

Cc Jason Cooper <[email protected]>

Cc Andrew Lunn <[email protected]>

Cc Gregory Clement <[email protected]>

Cc Sebastian Hesselbarth <[email protected]>

Cc Lior Amsalem <[email protected]>

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(-)

Documentation/devicetree/bindings/ata/marvell.txt +0 -0

index b5cdd20cde9c..a52a5d018042 100644

Documentation/devicetree/bindings/ata/marvell.txt +0 -0

 - reg           : Address range of controller
 - interrupts    : Interrupt controller is using
 - nr-ports      : Number of SATA ports in use.

drivers/ata/sata_mv.c +1 -0

index 56be31819897..df35c521a141 100644
@@ -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", },
 	{},
 };

Dear Simon Guinot,

drivers/ata/sata_mv.c static int mv_scr_write(struct ata_link *link, unsigned int sc_reg_in, u32 val) · line 1391

Quoted 12 lines

drivers/ata/sata_mv.c +11 -0

+			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);

drivers/ata/sata_mv.c static int mv_platform_probe(struct platform_device *pdev) · line 4134

Quoted 9 lines

drivers/ata/sata_mv.c +7 -0

+	/*
+	 * 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.

Quoted 4 lines

Quoted 4 lines

drivers/ata/sata_mv.c +4 -0

+	/*
+	 * To allow disk hotplug on Armada 370/XP SoCs, the PHY speed must be
+	 * updated in the LP_PHY_CTL register.
+	 */

Quoted 8 lines

drivers/ata/sata_mv.c static int mv_platform_probe(struct platform_device *pdev) · line 4134

Quoted 3 lines

drivers/ata/sata_mv.c +3 -0

+	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 ?

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

Signed-off-by Simon Guinot <[email protected]>

Cc Thomas Petazzoni <[email protected]>

Cc Jason Cooper <[email protected]>

Cc Andrew Lunn <[email protected]>

Cc Gregory Clement <[email protected]>

Cc Sebastian Hesselbarth <[email protected]>

Cc Lior Amsalem <[email protected]>

Cc stable at vger.kernel.org

---
 arch/arm/boot/dts/armada-370-xp.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm/boot/dts/armada-370-xp.dtsi +1 -1

index 7f10f627ae5b..b5def2416800 100644
@@ -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>;

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.

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(-)

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?

Hey, Jason.

Quoted 2 lines · Jason Cooper

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?

Simon,

Quoted 6 lines · 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.

Quoted 1 line

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)

Simon,

Quoted 6 lines · 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.

Quoted 1 line

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 <[email protected]>

Quoted 4 lines

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

Quoted 8 lines

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.

As stable kernel older than 3.10 are no longer maintained

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.

Quoted 9 lines · Jason Cooper

Simon,

Quoted 6 lines · 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.

Quoted 7 lines

Quoted 1 line

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...

Quoted 1 line

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 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.

Simon,

Quoted 25 lines · Simon Guinot

Quoted 9 lines · Jason Cooper

Simon,

Quoted 6 lines · 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.

Quoted 7 lines

Quoted 1 line

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:

Quoted 4 lines

"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 ;-)

Quoted 8 lines

Quoted 1 line

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.
--
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

Simon,

Quoted 25 lines · Simon Guinot

Quoted 9 lines · Jason Cooper

Simon,

Quoted 6 lines · 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.

Quoted 7 lines

Quoted 1 line

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:

Quoted 4 lines

"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 ;-)

Quoted 8 lines

Quoted 1 line

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.