[PATCH v2] PCI: rcar: Fix writing the MACCTLR register value

[PATCH v2] PCI: rcar: Fix writing the MACCTLR register value

[Yoshihiro Shimoda]

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);
-- 
2.7.4

Re: [PATCH v2] PCI: rcar: Fix writing the MACCTLR register value

[Sergei Shtylyov]

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

RE: [PATCH v2] PCI: rcar: Fix writing the MACCTLR register value

[Yoshihiro Shimoda]

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

Re: [PATCH v2] PCI: rcar: Fix writing the MACCTLR register value

[Sergei Shtylyov]

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

Re: [PATCH v2] PCI: rcar: Fix writing the MACCTLR register value

[Geert Uytterhoeven]

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

-- 
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

RE: [PATCH v2] PCI: rcar: Fix writing the MACCTLR register value

[Yoshihiro Shimoda]

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

RE: [PATCH v2] PCI: rcar: Fix writing the MACCTLR register value

[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