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@xxxxxxxxxxx>
> > >
> > > 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
