| Message ID | 20250317025922.1526937-5-jacky_chou@aspeedtech.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add AST2600 RGMII delay into ftgmac100 | expand |
Hi, On Mon, 17 Mar 2025 10:59:22 +0800 Jacky Chou <jacky_chou@aspeedtech.com> wrote: > Use rx-internal-delay-ps and tx-internal-delay-ps > properties to configue the RGMII delay into corresponding register of > scu. Currently, the ftgmac100 driver only configures on AST2600 and will > be by pass the other platforms. > The details are in faraday,ftgmac100.yaml. > > Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com> > --- > drivers/net/ethernet/faraday/ftgmac100.c | 88 ++++++++++++++++++++++++ > drivers/net/ethernet/faraday/ftgmac100.h | 12 ++++ > 2 files changed, 100 insertions(+) > > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c > index 17ec35e75a65..ea2061488cba 100644 > --- a/drivers/net/ethernet/faraday/ftgmac100.c > +++ b/drivers/net/ethernet/faraday/ftgmac100.c > @@ -27,6 +27,9 @@ > #include <linux/phy_fixed.h> > #include <net/ip.h> > #include <net/ncsi.h> > +#include <linux/mfd/syscon.h> > +#include <linux/regmap.h> > +#include <linux/bitfield.h> > > #include "ftgmac100.h" > > @@ -1812,6 +1815,88 @@ static bool ftgmac100_has_child_node(struct device_node *np, const char *name) > return ret; > } > > +static void ftgmac100_set_internal_delay(struct platform_device *pdev) > +{ > + struct device_node *np = pdev->dev.of_node; > + struct net_device *netdev; > + struct ftgmac100 *priv; > + struct regmap *scu; > + u32 rgmii_tx_delay, rgmii_rx_delay; > + u32 dly_reg, tx_dly_mask, rx_dly_mask; > + int tx, rx; Please use the reverse christmas tree notation, sorting declarations by descending line length > + netdev = platform_get_drvdata(pdev); > + priv = netdev_priv(netdev); > + > + tx = of_property_read_u32(np, "tx-internal-delay-ps", &rgmii_tx_delay); > + rx = of_property_read_u32(np, "rx-internal-delay-ps", &rgmii_rx_delay); > + > + if (of_device_is_compatible(np, "aspeed,ast2600-mac")) { > + /* According to mac base address to get mac index */ > + switch (priv->res->start) { > + case 0x1e660000: > + dly_reg = AST2600_MAC12_CLK_DLY; > + tx_dly_mask = AST2600_MAC1_TX_DLY; > + rx_dly_mask = AST2600_MAC1_RX_DLY; > + rgmii_tx_delay = FIELD_PREP(AST2600_MAC1_TX_DLY, rgmii_tx_delay); > + rgmii_rx_delay = FIELD_PREP(AST2600_MAC1_RX_DLY, rgmii_rx_delay); > + break; > + case 0x1e680000: > + dly_reg = AST2600_MAC12_CLK_DLY; > + tx_dly_mask = AST2600_MAC2_TX_DLY; > + rx_dly_mask = AST2600_MAC2_RX_DLY; > + rgmii_tx_delay = FIELD_PREP(AST2600_MAC2_TX_DLY, rgmii_tx_delay); > + rgmii_rx_delay = FIELD_PREP(AST2600_MAC2_RX_DLY, rgmii_rx_delay); > + break; > + case 0x1e670000: > + dly_reg = AST2600_MAC34_CLK_DLY; > + tx_dly_mask = AST2600_MAC3_TX_DLY; > + rx_dly_mask = AST2600_MAC3_RX_DLY; > + rgmii_tx_delay = FIELD_PREP(AST2600_MAC3_TX_DLY, rgmii_tx_delay); > + rgmii_rx_delay = FIELD_PREP(AST2600_MAC3_RX_DLY, rgmii_rx_delay); > + break; > + case 0x1e690000: > + dly_reg = AST2600_MAC34_CLK_DLY; > + tx_dly_mask = AST2600_MAC4_TX_DLY; > + rx_dly_mask = AST2600_MAC4_RX_DLY; > + rgmii_tx_delay = FIELD_PREP(AST2600_MAC4_TX_DLY, rgmii_tx_delay); > + rgmii_rx_delay = FIELD_PREP(AST2600_MAC4_RX_DLY, rgmii_rx_delay); > + break; > + default: > + dev_warn(&pdev->dev, "Invalid mac base address"); > + return; There has to be a better way that directly looking up the base address. Maybe you need an extra DT property ? > + } > + } else { > + return; > + } > + > + scu = syscon_regmap_lookup_by_phandle(np, "scu"); > + if (IS_ERR(scu)) { > + dev_warn(&pdev->dev, "failed to map scu base"); > + return; > + } > + > + if (!tx) { > + /* Use tx-internal-delay-ps as index to configure tx delay > + * into scu register. > + */ So this goes completely against the naming of the property. It has the -ps suffix, so you would expect to have picoseconds values passed, and not an arbiraty index. Take a look at other drivers, you should accept picseconds values from these properties, then compute the relevant index in the driver. That index should be something internal to your driver. An example here : https://elixir.bootlin.com/linux/v6.14-rc6/source/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_rgmii.c#L51 > + if (rgmii_tx_delay > 64) > + dev_warn(&pdev->dev, "Get invalid tx delay value"); > + else > + regmap_update_bits(scu, dly_reg, tx_dly_mask, rgmii_tx_delay); > + } > + > + if (!rx) { > + /* Use rx-internal-delay-ps as index to configure rx delay > + * into scu register. > + */ > + if (rgmii_tx_delay > 64) > + dev_warn(&pdev->dev, "Get invalid rx delay value"); > + else > + regmap_update_bits(scu, dly_reg, rx_dly_mask, rgmii_rx_delay); > + } > +} > + > static int ftgmac100_probe(struct platform_device *pdev) > { > struct resource *res; > @@ -1977,6 +2062,9 @@ static int ftgmac100_probe(struct platform_device *pdev) > if (of_device_is_compatible(np, "aspeed,ast2600-mac")) > iowrite32(FTGMAC100_TM_DEFAULT, > priv->base + FTGMAC100_OFFSET_TM); > + > + /* Configure RGMII delay if there are the corresponding properties */ > + ftgmac100_set_internal_delay(pdev); > } > > /* Default ring sizes */ > diff --git a/drivers/net/ethernet/faraday/ftgmac100.h b/drivers/net/ethernet/faraday/ftgmac100.h > index 4968f6f0bdbc..d464d287502c 100644 > --- a/drivers/net/ethernet/faraday/ftgmac100.h > +++ b/drivers/net/ethernet/faraday/ftgmac100.h > @@ -271,4 +271,16 @@ struct ftgmac100_rxdes { > #define FTGMAC100_RXDES1_UDP_CHKSUM_ERR (1 << 26) > #define FTGMAC100_RXDES1_IP_CHKSUM_ERR (1 << 27) > > +/* Aspeed SCU */ > +#define AST2600_MAC12_CLK_DLY 0x340 > +#define AST2600_MAC1_TX_DLY GENMASK(5, 0) > +#define AST2600_MAC1_RX_DLY GENMASK(17, 12) > +#define AST2600_MAC2_TX_DLY GENMASK(11, 6) > +#define AST2600_MAC2_RX_DLY GENMASK(23, 18) > +#define AST2600_MAC34_CLK_DLY 0x350 > +#define AST2600_MAC3_TX_DLY AST2600_MAC1_TX_DLY > +#define AST2600_MAC3_RX_DLY AST2600_MAC1_RX_DLY > +#define AST2600_MAC4_TX_DLY AST2600_MAC2_TX_DLY > +#define AST2600_MAC4_RX_DLY AST2600_MAC2_RX_DLY > + > #endif /* __FTGMAC100_H */ Maxime
> > +static void ftgmac100_set_internal_delay(struct platform_device > > +*pdev) { > > + struct device_node *np = pdev->dev.of_node; > > + struct net_device *netdev; > > + struct ftgmac100 *priv; > > + struct regmap *scu; > > + u32 rgmii_tx_delay, rgmii_rx_delay; > > + u32 dly_reg, tx_dly_mask, rx_dly_mask; > > + int tx, rx; > > Please use the reverse christmas tree notation, sorting declarations by > descending line length Got it. I will modify these in next version. > > + netdev = platform_get_drvdata(pdev); > > + priv = netdev_priv(netdev); > > + > > + tx = of_property_read_u32(np, "tx-internal-delay-ps", &rgmii_tx_delay); > > + rx = of_property_read_u32(np, "rx-internal-delay-ps", > > +&rgmii_rx_delay); > > + > > + if (of_device_is_compatible(np, "aspeed,ast2600-mac")) { > > + /* According to mac base address to get mac index */ > > + switch (priv->res->start) { > > + case 0x1e660000: > > + dly_reg = AST2600_MAC12_CLK_DLY; > > + tx_dly_mask = AST2600_MAC1_TX_DLY; > > + rx_dly_mask = AST2600_MAC1_RX_DLY; > > + rgmii_tx_delay = FIELD_PREP(AST2600_MAC1_TX_DLY, > rgmii_tx_delay); > > + rgmii_rx_delay = FIELD_PREP(AST2600_MAC1_RX_DLY, > rgmii_rx_delay); > > + break; > > + case 0x1e680000: > > + dly_reg = AST2600_MAC12_CLK_DLY; > > + tx_dly_mask = AST2600_MAC2_TX_DLY; > > + rx_dly_mask = AST2600_MAC2_RX_DLY; > > + rgmii_tx_delay = FIELD_PREP(AST2600_MAC2_TX_DLY, > rgmii_tx_delay); > > + rgmii_rx_delay = FIELD_PREP(AST2600_MAC2_RX_DLY, > rgmii_rx_delay); > > + break; > > + case 0x1e670000: > > + dly_reg = AST2600_MAC34_CLK_DLY; > > + tx_dly_mask = AST2600_MAC3_TX_DLY; > > + rx_dly_mask = AST2600_MAC3_RX_DLY; > > + rgmii_tx_delay = FIELD_PREP(AST2600_MAC3_TX_DLY, > rgmii_tx_delay); > > + rgmii_rx_delay = FIELD_PREP(AST2600_MAC3_RX_DLY, > rgmii_rx_delay); > > + break; > > + case 0x1e690000: > > + dly_reg = AST2600_MAC34_CLK_DLY; > > + tx_dly_mask = AST2600_MAC4_TX_DLY; > > + rx_dly_mask = AST2600_MAC4_RX_DLY; > > + rgmii_tx_delay = FIELD_PREP(AST2600_MAC4_TX_DLY, > rgmii_tx_delay); > > + rgmii_rx_delay = FIELD_PREP(AST2600_MAC4_RX_DLY, > rgmii_rx_delay); > > + break; > > + default: > > + dev_warn(&pdev->dev, "Invalid mac base address"); > > + return; > > There has to be a better way that directly looking up the base address. > Maybe you need an extra DT property ? I use the base address to identify the MAC index because it is the only fixed value that is paired with the MAC. If I create a property to identify the MAC index and someone use the wrong value, the driver will configure the wrong register field. Therefore, I use the base address as MAC index, and it is very clear which MAC is configured. > > > + } > > + } else { > > + return; > > + } > > + > > + scu = syscon_regmap_lookup_by_phandle(np, "scu"); > > + if (IS_ERR(scu)) { > > + dev_warn(&pdev->dev, "failed to map scu base"); > > + return; > > + } > > + > > + if (!tx) { > > + /* Use tx-internal-delay-ps as index to configure tx delay > > + * into scu register. > > + */ > > So this goes completely against the naming of the property. It has the -ps suffix, > so you would expect to have picoseconds values passed, and not an arbiraty > index. > > Take a look at other drivers, you should accept picseconds values from these > properties, then compute the relevant index in the driver. That index should be > something internal to your driver. > > An example here : > > https://elixir.bootlin.com/linux/v6.14-rc6/source/drivers/net/ethernet/microchi > p/sparx5/lan969x/lan969x_rgmii.c#L51 > Agreed. Thank you for your information. I will adjust this part to use ps unit to translate to the relevant index in next version. Jacky
On Mon, Mar 17, 2025 at 09:53:33AM +0100, Maxime Chevallier wrote: > So this goes completely against the naming of the property. It has the > -ps suffix, so you would expect to have picoseconds values passed, and > not an arbiraty index. > > Take a look at other drivers, you should accept picseconds values from > these properties, then compute the relevant index in the driver. That > index should be something internal to your driver. > > An example here : > > https://elixir.bootlin.com/linux/v6.14-rc6/source/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_rgmii.c#L51 Another example would be drivers/net/phy/adin.c::adin_get_reg_value() and associated functions - these lookup a DT property and then look that up in a table to convert it to a register value. I suspect that's something which could become generic, as I suspect most hardware isn't going to accept a picosecond value, but be a choice of N different options.
> > So this goes completely against the naming of the property. It has the > > -ps suffix, so you would expect to have picoseconds values passed, and > > not an arbiraty index. > > > > Take a look at other drivers, you should accept picseconds values from > > these properties, then compute the relevant index in the driver. That > > index should be something internal to your driver. > > > > An example here : > > > > https://elixir.bootlin.com/linux/v6.14-rc6/source/drivers/net/ethernet > > /microchip/sparx5/lan969x/lan969x_rgmii.c#L51 > > Another example would be drivers/net/phy/adin.c::adin_get_reg_value() > and associated functions - these lookup a DT property and then look that up in > a table to convert it to a register value. > > I suspect that's something which could become generic, as I suspect most > hardware isn't going to accept a picosecond value, but be a choice of N > different options. Thank you for your information. The RGMII delay of AST2600 has a lot of steps can be configured. And the delay value of each MAC has different. Each MAC has 64 steps to configure RGMII TX/RX delay. Therefore, I use these properties as index to configure the register to reduce computing in driver. Or I do not use the property that defined in ethernet-controller.yaml, I create the property that defined by myself? Thanks, Jacky
> The RGMII delay of AST2600 has a lot of steps can be configured.
Are they uniformly space? Then it should be a simple formula to
calculate? Or a lookup table?
Andrew
> + u32 rgmii_tx_delay, rgmii_rx_delay; > + u32 dly_reg, tx_dly_mask, rx_dly_mask; > + int tx, rx; > + > + netdev = platform_get_drvdata(pdev); > + priv = netdev_priv(netdev); > + > + tx = of_property_read_u32(np, "tx-internal-delay-ps", &rgmii_tx_delay); > + rx = of_property_read_u32(np, "rx-internal-delay-ps", &rgmii_rx_delay); > + if (!tx) { The documentation for of_property_read_u32() says: * Return: 0 on success, -EINVAL if the property does not exist, * -ENODATA if property does not have a value, and -EOVERFLOW if the * property data isn't large enough. You need to handle EINVAL different to the other errors, which are real errors and should fail the probe. The commit message, and probably the binding needs to document what happens when the properties are not in the DT blob. This needs to be part of the bigger picture of how you are going to sort out the mess with existing .dts files listing 'rgmii' when in fact they should be 'rgmii-id'. > + /* Use tx-internal-delay-ps as index to configure tx delay > + * into scu register. > + */ > + if (rgmii_tx_delay > 64) > + dev_warn(&pdev->dev, "Get invalid tx delay value"); Return EINVAL and fail the probe. Andrew
diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c index 17ec35e75a65..ea2061488cba 100644 --- a/drivers/net/ethernet/faraday/ftgmac100.c +++ b/drivers/net/ethernet/faraday/ftgmac100.c @@ -27,6 +27,9 @@ #include <linux/phy_fixed.h> #include <net/ip.h> #include <net/ncsi.h> +#include <linux/mfd/syscon.h> +#include <linux/regmap.h> +#include <linux/bitfield.h> #include "ftgmac100.h" @@ -1812,6 +1815,88 @@ static bool ftgmac100_has_child_node(struct device_node *np, const char *name) return ret; } +static void ftgmac100_set_internal_delay(struct platform_device *pdev) +{ + struct device_node *np = pdev->dev.of_node; + struct net_device *netdev; + struct ftgmac100 *priv; + struct regmap *scu; + u32 rgmii_tx_delay, rgmii_rx_delay; + u32 dly_reg, tx_dly_mask, rx_dly_mask; + int tx, rx; + + netdev = platform_get_drvdata(pdev); + priv = netdev_priv(netdev); + + tx = of_property_read_u32(np, "tx-internal-delay-ps", &rgmii_tx_delay); + rx = of_property_read_u32(np, "rx-internal-delay-ps", &rgmii_rx_delay); + + if (of_device_is_compatible(np, "aspeed,ast2600-mac")) { + /* According to mac base address to get mac index */ + switch (priv->res->start) { + case 0x1e660000: + dly_reg = AST2600_MAC12_CLK_DLY; + tx_dly_mask = AST2600_MAC1_TX_DLY; + rx_dly_mask = AST2600_MAC1_RX_DLY; + rgmii_tx_delay = FIELD_PREP(AST2600_MAC1_TX_DLY, rgmii_tx_delay); + rgmii_rx_delay = FIELD_PREP(AST2600_MAC1_RX_DLY, rgmii_rx_delay); + break; + case 0x1e680000: + dly_reg = AST2600_MAC12_CLK_DLY; + tx_dly_mask = AST2600_MAC2_TX_DLY; + rx_dly_mask = AST2600_MAC2_RX_DLY; + rgmii_tx_delay = FIELD_PREP(AST2600_MAC2_TX_DLY, rgmii_tx_delay); + rgmii_rx_delay = FIELD_PREP(AST2600_MAC2_RX_DLY, rgmii_rx_delay); + break; + case 0x1e670000: + dly_reg = AST2600_MAC34_CLK_DLY; + tx_dly_mask = AST2600_MAC3_TX_DLY; + rx_dly_mask = AST2600_MAC3_RX_DLY; + rgmii_tx_delay = FIELD_PREP(AST2600_MAC3_TX_DLY, rgmii_tx_delay); + rgmii_rx_delay = FIELD_PREP(AST2600_MAC3_RX_DLY, rgmii_rx_delay); + break; + case 0x1e690000: + dly_reg = AST2600_MAC34_CLK_DLY; + tx_dly_mask = AST2600_MAC4_TX_DLY; + rx_dly_mask = AST2600_MAC4_RX_DLY; + rgmii_tx_delay = FIELD_PREP(AST2600_MAC4_TX_DLY, rgmii_tx_delay); + rgmii_rx_delay = FIELD_PREP(AST2600_MAC4_RX_DLY, rgmii_rx_delay); + break; + default: + dev_warn(&pdev->dev, "Invalid mac base address"); + return; + } + } else { + return; + } + + scu = syscon_regmap_lookup_by_phandle(np, "scu"); + if (IS_ERR(scu)) { + dev_warn(&pdev->dev, "failed to map scu base"); + return; + } + + if (!tx) { + /* Use tx-internal-delay-ps as index to configure tx delay + * into scu register. + */ + if (rgmii_tx_delay > 64) + dev_warn(&pdev->dev, "Get invalid tx delay value"); + else + regmap_update_bits(scu, dly_reg, tx_dly_mask, rgmii_tx_delay); + } + + if (!rx) { + /* Use rx-internal-delay-ps as index to configure rx delay + * into scu register. + */ + if (rgmii_tx_delay > 64) + dev_warn(&pdev->dev, "Get invalid rx delay value"); + else + regmap_update_bits(scu, dly_reg, rx_dly_mask, rgmii_rx_delay); + } +} + static int ftgmac100_probe(struct platform_device *pdev) { struct resource *res; @@ -1977,6 +2062,9 @@ static int ftgmac100_probe(struct platform_device *pdev) if (of_device_is_compatible(np, "aspeed,ast2600-mac")) iowrite32(FTGMAC100_TM_DEFAULT, priv->base + FTGMAC100_OFFSET_TM); + + /* Configure RGMII delay if there are the corresponding properties */ + ftgmac100_set_internal_delay(pdev); } /* Default ring sizes */ diff --git a/drivers/net/ethernet/faraday/ftgmac100.h b/drivers/net/ethernet/faraday/ftgmac100.h index 4968f6f0bdbc..d464d287502c 100644 --- a/drivers/net/ethernet/faraday/ftgmac100.h +++ b/drivers/net/ethernet/faraday/ftgmac100.h @@ -271,4 +271,16 @@ struct ftgmac100_rxdes { #define FTGMAC100_RXDES1_UDP_CHKSUM_ERR (1 << 26) #define FTGMAC100_RXDES1_IP_CHKSUM_ERR (1 << 27) +/* Aspeed SCU */ +#define AST2600_MAC12_CLK_DLY 0x340 +#define AST2600_MAC1_TX_DLY GENMASK(5, 0) +#define AST2600_MAC1_RX_DLY GENMASK(17, 12) +#define AST2600_MAC2_TX_DLY GENMASK(11, 6) +#define AST2600_MAC2_RX_DLY GENMASK(23, 18) +#define AST2600_MAC34_CLK_DLY 0x350 +#define AST2600_MAC3_TX_DLY AST2600_MAC1_TX_DLY +#define AST2600_MAC3_RX_DLY AST2600_MAC1_RX_DLY +#define AST2600_MAC4_TX_DLY AST2600_MAC2_TX_DLY +#define AST2600_MAC4_RX_DLY AST2600_MAC2_RX_DLY + #endif /* __FTGMAC100_H */
Use rx-internal-delay-ps and tx-internal-delay-ps properties to configue the RGMII delay into corresponding register of scu. Currently, the ftgmac100 driver only configures on AST2600 and will be by pass the other platforms. The details are in faraday,ftgmac100.yaml. Signed-off-by: Jacky Chou <jacky_chou@aspeedtech.com> --- drivers/net/ethernet/faraday/ftgmac100.c | 88 ++++++++++++++++++++++++ drivers/net/ethernet/faraday/ftgmac100.h | 12 ++++ 2 files changed, 100 insertions(+)