linux-kernel - Re: [PATCH v2 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] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <BM1PR01MB21161E41CA05A919E0A6D64AFE062@BM1PR01MB2116.INDPRD01.PROD.OUTLOOK.COM>
Date: Thu, 19 Dec 2024 11:23:10 +0800
From: Chen Wang <unicorn_wang@...look.com>
To: Bjorn Helgaas <helgaas@...nel.org>, Chen Wang <unicornxw@...il.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 v2 2/5] PCI: sg2042: Add Sophgo SG2042 PCIe driver


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