| 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: <7dbvco63tgz65p7xx5tufwjlwgkpujjbqet52atrbj7s4zyrbx@qjxxtumcbbga>
Date: Mon, 5 Feb 2024 19:49:46 +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@...ngson.cn, linux@...linux.org.uk, guyinggang@...ngson.cn,
netdev@...r.kernel.org, chris.chenfeiyang@...il.com
Subject: Re: [PATCH net-next v8 03/11] net: stmmac: dwmac-loongson: Add full
PCI support
On Tue, Jan 30, 2024 at 04:43:23PM +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).
Please add to the commit log more details of what LS7A is like and
bind that description to the changes below like the interface
settings, ref and PTP clock settings, etc. What is the difference
between LS2K and LS7A?
>
> Signed-off-by: Yanteng Si <siyanteng@...ngson.cn>
> Signed-off-by: Feiyang Chen <chenfeiyang@...ngson.cn>
> Signed-off-by: Yinggang Gu <guyinggang@...ngson.cn>
> ---
> .../ethernet/stmicro/stmmac/dwmac-loongson.c | 79 +++++++++++--------
> 1 file changed, 44 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> index e2dcb339b8b0..979c9b6dab3f 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-loongson.c
> @@ -16,6 +16,10 @@ struct stmmac_pci_info {
> static void loongson_default_data(struct pci_dev *pdev,
> struct plat_stmmacenet_data *plat)
> {
> + /* Get bus_id, this can be overloaded later */
> + plat->bus_id = (pci_domain_nr(pdev->bus) << 16) |
> + PCI_DEVID(pdev->bus->number, pdev->devfn);
> +
> plat->clk_csr = 2; /* clk_csr_i = 20-35MHz & MDC = clk_csr_i/16 */
> plat->has_gmac = 1;
> plat->force_sf_dma_mode = 1;
> @@ -51,10 +55,14 @@ static int loongson_gmac_data(struct pci_dev *pdev,
> plat->mdio_bus_data->phy_mask = 0;
>
> plat->phy_addr = -1;
> + plat->phy_interface = PHY_INTERFACE_MODE_RGMII_ID;
>
> plat->dma_cfg->pbl = 32;
> plat->dma_cfg->pblx8 = true;
>
> + plat->clk_ref_rate = 125000000;
> + plat->clk_ptp_rate = 125000000;
> +
Is this compatible with the LS2K GMAC?
> return 0;
> }
>
> @@ -71,13 +79,6 @@ static int loongson_dwmac_probe(struct pci_dev *pdev,
> struct stmmac_resources res;
> struct device_node *np;
>
> - 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;
> @@ -93,6 +94,7 @@ static int loongson_dwmac_probe(struct pci_dev *pdev,
> if (!plat->dma_cfg)
> return -ENOMEM;
>
> + np = dev_of_node(&pdev->dev);
> plat->mdio_node = of_get_child_by_name(np, "mdio");
> if (plat->mdio_node) {
> dev_info(&pdev->dev, "Found MDIO subnode\n");
Shouldn't mdio_node setup being done under the "if (np)" clause?
> @@ -123,41 +125,48 @@ static int loongson_dwmac_probe(struct pci_dev *pdev,
> if (ret)
> goto err_disable_device;
>
> - bus_id = of_alias_get_id(np, "ethernet");
> - if (bus_id >= 0)
> - plat->bus_id = bus_id;
> + if (np) {
> + bus_id = of_alias_get_id(np, "ethernet");
> + if (bus_id >= 0)
> + plat->bus_id = bus_id;
>
> - 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;
> + 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;
> }
Please collect all OF-specific code in the same "if (np)" clause if
possible.
>
> - plat->phy_interface = phy_mode;
> -
> 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;
> + if (np) {
> + 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;
> + res.wol_irq = pdev->irq;
This seems redundant. If res.wol_irq matches res_irq it won't be used
(see stmmac_request_irq_multi_msi() and stmmac_request_irq_single()).
What about dropping it?
-Serge(y)
> }
>
> ret = stmmac_dvr_probe(&pdev->dev, plat, &res);
> --
> 2.31.4
>
Powered by blists - more mailing lists