diff mbox series

[v3,2/2] PCI: xilinx-nwl: Increase ECAM size to accommodate 256 buses

Message ID 20230810122002.133531-4-thippeswamy.havalige@amd.com (mailing list archive)
State New, archived
Headers show
Series [v3,1/2] dt-bindings: PCI: xilinx-nwl: Modify ECAM size in example | expand

Commit Message

Thippeswamy Havalige Aug. 10, 2023, 12:20 p.m. UTC 
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 <thippeswamy.havalige@amd.com>
Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@amd.com>
---
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(-)

Comments

Rob Herring (Arm) Aug. 10, 2023, 9:17 p.m. UTC | #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 <thippeswamy.havalige@amd.com>
> Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@amd.com>
> ---
> 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
>
Thippeswamy Havalige Aug. 11, 2023, 5:07 a.m. UTC | #2 
Hi Rob,

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Friday, August 11, 2023 2:47 AM
> To: Havalige, Thippeswamy <thippeswamy.havalige@amd.com>
> Cc: linux-kernel@vger.kernel.org; bhelgaas@google.com;
> krzysztof.kozlowski@linaro.org; linux-pci@vger.kernel.org;
> devicetree@vger.kernel.org; conor+dt@kernel.org; lpieralisi@kernel.org;
> Gogada, Bharat Kumar <bharat.kumar.gogada@amd.com>; Simek, Michal
> <michal.simek@amd.com>; linux-arm-kernel@lists.infradead.org
> 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 <thippeswamy.havalige@amd.com>
> > Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@amd.com>
> > ---
> > 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
> >
Bjorn Helgaas Aug. 11, 2023, 4:51 p.m. UTC | #3 
On Fri, Aug 11, 2023 at 05:07:09AM +0000, Havalige, Thippeswamy wrote:
> > -----Original Message-----
> > From: Rob Herring <robh@kernel.org>
> > 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 <thippeswamy.havalige@amd.com>
> > > Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@amd.com>
> > > ---
> > > 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
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff mbox series

Patch

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) {