diff mbox

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

Message ID 1389711007-7239-3-git-send-email-simon.guinot@sequanux.org (mailing list archive)
State New, archived
Headers show

Commit Message

Simon Guinot Jan. 14, 2014, 2:50 p.m. UTC 
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@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(+)

Comments

Thomas Petazzoni Jan. 26, 2014, 7:32 a.m. UTC | #1 
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
Simon Guinot Jan. 31, 2014, 10:46 a.m. UTC | #2 
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
Thomas Petazzoni Jan. 31, 2014, 10:54 a.m. UTC | #3 
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
diff mbox

Patch

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)