ata: sata_mv: setting PHY speed according to SControl speed
PATCH 1/1 ata: sata_mv: setting PHY speed according to SControl speed
From: Lior Amsalem <[email protected]>
From: Lior Amsalem <[email protected]>
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.
--- drivers/ata/sata_mv.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
drivers/ata/sata_mv.c +10 -0
@@ -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 ;
Signature · 1 line
1.8.5.1
PATCH 1/1 ata: sata_mv: setting PHY speed according to SControl speed
Quoted 9 lines · Simon Guinot
From: Lior Amsalem <[email protected]>
From: Lior Amsalem <[email protected]>
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.
Quoted 52 lines
Signed-off-by Lior Amsalem <[email protected]>Signed-off-by Nadav Haklai <[email protected]>Signed-off-by Simon Guinot <[email protected]>Cc Thomas Petazzoni <[email protected]>Cc Jason Cooper <[email protected]>Cc Andrew Lunn <[email protected]>Cc Gregory Clement <[email protected]>Cc Sebastian Hesselbarth <[email protected]>Cc stable at vger.kernel.org--- drivers/ata/sata_mv.c | 10 ++++++++++ 1 file changed, 10 insertions(+)drivers/ata/sata_mv.c +10
-0@@ -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 ;Signature · 1 line
1.8.5.1
PATCH 1/1 ata: sata_mv: setting PHY speed according to SControl speed
Quoted 9 lines · Jason Cooper
Quoted 8 lines · Simon Guinot
From: Lior Amsalem <[email protected]>
From: Lior Amsalem <[email protected]>
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.
Quoted 8 lines
Quoted 1 line
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_mvdriver predates the Armada SoCs introduction. Is it possible Kirkwood et al also experience this problem?
Hi Jason
It is on my TODO list to try this out on Kirkwood and Dove. I'm pretty sure replug works on Dove without this, not tried Kirkwood yet.
Andrew
PATCH 1/1 ata: sata_mv: setting PHY speed according to SControl speed
In reply to Jason CooperQuoted 5 lines · Jason Cooper
Quoted 4 lines · Simon Guinot
From: Lior Amsalem <[email protected]>
From: Lior Amsalem <[email protected]>
Quoted 8 lines
Quoted 5 lines
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.
Quoted 4 lines
It's really needed here since the
sata_mvdriver 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
Quoted 54 lines
thx,
Jason.
Quoted 49 lines
Signed-off-by Lior Amsalem <[email protected]>Signed-off-by Nadav Haklai <[email protected]>Signed-off-by Simon Guinot <[email protected]>Cc Thomas Petazzoni <[email protected]>Cc Jason Cooper <[email protected]>Cc Andrew Lunn <[email protected]>Cc Gregory Clement <[email protected]>Cc Sebastian Hesselbarth <[email protected]>Cc stable at vger.kernel.org--- drivers/ata/sata_mv.c | 10 ++++++++++ 1 file changed, 10 insertions(+)drivers/ata/sata_mv.c +10
-0@@ -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 ;
Quoted 8 lines
Quoted 3 lines
Signature · 1 line
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>
PATCH 1/1 ata: sata_mv: setting PHY speed according to SControl speed
Dear Simon Guinot,
Quoted 7 lines · Simon Guinot
Quoted 6 lines
Quoted 2 lines
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"'
Quoted 8 lines
Well, since the DT support for the
sata_mvdriver 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
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
PATCH 1/1 ata: sata_mv: setting PHY speed according to SControl speed
Quoted 20 lines · Thomas Petazzoni
Dear Simon Guinot,
Quoted 7 lines · Simon Guinot
Quoted 6 lines
Quoted 2 lines
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"'Quoted 8 lines
Well, since the DT support for the
sata_mvdriver 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.
Quoted 6 lines
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>
PATCH 1/1 ata: sata_mv: setting PHY speed according to SControl speed
Dear Simon Guinot,
Quoted 11 lines · Simon Guinot
Quoted 6 lines
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
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
PATCH 1/1 ata: sata_mv: setting PHY speed according to SControl speed
In reply to Simon GuinotSimon,
Quoted 12 lines · Simon Guinot
Quoted 11 lines · Jason Cooper
Quoted 9 lines · Simon Guinot
From: Lior Amsalem <[email protected]>
From: Lior Amsalem <[email protected]>
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.
Quoted 7 lines
Quoted 2 lines
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_mvdriver 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'
Quoted 2 lines
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.
Quoted 5 lines
Quoted 3 lines
It's really needed here since the
sata_mvdriver predates the Armada SoCs introduction. Is it possible Kirkwood et al also experience this problem?On my Orion and Kirkwood based bords, SATA disk hotplug works correctly.
Good, one less thing to worry about.
thx,
Jason.
PATCH 1/1 ata: sata_mv: setting PHY speed according to SControl speed
In reply to Simon GuinotQuoted 10 lines · Simon Guinot
From: Lior Amsalem <[email protected]>
From: Lior Amsalem <[email protected]>
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.
Quoted 9 lines
Signed-off-by Lior Amsalem <[email protected]>Signed-off-by Nadav Haklai <[email protected]>Signed-off-by Simon Guinot <[email protected]>Cc Thomas Petazzoni <[email protected]>Cc Jason Cooper <[email protected]>Cc Andrew Lunn <[email protected]>Cc Gregory Clement <[email protected]>Cc Sebastian Hesselbarth <[email protected]>Cc 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?
Andrew
Quoted 42 lines
--- drivers/ata/sata_mv.c | 10 ++++++++++ 1 file changed, 10 insertions(+)drivers/ata/sata_mv.c +10
-0@@ -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 ;Signature · 1 line
1.8.5.1
PATCH 1/1 ata: sata_mv: setting PHY speed according to SControl speed
Quoted 21 lines · Andrew Lunn
Quoted 10 lines · Simon Guinot
From: Lior Amsalem <[email protected]>
From: Lior Amsalem <[email protected]>
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.
Quoted 9 lines
Signed-off-by Lior Amsalem <[email protected]>Signed-off-by Nadav Haklai <[email protected]>Signed-off-by Simon Guinot <[email protected]>Cc Thomas Petazzoni <[email protected]>Cc Jason Cooper <[email protected]>Cc Andrew Lunn <[email protected]>Cc Gregory Clement <[email protected]>Cc Sebastian Hesselbarth <[email protected]>Cc stable at vger.kernel.org
Quoted 2 lines
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.
Quoted 4 lines
My only reservation is that neither the Kirkwood nor Dove datasheet list the
LP_PHY_CTLregister. Are we now poking something which does not exist on these SoCs? Is that safe?
I will check that.
Simon
Quoted 41 lines
Tested-by Andrew Lunn <[email protected]>Andrew
Quoted 34 lines
--- drivers/ata/sata_mv.c | 10 ++++++++++ 1 file changed, 10 insertions(+)drivers/ata/sata_mv.c +8
-0@@ -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);
Quoted 8 lines
Quoted 8 lines
drivers/ata/sata_mv.c +2
-0+else +writelfl (0x227 , lp_phy_addr); }writelfl (val, addr);return 0 ;Signature · 1 line
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>
PATCH 1/1 ata: sata_mv: setting PHY speed according to SControl speed
In reply to Simon GuinotQuoted 10 lines · Simon Guinot
drivers/ata/sata_mv.c +1
-0@@ -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)*/
drivers/ata/sata_mv.c static int mv_scr_write(struct ata_link *link, unsigned int sc_reg_in, u32 val) · line 1386
Quoted 10 lines
drivers/ata/sata_mv.c +8
-0if ((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.
Signature · 1 line
tejun
PATCH 1/1 ata: sata_mv: setting PHY speed according to SControl speed
Quoted 19 lines · Tejun Heo
Quoted 18 lines · Simon Guinot
drivers/ata/sata_mv.c +7
-0@@ -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);
Quoted 8 lines
drivers/ata/sata_mv.c static int mv_scr_write(struct ata_link *link, unsigned int sc_reg_in, u32 val) · line 1386
Quoted 2 lines
drivers/ata/sata_mv.c +2
-0+else +writelfl (0x227 , lp_phy_addr);Do we know that this is safe for all
sata_mvs? If othersata_mvshaven't had the described issue, maybe this should be applied selectively to the said soc? I'd actually prefer to avoid such conditionals but we need to confirm this is okay for other implementations.
Hi Tejun
I've tested on Kirkwood, and not had problems. But i agree with you. We need somebody in Marvell to say this is safe with all sata_mv variants.
Lior, can you comment?
Thanks
Andrew
PATCH 1/1 ata: sata_mv: setting PHY speed according to SControl speed
Hi Andrew,
Quoted 35 lines
From: Andrew Lunn [mailto:andrew at lunn.ch]
Sent: Tuesday, December 31, 2013 7:05 PMQuoted 7 lines · Tejun Heo
drivers/ata/sata_mv.c static int mv_scr_write(struct ata_link *link, · line 1363
Quoted 6 lines · Simon Guinot
drivers/ata/sata_mv.c +1
-0@@ -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;Quoted 4 lines
drivers/ata/sata_mv.c static int mv_scr_write(struct ata_link *link,
Quoted 4 lines
drivers/ata/sata_mv.c +0
-0if (sc_reg_in ==SCR_CONTROL ) {/* drivers/ata/sata_mv.c +0
-0* 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)Quoted 9 lines
Quoted 9 lines
drivers/ata/sata_mv.c +6
-0*/ 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);Quoted 8 lines
drivers/ata/sata_mv.c static int mv_scr_write(struct ata_link *link, unsigned int sc_reg_in, u32 val) · line 1386
Quoted 2 lines
drivers/ata/sata_mv.c +2
-0+else +writelfl (0x227 , lp_phy_addr);Do we know that this is safe for all
sata_mvs? If othersata_mvshaven'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.
Quoted 6 lines
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_mvvariants.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?)
Quoted 3 lines
Thanks
Andrew
Regards,
Lior Amsalem
PATCH 1/1 ata: sata_mv: setting PHY speed according to SControl speed
Lior, Simon,
Quoted 44 lines · Lior Amsalem
Hi Andrew,
Quoted 35 lines
From: Andrew Lunn [mailto:andrew at lunn.ch]
Sent: Tuesday, December 31, 2013 7:05 PMQuoted 7 lines · Tejun Heo
drivers/ata/sata_mv.c static int mv_scr_write(struct ata_link *link, · line 1363
Quoted 6 lines · Simon Guinot
drivers/ata/sata_mv.c +1
-0@@ -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;Quoted 4 lines
drivers/ata/sata_mv.c static int mv_scr_write(struct ata_link *link,
Quoted 4 lines
drivers/ata/sata_mv.c +0
-0if (sc_reg_in ==SCR_CONTROL ) {/* drivers/ata/sata_mv.c +0
-0* 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)Quoted 9 lines
Quoted 9 lines
drivers/ata/sata_mv.c +6
-0*/ 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);Quoted 8 lines
drivers/ata/sata_mv.c static int mv_scr_write(struct ata_link *link, unsigned int sc_reg_in, u32 val) · line 1386
Quoted 2 lines
drivers/ata/sata_mv.c +2
-0+else +writelfl (0x227 , lp_phy_addr);Do we know that this is safe for all
sata_mvs? If othersata_mvshaven'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.Quoted 6 lines
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_mvvariants.Lior, can you comment?
Quoted 7 lines
This register (
0x58) was introduced in a370/axp (with a new SATA PHY version). In other SoCs the register does not exist.Writing/reading the registers on A300/A310 will not cause any harm (I haven't tried A510).
But, I think it's better to check if we're running on A370/AXP. (maybe with a new compatibility string in DT?)
Lior, thanks for the clarification. Simon, care to respin this with a check for "marvell,armada-370-xp" root compatible string? It should be safe to say that if there is no DT, don't write the register.
Alternatively, we could do as Lior suggests, and create a new sata compatible string. But I think that is overkill.
Also, I'm growing more leery creating compatible strings for IP blocks which are tied to the SoC revision. If the IP block doesn't get issued it's own version number/codename, we should just use the root compatible strings to determine which SoC we are on. I'll expand on this though as I get caught up with Gregory's series's.
thx,
Jason.
PATCH 1/1 ata: sata_mv: setting PHY speed according to SControl speed
Dear Jason Cooper,
Quoted 3 lines · Jason Cooper
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.
Quoted 2 lines
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.
Quoted 5 lines
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
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
PATCH 1/1 ata: sata_mv: setting PHY speed according to SControl speed
Quoted 8 lines · Thomas Petazzoni
Dear Jason Cooper,
Quoted 3 lines · Jason Cooper
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.
Quoted 8 lines
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.
Quoted 11 lines
Quoted 2 lines
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.
Quoted 5 lines
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.
Quoted 4 lines
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.