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

[Chen Wang]

On 2025/1/23 5:33, Bjorn Helgaas wrote:
> On Wed, Jan 15, 2025 at 03:06:57PM +0800, Chen Wang wrote:
>> From: Chen Wang <unicorn_wang at 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.
>> + * pcie-sg2042 - PCIe controller driver for Sophgo SG2042 SoC
> I'm guessing this is the first of a *family* of Sophgo SoCs, so
> "sg2042" looks like it might be a little too specific if there will be
> things like "sg3042" etc added in the future.
As I know, SG2042 will be the only one SoC using Cadence IP from Sophgo. 
They have switched to other IP(DWC) later.
>> +#include "../../../irqchip/irq-msi-lib.h"
> I know you're using this path because you're relying on Marc's
> work in progress [1].
>
> But I don't want to carry around an #include like this in drivers/pci
> while we're waiting for that, so I think you should use the existing
> published MSI model until after Marc's update is merged.  Otherwise we
> might end up with this ugly path here and no real path to migrate to
> the published, supported one.
>
> [1] https://lore.kernel.org/linux-riscv/20241204124549.607054-2-maz@kernel.org/
OK, I will switch back to use the existing published MSI model.
>> + * SG2042 PCIe controller supports two ways to report MSI:
>> + *
>> + * - Method A, the PCIe 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 PCIe 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.
> Maybe expand "PLIC" the first time?
Sure.
>
> s/SOC/SoC/ to match previous uses, e.g., in commit log
> s/multiple(up to 32)/up to 32/
ok
>> + *   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.
> "Supporting 64-bit address" means supporting any address from 0 to
> 2^64 - 1, but I don't think that's what you mean here.
>
> I think what you mean here is that with Method B, the MSI address is
> fixed and it can only be above 4GB.
Yes, I will fix it.
>> +#define REG_CLEAR_LINK0_BIT	2
>> +#define REG_CLEAR_LINK1_BIT	3
>> +#define REG_STATUS_LINK0_BIT	2
>> +#define REG_STATUS_LINK1_BIT	3
>> +static void sg2042_pcie_msi_clear_status(struct sg2042_pcie *pcie)
>> +{
>> +	u32 status, clr_msi_in_bit;
>> +
>> +	if (pcie->link_id == 1)
>> +		clr_msi_in_bit = BIT(REG_CLEAR_LINK1_BIT);
>> +	else
>> +		clr_msi_in_bit = BIT(REG_CLEAR_LINK0_BIT);
> Why not put the BIT() in the #defines for REG_CLEAR_LINK0_BIT,
> REG_STATUS_LINK0_BIT, ...?  Then this code is slightly simpler, and
> you can use similar style in sg2042_pcie_msi_chained_isr() instead of
> shifting there.
Ok, I will check this out in new version.
>> +	regmap_read(pcie->syscon, REG_CLEAR, &status);
>> +	status |= clr_msi_in_bit;
>> +	regmap_write(pcie->syscon, REG_CLEAR, status);
>> +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);
> This seems a little suspect because adding "BYTE_NUM_PER_MSI_VEC *
> d->hwirq" could cause overflow into the upper 32 bits.  I think you
> should add first, then take the lower/upper 32 bits of the 64-bit
> result.
OK, I will check this out in new version.
>> +	if (d->hwirq > pcie->num_applied_vecs)
>> +		pcie->num_applied_vecs = d->hwirq;
> "num_applied_vecs" is a bit of a misnomer; it's actually the *max*
> hwirq.
"max_applied_vecs"？
>
>> +static const struct irq_domain_ops sg2042_pcie_msi_domain_ops = {
>> +	.alloc	= sg2042_pcie_irq_domain_alloc,
>> +	.free	= sg2042_pcie_irq_domain_free,
> Mention "msi" in these function names, e.g.,
> sg2042_pcie_msi_domain_alloc().
ok
>
>> +static int sg2042_pcie_init_msi_data(struct sg2042_pcie *pcie)
>> +{
>> ...
>> +	/* Program the MSI address and size */
>> +	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);
>> +	}
> Would be nicer to set temporaries to the link_id-dependent values (as
> you did elsewhere) so it's obvious that the code is identical, e.g.,
>
>    if (pcie->link_id == 1) {
>      msi_addr = REG_LINK1_MSI_ADDR_LOW;
>      msi_addr_size = REG_LINK1_MSI_ADDR_SIZE;
>      msi_addr_size_mask = REG_LINK1_MSI_ADDR_SIZE;
>    } else {
>      ...
>    }
>
>    regmap_write(pcie->syscon, msi_addr, lower_32_bits(pcie->msi_phys));
>    regmap_write(pcie->syscon, msi_addr + 4, upper_32_bits(pcie->msi_phys));
>    ...
Ok，I will check this out.
>> +
>> +	return 0;
>> +}
>> +
>> +static irqreturn_t sg2042_pcie_msi_handle_irq(struct sg2042_pcie *pcie)
> Which driver are you using as a template for function names and code
> structure?  There are probably a dozen different names for functions
> that iterate like this around a call to generic_handle_domain_irq(),
> but you've managed to come up with a new one.  If you can pick a
> similar name to copy, it makes it easier to compare drivers and find
> and fix defects across them.
>
>> +{
>> +	u32 i, pos;
>> +	unsigned long val;
>> +	u32 status, num_vectors;
>> +	irqreturn_t ret = IRQ_NONE;
>> +
>> +	num_vectors = pcie->num_applied_vecs;
>> +	for (i = 0; i <= num_vectors; i++) {
>> +		status = readl((void *)(pcie->msi_virt + i * BYTE_NUM_PER_MSI_VEC));
>> +		if (!status)
>> +			continue;
>> +
>> +		ret = IRQ_HANDLED;
>> +		val = status;
> I don't think you need both val and status.
Yes, I will fix this.
>
>> +		pos = 0;
>> +		while ((pos = find_next_bit(&val, MAX_MSI_IRQS_PER_CTRL,
>> +					    pos)) != MAX_MSI_IRQS_PER_CTRL) {
> Most drivers use for_each_set_bit() here.
Ok, I will check this out.
>> +			generic_handle_domain_irq(pcie->msi_domain,
>> +						  (i * MAX_MSI_IRQS_PER_CTRL) +
>> +						  pos);
>> +			pos++;
>> +		}
>> +		writel(0, ((void *)(pcie->msi_virt) + i * BYTE_NUM_PER_MSI_VEC));
>> +	}
>> +	return ret;
>> +}
>> +static int sg2042_pcie_setup_msi(struct sg2042_pcie *pcie,
>> +				 struct device_node *msi_node)
>> +{
>> +	struct device *dev = pcie->cdns_pcie->dev;
>> +	struct fwnode_handle *fwnode = of_node_to_fwnode(dev->of_node);
>> +	struct irq_domain *parent_domain;
>> +	int ret = 0;
> Pointless initialization of "ret".
Yes, I will fix this.
>> +	if (!of_property_read_bool(msi_node, "msi-controller"))
>> +		return -ENODEV;
>> +
>> +	ret = of_irq_get_byname(msi_node, "msi");
>> +	if (ret <= 0) {
>> +		dev_err(dev, "%pOF: failed to get MSI irq\n", msi_node);
>> +		return ret;
>> +	}
>> +	pcie->msi_irq = ret;
>> +
>> +	irq_set_chained_handler_and_data(pcie->msi_irq,
>> +					 sg2042_pcie_msi_chained_isr, pcie);
>> +
>> +	parent_domain = irq_domain_create_linear(fwnode, MSI_DEF_NUM_VECTORS,
>> +						 &sg2042_pcie_msi_domain_ops, pcie);
> Wrap to fit in 80 columns like 99% of the rest of this file.

Ok, I will check this out.

Thanks,

Chen

>
> Bjorn