| 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
| ||
|
Date: Mon, 13 May 2024 18:49:54 +0800
From: Yanteng Si <siyanteng@...ngson.cn>
To: Serge Semin <fancer.lancer@...il.com>
Cc: andrew@...n.ch, hkallweit1@...il.com, peppe.cavallaro@...com,
alexandre.torgue@...s.st.com, joabreu@...opsys.com, Jose.Abreu@...opsys.com,
chenhuacai@...nel.org, linux@...linux.org.uk, guyinggang@...ngson.cn,
netdev@...r.kernel.org, chris.chenfeiyang@...il.com, siyanteng01@...il.com
Subject: Re: [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full
PCI support
在 2024/5/5 04:46, Serge Semin 写道:
>> [PATCH net-next v12 10/15] net: stmmac: dwmac-loongson: Add full PCI support
> I would have changed the subject to:
>
> net: stmmac: dwmac-loongson: Add DT-less GMAC PCI-device support
OK.
>
> On Thu, Apr 25, 2024 at 09:10:35PM +0800, Yanteng Si wrote:
>> Current dwmac-loongson only support LS2K in the "probed with PCI and
>> configured with DT" manner. Add LS7A support on which the devices are
>> fully PCI (non-DT).
>>
>> Others:
>> LS2K is a SoC and LS7A is a bridge chip.
> The text seems like misleading or just wrong. I see both of these
> platforms having the GMAC defined in the DT source:
>
> arch/mips/boot/dts/loongson/loongson64-2k1000.dtsi
> arch/mips/boot/dts/loongson/ls7a-pch.dtsi
>
> What do I miss in your description?
You are right.
>
> If nothing has been missed and it's just wrong I suggest to convert
> the commit log to something like this:
>
> "The Loongson GMAC driver currently supports the network controllers
> installed on the LS2K1000 SoC and LS7A1000 chipset, for which the GMAC
> devices are required to be defined in the platform device tree source.
> Let's extend the driver functionality with the case of having the
> Loongson GMAC probed on the PCI bus with no device tree node defined
> for it. That requires to make the device DT-node optional, to rely on
> the IRQ line detected by the PCI core and to have the MDIO bus ID
> calculated using the PCIe Domain+BDF numbers."
OK, Thanks!
>
>> Signed-off-by: Feiyang Chen <chenfeiyang@...ngson.cn>
>> Signed-off-by: Yinggang Gu <guyinggang@...ngson.cn>
>> Signed-off-by: Yanteng Si <siyanteng@...ngson.cn>
>> ---
>> .../ethernet/stmicro/stmmac/dwmac-loongson.c | 113 ++++++++++--------
>> 1 file changed, 65 insertions(+), 48 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
>> index e989cb835340..1022bceaa680 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
>> @@ -11,8 +11,17 @@
>>
>> #define PCI_DEVICE_ID_LOONGSON_GMAC 0x7a03
>>
>> -static void loongson_default_data(struct plat_stmmacenet_data *plat)
>> +struct stmmac_pci_info {
>> + int (*setup)(struct pci_dev *pdev, struct plat_stmmacenet_data *plat);
>> +};
> Please move this and the rest of the setup-callback introduction
> change into a separate patch. It' subject could be something like
> this:
> net: stmmac: dwmac-loongson: Introduce PCI device info data
OK.
>
>> +
>> +static void loongson_default_data(struct pci_dev *pdev,
>> + struct plat_stmmacenet_data *plat)
>> {
>> + /* Get bus_id, this can be overloaded later */
> s/overloaded/overwritten
OK
>
>> + plat->bus_id = (pci_domain_nr(pdev->bus) << 16) |
>> + PCI_DEVID(pdev->bus->number, pdev->devfn);
> Em, so you removed the code from the probe() function:
> - plat->bus_id = of_alias_get_id(np, "ethernet");
> - if (plat->bus_id < 0)
> - plat->bus_id = pci_dev_id(pdev);
> and instead of using the pci_dev_id() method here just opencoded it'
> content. Nice. Why not to use pci_dev_id() instead of PCI_DEVID()?
OK, I will try it!
>
>> +
>> plat->clk_csr = 2; /* clk_csr_i = 20-35MHz & MDC = clk_csr_i/16 */
>> plat->has_gmac = 1;
>> plat->force_sf_dma_mode = 1;
>> @@ -44,9 +53,10 @@ static void loongson_default_data(struct plat_stmmacenet_data *plat)
>> plat->multicast_filter_bins = 256;
>> }
>>
>> -static int loongson_gmac_data(struct plat_stmmacenet_data *plat)
>> +static int loongson_gmac_data(struct pci_dev *pdev,
>> + struct plat_stmmacenet_data *plat)
>> {
>> - loongson_default_data(plat);
>> + loongson_default_data(pdev, plat);
>>
>> plat->mdio_bus_data->phy_mask = 0;
>> plat->phy_interface = PHY_INTERFACE_MODE_RGMII_ID;
>> @@ -54,20 +64,20 @@ static int loongson_gmac_data(struct plat_stmmacenet_data *plat)
>> return 0;
>> }
>>
>> +static struct stmmac_pci_info loongson_gmac_pci_info = {
>> + .setup = loongson_gmac_data,
>> +};
>> +
> To the separate patch please.
OK.
>
>> static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>> {
>> struct plat_stmmacenet_data *plat;
>> + int ret, i, bus_id, phy_mode;
>> + struct stmmac_pci_info *info;
>> struct stmmac_resources res;
>> struct device_node *np;
>> - int ret, i, phy_mode;
> You can drop the bus_id and phy_mode variables, and use ret in the
> respective statements instead.
OK, I will try it.
>
>>
>> np = dev_of_node(&pdev->dev);
>>
>> - if (!np) {
>> - pr_info("dwmac_loongson_pci: No OF node\n");
>> - return -ENODEV;
>> - }
>> -
>> plat = devm_kzalloc(&pdev->dev, sizeof(*plat), GFP_KERNEL);
>> if (!plat)
>> return -ENOMEM;
>> @@ -78,12 +88,6 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
>> if (!plat->mdio_bus_data)
>> return -ENOMEM;
>>
>> - plat->mdio_node = of_get_child_by_name(np, "mdio");
>> - if (plat->mdio_node) {
>> - dev_info(&pdev->dev, "Found MDIO subnode\n");
>> - plat->mdio_bus_data->needs_reset = true;
>> - }
>> -
>> plat->dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*plat->dma_cfg), GFP_KERNEL);
>> if (!plat->dma_cfg) {
>> ret = -ENOMEM;
>> @@ -107,46 +111,59 @@ static int loongson_dwmac_probe(struct pci_dev *pdev, const struct pci_device_id
>> break;
>> }
>>
>> - plat->bus_id = of_alias_get_id(np, "ethernet");
>> - if (plat->bus_id < 0)
>> - plat->bus_id = pci_dev_id(pdev);
>> + pci_set_master(pdev);
>>
>> - phy_mode = device_get_phy_mode(&pdev->dev);
>> - if (phy_mode < 0) {
>> - dev_err(&pdev->dev, "phy_mode not found\n");
>> - ret = phy_mode;
>> + info = (struct stmmac_pci_info *)id->driver_data;
>> + ret = info->setup(pdev, plat);
>> + if (ret)
>> goto err_disable_device;
> To the separate patch please.
OK.
>
>> - }
>>
>> - plat->phy_interface = phy_mode;
>> -
>> - pci_set_master(pdev);
>> + if (np) {
>> + plat->mdio_node = of_get_child_by_name(np, "mdio");
>> + if (plat->mdio_node) {
>> + dev_info(&pdev->dev, "Found MDIO subnode\n");
>> + plat->mdio_bus_data->needs_reset = true;
>> + }
>> +
>> + bus_id = of_alias_get_id(np, "ethernet");
>> + if (bus_id >= 0)
>> + plat->bus_id = bus_id;
> ret = of_alias_get_id(np, "ethernet");
> if (ret >= 0)
> plat->bus_id = ret;
>
>> +
>> + phy_mode = device_get_phy_mode(&pdev->dev);
>> + if (phy_mode < 0) {
>> + dev_err(&pdev->dev, "phy_mode not found\n");
>> + ret = phy_mode;
>> + goto err_disable_device;
>> + }
>> + plat->phy_interface = phy_mode;
> ret = device_get_phy_mode(&pdev->dev);
> if (ret < 0) {
> dev_err(&pdev->dev, "phy_mode not found\n");
> goto err_disable_device;
> }
>
> plat->phy_interface = ret;
>
> * note empty line between the if-clause and the last statement.
OK.
>
>> +
>> + res.irq = of_irq_get_byname(np, "macirq");
>> + if (res.irq < 0) {
>> + dev_err(&pdev->dev, "IRQ macirq not found\n");
>> + ret = -ENODEV;
>> + goto err_disable_msi;
>> + }
>> +
>> + res.wol_irq = of_irq_get_byname(np, "eth_wake_irq");
>> + if (res.wol_irq < 0) {
>> + dev_info(&pdev->dev, "IRQ eth_wake_irq not found, using macirq\n");
>> + res.wol_irq = res.irq;
>> + }
>> +
>> + res.lpi_irq = of_irq_get_byname(np, "eth_lpi");
>> + if (res.lpi_irq < 0) {
>> + dev_err(&pdev->dev, "IRQ eth_lpi not found\n");
>> + ret = -ENODEV;
>> + goto err_disable_msi;
>> + }
>> + } else {
>> + res.irq = pdev->irq;
>> + }
>>
>> - loongson_gmac_data(plat);
> To the separate patch please.
OK.
>
>> pci_enable_msi(pdev);
>> memset(&res, 0, sizeof(res));
>> res.addr = pcim_iomap_table(pdev)[0];
>>
>> - res.irq = of_irq_get_byname(np, "macirq");
>> - if (res.irq < 0) {
>> - dev_err(&pdev->dev, "IRQ macirq not found\n");
>> - ret = -ENODEV;
>> - goto err_disable_msi;
>> - }
>> -
>> - res.wol_irq = of_irq_get_byname(np, "eth_wake_irq");
>> - if (res.wol_irq < 0) {
>> - dev_info(&pdev->dev, "IRQ eth_wake_irq not found, using macirq\n");
>> - res.wol_irq = res.irq;
>> - }
>> -
>> - res.lpi_irq = of_irq_get_byname(np, "eth_lpi");
>> - if (res.lpi_irq < 0) {
>> - dev_err(&pdev->dev, "IRQ eth_lpi not found\n");
>> - ret = -ENODEV;
>> - goto err_disable_msi;
>> - }
>> -
>> plat->tx_queues_to_use = 1;
>> plat->rx_queues_to_use = 1;
>>
>> @@ -224,7 +241,7 @@ static SIMPLE_DEV_PM_OPS(loongson_dwmac_pm_ops, loongson_dwmac_suspend,
>> loongson_dwmac_resume);
>>
>> static const struct pci_device_id loongson_dwmac_id_table[] = {
>> - { PCI_DEVICE_DATA(LOONGSON, GMAC, NULL) },
>> + { PCI_DEVICE_DATA(LOONGSON, GMAC, &loongson_gmac_pci_info) },
> To the separate patch please.
OK.
Thanks,
Yanteng
Powered by blists - more mailing lists