Hi Bjorn, > -----Original Message----- > From: Bjorn Helgaas <helgaas@xxxxxxxxxx> > Sent: Thursday, August 3, 2023 10:26 PM > To: Havalige, Thippeswamy <thippeswamy.havalige@xxxxxxx> > Cc: linux-kernel@xxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx; > bhelgaas@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; > krzysztof.kozlowski@xxxxxxxxxx; lpieralisi@xxxxxxxxxx; Gogada, Bharat Kumar > <bharat.kumar.gogada@xxxxxxx>; Simek, Michal > <michal.simek@xxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH] PCI: xilinx-nwl: Remove unnecessary code and updating > ecam default value. > > On Thu, Aug 03, 2023 at 05:20:16PM +0530, Thippeswamy Havalige wrote: > > Remove reduntant code. > > Change NWL_ECAM_VALUE_DEFAULT to 16 to support maximum 256 > buses. > > Remove period from subject line. > > Please mention the most important part first in the subject -- the > ECAM change sounds more important than removing redundant code. > > s/ecam/ECAM/ > s/reduntant/redundant/ > > Please elaborate on why this code is redundant. What makes it > redundant? Apparently the bus number registers default to the correct > values or some other software programs them? - Yes, The Primary,Secondary and sub-ordinate bus number registers are programmed/updated as part of linux enumeration so updating these registers are redundant. > I don't see the point of the struct nwl_pcie.ecam_value member. It is > set once and never updated, so we could just use > NWL_ECAM_VALUE_DEFAULT instead. -Agreed, I ll update it in next patch. > "ECAM_VALUE" is not a very informative name. I don't know what an > "ECAM value" would be. How is the value 16 related to a maximum of > 256 buses? We only need 8 bits to address 256 buses, so it must be > something else. The bus number appears at bits 20-27 > (PCIE_ECAM_BUS_SHIFT) in a standard ECAM address, so probably not the > bit location? Yes, Agreed I'll modify ECAM_VALUE as ECAM_SIZE here and it is not related to a maximum 256 buses. > Does this fix a problem? - Yes, It is fixing a problem. Our controller is expecting ECAM size to be programmed by software. By programming "NWL_ECAM_VALUE_DEFAULT 12" controller can access upto 16MB ECAM region which is used to detect 16 buses so by updating "NWL_ECAM_VALUE_DEFAULT " to 16 controller can access upto 256 Mb ECAM region to detect 256 buses. 2^(ecam_size_offset+ecam_size) Here (ecam_size_offset=12 and ecam_size=16) --> 256Mb > > Signed-off-by: Thippeswamy Havalige <thippeswamy.havalige@xxxxxxx> > > --- > > drivers/pci/controller/pcie-xilinx-nwl.c | 11 +---------- > > 1 file changed, 1 insertion(+), 10 deletions(-) > > > > diff --git a/drivers/pci/controller/pcie-xilinx-nwl.c > b/drivers/pci/controller/pcie-xilinx-nwl.c > > index 176686b..6d40543 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) > > @@ -683,15 +683,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)); > > "ecam_val" looks like it's supposed to be the 32-bit value containing > PCI_PRIMARY_BUS (low 8 bits, from the pointless "first_busno" that is > always 0), PCI_SECONDARY_BUS (bits 8-15, always bus 1), > PCI_SUBORDINATE_BUS (bits 16-23, totally unrelated to > E_ECAM_SIZE_SHIFT although E_ECAM_SIZE_SHIFT happens to be the correct > value (16)), and PCI_SEC_LATENCY_TIMER (not applicable for PCIe). > > So I guess the assumption is that these registers already contain the > correct values? > > It looks like previously PCI_SUBORDINATE_BUS (i.e., pcie->last_busno) > was 12, since we wrote NWL_ECAM_VALUE_DEFAULT to E_ECAM_CONTROL > and > then read it back? > > And now pcie->last_busno is competely unused? > > This all seems not quite baked. Am I missing something? > > Bjorn
