[PATCH 1/2] net: phy: micrel.c: add rgmii interface delay possibility to ksz9131

mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed

* [PATCH 1/2] net: phy: micrel.c: add rgmii interface delay possibility to ksz9131
@ 2020-03-25 15:03 Philippe Schenker
  2020-03-25 15:03 ` [PATCH 2/2] ARM: dts: apalis-imx6qdl: use rgmii-id instead of rgmii Philippe Schenker
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Philippe Schenker @ 2020-03-25 15:03 UTC (permalink / raw)
  To: andrew, f.fainelli, hkallweit1, linux, netdev, robh+dt,
	devicetree, shawnguo, mark.rutland
  Cc: o.rempel, linux-kernel, silvan.murer, s.hauer, a.fatoum,
	Philippe Schenker, David S. Miller

The KSZ9131 provides DLL controlled delays on RXC and TXC lines. This
patch makes use of those delays. The information which delays should
be enabled or disabled comes from the interface names, documented in
ethernet-controller.yaml:

rgmii:      Disable RXC and TXC delays
rgmii-id:   Enable RXC and TXC delays
rgmii-txid: Enable only TXC delay, disable RXC delay
rgmii-rxid: Enable onlx RXC delay, disable TXC delay

Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
---

 drivers/net/phy/micrel.c | 45 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 63dedec0433d..d3ad09774847 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -704,6 +704,48 @@ static int ksz9131_of_load_skew_values(struct phy_device *phydev,
 	return phy_write_mmd(phydev, 2, reg, newval);
 }
 
+/* MMD Address 0x2 */
+#define KSZ9131RN_RXC_DLL_CTRL		76
+#define KSZ9131RN_TXC_DLL_CTRL		77
+#define KSZ9131RN_DLL_CTRL_BYPASS	BIT_MASK(12)
+#define KSZ9131RN_DLL_ENABLE_DELAY	0
+#define KSZ9131RN_DLL_DISABLE_DELAY	BIT(12)
+
+static int ksz9131_config_rgmii_delay(struct phy_device *phydev)
+{
+	int ret;
+	u16 rxcdll_val, txcdll_val;
+
+	switch (phydev->interface) {
+	case PHY_INTERFACE_MODE_RGMII:
+		rxcdll_val = KSZ9131RN_DLL_DISABLE_DELAY;
+		txcdll_val = KSZ9131RN_DLL_DISABLE_DELAY;
+		break;
+	case PHY_INTERFACE_MODE_RGMII_ID:
+		rxcdll_val = KSZ9131RN_DLL_ENABLE_DELAY;
+		txcdll_val = KSZ9131RN_DLL_ENABLE_DELAY;
+		break;
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+		rxcdll_val = KSZ9131RN_DLL_ENABLE_DELAY;
+		txcdll_val = KSZ9131RN_DLL_DISABLE_DELAY;
+		break;
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+		rxcdll_val = KSZ9131RN_DLL_DISABLE_DELAY;
+		txcdll_val = KSZ9131RN_DLL_ENABLE_DELAY;
+		break;
+	default:
+		return 0;
+	}
+
+	ret = phy_modify_mmd_changed(phydev, 2, KSZ9131RN_RXC_DLL_CTRL,
+				     KSZ9131RN_DLL_CTRL_BYPASS, rxcdll_val);
+	if (ret < 0)
+		return ret;
+
+	return phy_modify_mmd_changed(phydev, 2, KSZ9131RN_TXC_DLL_CTRL,
+				     KSZ9131RN_DLL_CTRL_BYPASS, txcdll_val);
+}
+
 static int ksz9131_config_init(struct phy_device *phydev)
 {
 	const struct device *dev = &phydev->mdio.dev;
@@ -730,6 +772,9 @@ static int ksz9131_config_init(struct phy_device *phydev)
 	if (!of_node)
 		return 0;
 
+	if (phy_interface_is_rgmii(phydev))
+		ksz9131_config_rgmii_delay(phydev);
+
 	ret = ksz9131_of_load_skew_values(phydev, of_node,
 					  MII_KSZ9031RN_CLK_PAD_SKEW, 5,
 					  clk_skews, 2);
-- 
2.26.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 2/2] ARM: dts: apalis-imx6qdl: use rgmii-id instead of rgmii
  2020-03-25 15:03 [PATCH 1/2] net: phy: micrel.c: add rgmii interface delay possibility to ksz9131 Philippe Schenker
@ 2020-03-25 15:03 ` Philippe Schenker
  2020-03-25 15:24 ` [PATCH 1/2] net: phy: micrel.c: add rgmii interface delay possibility to ksz9131 Heiner Kallweit
  2020-03-25 15:28 ` Heiner Kallweit
  2 siblings, 0 replies; 5+ messages in thread
