Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v4] PCI: rcar: Fix missing MACCTLR register setting in rcar_pcie_hw_init()
@ 2019-10-11  4:50 Yoshihiro Shimoda
  2019-10-15 14:46 ` Lorenzo Pieralisi
  2019-10-29 14:37 ` Eugeniu Rosca
  0 siblings, 2 replies; 6+ messages in thread
From: Yoshihiro Shimoda @ 2019-10-11  4:50 UTC (permalink / raw)
  To: horms, linux-pci; +Cc: linux-renesas-soc, stable, Yoshihiro Shimoda

According to the R-Car Gen2/3 manual, the bit 0 of MACCTLR register
should be written to 0 before enabling PCIETCTLR.CFINIT because
the bit 0 is set to 1 on reset. To avoid unexpected behaviors from
this incorrect setting, this patch fixes it.

Fixes: c25da4778803 ("PCI: rcar: Add Renesas R-Car PCIe driver")
Fixes: be20bbcb0a8c ("PCI: rcar: Add the initialization of PCIe link in resume_noirq()")
Cc: <stable@vger.kernel.org> # v5.2+
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 Changes from v3:
 - Add the setting in rcar_pcie_resume_noirq().
 - Add Fixes tag for rcar_pcie_resume_noirq().
 - Change the version of the stable ML from v3.16 to v5.2.
 https://patchwork.kernel.org/patch/11181005/

 Changes from v2:
 - Change the subject.
 - Fix commit log again.
 - Add the register setting into the initialization, instead of speedup.
 - Change commit hash/target version on Fixes and Cc stable tags.
 - Add Geert-san's Reviewed-by.
 https://patchwork.kernel.org/patch/11180429/

 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, 4 insertions(+)

diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
index f6a669a..302c9ea 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
@@ -615,6 +616,8 @@ static int rcar_pcie_hw_init(struct rcar_pcie *pcie)
 	if (IS_ENABLED(CONFIG_PCI_MSI))
 		rcar_pci_write_reg(pcie, 0x801f0000, PCIEMSITXR);
 
+	rcar_rmw32(pcie, MACCTLR, MACCTLR_RESERVED, 0);
+
 	/* Finish initialization - establish a PCI Express link */
 	rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
 
@@ -1237,6 +1240,7 @@ static int rcar_pcie_resume_noirq(struct device *dev)
 		return 0;
 
 	/* Re-establish the PCIe link */
+	rcar_rmw32(pcie, MACCTLR, MACCTLR_RESERVED, 0);
 	rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
 	return rcar_pcie_wait_for_dl(pcie);
 }
-- 
2.7.4


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4] PCI: rcar: Fix missing MACCTLR register setting in rcar_pcie_hw_init()
  2019-10-11  4:50 [PATCH v4] PCI: rcar: Fix missing MACCTLR register setting in rcar_pcie_hw_init() Yoshihiro Shimoda
@ 2019-10-15 14:46 ` Lorenzo Pieralisi
  2019-10-29 14:37 ` Eugeniu Rosca
  1 sibling, 0 replies; 6+ messages in thread
From: Lorenzo Pieralisi @ 2019-10-15 14:46 UTC (permalink / raw)
  To: Yoshihiro Shimoda; +Cc: horms, linux-pci, linux-renesas-soc, stable

On Fri, Oct 11, 2019 at 01:50:32PM +0900, Yoshihiro Shimoda wrote:
> According to the R-Car Gen2/3 manual, the bit 0 of MACCTLR register
> should be written to 0 before enabling PCIETCTLR.CFINIT because
> the bit 0 is set to 1 on reset. To avoid unexpected behaviors from
> this incorrect setting, this patch fixes it.
> 
> Fixes: c25da4778803 ("PCI: rcar: Add Renesas R-Car PCIe driver")
> Fixes: be20bbcb0a8c ("PCI: rcar: Add the initialization of PCIe link in resume_noirq()")
> Cc: <stable@vger.kernel.org> # v5.2+
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  Changes from v3:
>  - Add the setting in rcar_pcie_resume_noirq().
>  - Add Fixes tag for rcar_pcie_resume_noirq().
>  - Change the version of the stable ML from v3.16 to v5.2.
>  https://patchwork.kernel.org/patch/11181005/
> 
>  Changes from v2:
>  - Change the subject.
>  - Fix commit log again.
>  - Add the register setting into the initialization, instead of speedup.
>  - Change commit hash/target version on Fixes and Cc stable tags.
>  - Add Geert-san's Reviewed-by.
>  https://patchwork.kernel.org/patch/11180429/
> 
>  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, 4 insertions(+)

Applied to pci/rcar, thanks.

Lorenzo

> diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
> index f6a669a..302c9ea 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
> @@ -615,6 +616,8 @@ static int rcar_pcie_hw_init(struct rcar_pcie *pcie)
>  	if (IS_ENABLED(CONFIG_PCI_MSI))
>  		rcar_pci_write_reg(pcie, 0x801f0000, PCIEMSITXR);
>  
> +	rcar_rmw32(pcie, MACCTLR, MACCTLR_RESERVED, 0);
> +
>  	/* Finish initialization - establish a PCI Express link */
>  	rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
>  
> @@ -1237,6 +1240,7 @@ static int rcar_pcie_resume_noirq(struct device *dev)
>  		return 0;
>  
>  	/* Re-establish the PCIe link */
> +	rcar_rmw32(pcie, MACCTLR, MACCTLR_RESERVED, 0);
>  	rcar_pci_write_reg(pcie, CFINIT, PCIETCTLR);
>  	return rcar_pcie_wait_for_dl(pcie);
>  }
> -- 
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4] PCI: rcar: Fix missing MACCTLR register setting in rcar_pcie_hw_init()
  2019-10-11  4:50 [PATCH v4] PCI: rcar: Fix missing MACCTLR register setting in rcar_pcie_hw_init() Yoshihiro Shimoda
  2019-10-15 14:46 ` Lorenzo Pieralisi
@ 2019-10-29 14:37 ` Eugeniu Rosca
  2019-10-30  2:15   ` Yoshihiro Shimoda
  1 sibling, 1 reply; 6+ messages in thread
From: Eugeniu Rosca @ 2019-10-29 14:37 UTC (permalink / raw)
  To: Yoshihiro Shimoda
  Cc: horms, linux-pci, linux-renesas-soc, stable, Sergei Shtylyov,
	Geert Uytterhoeven, Yohhei Fukui, Asano Yasushi, Steffen Pengel,
	Andrew Gabbasov, Eugeniu Rosca, Eugeniu Rosca

Dear Shimoda-san, dear reviewers,

On Fri, Oct 11, 2019 at 01:50:32PM +0900, Yoshihiro Shimoda wrote:
> According to the R-Car Gen2/3 manual, the bit 0 of MACCTLR register
> should be written to 0 before enabling PCIETCTLR.CFINIT because
> the bit 0 is set to 1 on reset. To avoid unexpected behaviors from
> this incorrect setting, this patch fixes it.

Your development and reviewing effort to reach v4 is very appreciated.

However, in the context of some internal reviews of this patch, we are
having hard times reconciling the change with our (possibly incomplete
or inaccurate) interpretation of the R-Car3 HW User’s Manual (Rev.2.00
Jul 2019). The latter says in
Chapter "54. PCIE Controller" / "(2) Initial Setting of PCI Express":

 ----snip----
 Be sure to write the initial value (= H'80FF 0000) to MACCTLR before
 enabling PCIETCTLR.CFINIT.
 ----snip----

Is my assumption correct that the description of this patch is a
rewording of the above quote from the manual or is there another more
precise statement referring to resetting LSB only (w/o touching the
rest of the MACCTLR bits)?

If it is only the LSB which "should be written to 0 before enabling
PCIETCTLR.CFINIT", would you agree that the statement quoted from the
manual would better be rephrased appropriately? TIA.

> 
> Fixes: c25da4778803 ("PCI: rcar: Add Renesas R-Car PCIe driver")
> Fixes: be20bbcb0a8c ("PCI: rcar: Add the initialization of PCIe link in resume_noirq()")
> Cc: <stable@vger.kernel.org> # v5.2+
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

-- 
Best Regards,
Eugeniu

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH v4] PCI: rcar: Fix missing MACCTLR register setting in rcar_pcie_hw_init()
  2019-10-29 14:37 ` Eugeniu Rosca
@ 2019-10-30  2:15   ` Yoshihiro Shimoda
  2019-10-30  8:30     ` Geert Uytterhoeven
  0 siblings, 1 reply; 6+ messages in thread
From: Yoshihiro Shimoda @ 2019-10-30  2:15 UTC (permalink / raw)
  To: REE erosca, Sergei Shtylyov, Geert Uytterhoeven
  Cc: horms, linux-pci, linux-renesas-soc, stable, Yohhei Fukui,
	Asano Yasushi, Steffen Pengel, Andrew Gabbasov, REE erosca,
	Eugeniu Rosca

Dear Eugeniu-san,

Thank you for your comments!

