diff mbox series

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

Message ID 20231011071423.249458-9-yoshihiro.shimoda.uh@renesas.com (mailing list archive)
State Superseded
Delegated to: Krzysztof Wilczyński
Headers show
Series PCI: dwc: rcar-gen4: Add R-Car Gen4 PCIe support | expand

Commit Message

Yoshihiro Shimoda Oct. 11, 2023, 7:14 a.m. UTC
According to the section 3.5.7.2 "RC Mode" in DWC PCIe Dual Mode
Rev.5.20a, we should disable two BARs to avoid unnecessary memory
assignment during device enumeration. Otherwise, Renesas R-Car Gen4
PCIe controllers cannot work correctly in host mode.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Serge Semin Oct. 11, 2023, 11:45 a.m. UTC | #1
On Wed, Oct 11, 2023 at 04:14:15PM +0900, Yoshihiro Shimoda wrote:
> According to the section 3.5.7.2 "RC Mode" in DWC PCIe Dual Mode
> Rev.5.20a, we should disable two BARs to avoid unnecessary memory
> assignment during device enumeration. Otherwise, Renesas R-Car Gen4
> PCIe controllers cannot work correctly in host mode.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index a7170fd0e847..56cc7ff6d508 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -737,6 +737,14 @@ int dw_pcie_setup_rc(struct dw_pcie_rp *pp)
>  	u32 val, ctrl, num_ctrls;
>  	int ret;
>  
> +	/*
> +	 * 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.

> +      ... 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").

* BTW Yoshihiro, I am sorry to see your patchset is still under review...(

-Serge(y)

>  	/*
>  	 * Enable DBI read-only registers for writing/updating configuration.
>  	 * Write permission gets disabled towards the end of this function.
> -- 
> 2.25.1
>
Krzysztof Wilczyński Oct. 11, 2023, 1:07 p.m. UTC | #2
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?

> * 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
Manivannan Sadhasivam Oct. 11, 2023, 1:18 p.m. UTC | #3
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
Serge Semin Oct. 11, 2023, 2:50 p.m. UTC | #4
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@TYBPR01MB5341.jpnprd01.prod.outlook.com/
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
> 
> -- 
> மணிவண்ணன் சதாசிவம்
Yoshihiro Shimoda Oct. 12, 2023, 8:03 a.m. UTC | #5
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@TYBPR01MB5341.jpnprd01.prod.outlook.com/

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
> >
> > --
> > மணிவண்ணன் சதாசிவம்
Bjorn Helgaas Oct. 16, 2023, 9:48 p.m. UTC | #6
[+cc Siddharth, Ravi, Sriramakrishnan]

On Wed, Oct 11, 2023 at 04:14:15PM +0900, Yoshihiro Shimoda wrote:
> According to the section 3.5.7.2 "RC Mode" in DWC PCIe Dual Mode
> Rev.5.20a, we should disable two BARs to avoid unnecessary memory
> assignment during device enumeration. Otherwise, Renesas R-Car Gen4
> PCIe controllers cannot work correctly in host mode.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index a7170fd0e847..56cc7ff6d508 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -737,6 +737,14 @@ int dw_pcie_setup_rc(struct dw_pcie_rp *pp)
>  	u32 val, ctrl, num_ctrls;
>  	int ret;
>  
> +	/*
> +	 * According to the section 3.5.7.2 "RC Mode" in DWC PCIe Dual Mode
> +	 * Rev.5.20a, 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);

I cc'd Siddharth and others because they are working on a Keystone
issue with MSI-X that requires BAR0; see
https://lore.kernel.org/r/20231011123451.34827-1-s-vadapalli@ti.com 

I assume any DWC controller that uses MSI-X would require BAR0 or BAR1
for the MSI-X Table.

I don't have any of the DWC specs and don't know whether any
controllers use MSI-X, so just heads up in case they do.  This patch
was recently merged and will appear in v6.7.

Bjorn
Siddharth Vadapalli Oct. 17, 2023, 4:39 a.m. UTC | #7
Hello Bjorn,

On 17/10/23 03:18, Bjorn Helgaas wrote:
> [+cc Siddharth, Ravi, Sriramakrishnan]
> 
> On Wed, Oct 11, 2023 at 04:14:15PM +0900, Yoshihiro Shimoda wrote:
>> According to the section 3.5.7.2 "RC Mode" in DWC PCIe Dual Mode
>> Rev.5.20a, we should disable two BARs to avoid unnecessary memory
>> assignment during device enumeration. Otherwise, Renesas R-Car Gen4
>> PCIe controllers cannot work correctly in host mode.
>>
>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>> ---
>>  drivers/pci/controller/dwc/pcie-designware-host.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>> index a7170fd0e847..56cc7ff6d508 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>> @@ -737,6 +737,14 @@ int dw_pcie_setup_rc(struct dw_pcie_rp *pp)
>>  	u32 val, ctrl, num_ctrls;
>>  	int ret;
>>  
>> +	/*
>> +	 * According to the section 3.5.7.2 "RC Mode" in DWC PCIe Dual Mode
>> +	 * Rev.5.20a, 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);
> 
> I cc'd Siddharth and others because they are working on a Keystone
> issue with MSI-X that requires BAR0; see
> https://lore.kernel.org/r/20231011123451.34827-1-s-vadapalli@ti.com 
> 
> I assume any DWC controller that uses MSI-X would require BAR0 or BAR1
> for the MSI-X Table.

Disabling BAR0 in this section will not affect the pci-keystone.c driver. The
MSI-X setup in the pci-keystone.c driver is done via the ks_pcie_v3_65_add_bus()
function which is invoked in the pci_host_probe(bridge) function call within
dw_pcie_host_init(). Since pci_host_probe(bridge) is invoked *after*
dw_pcie_setup_rc(), the MSI-X setup which involves enabling BAR0 will be
performed after BAR0 is disabled in this patch, due to which BAR0 will remain
enabled as far as MSI-X setup is concerned.

Also, the DW PCIe IP version corresponding to the Keystone PCIe host controller
is 4.90a. Even in the 4.90a Databook, in section 3.5.7.2 RC Mode, it is
suggested that the BARs should be disabled.

> 
> I don't have any of the DWC specs and don't know whether any
> controllers use MSI-X, so just heads up in case they do.  This patch
> was recently merged and will appear in v6.7.
> 
> Bjorn
Marek Szyprowski Oct. 17, 2023, 9:19 a.m. UTC | #8
Dear All,

On 11.10.2023 09:14, Yoshihiro Shimoda wrote:
> According to the section 3.5.7.2 "RC Mode" in DWC PCIe Dual Mode
> Rev.5.20a, we should disable two BARs to avoid unnecessary memory
> assignment during device enumeration. Otherwise, Renesas R-Car Gen4
> PCIe controllers cannot work correctly in host mode.
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

This patch landed in today's linux-next 20231017 as commit e308528cac3e 
("PCI: dwc: Disable two BARs to avoid unnecessary memory assignment"). 
Unfortunately it causes the following kernel panic on Samsung 
Exynos5433-based TM2e board:

exynos-pcie 15700000.pcie: host bridge /soc@0/pcie@15700000 ranges:
exynos-pcie 15700000.pcie:       IO 0x000c001000..0x000c010fff -> 
0x0000000000
exynos-pcie 15700000.pcie:      MEM 0x000c011000..0x000ffffffe -> 
0x000c011000
exynos-pcie 15700000.pcie: iATU: unroll F, 3 ob, 5 ib, align 4K, limit 4G
Unable to handle kernel paging request at virtual address ffff800084196010
Mem abort info:
...
Data abort info:
...
swapper pgtable: 4k pages, 48-bit VAs, pgdp=0000000022047000
[ffff800084196010] pgd=10000000df6ff003, p4d=10000000df6ff003, 
pud=10000000df6fe003, pmd=1000000024ad9003, pte=0000000000000000
Internal error: Oops: 0000000096000047 [#1] PREEMPT SMP
Modules linked in:
CPU: 4 PID: 55 Comm: kworker/u18:0 Not tainted 6.6.0-rc1+ #14129
Hardware name: Samsung TM2E board (DT)
Workqueue: events_unbound deferred_probe_work_func
pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : dw_pcie_write_dbi2+0xb8/0xc8
lr : dw_pcie_setup_rc+0x30/0x4e4
...
Call trace:
  dw_pcie_write_dbi2+0xb8/0xc8
  dw_pcie_setup_rc+0x30/0x4e4
  dw_pcie_host_init+0x238/0x608
  exynos_pcie_probe+0x23c/0x340
  platform_probe+0x68/0xd8
  really_probe+0x148/0x2b4
  __driver_probe_device+0x78/0x12c
  driver_probe_device+0xd8/0x160
  __device_attach_driver+0xb8/0x138
  bus_for_each_drv+0x84/0xe0
  __device_attach+0xa8/0x1b0
  device_initial_probe+0x14/0x20
  bus_probe_device+0xb0/0xb4
  deferred_probe_work_func+0x8c/0xc8
  process_one_work+0x1ec/0x53c
  worker_thread+0x298/0x408
  kthread+0x124/0x128
  ret_from_fork+0x10/0x20
Code: d50332bf 79000023 17ffffe2 d50332bf (b9000023)
---[ end trace 0000000000000000 ]---
Kernel panic - not syncing: Oops: Fatal exception
SMP: stopping secondary CPUs
Kernel Offset: disabled
CPU features: 0x8c00020e,3c020000,0000421b
Memory Limit: none
---[ end Kernel panic - not syncing: Oops: Fatal exception ]---

I've observed similar issue on Qualcomm's RB5 platform with some 
additional not-yet merged patches enabling PCIe support. Reverting 
$subject on top of linux-next fixes this issue.

Let me know if you need more information.

> ---
>   drivers/pci/controller/dwc/pcie-designware-host.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index a7170fd0e847..56cc7ff6d508 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -737,6 +737,14 @@ int dw_pcie_setup_rc(struct dw_pcie_rp *pp)
>   	u32 val, ctrl, num_ctrls;
>   	int ret;
>   
> +	/*
> +	 * According to the section 3.5.7.2 "RC Mode" in DWC PCIe Dual Mode
> +	 * Rev.5.20a, 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);
> +
>   	/*
>   	 * Enable DBI read-only registers for writing/updating configuration.
>   	 * Write permission gets disabled towards the end of this function.

Best regards
Yoshihiro Shimoda Oct. 17, 2023, 12:05 p.m. UTC | #9
Dear Marek,

> From: Marek Szyprowski, Sent: Tuesday, October 17, 2023 6:19 PM
> 
> Dear All,
> 
> On 11.10.2023 09:14, Yoshihiro Shimoda wrote:
> > According to the section 3.5.7.2 "RC Mode" in DWC PCIe Dual Mode
> > Rev.5.20a, we should disable two BARs to avoid unnecessary memory
> > assignment during device enumeration. Otherwise, Renesas R-Car Gen4
> > PCIe controllers cannot work correctly in host mode.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> 
> This patch landed in today's linux-next 20231017 as commit e308528cac3e
> ("PCI: dwc: Disable two BARs to avoid unnecessary memory assignment").
> Unfortunately it causes the following kernel panic on Samsung
> Exynos5433-based TM2e board:
> 
> exynos-pcie 15700000.pcie: host bridge /soc@0/pcie@15700000 ranges:
> exynos-pcie 15700000.pcie:       IO 0x000c001000..0x000c010fff ->
> 0x0000000000
> exynos-pcie 15700000.pcie:      MEM 0x000c011000..0x000ffffffe ->
> 0x000c011000
> exynos-pcie 15700000.pcie: iATU: unroll F, 3 ob, 5 ib, align 4K, limit 4G
> Unable to handle kernel paging request at virtual address ffff800084196010
> Mem abort info:
> ...
> Data abort info:
> ...
> swapper pgtable: 4k pages, 48-bit VAs, pgdp=0000000022047000
> [ffff800084196010] pgd=10000000df6ff003, p4d=10000000df6ff003,
> pud=10000000df6fe003, pmd=1000000024ad9003, pte=0000000000000000
> Internal error: Oops: 0000000096000047 [#1] PREEMPT SMP
> Modules linked in:
> CPU: 4 PID: 55 Comm: kworker/u18:0 Not tainted 6.6.0-rc1+ #14129
> Hardware name: Samsung TM2E board (DT)
> Workqueue: events_unbound deferred_probe_work_func
> pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> pc : dw_pcie_write_dbi2+0xb8/0xc8
> lr : dw_pcie_setup_rc+0x30/0x4e4
> ...
> Call trace:
>   dw_pcie_write_dbi2+0xb8/0xc8
>   dw_pcie_setup_rc+0x30/0x4e4
>   dw_pcie_host_init+0x238/0x608
>   exynos_pcie_probe+0x23c/0x340
>   platform_probe+0x68/0xd8
>   really_probe+0x148/0x2b4
>   __driver_probe_device+0x78/0x12c
>   driver_probe_device+0xd8/0x160
>   __device_attach_driver+0xb8/0x138
>   bus_for_each_drv+0x84/0xe0
>   __device_attach+0xa8/0x1b0
>   device_initial_probe+0x14/0x20
>   bus_probe_device+0xb0/0xb4
>   deferred_probe_work_func+0x8c/0xc8
>   process_one_work+0x1ec/0x53c
>   worker_thread+0x298/0x408
>   kthread+0x124/0x128
>   ret_from_fork+0x10/0x20
> Code: d50332bf 79000023 17ffffe2 d50332bf (b9000023)
> ---[ end trace 0000000000000000 ]---
> Kernel panic - not syncing: Oops: Fatal exception
> SMP: stopping secondary CPUs
> Kernel Offset: disabled
> CPU features: 0x8c00020e,3c020000,0000421b
> Memory Limit: none
> ---[ end Kernel panic - not syncing: Oops: Fatal exception ]---
> 
> I've observed similar issue on Qualcomm's RB5 platform with some
> additional not-yet merged patches enabling PCIe support. Reverting
> $subject on top of linux-next fixes this issue.
> 
> Let me know if you need more information.

Thank you for the report. I guess that the issue is related to
out-of-range access of dbi2:
- In arch/arm64/boot/dts/exynos/exynos5433.dtsi, the dbi reg size is 0x1000
  like below:
-----
                pcie: pcie@15700000 {
                        compatible = "samsung,exynos5433-pcie";
                        reg = <0x15700000 0x1000>, <0x156b0000 0x1000>,
                              <0x0c000000 0x1000>;
                        reg-names = "dbi", "elbi", "config";
...
-----

- In drivers/pci/controller/dwc/pcie-designware.c, "dbi2" area is calculated
  by the following if reg-names "dbi2" didn't exist:
-----
                        pci->dbi_base2 = pci->dbi_base + SZ_4K;
-----

- However, this is out-of-memory on exynos5433.dtsi because the "dbi" size is
  0x1000 only.
- And then, this patch always writes PCI_BASE_ADDRESS_[01] to dbi2 area.
  So, since this is out-of-range, the kernel panic happens.

Perhaps, we should revert this patch at first. And, add the settings into
my environment (pcie-rcar-gen4.c) only. I also have alternative solution which
modifies the "dbi2" area calculation and avoid out-of-range access somehow.
But, it may complicate source code...

Best regards,
Yoshihiro Shimoda

> > ---
> >   drivers/pci/controller/dwc/pcie-designware-host.c | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index a7170fd0e847..56cc7ff6d508 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -737,6 +737,14 @@ int dw_pcie_setup_rc(struct dw_pcie_rp *pp)
> >   	u32 val, ctrl, num_ctrls;
> >   	int ret;
> >
> > +	/*
> > +	 * According to the section 3.5.7.2 "RC Mode" in DWC PCIe Dual Mode
> > +	 * Rev.5.20a, 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);
> > +
> >   	/*
> >   	 * Enable DBI read-only registers for writing/updating configuration.
> >   	 * Write permission gets disabled towards the end of this function.
> 
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
Manivannan Sadhasivam Oct. 17, 2023, 3:16 p.m. UTC | #10
On Tue, Oct 17, 2023 at 12:05:12PM +0000, Yoshihiro Shimoda wrote:
> Dear Marek,
> 
> > From: Marek Szyprowski, Sent: Tuesday, October 17, 2023 6:19 PM
> > 
> > Dear All,
> > 
> > On 11.10.2023 09:14, Yoshihiro Shimoda wrote:
> > > According to the section 3.5.7.2 "RC Mode" in DWC PCIe Dual Mode
> > > Rev.5.20a, we should disable two BARs to avoid unnecessary memory
> > > assignment during device enumeration. Otherwise, Renesas R-Car Gen4
> > > PCIe controllers cannot work correctly in host mode.
> > >
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > 
> > This patch landed in today's linux-next 20231017 as commit e308528cac3e
> > ("PCI: dwc: Disable two BARs to avoid unnecessary memory assignment").
> > Unfortunately it causes the following kernel panic on Samsung
> > Exynos5433-based TM2e board:
> > 
> > exynos-pcie 15700000.pcie: host bridge /soc@0/pcie@15700000 ranges:
> > exynos-pcie 15700000.pcie:       IO 0x000c001000..0x000c010fff ->
> > 0x0000000000
> > exynos-pcie 15700000.pcie:      MEM 0x000c011000..0x000ffffffe ->
> > 0x000c011000
> > exynos-pcie 15700000.pcie: iATU: unroll F, 3 ob, 5 ib, align 4K, limit 4G
> > Unable to handle kernel paging request at virtual address ffff800084196010
> > Mem abort info:
> > ...
> > Data abort info:
> > ...
> > swapper pgtable: 4k pages, 48-bit VAs, pgdp=0000000022047000
> > [ffff800084196010] pgd=10000000df6ff003, p4d=10000000df6ff003,
> > pud=10000000df6fe003, pmd=1000000024ad9003, pte=0000000000000000
> > Internal error: Oops: 0000000096000047 [#1] PREEMPT SMP
> > Modules linked in:
> > CPU: 4 PID: 55 Comm: kworker/u18:0 Not tainted 6.6.0-rc1+ #14129
> > Hardware name: Samsung TM2E board (DT)
> > Workqueue: events_unbound deferred_probe_work_func
> > pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > pc : dw_pcie_write_dbi2+0xb8/0xc8
> > lr : dw_pcie_setup_rc+0x30/0x4e4
> > ...
> > Call trace:
> >   dw_pcie_write_dbi2+0xb8/0xc8
> >   dw_pcie_setup_rc+0x30/0x4e4
> >   dw_pcie_host_init+0x238/0x608
> >   exynos_pcie_probe+0x23c/0x340
> >   platform_probe+0x68/0xd8
> >   really_probe+0x148/0x2b4
> >   __driver_probe_device+0x78/0x12c
> >   driver_probe_device+0xd8/0x160
> >   __device_attach_driver+0xb8/0x138
> >   bus_for_each_drv+0x84/0xe0
> >   __device_attach+0xa8/0x1b0
> >   device_initial_probe+0x14/0x20
> >   bus_probe_device+0xb0/0xb4
> >   deferred_probe_work_func+0x8c/0xc8
> >   process_one_work+0x1ec/0x53c
> >   worker_thread+0x298/0x408
> >   kthread+0x124/0x128
> >   ret_from_fork+0x10/0x20
> > Code: d50332bf 79000023 17ffffe2 d50332bf (b9000023)
> > ---[ end trace 0000000000000000 ]---
> > Kernel panic - not syncing: Oops: Fatal exception
> > SMP: stopping secondary CPUs
> > Kernel Offset: disabled
> > CPU features: 0x8c00020e,3c020000,0000421b
> > Memory Limit: none
> > ---[ end Kernel panic - not syncing: Oops: Fatal exception ]---
> > 
> > I've observed similar issue on Qualcomm's RB5 platform with some
> > additional not-yet merged patches enabling PCIe support. Reverting
> > $subject on top of linux-next fixes this issue.
> > 
> > Let me know if you need more information.
> 
> Thank you for the report. I guess that the issue is related to
> out-of-range access of dbi2:
> - In arch/arm64/boot/dts/exynos/exynos5433.dtsi, the dbi reg size is 0x1000
>   like below:
> -----
>                 pcie: pcie@15700000 {
>                         compatible = "samsung,exynos5433-pcie";
>                         reg = <0x15700000 0x1000>, <0x156b0000 0x1000>,
>                               <0x0c000000 0x1000>;
>                         reg-names = "dbi", "elbi", "config";
> ...
> -----
> 
> - In drivers/pci/controller/dwc/pcie-designware.c, "dbi2" area is calculated
>   by the following if reg-names "dbi2" didn't exist:
> -----
>                         pci->dbi_base2 = pci->dbi_base + SZ_4K;
> -----
> 
> - However, this is out-of-memory on exynos5433.dtsi because the "dbi" size is
>   0x1000 only.
> - And then, this patch always writes PCI_BASE_ADDRESS_[01] to dbi2 area.
>   So, since this is out-of-range, the kernel panic happens.
> 

I could reproduce the issue Marek reported on RB5. As you pointed out, it is due
to not mapping dbi2 explicitly. But we were not using DBI2 on the host earlier
and it looks to me that DBI2 may not be implemented on Qcom host platforms.

Atleast on EP, I confirmed with Qcom that DBI=DBI2 as represented in the driver,
but I couldn't confirm if it is same for host as well.

> Perhaps, we should revert this patch at first. And, add the settings into
> my environment (pcie-rcar-gen4.c) only. I also have alternative solution which
> modifies the "dbi2" area calculation and avoid out-of-range access somehow.
> But, it may complicate source code...
> 

Yes, let's revert this patch for now and move it to rcar driver.

- Mani

> Best regards,
> Yoshihiro Shimoda
> 
> > > ---
> > >   drivers/pci/controller/dwc/pcie-designware-host.c | 8 ++++++++
> > >   1 file changed, 8 insertions(+)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > index a7170fd0e847..56cc7ff6d508 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > @@ -737,6 +737,14 @@ int dw_pcie_setup_rc(struct dw_pcie_rp *pp)
> > >   	u32 val, ctrl, num_ctrls;
> > >   	int ret;
> > >
> > > +	/*
> > > +	 * According to the section 3.5.7.2 "RC Mode" in DWC PCIe Dual Mode
> > > +	 * Rev.5.20a, 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);
> > > +
> > >   	/*
> > >   	 * Enable DBI read-only registers for writing/updating configuration.
> > >   	 * Write permission gets disabled towards the end of this function.
> > 
> > Best regards
> > --
> > Marek Szyprowski, PhD
> > Samsung R&D Institute Poland
>
Bjorn Helgaas Oct. 17, 2023, 6:58 p.m. UTC | #11
On Tue, Oct 17, 2023 at 11:19:21AM +0200, Marek Szyprowski wrote:
> Dear All,
> 
> On 11.10.2023 09:14, Yoshihiro Shimoda wrote:
> > According to the section 3.5.7.2 "RC Mode" in DWC PCIe Dual Mode
> > Rev.5.20a, we should disable two BARs to avoid unnecessary memory
> > assignment during device enumeration. Otherwise, Renesas R-Car Gen4
> > PCIe controllers cannot work correctly in host mode.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> 
> This patch landed in today's linux-next 20231017 as commit e308528cac3e 
> ("PCI: dwc: Disable two BARs to avoid unnecessary memory assignment"). 
> Unfortunately it causes the following kernel panic on Samsung 
> Exynos5433-based TM2e board:

I dropped the pci/controller/rcar branch for today until we resolve
this.

If you want to reorder the series like this:

  PCI: Add T_PVPERL macro
  PCI: dwc: Add dw_pcie_link_set_max_link_width()
  PCI: dwc: Add missing PCI_EXP_LNKCAP_MLW handling
  PCI: tegra194: Drop PCI_EXP_LNKSTA_NLW setting

  PCI: dwc: endpoint: Add multiple PFs support for dbi2
  PCI: dwc: Add EDMA_UNROLL capability flag
  PCI: dwc: Expose dw_pcie_ep_exit() to module
  PCI: dwc: endpoint: Introduce .pre_init() and .deinit()
  PCI: dwc: Disable two BARs to avoid unnecessary memory assignment
  dt-bindings: PCI: dwc: Update maxItems of reg and reg-names
  dt-bindings: PCI: renesas: Add R-Car Gen4 PCIe Host
  dt-bindings: PCI: renesas: Add R-Car Gen4 PCIe Endpoint
  PCI: rcar-gen4: Add R-Car Gen4 PCIe controller support for host mode
  PCI: rcar-gen4: Add endpoint mode support
  MAINTAINERS: Update PCI DRIVER FOR RENESAS R-CAR for R-Car Gen4
  misc: pci_endpoint_test: Add Device ID for R-Car S4-8 PCIe controller

I think the first four patches could be applied because they're useful
regardless of R-Car support.

The rest are only needed for R-Car, and it seems like we should apply
them all together as soon as this regression is solved.

Bjorn
Yoshihiro Shimoda Oct. 18, 2023, 12:11 a.m. UTC | #12
> From: mani@kernel.org, Sent: Wednesday, October 18, 2023 12:16 AM
> 
> On Tue, Oct 17, 2023 at 12:05:12PM +0000, Yoshihiro Shimoda wrote:
> > Dear Marek,
> >
> > > From: Marek Szyprowski, Sent: Tuesday, October 17, 2023 6:19 PM
> > >
> > > Dear All,
> > >
> > > On 11.10.2023 09:14, Yoshihiro Shimoda wrote:
> > > > According to the section 3.5.7.2 "RC Mode" in DWC PCIe Dual Mode
> > > > Rev.5.20a, we should disable two BARs to avoid unnecessary memory
> > > > assignment during device enumeration. Otherwise, Renesas R-Car Gen4
> > > > PCIe controllers cannot work correctly in host mode.
> > > >
> > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > >
> > > This patch landed in today's linux-next 20231017 as commit e308528cac3e
> > > ("PCI: dwc: Disable two BARs to avoid unnecessary memory assignment").
> > > Unfortunately it causes the following kernel panic on Samsung
> > > Exynos5433-based TM2e board:
> > >
> > > exynos-pcie 15700000.pcie: host bridge /soc@0/pcie@15700000 ranges:
> > > exynos-pcie 15700000.pcie:       IO 0x000c001000..0x000c010fff ->
> > > 0x0000000000
> > > exynos-pcie 15700000.pcie:      MEM 0x000c011000..0x000ffffffe ->
> > > 0x000c011000
> > > exynos-pcie 15700000.pcie: iATU: unroll F, 3 ob, 5 ib, align 4K, limit 4G
> > > Unable to handle kernel paging request at virtual address ffff800084196010
> > > Mem abort info:
> > > ...
> > > Data abort info:
> > > ...
> > > swapper pgtable: 4k pages, 48-bit VAs, pgdp=0000000022047000
> > > [ffff800084196010] pgd=10000000df6ff003, p4d=10000000df6ff003,
> > > pud=10000000df6fe003, pmd=1000000024ad9003, pte=0000000000000000
> > > Internal error: Oops: 0000000096000047 [#1] PREEMPT SMP
> > > Modules linked in:
> > > CPU: 4 PID: 55 Comm: kworker/u18:0 Not tainted 6.6.0-rc1+ #14129
> > > Hardware name: Samsung TM2E board (DT)
> > > Workqueue: events_unbound deferred_probe_work_func
> > > pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > > pc : dw_pcie_write_dbi2+0xb8/0xc8
> > > lr : dw_pcie_setup_rc+0x30/0x4e4
> > > ...
> > > Call trace:
> > >   dw_pcie_write_dbi2+0xb8/0xc8
> > >   dw_pcie_setup_rc+0x30/0x4e4
> > >   dw_pcie_host_init+0x238/0x608
> > >   exynos_pcie_probe+0x23c/0x340
> > >   platform_probe+0x68/0xd8
> > >   really_probe+0x148/0x2b4
> > >   __driver_probe_device+0x78/0x12c
> > >   driver_probe_device+0xd8/0x160
> > >   __device_attach_driver+0xb8/0x138
> > >   bus_for_each_drv+0x84/0xe0
> > >   __device_attach+0xa8/0x1b0
> > >   device_initial_probe+0x14/0x20
> > >   bus_probe_device+0xb0/0xb4
> > >   deferred_probe_work_func+0x8c/0xc8
> > >   process_one_work+0x1ec/0x53c
> > >   worker_thread+0x298/0x408
> > >   kthread+0x124/0x128
> > >   ret_from_fork+0x10/0x20
> > > Code: d50332bf 79000023 17ffffe2 d50332bf (b9000023)
> > > ---[ end trace 0000000000000000 ]---
> > > Kernel panic - not syncing: Oops: Fatal exception
> > > SMP: stopping secondary CPUs
> > > Kernel Offset: disabled
> > > CPU features: 0x8c00020e,3c020000,0000421b
> > > Memory Limit: none
> > > ---[ end Kernel panic - not syncing: Oops: Fatal exception ]---
> > >
> > > I've observed similar issue on Qualcomm's RB5 platform with some
> > > additional not-yet merged patches enabling PCIe support. Reverting
> > > $subject on top of linux-next fixes this issue.
> > >
> > > Let me know if you need more information.
> >
> > Thank you for the report. I guess that the issue is related to
> > out-of-range access of dbi2:
> > - In arch/arm64/boot/dts/exynos/exynos5433.dtsi, the dbi reg size is 0x1000
> >   like below:
> > -----
> >                 pcie: pcie@15700000 {
> >                         compatible = "samsung,exynos5433-pcie";
> >                         reg = <0x15700000 0x1000>, <0x156b0000 0x1000>,
> >                               <0x0c000000 0x1000>;
> >                         reg-names = "dbi", "elbi", "config";
> > ...
> > -----
> >
> > - In drivers/pci/controller/dwc/pcie-designware.c, "dbi2" area is calculated
> >   by the following if reg-names "dbi2" didn't exist:
> > -----
> >                         pci->dbi_base2 = pci->dbi_base + SZ_4K;
> > -----
> >
> > - However, this is out-of-memory on exynos5433.dtsi because the "dbi" size is
> >   0x1000 only.
> > - And then, this patch always writes PCI_BASE_ADDRESS_[01] to dbi2 area.
> >   So, since this is out-of-range, the kernel panic happens.
> >
> 
> I could reproduce the issue Marek reported on RB5. As you pointed out, it is due
> to not mapping dbi2 explicitly. But we were not using DBI2 on the host earlier
> and it looks to me that DBI2 may not be implemented on Qcom host platforms.
> 
> Atleast on EP, I confirmed with Qcom that DBI=DBI2 as represented in the driver,
> but I couldn't confirm if it is same for host as well.

Thank you for the confirmation. I understood it.

> > Perhaps, we should revert this patch at first. And, add the settings into
> > my environment (pcie-rcar-gen4.c) only. I also have alternative solution which
> > modifies the "dbi2" area calculation and avoid out-of-range access somehow.
> > But, it may complicate source code...
> >
> 
> Yes, let's revert this patch for now and move it to rcar driver.

I got it.

Best regards,
Yoshihiro Shimoda

> - Mani
> 
> > Best regards,
> > Yoshihiro Shimoda
> >
> > > > ---
> > > >   drivers/pci/controller/dwc/pcie-designware-host.c | 8 ++++++++
> > > >   1 file changed, 8 insertions(+)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > index a7170fd0e847..56cc7ff6d508 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > @@ -737,6 +737,14 @@ int dw_pcie_setup_rc(struct dw_pcie_rp *pp)
> > > >   	u32 val, ctrl, num_ctrls;
> > > >   	int ret;
> > > >
> > > > +	/*
> > > > +	 * According to the section 3.5.7.2 "RC Mode" in DWC PCIe Dual Mode
> > > > +	 * Rev.5.20a, 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);
> > > > +
> > > >   	/*
> > > >   	 * Enable DBI read-only registers for writing/updating configuration.
> > > >   	 * Write permission gets disabled towards the end of this function.
> > >
> > > Best regards
> > > --
> > > Marek Szyprowski, PhD
> > > Samsung R&D Institute Poland
> >
> 
> --
> மணிவண்ணன் சதாசிவம்
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index a7170fd0e847..56cc7ff6d508 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -737,6 +737,14 @@  int dw_pcie_setup_rc(struct dw_pcie_rp *pp)
 	u32 val, ctrl, num_ctrls;
 	int ret;
 
+	/*
+	 * According to the section 3.5.7.2 "RC Mode" in DWC PCIe Dual Mode
+	 * Rev.5.20a, 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);
+
 	/*
 	 * Enable DBI read-only registers for writing/updating configuration.
 	 * Write permission gets disabled towards the end of this function.