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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20241129195000.GA2770247@bhelgaas>
Date: Fri, 29 Nov 2024 13:50:00 -0600
From: Bjorn Helgaas <helgaas@...nel.org>
To: Chen Wang <unicorn_wang@...look.com>
Cc: kw@...ux.com, u.kleine-koenig@...libre.com, aou@...s.berkeley.edu,
	arnd@...db.de, bhelgaas@...gle.com, conor+dt@...nel.org,
	guoren@...nel.org, inochiama@...look.com, krzk+dt@...nel.org,
	lee@...nel.org, lpieralisi@...nel.org,
	manivannan.sadhasivam@...aro.org, palmer@...belt.com,
	paul.walmsley@...ive.com, pbrobinson@...il.com, robh@...nel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-pci@...r.kernel.org, linux-riscv@...ts.infradead.org,
	chao.wei@...hgo.com, xiaoguang.xing@...hgo.com,
	fengchun.li@...hgo.com
Subject: Re: [PATCH 2/5] PCI: sg2042: Add Sophgo SG2042 PCIe driver

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