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
--
மணிவண்ணன் சதாசிவம்
