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

[Chen Wang]

On 2024/11/30 3:50, Bjorn Helgaas wrote:
> 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".

I see what you mean. I thought we just had to make sure the item IDs in 
the Kconfig file were in alphabetical order.

I will make changes in the next version based on your suggestions.

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

Thanks for your input. I looked at the Marvell solution you recommended. 
It seems that it is a relatively complex IP design to support mvebu, 
which can flexibly support the export of 1 to 4 variable root ports.
The Cadence IP used by SG2042 is much simpler, and it is fixed to export 
up to two root ports. So I plan to implement it in a simple way. 
Changing "link_id" to "pcie-port" will look better. The writing of link 
actually refers to the terminology in the Cadence manual, which is 
actually the concept of port.

By the way, there is no demand for scalability at present, because it is 
said that Sophgo currently only uses Cadence IP for the SG2042 SoC, and 
subsequent products should switch to solutions from other suppliers.

Any question please let me know.

Regards,

Chen