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

[Chen Wang]

On 2024/12/11 1:31, Bjorn Helgaas wrote:
> On Mon, Dec 09, 2024 at 03:19: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.
>> +++ b/drivers/pci/controller/cadence/Makefile
>> @@ -4,3 +4,4 @@ obj-$(CONFIG_PCIE_CADENCE_HOST) += pcie-cadence-host.o
>>   obj-$(CONFIG_PCIE_CADENCE_EP) += pcie-cadence-ep.o
>>   obj-$(CONFIG_PCIE_CADENCE_PLAT) += pcie-cadence-plat.o
>>   obj-$(CONFIG_PCI_J721E) += pci-j721e.o
>> +obj-$(CONFIG_PCIE_SG2042) += pcie-sg2042.o
>> \ No newline at end of file
> Add the newline.
ok
>
>> +++ b/drivers/pci/controller/cadence/pcie-sg2042.c
>> +#include "../../../irqchip/irq-msi-lib.h"
> This is the only file outside drivers/irqchip/ that includes this.
> What's special about this driver?  Maybe this is a hint that something
> here belongs in drivers/irqchip/?

This file is included due to MSI parent model is used in this driver and 
I used helper functions such as msi_lib_init_dev_msi_info.

Actually I see Marc is working on MSI cleanup and will make 
irq-msi-lib.h globally available, see 
https://lore.kernel.org/linux-riscv/20241204124549.607054-2-maz@kernel.org/

But before his PR is merged, we may have to include the file like this now.

>
>> +#ifdef CONFIG_SMP
> No other drivers test CONFIG_SMP, why should this be different?
ok, seems it'ok to remove this test.
>
>> +static int sg2042_pcie_msi_irq_set_affinity(struct irq_data *d,
>> +					    const struct cpumask *mask,
>> +					    bool force)
>> +{
>> +	if (d->parent_data)
>> +		return irq_chip_set_affinity_parent(d, mask, force);
>> +
>> +	return -EINVAL;
>> +}
>> +#endif /* CONFIG_SMP */
>> +static int sg2042_pcie_init_msi_data(struct sg2042_pcie *pcie)
>> +{
>> +	struct device *dev = pcie->cdns_pcie->dev;
>> +	u32 value;
>> +	int ret;
>> +
>> +	raw_spin_lock_init(&pcie->msi_lock);
>> +
>> +	/*
>> +	 * Though the PCIe controller can address >32-bit address space, to
>> +	 * facilitate endpoints that support only 32-bit MSI target address,
>> +	 * the mask is set to 32-bit to make sure that MSI target address is
>> +	 * always a 32-bit address
>> +	 */
>> +	ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
> Not sure this is needed.  Does DT dma-ranges not cover this?
I prefer not introduce dma-ranges, we just need to allocate a small 
continuous space here, and using dma_set_coherent_mask is also one of 
the commonly used methods.
>
>> +static int sg2042_pcie_setup_msi(struct sg2042_pcie *pcie, struct device_node *msi_node)
> Wrap to fit in 80 columns like the rest.
ok
>
>> +/*
>> + * SG2042 only support 4-byte aligned access, so for the rootbus (i.e. to read
>> + * the PCIe controller itself, read32 is required. For non-rootbus (i.e. to read
> s/PCIe controller/Root Port/
ok
>
>> + * the PCIe peripheral registers, supports 1/2/4 byte aligned access, so
>> + * directly use read should be fine.
> s/use read/using read/
ok, thanks
>
>> +static int sg2042_pcie_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *np = dev->of_node;
>> +	struct pci_host_bridge *bridge;
>> +	struct device_node *np_syscon;
>> +	struct device_node *msi_node;
>> +	struct cdns_pcie *cdns_pcie;
>> +	struct sg2042_pcie *pcie;
>> +	struct cdns_pcie_rc *rc;
>> +	struct regmap *syscon;
>> +	int ret;
>> +
>> +	if (!IS_ENABLED(CONFIG_PCIE_CADENCE_HOST))
>> +		return -ENODEV;
> I don't think this is needed since CONFIG_PCIE_SG2042 selects
> PCIE_CADENCE_HOST.

Yes, you are right.

Thanks,

Chen

>
> Bjorn