| 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
| ||
|
Message-ID: <rvz3ebfbxuz4fq34epujowab5tyf4o2uhvrcc2bqzla6odxfnl@aqypyjpr6awj>
Date: Sat, 4 May 2024 23:46:23 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: Yanteng Si <siyanteng@...ngson.cn>
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
> [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
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?
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."
>
> 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
> +
> +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
> + 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()?
> +
> 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.
> 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.
>
> 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.
> - }
>
> - 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.
> +
> + 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.
> 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.
-Serge(y)
> {}
> };
> MODULE_DEVICE_TABLE(pci, loongson_dwmac_id_table);
> --
> 2.31.4
>
Powered by blists - more mailing lists