| Message ID | 20220126101432.822818-5-tobias@waldekranz.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net/fsl: xgmac_mdio: Preamble suppression and custom MDC frequencies | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 4 of 4 maintainers |
| netdev/build_clang | success | Errors and warnings before: 2 this patch: 2 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 65 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
Hi Tobias > struct mdio_fsl_priv { > struct tgec_mdio_controller __iomem *mdio_base; > + struct clk *enet_clk; It looks like there is a whitespace issue here? > + u32 mdc_freq; > bool is_little_endian; > bool has_a009885; > bool has_a011043; > @@ -255,6 +258,34 @@ static int xgmac_mdio_read(struct mii_bus *bus, int phy_id, int regnum) > return ret; > } > > +static void xgmac_mdio_set_mdc_freq(struct mii_bus *bus) > +{ > + struct mdio_fsl_priv *priv = (struct mdio_fsl_priv *)bus->priv; > + struct tgec_mdio_controller __iomem *regs = priv->mdio_base; > + struct device *dev = bus->parent; > + u32 mdio_stat, div; > + > + if (device_property_read_u32(dev, "clock-frequency", &priv->mdc_freq)) > + return; > + > + priv->enet_clk = devm_clk_get(dev, NULL); > + if (IS_ERR(priv->enet_clk)) { > + dev_err(dev, "Input clock unknown, not changing MDC frequency"); Is the clock optional or mandatory? If mandatory, then i would prefer -ENODEV and fail the probe. > + return; > + } > + > + div = ((clk_get_rate(priv->enet_clk) / priv->mdc_freq) - 1) / 2; > + if (div < 5 || div > 0x1ff) { > + dev_err(dev, "Requested MDC frequecy is out of range, ignoring"); and here return -EINVAL. I always think it is best to make it obvious something is broken. One additional line on the console can be missed for a while. All the Ethernet devices missing because the PHYs are missing, because the MDIO bus is missing is going to get noticed very quickly. Andrew
On Wed, Jan 26, 2022 at 14:07, Andrew Lunn <andrew@lunn.ch> wrote: > Hi Tobias > >> struct mdio_fsl_priv { >> struct tgec_mdio_controller __iomem *mdio_base; >> + struct clk *enet_clk; > > It looks like there is a whitespace issue here? > Ahh, sorry about that! >> + u32 mdc_freq; >> bool is_little_endian; >> bool has_a009885; >> bool has_a011043; >> @@ -255,6 +258,34 @@ static int xgmac_mdio_read(struct mii_bus *bus, int phy_id, int regnum) >> return ret; >> } >> >> +static void xgmac_mdio_set_mdc_freq(struct mii_bus *bus) >> +{ >> + struct mdio_fsl_priv *priv = (struct mdio_fsl_priv *)bus->priv; >> + struct tgec_mdio_controller __iomem *regs = priv->mdio_base; >> + struct device *dev = bus->parent; >> + u32 mdio_stat, div; >> + >> + if (device_property_read_u32(dev, "clock-frequency", &priv->mdc_freq)) >> + return; >> + >> + priv->enet_clk = devm_clk_get(dev, NULL); >> + if (IS_ERR(priv->enet_clk)) { >> + dev_err(dev, "Input clock unknown, not changing MDC frequency"); > > Is the clock optional or mandatory? As the code is now, it is mandatory. I could add some default frequency, but I fear that could do more harm than good? > If mandatory, then i would prefer -ENODEV and fail the probe. > >> + return; >> + } >> + >> + div = ((clk_get_rate(priv->enet_clk) / priv->mdc_freq) - 1) / 2; >> + if (div < 5 || div > 0x1ff) { >> + dev_err(dev, "Requested MDC frequecy is out of range, ignoring"); > > and here return -EINVAL. > > I always think it is best to make it obvious something is broken. One > additional line on the console can be missed for a while. All the > Ethernet devices missing because the PHYs are missing, because the > MDIO bus is missing is going to get noticed very quickly. Darn, the last thing I did was to change this from a hard to a soft error :) Ok, no worries about regressions for deployed stuff already out there?
> >> + if (device_property_read_u32(dev, "clock-frequency", &priv->mdc_freq)) > >> + return; > >> + > >> + priv->enet_clk = devm_clk_get(dev, NULL); > >> + if (IS_ERR(priv->enet_clk)) { > >> + dev_err(dev, "Input clock unknown, not changing MDC frequency"); > > > > Is the clock optional or mandatory? > > As the code is now, it is mandatory. I could add some default frequency, > but I fear that could do more harm than good? As i said in the reply to the DT patch, it is mandatory if the "clock-frequency" parameter is present. > Ok, no worries about regressions for deployed stuff already out there? It would only cause a regression if a DT blob happened to have a 'clock-frequency' parameter and not clock, which it should not. Andrew
diff --git a/drivers/net/ethernet/freescale/xgmac_mdio.c b/drivers/net/ethernet/freescale/xgmac_mdio.c index 18bf2370d45a..2199f8f4ff68 100644 --- a/drivers/net/ethernet/freescale/xgmac_mdio.c +++ b/drivers/net/ethernet/freescale/xgmac_mdio.c @@ -14,6 +14,7 @@ #include <linux/acpi.h> #include <linux/acpi_mdio.h> +#include <linux/clk.h> #include <linux/interrupt.h> #include <linux/kernel.h> #include <linux/mdio.h> @@ -36,7 +37,7 @@ struct tgec_mdio_controller { } __packed; #define MDIO_STAT_ENC BIT(6) -#define MDIO_STAT_CLKDIV(x) (((x>>1) & 0xff) << 8) +#define MDIO_STAT_CLKDIV(x) (((x) & 0x1ff) << 7) #define MDIO_STAT_BSY BIT(0) #define MDIO_STAT_RD_ER BIT(1) #define MDIO_STAT_PRE_DIS BIT(5) @@ -51,6 +52,8 @@ struct tgec_mdio_controller { struct mdio_fsl_priv { struct tgec_mdio_controller __iomem *mdio_base; + struct clk *enet_clk; + u32 mdc_freq; bool is_little_endian; bool has_a009885; bool has_a011043; @@ -255,6 +258,34 @@ static int xgmac_mdio_read(struct mii_bus *bus, int phy_id, int regnum) return ret; } +static void xgmac_mdio_set_mdc_freq(struct mii_bus *bus) +{ + struct mdio_fsl_priv *priv = (struct mdio_fsl_priv *)bus->priv; + struct tgec_mdio_controller __iomem *regs = priv->mdio_base; + struct device *dev = bus->parent; + u32 mdio_stat, div; + + if (device_property_read_u32(dev, "clock-frequency", &priv->mdc_freq)) + return; + + priv->enet_clk = devm_clk_get(dev, NULL); + if (IS_ERR(priv->enet_clk)) { + dev_err(dev, "Input clock unknown, not changing MDC frequency"); + return; + } + + div = ((clk_get_rate(priv->enet_clk) / priv->mdc_freq) - 1) / 2; + if (div < 5 || div > 0x1ff) { + dev_err(dev, "Requested MDC frequecy is out of range, ignoring"); + return; + } + + mdio_stat = xgmac_read32(®s->mdio_stat, priv->is_little_endian); + mdio_stat &= ~MDIO_STAT_CLKDIV(0x1ff); + mdio_stat |= MDIO_STAT_CLKDIV(div); + xgmac_write32(mdio_stat, ®s->mdio_stat, priv->is_little_endian); +} + static void xgmac_mdio_set_suppress_preamble(struct mii_bus *bus) { struct mdio_fsl_priv *priv = (struct mdio_fsl_priv *)bus->priv; @@ -319,6 +350,8 @@ static int xgmac_mdio_probe(struct platform_device *pdev) xgmac_mdio_set_suppress_preamble(bus); + xgmac_mdio_set_mdc_freq(bus); + fwnode = pdev->dev.fwnode; if (is_of_node(fwnode)) ret = of_mdiobus_register(bus, to_of_node(fwnode));
Support the standard "clock-frequency" attribute to set the generated MDC frequency. If not specified, the driver will leave the divisor bits untouched. Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com> --- drivers/net/ethernet/freescale/xgmac_mdio.c | 35 ++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-)