Re: [PATCH v3 2/2] PCI: rcar: Fix missing MACCTLR register setting in initialize sequence

From: Geert Uytterhoeven <geert@linux-m68k.org>
To: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Cc: Marek Vasut <marek.vasut+renesas@gmail.com>,
	linux-pci <linux-pci@vger.kernel.org>,
	Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
	stable <stable@vger.kernel.org>
Subject: Re: [PATCH v3 2/2] PCI: rcar: Fix missing MACCTLR register setting in initialize sequence
Date: Tue, 5 Nov 2019 10:52:53 +0100	[thread overview]
Message-ID: <CAMuHMdXvFaPwqo2EHiBMTot05KggRNtL56JOrW_MUrBLL6NHxQ@mail.gmail.com> (raw)
In-Reply-To: <TYAPR01MB45448947CB09B1A8AC1774A8D87E0@TYAPR01MB4544.jpnprd01.prod.outlook.com>

Hi Shimoda-san,

On Tue, Nov 5, 2019 at 10:26 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> > From: Geert Uytterhoeven, Sent: Tuesday, November 5, 2019 5:50 PM
> > On Tue, Nov 5, 2019 at 3:48 AM Yoshihiro Shimoda
> > <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > According to the R-Car Gen2/3 manual, "Be sure to write the initial
> > > value (= H'80FF 0000) to MACCTLR before enabling PCIETCTLR.CFINIT".
> > > To avoid unexpected behaviors, this patch fixes it. Note that
> > > the SPCHG bit of MACCTLR register description said "Only writing 1
> > > is valid and writing 0 is invalid" but this "invalid" means
> > > "ignored", not "prohibited". So, any documentation conflict doesn't
> > > exist about writing the MACCTLR register.
> > >
> > > Reported-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> > > 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>
> >
> > Thanks for your patch!
> >
> > > --- a/drivers/pci/controller/pcie-rcar.c
> > > +++ b/drivers/pci/controller/pcie-rcar.c
> > > @@ -91,8 +91,12 @@
> > >  #define  LINK_SPEED_2_5GTS     (1 << 16)
> > >  #define  LINK_SPEED_5_0GTS     (2 << 16)
> > >  #define MACCTLR                        0x011058
> > > +#define  MACCTLR_RESERVED23_16 GENMASK(23, 16)
> >
> > MACCTLR_NFTS_MASK?
>
> I should have said on previous email thread [1] though,
> since SH7786 PCIE HW manual said NFTS (NFTS) but
> any R-Car SoCs' HW manual said just Reserved with H'FF,
> so that I prefer to describe RESERVED instead of NFTS.
> Do you agree?
>
> [1]
> https://marc.info/?l=linux-renesas-soc&m=157242422327368&w=2

My personal stance is to make it as easy as possible for the reader of
the code ("optimize for reading, not for writing"), as code is written once,
but read many more times later.
This is not the first time register bits were documented before, and changed
to reserved later.
In this case the resemblance to the SH7786 PCIe block is obvious, and
the SH7786 hardware user's manual is available publicly.

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