Re: [PATCH v24 08/16] PCI: dwc: Disable two BARs to avoid unnecessary memory assignment

[Manivannan Sadhasivam <mani@kernel.org>]

On Wed, Oct 11, 2023 at 10:07:27PM +0900, Krzysztof Wilczyński wrote:
> Hello,
> 
> [...]
> > > +	/*
> > > +	 * According to the section 3.5.7.2 "RC Mode" in DWC PCIe Dual Mode
> > > +	 * Rev.5.20a,
> > 
> > and 3.5.6.1 "RC mode" in DWC PCIe RC databook v5.20a.
> 
> OK.  I can fix this citation later.
> 
> > > +      ... we should disable two BARs to avoid unnecessary memory
> > > +	 * assignment during device enumeration.
> > > +	 */
> > > +	dw_pcie_writel_dbi2(pci, PCI_BASE_ADDRESS_0, 0x0);
> > > +	dw_pcie_writel_dbi2(pci, PCI_BASE_ADDRESS_1, 0x0);
> > > +
> > 
> > What's the point in doing this
> > 	dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 0x00000004);
> > 	dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_1, 0x00000000);
> >         ...
> >         dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 0);
> > afterward?
> > 
> > I guess if the BARs are disabled there is no need in having them
> > touched. Am I wrong?
> > 
> > BTW I failed to understand why the BARs inits was originally needed:
> > first merging the BAR0 and BAR1 into a single 64-bit BAR, then
> > switching it back to two 32-bit BARs. Moreover here is what prior DW
> > PCIe RC v5.x databooks say about the BARs:
> > 
> > "3.5.6 BAR Details
> > Base Address Registers (Offset: 0x10-x14)
> > The Synopsys core does not implement the optional BARs for the RC
> > product. This is based on the assumption that the RC host probably has
> > registers on some other internal bus and has knowledge and setup
> > access to these registers already."
> > 
> > I am not sure I fully understand what it means, but it seems as DW
> > PCIe cores didn't have anything behind the RC BARs even back then. So
> > it seems to me that the BARs manipulation was the Exinos PCIe host
> > specific, from which driver they are originating - commit 340cba6092c2
> > ("pci: Add PCIe driver for Samsung Exynos").
> 
> Would any of the above be something we need to address before this series
> can be successfully merged?  I am asking if this is a show stopper,
> something we can fix later, or even something I could address once I take
> this series again.
> 
> Thoughts?
> 

If Yoshihiro can confirm that his controller can work without this patch, then
I'd vote for dropping this patch and applying the rest.

This can be submitted later if required.

- Mani

> > * BTW Yoshihiro, I am sorry to see your patchset is still under review...(
> 
> Yes, we need to draw a line somewhere. :)  I am happy to take this series
> so we don't miss another merge window.  We can always fix other bits and
> pieces later and iron out any kinks that might have fallen through the
> cracks, so to speak.
> 
> 	Krzysztof

-- 
மணிவண்ணன் சதாசிவம்