| Message ID | 20230810122002.133531-4-thippeswamy.havalige@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] PCI: xilinx-nwl: Remove unnecessary code which updates primary, secondary and sub-ordinate bus numbers | expand |
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 >
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 > >
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 --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) {