[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <SN7PR12MB72014E79448FE6C7A47718E28B09A@SN7PR12MB7201.namprd12.prod.outlook.com>
Date: Fri, 4 Aug 2023 19:05:30 +0000
From: "Havalige, Thippeswamy" <thippeswamy.havalige@....com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"bhelgaas@...gle.com" <bhelgaas@...gle.com>,
"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
"krzysztof.kozlowski@...aro.org" <krzysztof.kozlowski@...aro.org>,
"lpieralisi@...nel.org" <lpieralisi@...nel.org>,
"Gogada, Bharat Kumar" <bharat.kumar.gogada@....com>,
"Simek, Michal" <michal.simek@....com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>
Subject: RE: [PATCH] PCI: xilinx-nwl: Remove unnecessary code and updating
ecam default value.
Hi Bjorn,
> -----Original Message-----
> From: Bjorn Helgaas <helgaas@...nel.org>
> Sent: Thursday, August 3, 2023 10:26 PM
> To: Havalige, Thippeswamy <thippeswamy.havalige@....com>
> Cc: linux-kernel@...r.kernel.org; robh+dt@...nel.org;
> bhelgaas@...gle.com; linux-pci@...r.kernel.org;
> krzysztof.kozlowski@...aro.org; lpieralisi@...nel.org; Gogada, Bharat Kumar
> <bharat.kumar.gogada@....com>; Simek, Michal
> <michal.simek@....com>; linux-arm-kernel@...ts.infradead.org
> 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@....com>
> > ---
> > 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
Powered by blists - more mailing lists