Re: [-net-next v11 5/6] net: stmmac: Add glue layer for StarFive JH7110 SoC
From: Paolo Abeni <pabeni@xxxxxxxxxx>
to: Guo Samin <samin.guo@xxxxxxxxxxxxxxxx>, Emil Renner Berthing <emil.renner.berthing@xxxxxxxxxxxxx>, Arun.Ramadoss@xxxxxxxxxxxxx
data: 2023/4/11
> On Mon, 2023-04-10 at 16:29 +0800, Guo Samin wrote:
>> Re: [-net-next v11 5/6] net: stmmac: Add glue layer for StarFive JH7110 SoC
>> From: Emil Renner Berthing <emil.renner.berthing@xxxxxxxxxxxxx>
>> to: Guo Samin <samin.guo@xxxxxxxxxxxxxxxx>
>> data: 2023/4/9
>>
>>> On Sat, 8 Apr 2023 at 03:16, Guo Samin <samin.guo@xxxxxxxxxxxxxxxx> wrote:
>>>>
>>>> Re: [-net-next v11 5/6] net: stmmac: Add glue layer for StarFive JH7110 SoC
>>>> From: Emil Renner Berthing <emil.renner.berthing@xxxxxxxxxxxxx>
>>>> to: Samin Guo <samin.guo@xxxxxxxxxxxxxxxx>
>>>> data: 2023/4/8
>>>>
>>>>> On Fri, 7 Apr 2023 at 13:05, Samin Guo <samin.guo@xxxxxxxxxxxxxxxx> wrote:
>>>>>>
>>>>>> This adds StarFive dwmac driver support on the StarFive JH7110 SoC.
>>>>>>
>>>>>> Tested-by: Tommaso Merciai <tomm.merciai@xxxxxxxxx>
>>>>>> Co-developed-by: Emil Renner Berthing <kernel@xxxxxxxx>
>>>>>> Signed-off-by: Emil Renner Berthing <kernel@xxxxxxxx>
>>>>>> Signed-off-by: Samin Guo <samin.guo@xxxxxxxxxxxxxxxx>
>>>>>> ---
>>>>>> MAINTAINERS | 1 +
>>>>>> drivers/net/ethernet/stmicro/stmmac/Kconfig | 12 ++
>>>>>> drivers/net/ethernet/stmicro/stmmac/Makefile | 1 +
>>>>>> .../ethernet/stmicro/stmmac/dwmac-starfive.c | 123 ++++++++++++++++++
>>>>>> 4 files changed, 137 insertions(+)
>>>>>> create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
>>>>>>
>>>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>>>> index 6b6b67468b8f..46b366456cee 100644
>>>>>> --- a/MAINTAINERS
>>>>>> +++ b/MAINTAINERS
>>>>>> @@ -19910,6 +19910,7 @@ M: Emil Renner Berthing <kernel@xxxxxxxx>
>>>>>> M: Samin Guo <samin.guo@xxxxxxxxxxxxxxxx>
>>>>>> S: Maintained
>>>>>> F: Documentation/devicetree/bindings/net/starfive,jh7110-dwmac.yaml
>>>>>> +F: drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
>>>>>>
>>>>>> STARFIVE JH7100 CLOCK DRIVERS
>>>>>> M: Emil Renner Berthing <kernel@xxxxxxxx>
>>>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
>>>>>> index f77511fe4e87..5f5a997f21f3 100644
>>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
>>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
>>>>>> @@ -165,6 +165,18 @@ config DWMAC_SOCFPGA
>>>>>> for the stmmac device driver. This driver is used for
>>>>>> arria5 and cyclone5 FPGA SoCs.
>>>>>>
>>>>>> +config DWMAC_STARFIVE
>>>>>> + tristate "StarFive dwmac support"
>>>>>> + depends on OF && (ARCH_STARFIVE || COMPILE_TEST)
>>>>>> + select MFD_SYSCON
>>>>>> + default m if ARCH_STARFIVE
>>>>>> + help
>>>>>> + Support for ethernet controllers on StarFive RISC-V SoCs
>>>>>> +
>>>>>> + This selects the StarFive platform specific glue layer support for
>>>>>> + the stmmac device driver. This driver is used for StarFive JH7110
>>>>>> + ethernet controller.
>>>>>> +
>>>>>> config DWMAC_STI
>>>>>> tristate "STi GMAC support"
>>>>>> default ARCH_STI
>>>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile
>>>>>> index 057e4bab5c08..8738fdbb4b2d 100644
>>>>>> --- a/drivers/net/ethernet/stmicro/stmmac/Makefile
>>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
>>>>>> @@ -23,6 +23,7 @@ obj-$(CONFIG_DWMAC_OXNAS) += dwmac-oxnas.o
>>>>>> obj-$(CONFIG_DWMAC_QCOM_ETHQOS) += dwmac-qcom-ethqos.o
>>>>>> obj-$(CONFIG_DWMAC_ROCKCHIP) += dwmac-rk.o
>>>>>> obj-$(CONFIG_DWMAC_SOCFPGA) += dwmac-altr-socfpga.o
>>>>>> +obj-$(CONFIG_DWMAC_STARFIVE) += dwmac-starfive.o
>>>>>> obj-$(CONFIG_DWMAC_STI) += dwmac-sti.o
>>>>>> obj-$(CONFIG_DWMAC_STM32) += dwmac-stm32.o
>>>>>> obj-$(CONFIG_DWMAC_SUNXI) += dwmac-sunxi.o
>>>>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
>>>>>> new file mode 100644
>>>>>> index 000000000000..4963d4008485
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
>>>>>> @@ -0,0 +1,123 @@
>>>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>>>> +/*
>>>>>> + * StarFive DWMAC platform driver
>>>>>> + *
>>>>>> + * Copyright (C) 2021 Emil Renner Berthing <kernel@xxxxxxxx>
>>>>>> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
>>>>>> + *
>>>>>> + */
>>>>>> +
>>>>>> +#include <linux/mfd/syscon.h>
>>>>>> +#include <linux/of_device.h>
>>>>>> +#include <linux/regmap.h>
>>>>>> +
>>>>>> +#include "stmmac_platform.h"
>>>>>> +
>>>>>> +struct starfive_dwmac {
>>>>>> + struct device *dev;
>>>>>> + struct clk *clk_tx;
>>>>>> +};
>>>>>> +
>>>>>> +static void starfive_dwmac_fix_mac_speed(void *priv, unsigned int speed)
>>>>>> +{
>>>>>> + struct starfive_dwmac *dwmac = priv;
>>>>>> + unsigned long rate;
>>>>>> + int err;
>>>>>> +
>>>>>> + rate = clk_get_rate(dwmac->clk_tx);
>>>>>
>>>>> Hi Samin,
>>>>>
>>>>> I'm not sure why you added this line in this revision. If it's just to
>>>>> not call clk_set_rate on the uninitialized rate, I'd much rather you
>>>>> just returned early and don't call clk_set_rate at all in case of
>>>>> errors.
>>>>>
>>>>>> +
>>>>>> + switch (speed) {
>>>>>> + case SPEED_1000:
>>>>>> + rate = 125000000;
>>>>>> + break;
>>>>>> + case SPEED_100:
>>>>>> + rate = 25000000;
>>>>>> + break;
>>>>>> + case SPEED_10:
>>>>>> + rate = 2500000;
>>>>>> + break;
>>>>>> + default:
>>>>>> + dev_err(dwmac->dev, "invalid speed %u\n", speed);
>>>>>> + break;
>>>>>
>>>>> That is skip the clk_get_rate above and just change this break to a return.
>>>>>
>>>>
>>>> Hi Emil,
>>>>
>>>> We used the solution you mentioned before V3, but Arun Ramadoss doesn't think that's great.
>>>> (https://patchwork.kernel.org/project/linux-riscv/patch/20230106030001.1952-6-yanhong.wang@xxxxxxxxxxxxxxxx)
>>>>
>>>>
>>>>> +static void starfive_eth_plat_fix_mac_speed(void *priv, unsigned int
>>>>> speed)
>>>>> +{
>>>>> + struct starfive_dwmac *dwmac = priv;
>>>>> + unsigned long rate;
>>>>> + int err;
>>>>> +
>>>>> + switch (speed) {
>>>>> + case SPEED_1000:
>>>>> + rate = 125000000;
>>>>> + break;
>>>>> + case SPEED_100:
>>>>> + rate = 25000000;
>>>>> + break;
>>>>> + case SPEED_10:
>>>>> + rate = 2500000;
>>>>> + break;
>>>>> + default:
>>>>> + dev_err(dwmac->dev, "invalid speed %u\n", speed);
>>>>> + return;
>>>>
>>>> Do we need to return value, since it is invalid speed. But the return
>>>> value of function is void.(Arun Ramadoss)
>>>>
>>>>
>>>> So in v9, after discussing with Jakub Kicinski, the clk_set_rate was used to initialize the rate.
>>>> (It is a reference to Intel's scheme: dwmac-intel-plat.c: kmb_eth_fix_mac_speed)
>>>> (https://patchwork.kernel.org/project/linux-riscv/patch/20230328062009.25454-6-samin.guo@xxxxxxxxxxxxxxxx)
>>>>
>>>
>>> Yeah, I think this is a misunderstanding and Arun is considering if we
>>> ought to return the error which we can't without changing generic
>>> dwmac code, and Jakub is rightly concerned about using a local
>>> variable uninitialized. I don't think anyone is suggesting that
>>> getting the rate just to set it to the exact same value is better than
>>> just leaving the clock alone.
>>>
>> HI Emil,
>>
>> Yeah, return early saves time and code complexity, and seems like a good solution so Yanhong did the same before v3. (Jakub has suggested it before),
>> I wonder if Arun or other maintainers accept this solution or if there are other solutions?
>
> I think is not a big deal either way.
>
> To avoid too much back and forth I'll stick to the current code.
>
> Please address Emil comment on patch 6/6
>
> Thanks!
>
> Paolo
>
Hi Paolo,
Thanks! I'll fix it on patch 6/6 as suggested by Emil.
Best regards,
Samin
