Re: [PATCH 2/5] PCI: sg2042: Add Sophgo SG2042 PCIe driver

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Nov 29, 2024 at 05:51:39PM +0800, Chen Wang wrote:
> On 2024/11/13 5:20, Bjorn Helgaas wrote:
> > On Mon, Nov 11, 2024 at 01:59:56PM +0800, Chen Wang wrote:
> > > From: Chen Wang <unicorn_wang@outlook.com>
> > > 
> > > Add support for PCIe controller in SG2042 SoC. The controller
> > > uses the Cadence PCIe core programmed by pcie-cadence*.c. The
> > > PCIe controller will work in host mode only.
> > > +++ b/drivers/pci/controller/cadence/Kconfig
> > > @@ -67,4 +67,15 @@ config PCI_J721E_EP
> > >   	  Say Y here if you want to support the TI J721E PCIe platform
> > >   	  controller in endpoint mode. TI J721E PCIe controller uses Cadence PCIe
> > >   	  core.
> > > +
> > > +config PCIE_SG2042
> > > +	bool "Sophgo SG2042 PCIe controller (host mode)"
> > > +	depends on ARCH_SOPHGO || COMPILE_TEST
> > > +	depends on OF
> > > +	select PCIE_CADENCE_HOST
> > > +	help
> > > +	  Say Y here if you want to support the Sophgo SG2042 PCIe platform
> > > +	  controller in host mode. Sophgo SG2042 PCIe controller uses Cadence
> > > +	  PCIe core.
> >
> > Reorder to keep these menu items in alphabetical order by vendor.
> 
> Sorry, I don't understand your question. I think the menu items in this
> Kconfig file are already sorted alphabetically.

Here's what menuconfig looks like after this patch:

  [*] Cadence platform PCIe controller (host mode)
  [*] Cadence platform PCIe controller (endpoint mode)
  [*] TI J721E PCIe controller (host mode)
  [*] TI J721E PCIe controller (endpoint mode)
  [ ] Sophgo SG2042 PCIe controller (host mode) (NEW)

"Sophgo" sorts *before* "TI".

> > > +	if (pcie->link_id == 1) {
> > > +		regmap_write(pcie->syscon, REG_LINK1_MSI_ADDR_LOW,
> > > +			     lower_32_bits(pcie->msi_phys));
> > > +		regmap_write(pcie->syscon, REG_LINK1_MSI_ADDR_HIGH,
> > > +			     upper_32_bits(pcie->msi_phys));
> > > +
> > > +		regmap_read(pcie->syscon, REG_LINK1_MSI_ADDR_SIZE, &value);
> > > +		value = (value & REG_LINK1_MSI_ADDR_SIZE_MASK) | MAX_MSI_IRQS;
> > > +		regmap_write(pcie->syscon, REG_LINK1_MSI_ADDR_SIZE, value);
> > > +	} else {
> > > +		regmap_write(pcie->syscon, REG_LINK0_MSI_ADDR_LOW,
> > > +			     lower_32_bits(pcie->msi_phys));
> > > +		regmap_write(pcie->syscon, REG_LINK0_MSI_ADDR_HIGH,
> > > +			     upper_32_bits(pcie->msi_phys));
> > > +
> > > +		regmap_read(pcie->syscon, REG_LINK0_MSI_ADDR_SIZE, &value);
> > > +		value = (value & REG_LINK0_MSI_ADDR_SIZE_MASK) | (MAX_MSI_IRQS << 16);
> > > +		regmap_write(pcie->syscon, REG_LINK0_MSI_ADDR_SIZE, value);
> > > +	}
> >
> > Lot of pcie->link_id checking going on here.  Consider saving these
> > offsets in the struct sg2042_pcie so you don't need to test
> > everywhere.
> 
> Actually, there are not many places in the code to check link_id. If to add
> storage information in struct sg2042_pcie, at least four  u32 are needed.
> And this logic will only be called one time in the probe. So I think it is
> better to keep the current method. What do you think?

1) I don't think "link_id" is a very good name since it appears to
refer to one of two PCIe Root Ports.  mvebu uses "marvell,pcie-port"
which looks like a similar usage, although unnecessarily
Marvell-specific.  And arch/arm/boot/dts/marvell/armada-370-db.dts has
separate stanzas for two Root Ports, without needing a property to
distinguish them, which would be even better.  I'm not sure why
arch/arm/boot/dts/marvell/armada-370.dtsi needs "marvell,pcie-port"
even though it also has separate stanzas.

2) I think checking pcie->link_id is likely to be harder to maintain
in the future, e.g., if a new chip adds more Root Ports, you'll have
to touch each use.

Bjorn