* [PATCH] ata: sata_mv: setting PHY speed according to SControl speed
@ 2013-12-23 12:07 ` Simon Guinot
0 siblings, 0 replies; 32+ messages in thread
From: Simon Guinot @ 2013-12-23 12:07 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH] ata: sata_mv: setting PHY speed according to SControl speed
2013-12-23 12:07 ` Simon Guinot
@ 2013-12-24 19:46 ` Jason Cooper
-1 siblings, 0 replies; 32+ messages in thread
From: Jason Cooper @ 2013-12-24 19:46 UTC (permalink / raw)
To: Simon Guinot
Cc: Jeff Garzik, Tejun Heo, linux-ide, linux-arm-kernel, Lior Amsalem,
Thomas Petazzoni, Andrew Lunn, Gregory Clement,
Sebastian Hesselbarth, stable
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
>
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH] ata: sata_mv: setting PHY speed according to SControl speed
@ 2013-12-24 19:46 ` Jason Cooper
0 siblings, 0 replies; 32+ messages in thread
From: Jason Cooper @ 2013-12-24 19:46 UTC (permalink / raw)
To: linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH] ata: sata_mv: setting PHY speed according to SControl speed
2013-12-24 19:46 ` Jason Cooper
@ 2013-12-25 16:41 ` Andrew Lunn
-1 siblings, 0 replies; 32+ messages in thread
From: Andrew Lunn @ 2013-12-25 16:41 UTC (permalink / raw)
To: Jason Cooper
Cc: Simon Guinot, Jeff Garzik, Tejun Heo, linux-ide, linux-arm-kernel,
Lior Amsalem, Thomas Petazzoni, Andrew Lunn, Gregory Clement,
Sebastian Hesselbarth, stable
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH] ata: sata_mv: setting PHY speed according to SControl speed
@ 2013-12-25 16:41 ` Andrew Lunn
0 siblings, 0 replies; 32+ messages in thread
From: Andrew Lunn @ 2013-12-25 16:41 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] ata: sata_mv: setting PHY speed according to SControl speed
2013-12-24 19:46 ` Jason Cooper
@ 2013-12-25 22:40 ` Simon Guinot
-1 siblings, 0 replies; 32+ messages in thread
From: Simon Guinot @ 2013-12-25 22:40 UTC (permalink / raw)
To: Jason Cooper
Cc: Lior Amsalem, Thomas Petazzoni, Andrew Lunn, stable, linux-ide,
Tejun Heo, Gregory Clement, Jeff Garzik, linux-arm-kernel,
Sebastian Hesselbarth
[-- 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 --]
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH] ata: sata_mv: setting PHY speed according to SControl speed
@ 2013-12-25 22:40 ` Simon Guinot
0 siblings, 0 replies; 32+ messages in thread
From: Simon Guinot @ 2013-12-25 22:40 UTC (permalink / raw)
To: linux-arm-kernel
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>
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH] ata: sata_mv: setting PHY speed according to SControl speed
2013-12-25 22:40 ` Simon Guinot
@ 2013-12-26 7:53 ` Thomas Petazzoni
-1 siblings, 0 replies; 32+ messages in thread
From: Thomas Petazzoni @ 2013-12-26 7:53 UTC (permalink / raw)
To: Simon Guinot
Cc: Jason Cooper, Lior Amsalem, Andrew Lunn, stable, linux-ide,
Tejun Heo, Gregory Clement, Jeff Garzik, linux-arm-kernel,
Sebastian Hesselbarth
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH] ata: sata_mv: setting PHY speed according to SControl speed
@ 2013-12-26 7:53 ` Thomas Petazzoni
0 siblings, 0 replies; 32+ messages in thread
From: Thomas Petazzoni @ 2013-12-26 7:53 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] ata: sata_mv: setting PHY speed according to SControl speed
2013-12-26 7:53 ` Thomas Petazzoni
@ 2013-12-26 11:54 ` Simon Guinot
-1 siblings, 0 replies; 32+ messages in thread
From: Simon Guinot @ 2013-12-26 11:54 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: Jason Cooper, Lior Amsalem, Andrew Lunn, stable, linux-ide,
Tejun Heo, Gregory Clement, Jeff Garzik, linux-arm-kernel,
Sebastian Hesselbarth
[-- 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 --]
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH] ata: sata_mv: setting PHY speed according to SControl speed
@ 2013-12-26 11:54 ` Simon Guinot
0 siblings, 0 replies; 32+ messages in thread
From: Simon Guinot @ 2013-12-26 11:54 UTC (permalink / raw)
To: linux-arm-kernel
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>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] ata: sata_mv: setting PHY speed according to SControl speed
2013-12-26 11:54 ` Simon Guinot
@ 2013-12-26 13:34 ` Thomas Petazzoni
-1 siblings, 0 replies; 32+ messages in thread
From: Thomas Petazzoni @ 2013-12-26 13:34 UTC (permalink / raw)
To: Simon Guinot
Cc: Jason Cooper, Lior Amsalem, Andrew Lunn, stable, linux-ide,
Tejun Heo, Gregory Clement, Jeff Garzik, linux-arm-kernel,
Sebastian Hesselbarth
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH] ata: sata_mv: setting PHY speed according to SControl speed
@ 2013-12-26 13:34 ` Thomas Petazzoni
0 siblings, 0 replies; 32+ messages in thread
From: Thomas Petazzoni @ 2013-12-26 13:34 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] ata: sata_mv: setting PHY speed according to SControl speed
2013-12-25 22:40 ` Simon Guinot
@ 2013-12-27 15:49 ` Jason Cooper
-1 siblings, 0 replies; 32+ messages in thread
From: Jason Cooper @ 2013-12-27 15:49 UTC (permalink / raw)
To: Simon Guinot
Cc: Lior Amsalem, Thomas Petazzoni, Andrew Lunn, stable, linux-ide,
Tejun Heo, Gregory Clement, Jeff Garzik, linux-arm-kernel,
Sebastian Hesselbarth
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.
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH] ata: sata_mv: setting PHY speed according to SControl speed
@ 2013-12-27 15:49 ` Jason Cooper
0 siblings, 0 replies; 32+ messages in thread
From: Jason Cooper @ 2013-12-27 15:49 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] ata: sata_mv: setting PHY speed according to SControl speed
2013-12-23 12:07 ` Simon Guinot
@ 2013-12-26 18:01 ` Andrew Lunn
-1 siblings, 0 replies; 32+ messages in thread
From: Andrew Lunn @ 2013-12-26 18:01 UTC (permalink / raw)
To: Simon Guinot
Cc: Jeff Garzik, Tejun Heo, linux-ide, linux-arm-kernel, Lior Amsalem,
Thomas Petazzoni, Jason Cooper, Andrew Lunn, Gregory Clement,
Sebastian Hesselbarth, stable
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
>
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH] ata: sata_mv: setting PHY speed according to SControl speed
@ 2013-12-26 18:01 ` Andrew Lunn
0 siblings, 0 replies; 32+ messages in thread
From: Andrew Lunn @ 2013-12-26 18:01 UTC (permalink / raw)
To: linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH] ata: sata_mv: setting PHY speed according to SControl speed
2013-12-26 18:01 ` Andrew Lunn
@ 2013-12-27 17:37 ` Simon Guinot
-1 siblings, 0 replies; 32+ messages in thread
From: Simon Guinot @ 2013-12-27 17:37 UTC (permalink / raw)
To: Andrew Lunn
Cc: Jeff Garzik, Tejun Heo, linux-ide, linux-arm-kernel, Lior Amsalem,
Thomas Petazzoni, Jason Cooper, Gregory Clement,
Sebastian Hesselbarth, stable
[-- 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 --]
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH] ata: sata_mv: setting PHY speed according to SControl speed
@ 2013-12-27 17:37 ` Simon Guinot
0 siblings, 0 replies; 32+ messages in thread
From: Simon Guinot @ 2013-12-27 17:37 UTC (permalink / raw)
To: linux-arm-kernel
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>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] ata: sata_mv: setting PHY speed according to SControl speed
2013-12-23 12:07 ` Simon Guinot
@ 2013-12-31 12:12 ` Tejun Heo
-1 siblings, 0 replies; 32+ messages in thread
From: Tejun Heo @ 2013-12-31 12:12 UTC (permalink / raw)
To: Simon Guinot
Cc: Jeff Garzik, linux-ide, linux-arm-kernel, Lior Amsalem,
Thomas Petazzoni, Jason Cooper, Andrew Lunn, Gregory Clement,
Sebastian Hesselbarth, stable
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH] ata: sata_mv: setting PHY speed according to SControl speed
@ 2013-12-31 12:12 ` Tejun Heo
0 siblings, 0 replies; 32+ messages in thread
From: Tejun Heo @ 2013-12-31 12:12 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH] ata: sata_mv: setting PHY speed according to SControl speed
2013-12-31 12:12 ` Tejun Heo
@ 2013-12-31 17:05 ` Andrew Lunn
-1 siblings, 0 replies; 32+ messages in thread
From: Andrew Lunn @ 2013-12-31 17:05 UTC (permalink / raw)
To: Tejun Heo
Cc: Simon Guinot, Jeff Garzik, linux-ide, linux-arm-kernel,
Lior Amsalem, Thomas Petazzoni, Jason Cooper, Andrew Lunn,
Gregory Clement, Sebastian Hesselbarth, stable
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH] ata: sata_mv: setting PHY speed according to SControl speed
@ 2013-12-31 17:05 ` Andrew Lunn
0 siblings, 0 replies; 32+ messages in thread
From: Andrew Lunn @ 2013-12-31 17:05 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 32+ messages in thread* RE: [PATCH] ata: sata_mv: setting PHY speed according to SControl speed
2013-12-31 17:05 ` Andrew Lunn
@ 2014-01-08 13:45 ` Lior Amsalem
-1 siblings, 0 replies; 32+ messages in thread
From: Lior Amsalem @ 2014-01-08 13:45 UTC (permalink / raw)
To: Andrew Lunn, Tejun Heo
Cc: Simon Guinot, Jeff Garzik, linux-ide@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Thomas Petazzoni,
Jason Cooper, Gregory Clement, Sebastian Hesselbarth,
stable@vger.kernel.org
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH] ata: sata_mv: setting PHY speed according to SControl speed
@ 2014-01-08 13:45 ` Lior Amsalem
0 siblings, 0 replies; 32+ messages in thread
From: Lior Amsalem @ 2014-01-08 13:45 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH] ata: sata_mv: setting PHY speed according to SControl speed
2014-01-08 13:45 ` Lior Amsalem
@ 2014-01-10 17:44 ` Jason Cooper
-1 siblings, 0 replies; 32+ messages in thread
From: Jason Cooper @ 2014-01-10 17:44 UTC (permalink / raw)
To: Lior Amsalem
Cc: Andrew Lunn, Tejun Heo, Thomas Petazzoni, stable@vger.kernel.org,
linux-ide@vger.kernel.org, Gregory Clement, Sebastian Hesselbarth,
Jeff Garzik, linux-arm-kernel@lists.infradead.org, Simon Guinot
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.
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH] ata: sata_mv: setting PHY speed according to SControl speed
@ 2014-01-10 17:44 ` Jason Cooper
0 siblings, 0 replies; 32+ messages in thread
From: Jason Cooper @ 2014-01-10 17:44 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH] ata: sata_mv: setting PHY speed according to SControl speed
2014-01-10 17:44 ` Jason Cooper
@ 2014-01-10 23:55 ` Thomas Petazzoni
-1 siblings, 0 replies; 32+ messages in thread
From: Thomas Petazzoni @ 2014-01-10 23:55 UTC (permalink / raw)
To: Jason Cooper
Cc: Lior Amsalem, Andrew Lunn, Tejun Heo, stable@vger.kernel.org,
linux-ide@vger.kernel.org, Gregory Clement, Sebastian Hesselbarth,
Jeff Garzik, linux-arm-kernel@lists.infradead.org, Simon Guinot
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH] ata: sata_mv: setting PHY speed according to SControl speed
@ 2014-01-10 23:55 ` Thomas Petazzoni
0 siblings, 0 replies; 32+ messages in thread
From: Thomas Petazzoni @ 2014-01-10 23:55 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH] ata: sata_mv: setting PHY speed according to SControl speed
2014-01-10 23:55 ` Thomas Petazzoni
@ 2014-01-13 14:36 ` Jason Cooper
-1 siblings, 0 replies; 32+ messages in thread
From: Jason Cooper @ 2014-01-13 14:36 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: Lior Amsalem, Andrew Lunn, Tejun Heo, stable@vger.kernel.org,
linux-ide@vger.kernel.org, Gregory Clement, Sebastian Hesselbarth,
Jeff Garzik, linux-arm-kernel@lists.infradead.org, Simon Guinot
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.
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH] ata: sata_mv: setting PHY speed according to SControl speed
@ 2014-01-13 14:36 ` Jason Cooper
0 siblings, 0 replies; 32+ messages in thread
From: Jason Cooper @ 2014-01-13 14:36 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 32+ messages in thread