According to the R-Car Gen2/3 manual, the bit 0 of MACCTLR register should be written to 0 because the register is set to 1 on reset. To avoid unexpected behaviors from this incorrect setting, this patch fixes it.
Fixes: b3327f7fae66 ("PCI: rcar: Try increasing PCIe link speed to 5 GT/s at boot") Cc: stable@vger.kernel.org # v4.9+ Signed-off-by: Yoshihiro Shimoda yoshihiro.shimoda.uh@renesas.com Reviewed-by: Sergei Shtylyov sergei.shtylyov@cogentembedded.com --- Changes from v1: - Fix commit log. - Add Sergei-san's Reviewed-by. https://patchwork.kernel.org/patch/11179279/
drivers/pci/controller/pcie-rcar.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c index f6a669a..9eb9b25 100644 --- a/drivers/pci/controller/pcie-rcar.c +++ b/drivers/pci/controller/pcie-rcar.c @@ -93,6 +93,7 @@ #define LINK_SPEED_2_5GTS (1 << 16) #define LINK_SPEED_5_0GTS (2 << 16) #define MACCTLR 0x011058 +#define MACCTLR_RESERVED BIT(0) #define SPEED_CHANGE BIT(24) #define SCRAMBLE_DISABLE BIT(27) #define PMSR 0x01105c @@ -427,7 +428,8 @@ static void rcar_pcie_force_speedup(struct rcar_pcie *pcie) rcar_pci_write_reg(pcie, macsr, MACSR);
/* Start link speed change */ - rcar_rmw32(pcie, MACCTLR, SPEED_CHANGE, SPEED_CHANGE); + rcar_rmw32(pcie, MACCTLR, SPEED_CHANGE | MACCTLR_RESERVED, + SPEED_CHANGE);
while (timeout--) { macsr = rcar_pci_read_reg(pcie, MACSR);
On 09.10.2019 7:03, Yoshihiro Shimoda wrote:
According to the R-Car Gen2/3 manual, the bit 0 of MACCTLR register should be written to 0 because the register is set to 1 on reset.
The bit 0 set to 1, not the whole register (it has 1s also in the bits 16-23).
To avoid unexpected behaviors from this incorrect setting, this patch fixes it.
Fixes: b3327f7fae66 ("PCI: rcar: Try increasing PCIe link speed to 5 GT/s at boot") Cc: stable@vger.kernel.org # v4.9+ Signed-off-by: Yoshihiro Shimoda yoshihiro.shimoda.uh@renesas.com Reviewed-by: Sergei Shtylyov sergei.shtylyov@cogentembedded.com
[...]
MBR, Sergei
Hi Sergei-san,
From: Sergei Shtylyov, Sent: Wednesday, October 9, 2019 5:49 PM
On 09.10.2019 7:03, Yoshihiro Shimoda wrote:
According to the R-Car Gen2/3 manual, the bit 0 of MACCTLR register should be written to 0 because the register is set to 1 on reset.
The bit 0 set to 1, not the whole register (it has 1s also in thebits 16-23).
Thank you for the comment. So, I'll fix the commit log as following. Is it acceptable?
--- According to the R-Car Gen2/3 manual, the bit 0 of MACCTLR register should be written to 0 because the bit 0 is set to 1 on reset. To avoid unexpected behaviors from this incorrect setting, this patch fixes it. ---
Best regards, Yoshihiro Shimoda
On 09.10.2019 11:54, Yoshihiro Shimoda wrote:
According to the R-Car Gen2/3 manual, the bit 0 of MACCTLR register should be written to 0 because the register is set to 1 on reset.
The bit 0 set to 1, not the whole register (it has 1s also in thebits 16-23).
Thank you for the comment. So, I'll fix the commit log as following. Is it acceptable?
According to the R-Car Gen2/3 manual, the bit 0 of MACCTLR register should be written to 0 because the bit 0 is set to 1 on reset. To avoid unexpected behaviors from this incorrect setting, this patch fixes it.
Alright now.
Best regards, Yoshihiro Shimoda
MBR, Sergei
Hi Sergei-san,
From: Sergei Shtylyov, Sent: Wednesday, October 9, 2019 5:58 PM
On 09.10.2019 11:54, Yoshihiro Shimoda wrote:
According to the R-Car Gen2/3 manual, the bit 0 of MACCTLR register should be written to 0 because the register is set to 1 on reset.
The bit 0 set to 1, not the whole register (it has 1s also in thebits 16-23).
Thank you for the comment. So, I'll fix the commit log as following. Is it acceptable?
According to the R-Car Gen2/3 manual, the bit 0 of MACCTLR register should be written to 0 because the bit 0 is set to 1 on reset. To avoid unexpected behaviors from this incorrect setting, this patch fixes it.
Alright now.
Thanks!
Best regards, Yoshihiro Shimoda
Hi Shimoda-san,
On Wed, Oct 9, 2019 at 6:03 AM Yoshihiro Shimoda yoshihiro.shimoda.uh@renesas.com wrote:
According to the R-Car Gen2/3 manual, the bit 0 of MACCTLR register should be written to 0 because the register is set to 1 on reset. To avoid unexpected behaviors from this incorrect setting, this patch fixes it.
Fixes: b3327f7fae66 ("PCI: rcar: Try increasing PCIe link speed to 5 GT/s at boot") Cc: stable@vger.kernel.org # v4.9+ Signed-off-by: Yoshihiro Shimoda yoshihiro.shimoda.uh@renesas.com Reviewed-by: Sergei Shtylyov sergei.shtylyov@cogentembedded.com
Thanks for your patch!
This patch fixes the issue where the register is written, so Reviewed-by: Geert Uytterhoeven geert+renesas@glider.be
However, according to the R-Car H1, Gen2, and Gen3 Hardware User's Manuals, this reserved bit should be cleared on initialization. Are we sure that is guaranteed to happen? If the checks at the top of rcar_pcie_force_speedup() trigger, the register is never written to, and the bit may still be set?
Gr{oetje,eeting}s,
Geert
Hi Geert-san.
From: Geert Uytterhoeven, Sent: Wednesday, October 9, 2019 5:58 PM
Hi Shimoda-san,
On Wed, Oct 9, 2019 at 6:03 AM Yoshihiro Shimoda yoshihiro.shimoda.uh@renesas.com wrote:
According to the R-Car Gen2/3 manual, the bit 0 of MACCTLR register should be written to 0 because the register is set to 1 on reset. To avoid unexpected behaviors from this incorrect setting, this patch fixes it.
Fixes: b3327f7fae66 ("PCI: rcar: Try increasing PCIe link speed to 5 GT/s at boot") Cc: stable@vger.kernel.org # v4.9+ Signed-off-by: Yoshihiro Shimoda yoshihiro.shimoda.uh@renesas.com Reviewed-by: Sergei Shtylyov sergei.shtylyov@cogentembedded.com
Thanks for your patch!
This patch fixes the issue where the register is written, so Reviewed-by: Geert Uytterhoeven geert+renesas@glider.be
Thank you for your review!
However, according to the R-Car H1, Gen2, and Gen3 Hardware User's Manuals, this reserved bit should be cleared on initialization. Are we sure that is guaranteed to happen? If the checks at the top of rcar_pcie_force_speedup() trigger, the register is never written to, and the bit may still be set?
Thank you for the pointed it out! As you said, this driver should set the bit 0 of register on rcar_pcie_hw_init(), not rcar_pcie_force_speedup(). I checked that the bit 0t of register keep to be 0 after the driver cleared the bit. So, clearing the bit 0 on rcar_pcie_hw_init() only is enough, I think. So, I'll send v3 patch.
Best regards, Yoshihiro Shimoda
Gr{oetje,eeting}s,
Geert-- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
linux-stable-mirror@lists.linaro.org
-
Geert Uytterhoeven -
Sergei Shtylyov -
Yoshihiro Shimoda