> From: Eugeniu Rosca, Sent: Tuesday, October 29, 2019 11:38 PM
> 
> Dear Shimoda-san, dear reviewers,
> 
> On Fri, Oct 11, 2019 at 01:50:32PM +0900, Yoshihiro Shimoda wrote:
> > According to the R-Car Gen2/3 manual, the bit 0 of MACCTLR register
> > should be written to 0 before enabling PCIETCTLR.CFINIT because
> > the bit 0 is set to 1 on reset. To avoid unexpected behaviors from
> > this incorrect setting, this patch fixes it.
> 
> Your development and reviewing effort to reach v4 is very appreciated.
> 
> However, in the context of some internal reviews of this patch, we are
> having hard times reconciling the change with our (possibly incomplete
> or inaccurate) interpretation of the R-Car3 HW User’s Manual (Rev.2.00
> Jul 2019). The latter says in
> Chapter "54. PCIE Controller" / "(2) Initial Setting of PCI Express":
> 
>  ----snip----
>  Be sure to write the initial value (= H'80FF 0000) to MACCTLR before
>  enabling PCIETCTLR.CFINIT.
>  ----snip----
> 
> Is my assumption correct that the description of this patch is a
> rewording of the above quote from the manual <snip>

You are correct. Since the reset value of MACCTLR is H'80FF 0001, I thought
clearing the LSB bit was enough.
However, as your situation, I think I should have described the above quote
from the manual and have such a code (writing the value instead of clearing
the LSB only). 

> If it is only the LSB which "should be written to 0 before enabling
> PCIETCTLR.CFINIT", would you agree that the statement quoted from the
> manual would better be rephrased appropriately? TIA.

As I mentioned above, I think the above quote from the manual is better
than rephrased.

Sergei, Geert-san, I think we should revert this patch and fix code/commit
log to follow the manual. What do you think?

Best regards,
Yoshihiro Shimoda

> >
> > Fixes: c25da4778803 ("PCI: rcar: Add Renesas R-Car PCIe driver")
> > Fixes: be20bbcb0a8c ("PCI: rcar: Add the initialization of PCIe link in resume_noirq()")
> > Cc: <stable@vger.kernel.org> # v5.2+
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> --
> Best Regards,
> Eugeniu

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v4] PCI: rcar: Fix missing MACCTLR register setting in rcar_pcie_hw_init()
  2019-10-30  2:15   ` Yoshihiro Shimoda
@ 2019-10-30  8:30     ` Geert Uytterhoeven
  2019-10-30 10:54       ` Yoshihiro Shimoda
  0 siblings, 1 reply; 6+ messages in thread
From: Geert Uytterhoeven @ 2019-10-30  8:30 UTC (permalink / raw)
  To: Yoshihiro Shimoda
  Cc: REE erosca, Sergei Shtylyov, Geert Uytterhoeven, horms,
	linux-pci, linux-renesas-soc, stable, Yohhei Fukui,
	Asano Yasushi, Steffen Pengel, Andrew Gabbasov, Eugeniu Rosca

Hi Shimoda-san,

On Wed, Oct 30, 2019 at 3:15 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> > From: Eugeniu Rosca, Sent: Tuesday, October 29, 2019 11:38 PM
> > On Fri, Oct 11, 2019 at 01:50:32PM +0900, Yoshihiro Shimoda wrote:
> > > According to the R-Car Gen2/3 manual, the bit 0 of MACCTLR register
> > > should be written to 0 before enabling PCIETCTLR.CFINIT because
> > > the bit 0 is set to 1 on reset. To avoid unexpected behaviors from
> > > this incorrect setting, this patch fixes it.
> >
> > Your development and reviewing effort to reach v4 is very appreciated.
> >
> > However, in the context of some internal reviews of this patch, we are
> > having hard times reconciling the change with our (possibly incomplete
> > or inaccurate) interpretation of the R-Car3 HW User’s Manual (Rev.2.00
> > Jul 2019). The latter says in
> > Chapter "54. PCIE Controller" / "(2) Initial Setting of PCI Express":
> >
> >  ----snip----
> >  Be sure to write the initial value (= H'80FF 0000) to MACCTLR before
> >  enabling PCIETCTLR.CFINIT.
> >  ----snip----
> >
> > Is my assumption correct that the description of this patch is a
> > rewording of the above quote from the manual <snip>
>
> You are correct. Since the reset value of MACCTLR is H'80FF 0001, I thought
> clearing the LSB bit was enough.
> However, as your situation, I think I should have described the above quote
> from the manual and have such a code (writing the value instead of clearing
> the LSB only).
>
> > If it is only the LSB which "should be written to 0 before enabling
> > PCIETCTLR.CFINIT", would you agree that the statement quoted from the
> > manual would better be rephrased appropriately? TIA.
>
> As I mentioned above, I think the above quote from the manual is better
> than rephrased.
>
> Sergei, Geert-san, I think we should revert this patch and fix code/commit
> log to follow the manual. What do you think?

The initial value mentioned in the manual makes sense to me.
Of course when using that, #defines should be added for bits used, to
avoid writing the magical value "0x80ff0001".

Initially, the "ff" part worried me.  Fortunately some archaeology learned
me that these bits where called "NFTS" in the SH7786 Hardware User's
Manual, and used to specify the number of Fast Training Sequences to
be transferred when the MAC returns from L0 to L0s (6--255).

arch/sh/drivers/pci/pcie-sh7786.c seems to be aware of this:

        /*
         * Set fast training sequences to the maximum 255,
         * and enable MAC data scrambling.
         */
        data = pci_read_reg(chan, SH4A_PCIEMACCTLR);
        data &= ~PCIEMACCTLR_SCR_DIS;
        data |= (0xff << 16);
        pci_write_reg(chan, data, SH4A_PCIEMACCTLR);

No idea why this was deemed not-to-be-modified by the user later
(as of R-Car H1).

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH v4] PCI: rcar: Fix missing MACCTLR register setting in rcar_pcie_hw_init()
  2019-10-30  8:30     ` Geert Uytterhoeven
