| Message ID | 1387800455-30629-1-git-send-email-simon.guinot@sequanux.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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
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
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
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
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
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 >
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.
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 > >
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.
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
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
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.
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
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.
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;