[PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Simon Guinot]

From: Lior Amsalem <alior@marvell.com>

From: Lior Amsalem <alior@marvell.com>

This patch fixes a SATA hotplug issue on the Armada 370 and Armada XP
SoCs. Without it, if a disk is unplugged from a SATA port, then further
hotplug notification are now longer received on this port.

This should be applied to every -stable kernel supporting Armada SoCs.

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
---
 drivers/ata/sata_mv.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index 56be318..89ca472 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,
 
@@ -1358,6 +1359,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;
+		void __iomem *lp_phy_addr = mv_ap_base(link->ap) + LP_PHY_CTL;
 		if (sc_reg_in == SCR_CONTROL) {
 			/*
 			 * Workaround for 88SX60x1 FEr SATA#26:
@@ -1374,6 +1376,14 @@ 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;
+
+			/*
+			 * Setting 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;
-- 
1.8.5.1

[PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Simon Guinot]

From: Lior Amsalem <alior@marvell.com>

From: Lior Amsalem <alior@marvell.com>

This patch fixes a SATA hotplug issue on the Armada 370 and Armada XP
SoCs. Without it, if a disk is unplugged from a SATA port, then further
hotplug notification are now longer received on this port.

This should be applied to every -stable kernel supporting Armada SoCs.

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
---
 drivers/ata/sata_mv.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index 56be318..89ca472 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,
 
@@ -1358,6 +1359,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;
+		void __iomem *lp_phy_addr = mv_ap_base(link->ap) + LP_PHY_CTL;
 		if (sc_reg_in == SCR_CONTROL) {
 			/*
 			 * Workaround for 88SX60x1 FEr SATA#26:
@@ -1374,6 +1376,14 @@ 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;
+
+			/*
+			 * Setting 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;
-- 
1.8.5.1

Re: [PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Jason Cooper]

On Mon, Dec 23, 2013 at 01:07:35PM +0100, Simon Guinot wrote:
> From: Lior Amsalem <alior@marvell.com>
> 
> From: Lior Amsalem <alior@marvell.com>
> 
> This patch fixes a SATA hotplug issue on the Armada 370 and Armada XP
> SoCs. Without it, if a disk is unplugged from a SATA port, then further
> hotplug notification are now longer received on this port.
> 
> This should be applied to every -stable kernel supporting Armada SoCs.

Could we get a little more specific here?  Please determine which commit
introduced the regression and note it with 'Fixes: <commitish> "oneline"'

It's really needed here since the sata_mv driver predates the Armada
SoCs introduction.  Is it possible Kirkwood et al also experience this
problem?

thx,

Jason.

> 
> 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
> ---
>  drivers/ata/sata_mv.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index 56be318..89ca472 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,
>  
> @@ -1358,6 +1359,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;
> +		void __iomem *lp_phy_addr = mv_ap_base(link->ap) + LP_PHY_CTL;
>  		if (sc_reg_in == SCR_CONTROL) {
>  			/*
>  			 * Workaround for 88SX60x1 FEr SATA#26:
> @@ -1374,6 +1376,14 @@ 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;
> +
> +			/*
> +			 * Setting 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;
> -- 
> 1.8.5.1
> 

[PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Jason Cooper]

On Mon, Dec 23, 2013 at 01:07:35PM +0100, Simon Guinot wrote:
> From: Lior Amsalem <alior@marvell.com>
> 
> From: Lior Amsalem <alior@marvell.com>
> 
> This patch fixes a SATA hotplug issue on the Armada 370 and Armada XP
> SoCs. Without it, if a disk is unplugged from a SATA port, then further
> hotplug notification are now longer received on this port.
> 
> This should be applied to every -stable kernel supporting Armada SoCs.

Could we get a little more specific here?  Please determine which commit
introduced the regression and note it with 'Fixes: <commitish> "oneline"'

It's really needed here since the sata_mv driver predates the Armada
SoCs introduction.  Is it possible Kirkwood et al also experience this
problem?

thx,

Jason.

> 
> 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
> ---
>  drivers/ata/sata_mv.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index 56be318..89ca472 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,
>  
> @@ -1358,6 +1359,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;
> +		void __iomem *lp_phy_addr = mv_ap_base(link->ap) + LP_PHY_CTL;
>  		if (sc_reg_in == SCR_CONTROL) {
>  			/*
>  			 * Workaround for 88SX60x1 FEr SATA#26:
> @@ -1374,6 +1376,14 @@ 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;
> +
> +			/*
> +			 * Setting 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;
> -- 
> 1.8.5.1
> 

Re: [PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Andrew Lunn]

On Tue, Dec 24, 2013 at 02:46:03PM -0500, Jason Cooper wrote:
> On Mon, Dec 23, 2013 at 01:07:35PM +0100, Simon Guinot wrote:
> > From: Lior Amsalem <alior@marvell.com>
> > 
> > From: Lior Amsalem <alior@marvell.com>
> > 
> > This patch fixes a SATA hotplug issue on the Armada 370 and Armada XP
> > SoCs. Without it, if a disk is unplugged from a SATA port, then further
> > hotplug notification are now longer received on this port.
> > 
> > This should be applied to every -stable kernel supporting Armada SoCs.
> 
> Could we get a little more specific here?  Please determine which commit
> introduced the regression and note it with 'Fixes: <commitish> "oneline"'
> 
> It's really needed here since the sata_mv driver predates the Armada
> SoCs introduction.  Is it possible Kirkwood et al also experience this
> problem?

Hi Jason

It is on my TODO list to try this out on Kirkwood and Dove. I'm pretty
sure replug works on Dove without this, not tried Kirkwood yet.

     Andrew

[PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Andrew Lunn]

On Tue, Dec 24, 2013 at 02:46:03PM -0500, Jason Cooper wrote:
> On Mon, Dec 23, 2013 at 01:07:35PM +0100, Simon Guinot wrote:
> > From: Lior Amsalem <alior@marvell.com>
> > 
> > From: Lior Amsalem <alior@marvell.com>
> > 
> > This patch fixes a SATA hotplug issue on the Armada 370 and Armada XP
> > SoCs. Without it, if a disk is unplugged from a SATA port, then further
> > hotplug notification are now longer received on this port.
> > 
> > This should be applied to every -stable kernel supporting Armada SoCs.
> 
> Could we get a little more specific here?  Please determine which commit
> introduced the regression and note it with 'Fixes: <commitish> "oneline"'
> 
> It's really needed here since the sata_mv driver predates the Armada
> SoCs introduction.  Is it possible Kirkwood et al also experience this
> problem?

Hi Jason

It is on my TODO list to try this out on Kirkwood and Dove. I'm pretty
sure replug works on Dove without this, not tried Kirkwood yet.

     Andrew

Re: [PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 3273 bytes --]

On Tue, Dec 24, 2013 at 02:46:03PM -0500, Jason Cooper wrote:
> On Mon, Dec 23, 2013 at 01:07:35PM +0100, Simon Guinot wrote:
> > From: Lior Amsalem <alior@marvell.com>
> > 
> > From: Lior Amsalem <alior@marvell.com>
> > 
> > This patch fixes a SATA hotplug issue on the Armada 370 and Armada XP
> > SoCs. Without it, if a disk is unplugged from a SATA port, then further
> > hotplug notification are now longer received on this port.
> > 
> > This should be applied to every -stable kernel supporting Armada SoCs.
> 
> Could we get a little more specific here?  Please determine which commit
> introduced the regression and note it with 'Fixes: <commitish> "oneline"'

Well, since the DT support for the sata_mv driver precedes SATA support
for Armada SoCs, I'd say that the bug has been introduced by:

a6a6de1a "arm: mvebu: SATA support: SoC-level DT data for Armada 370/XP"

Let me know if you agree with that. I will update the commit message
accorgingly.

> 
> It's really needed here since the sata_mv driver predates the Armada
> SoCs introduction.  Is it possible Kirkwood et al also experience this
> problem?

On my Orion and Kirkwood based bords, SATA disk hotplug works correctly.

Simon

> 
> thx,
> 
> Jason.
> 
> > 
> > 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
> > ---
> >  drivers/ata/sata_mv.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> > index 56be318..89ca472 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,
> >  
> > @@ -1358,6 +1359,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;
> > +		void __iomem *lp_phy_addr = mv_ap_base(link->ap) + LP_PHY_CTL;
> >  		if (sc_reg_in == SCR_CONTROL) {
> >  			/*
> >  			 * Workaround for 88SX60x1 FEr SATA#26:
> > @@ -1374,6 +1376,14 @@ 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;
> > +
> > +			/*
> > +			 * Setting 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;
> > -- 
> > 1.8.5.1
> > 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Simon Guinot]

On Tue, Dec 24, 2013 at 02:46:03PM -0500, Jason Cooper wrote:
> On Mon, Dec 23, 2013 at 01:07:35PM +0100, Simon Guinot wrote:
> > From: Lior Amsalem <alior@marvell.com>
> > 
> > From: Lior Amsalem <alior@marvell.com>
> > 
> > This patch fixes a SATA hotplug issue on the Armada 370 and Armada XP
> > SoCs. Without it, if a disk is unplugged from a SATA port, then further
> > hotplug notification are now longer received on this port.
> > 
> > This should be applied to every -stable kernel supporting Armada SoCs.
> 
> Could we get a little more specific here?  Please determine which commit
> introduced the regression and note it with 'Fixes: <commitish> "oneline"'

Well, since the DT support for the sata_mv driver precedes SATA support
for Armada SoCs, I'd say that the bug has been introduced by:

a6a6de1a "arm: mvebu: SATA support: SoC-level DT data for Armada 370/XP"

Let me know if you agree with that. I will update the commit message
accorgingly.

> 
> It's really needed here since the sata_mv driver predates the Armada
> SoCs introduction.  Is it possible Kirkwood et al also experience this
> problem?

On my Orion and Kirkwood based bords, SATA disk hotplug works correctly.

Simon

> 
> thx,
> 
> Jason.
> 
> > 
> > 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
> > ---
> >  drivers/ata/sata_mv.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> > index 56be318..89ca472 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,
> >  
> > @@ -1358,6 +1359,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;
> > +		void __iomem *lp_phy_addr = mv_ap_base(link->ap) + LP_PHY_CTL;
> >  		if (sc_reg_in == SCR_CONTROL) {
> >  			/*
> >  			 * Workaround for 88SX60x1 FEr SATA#26:
> > @@ -1374,6 +1376,14 @@ 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;
> > +
> > +			/*
> > +			 * Setting 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;
> > -- 
> > 1.8.5.1
> > 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
-------------- 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/20131225/2570a669/attachment.sig>

Re: [PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Thomas Petazzoni]

Dear Simon Guinot,

On Wed, 25 Dec 2013 23:40:34 +0100, Simon Guinot wrote:

> > > This should be applied to every -stable kernel supporting Armada
> > > SoCs.
> > 
> > Could we get a little more specific here?  Please determine which
> > commit introduced the regression and note it with 'Fixes:
> > <commitish> "oneline"'
> 
> Well, since the DT support for the sata_mv driver precedes SATA
> support for Armada SoCs, I'd say that the bug has been introduced by:
> 
> a6a6de1a "arm: mvebu: SATA support: SoC-level DT data for Armada
> 370/XP"
> 
> Let me know if you agree with that. I will update the commit message
> accorgingly.

In some sense, we could say that this is not a regression. According to
what you mean, SATA hotplug has *never* been working on Armada 370/XP.
So technically, it could be seen as a new feature for this platform,
and is therefore not a regression (i.e something that used to work, and
that no longer works). There has been no kernel release for which SATA
hotplug was working for Armada 370/XP.

Best regards,

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

[PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Thomas Petazzoni]

Dear Simon Guinot,

On Wed, 25 Dec 2013 23:40:34 +0100, Simon Guinot wrote:

> > > This should be applied to every -stable kernel supporting Armada
> > > SoCs.
> > 
> > Could we get a little more specific here?  Please determine which
> > commit introduced the regression and note it with 'Fixes:
> > <commitish> "oneline"'
> 
> Well, since the DT support for the sata_mv driver precedes SATA
> support for Armada SoCs, I'd say that the bug has been introduced by:
> 
> a6a6de1a "arm: mvebu: SATA support: SoC-level DT data for Armada
> 370/XP"
> 
> Let me know if you agree with that. I will update the commit message
> accorgingly.

In some sense, we could say that this is not a regression. According to
what you mean, SATA hotplug has *never* been working on Armada 370/XP.
So technically, it could be seen as a new feature for this platform,
and is therefore not a regression (i.e something that used to work, and
that no longer works). There has been no kernel release for which SATA
hotplug was working for Armada 370/XP.

Best regards,

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

Re: [PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 1435 bytes --]

On Thu, Dec 26, 2013 at 08:53:55AM +0100, Thomas Petazzoni wrote:
> Dear Simon Guinot,
> 
> On Wed, 25 Dec 2013 23:40:34 +0100, Simon Guinot wrote:
> 
> > > > This should be applied to every -stable kernel supporting Armada
> > > > SoCs.
> > > 
> > > Could we get a little more specific here?  Please determine which
> > > commit introduced the regression and note it with 'Fixes:
> > > <commitish> "oneline"'
> > 
> > Well, since the DT support for the sata_mv driver precedes SATA
> > support for Armada SoCs, I'd say that the bug has been introduced by:
> > 
> > a6a6de1a "arm: mvebu: SATA support: SoC-level DT data for Armada
> > 370/XP"
> > 
> > Let me know if you agree with that. I will update the commit message
> > accorgingly.
> 
> In some sense, we could say that this is not a regression. According to
> what you mean, SATA hotplug has *never* been working on Armada 370/XP.
> So technically, it could be seen as a new feature for this platform,
> and is therefore not a regression (i.e something that used to work, and
> that no longer works). There has been no kernel release for which SATA
> hotplug was working for Armada 370/XP.

I agree with that. It is not correct to call this issue a regression.
But, it would also be nice to have this patch applied in the stable
branches 3.10 and onwards. Maybe, I could just add this information
while CC'ing Linux stable ?

Regards,

Simon

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Simon Guinot]

On Thu, Dec 26, 2013 at 08:53:55AM +0100, Thomas Petazzoni wrote:
> Dear Simon Guinot,
> 
> On Wed, 25 Dec 2013 23:40:34 +0100, Simon Guinot wrote:
> 
> > > > This should be applied to every -stable kernel supporting Armada
> > > > SoCs.
> > > 
> > > Could we get a little more specific here?  Please determine which
> > > commit introduced the regression and note it with 'Fixes:
> > > <commitish> "oneline"'
> > 
> > Well, since the DT support for the sata_mv driver precedes SATA
> > support for Armada SoCs, I'd say that the bug has been introduced by:
> > 
> > a6a6de1a "arm: mvebu: SATA support: SoC-level DT data for Armada
> > 370/XP"
> > 
> > Let me know if you agree with that. I will update the commit message
> > accorgingly.
> 
> In some sense, we could say that this is not a regression. According to
> what you mean, SATA hotplug has *never* been working on Armada 370/XP.
> So technically, it could be seen as a new feature for this platform,
> and is therefore not a regression (i.e something that used to work, and
> that no longer works). There has been no kernel release for which SATA
> hotplug was working for Armada 370/XP.

I agree with that. It is not correct to call this issue a regression.
But, it would also be nice to have this patch applied in the stable
branches 3.10 and onwards. Maybe, I could just add this information
while CC'ing Linux stable ?

Regards,

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/20131226/4cdb287c/attachment.sig>

Re: [PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Thomas Petazzoni]

Dear Simon Guinot,

On Thu, 26 Dec 2013 12:54:28 +0100, Simon Guinot wrote:

> > In some sense, we could say that this is not a regression. According to
> > what you mean, SATA hotplug has *never* been working on Armada 370/XP.
> > So technically, it could be seen as a new feature for this platform,
> > and is therefore not a regression (i.e something that used to work, and
> > that no longer works). There has been no kernel release for which SATA
> > hotplug was working for Armada 370/XP.
> 
> I agree with that. It is not correct to call this issue a regression.
> But, it would also be nice to have this patch applied in the stable
> branches 3.10 and onwards. Maybe, I could just add this information
> while CC'ing Linux stable ?

Yes. Even though not a regression, I believe it qualifies for -stable,
at least according to the rules in
Documentation/stable_kernel_rules.txt.

Best regards,

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

[PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Thomas Petazzoni]

Dear Simon Guinot,

On Thu, 26 Dec 2013 12:54:28 +0100, Simon Guinot wrote:

> > In some sense, we could say that this is not a regression. According to
> > what you mean, SATA hotplug has *never* been working on Armada 370/XP.
> > So technically, it could be seen as a new feature for this platform,
> > and is therefore not a regression (i.e something that used to work, and
> > that no longer works). There has been no kernel release for which SATA
> > hotplug was working for Armada 370/XP.
> 
> I agree with that. It is not correct to call this issue a regression.
> But, it would also be nice to have this patch applied in the stable
> branches 3.10 and onwards. Maybe, I could just add this information
> while CC'ing Linux stable ?

Yes. Even though not a regression, I believe it qualifies for -stable,
at least according to the rules in
Documentation/stable_kernel_rules.txt.

Best regards,

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

Re: [PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Andrew Lunn]

On Mon, Dec 23, 2013 at 01:07:35PM +0100, Simon Guinot wrote:
> From: Lior Amsalem <alior@marvell.com>
> 
> From: Lior Amsalem <alior@marvell.com>
> 
> This patch fixes a SATA hotplug issue on the Armada 370 and Armada XP
> SoCs. Without it, if a disk is unplugged from a SATA port, then further
> hotplug notification are now longer received on this port.
> 
> This should be applied to every -stable kernel supporting Armada SoCs.
> 
> 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

I tested this on Kirkwood, which does not have any issues to
(re)hotplug. Still works O.K. with this patch.

My only reservation is that neither the Kirkwood nor Dove datasheet
list the LP_PHY_CTL register. Are we now poking something which does
not exist on these SoCs? Is that safe?

Tested-by: Andrew Lunn <andrew@lunn.ch>

    Andrew


> ---
>  drivers/ata/sata_mv.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index 56be318..89ca472 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,
>  
> @@ -1358,6 +1359,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;
> +		void __iomem *lp_phy_addr = mv_ap_base(link->ap) + LP_PHY_CTL;
>  		if (sc_reg_in == SCR_CONTROL) {
>  			/*
>  			 * Workaround for 88SX60x1 FEr SATA#26:
> @@ -1374,6 +1376,14 @@ 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;
> +
> +			/*
> +			 * Setting 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;
> -- 
> 1.8.5.1
> 

[PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Andrew Lunn]

On Mon, Dec 23, 2013 at 01:07:35PM +0100, Simon Guinot wrote:
> From: Lior Amsalem <alior@marvell.com>
> 
> From: Lior Amsalem <alior@marvell.com>
> 
> This patch fixes a SATA hotplug issue on the Armada 370 and Armada XP
> SoCs. Without it, if a disk is unplugged from a SATA port, then further
> hotplug notification are now longer received on this port.
> 
> This should be applied to every -stable kernel supporting Armada SoCs.
> 
> 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

I tested this on Kirkwood, which does not have any issues to
(re)hotplug. Still works O.K. with this patch.

My only reservation is that neither the Kirkwood nor Dove datasheet
list the LP_PHY_CTL register. Are we now poking something which does
not exist on these SoCs? Is that safe?

Tested-by: Andrew Lunn <andrew@lunn.ch>

    Andrew


> ---
>  drivers/ata/sata_mv.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index 56be318..89ca472 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,
>  
> @@ -1358,6 +1359,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;
> +		void __iomem *lp_phy_addr = mv_ap_base(link->ap) + LP_PHY_CTL;
>  		if (sc_reg_in == SCR_CONTROL) {
>  			/*
>  			 * Workaround for 88SX60x1 FEr SATA#26:
> @@ -1374,6 +1376,14 @@ 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;
> +
> +			/*
> +			 * Setting 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;
> -- 
> 1.8.5.1
> 

Re: [PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Jason Cooper]

Simon,

On Wed, Dec 25, 2013 at 11:40:34PM +0100, Simon Guinot wrote:
> On Tue, Dec 24, 2013 at 02:46:03PM -0500, Jason Cooper wrote:
> > On Mon, Dec 23, 2013 at 01:07:35PM +0100, Simon Guinot wrote:
> > > From: Lior Amsalem <alior@marvell.com>
> > > 
> > > From: Lior Amsalem <alior@marvell.com>
> > > 
> > > This patch fixes a SATA hotplug issue on the Armada 370 and Armada XP
> > > SoCs. Without it, if a disk is unplugged from a SATA port, then further
> > > hotplug notification are now longer received on this port.
> > > 
> > > This should be applied to every -stable kernel supporting Armada SoCs.
> > 
> > Could we get a little more specific here?  Please determine which commit
> > introduced the regression and note it with 'Fixes: <commitish> "oneline"'
> 
> Well, since the DT support for the sata_mv driver precedes SATA support
> for Armada SoCs, I'd say that the bug has been introduced by:
> 
> a6a6de1a "arm: mvebu: SATA support: SoC-level DT data for Armada 370/XP"

fyi - we've started using 12 chars for the abbreviated sha1.  You can
update your config with 'git config --global core.abbrev 12'

> Let me know if you agree with that. I will update the commit message
> accorgingly.

No, I don't really agree with that.  We should try not to tie the dts
and the kernel version if at all possible.  The correct commit will be
the earliest one that can experience the regression.  I suspect it will
be the either the commit adding the DT binding to sata_mv or the commit
adding mach-mvebu.

> > It's really needed here since the sata_mv driver predates the Armada
> > SoCs introduction.  Is it possible Kirkwood et al also experience this
> > problem?
> 
> On my Orion and Kirkwood based bords, SATA disk hotplug works correctly.

Good, one less thing to worry about.

thx,

Jason.

[PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Jason Cooper]

Simon,

On Wed, Dec 25, 2013 at 11:40:34PM +0100, Simon Guinot wrote:
> On Tue, Dec 24, 2013 at 02:46:03PM -0500, Jason Cooper wrote:
> > On Mon, Dec 23, 2013 at 01:07:35PM +0100, Simon Guinot wrote:
> > > From: Lior Amsalem <alior@marvell.com>
> > > 
> > > From: Lior Amsalem <alior@marvell.com>
> > > 
> > > This patch fixes a SATA hotplug issue on the Armada 370 and Armada XP
> > > SoCs. Without it, if a disk is unplugged from a SATA port, then further
> > > hotplug notification are now longer received on this port.
> > > 
> > > This should be applied to every -stable kernel supporting Armada SoCs.
> > 
> > Could we get a little more specific here?  Please determine which commit
> > introduced the regression and note it with 'Fixes: <commitish> "oneline"'
> 
> Well, since the DT support for the sata_mv driver precedes SATA support
> for Armada SoCs, I'd say that the bug has been introduced by:
> 
> a6a6de1a "arm: mvebu: SATA support: SoC-level DT data for Armada 370/XP"

fyi - we've started using 12 chars for the abbreviated sha1.  You can
update your config with 'git config --global core.abbrev 12'

> Let me know if you agree with that. I will update the commit message
> accorgingly.

No, I don't really agree with that.  We should try not to tie the dts
and the kernel version if at all possible.  The correct commit will be
the earliest one that can experience the regression.  I suspect it will
be the either the commit adding the DT binding to sata_mv or the commit
adding mach-mvebu.

> > It's really needed here since the sata_mv driver predates the Armada
> > SoCs introduction.  Is it possible Kirkwood et al also experience this
> > problem?
> 
> On my Orion and Kirkwood based bords, SATA disk hotplug works correctly.

Good, one less thing to worry about.

thx,

Jason.

Re: [PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 2798 bytes --]

On Thu, Dec 26, 2013 at 07:01:57PM +0100, Andrew Lunn wrote:
> On Mon, Dec 23, 2013 at 01:07:35PM +0100, Simon Guinot wrote:
> > From: Lior Amsalem <alior@marvell.com>
> > 
> > From: Lior Amsalem <alior@marvell.com>
> > 
> > This patch fixes a SATA hotplug issue on the Armada 370 and Armada XP
> > SoCs. Without it, if a disk is unplugged from a SATA port, then further
> > hotplug notification are now longer received on this port.
> > 
> > This should be applied to every -stable kernel supporting Armada SoCs.
> > 
> > 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
> 
> I tested this on Kirkwood, which does not have any issues to
> (re)hotplug. Still works O.K. with this patch.

Hi Andrew,

Thanks for the tests.

> 
> My only reservation is that neither the Kirkwood nor Dove datasheet
> list the LP_PHY_CTL register. Are we now poking something which does
> not exist on these SoCs? Is that safe?

I will check that.

Simon

> 
> Tested-by: Andrew Lunn <andrew@lunn.ch>

> 
>     Andrew
> 
> 
> > ---
> >  drivers/ata/sata_mv.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> > index 56be318..89ca472 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,
> >  
> > @@ -1358,6 +1359,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;
> > +		void __iomem *lp_phy_addr = mv_ap_base(link->ap) + LP_PHY_CTL;
> >  		if (sc_reg_in == SCR_CONTROL) {
> >  			/*
> >  			 * Workaround for 88SX60x1 FEr SATA#26:
> > @@ -1374,6 +1376,14 @@ 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;
> > +
> > +			/*
> > +			 * Setting 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;
> > -- 
> > 1.8.5.1
> > 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Simon Guinot]

On Thu, Dec 26, 2013 at 07:01:57PM +0100, Andrew Lunn wrote:
> On Mon, Dec 23, 2013 at 01:07:35PM +0100, Simon Guinot wrote:
> > From: Lior Amsalem <alior@marvell.com>
> > 
> > From: Lior Amsalem <alior@marvell.com>
> > 
> > This patch fixes a SATA hotplug issue on the Armada 370 and Armada XP
> > SoCs. Without it, if a disk is unplugged from a SATA port, then further
> > hotplug notification are now longer received on this port.
> > 
> > This should be applied to every -stable kernel supporting Armada SoCs.
> > 
> > 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
> 
> I tested this on Kirkwood, which does not have any issues to
> (re)hotplug. Still works O.K. with this patch.

Hi Andrew,

Thanks for the tests.

> 
> My only reservation is that neither the Kirkwood nor Dove datasheet
> list the LP_PHY_CTL register. Are we now poking something which does
> not exist on these SoCs? Is that safe?

I will check that.

Simon

> 
> Tested-by: Andrew Lunn <andrew@lunn.ch>

> 
>     Andrew
> 
> 
> > ---
> >  drivers/ata/sata_mv.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> > index 56be318..89ca472 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,
> >  
> > @@ -1358,6 +1359,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;
> > +		void __iomem *lp_phy_addr = mv_ap_base(link->ap) + LP_PHY_CTL;
> >  		if (sc_reg_in == SCR_CONTROL) {
> >  			/*
> >  			 * Workaround for 88SX60x1 FEr SATA#26:
> > @@ -1374,6 +1376,14 @@ 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;
> > +
> > +			/*
> > +			 * Setting 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;
> > -- 
> > 1.8.5.1
> > 
-------------- 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/20131227/3d5ce7c8/attachment.sig>

Re: [PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Tejun Heo]

On Mon, Dec 23, 2013 at 01:07:35PM +0100, Simon Guinot wrote:
> @@ -1358,6 +1359,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;
> +		void __iomem *lp_phy_addr = mv_ap_base(link->ap) + LP_PHY_CTL;
>  		if (sc_reg_in == SCR_CONTROL) {
>  			/*
>  			 * Workaround for 88SX60x1 FEr SATA#26:
> @@ -1374,6 +1376,14 @@ 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;
> +
> +			/*
> +			 * Setting PHY speed according to SControl speed
> +			 */
> +			if ((val & 0xf0) == 0x10)
> +				writelfl(0x7, lp_phy_addr);
> +			else
> +				writelfl(0x227, lp_phy_addr);

Do we know that this is safe for all sata_mvs?  If other sata_mvs
haven't had the described issue, maybe this should be applied
selectively to the said soc?  I'd actually prefer to avoid such
conditionals but we need to confirm this is okay for other
implementations.

Thanks.

-- 
tejun

[PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Tejun Heo]

On Mon, Dec 23, 2013 at 01:07:35PM +0100, Simon Guinot wrote:
> @@ -1358,6 +1359,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;
> +		void __iomem *lp_phy_addr = mv_ap_base(link->ap) + LP_PHY_CTL;
>  		if (sc_reg_in == SCR_CONTROL) {
>  			/*
>  			 * Workaround for 88SX60x1 FEr SATA#26:
> @@ -1374,6 +1376,14 @@ 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;
> +
> +			/*
> +			 * Setting PHY speed according to SControl speed
> +			 */
> +			if ((val & 0xf0) == 0x10)
> +				writelfl(0x7, lp_phy_addr);
> +			else
> +				writelfl(0x227, lp_phy_addr);

Do we know that this is safe for all sata_mvs?  If other sata_mvs
haven't had the described issue, maybe this should be applied
selectively to the said soc?  I'd actually prefer to avoid such
conditionals but we need to confirm this is okay for other
implementations.

Thanks.

-- 
tejun

Re: [PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Andrew Lunn]

On Tue, Dec 31, 2013 at 07:12:14AM -0500, Tejun Heo wrote:
> On Mon, Dec 23, 2013 at 01:07:35PM +0100, Simon Guinot wrote:
> > @@ -1358,6 +1359,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;
> > +		void __iomem *lp_phy_addr = mv_ap_base(link->ap) + LP_PHY_CTL;
> >  		if (sc_reg_in == SCR_CONTROL) {
> >  			/*
> >  			 * Workaround for 88SX60x1 FEr SATA#26:
> > @@ -1374,6 +1376,14 @@ 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;
> > +
> > +			/*
> > +			 * Setting PHY speed according to SControl speed
> > +			 */
> > +			if ((val & 0xf0) == 0x10)
> > +				writelfl(0x7, lp_phy_addr);
> > +			else
> > +				writelfl(0x227, lp_phy_addr);
> 
> Do we know that this is safe for all sata_mvs?  If other sata_mvs
> haven't had the described issue, maybe this should be applied
> selectively to the said soc?  I'd actually prefer to avoid such
> conditionals but we need to confirm this is okay for other
> implementations.

Hi Tejun

I've tested on Kirkwood, and not had problems. But i agree with
you. We need somebody in Marvell to say this is safe with all sata_mv
variants.

Lior, can you comment?

Thanks
	Andrew

[PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Andrew Lunn]

On Tue, Dec 31, 2013 at 07:12:14AM -0500, Tejun Heo wrote:
> On Mon, Dec 23, 2013 at 01:07:35PM +0100, Simon Guinot wrote:
> > @@ -1358,6 +1359,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;
> > +		void __iomem *lp_phy_addr = mv_ap_base(link->ap) + LP_PHY_CTL;
> >  		if (sc_reg_in == SCR_CONTROL) {
> >  			/*
> >  			 * Workaround for 88SX60x1 FEr SATA#26:
> > @@ -1374,6 +1376,14 @@ 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;
> > +
> > +			/*
> > +			 * Setting PHY speed according to SControl speed
> > +			 */
> > +			if ((val & 0xf0) == 0x10)
> > +				writelfl(0x7, lp_phy_addr);
> > +			else
> > +				writelfl(0x227, lp_phy_addr);
> 
> Do we know that this is safe for all sata_mvs?  If other sata_mvs
> haven't had the described issue, maybe this should be applied
> selectively to the said soc?  I'd actually prefer to avoid such
> conditionals but we need to confirm this is okay for other
> implementations.

Hi Tejun

I've tested on Kirkwood, and not had problems. But i agree with
you. We need somebody in Marvell to say this is safe with all sata_mv
variants.

Lior, can you comment?

Thanks
	Andrew

RE: [PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Lior Amsalem]

Hi Andrew,

> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: Tuesday, December 31, 2013 7:05 PM
> 
> On Tue, Dec 31, 2013 at 07:12:14AM -0500, Tejun Heo wrote:
> > On Mon, Dec 23, 2013 at 01:07:35PM +0100, Simon Guinot wrote:
> > > @@ -1358,6 +1359,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;
> > > +		void __iomem *lp_phy_addr = mv_ap_base(link->ap) +
> LP_PHY_CTL;
> > >  		if (sc_reg_in == SCR_CONTROL) {
> > >  			/*
> > >  			 * Workaround for 88SX60x1 FEr SATA#26:
> > > @@ -1374,6 +1376,14 @@ 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;
> > > +
> > > +			/*
> > > +			 * Setting PHY speed according to SControl speed
> > > +			 */
> > > +			if ((val & 0xf0) == 0x10)
> > > +				writelfl(0x7, lp_phy_addr);
> > > +			else
> > > +				writelfl(0x227, lp_phy_addr);
> >
> > Do we know that this is safe for all sata_mvs?  If other sata_mvs
> > haven't had the described issue, maybe this should be applied
> > selectively to the said soc?  I'd actually prefer to avoid such
> > conditionals but we need to confirm this is okay for other
> > implementations.
> 
> Hi Tejun
> 
> I've tested on Kirkwood, and not had problems. But i agree with you. We
> need somebody in Marvell to say this is safe with all sata_mv variants.
> 
> Lior, can you comment?

This register (0x58) was introduced in a370/axp (with a new SATA PHY version).
In other SoCs the register does not exist.

Writing/reading the registers on A300/A310 will not cause any harm (I haven't
tried A510).
But, I think it's better to check if we're running on A370/AXP. (maybe with a new
compatibility string in DT?)

> 
> Thanks
> 	Andrew


Regards,
Lior Amsalem

[PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Lior Amsalem]

Hi Andrew,

> From: Andrew Lunn [mailto:andrew at lunn.ch]
> Sent: Tuesday, December 31, 2013 7:05 PM
> 
> On Tue, Dec 31, 2013 at 07:12:14AM -0500, Tejun Heo wrote:
> > On Mon, Dec 23, 2013 at 01:07:35PM +0100, Simon Guinot wrote:
> > > @@ -1358,6 +1359,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;
> > > +		void __iomem *lp_phy_addr = mv_ap_base(link->ap) +
> LP_PHY_CTL;
> > >  		if (sc_reg_in == SCR_CONTROL) {
> > >  			/*
> > >  			 * Workaround for 88SX60x1 FEr SATA#26:
> > > @@ -1374,6 +1376,14 @@ 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;
> > > +
> > > +			/*
> > > +			 * Setting PHY speed according to SControl speed
> > > +			 */
> > > +			if ((val & 0xf0) == 0x10)
> > > +				writelfl(0x7, lp_phy_addr);
> > > +			else
> > > +				writelfl(0x227, lp_phy_addr);
> >
> > Do we know that this is safe for all sata_mvs?  If other sata_mvs
> > haven't had the described issue, maybe this should be applied
> > selectively to the said soc?  I'd actually prefer to avoid such
> > conditionals but we need to confirm this is okay for other
> > implementations.
> 
> Hi Tejun
> 
> I've tested on Kirkwood, and not had problems. But i agree with you. We
> need somebody in Marvell to say this is safe with all sata_mv variants.
> 
> Lior, can you comment?

This register (0x58) was introduced in a370/axp (with a new SATA PHY version).
In other SoCs the register does not exist.

Writing/reading the registers on A300/A310 will not cause any harm (I haven't
tried A510).
But, I think it's better to check if we're running on A370/AXP. (maybe with a new
compatibility string in DT?)

> 
> Thanks
> 	Andrew


Regards,
Lior Amsalem

Re: [PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Jason Cooper]

Lior, Simon,

On Wed, Jan 08, 2014 at 03:45:31PM +0200, Lior Amsalem wrote:
> Hi Andrew,
> 
> > From: Andrew Lunn [mailto:andrew@lunn.ch]
> > Sent: Tuesday, December 31, 2013 7:05 PM
> > 
> > On Tue, Dec 31, 2013 at 07:12:14AM -0500, Tejun Heo wrote:
> > > On Mon, Dec 23, 2013 at 01:07:35PM +0100, Simon Guinot wrote:
> > > > @@ -1358,6 +1359,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;
> > > > +		void __iomem *lp_phy_addr = mv_ap_base(link->ap) +
> > LP_PHY_CTL;
> > > >  		if (sc_reg_in == SCR_CONTROL) {
> > > >  			/*
> > > >  			 * Workaround for 88SX60x1 FEr SATA#26:
> > > > @@ -1374,6 +1376,14 @@ 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;
> > > > +
> > > > +			/*
> > > > +			 * Setting PHY speed according to SControl speed
> > > > +			 */
> > > > +			if ((val & 0xf0) == 0x10)
> > > > +				writelfl(0x7, lp_phy_addr);
> > > > +			else
> > > > +				writelfl(0x227, lp_phy_addr);
> > >
> > > Do we know that this is safe for all sata_mvs?  If other sata_mvs
> > > haven't had the described issue, maybe this should be applied
> > > selectively to the said soc?  I'd actually prefer to avoid such
> > > conditionals but we need to confirm this is okay for other
> > > implementations.
> > 
> > Hi Tejun
> > 
> > I've tested on Kirkwood, and not had problems. But i agree with you. We
> > need somebody in Marvell to say this is safe with all sata_mv variants.
> > 
> > Lior, can you comment?
> 
> This register (0x58) was introduced in a370/axp (with a new SATA PHY version).
> In other SoCs the register does not exist.
> 
> Writing/reading the registers on A300/A310 will not cause any harm (I haven't
> tried A510).
> But, I think it's better to check if we're running on A370/AXP. (maybe with a new
> compatibility string in DT?)

Lior, thanks for the clarification.  Simon, care to respin this with a
check for "marvell,armada-370-xp" root compatible string?  It should be
safe to say that if there is no DT, don't write the register.

Alternatively, we could do as Lior suggests, and create a new sata
compatible string.  But I think that is overkill.

Also, I'm growing more leery creating compatible strings for IP blocks
which are tied to the SoC revision.  If the IP block doesn't get issued
it's own version number/codename, we should just use the root compatible
strings to determine which SoC we are on.  I'll expand on this though as
I get caught up with Gregory's series's. 

thx,

Jason.

[PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Jason Cooper]

Lior, Simon,

On Wed, Jan 08, 2014 at 03:45:31PM +0200, Lior Amsalem wrote:
> Hi Andrew,
> 
> > From: Andrew Lunn [mailto:andrew at lunn.ch]
> > Sent: Tuesday, December 31, 2013 7:05 PM
> > 
> > On Tue, Dec 31, 2013 at 07:12:14AM -0500, Tejun Heo wrote:
> > > On Mon, Dec 23, 2013 at 01:07:35PM +0100, Simon Guinot wrote:
> > > > @@ -1358,6 +1359,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;
> > > > +		void __iomem *lp_phy_addr = mv_ap_base(link->ap) +
> > LP_PHY_CTL;
> > > >  		if (sc_reg_in == SCR_CONTROL) {
> > > >  			/*
> > > >  			 * Workaround for 88SX60x1 FEr SATA#26:
> > > > @@ -1374,6 +1376,14 @@ 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;
> > > > +
> > > > +			/*
> > > > +			 * Setting PHY speed according to SControl speed
> > > > +			 */
> > > > +			if ((val & 0xf0) == 0x10)
> > > > +				writelfl(0x7, lp_phy_addr);
> > > > +			else
> > > > +				writelfl(0x227, lp_phy_addr);
> > >
> > > Do we know that this is safe for all sata_mvs?  If other sata_mvs
> > > haven't had the described issue, maybe this should be applied
> > > selectively to the said soc?  I'd actually prefer to avoid such
> > > conditionals but we need to confirm this is okay for other
> > > implementations.
> > 
> > Hi Tejun
> > 
> > I've tested on Kirkwood, and not had problems. But i agree with you. We
> > need somebody in Marvell to say this is safe with all sata_mv variants.
> > 
> > Lior, can you comment?
> 
> This register (0x58) was introduced in a370/axp (with a new SATA PHY version).
> In other SoCs the register does not exist.
> 
> Writing/reading the registers on A300/A310 will not cause any harm (I haven't
> tried A510).
> But, I think it's better to check if we're running on A370/AXP. (maybe with a new
> compatibility string in DT?)

Lior, thanks for the clarification.  Simon, care to respin this with a
check for "marvell,armada-370-xp" root compatible string?  It should be
safe to say that if there is no DT, don't write the register.

Alternatively, we could do as Lior suggests, and create a new sata
compatible string.  But I think that is overkill.

Also, I'm growing more leery creating compatible strings for IP blocks
which are tied to the SoC revision.  If the IP block doesn't get issued
it's own version number/codename, we should just use the root compatible
strings to determine which SoC we are on.  I'll expand on this though as
I get caught up with Gregory's series's. 

thx,

Jason.

Re: [PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Thomas Petazzoni]

Dear Jason Cooper,

On Fri, 10 Jan 2014 12:44:12 -0500, Jason Cooper wrote:

> Lior, thanks for the clarification.  Simon, care to respin this with a
> check for "marvell,armada-370-xp" root compatible string?  It should be
> safe to say that if there is no DT, don't write the register.

Why check a root compatible string? If we do this, then we will have to
change the SATA driver for each and every new Marvell SoC that has this
PHY speed control register (and these new SOCs will not use the
"marvell,armada-370-xp" root compatible string, since they are clearly
not Armada 370/XP).

Instead, we should introduce an additional compatible string for the
SATA driver itself.

> Alternatively, we could do as Lior suggests, and create a new sata
> compatible string.  But I think that is overkill.

No, this is the right thing to do, IMO.

> Also, I'm growing more leery creating compatible strings for IP blocks
> which are tied to the SoC revision.  If the IP block doesn't get issued
> it's own version number/codename, we should just use the root compatible
> strings to determine which SoC we are on.  I'll expand on this though as
> I get caught up with Gregory's series's. 

I really disagree. It means that whenever a new root compatible string
is created for a new SOC, we will have to edit gazillions of drivers.
It's not because two SOCs have the same SATA IP that they are globally
compatible, and can therefore share the same root compatible strings.

Best regards,

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

[PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Thomas Petazzoni]

Dear Jason Cooper,

On Fri, 10 Jan 2014 12:44:12 -0500, Jason Cooper wrote:

> Lior, thanks for the clarification.  Simon, care to respin this with a
> check for "marvell,armada-370-xp" root compatible string?  It should be
> safe to say that if there is no DT, don't write the register.

Why check a root compatible string? If we do this, then we will have to
change the SATA driver for each and every new Marvell SoC that has this
PHY speed control register (and these new SOCs will not use the
"marvell,armada-370-xp" root compatible string, since they are clearly
not Armada 370/XP).

Instead, we should introduce an additional compatible string for the
SATA driver itself.

> Alternatively, we could do as Lior suggests, and create a new sata
> compatible string.  But I think that is overkill.

No, this is the right thing to do, IMO.

> Also, I'm growing more leery creating compatible strings for IP blocks
> which are tied to the SoC revision.  If the IP block doesn't get issued
> it's own version number/codename, we should just use the root compatible
> strings to determine which SoC we are on.  I'll expand on this though as
> I get caught up with Gregory's series's. 

I really disagree. It means that whenever a new root compatible string
is created for a new SOC, we will have to edit gazillions of drivers.
It's not because two SOCs have the same SATA IP that they are globally
compatible, and can therefore share the same root compatible strings.

Best regards,

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

Re: [PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Jason Cooper]

On Sat, Jan 11, 2014 at 07:55:57AM +0800, Thomas Petazzoni wrote:
> Dear Jason Cooper,
> 
> On Fri, 10 Jan 2014 12:44:12 -0500, Jason Cooper wrote:
> 
> > Lior, thanks for the clarification.  Simon, care to respin this with a
> > check for "marvell,armada-370-xp" root compatible string?  It should be
> > safe to say that if there is no DT, don't write the register.
> 
> Why check a root compatible string? If we do this, then we will have to
> change the SATA driver for each and every new Marvell SoC that has this
> PHY speed control register (and these new SOCs will not use the
> "marvell,armada-370-xp" root compatible string, since they are clearly
> not Armada 370/XP).
> 
> Instead, we should introduce an additional compatible string for the
> SATA driver itself.

Agreed.  

> > Alternatively, we could do as Lior suggests, and create a new sata
> > compatible string.  But I think that is overkill.
> 
> No, this is the right thing to do, IMO.
> 
> > Also, I'm growing more leery creating compatible strings for IP blocks
> > which are tied to the SoC revision.  If the IP block doesn't get issued
> > it's own version number/codename, we should just use the root compatible
> > strings to determine which SoC we are on.  I'll expand on this though as
> > I get caught up with Gregory's series's. 
> 
> I really disagree. It means that whenever a new root compatible string
> is created for a new SOC, we will have to edit gazillions of drivers.
> It's not because two SOCs have the same SATA IP that they are globally
> compatible, and can therefore share the same root compatible strings.

Yes, you're right.  I really went off the deep end on that one.  Simon,
please do as Lior and Thomas suggested.

thx,

Jason.

[PATCH] ata: sata_mv: setting PHY speed according to SControl speed

[Jason Cooper]

On Sat, Jan 11, 2014 at 07:55:57AM +0800, Thomas Petazzoni wrote:
> Dear Jason Cooper,
> 
> On Fri, 10 Jan 2014 12:44:12 -0500, Jason Cooper wrote:
> 
> > Lior, thanks for the clarification.  Simon, care to respin this with a
> > check for "marvell,armada-370-xp" root compatible string?  It should be
> > safe to say that if there is no DT, don't write the register.
> 
> Why check a root compatible string? If we do this, then we will have to
> change the SATA driver for each and every new Marvell SoC that has this
> PHY speed control register (and these new SOCs will not use the
> "marvell,armada-370-xp" root compatible string, since they are clearly
> not Armada 370/XP).
> 
> Instead, we should introduce an additional compatible string for the
> SATA driver itself.

Agreed.  

> > Alternatively, we could do as Lior suggests, and create a new sata
> > compatible string.  But I think that is overkill.
> 
> No, this is the right thing to do, IMO.
> 
> > Also, I'm growing more leery creating compatible strings for IP blocks
> > which are tied to the SoC revision.  If the IP block doesn't get issued
> > it's own version number/codename, we should just use the root compatible
> > strings to determine which SoC we are on.  I'll expand on this though as
> > I get caught up with Gregory's series's. 
> 
> I really disagree. It means that whenever a new root compatible string
> is created for a new SOC, we will have to edit gazillions of drivers.
> It's not because two SOCs have the same SATA IP that they are globally
> compatible, and can therefore share the same root compatible strings.

Yes, you're right.  I really went off the deep end on that one.  Simon,
please do as Lior and Thomas suggested.

thx,

Jason.