From: Philippe Schenker @ 2020-03-25 15:03 UTC (permalink / raw)
  To: andrew, f.fainelli, hkallweit1, linux, netdev, robh+dt,
	devicetree, shawnguo, mark.rutland
  Cc: o.rempel, linux-kernel, silvan.murer, s.hauer, a.fatoum,
	Philippe Schenker, Fabio Estevam, NXP Linux Team,
	Pengutronix Kernel Team, linux-arm-kernel

Until now a PHY-fixup in mach-imx set our rgmii timing correctly. For
the PHY KSZ9131 there is no PHY-fixup in mach-imx. To support this PHY
too, use rgmii-id.
For the now used KSZ9031 nothing will change, as rgmii-id is only
implemented and supported by the KSZ9131.

Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>

---

 arch/arm/boot/dts/imx6qdl-apalis.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/imx6qdl-apalis.dtsi b/arch/arm/boot/dts/imx6qdl-apalis.dtsi
index 1b5bc6b5e806..347a5edc6927 100644
--- a/arch/arm/boot/dts/imx6qdl-apalis.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-apalis.dtsi
@@ -180,7 +180,7 @@ &ecspi2 {
 &fec {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_enet>;
-	phy-mode = "rgmii";
+	phy-mode = "rgmii-id";
 	phy-handle = <&ethphy>;
 	phy-reset-duration = <10>;
 	phy-reset-gpios = <&gpio1 25 GPIO_ACTIVE_LOW>;
-- 
2.26.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2] net: phy: micrel.c: add rgmii interface delay possibility to ksz9131
  2020-03-25 15:03 [PATCH 1/2] net: phy: micrel.c: add rgmii interface delay possibility to ksz9131 Philippe Schenker
  2020-03-25 15:03 ` [PATCH 2/2] ARM: dts: apalis-imx6qdl: use rgmii-id instead of rgmii Philippe Schenker
@ 2020-03-25 15:24 ` Heiner Kallweit
  2020-03-25 15:47   ` Philippe Schenker
  2020-03-25 15:28 ` Heiner Kallweit
  2 siblings, 1 reply; 5+ messages in thread
From: Heiner Kallweit @ 2020-03-25 15:24 UTC (permalink / raw)
  To: Philippe Schenker, andrew, f.fainelli, linux, netdev, robh+dt,
	devicetree, shawnguo, mark.rutland
  Cc: o.rempel, linux-kernel, silvan.murer, s.hauer, a.fatoum, David S. Miller

On 25.03.2020 16:03, Philippe Schenker wrote:
> The KSZ9131 provides DLL controlled delays on RXC and TXC lines. This
> patch makes use of those delays. The information which delays should
> be enabled or disabled comes from the interface names, documented in
> ethernet-controller.yaml:
> 
> rgmii:      Disable RXC and TXC delays
> rgmii-id:   Enable RXC and TXC delays
> rgmii-txid: Enable only TXC delay, disable RXC delay
> rgmii-rxid: Enable onlx RXC delay, disable TXC delay
> 
> Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
> ---
> 
>  drivers/net/phy/micrel.c | 45 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index 63dedec0433d..d3ad09774847 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -704,6 +704,48 @@ static int ksz9131_of_load_skew_values(struct phy_device *phydev,
>  	return phy_write_mmd(phydev, 2, reg, newval);
>  }
>  
> +/* MMD Address 0x2 */
> +#define KSZ9131RN_RXC_DLL_CTRL		76
> +#define KSZ9131RN_TXC_DLL_CTRL		77
> +#define KSZ9131RN_DLL_CTRL_BYPASS	BIT_MASK(12)
> +#define KSZ9131RN_DLL_ENABLE_DELAY	0
> +#define KSZ9131RN_DLL_DISABLE_DELAY	BIT(12)
> +
> +static int ksz9131_config_rgmii_delay(struct phy_device *phydev)
> +{
> +	int ret;
> +	u16 rxcdll_val, txcdll_val;
> +

Reverse xmas tree order please.

> +	switch (phydev->interface) {
> +	case PHY_INTERFACE_MODE_RGMII:
> +		rxcdll_val = KSZ9131RN_DLL_DISABLE_DELAY;
> +		txcdll_val = KSZ9131RN_DLL_DISABLE_DELAY;
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_ID:
> +		rxcdll_val = KSZ9131RN_DLL_ENABLE_DELAY;
> +		txcdll_val = KSZ9131RN_DLL_ENABLE_DELAY;
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +		rxcdll_val = KSZ9131RN_DLL_ENABLE_DELAY;
> +		txcdll_val = KSZ9131RN_DLL_DISABLE_DELAY;
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_TXID:
> +		rxcdll_val = KSZ9131RN_DLL_DISABLE_DELAY;
> +		txcdll_val = KSZ9131RN_DLL_ENABLE_DELAY;
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	ret = phy_modify_mmd_changed(phydev, 2, KSZ9131RN_RXC_DLL_CTRL,
> +				     KSZ9131RN_DLL_CTRL_BYPASS, rxcdll_val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return phy_modify_mmd_changed(phydev, 2, KSZ9131RN_TXC_DLL_CTRL,
> +				     KSZ9131RN_DLL_CTRL_BYPASS, txcdll_val);

phy_modify_mmd_changed() returns 1 if the register value was changed,
and that's not what you want here. Simply use phy_modify_mmd() in both
occurrences. And your function has a return value, but it's not used by
the caller.

> +}
> +
>  static int ksz9131_config_init(struct phy_device *phydev)
>  {
>  	const struct device *dev = &phydev->mdio.dev;
> @@ -730,6 +772,9 @@ static int ksz9131_config_init(struct phy_device *phydev)
>  	if (!of_node)
>  		return 0;
>  
> +	if (phy_interface_is_rgmii(phydev))
> +		ksz9131_config_rgmii_delay(phydev);
> +
>  	ret = ksz9131_of_load_skew_values(phydev, of_node,
>  					  MII_KSZ9031RN_CLK_PAD_SKEW, 5,
>  					  clk_skews, 2);
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2] net: phy: micrel.c: add rgmii interface delay possibility to ksz9131
  2020-03-25 15:24 ` [PATCH 1/2] net: phy: micrel.c: add rgmii interface delay possibility to ksz9131 Heiner Kallweit
@ 2020-03-25 15:47   ` Philippe Schenker
  0 siblings, 0 replies; 5+ messages in thread
From: Philippe Schenker @ 2020-03-25 15:47 UTC (permalink / raw)
  To: andrew, linux, hkallweit1, devicetree, mark.rutland, shawnguo,
	netdev, f.fainelli, robh+dt
  Cc: o.rempel, s.hauer, linux-kernel, a.fatoum, silvan.murer, davem

On Wed, 2020-03-25 at 16:24 +0100, Heiner Kallweit wrote:
> On 25.03.2020 16:03, Philippe Schenker wrote:
> > The KSZ9131 provides DLL controlled delays on RXC and TXC lines.
> > This
> > patch makes use of those delays. The information which delays should
> > be enabled or disabled comes from the interface names, documented in
> > ethernet-controller.yaml:
> > 
> > rgmii:      Disable RXC and TXC delays
> > rgmii-id:   Enable RXC and TXC delays
> > rgmii-txid: Enable only TXC delay, disable RXC delay
> > rgmii-rxid: Enable onlx RXC delay, disable TXC delay
> > 
> > Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
> > ---
> > 
> >  drivers/net/phy/micrel.c | 45
> > ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 45 insertions(+)
> > 
> > diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> > index 63dedec0433d..d3ad09774847 100644
> > --- a/drivers/net/phy/micrel.c
> > +++ b/drivers/net/phy/micrel.c
> > @@ -704,6 +704,48 @@ static int ksz9131_of_load_skew_values(struct
> > phy_device *phydev,
> >  	return phy_write_mmd(phydev, 2, reg, newval);
> >  }
> >  
> > +/* MMD Address 0x2 */
> > +#define KSZ9131RN_RXC_DLL_CTRL		76
> > +#define KSZ9131RN_TXC_DLL_CTRL		77
> > +#define KSZ9131RN_DLL_CTRL_BYPASS	BIT_MASK(12)
> > +#define KSZ9131RN_DLL_ENABLE_DELAY	0
> > +#define KSZ9131RN_DLL_DISABLE_DELAY	BIT(12)
> > +
> > +static int ksz9131_config_rgmii_delay(struct phy_device *phydev)
> > +{
> > +	int ret;
> > +	u16 rxcdll_val, txcdll_val;
> > +
> 
> Reverse xmas tree order please.
> 
> > +	switch (phydev->interface) {
> > +	case PHY_INTERFACE_MODE_RGMII:
> > +		rxcdll_val = KSZ9131RN_DLL_DISABLE_DELAY;
> > +		txcdll_val = KSZ9131RN_DLL_DISABLE_DELAY;
> > +		break;
> > +	case PHY_INTERFACE_MODE_RGMII_ID:
> > +		rxcdll_val = KSZ9131RN_DLL_ENABLE_DELAY;
> > +		txcdll_val = KSZ9131RN_DLL_ENABLE_DELAY;
> > +		break;
> > +	case PHY_INTERFACE_MODE_RGMII_RXID:
> > +		rxcdll_val = KSZ9131RN_DLL_ENABLE_DELAY;
> > +		txcdll_val = KSZ9131RN_DLL_DISABLE_DELAY;
> > +		break;
> > +	case PHY_INTERFACE_MODE_RGMII_TXID:
> > +		rxcdll_val = KSZ9131RN_DLL_DISABLE_DELAY;
> > +		txcdll_val = KSZ9131RN_DLL_ENABLE_DELAY;
> > +		break;
> > +	default:
> > +		return 0;
> > +	}
> > +
> > +	ret = phy_modify_mmd_changed(phydev, 2, KSZ9131RN_RXC_DLL_CTRL,
> > +				     KSZ9131RN_DLL_CTRL_BYPASS,
> > rxcdll_val);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return phy_modify_mmd_changed(phydev, 2, KSZ9131RN_TXC_DLL_CTRL,
> > +				     KSZ9131RN_DLL_CTRL_BYPASS,
> > txcdll_val);
> 
> phy_modify_mmd_changed() returns 1 if the register value was changed,
> and that's not what you want here. Simply use phy_modify_mmd() in both
> occurrences. And your function has a return value, but it's not used
> by
> the caller.

Thanks for your feedback, I'll send a v2 with those fixes.
> 
> > +}
> > +
> >  static int ksz9131_config_init(struct phy_device *phydev)
> >  {
> >  	const struct device *dev = &phydev->mdio.dev;
> > @@ -730,6 +772,9 @@ static int ksz9131_config_init(struct phy_device
> > *phydev)
> >  	if (!of_node)
> >  		return 0;
> >  
> > +	if (phy_interface_is_rgmii(phydev))
> > +		ksz9131_config_rgmii_delay(phydev);
> > +
> >  	ret = ksz9131_of_load_skew_values(phydev, of_node,
> >  					  MII_KSZ9031RN_CLK_PAD_SKEW, 5,
> >  					  clk_skews, 2);
> > 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2] net: phy: micrel.c: add rgmii interface delay possibility to ksz9131
  2020-03-25 15:03 [PATCH 1/2] net: phy: micrel.c: add rgmii interface delay possibility to ksz9131 Philippe Schenker
  2020-03-25 15:03 ` [PATCH 2/2] ARM: dts: apalis-imx6qdl: use rgmii-id instead of rgmii Philippe Schenker
  2020-03-25 15:24 ` [PATCH 1/2] net: phy: micrel.c: add rgmii interface delay possibility to ksz9131 Heiner Kallweit
@ 2020-03-25 15:28 ` Heiner Kallweit
  2 siblings, 0 replies; 5+ messages in thread
From: Heiner Kallweit @ 2020-03-25 15:28 UTC (permalink / raw)
  To: Philippe Schenker, andrew, f.fainelli, linux, netdev, robh+dt,
	devicetree, shawnguo, mark.rutland
  Cc: o.rempel, linux-kernel, silvan.murer, s.hauer, a.fatoum, David S. Miller

On 25.03.2020 16:03, Philippe Schenker wrote:
> The KSZ9131 provides DLL controlled delays on RXC and TXC lines. This
> patch makes use of those delays. The information which delays should
> be enabled or disabled comes from the interface names, documented in
> ethernet-controller.yaml:
> 
> rgmii:      Disable RXC and TXC delays
> rgmii-id:   Enable RXC and TXC delays
> rgmii-txid: Enable only TXC delay, disable RXC delay
> rgmii-rxid: Enable onlx RXC delay, disable TXC delay
> 
> Signed-off-by: Philippe Schenker <philippe.schenker@toradex.com>
> ---
> 
>  drivers/net/phy/micrel.c | 45 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index 63dedec0433d..d3ad09774847 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -704,6 +704,48 @@ static int ksz9131_of_load_skew_values(struct phy_device *phydev,
>  	return phy_write_mmd(phydev, 2, reg, newval);
>  }
>  
> +/* MMD Address 0x2 */
> +#define KSZ9131RN_RXC_DLL_CTRL		76
> +#define KSZ9131RN_TXC_DLL_CTRL		77
> +#define KSZ9131RN_DLL_CTRL_BYPASS	BIT_MASK(12)
> +#define KSZ9131RN_DLL_ENABLE_DELAY	0
> +#define KSZ9131RN_DLL_DISABLE_DELAY	BIT(12)
> +
> +static int ksz9131_config_rgmii_delay(struct phy_device *phydev)
> +{
> +	int ret;
> +	u16 rxcdll_val, txcdll_val;
> +
> +	switch (phydev->interface) {
> +	case PHY_INTERFACE_MODE_RGMII:
> +		rxcdll_val = KSZ9131RN_DLL_DISABLE_DELAY;
> +		txcdll_val = KSZ9131RN_DLL_DISABLE_DELAY;
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_ID:
> +		rxcdll_val = KSZ9131RN_DLL_ENABLE_DELAY;
> +		txcdll_val = KSZ9131RN_DLL_ENABLE_DELAY;
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +		rxcdll_val = KSZ9131RN_DLL_ENABLE_DELAY;
> +		txcdll_val = KSZ9131RN_DLL_DISABLE_DELAY;
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII_TXID:
> +		rxcdll_val = KSZ9131RN_DLL_DISABLE_DELAY;
> +		txcdll_val = KSZ9131RN_DLL_ENABLE_DELAY;
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	ret = phy_modify_mmd_changed(phydev, 2, KSZ9131RN_RXC_DLL_CTRL,

Using a constant for the device address would be good.

Last but not least your patch misses the net-next annotation
(except you consider it a fix, then the net annotation and
a Fixes tag would be needed).

> +				     KSZ9131RN_DLL_CTRL_BYPASS, rxcdll_val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return phy_modify_mmd_changed(phydev, 2, KSZ9131RN_TXC_DLL_CTRL,
> +				     KSZ9131RN_DLL_CTRL_BYPASS, txcdll_val);
> +}
> +
>  static int ksz9131_config_init(struct phy_device *phydev)
>  {
>  	const struct device *dev = &phydev->mdio.dev;
> @@ -730,6 +772,9 @@ static int ksz9131_config_init(struct phy_device *phydev)
>  	if (!of_node)
>  		return 0;
>  
> +	if (phy_interface_is_rgmii(phydev))
> +		ksz9131_config_rgmii_delay(phydev);
> +
>  	ret = ksz9131_of_load_skew_values(phydev, of_node,
>  					  MII_KSZ9031RN_CLK_PAD_SKEW, 5,
>  					  clk_skews, 2);
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-03-25 15:47 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-25 15:03 [PATCH 1/2] net: phy: micrel.c: add rgmii interface delay possibility to ksz9131 Philippe Schenker
2020-03-25 15:03 ` [PATCH 2/2] ARM: dts: apalis-imx6qdl: use rgmii-id instead of rgmii Philippe Schenker
2020-03-25 15:24 ` [PATCH 1/2] net: phy: micrel.c: add rgmii interface delay possibility to ksz9131 Heiner Kallweit
2020-03-25 15:47   ` Philippe Schenker
2020-03-25 15:28 ` Heiner Kallweit

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

Powered by JetHome