[PATCH 2/5] PCI: sg2042: Add Sophgo SG2042 PCIe driver
Chen Wang
unicorn_wang at outlook.com
Fri Nov 29 01:51:39 PST 2024
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 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.
>> +++ 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?
[......]
More information about the linux-riscv
mailing list