Message-ID: <CAJM55Z8zYUQc33r9tJB1du-FSp+uDf40720taMuGTuPcPU+aZg@mail.gmail.com>
Date:   Mon, 6 Mar 2023 13:49:57 +0100
From:   Emil Renner Berthing <emil.renner.berthing@...onical.com>
To:     Guo Samin <samin.guo@...rfivetech.com>
Cc:     linux-riscv@...ts.infradead.org, netdev@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Emil Renner Berthing <kernel@...il.dk>,
        Richard Cochran <richardcochran@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Peter Geis <pgwipeout@...il.com>,
        Yanhong Wang <yanhong.wang@...rfivetech.com>
Subject: Re: [PATCH v5 08/12] net: stmmac: starfive_dmac: Add phy interface settings

On Mon, 6 Mar 2023 at 04:07, Guo Samin <samin.guo@...rfivetech.com> wrote:
> 在 2023/3/4 0:50:54, Emil Renner Berthing 写道:
> > On Fri, 3 Mar 2023 at 10:01, Samin Guo <samin.guo@...rfivetech.com> wrote:
> >>
> >> dwmac supports multiple modess. When working under rmii and rgmii,
> >> you need to set different phy interfaces.
> >>
> >> According to the dwmac document, when working in rmii, it needs to be
> >> set to 0x4, and rgmii needs to be set to 0x1.
> >>
> >> The phy interface needs to be set in syscon, the format is as follows:
> >> starfive,syscon: <&syscon, offset, mask>
> >>
> >> Signed-off-by: Samin Guo <samin.guo@...rfivetech.com>
> >> ---
> >>  .../ethernet/stmicro/stmmac/dwmac-starfive.c  | 46 +++++++++++++++++++
> >>  1 file changed, 46 insertions(+)
> >>
> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
> >> index 566378306f67..40fdd7036127 100644
> >> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
> >> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
> >> @@ -7,10 +7,15 @@
> >>   *
> >>   */
> >>
> >> +#include <linux/mfd/syscon.h>
> >>  #include <linux/of_device.h>
> >> +#include <linux/regmap.h>
> >>
> >>  #include "stmmac_platform.h"
> >>
> >> +#define MACPHYC_PHY_INFT_RMII  0x4
> >> +#define MACPHYC_PHY_INFT_RGMII 0x1
> >
> > Please prefix these with something like STARFIVE_DWMAC_
> >
> Hi, Emil, These definitions come from the datasheet of dwmac. However, add STARDRIVE_ DWMAC is a good idea. I will fix it,thanks.
> >>  struct starfive_dwmac {
> >>         struct device *dev;
> >>         struct clk *clk_tx;
> >> @@ -53,6 +58,46 @@ static void starfive_eth_fix_mac_speed(void *priv, unsigned int speed)
> >>                 dev_err(dwmac->dev, "failed to set tx rate %lu\n", rate);
> >>  }
> >>
> >> +static int starfive_dwmac_set_mode(struct plat_stmmacenet_data *plat_dat)
> >> +{
> >> +       struct starfive_dwmac *dwmac = plat_dat->bsp_priv;
> >> +       struct of_phandle_args args;
> >> +       struct regmap *regmap;
> >> +       unsigned int reg, mask, mode;
> >> +       int err;
> >> +
> >> +       switch (plat_dat->interface) {
> >> +       case PHY_INTERFACE_MODE_RMII:
> >> +               mode = MACPHYC_PHY_INFT_RMII;
> >> +               break;
> >> +
> >> +       case PHY_INTERFACE_MODE_RGMII:
> >> +       case PHY_INTERFACE_MODE_RGMII_ID:
> >> +               mode = MACPHYC_PHY_INFT_RGMII;
> >> +               break;
> >> +
> >> +       default:
> >> +               dev_err(dwmac->dev, "Unsupported interface %d\n",
> >> +                       plat_dat->interface);
> >> +       }
> >> +
> >> +       err = of_parse_phandle_with_fixed_args(dwmac->dev->of_node,
> >> +                                              "starfive,syscon", 2, 0, &args);
> >> +       if (err) {
> >> +               dev_dbg(dwmac->dev, "syscon reg not found\n");
> >> +               return -EINVAL;
> >> +       }
> >> +
> >> +       reg = args.args[0];
> >> +       mask = args.args[1];
> >> +       regmap = syscon_node_to_regmap(args.np);
> >> +       of_node_put(args.np);
> >
> > I think the above is basically
> > unsigned int args[2];
> > syscon_regmap_lookup_by_phandle_args(dwmac->dev_of_node,
> > "starfive,syscon", 2, args);
> >
> > ..but as Andrew points out another solution is to use platform match
> > data for this. Eg.
> >
> > static const struct starfive_dwmac_match_data starfive_dwmac_jh7110_data {
> >   .phy_interface_offset = 0xc,
> >   .phy_interface_mask = 0x1c0000,
> > };
> >
> > static const struct of_device_id starfive_dwmac_match[] = {
> >   { .compatible = "starfive,jh7110-dwmac", .data =
> > &starfive_dwmac_jh7110_data },
> >   { /* sentinel */ }
> > };
> >
> > and in the probe function:
> >
> Hi Emil, Yes，this is usually a good solution, and I have considered this plan before.
> However, gmac0 of jh7110 is different from the reg/mask of gmac1.
> You can find it in patch-9:
>
> &gmac0 {
>         starfive,syscon = <&aon_syscon 0xc 0x1c0000>;
> };
>
> &gmac1 {
>         starfive,syscon = <&sys_syscon 0x90 0x1c>;
> };
>
> In this case, using match_data of starfive,jh7110-dwma does not seem to be compatible.

Ugh, you're right. Both the syscon block, the register offset and the
bit position in those registers are different from gmac0 to gmac1, and
since we need a phandle to the syscon block anyway passing those two
other parameters as arguments is probably the nicest solution. For the
next version I'd change the 2nd argument from mask to the bit position
though. It seems the field is always 3 bits wide and this makes it a
little clearer that we're not just putting register values in the
device tree. Eg. something like

regmap = syscon_regmap_lookup_by_phandle_args(dev->of_node,
"starfive,syscon", 2, args);
...
err = regmap_update_bits(regmap, args[0], 7U << args[1], mode << args[1]);
...

Alternatively we'd put data for each gmac interface in the platform
data including the syscon compatible string, and use
syscon_regmap_lookup_by_compatible("starfive,jh7110-aon-syscon"); for
gmac0 fx. This way the dependency from the gmac nodes to the syscon
nodes won't be recorded is the device tree though.

@Andrew is this what you were suggesting?

> > struct starfive_dwmac_match_data *pdata = device_get_match_data(&pdev->dev);
> >
> >> +       if (IS_ERR(regmap))
> >> +               return PTR_ERR(regmap);
> >> +
> >> +       return regmap_update_bits(regmap, reg, mask, mode << __ffs(mask));
> >> +}
> >> +
> >>  static int starfive_dwmac_probe(struct platform_device *pdev)
> >>  {
> >>         struct plat_stmmacenet_data *plat_dat;
> >> @@ -93,6 +138,7 @@ static int starfive_dwmac_probe(struct platform_device *pdev)
> >>         plat_dat->bsp_priv = dwmac;
> >>         plat_dat->dma_cfg->dche = true;
> >>
> >> +       starfive_dwmac_set_mode(plat_dat);
> >
> > The function returns errors in an int, but you never check it :(
> >
> Thank you for pointing out that it will be added in the next version.
> >>         err = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
> >>         if (err) {
> >>                 stmmac_remove_config_dt(pdev, plat_dat);
>
>
> Best regards,
> Samin
>
> >> --
> >> 2.17.1
> >>
> >>
> >> _______________________________________________
> >> linux-riscv mailing list
> >> linux-riscv@...ts.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-riscv
>
> --
> Best regards,
> Samin

Powered by blists - more mailing lists