| Message ID | 20231029042712.520010-6-cristian.ciocaltea@collabora.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enable networking support for StarFive JH7100 SoC | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/vmtest-fixes-PR | fail | merge-conflict |
Cristian Ciocaltea wrote: > Add a missing quirk to enable support for the StarFive JH7100 SoC. > > Additionally, for greater flexibility in operation, allow using the > rgmii-rxid and rgmii-txid phy modes. > > Co-developed-by: Emil Renner Berthing <kernel@esmil.dk> > Signed-off-by: Emil Renner Berthing <kernel@esmil.dk> > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> Hi Cristian, Thanks for working on this! This driver has code to update the phy clock for different line speeds. I don't think that will work without the CLK_SET_RATE_PARENT flag added to the clock in [1] which in turn depends on [2]. [1]: https://github.com/esmil/linux/commit/b200c3054b58a49ba25af67aff82d9045e3c3666 [2]: https://github.com/esmil/linux/commit/dce189542c16bf0eb8533d96c0305cb59d149dae Two more comments below.. > --- > drivers/net/ethernet/stmicro/stmmac/Kconfig | 6 ++-- > .../ethernet/stmicro/stmmac/dwmac-starfive.c | 32 ++++++++++++++++--- > 2 files changed, 31 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig > index a2b9e289aa36..c3c2c8360047 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig > +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig > @@ -165,9 +165,9 @@ config DWMAC_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. > + This selects the StarFive platform specific glue layer support > + for the stmmac device driver. This driver is used for the > + StarFive JH7100 and JH7110 ethernet controllers. > > config DWMAC_STI > tristate "STi GMAC support" > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c > index 5d630affb4d1..88c431edcea0 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c > @@ -15,13 +15,20 @@ > > #include "stmmac_platform.h" > > -#define STARFIVE_DWMAC_PHY_INFT_RGMII 0x1 > -#define STARFIVE_DWMAC_PHY_INFT_RMII 0x4 > -#define STARFIVE_DWMAC_PHY_INFT_FIELD 0x7U > +#define STARFIVE_DWMAC_PHY_INFT_RGMII 0x1 > +#define STARFIVE_DWMAC_PHY_INFT_RMII 0x4 > +#define STARFIVE_DWMAC_PHY_INFT_FIELD 0x7U > + > +#define JH7100_SYSMAIN_REGISTER49_DLYCHAIN 0xc8 > + > +struct starfive_dwmac_data { > + unsigned int gtxclk_dlychain; > +}; > > struct starfive_dwmac { > struct device *dev; > struct clk *clk_tx; > + const struct starfive_dwmac_data *data; > }; > > static void starfive_dwmac_fix_mac_speed(void *priv, unsigned int speed, unsigned int mode) > @@ -67,6 +74,8 @@ static int starfive_dwmac_set_mode(struct plat_stmmacenet_data *plat_dat) > > case PHY_INTERFACE_MODE_RGMII: > case PHY_INTERFACE_MODE_RGMII_ID: > + case PHY_INTERFACE_MODE_RGMII_RXID: > + case PHY_INTERFACE_MODE_RGMII_TXID: > mode = STARFIVE_DWMAC_PHY_INFT_RGMII; > break; > > @@ -89,6 +98,14 @@ static int starfive_dwmac_set_mode(struct plat_stmmacenet_data *plat_dat) > if (err) > return dev_err_probe(dwmac->dev, err, "error setting phy mode\n"); > > + if (dwmac->data) { I think you want something like this so future quirks don't need to touch this code: if (dwmac->data && dwmac->data->gtxclk_dlychain) > + err = regmap_write(regmap, JH7100_SYSMAIN_REGISTER49_DLYCHAIN, > + dwmac->data->gtxclk_dlychain); > + if (err) > + return dev_err_probe(dwmac->dev, err, > + "error selecting gtxclk delay chain\n"); > + } > + > return 0; > } > > @@ -114,6 +131,8 @@ static int starfive_dwmac_probe(struct platform_device *pdev) > if (!dwmac) > return -ENOMEM; > > + dwmac->data = device_get_match_data(&pdev->dev); > + > dwmac->clk_tx = devm_clk_get_enabled(&pdev->dev, "tx"); > if (IS_ERR(dwmac->clk_tx)) > return dev_err_probe(&pdev->dev, PTR_ERR(dwmac->clk_tx), > @@ -144,8 +163,13 @@ static int starfive_dwmac_probe(struct platform_device *pdev) > return stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res); > } > > +static const struct starfive_dwmac_data jh7100_data = { > + .gtxclk_dlychain = 4 Please add a , at the end of this line. I know it's unlikely that we need to add more properties, but it's still good practice to do. This way such patches won't need to touch this line. > +}; > + > static const struct of_device_id starfive_dwmac_match[] = { > - { .compatible = "starfive,jh7110-dwmac" }, > + { .compatible = "starfive,jh7100-dwmac", .data = &jh7100_data }, > + { .compatible = "starfive,jh7110-dwmac" }, > { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(of, starfive_dwmac_match); > -- > 2.42.0 >
On 10/31/23 16:33, Emil Renner Berthing wrote: > Cristian Ciocaltea wrote: >> Add a missing quirk to enable support for the StarFive JH7100 SoC. >> >> Additionally, for greater flexibility in operation, allow using the >> rgmii-rxid and rgmii-txid phy modes. >> >> Co-developed-by: Emil Renner Berthing <kernel@esmil.dk> >> Signed-off-by: Emil Renner Berthing <kernel@esmil.dk> >> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com> > > Hi Cristian, > > Thanks for working on this! This driver has code to update the phy clock for > different line speeds. I don't think that will work without the > CLK_SET_RATE_PARENT flag added to the clock in [1] which in turn depends on > [2]. > > [1]: https://github.com/esmil/linux/commit/b200c3054b58a49ba25af67aff82d9045e3c3666 > [2]: https://github.com/esmil/linux/commit/dce189542c16bf0eb8533d96c0305cb59d149dae > > Two more comments below.. Hi Emil, Thanks for the review! I've been only testing this at 1000 Mbps and so far it seems to work fine. I will try with different line speeds and report back. >> --- >> drivers/net/ethernet/stmicro/stmmac/Kconfig | 6 ++-- >> .../ethernet/stmicro/stmmac/dwmac-starfive.c | 32 ++++++++++++++++--- >> 2 files changed, 31 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig >> index a2b9e289aa36..c3c2c8360047 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig >> +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig >> @@ -165,9 +165,9 @@ config DWMAC_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. >> + This selects the StarFive platform specific glue layer support >> + for the stmmac device driver. This driver is used for the >> + StarFive JH7100 and JH7110 ethernet controllers. >> >> config DWMAC_STI >> tristate "STi GMAC support" >> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c >> index 5d630affb4d1..88c431edcea0 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c >> @@ -15,13 +15,20 @@ >> >> #include "stmmac_platform.h" >> >> -#define STARFIVE_DWMAC_PHY_INFT_RGMII 0x1 >> -#define STARFIVE_DWMAC_PHY_INFT_RMII 0x4 >> -#define STARFIVE_DWMAC_PHY_INFT_FIELD 0x7U >> +#define STARFIVE_DWMAC_PHY_INFT_RGMII 0x1 >> +#define STARFIVE_DWMAC_PHY_INFT_RMII 0x4 >> +#define STARFIVE_DWMAC_PHY_INFT_FIELD 0x7U >> + >> +#define JH7100_SYSMAIN_REGISTER49_DLYCHAIN 0xc8 >> + >> +struct starfive_dwmac_data { >> + unsigned int gtxclk_dlychain; >> +}; >> >> struct starfive_dwmac { >> struct device *dev; >> struct clk *clk_tx; >> + const struct starfive_dwmac_data *data; >> }; >> >> static void starfive_dwmac_fix_mac_speed(void *priv, unsigned int speed, unsigned int mode) >> @@ -67,6 +74,8 @@ static int starfive_dwmac_set_mode(struct plat_stmmacenet_data *plat_dat) >> >> case PHY_INTERFACE_MODE_RGMII: >> case PHY_INTERFACE_MODE_RGMII_ID: >> + case PHY_INTERFACE_MODE_RGMII_RXID: >> + case PHY_INTERFACE_MODE_RGMII_TXID: >> mode = STARFIVE_DWMAC_PHY_INFT_RGMII; >> break; >> >> @@ -89,6 +98,14 @@ static int starfive_dwmac_set_mode(struct plat_stmmacenet_data *plat_dat) >> if (err) >> return dev_err_probe(dwmac->dev, err, "error setting phy mode\n"); >> >> + if (dwmac->data) { > > I think you want something like this so future quirks don't need to touch this > code: > > if (dwmac->data && dwmac->data->gtxclk_dlychain) Yes, but that would prevent having a quirk that explicitly wants to write 0. I was initially thinking to something more generic, like providing a list of register-value pairs, but I'm not sure this is going to be ever needed. I'm still open to apply the suggested change, though. >> + err = regmap_write(regmap, JH7100_SYSMAIN_REGISTER49_DLYCHAIN, >> + dwmac->data->gtxclk_dlychain); >> + if (err) >> + return dev_err_probe(dwmac->dev, err, >> + "error selecting gtxclk delay chain\n"); >> + } >> + >> return 0; >> } >> >> @@ -114,6 +131,8 @@ static int starfive_dwmac_probe(struct platform_device *pdev) >> if (!dwmac) >> return -ENOMEM; >> >> + dwmac->data = device_get_match_data(&pdev->dev); >> + >> dwmac->clk_tx = devm_clk_get_enabled(&pdev->dev, "tx"); >> if (IS_ERR(dwmac->clk_tx)) >> return dev_err_probe(&pdev->dev, PTR_ERR(dwmac->clk_tx), >> @@ -144,8 +163,13 @@ static int starfive_dwmac_probe(struct platform_device *pdev) >> return stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res); >> } >> >> +static const struct starfive_dwmac_data jh7100_data = { >> + .gtxclk_dlychain = 4 > > Please add a , at the end of this line. I know it's unlikely that we need to > add more properties, but it's still good practice to do. This way such patches > won't need to touch this line. Sure, will do. >> +}; >> + >> static const struct of_device_id starfive_dwmac_match[] = { >> - { .compatible = "starfive,jh7110-dwmac" }, >> + { .compatible = "starfive,jh7100-dwmac", .data = &jh7100_data }, >> + { .compatible = "starfive,jh7110-dwmac" }, >> { /* sentinel */ } >> }; >> MODULE_DEVICE_TABLE(of, starfive_dwmac_match); >> -- >> 2.42.0 >>
diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig index a2b9e289aa36..c3c2c8360047 100644 --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig @@ -165,9 +165,9 @@ config DWMAC_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. + This selects the StarFive platform specific glue layer support + for the stmmac device driver. This driver is used for the + StarFive JH7100 and JH7110 ethernet controllers. config DWMAC_STI tristate "STi GMAC support" diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c index 5d630affb4d1..88c431edcea0 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c @@ -15,13 +15,20 @@ #include "stmmac_platform.h" -#define STARFIVE_DWMAC_PHY_INFT_RGMII 0x1 -#define STARFIVE_DWMAC_PHY_INFT_RMII 0x4 -#define STARFIVE_DWMAC_PHY_INFT_FIELD 0x7U +#define STARFIVE_DWMAC_PHY_INFT_RGMII 0x1 +#define STARFIVE_DWMAC_PHY_INFT_RMII 0x4 +#define STARFIVE_DWMAC_PHY_INFT_FIELD 0x7U + +#define JH7100_SYSMAIN_REGISTER49_DLYCHAIN 0xc8 + +struct starfive_dwmac_data { + unsigned int gtxclk_dlychain; +}; struct starfive_dwmac { struct device *dev; struct clk *clk_tx; + const struct starfive_dwmac_data *data; }; static void starfive_dwmac_fix_mac_speed(void *priv, unsigned int speed, unsigned int mode) @@ -67,6 +74,8 @@ static int starfive_dwmac_set_mode(struct plat_stmmacenet_data *plat_dat) case PHY_INTERFACE_MODE_RGMII: case PHY_INTERFACE_MODE_RGMII_ID: + case PHY_INTERFACE_MODE_RGMII_RXID: + case PHY_INTERFACE_MODE_RGMII_TXID: mode = STARFIVE_DWMAC_PHY_INFT_RGMII; break; @@ -89,6 +98,14 @@ static int starfive_dwmac_set_mode(struct plat_stmmacenet_data *plat_dat) if (err) return dev_err_probe(dwmac->dev, err, "error setting phy mode\n"); + if (dwmac->data) { + err = regmap_write(regmap, JH7100_SYSMAIN_REGISTER49_DLYCHAIN, + dwmac->data->gtxclk_dlychain); + if (err) + return dev_err_probe(dwmac->dev, err, + "error selecting gtxclk delay chain\n"); + } + return 0; } @@ -114,6 +131,8 @@ static int starfive_dwmac_probe(struct platform_device *pdev) if (!dwmac) return -ENOMEM; + dwmac->data = device_get_match_data(&pdev->dev); + dwmac->clk_tx = devm_clk_get_enabled(&pdev->dev, "tx"); if (IS_ERR(dwmac->clk_tx)) return dev_err_probe(&pdev->dev, PTR_ERR(dwmac->clk_tx), @@ -144,8 +163,13 @@ static int starfive_dwmac_probe(struct platform_device *pdev) return stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res); } +static const struct starfive_dwmac_data jh7100_data = { + .gtxclk_dlychain = 4 +}; + static const struct of_device_id starfive_dwmac_match[] = { - { .compatible = "starfive,jh7110-dwmac" }, + { .compatible = "starfive,jh7100-dwmac", .data = &jh7100_data }, + { .compatible = "starfive,jh7110-dwmac" }, { /* sentinel */ } }; MODULE_DEVICE_TABLE(of, starfive_dwmac_match);