Re: [PATCH] PCI: xilinx-nwl: Remove unnecessary code and updating ecam default value.

[Bjorn Helgaas]

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.

> > 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?

Bjorn