RE: [PATCH v4] PCI: rcar: Fix missing MACCTLR register setting in rcar_pcie_hw_init()

From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
To: "REE erosca@DE.ADIT-JV.COM" <erosca@DE.ADIT-JV.COM>,
	Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>,
	Geert Uytterhoeven <geert+renesas@glider.be>
Cc: "horms@verge.net.au" <horms@verge.net.au>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"linux-renesas-soc@vger.kernel.org" 
	<linux-renesas-soc@vger.kernel.org>,
	"stable@vger.kernel.org" <stable@vger.kernel.org>,
	Yohhei Fukui <yohhei.fukui@denso-ten.com>,
	Asano Yasushi <yasano@jp.adit-jv.com>,
	Steffen Pengel <spengel@jp.adit-jv.com>,
	Andrew Gabbasov <andrew_gabbasov@mentor.com>,
	"REE erosca@DE.ADIT-JV.COM" <erosca@DE.ADIT-JV.COM>,
	Eugeniu Rosca <roscaeugeniu@gmail.com>
Subject: RE: [PATCH v4] PCI: rcar: Fix missing MACCTLR register setting in rcar_pcie_hw_init()
Date: Wed, 30 Oct 2019 02:15:50 +0000	[thread overview]
Message-ID: <TYAPR01MB45441F470C83E7CAEF4D72E0D8600@TYAPR01MB4544.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <20191029143753.GA28404@vmlxhi-102.adit-jv.com>

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