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

[Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>]

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