lkml.org 
[lkml]   [2023]   [Oct]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v2 05/12] net: stmmac: dwmac-starfive: Add support for JH7100 SoC
    From
    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
    >>

    \
     
     \ /
      Last update: 2023-10-31 19:08    [W:5.595 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site