Message-ID:
 <MAXPR01MB3984C307B163E615811A25AAFE2A2@MAXPR01MB3984.INDPRD01.PROD.OUTLOOK.COM>
Date: Fri, 29 Nov 2024 17:51:39 +0800
From: Chen Wang <unicorn_wang@...look.com>
To: Bjorn Helgaas <helgaas@...nel.org>
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

Hello~

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.

>> +++ b/drivers/pci/controller/cadence/pcie-sg2042.c
>> + * SG2042 PCIe controller supports two ways to report MSI:
>> + * - Method A, the PICe controller implements an MSI interrupt controller inside,
>> + *   and connect to PLIC upward through one interrupt line. Provides
>> + *   memory-mapped msi address, and by programming the upper 32 bits of the
>> + *   address to zero, it can be compatible with old pcie devices that only
>> + *   support 32-bit msi address.
>> + * - Method B, the PICe controller connects to PLIC upward through an
>> + *   independent MSI controller "sophgo,sg2042-msi" on the SOC. The MSI
>> + *   controller provides multiple(up to 32) interrupt sources to PLIC.
>> + *   Compared with the first method, the advantage is that the interrupt source
>> + *   is expanded, but because for SG2042, the msi address provided by the MSI
>> + *   controller is fixed and only supports 64-bit address(> 2^32), it is not
>> + *   compatible with old pcie devices that only support 32-bit msi address.
>> + * Method A & B can be configured in DTS with property "sophgo,internal-msi",
>> + * default is Method B.
> s/PICe/PCIe/ (multiple)
> s/msi/MSI/ (multiple)
> s/pcie/PCIe/ (multiple)
>
> Wrap comment (and code below) to fit in 80 columns.  Add blank lines
> between paragraphs.
Got, will change.
>
>> +#define SG2042_CDNS_PLAT_CPU_TO_BUS_ADDR	0xCFFFFFFFFF
> Remove (see below).
Yes, after some testing, seems this address fixup is not need, will 
remove it.
>> +static void sg2042_pcie_msi_irq_compose_msi_msg(struct irq_data *d,
>> +						struct msi_msg *msg)
>> +{
>> +	struct sg2042_pcie *pcie = irq_data_get_irq_chip_data(d);
>> +	struct device *dev = pcie->cdns_pcie->dev;
>> +
>> +	msg->address_lo = lower_32_bits(pcie->msi_phys) + BYTE_NUM_PER_MSI_VEC * d->hwirq;
>> +	msg->address_hi = upper_32_bits(pcie->msi_phys);
>> +	msg->data = 1;
>> +
>> +	pcie->num_applied_vecs = d->hwirq;
> This looks questionable.  How do you know d->hwirq increases every
> time this is called?
Thanks, it's a bug, I will fix it.
>> +	dev_info(dev, "compose msi msg hwirq[%d] address_hi[%#x] address_lo[%#x]\n",
>> +		 (int)d->hwirq, msg->address_hi, msg->address_lo);
> This seems too verbose to be a dev_info().  Maybe a dev_dbg() or
> remove it altogether.
Will change it to dev_dbg.
>> + * We use the usual two domain structure, the top one being a generic PCI/MSI
>> + * domain, the bottom one being SG2042-specific and handling the actual HW
>> + * interrupt allocation.
>> + * At the same time, for internal MSI controller(Method A), bottom chip uses a
>> + * chained handler to handle the controller's MSI IRQ edge triggered.
> Add blank line between paragraphs.
OK.
>
>> +static int sg2042_pcie_setup_msi_external(struct sg2042_pcie *pcie)
>> +{
>> +	struct device *dev = pcie->cdns_pcie->dev;
>> +	struct device_node *np = dev->of_node;
>> +	struct irq_domain *parent_domain;
>> +	struct device_node *parent_np;
>> +
>> +	if (!of_find_property(np, "interrupt-parent", NULL)) {
>> +		dev_err(dev, "Can't find interrupt-parent!\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	parent_np = of_irq_find_parent(np);
>> +	if (!parent_np) {
>> +		dev_err(dev, "Can't find node of interrupt-parent!\n");
> Can you use some kind of %pOF format to include more information here?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/core-api/printk-formats.rst?id=v6.11#n463

Thanks, will double check this.

[......]

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

[......]

Powered by blists - more mailing lists