Hello Krzysztof, Mani
On Wed, Oct 11, 2023 at 06:48:40PM +0530, Manivannan Sadhasivam wrote:
> 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?
> >
I can't confirm for sure that the BARs manipulations in this patch
will work on the older IP-cores (prior 5.10a) or will be required for
all new controllers (5.10a and newer). Based on the BARs description
posted in the IP-core HW manuals, the CSRs semantic has changed
between the major releases. Old DW PCIe RC IP-core HW-manuals
explicitly state that the BARs are unavailable:
"The Synopsys core does not implement the optional BARs for the RC
product"
New DW PCIe RC IP-cores manual say that the BARs exist, but are
normally unused:
"Two BARs are present but are not expected to be used. You should
disable them to avoid unnecessary memory assignment during device
enumeration. If you do use a BAR, then you should program it to
capture TLPs that are targeted to your local non-application memory
space.... The BAR range must be outside of the three Base/Limit
regions..."
So in theory it's possible to have platforms with the BARs somehow
utilized even in the Root Ports. Though currently AFAICS we don't
have such devices supported in kernel.
>
> If Yoshihiro can confirm that his controller can work without this patch, then
> I'd vote for dropping this patch and applying the rest.
AFAIR Yoshihiro insisted to have the BARs reset because without
it something didn't work, so he added some comment to justify it:
https://lore.kernel.org/linux-pci/TYBPR01MB534104389952D87385E8745ED8879@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
Though based on the comment the BARs reset still seems optional.
One more low-level driver which already does what is implemented in
this patch is the Keystone PCI host-controller driver (see,
pci-keystone.c also activates dbi_cs2 and zeros out the
PCI_TYPE0_BAR0_ENABLED flag). Moreover something similar is done in
the generic DW PCIe EP driver in the framework of the
__dw_pcie_ep_reset_bar() method including the direct BARs zeroing out
(which I questioned in my initial message in this thread). So seeing
this patch would re-do what is already done for the Keystone device
and would add a partly duplicated code it would be reasonable to drop
the patch for now and get the BARs reset back to the Rcar host
low-level driver as it was in v23. We can get back to the topic
afterward and see whether the BARs reset could be done generically for
the RPs. If we figure out that it's required at least for the new
controllers then we'll be able to implement a generic RP/EP BARs reset
method, have it utilized in both DW PCIe core drivers and drop the
respective code from both Rcar and Keystone LLDDs.
-Serge(y)
>
> 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
>
> --
> மணிவண்ணன் சதாசிவம்
