On Mon, Feb 10, 2025 at 12:01:56PM +0100, Romain Gantois wrote:
> Hello Inochi,
>
> On dimanche 9 février 2025 02:30:52 heure normale d’Europe centrale Inochi
> Amaoto wrote:
> > Adds Sophgo dwmac driver support on the Sophgo SG2044 SoC.
> ...
> > --- /dev/null
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sophgo.c
> > @@ -0,0 +1,105 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Sophgo DWMAC platform driver
> > + *
> > + * Copyright (C) 2024 Inochi Amaoto <inochiama@xxxxxxxxx>
> > + */
> > +
> > +#include <linux/bits.h>
>
> It doesn't look like this include is used, could you please remove it?
>
Thanks, I will. And I will add the miss header
linux/clk.h and linux/module.h.
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/phy.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include "stmmac_platform.h"
> > +
> > +struct sophgo_dwmac {
> > + struct device *dev;
> > + struct clk *clk_tx;
> > +};
> > +
> > +static void sophgo_dwmac_fix_mac_speed(void *priv, unsigned int speed,
> > unsigned int mode) +{
> > + struct sophgo_dwmac *dwmac = priv;
> > + long rate;
> > + int ret;
> > +
> > + rate = rgmii_clock(speed);
> > + if (rate < 0) {
> > + dev_err(dwmac->dev, "invalid speed %u\n", speed);
> > + return;
> > + }
> > +
> > + ret = clk_set_rate(dwmac->clk_tx, rate);
> > + if (ret)
> > + dev_err(dwmac->dev, "failed to set tx rate %lu: %pe\n",
>
> nit: shouldn't this be "%ld"?
>
Yeah, it is my mistake, I will fix it.
> > + rate, ERR_PTR(ret));
> > +}
> > +
> > +static int sophgo_sg2044_dwmac_init(struct platform_device *pdev,
> > + struct plat_stmmacenet_data *plat_dat,
> > + struct stmmac_resources *stmmac_res)
> > +{
> > + struct sophgo_dwmac *dwmac;
> > +
> > + dwmac = devm_kzalloc(&pdev->dev, sizeof(*dwmac), GFP_KERNEL);
> > + if (!dwmac)
> > + return -ENOMEM;
> > +
> > + dwmac->clk_tx = devm_clk_get_enabled(&pdev->dev, "tx");
> > + if (IS_ERR(dwmac->clk_tx))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(dwmac->clk_tx),
> > + "failed to get tx clock\n");
> > +
> > + dwmac->dev = &pdev->dev;
> > + plat_dat->bsp_priv = dwmac;
> > + plat_dat->flags |= STMMAC_FLAG_SPH_DISABLE;
> > + plat_dat->fix_mac_speed = sophgo_dwmac_fix_mac_speed;
> > + plat_dat->multicast_filter_bins = 0;
> > + plat_dat->unicast_filter_entries = 1;
> > +
> > + return 0;
> > +}
> > +
> > +static int sophgo_dwmac_probe(struct platform_device *pdev)
> > +{
> > + struct plat_stmmacenet_data *plat_dat;
> > + struct stmmac_resources stmmac_res;
>
> nit: I think adding "struct device *dev = &pdev->dev;" here would
> be better than repeating "&pdev->dev" later on.
>
Thanks, I will change that.
> > + int ret;
> > +
> > + ret = stmmac_get_platform_resources(pdev, &stmmac_res);
> > + if (ret)
> > + return dev_err_probe(&pdev->dev, ret,
> > + "failed to get resources\n");
>
> This error message is a bit too vague, maybe replace it with "failed to get
> platform resources"?
>
OK.
> > +
> > + plat_dat = devm_stmmac_probe_config_dt(pdev, stmmac_res.mac);
> > + if (IS_ERR(plat_dat))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(plat_dat),
> > + "dt configuration failed\n");
>
> This error message is a bit misleading IMO, I would replace it with
> something like "failed to parse device-tree parameters".
>
OK.
> > +
> > + ret = sophgo_sg2044_dwmac_init(pdev, plat_dat, &stmmac_res);
> > + if (ret)
> > + return ret;
> > +
> > + return stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
> > +}
> > +
> > +static const struct of_device_id sophgo_dwmac_match[] = {
> > + { .compatible = "sophgo,sg2044-dwmac" },
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, sophgo_dwmac_match);
> > +
> > +static struct platform_driver sophgo_dwmac_driver = {
> > + .probe = sophgo_dwmac_probe,
> > + .remove = stmmac_pltfr_remove,
> > + .driver = {
> > + .name = "sophgo-dwmac",
> > + .pm = &stmmac_pltfr_pm_ops,
> > + .of_match_table = sophgo_dwmac_match,
> > + },
> > +};
> > +module_platform_driver(sophgo_dwmac_driver);
> > +
> > +MODULE_AUTHOR("Inochi Amaoto <inochiama@xxxxxxxxx>");
> > +MODULE_DESCRIPTION("Sophgo DWMAC platform driver");
> > +MODULE_LICENSE("GPL");
>
> Thanks,
>
> --
> Romain Gantois, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
