Hello Serge, Krzysztof, Mani,
> From: Serge Semin, Sent: Wednesday, October 11, 2023 11:50 PM
>
> 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?
I tried to remove these setting on my environment, and then it also worked.
So, as you mentioned, there is no need, I think.
> > > > 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/
Yes, the settings are really needed on my environment.
# I checked this again today.
> 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).
Thank you for the information. I could not find the settings...
-----
static void ks_pcie_setup_rc_app_regs(struct keystone_pcie *ks_pcie)
{
...
/* Disable BARs for inbound access */
ks_pcie_set_dbi_mode(ks_pcie);
dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 0);
dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_1, 0);
ks_pcie_clear_dbi_mode(ks_pcie);
-----
> 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.
If possible, I would like to have the setting on pcie-rcar-gen4.c only,
for avoiding any trouble, especially on other DWC drivers.
> -Serge(y)
>
> >
> > This can be submitted later if required.
> >
> > - Mani
> >
> > > > * BTW Yoshihiro, I am sorry to see your patchset is still under review...(
Serge, no worries! Thank you very much for your support again and again!
> > > 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, thank you very much for your support! I hope that this patchset can be
merged into v6.7-rc1...
Best regards,
Yoshihiro Shimoda
> > > Krzysztof
> >
> > --
> > மணிவண்ணன் சதாசிவம்
