Re: [-net-next v11 5/6] net: stmmac: Add glue layer for StarFive JH7110 SoC

[Guo Samin <samin.guo@starfivetech.com>]

Re: [-net-next v11 5/6] net: stmmac: Add glue layer for StarFive JH7110 SoC
From: Paolo Abeni <pabeni@redhat.com>
to: Guo Samin <samin.guo@starfivetech.com>, Emil Renner Berthing <emil.renner.berthing@canonical.com>, Arun.Ramadoss@microchip.com
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@canonical.com>
>> to: Guo Samin <samin.guo@starfivetech.com>
>> data: 2023/4/9
>>
>>> On Sat, 8 Apr 2023 at 03:16, Guo Samin <samin.guo@starfivetech.com> wrote:
>>>>
>>>>  Re: [-net-next v11 5/6] net: stmmac: Add glue layer for StarFive JH7110 SoC
>>>> From: Emil Renner Berthing <emil.renner.berthing@canonical.com>
>>>> to: Samin Guo <samin.guo@starfivetech.com>
>>>> data: 2023/4/8
>>>>
>>>>> On Fri, 7 Apr 2023 at 13:05, Samin Guo <samin.guo@starfivetech.com> wrote:
>>>>>>
>>>>>> This adds StarFive dwmac driver support on the StarFive JH7110 SoC.
>>>>>>
>>>>>> Tested-by: Tommaso Merciai <tomm.merciai@gmail.com>
>>>>>> Co-developed-by: Emil Renner Berthing <kernel@esmil.dk>
>>>>>> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk>
>>>>>> Signed-off-by: Samin Guo <samin.guo@starfivetech.com>
>>>>>> ---
>>>>>>  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@esmil.dk>
>>>>>>  M:     Samin Guo <samin.guo@starfivetech.com>
>>>>>>  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@esmil.dk>
>>>>>> 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@esmil.dk>
>>>>>> + * 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@starfivetech.com)
>>>>
>>>>
>>>>> +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@starfivetech.com)
>>>>
>>>
>>> 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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv