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 <[email protected]>
---
drivers/net/ethernet/freescale/xgmac_mdio.c | 35 ++++++++++++++++++++-
1 file changed, 34 insertions(+), 1 deletion(-)
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));
--
2.25.1
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 <[email protected]> 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