Re: [PATCH] PCI: dwc: Fix writing wrong value if snps,enable-cdm-check

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Feb 17, 2023 at 11:06:56AM +0300, Serge Semin wrote:
> On Fri, Feb 17, 2023 at 12:46:03AM +0000, Yoshihiro Shimoda wrote:
> > > From: Serge Semin, Sent: Friday, February 17, 2023 5:50 AM
> > > On Thu, Feb 16, 2023 at 11:58:22AM -0600, Bjorn Helgaas wrote:
> > > > On Thu, Feb 16, 2023 at 06:20:12PM +0900, Yoshihiro Shimoda wrote:
> > > > > The "val" of PCIE_PORT_LINK_CONTROL will be reused on the
> > > > > "Set the number of lanes". But, if snps,enable-cdm-check" exists,
> > > > > the "val" will be set to PCIE_PL_CHK_REG_CONTROL_STATUS.
> > > > > Therefore, unexpected register value is possible to be used
> > > > > to PCIE_PORT_LINK_CONTROL register if snps,enable-cdm-check" exists.
> > > > > So, read PCIE_PORT_LINK_CONTROL register again to fix the issue.
> > > > >
> > > > > Fixes: ec7b952f453c ("PCI: dwc: Always enable CDM check if "snps,enable-cdm-check" exists")
> > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > > > ---
> > > > >  drivers/pci/controller/dwc/pcie-designware.c | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > > > > index 6d5d619ab2e9..3bb9ca14fb9c 100644
> > > > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > > > @@ -824,6 +824,7 @@ void dw_pcie_setup(struct dw_pcie *pci)
> > > > >  	}
> > > > >
> > > > >  	/* Set the number of lanes */
> > > > > +	val = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL);
> > > >
> > > > Definitely a bug, thanks for the fix and the Fixes: tag.
> > > >
> > > > But I would like the whole function better if it could be structured
> > > > so we read PCIE_PORT_LINK_CONTROL once and wrote it once.  And the
> > > > same for PCIE_LINK_WIDTH_SPEED_CONTROL.

> ...
> IMO I would leave the procedure as is for now seeing you are going to
> move the rcar_gen4_pcie_set_max_link_width() code to the generic part
> of the driver in the framework of this patch:
> https://lore.kernel.org/linux-pci/20230210134917.2909314-7-yoshihiro.shimoda.uh@renesas.com/
> per Rob and my requests.
> 
> Thus you'll be able to combine all the bus-width updates into a single
> method, like dw_pcie_link_set_max_link_width(). The function will look
> as coherent as possible meanwhile the dw_pcie_setup() method body will
> turn to be smaller and easier to comprehend. Alas that will imply
> updating the PCIE_PORT_LINK_CONTROL and PCIE_LINK_WIDTH_SPEED_CONTROL
> registers twice.
> 
> @Bjorn, are you ok with that?

I haven't looked at the rcar_gen4_pcie_set_max_link_width()
restructuring so don't have an opinion on that.

We read PCIE_PORT_LINK_CONTROL once, we lost the value by using "val"
for something else, and then we need it again.  My point was only that
re-reading PCIE_PORT_LINK_CONTROL seems like overkill compared to
adding another local variable to remember it.

If the updates are in different parts of the framework, maybe two
updates make sense.  I'm not suggesting we need to contort things to
achieve a single update at all costs.  It's just that two updates in
a single function looks like it should be avoidable.

Bjorn