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

Bjorn Helgaas
2023-08-03T16:56:20Z

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@amd.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