| Message ID | 20230303085928.4535-9-samin.guo@starfivetech.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Ethernet driver for StarFive JH7110 SoC | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/tree_selection | fail | Failed to apply to next/pending-fixes or riscv/for-next |
> +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); > + } Please add a return -EINVAL; > + > + 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); > + if (IS_ERR(regmap)) > + return PTR_ERR(regmap); > + > + return regmap_update_bits(regmap, reg, mask, mode << __ffs(mask)); This is a poor device tree binding. We generally don't allow bindings which say put value X in register Y. Could you add a table: interface mode, reg, mask? You can then do a simple lookup based on the interface mode? No device tree binding needed at all? Andrew
On Fri, 3 Mar 2023 at 10:01, Samin Guo <samin.guo@starfivetech.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@starfivetech.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_ > 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: 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 :( > err = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res); > if (err) { > stmmac_remove_config_dt(pdev, plat_dat); > -- > 2.17.1 > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
在 2023/3/4 0:50:54, Emil Renner Berthing 写道: > On Fri, 3 Mar 2023 at 10:01, Samin Guo <samin.guo@starfivetech.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@starfivetech.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. > 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@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-riscv
On Mon, 6 Mar 2023 at 04:07, Guo Samin <samin.guo@starfivetech.com> wrote: > 在 2023/3/4 0:50:54, Emil Renner Berthing 写道: > > On Fri, 3 Mar 2023 at 10:01, Samin Guo <samin.guo@starfivetech.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@starfivetech.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@lists.infradead.org > >> http://lists.infradead.org/mailman/listinfo/linux-riscv > > -- > Best regards, > Samin
> 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. I prefer bit position over mask. But please fully document this in the device tree. This is something a board developer is going to get wrong, because they assume MAC blocks are identical, and normally need identical configuration. I assume this is also a hardware 'bug', and the next generation of the silicon will have this fixed? So this will go away? Andrew
在 2023/3/6 21:06:20, Andrew Lunn 写道: >> 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. > > I prefer bit position over mask. > > But please fully document this in the device tree. This is something a > board developer is going to get wrong, because they assume MAC blocks > are identical, and normally need identical configuration. > > I assume this is also a hardware 'bug', and the next generation of the > silicon will have this fixed? So this will go away? > > Andrew Hi Andrew, Yes, the hardware design does not take into account the feasibility of the software. The next version will be fixed. Thank you. I will use bit position instead of mask, which is described in detail in the document. Best regards, Samin
在 2023/3/6 20:49:57, Emil Renner Berthing 写道: > On Mon, 6 Mar 2023 at 04:07, Guo Samin <samin.guo@starfivetech.com> wrote: >> 在 2023/3/4 0:50:54, Emil Renner Berthing 写道: >>> On Fri, 3 Mar 2023 at 10:01, Samin Guo <samin.guo@starfivetech.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@starfivetech.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 > Yes，the field is always 3 bits wide, the next version will use bit position instead of mask. Thank you for your advice. > 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]); > ... > I also think the current method is relatively simple and compatible. Best regards, Samin > 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@lists.infradead.org >>>> http://lists.infradead.org/mailman/listinfo/linux-riscv >> >> -- >> Best regards, >> Samin
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 + 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); + 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); err = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res); if (err) { stmmac_remove_config_dt(pdev, plat_dat);
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@starfivetech.com> --- .../ethernet/stmicro/stmmac/dwmac-starfive.c | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+)