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

[Bjorn Helgaas]

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?

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.

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

Does this fix a problem?

> 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