[PATCH v3] PCI: xilinx-nwl: Remove unnecessary code which updates primary, secondary and sub-ordinate bus numbers

[PATCH v3] PCI: xilinx-nwl: Remove unnecessary code which updates primary, secondary and sub-ordinate bus numbers

[Thippeswamy Havalige]

The primary,secondary and sub-ordinate bus number registers are updated by
Linux PCI core, so remove code which updates respective fields of type 1
header.

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.
- Updated commit message.
changes in v2:
- Code increasing ECAM Size value is added into a seperate patch.
- Modified commit messages.
changes in v1:
- Modified commit messages.
---
 drivers/pci/controller/pcie-xilinx-nwl.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/pci/controller/pcie-xilinx-nwl.c b/drivers/pci/controller/pcie-xilinx-nwl.c
index 176686bdb15c..d8a3a08be1d5 100644
--- a/drivers/pci/controller/pcie-xilinx-nwl.c
+++ b/drivers/pci/controller/pcie-xilinx-nwl.c
@@ -166,7 +166,6 @@ struct nwl_pcie {
 	int irq_intx;
 	int irq_misc;
 	u32 ecam_value;
-	u8 last_busno;
 	struct nwl_msi msi;
 	struct irq_domain *legacy_irq_domain;
 	struct clk *clk;
@@ -625,7 +624,7 @@ static int nwl_pcie_bridge_init(struct nwl_pcie *pcie)
 {
 	struct device *dev = pcie->dev;
 	struct platform_device *pdev = to_platform_device(dev);
-	u32 breg_val, ecam_val, first_busno = 0;
+	u32 breg_val, ecam_val;
 	int err;
 
 	breg_val = nwl_bridge_readl(pcie, E_BREG_CAPABILITIES) & BREG_PRESENT;
@@ -683,15 +682,6 @@ static int nwl_pcie_bridge_init(struct nwl_pcie *pcie)
 	nwl_bridge_writel(pcie, upper_32_bits(pcie->phys_ecam_base),
 			  E_ECAM_BASE_HI);
 
-	/* Get bus range */
-	ecam_val = nwl_bridge_readl(pcie, E_ECAM_CONTROL);
-	pcie->last_busno = (ecam_val & E_ECAM_SIZE_LOC) >> E_ECAM_SIZE_SHIFT;
-	/* Write primary, secondary and subordinate bus numbers */
-	ecam_val = first_busno;
-	ecam_val |= (first_busno + 1) << 8;
-	ecam_val |= (pcie->last_busno << E_ECAM_SIZE_SHIFT);
-	writel(ecam_val, (pcie->ecam_base + PCI_PRIMARY_BUS));
-
 	if (nwl_pcie_link_up(pcie))
 		dev_info(dev, "Link is UP\n");
 	else
-- 
2.17.1

[PATCH v3 0/2] Increase ecam size value to discover 256 buses during

[Thippeswamy Havalige]

Current driver is supports up to 16 buses. The following code fixes 
to support up to 256 buses.

update "NWL_ECAM_VALUE_DEFAULT " to 16  can access up to 256MB ECAM
region to detect 256 buses.

Update ecam size to 256MB in device tree binding example.

Thippeswamy Havalige (2):
  dt-bindings: PCI: xilinx-nwl: Modify ECAM size in example
  PCI: xilinx-nwl: Increase ECAM size to accommodate 256 buses

 Documentation/devicetree/bindings/pci/xlnx,nwl-pcie.yaml | 2 +-
 drivers/pci/controller/pcie-xilinx-nwl.c                 | 6 ++----
 2 files changed, 3 insertions(+), 5 deletions(-)

-- 
2.17.1

[PATCH v3 1/2] dt-bindings: PCI: xilinx-nwl: Modify ECAM size in example

[Thippeswamy Havalige]

Update ECAM size in example to discover up to 256 buses.

Signed-off-by: Thippeswamy Havalige <thippeswamy.havalige@amd.com>
Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@amd.com>
---
changes in v2:
None.
changes in v1:
None.
---
 Documentation/devicetree/bindings/pci/xlnx,nwl-pcie.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pci/xlnx,nwl-pcie.yaml b/Documentation/devicetree/bindings/pci/xlnx,nwl-pcie.yaml
index 897602559b37..426f90a47f35 100644
--- a/Documentation/devicetree/bindings/pci/xlnx,nwl-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/xlnx,nwl-pcie.yaml
@@ -118,7 +118,7 @@ examples:
             compatible = "xlnx,nwl-pcie-2.11";
             reg = <0x0 0xfd0e0000 0x0 0x1000>,
                   <0x0 0xfd480000 0x0 0x1000>,
-                  <0x80 0x00000000 0x0 0x1000000>;
+                  <0x80 0x00000000 0x0 0x10000000>;
             reg-names = "breg", "pcireg", "cfg";
             ranges = <0x02000000 0x0 0xe0000000 0x0 0xe0000000 0x0 0x10000000>,
                      <0x43000000 0x00000006 0x0 0x00000006 0x0 0x00000002 0x0>;
-- 
2.17.1

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

[Thippeswamy Havalige]

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

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

Re: [PATCH v3 1/2] dt-bindings: PCI: xilinx-nwl: Modify ECAM size in example

[Rob Herring]

On Thu, 10 Aug 2023 17:50:01 +0530, Thippeswamy Havalige wrote:
> Update ECAM size in example to discover up to 256 buses.
> 
> Signed-off-by: Thippeswamy Havalige <thippeswamy.havalige@amd.com>
> Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@amd.com>
> ---
> changes in v2:
> None.
> changes in v1:
> None.
> ---
>  Documentation/devicetree/bindings/pci/xlnx,nwl-pcie.yaml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:


doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230810122002.133531-3-thippeswamy.havalige@amd.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

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

[Rob Herring]

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
> 

Re: [PATCH v3 1/2] dt-bindings: PCI: xilinx-nwl: Modify ECAM size in example

[Rob Herring]

On Thu, 10 Aug 2023 17:50:01 +0530, Thippeswamy Havalige wrote:
> Update ECAM size in example to discover up to 256 buses.
> 
> Signed-off-by: Thippeswamy Havalige <thippeswamy.havalige@amd.com>
> Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@amd.com>
> ---
> changes in v2:
> None.
> changes in v1:
> None.
> ---
>  Documentation/devicetree/bindings/pci/xlnx,nwl-pcie.yaml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

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

[Havalige, Thippeswamy]

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

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

[Bjorn Helgaas]

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