Re: [PATCH v2 2/3] PCI: tegra194: Reset BARs when running in PCIe endpoint mode

[Aksh Garg <a-garg7@ti.com>]

+ Kishon

On 10/02/26 16:09, Niklas Cassel wrote:
>> BAR_RESERVED already means disabled, it just assumes that an EPC driver
>> disables all BARs by default, which is the case for:
>> pci-dra7xx.c, pci-imx6.c, pci-layerscape-ep.c, pcie-artpec6.c,
>> pcie-designware-plat.c, pcie-dw-rockchip.c, pcie-qcom-ep.c, pcie-rcar-gen4.c,
>> pcie-stm32-ep.c, pcie-uniphier-ep.c.
>> (All drivers which disables all BARs by default in the init() callback using
>> dw_pcie_ep_reset_bar(). pci-epf-test will later enable all BARs that are not
>> marked as BAR_RESERVED.)
>> 
>> That leaves: pcie-keembay.c, pci-keystone.c, pcie-tegra194.c (before my patch).
>> 
>> For pcie-keembay.c, this is not a problem, because BAR0, BAR2, BAR4 are marked
>> as only_64bit, so pci-epf-test configure these BARs as 64-bit BARs, and thus
>> BAR1, BAR3, and BAR5 will get disabled implicitly.
>> 
>> For pci-keystone.c, this is the only driver that is a bit weird, it marks
>> BAR0 and BAR1 as reserved, but does not disable them in the init() callback.
>> It seems force set BAR0 as a 32-bit BAR in the init() callback.
>> 
>> Thus, for all drivers except for pci-keystone.c, BAR_RESERVED does mean
>> BAR_DISABLED. Feel free to send a patch that renames BAR_RESERVED to
>> BAR_DISABLED.
>> 
>> If you send such a patch, perhaps you also want to modify the PCI endpoint
>> core to call reset_bar() for all BARs marked as BAR_RESERVED/BAR_DISABLED,
>> instead of each EPC driver doing so in the init() callback. I think the main
>> reason why this is not done already is that thare is no reset_bar() op in
>> struct pci_epc_ops epc_ops, there is only clear_bar() which clears an BAR
>> enabled by an EPF driver. (So you would most likely also need to add a
>> .disable_bar() op in struct pci_epc_ops epc_ops.)
> 
> Aksh (on To:),
> 
> since you have a @ti.com email, perhaps you can explain how pci-keystone.c
> can pass all the pci-epf-test test cases, considering that this is the only
> driver that has BARs (BAR0 and BAR1) marked as BAR_RESERVED but do not also
> disable the BARs (using dw_pcie_ep_reset_bar()) in the init() callback.
> 
> Or, perhaps the simple answer is that pci-keystone.c does not pass all
> pci-epf-test test cases?
> 
> 
> Kind regards,
> Niklas

Hi Niklas,

I just joined the organization and have no context on why the
pci-keystone.c have BAR0 and BAR1 as reserved, without disabling the
bars using dw_pcie_ep_reset_bar() in the .init() callback. Because the
AM65 do not use any BARs for any purpose like Tegra194 does (ATU
registers or eDMA registers exposed in BAR4 for example), there would
be no issue if the BAR0 and BAR1 are overwritten.

This was introduced in the driver the time the EP support was added to
the driver by Kishon in commit 23284ad677a9 ("PCI: keystone: Add support
for PCIe EP in AM654x Platforms"), where no context is provided in the
comments or commit message why the BAR0/1 are marked as reserved in the
features. Perhaps Kishon can provide a better insight over this.

Regards,
Aksh Garg

>