@ 2019-10-30 10:54       ` Yoshihiro Shimoda
  0 siblings, 0 replies; 6+ messages in thread
From: Yoshihiro Shimoda @ 2019-10-30 10:54 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: REE erosca, Sergei Shtylyov, Geert Uytterhoeven, horms,
	linux-pci, linux-renesas-soc, stable, Yohhei Fukui,
	Asano Yasushi, Steffen Pengel, Andrew Gabbasov, Eugeniu Rosca

Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Wednesday, October 30, 2019 5:30 PM
<snip>
> > > Your development and reviewing effort to reach v4 is very appreciated.
> > >
> > > However, in the context of some internal reviews of this patch, we are
> > > having hard times reconciling the change with our (possibly incomplete
> > > or inaccurate) interpretation of the R-Car3 HW User’s Manual (Rev.2.00
> > > Jul 2019). The latter says in
> > > Chapter "54. PCIE Controller" / "(2) Initial Setting of PCI Express":
> > >
> > >  ----snip----
> > >  Be sure to write the initial value (= H'80FF 0000) to MACCTLR before
> > >  enabling PCIETCTLR.CFINIT.
> > >  ----snip----
> > >
> > > Is my assumption correct that the description of this patch is a
> > > rewording of the above quote from the manual <snip>
> >
> > You are correct. Since the reset value of MACCTLR is H'80FF 0001, I thought
> > clearing the LSB bit was enough.
> > However, as your situation, I think I should have described the above quote
> > from the manual and have such a code (writing the value instead of clearing
> > the LSB only).
> >
> > > If it is only the LSB which "should be written to 0 before enabling
> > > PCIETCTLR.CFINIT", would you agree that the statement quoted from the
> > > manual would better be rephrased appropriately? TIA.
> >
> > As I mentioned above, I think the above quote from the manual is better
> > than rephrased.
> >
> > Sergei, Geert-san, I think we should revert this patch and fix code/commit
> > log to follow the manual. What do you think?
> 
> The initial value mentioned in the manual makes sense to me.
> Of course when using that, #defines should be added for bits used, to
> avoid writing the magical value "0x80ff0001".

Thank you for your reply! So, I'll submit two patches (revert it at first
and then fix again) later.

> Initially, the "ff" part worried me.  Fortunately some archaeology learned
> me that these bits where called "NFTS" in the SH7786 Hardware User's
> Manual, and used to specify the number of Fast Training Sequences to
> be transferred when the MAC returns from L0 to L0s (6--255).
> 
> arch/sh/drivers/pci/pcie-sh7786.c seems to be aware of this:
> 
>         /*
>          * Set fast training sequences to the maximum 255,
>          * and enable MAC data scrambling.
>          */
>         data = pci_read_reg(chan, SH4A_PCIEMACCTLR);
>         data &= ~PCIEMACCTLR_SCR_DIS;
>         data |= (0xff << 16);
>         pci_write_reg(chan, data, SH4A_PCIEMACCTLR);

I didn't know that..

> No idea why this was deemed not-to-be-modified by the user later
> (as of R-Car H1).

Same here...

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-11  4:50 [PATCH v4] PCI: rcar: Fix missing MACCTLR register setting in rcar_pcie_hw_init() Yoshihiro Shimoda
2019-10-15 14:46 ` Lorenzo Pieralisi
2019-10-29 14:37 ` Eugeniu Rosca
2019-10-30  2:15   ` Yoshihiro Shimoda
2019-10-30  8:30     ` Geert Uytterhoeven
2019-10-30 10:54       ` Yoshihiro Shimoda

Linux-PCI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pci linux-pci/ https://lore.kernel.org/linux-pci \
		linux-pci@vger.kernel.org
	public-inbox-index linux-pci

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pci


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git