RE: [PATCH v1 1/2] PCI: xilinx-nwl: Update ECAM default value and remove unnecessary code.

[Havalige, Thippeswamy]

Hi Bjorn,

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Sent: Tuesday, August 8, 2023 2:53 AM
> To: Havalige, Thippeswamy <thippeswamy.havalige@xxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx;
> bhelgaas@xxxxxxxxxx; krzysztof.kozlowski@xxxxxxxxxx; linux-
> pci@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; lpieralisi@xxxxxxxxxx;
> Gogada, Bharat Kumar <bharat.kumar.gogada@xxxxxxx>; Simek, Michal
> <michal.simek@xxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v1 1/2] PCI: xilinx-nwl: Update ECAM default value and
> remove unnecessary code.
> 
> Ideally the subject would include useful information about *why* you're
> changing the ECAM value.  Drop the period at the end of the subject.  E.g.,
> something like:
> 
>   PCI: xilinx-nwl: Increase ECAM size to accommodate 256 buses
- Agreed, I ll update in next patch.
> On Mon, Aug 07, 2023 at 04:37:32PM +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.
> Rob needs to ack this because it sounds like this change might make the
> driver incompatible with DTs in the field, i.e., the user might be forced to
> update the DT at the same time as picking up this driver change.
> 
> > E_ECAM_CONTROL register from bit 16 to 20 uses this value as input to
> > calculate ECAM Size.
> >
> > The primary,secondary and sub-ordinate bus number registers are
> > updated by Linux PCI core, so removing code which is updating
> > primary,secondary and sub-ordinate bus numbers of type 1 header 18th
> offset of ECAM.
> 
> This code removal sounds like a separate logical change that could be a
> separate patch.
> 
> s/primary,secondary/primary, secondary/ (twice) s/removing/remove/
- Agreed, I ll update in next patch.
> > Signed-off-by: Thippeswamy Havalige <thippeswamy.havalige@xxxxxxx>
> > Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xxxxxxx>
> > ---
> > | Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > | Closes:
> > | https://lore.kernel.org/oe-kbuild-all/202308040330.eMTjX3tF-lkp@intel.
> > | com/
> > ---
> >  drivers/pci/controller/pcie-xilinx-nwl.c | 18 +++---------------
> >  1 file changed, 3 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-xilinx-nwl.c
> > b/drivers/pci/controller/pcie-xilinx-nwl.c
> > index 176686b..b515019 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,8 +165,6 @@ struct nwl_pcie {
> >  	u32 ecam_size;
> >  	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 +623,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; @@ -675,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),
> > @@ -683,15 +681,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
> > @@ -792,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) {
> > --
> > 1.8.3.1
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel