[PATCH net-next 0/2] add interface mode select and RMII

[PATCH net-next 0/2] add interface mode select and RMII

[wei.fang]

From: Wei Fang <wei.fang@nxp.com>

The patches add the below feature support for both TJA1100 and
TJA1101 PHYs cards:
- Add MII and RMII mode support.
- Add REF_CLK input/output support for RMII mode.

Wei Fang (2):
  dt-bindings: net: tja11xx: add nxp,refclk_in property
  net: phy: tja11xx: add interface mode and RMII REF_CLK support

 .../devicetree/bindings/net/nxp,tja11xx.yaml  | 17 ++++
 drivers/net/phy/nxp-tja11xx.c                 | 83 +++++++++++++++++--
 2 files changed, 95 insertions(+), 5 deletions(-)

-- 
2.25.1

[PATCH net-next 1/2] dt-bindings: net: tja11xx: add nxp,refclk_in property

[wei.fang]

From: Wei Fang <wei.fang@nxp.com>

TJA110x REF_CLK can be configured as interface reference clock
intput or output when the RMII mode enabled. This patch add the
property to make the REF_CLK can be configurable.

Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 .../devicetree/bindings/net/nxp,tja11xx.yaml    | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
index d51da24f3505..c51ee52033e8 100644
--- a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
+++ b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
@@ -31,6 +31,22 @@ patternProperties:
         description:
           The ID number for the child PHY. Should be +1 of parent PHY.
 
+      nxp,rmii_refclk_in:
+        type: boolean
+        description: |
+          The REF_CLK is provided for both transmitted and receivced data
+          in RMII mode. This clock signal is provided by the PHY and is
+          typically derived from an external 25MHz crystal. Alternatively,
+          a 50MHz clock signal generated by an external oscillator can be
+          connected to pin REF_CLK. A third option is to connect a 25MHz
+          clock to pin CLK_IN_OUT. So, the REF_CLK should be configured
+          as input or output according to the actual circuit connection.
+          If present, indicates that the REF_CLK will be configured as
+          interface reference clock input when RMII mode enabled.
+          If not present, the REF_CLK will be configured as interface
+          reference clock output when RMII mode enabled.
+          Only supported on TJA1100 and TJA1101.
+
     required:
       - reg
 
@@ -44,6 +60,7 @@ examples:
 
         tja1101_phy0: ethernet-phy@4 {
             reg = <0x4>;
+            nxp,rmii_refclk_in;
         };
     };
   - |
-- 
2.25.1

Re: [PATCH net-next 1/2] dt-bindings: net: tja11xx: add nxp,refclk_in property

[Krzysztof Kozlowski]

On 19/08/2022 10:47, wei.fang@nxp.com wrote:
> From: Wei Fang <wei.fang@nxp.com>
> 
> TJA110x REF_CLK can be configured as interface reference clock
> intput or output when the RMII mode enabled. This patch add the
> property to make the REF_CLK can be configurable.
> 
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> ---
>  .../devicetree/bindings/net/nxp,tja11xx.yaml    | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> index d51da24f3505..c51ee52033e8 100644
> --- a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> +++ b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> @@ -31,6 +31,22 @@ patternProperties:
>          description:
>            The ID number for the child PHY. Should be +1 of parent PHY.
>  
> +      nxp,rmii_refclk_in:

No underscores in properties.

> +        type: boolean
> +        description: |
> +          The REF_CLK is provided for both transmitted and receivced data

typo: received

> +          in RMII mode. This clock signal is provided by the PHY and is
> +          typically derived from an external 25MHz crystal. Alternatively,
> +          a 50MHz clock signal generated by an external oscillator can be
> +          connected to pin REF_CLK. A third option is to connect a 25MHz
> +          clock to pin CLK_IN_OUT. So, the REF_CLK should be configured
> +          as input or output according to the actual circuit connection.
> +          If present, indicates that the REF_CLK will be configured as
> +          interface reference clock input when RMII mode enabled.
> +          If not present, the REF_CLK will be configured as interface
> +          reference clock output when RMII mode enabled.
> +          Only supported on TJA1100 and TJA1101.

Then disallow it on other variants.

Shouldn't this be just "clocks" property?


Best regards,
Krzysztof

RE: [PATCH net-next 1/2] dt-bindings: net: tja11xx: add nxp,refclk_in property

[Wei Fang]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: 2022年8月19日 17:14
> To: Wei Fang <wei.fang@nxp.com>; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; andrew@lunn.ch;
> f.fainelli@gmail.com; hkallweit1@gmail.com; linux@armlinux.org.uk
> Cc: netdev@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH net-next 1/2] dt-bindings: net: tja11xx: add nxp,refclk_in
> property
> 
> On 19/08/2022 10:47, wei.fang@nxp.com wrote:
> > From: Wei Fang <wei.fang@nxp.com>
> >
> > TJA110x REF_CLK can be configured as interface reference clock intput
> > or output when the RMII mode enabled. This patch add the property to
> > make the REF_CLK can be configurable.
> >
> > Signed-off-by: Wei Fang <wei.fang@nxp.com>
> > ---
> >  .../devicetree/bindings/net/nxp,tja11xx.yaml    | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> > b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> > index d51da24f3505..c51ee52033e8 100644
> > --- a/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> > +++ b/Documentation/devicetree/bindings/net/nxp,tja11xx.yaml
> > @@ -31,6 +31,22 @@ patternProperties:
> >          description:
> >            The ID number for the child PHY. Should be +1 of parent PHY.
> >
> > +      nxp,rmii_refclk_in:
> 
> No underscores in properties.
> 
Sorry, It's first time for me to know this.

> > +        type: boolean
> > +        description: |
> > +          The REF_CLK is provided for both transmitted and receivced
> > + data
> 
> typo: received
> 
> > +          in RMII mode. This clock signal is provided by the PHY and is
> > +          typically derived from an external 25MHz crystal. Alternatively,
> > +          a 50MHz clock signal generated by an external oscillator can be
> > +          connected to pin REF_CLK. A third option is to connect a 25MHz
> > +          clock to pin CLK_IN_OUT. So, the REF_CLK should be configured
> > +          as input or output according to the actual circuit connection.
> > +          If present, indicates that the REF_CLK will be configured as
> > +          interface reference clock input when RMII mode enabled.
> > +          If not present, the REF_CLK will be configured as interface
> > +          reference clock output when RMII mode enabled.
> > +          Only supported on TJA1100 and TJA1101.
> 
> Then disallow it on other variants.
> 
> Shouldn't this be just "clocks" property?
> 
> 
This property is to configure the pin REF_CLK of PHY as a input pin through phy register,
indicates that the REF_CLK signal is provided by an external oscillator. so I don't think it's a
"clock" property.

Re: [PATCH net-next 1/2] dt-bindings: net: tja11xx: add nxp,refclk_in property

[Krzysztof Kozlowski]

On 19/08/2022 12:37, Wei Fang wrote:
>>
>>> +          in RMII mode. This clock signal is provided by the PHY and is
>>> +          typically derived from an external 25MHz crystal. Alternatively,
>>> +          a 50MHz clock signal generated by an external oscillator can be
>>> +          connected to pin REF_CLK. A third option is to connect a 25MHz
>>> +          clock to pin CLK_IN_OUT. So, the REF_CLK should be configured
>>> +          as input or output according to the actual circuit connection.
>>> +          If present, indicates that the REF_CLK will be configured as
>>> +          interface reference clock input when RMII mode enabled.
>>> +          If not present, the REF_CLK will be configured as interface
>>> +          reference clock output when RMII mode enabled.
>>> +          Only supported on TJA1100 and TJA1101.
>>
>> Then disallow it on other variants.
>>
>> Shouldn't this be just "clocks" property?
>>
>>
> This property is to configure the pin REF_CLK of PHY as a input pin through phy register,
> indicates that the REF_CLK signal is provided by an external oscillator. so I don't think it's a
> "clock" property.

clocks, not clock.

You just repeated pieces of description as an counter-argument, so this
does not explain anything.

If it is external oscillator shouldn't it be represented in DTS and then
obtained by driver (clk_get + clk_prepare_enable)? Otherwise how are you
sure that clock is actually enabled? And the lack of presence of the
external clock means it is derived from PHY?

Best regards,
Krzysztof

Re: [PATCH net-next 1/2] dt-bindings: net: tja11xx: add nxp,refclk_in property

[Andrew Lunn]

On Fri, Aug 19, 2022 at 02:37:36PM +0300, Krzysztof Kozlowski wrote:
> On 19/08/2022 12:37, Wei Fang wrote:
> >>
> >>> +          in RMII mode. This clock signal is provided by the PHY and is
> >>> +          typically derived from an external 25MHz crystal. Alternatively,
> >>> +          a 50MHz clock signal generated by an external oscillator can be
> >>> +          connected to pin REF_CLK. A third option is to connect a 25MHz
> >>> +          clock to pin CLK_IN_OUT. So, the REF_CLK should be configured
> >>> +          as input or output according to the actual circuit connection.
> >>> +          If present, indicates that the REF_CLK will be configured as
> >>> +          interface reference clock input when RMII mode enabled.
> >>> +          If not present, the REF_CLK will be configured as interface
> >>> +          reference clock output when RMII mode enabled.
> >>> +          Only supported on TJA1100 and TJA1101.
> >>
> >> Then disallow it on other variants.
> >>
> >> Shouldn't this be just "clocks" property?
> >>
> >>
> > This property is to configure the pin REF_CLK of PHY as a input pin through phy register,
> > indicates that the REF_CLK signal is provided by an external oscillator. so I don't think it's a
> > "clock" property.
> 
> clocks, not clock.
> 
> You just repeated pieces of description as an counter-argument, so this
> does not explain anything.
> 
> If it is external oscillator shouldn't it be represented in DTS and then
> obtained by driver (clk_get + clk_prepare_enable)? Otherwise how are you
> sure that clock is actually enabled? And the lack of presence of the
> external clock means it is derived from PHY?

Using the common clock framework has been discussed in the past. But
no PHY actually does this. When the SoC provides the clock, a few PHYs
do make use of the common clock framework as clock consumers to ensure
the clock is ticking.

Plus, as the description says, this pin can be either a clock producer
or a consumer. I don't think the common clock code allows this. It is
also not something you negotiate between the MAC and PHY. The hardware
designer typically decides based on the MAC and PHY actually used. So
this is a fixed hardware property.

     Andrew

Re: [PATCH net-next 1/2] dt-bindings: net: tja11xx: add nxp,refclk_in property

[Krzysztof Kozlowski]

On 19/08/2022 15:44, Andrew Lunn wrote:
> On Fri, Aug 19, 2022 at 02:37:36PM +0300, Krzysztof Kozlowski wrote:
>> On 19/08/2022 12:37, Wei Fang wrote:
>>>>
>>>>> +          in RMII mode. This clock signal is provided by the PHY and is
>>>>> +          typically derived from an external 25MHz crystal. Alternatively,
>>>>> +          a 50MHz clock signal generated by an external oscillator can be
>>>>> +          connected to pin REF_CLK. A third option is to connect a 25MHz
>>>>> +          clock to pin CLK_IN_OUT. So, the REF_CLK should be configured
>>>>> +          as input or output according to the actual circuit connection.
>>>>> +          If present, indicates that the REF_CLK will be configured as
>>>>> +          interface reference clock input when RMII mode enabled.
>>>>> +          If not present, the REF_CLK will be configured as interface
>>>>> +          reference clock output when RMII mode enabled.
>>>>> +          Only supported on TJA1100 and TJA1101.
>>>>
>>>> Then disallow it on other variants.
>>>>
>>>> Shouldn't this be just "clocks" property?
>>>>
>>>>
>>> This property is to configure the pin REF_CLK of PHY as a input pin through phy register,
>>> indicates that the REF_CLK signal is provided by an external oscillator. so I don't think it's a
>>> "clock" property.
>>
>> clocks, not clock.
>>
>> You just repeated pieces of description as an counter-argument, so this
>> does not explain anything.
>>
>> If it is external oscillator shouldn't it be represented in DTS and then
>> obtained by driver (clk_get + clk_prepare_enable)? Otherwise how are you
>> sure that clock is actually enabled? And the lack of presence of the
>> external clock means it is derived from PHY?
> 
> Using the common clock framework has been discussed in the past. But
> no PHY actually does this. When the SoC provides the clock, a few PHYs
> do make use of the common clock framework as clock consumers to ensure
> the clock is ticking.

IOW, all DTSes would have a fixed clock stub without any logic usable by
Common CF (like enabling)?

> Plus, as the description says, this pin can be either a clock producer
> or a consumer. I don't think the common clock code allows this. It is
> also not something you negotiate between the MAC and PHY. The hardware
> designer typically decides based on the MAC and PHY actually used. So
> this is a fixed hardware property.

Indeed.

Anyway the property name and typo need fixes.
Best regards,
Krzysztof

[PATCH net-next 2/2] net: phy: tja11xx: add interface mode and RMII REF_CLK support

[wei.fang]

From: Wei Fang <wei.fang@nxp.com>

Add below features support for both TJA1100 and TJA1101 cards:
- Add MII and RMII mode support.
- Add REF_CLK input/output support for RMII mode.

Signed-off-by: Wei Fang <wei.fang@nxp.com>
---
 drivers/net/phy/nxp-tja11xx.c | 83 ++++++++++++++++++++++++++++++++---
 1 file changed, 78 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
index 2a8195c50d14..487c881d20b1 100644
--- a/drivers/net/phy/nxp-tja11xx.c
+++ b/drivers/net/phy/nxp-tja11xx.c
@@ -10,6 +10,7 @@
 #include <linux/mdio.h>
 #include <linux/mii.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/phy.h>
 #include <linux/hwmon.h>
 #include <linux/bitfield.h>
@@ -34,6 +35,11 @@
 #define MII_CFG1			18
 #define MII_CFG1_MASTER_SLAVE		BIT(15)
 #define MII_CFG1_AUTO_OP		BIT(14)
+#define MII_CFG1_INTERFACE_MODE_MASK	GENMASK(9, 8)
+#define MII_CFG1_MII_MODE				(0x0 << 8)
+#define MII_CFG1_RMII_MODE_REFCLK_IN	BIT(8)
+#define MII_CFG1_RMII_MODE_REFCLK_OUT	BIT(9)
+#define MII_CFG1_REVMII_MODE			GENMASK(9, 8)
 #define MII_CFG1_SLEEP_CONFIRM		BIT(6)
 #define MII_CFG1_LED_MODE_MASK		GENMASK(5, 4)
 #define MII_CFG1_LED_MODE_LINKUP	0
@@ -72,11 +78,15 @@
 #define MII_COMMCFG			27
 #define MII_COMMCFG_AUTO_OP		BIT(15)
 
+/* Configure REF_CLK as input in RMII mode */
+#define TJA110X_RMII_MODE_REFCLK_IN       BIT(0)
+
 struct tja11xx_priv {
 	char		*hwmon_name;
 	struct device	*hwmon_dev;
 	struct phy_device *phydev;
 	struct work_struct phy_register_work;
+	u32 quirks;
 };
 
 struct tja11xx_phy_stats {
@@ -251,8 +261,34 @@ static int tja11xx_config_aneg(struct phy_device *phydev)
 	return __genphy_config_aneg(phydev, changed);
 }
 
+static int tja11xx_get_interface_mode(struct phy_device *phydev)
+{
+	struct tja11xx_priv *priv = phydev->priv;
+	int mii_mode;
+
+	switch (phydev->interface) {
+	case PHY_INTERFACE_MODE_MII:
+		mii_mode = MII_CFG1_MII_MODE;
+		break;
+	case PHY_INTERFACE_MODE_REVMII:
+		mii_mode = MII_CFG1_REVMII_MODE;
+		break;
+	case PHY_INTERFACE_MODE_RMII:
+		if (priv->quirks & TJA110X_RMII_MODE_REFCLK_IN)
+			mii_mode = MII_CFG1_RMII_MODE_REFCLK_IN;
+		else
+			mii_mode = MII_CFG1_RMII_MODE_REFCLK_OUT;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return mii_mode;
+}
+
 static int tja11xx_config_init(struct phy_device *phydev)
 {
+	u16 reg_mask, reg_val;
 	int ret;
 
 	ret = tja11xx_enable_reg_write(phydev);
@@ -265,15 +301,32 @@ static int tja11xx_config_init(struct phy_device *phydev)
 
 	switch (phydev->phy_id & PHY_ID_MASK) {
 	case PHY_ID_TJA1100:
-		ret = phy_modify(phydev, MII_CFG1,
-				 MII_CFG1_AUTO_OP | MII_CFG1_LED_MODE_MASK |
-				 MII_CFG1_LED_ENABLE,
-				 MII_CFG1_AUTO_OP | MII_CFG1_LED_MODE_LINKUP |
-				 MII_CFG1_LED_ENABLE);
+		reg_mask = MII_CFG1_AUTO_OP | MII_CFG1_LED_MODE_MASK |
+			   MII_CFG1_LED_ENABLE;
+		reg_val = MII_CFG1_AUTO_OP | MII_CFG1_LED_MODE_LINKUP |
+			  MII_CFG1_LED_ENABLE;
+
+		reg_mask |= MII_CFG1_INTERFACE_MODE_MASK;
+		ret = tja11xx_get_interface_mode(phydev);
+		if (ret < 0)
+			return ret;
+
+		reg_val |= (ret & 0xffff);
+		ret = phy_modify(phydev, MII_CFG1, reg_mask, reg_val);
 		if (ret)
 			return ret;
 		break;
 	case PHY_ID_TJA1101:
+		reg_mask = MII_CFG1_INTERFACE_MODE_MASK;
+		ret = tja11xx_get_interface_mode(phydev);
+		if (ret < 0)
+			return ret;
+
+		reg_val = ret & 0xffff;
+		ret = phy_modify(phydev, MII_CFG1, reg_mask, reg_val);
+		if (ret)
+			return ret;
+		fallthrough;
 	case PHY_ID_TJA1102:
 		ret = phy_set_bits(phydev, MII_COMMCFG, MII_COMMCFG_AUTO_OP);
 		if (ret)
@@ -458,16 +511,36 @@ static int tja11xx_hwmon_register(struct phy_device *phydev,
 	return PTR_ERR_OR_ZERO(priv->hwmon_dev);
 }
 
+static int tja11xx_parse_dt(struct phy_device *phydev)
+{
+	struct device_node *node = phydev->mdio.dev.of_node;
+	struct tja11xx_priv *priv = phydev->priv;
+
+	if (!IS_ENABLED(CONFIG_OF_MDIO))
+		return 0;
+
+	if (of_property_read_bool(node, "nxp,rmii_refclk_in"))
+		priv->quirks |= TJA110X_RMII_MODE_REFCLK_IN;
+
+	return 0;
+}
+
 static int tja11xx_probe(struct phy_device *phydev)
 {
 	struct device *dev = &phydev->mdio.dev;
 	struct tja11xx_priv *priv;
+	int ret;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
 	priv->phydev = phydev;
+	phydev->priv = priv;
+
+	ret = tja11xx_parse_dt(phydev);
+	if (ret)
+		return ret;
 
 	return tja11xx_hwmon_register(phydev, priv);
 }
-- 
2.25.1

Re: [PATCH net-next 2/2] net: phy: tja11xx: add interface mode and RMII REF_CLK support

[Andrew Lunn]

> +/* Configure REF_CLK as input in RMII mode */
> +#define TJA110X_RMII_MODE_REFCLK_IN       BIT(0)
> +
>  struct tja11xx_priv {
>  	char		*hwmon_name;
>  	struct device	*hwmon_dev;
>  	struct phy_device *phydev;
>  	struct work_struct phy_register_work;
> +	u32 quirks;

A quirk is generally a workaround for a bug. Configuring a clock is
not a quirk. I would rename this flags.

    Andrew

RE: [PATCH net-next 2/2] net: phy: tja11xx: add interface mode and RMII REF_CLK support

[Wei Fang]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: 2022年8月19日 20:51
> To: Wei Fang <wei.fang@nxp.com>
> Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
> f.fainelli@gmail.com; hkallweit1@gmail.com; linux@armlinux.org.uk;
> netdev@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH net-next 2/2] net: phy: tja11xx: add interface mode and
> RMII REF_CLK support
> 
> > +/* Configure REF_CLK as input in RMII mode */
> > +#define TJA110X_RMII_MODE_REFCLK_IN       BIT(0)
> > +
> >  struct tja11xx_priv {
> >  	char		*hwmon_name;
> >  	struct device	*hwmon_dev;
> >  	struct phy_device *phydev;
> >  	struct work_struct phy_register_work;
> > +	u32 quirks;
> 
> A quirk is generally a workaround for a bug. Configuring a clock is not a quirk. I
> would rename this flags.
> 
Thanks, I'll rename it.