Hi Bjorn, > -----Original Message----- > From: Bjorn Helgaas <helgaas@xxxxxxxxxx> > Sent: Saturday, August 5, 2023 1:28 AM > 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 Fri, Aug 04, 2023 at 07:05:30PM +0000, Havalige, Thippeswamy wrote: > > > -----Original Message----- > > > From: Bjorn Helgaas <helgaas@xxxxxxxxxx> > > > 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. > > Ah, so the Linux PCI core can handle updating these from whatever the > power-up values are. Good material for the revised commit log. > > > > "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. > > Well, it sounds like this value *does* determine the size of the ECAM > region, which does constrain the number of buses you can address via > ECAM. > - Yes, This ecam_size does determine the number of buses can be addressed via ECAM. > > > 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 > > More good material for the commit log. (1) Change in ECAM region > size, (2) previously could only address 16 buses, now can address 256 > buses. > > Is there any impact on DT from the address map change? > - Yes. Now device tree ECAM size needs to be modified to 256Mb. - I'll update device tree changes along with next patch. > Bjorn Regards, Thippeswamy H
