[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
<AM9PR04MB8506AD31D297BD27122D89B5E2832@AM9PR04MB8506.eurprd04.prod.outlook.com>
Date: Sun, 18 Aug 2024 19:35:34 +0000
From: "Jan Petrous (OSS)" <jan.petrous@....nxp.com>
To: Russell King <linux@...linux.org.uk>, "Jan Petrous (OSS)"
<jan.petrous@....nxp.com>
CC: Maxime Coquelin <mcoquelin.stm32@...il.com>, Alexandre Torgue
<alexandre.torgue@...s.st.com>, dl-S32 <S32@....com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-stm32@...md-mailman.stormreply.com"
<linux-stm32@...md-mailman.stormreply.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, "netdev@...r.kernel.org"
<netdev@...r.kernel.org>
Subject: RE: [PATCH 4/6] net: stmmac: dwmac-s32cc: add basic NXP S32G/S32R
glue
> -----Original Message-----
> From: Russell King <linux@...linux.org.uk>
> Sent: Monday, 5 August, 2024 17:09
> To: Jan Petrous (OSS) <jan.petrous@....nxp.com>
> Cc: Maxime Coquelin <mcoquelin.stm32@...il.com>; Alexandre Torgue
> <alexandre.torgue@...s.st.com>; dl-S32 <S32@....com>; linux-
> kernel@...r.kernel.org; linux-stm32@...md-mailman.stormreply.com; linux-
> arm-kernel@...ts.infradead.org; Claudiu Manoil <claudiu.manoil@....com>;
> netdev@...r.kernel.org
> Subject: Re: [PATCH 4/6] net: stmmac: dwmac-s32cc: add basic NXP
> S32G/S32R glue
>
> On Sun, Aug 04, 2024 at 08:50:10PM +0000, Jan Petrous (OSS) wrote:
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig
> b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> > index 05cc07b8f48c..31628c363d71 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
> > +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> > @@ -153,6 +153,17 @@ config DWMAC_RZN1
> > This selects the Renesas RZ/N1 SoC glue layer support for
> > the stmmac device driver. This support can make use of a custom
> MII
> > converter PCS device.
>
> There should be a blank line here.
>
Added to v2.
> > +config DWMAC_S32CC
> > + tristate "NXP S32G/S32R GMAC support"
> > + default ARCH_S32
> > + depends on OF && (ARCH_S32 || COMPILE_TEST)
> ...
>
> > +static void s32cc_fix_mac_speed(void *priv, unsigned int speed, unsigned
> int mode)
> > +{
> > + struct s32cc_priv_data *gmac = priv;
> > + int ret;
> > +
> > + if (!gmac->rx_clk_enabled) {
> > + ret = clk_prepare_enable(gmac->rx_clk);
> > + if (ret) {
> > + dev_err(gmac->dev, "Can't set rx clock\n");
> > + return;
> > + }
> > + dev_dbg(gmac->dev, "rx clock enabled\n");
> > + gmac->rx_clk_enabled = true;
> > + }
> > +
> > + switch (speed) {
> > + case SPEED_1000:
> > + dev_dbg(gmac->dev, "Set tx clock to 125M\n");
> > + ret = clk_set_rate(gmac->tx_clk, GMAC_TX_RATE_125M);
> > + break;
> > + case SPEED_100:
> > + dev_dbg(gmac->dev, "Set tx clock to 25M\n");
> > + ret = clk_set_rate(gmac->tx_clk, GMAC_TX_RATE_25M);
> > + break;
> > + case SPEED_10:
> > + dev_dbg(gmac->dev, "Set tx clock to 2.5M\n");
> > + ret = clk_set_rate(gmac->tx_clk, GMAC_TX_RATE_2M5);
> > + break;
> > + default:
> > + dev_err(gmac->dev, "Unsupported/Invalid speed: %d\n",
> speed);
> > + return;
> > + }
> > +
>
> We seem to have multiple cases of very similar logic in lots of stmmac
> platform drivers, and I think it's about time we said no more to this.
> So, what I think we should do is as follows:
>
> add the following helper - either in stmmac, or more generically
> (phylib? - in which case its name will need changing.)
>
> static long stmmac_get_rgmii_clock(int speed)
> {
> switch (speed) {
> case SPEED_10:
> return 2500000;
>
> case SPEED_100:
> return 25000000;
>
> case SPEED_1000:
> return 125000000;
>
> default:
> return -ENVAL;
> }
> }
>
> Then, this can become:
>
> long tx_clk_rate;
>
> ...
>
> tx_clk_rate = stmmac_get_rgmii_clock(speed);
> if (tx_clk_rate < 0) {
> dev_err(gmac->dev, "Unsupported/Invalid speed: %d\n",
> speed);
> return;
> }
>
> ret = clk_set_rate(gmac->tx_clk, tx_clk_rate);
>
OK, added rgmi_clk(speed) helper to the linux/phy.h for general usage
In other drivers.
> > + if (ret)
> > + dev_err(gmac->dev, "Can't set tx clock\n");
> > +}
> > +
> > +static int s32cc_dwmac_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct plat_stmmacenet_data *plat;
> > + struct s32cc_priv_data *gmac;
> > + struct stmmac_resources res;
> > + int ret;
> > +
> > + gmac = devm_kzalloc(&pdev->dev, sizeof(*gmac), GFP_KERNEL);
> > + if (!gmac)
> > + return PTR_ERR(gmac);
> > +
> > + gmac->dev = &pdev->dev;
> > +
> > + ret = stmmac_get_platform_resources(pdev, &res);
> > + if (ret)
> > + return dev_err_probe(dev, ret,
> > + "Failed to get platform resources\n");
> > +
> > + plat = devm_stmmac_probe_config_dt(pdev, res.mac);
> > + if (IS_ERR(plat))
> > + return dev_err_probe(dev, PTR_ERR(plat),
> > + "dt configuration failed\n");
> > +
> > + /* PHY interface mode control reg */
> > + gmac->ctrl_sts = devm_platform_get_and_ioremap_resource(pdev, 1,
> NULL);
> > + if (IS_ERR_OR_NULL(gmac->ctrl_sts))
> > + return dev_err_probe(dev, PTR_ERR(gmac->ctrl_sts),
> > + "S32CC config region is missing\n");
>
> Testing with IS_ERR() is all that's required here.
Removed _OR_NULL suffix.
Thanks.
/Jan
Powered by blists - more mailing lists