Our controller is expecting ECAM size to be programmed by software. By
programming "NWL_ECAM_VALUE_DEFAULT 12" controller can access up to 16MB
ECAM region which is used to detect 16 buses, so by updating
"NWL_ECAM_VALUE_DEFAULT" to 16 so that controller can access up to 256MB
ECAM region to detect 256 buses.
Signed-off-by: Thippeswamy Havalige <[email protected]>
Signed-off-by: Bharat Kumar Gogada <[email protected]>
---
changes in v3:
- Remove unnecessary period at end of subject line.
changes in v2:
- Update this changes in a seperate patch.
---
drivers/pci/controller/pcie-xilinx-nwl.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/controller/pcie-xilinx-nwl.c b/drivers/pci/controller/pcie-xilinx-nwl.c
index d8a3a08be1d5..b51501921d3b 100644
--- a/drivers/pci/controller/pcie-xilinx-nwl.c
+++ b/drivers/pci/controller/pcie-xilinx-nwl.c
@@ -126,7 +126,7 @@
#define E_ECAM_CR_ENABLE BIT(0)
#define E_ECAM_SIZE_LOC GENMASK(20, 16)
#define E_ECAM_SIZE_SHIFT 16
-#define NWL_ECAM_VALUE_DEFAULT 12
+#define NWL_ECAM_VALUE_DEFAULT 16
#define CFG_DMA_REG_BAR GENMASK(2, 0)
#define CFG_PCIE_CACHE GENMASK(7, 0)
@@ -165,7 +165,6 @@ struct nwl_pcie {
u32 ecam_size;
int irq_intx;
int irq_misc;
- u32 ecam_value;
struct nwl_msi msi;
struct irq_domain *legacy_irq_domain;
struct clk *clk;
@@ -674,7 +673,7 @@ static int nwl_pcie_bridge_init(struct nwl_pcie *pcie)
E_ECAM_CR_ENABLE, E_ECAM_CONTROL);
nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, E_ECAM_CONTROL) |
- (pcie->ecam_value << E_ECAM_SIZE_SHIFT),
+ (NWL_ECAM_VALUE_DEFAULT << E_ECAM_SIZE_SHIFT),
E_ECAM_CONTROL);
nwl_bridge_writel(pcie, lower_32_bits(pcie->phys_ecam_base),
@@ -782,7 +781,6 @@ static int nwl_pcie_probe(struct platform_device *pdev)
pcie = pci_host_bridge_priv(bridge);
pcie->dev = dev;
- pcie->ecam_value = NWL_ECAM_VALUE_DEFAULT;
err = nwl_pcie_parse_dt(pcie, pdev);
if (err) {
--
2.17.1
On Thu, Aug 10, 2023 at 05:50:02PM +0530, Thippeswamy Havalige wrote:
> Our controller is expecting ECAM size to be programmed by software. By
> programming "NWL_ECAM_VALUE_DEFAULT 12" controller can access up to 16MB
> ECAM region which is used to detect 16 buses, so by updating
> "NWL_ECAM_VALUE_DEFAULT" to 16 so that controller can access up to 256MB
> ECAM region to detect 256 buses.
What happens when your DT has the smaller size and the kernel configures
the larger size? Seems like you could have an ABI issue.
>
> Signed-off-by: Thippeswamy Havalige <[email protected]>
> Signed-off-by: Bharat Kumar Gogada <[email protected]>
> ---
> changes in v3:
> - Remove unnecessary period at end of subject line.
> changes in v2:
> - Update this changes in a seperate patch.
> ---
> drivers/pci/controller/pcie-xilinx-nwl.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-xilinx-nwl.c b/drivers/pci/controller/pcie-xilinx-nwl.c
> index d8a3a08be1d5..b51501921d3b 100644
> --- a/drivers/pci/controller/pcie-xilinx-nwl.c
> +++ b/drivers/pci/controller/pcie-xilinx-nwl.c
> @@ -126,7 +126,7 @@
> #define E_ECAM_CR_ENABLE BIT(0)
> #define E_ECAM_SIZE_LOC GENMASK(20, 16)
> #define E_ECAM_SIZE_SHIFT 16
> -#define NWL_ECAM_VALUE_DEFAULT 12
> +#define NWL_ECAM_VALUE_DEFAULT 16
Not really a meaningful name. It doesn't explain what '16' means.
> #define CFG_DMA_REG_BAR GENMASK(2, 0)
> #define CFG_PCIE_CACHE GENMASK(7, 0)
> @@ -165,7 +165,6 @@ struct nwl_pcie {
> u32 ecam_size;
> int irq_intx;
> int irq_misc;
> - u32 ecam_value;
> struct nwl_msi msi;
> struct irq_domain *legacy_irq_domain;
> struct clk *clk;
> @@ -674,7 +673,7 @@ static int nwl_pcie_bridge_init(struct nwl_pcie *pcie)
> E_ECAM_CR_ENABLE, E_ECAM_CONTROL);
>
> nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, E_ECAM_CONTROL) |
> - (pcie->ecam_value << E_ECAM_SIZE_SHIFT),
> + (NWL_ECAM_VALUE_DEFAULT << E_ECAM_SIZE_SHIFT),
> E_ECAM_CONTROL);
>
> nwl_bridge_writel(pcie, lower_32_bits(pcie->phys_ecam_base),
> @@ -782,7 +781,6 @@ static int nwl_pcie_probe(struct platform_device *pdev)
> pcie = pci_host_bridge_priv(bridge);
>
> pcie->dev = dev;
> - pcie->ecam_value = NWL_ECAM_VALUE_DEFAULT;
>
> err = nwl_pcie_parse_dt(pcie, pdev);
> if (err) {
> --
> 2.17.1
>
Hi Rob,
> -----Original Message-----
> From: Rob Herring <[email protected]>
> Sent: Friday, August 11, 2023 2:47 AM
> To: Havalige, Thippeswamy <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> Gogada, Bharat Kumar <[email protected]>; Simek, Michal
> <[email protected]>; [email protected]
> Subject: Re: [PATCH v3 2/2] PCI: xilinx-nwl: Increase ECAM size to
> accommodate 256 buses
>
> On Thu, Aug 10, 2023 at 05:50:02PM +0530, Thippeswamy Havalige wrote:
> > Our controller is expecting ECAM size to be programmed by software. By
> > programming "NWL_ECAM_VALUE_DEFAULT 12" controller can access up to
> > 16MB ECAM region which is used to detect 16 buses, so by updating
> > "NWL_ECAM_VALUE_DEFAULT" to 16 so that controller can access up to
> > 256MB ECAM region to detect 256 buses.
>
> What happens when your DT has the smaller size and the kernel configures
> the larger size? Seems like you could have an ABI issue.
- Here we are enabling hardware to support maximum buses. In this case kernel can enumerate up to device tree exposed ECAM size.
We will not face any issue.
> >
> > Signed-off-by: Thippeswamy Havalige <[email protected]>
> > Signed-off-by: Bharat Kumar Gogada <[email protected]>
> > ---
> > changes in v3:
> > - Remove unnecessary period at end of subject line.
> > changes in v2:
> > - Update this changes in a seperate patch.
> > ---
> > drivers/pci/controller/pcie-xilinx-nwl.c | 6 ++----
> > 1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-xilinx-nwl.c
> > b/drivers/pci/controller/pcie-xilinx-nwl.c
> > index d8a3a08be1d5..b51501921d3b 100644
> > --- a/drivers/pci/controller/pcie-xilinx-nwl.c
> > +++ b/drivers/pci/controller/pcie-xilinx-nwl.c
> > @@ -126,7 +126,7 @@
> > #define E_ECAM_CR_ENABLE BIT(0)
> > #define E_ECAM_SIZE_LOC GENMASK(20, 16)
> > #define E_ECAM_SIZE_SHIFT 16
> > -#define NWL_ECAM_VALUE_DEFAULT 12
> > +#define NWL_ECAM_VALUE_DEFAULT 16
- Agreed, ll fix it in next patch.
> Not really a meaningful name. It doesn't explain what '16' means.
>
> > #define CFG_DMA_REG_BAR GENMASK(2, 0)
> > #define CFG_PCIE_CACHE GENMASK(7, 0)
> > @@ -165,7 +165,6 @@ struct nwl_pcie {
> > u32 ecam_size;
> > int irq_intx;
> > int irq_misc;
> > - u32 ecam_value;
> > struct nwl_msi msi;
> > struct irq_domain *legacy_irq_domain;
> > struct clk *clk;
> > @@ -674,7 +673,7 @@ static int nwl_pcie_bridge_init(struct nwl_pcie *pcie)
> > E_ECAM_CR_ENABLE, E_ECAM_CONTROL);
> >
> > nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, E_ECAM_CONTROL) |
> > - (pcie->ecam_value << E_ECAM_SIZE_SHIFT),
> > + (NWL_ECAM_VALUE_DEFAULT <<
> E_ECAM_SIZE_SHIFT),
> > E_ECAM_CONTROL);
> >
> > nwl_bridge_writel(pcie, lower_32_bits(pcie->phys_ecam_base),
> > @@ -782,7 +781,6 @@ static int nwl_pcie_probe(struct platform_device
> *pdev)
> > pcie = pci_host_bridge_priv(bridge);
> >
> > pcie->dev = dev;
> > - pcie->ecam_value = NWL_ECAM_VALUE_DEFAULT;
> >
> > err = nwl_pcie_parse_dt(pcie, pdev);
> > if (err) {
> > --
> > 2.17.1
> >
On Fri, Aug 11, 2023 at 05:07:09AM +0000, Havalige, Thippeswamy wrote:
> > -----Original Message-----
> > From: Rob Herring <[email protected]>
> > On Thu, Aug 10, 2023 at 05:50:02PM +0530, Thippeswamy Havalige wrote:
> > > Our controller is expecting ECAM size to be programmed by software. By
> > > programming "NWL_ECAM_VALUE_DEFAULT 12" controller can access up to
> > > 16MB ECAM region which is used to detect 16 buses, so by updating
> > > "NWL_ECAM_VALUE_DEFAULT" to 16 so that controller can access up to
> > > 256MB ECAM region to detect 256 buses.
> >
> > What happens when your DT has the smaller size and the kernel configures
> > the larger size? Seems like you could have an ABI issue.
>
> - Here we are enabling hardware to support maximum buses. In this
> case kernel can enumerate up to device tree exposed ECAM size. We
> will not face any issue.
So IIUC, if you have a DT with the smaller size and you boot a kernel
that includes this change, nothing will break, but the kernel will
only be able to use 16 buses.
Conversely, if you have a DT with the larger size and boot a kernel
that does not include change, nothing will break, but the kernel will
still only be able to use 16 buses.
Probably worth capturing this in the commit log somehow, especially
the first case.
> > > Signed-off-by: Thippeswamy Havalige <[email protected]>
> > > Signed-off-by: Bharat Kumar Gogada <[email protected]>
> > > ---
> > > changes in v3:
> > > - Remove unnecessary period at end of subject line.
> > > changes in v2:
> > > - Update this changes in a seperate patch.
> > > ---
> > > drivers/pci/controller/pcie-xilinx-nwl.c | 6 ++----
> > > 1 file changed, 2 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/pcie-xilinx-nwl.c
> > > b/drivers/pci/controller/pcie-xilinx-nwl.c
> > > index d8a3a08be1d5..b51501921d3b 100644
> > > --- a/drivers/pci/controller/pcie-xilinx-nwl.c
> > > +++ b/drivers/pci/controller/pcie-xilinx-nwl.c
> > > @@ -126,7 +126,7 @@
> > > #define E_ECAM_CR_ENABLE BIT(0)
> > > #define E_ECAM_SIZE_LOC GENMASK(20, 16)
> > > #define E_ECAM_SIZE_SHIFT 16
> > > -#define NWL_ECAM_VALUE_DEFAULT 12
> > > +#define NWL_ECAM_VALUE_DEFAULT 16
> - Agreed, ll fix it in next patch.
> > Not really a meaningful name. It doesn't explain what '16' means.
> >
> > > #define CFG_DMA_REG_BAR GENMASK(2, 0)
> > > #define CFG_PCIE_CACHE GENMASK(7, 0)
> > > @@ -165,7 +165,6 @@ struct nwl_pcie {
> > > u32 ecam_size;
> > > int irq_intx;
> > > int irq_misc;
> > > - u32 ecam_value;
> > > struct nwl_msi msi;
> > > struct irq_domain *legacy_irq_domain;
> > > struct clk *clk;
> > > @@ -674,7 +673,7 @@ static int nwl_pcie_bridge_init(struct nwl_pcie *pcie)
> > > E_ECAM_CR_ENABLE, E_ECAM_CONTROL);
> > >
> > > nwl_bridge_writel(pcie, nwl_bridge_readl(pcie, E_ECAM_CONTROL) |
> > > - (pcie->ecam_value << E_ECAM_SIZE_SHIFT),
> > > + (NWL_ECAM_VALUE_DEFAULT <<
> > E_ECAM_SIZE_SHIFT),
> > > E_ECAM_CONTROL);
> > >
> > > nwl_bridge_writel(pcie, lower_32_bits(pcie->phys_ecam_base),
> > > @@ -782,7 +781,6 @@ static int nwl_pcie_probe(struct platform_device
> > *pdev)
> > > pcie = pci_host_bridge_priv(bridge);
> > >
> > > pcie->dev = dev;
> > > - pcie->ecam_value = NWL_ECAM_VALUE_DEFAULT;
> > >
> > > err = nwl_pcie_parse_dt(pcie, pdev);
> > > if (err) {
> > > --
> > > 2.17.1
> > >
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel