| Message ID | 1570593791-6958-1-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [v2] PCI: rcar: Fix writing the MACCTLR register value | expand |
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 the > bits 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 the >> bits 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 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 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 the > >> bits 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 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
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);
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(-)