linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1] net: phy: tja11xx: add TJA1102 support
@ 2020-03-03  7:37 Oleksij Rempel
  2020-03-03  8:46 ` Heiner Kallweit
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Oleksij Rempel @ 2020-03-03  7:37 UTC (permalink / raw)
  To: Andrew Lunn, Florian Fainelli, Heiner Kallweit
  Cc: Oleksij Rempel, Pengutronix Kernel Team, linux-kernel,
	David S. Miller, netdev, Marek Vasut, David Jander

TJA1102 is an dual T1 PHY chip. Both PHYs are separately addressable.
PHY 0 can be identified by PHY ID. PHY 1 has no PHY ID and can be
configured in device tree by setting compatible =
"ethernet-phy-id0180.dc81".

PHY 1 has less suported registers and functionality. For current driver
it will affect only the HWMON support.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/nxp-tja11xx.c | 43 +++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
index b705d0bd798b..52090cfaa54e 100644
--- a/drivers/net/phy/nxp-tja11xx.c
+++ b/drivers/net/phy/nxp-tja11xx.c
@@ -15,6 +15,7 @@
 #define PHY_ID_MASK			0xfffffff0
 #define PHY_ID_TJA1100			0x0180dc40
 #define PHY_ID_TJA1101			0x0180dd00
+#define PHY_ID_TJA1102			0x0180dc80
 
 #define MII_ECTRL			17
 #define MII_ECTRL_LINK_CONTROL		BIT(15)
@@ -190,6 +191,7 @@ static int tja11xx_config_init(struct phy_device *phydev)
 			return ret;
 		break;
 	case PHY_ID_TJA1101:
+	case PHY_ID_TJA1102:
 		ret = phy_set_bits(phydev, MII_COMMCFG, MII_COMMCFG_AUTO_OP);
 		if (ret)
 			return ret;
@@ -337,6 +339,31 @@ static int tja11xx_probe(struct phy_device *phydev)
 	if (!priv)
 		return -ENOMEM;
 
+	/* Use the phyid to distinguish between port 0 and port 1 of the
+	 * TJA1102. Port 0 has a proper phyid, while port 1 reads 0.
+	 */
+	if ((phydev->phy_id & PHY_ID_MASK) == PHY_ID_TJA1102) {
+		int ret;
+		u32 id;
+
+		ret = phy_read(phydev, MII_PHYSID1);
+		if (ret < 0)
+			return ret;
+
+		id = ret;
+		ret = phy_read(phydev, MII_PHYSID2);
+		if (ret < 0)
+			return ret;
+
+		id |= ret << 16;
+
+		/* TJA1102 Port 1 has phyid 0 and doesn't support temperature
+		 * and undervoltage alarms.
+		 */
+		if (id == 0)
+			return 0;
+	}
+
 	priv->hwmon_name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
 	if (!priv->hwmon_name)
 		return -ENOMEM;
@@ -385,6 +412,21 @@ static struct phy_driver tja11xx_driver[] = {
 		.get_sset_count = tja11xx_get_sset_count,
 		.get_strings	= tja11xx_get_strings,
 		.get_stats	= tja11xx_get_stats,
+	}, {
+		PHY_ID_MATCH_MODEL(PHY_ID_TJA1102),
+		.name		= "NXP TJA1102",
+		.features       = PHY_BASIC_T1_FEATURES,
+		.probe		= tja11xx_probe,
+		.soft_reset	= tja11xx_soft_reset,
+		.config_init	= tja11xx_config_init,
+		.read_status	= tja11xx_read_status,
+		.suspend	= genphy_suspend,
+		.resume		= genphy_resume,
+		.set_loopback   = genphy_loopback,
+		/* Statistics */
+		.get_sset_count = tja11xx_get_sset_count,
+		.get_strings	= tja11xx_get_strings,
+		.get_stats	= tja11xx_get_stats,
 	}
 };
 
@@ -393,6 +435,7 @@ module_phy_driver(tja11xx_driver);
 static struct mdio_device_id __maybe_unused tja11xx_tbl[] = {
 	{ PHY_ID_MATCH_MODEL(PHY_ID_TJA1100) },
 	{ PHY_ID_MATCH_MODEL(PHY_ID_TJA1101) },
+	{ PHY_ID_MATCH_MODEL(PHY_ID_TJA1102) },
 	{ }
 };
 
-- 
2.25.0


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

* Re: [PATCH v1] net: phy: tja11xx: add TJA1102 support
  2020-03-03  7:37 [PATCH v1] net: phy: tja11xx: add TJA1102 support Oleksij Rempel
@ 2020-03-03  8:46 ` Heiner Kallweit
  2020-03-03  8:56   ` Marc Kleine-Budde
  2020-03-03 12:18 ` Marek Vasut
  2020-03-03 13:59 ` Andrew Lunn
  2 siblings, 1 reply; 12+ messages in thread
From: Heiner Kallweit @ 2020-03-03  8:46 UTC (permalink / raw)
  To: Oleksij Rempel, Andrew Lunn, Florian Fainelli
  Cc: Pengutronix Kernel Team, linux-kernel, David S. Miller, netdev,
	Marek Vasut, David Jander

On 03.03.2020 08:37, Oleksij Rempel wrote:
> TJA1102 is an dual T1 PHY chip. Both PHYs are separately addressable.
> PHY 0 can be identified by PHY ID. PHY 1 has no PHY ID and can be
> configured in device tree by setting compatible =
> "ethernet-phy-id0180.dc81".
> 
> PHY 1 has less suported registers and functionality. For current driver
> it will affect only the HWMON support.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/phy/nxp-tja11xx.c | 43 +++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
> index b705d0bd798b..52090cfaa54e 100644
> --- a/drivers/net/phy/nxp-tja11xx.c
> +++ b/drivers/net/phy/nxp-tja11xx.c
> @@ -15,6 +15,7 @@
>  #define PHY_ID_MASK			0xfffffff0
>  #define PHY_ID_TJA1100			0x0180dc40
>  #define PHY_ID_TJA1101			0x0180dd00
> +#define PHY_ID_TJA1102			0x0180dc80
>  
>  #define MII_ECTRL			17
>  #define MII_ECTRL_LINK_CONTROL		BIT(15)
> @@ -190,6 +191,7 @@ static int tja11xx_config_init(struct phy_device *phydev)
>  			return ret;
>  		break;
>  	case PHY_ID_TJA1101:
> +	case PHY_ID_TJA1102:
>  		ret = phy_set_bits(phydev, MII_COMMCFG, MII_COMMCFG_AUTO_OP);
>  		if (ret)
>  			return ret;
> @@ -337,6 +339,31 @@ static int tja11xx_probe(struct phy_device *phydev)
>  	if (!priv)
>  		return -ENOMEM;
>  
> +	/* Use the phyid to distinguish between port 0 and port 1 of the
> +	 * TJA1102. Port 0 has a proper phyid, while port 1 reads 0.
> +	 */
> +	if ((phydev->phy_id & PHY_ID_MASK) == PHY_ID_TJA1102) {
> +		int ret;
> +		u32 id;
> +
> +		ret = phy_read(phydev, MII_PHYSID1);
> +		if (ret < 0)
> +			return ret;
> +
> +		id = ret;
> +		ret = phy_read(phydev, MII_PHYSID2);
> +		if (ret < 0)
> +			return ret;
> +
> +		id |= ret << 16;
> +
> +		/* TJA1102 Port 1 has phyid 0 and doesn't support temperature
> +		 * and undervoltage alarms.
> +		 */
> +		if (id == 0)
> +			return 0;

I'm not sure I understand what you're doing here. The two ports of the chip
are separate PHY's on individual MDIO bus addresses?
Reading the PHY ID registers here seems to repeat what phylib did already
to populate phydev->phy_id. If port 1 has PHD ID 0 then the driver wouldn't
bind and tja11xx_probe() would never be called (see phy_bus_match)

> +	}
> +
>  	priv->hwmon_name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
>  	if (!priv->hwmon_name)
>  		return -ENOMEM;
> @@ -385,6 +412,21 @@ static struct phy_driver tja11xx_driver[] = {
>  		.get_sset_count = tja11xx_get_sset_count,
>  		.get_strings	= tja11xx_get_strings,
>  		.get_stats	= tja11xx_get_stats,
> +	}, {
> +		PHY_ID_MATCH_MODEL(PHY_ID_TJA1102),
> +		.name		= "NXP TJA1102",
> +		.features       = PHY_BASIC_T1_FEATURES,
> +		.probe		= tja11xx_probe,
> +		.soft_reset	= tja11xx_soft_reset,
> +		.config_init	= tja11xx_config_init,
> +		.read_status	= tja11xx_read_status,
> +		.suspend	= genphy_suspend,
> +		.resume		= genphy_resume,
> +		.set_loopback   = genphy_loopback,
> +		/* Statistics */
> +		.get_sset_count = tja11xx_get_sset_count,
> +		.get_strings	= tja11xx_get_strings,
> +		.get_stats	= tja11xx_get_stats,
>  	}
>  };
>  
> @@ -393,6 +435,7 @@ module_phy_driver(tja11xx_driver);
>  static struct mdio_device_id __maybe_unused tja11xx_tbl[] = {
>  	{ PHY_ID_MATCH_MODEL(PHY_ID_TJA1100) },
>  	{ PHY_ID_MATCH_MODEL(PHY_ID_TJA1101) },
> +	{ PHY_ID_MATCH_MODEL(PHY_ID_TJA1102) },
>  	{ }
>  };
>  
> 


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

* Re: [PATCH v1] net: phy: tja11xx: add TJA1102 support
  2020-03-03  8:46 ` Heiner Kallweit
@ 2020-03-03  8:56   ` Marc Kleine-Budde
  2020-03-03 19:50     ` Heiner Kallweit
  0 siblings, 1 reply; 12+ messages in thread
From: Marc Kleine-Budde @ 2020-03-03  8:56 UTC (permalink / raw)
  To: Heiner Kallweit, Oleksij Rempel, Andrew Lunn, Florian Fainelli
  Cc: Marek Vasut, netdev, linux-kernel, Pengutronix Kernel Team,
	David Jander, David S. Miller

On 3/3/20 9:46 AM, Heiner Kallweit wrote:
> On 03.03.2020 08:37, Oleksij Rempel wrote:
>> TJA1102 is an dual T1 PHY chip. Both PHYs are separately addressable.
>> PHY 0 can be identified by PHY ID. PHY 1 has no PHY ID and can be
>> configured in device tree by setting compatible =
>> "ethernet-phy-id0180.dc81".
    ^^^^^^^^^^^^^^^^^^^^^^^^

> I'm not sure I understand what you're doing here. The two ports of the chip
> are separate PHY's on individual MDIO bus addresses?

Yes, Port 0 and Port 1 have seperate MFIO bus addresses, but only Port 0
has a proper phy_id (== PHY_ID_TJA1102), while Port 1 just has 0.

Currently we register Port 1 via the device tree compatible
"ethernet-phy-id0180.dc81".

> Reading the PHY ID registers here seems to repeat what phylib did already
> to populate phydev->phy_id. If port 1 has PHD ID 0 then the driver wouldn't
> bind and tja11xx_probe() would never be called (see phy_bus_match)

But we "force" it via the DT compatible. Another option would be to make
up a phyid for Port 1 and hope that it doesn't collide with a real phy
id in other or upcoming chips. But that sounds not like a clean solution
either.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

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

* Re: [PATCH v1] net: phy: tja11xx: add TJA1102 support
  2020-03-03  7:37 [PATCH v1] net: phy: tja11xx: add TJA1102 support Oleksij Rempel
  2020-03-03  8:46 ` Heiner Kallweit
@ 2020-03-03 12:18 ` Marek Vasut
  2020-03-03 12:29   ` Marc Kleine-Budde
  2020-03-03 13:59 ` Andrew Lunn
  2 siblings, 1 reply; 12+ messages in thread
From: Marek Vasut @ 2020-03-03 12:18 UTC (permalink / raw)
  To: Oleksij Rempel, Andrew Lunn, Florian Fainelli, Heiner Kallweit
  Cc: Pengutronix Kernel Team, linux-kernel, David S. Miller, netdev,
	David Jander

On 3/3/20 8:37 AM, Oleksij Rempel wrote:
> TJA1102 is an dual T1 PHY chip. Both PHYs are separately addressable.
> PHY 0 can be identified by PHY ID. PHY 1 has no PHY ID and can be
> configured in device tree by setting compatible =
> "ethernet-phy-id0180.dc81".
> 
> PHY 1 has less suported registers and functionality. For current driver
> it will affect only the HWMON support.

Can't you do some magic with match_phy_device (like in
8b95599c55ed24b36cf44a4720067cfe67edbcb4) to discern the second half of
the PHY ?

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

* Re: [PATCH v1] net: phy: tja11xx: add TJA1102 support
  2020-03-03 12:18 ` Marek Vasut
@ 2020-03-03 12:29   ` Marc Kleine-Budde
  0 siblings, 0 replies; 12+ messages in thread
From: Marc Kleine-Budde @ 2020-03-03 12:29 UTC (permalink / raw)
  To: Marek Vasut, Oleksij Rempel, Andrew Lunn, Florian Fainelli,
	Heiner Kallweit
  Cc: netdev, David Jander, linux-kernel, Pengutronix Kernel Team,
	David S. Miller

On 3/3/20 1:18 PM, Marek Vasut wrote:
> On 3/3/20 8:37 AM, Oleksij Rempel wrote:
>> TJA1102 is an dual T1 PHY chip. Both PHYs are separately addressable.
>> PHY 0 can be identified by PHY ID. PHY 1 has no PHY ID and can be
>> configured in device tree by setting compatible =
>> "ethernet-phy-id0180.dc81".
>>
>> PHY 1 has less suported registers and functionality. For current driver
>> it will affect only the HWMON support.
> 
> Can't you do some magic with match_phy_device (like in
> 8b95599c55ed24b36cf44a4720067cfe67edbcb4) to discern the second half of
> the PHY ?

Great, that looks better.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

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

* Re: [PATCH v1] net: phy: tja11xx: add TJA1102 support
  2020-03-03  7:37 [PATCH v1] net: phy: tja11xx: add TJA1102 support Oleksij Rempel
  2020-03-03  8:46 ` Heiner Kallweit
  2020-03-03 12:18 ` Marek Vasut
@ 2020-03-03 13:59 ` Andrew Lunn
  2020-03-03 15:36   ` Oleksij Rempel
  2 siblings, 1 reply; 12+ messages in thread
From: Andrew Lunn @ 2020-03-03 13:59 UTC (permalink / raw)
  To: Oleksij Rempel
  Cc: Florian Fainelli, Heiner Kallweit, Pengutronix Kernel Team,
	linux-kernel, David S. Miller, netdev, Marek Vasut, David Jander,
	Quentin Schulz

Hi Oleksij

> TJA1102 is an dual T1 PHY chip. Both PHYs are separately addressable.
> PHY 0 can be identified by PHY ID. PHY 1 has no PHY ID and can be
> configured in device tree by setting compatible =
> "ethernet-phy-id0180.dc81".

Why-o-why do silicon vendors make devices with invalid PHY IDs!?!?!

Did you try avoiding the compatible string. We know PHY 0 will probe
as normal. From its PHY ID we know it is a dual device. Could the
probe of PHY 0 register PHY 1?

No idea if it will work, but could nxp-tja11xx.c register is fixup for
PHY_ID_TJA1102. That fixup would do something like:

void tja1102_fixup(struct phy_device *phydev_phy0)
{
        struct mii_bus *bus = phydev_phy0->mdio.mii;
        struct phy_device *phydev_phy1;

        phydev_phy1 = phy_device_create(bus, phydev_phy0->addr + 1,
                                        PHY_ID_TJA1102, FALSE, NULL);
	if (phydev_phy1)
               phy_device_register(phydev_phy1);
}

I think the issue here is, it will deadlock when scanning for fixup
for phydev_phy1. So this basic idea, but maybe hooked in somewhere
else?

Something like this might also help vsc8584 which is a quad PHY with
some shared registers?

	  Andrew

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

* Re: [PATCH v1] net: phy: tja11xx: add TJA1102 support
  2020-03-03 13:59 ` Andrew Lunn
@ 2020-03-03 15:36   ` Oleksij Rempel
  0 siblings, 0 replies; 12+ messages in thread
From: Oleksij Rempel @ 2020-03-03 15:36 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Florian Fainelli, Heiner Kallweit, Pengutronix Kernel Team,
	linux-kernel, David S. Miller, netdev, Marek Vasut, David Jander,
	Quentin Schulz

On Tue, Mar 03, 2020 at 02:59:36PM +0100, Andrew Lunn wrote:
> Hi Oleksij
> 
> > TJA1102 is an dual T1 PHY chip. Both PHYs are separately addressable.
> > PHY 0 can be identified by PHY ID. PHY 1 has no PHY ID and can be
> > configured in device tree by setting compatible =
> > "ethernet-phy-id0180.dc81".
> 
> Why-o-why do silicon vendors make devices with invalid PHY IDs!?!?!
> 
> Did you try avoiding the compatible string. We know PHY 0 will probe
> as normal. From its PHY ID we know it is a dual device. Could the
> probe of PHY 0 register PHY 1?
> 
> No idea if it will work, but could nxp-tja11xx.c register is fixup for
> PHY_ID_TJA1102. That fixup would do something like:
> 
> void tja1102_fixup(struct phy_device *phydev_phy0)
> {
>         struct mii_bus *bus = phydev_phy0->mdio.mii;
>         struct phy_device *phydev_phy1;
> 
>         phydev_phy1 = phy_device_create(bus, phydev_phy0->addr + 1,
>                                         PHY_ID_TJA1102, FALSE, NULL);
> 	if (phydev_phy1)
>                phy_device_register(phydev_phy1);
> }
> 
> I think the issue here is, it will deadlock when scanning for fixup
> for phydev_phy1. So this basic idea, but maybe hooked in somewhere
> else?
> 
> Something like this might also help vsc8584 which is a quad PHY with
> some shared registers?

OK, thx! I'll take a look on it.
Currently there is not solved issues with controlled power on/reset sequence
of this chip. The reset and enable pins will affect both PHYs. So, may be vsc8584
will answer my questions.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

* Re: [PATCH v1] net: phy: tja11xx: add TJA1102 support
  2020-03-03  8:56   ` Marc Kleine-Budde
@ 2020-03-03 19:50     ` Heiner Kallweit
  0 siblings, 0 replies; 12+ messages in thread
From: Heiner Kallweit @ 2020-03-03 19:50 UTC (permalink / raw)
  To: Marc Kleine-Budde, Oleksij Rempel, Andrew Lunn, Florian Fainelli
  Cc: Marek Vasut, netdev, linux-kernel, Pengutronix Kernel Team,
	David Jander, David S. Miller

On 03.03.2020 09:56, Marc Kleine-Budde wrote:
> On 3/3/20 9:46 AM, Heiner Kallweit wrote:
>> On 03.03.2020 08:37, Oleksij Rempel wrote:
>>> TJA1102 is an dual T1 PHY chip. Both PHYs are separately addressable.
>>> PHY 0 can be identified by PHY ID. PHY 1 has no PHY ID and can be
>>> configured in device tree by setting compatible =
>>> "ethernet-phy-id0180.dc81".
>     ^^^^^^^^^^^^^^^^^^^^^^^^
> 
>> I'm not sure I understand what you're doing here. The two ports of the chip
>> are separate PHY's on individual MDIO bus addresses?
> 
> Yes, Port 0 and Port 1 have seperate MFIO bus addresses, but only Port 0
> has a proper phy_id (== PHY_ID_TJA1102), while Port 1 just has 0.
> 
> Currently we register Port 1 via the device tree compatible
> "ethernet-phy-id0180.dc81".
> 
Thanks for the explanation. Missed that part, then reading the PHY ID
of the chip is skipped. For checking whether we are port 0 or 1 it wouldn't
even be needed to read both PHY ID registers.
One drawback of the solution might be that it works on DT-configured
systems only (not sure how relevant this is). A little bit more detailed
comment may be helpful, to avoid misunderstandings like mine.

>> Reading the PHY ID registers here seems to repeat what phylib did already
>> to populate phydev->phy_id. If port 1 has PHD ID 0 then the driver wouldn't
>> bind and tja11xx_probe() would never be called (see phy_bus_match)
> 
> But we "force" it via the DT compatible. Another option would be to make
> up a phyid for Port 1 and hope that it doesn't collide with a real phy
> id in other or upcoming chips. But that sounds not like a clean solution
> either.
> 
> Marc
> 
Heiner

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

* Re: [PATCH v1] net: phy: tja11xx: add TJA1102 support
  2020-03-03 14:09   ` Andrew Lunn
@ 2020-03-03 14:11     ` Marc Kleine-Budde
  0 siblings, 0 replies; 12+ messages in thread
From: Marc Kleine-Budde @ 2020-03-03 14:11 UTC (permalink / raw)
  To: Andrew Lunn, Oleksij Rempel
  Cc: Marek Vasut, Florian Fainelli, netdev, linux-kernel,
	Pengutronix Kernel Team, David Jander, Christian Herber,
	David S. Miller, Heiner Kallweit

On 3/3/20 3:09 PM, Andrew Lunn wrote:
>>> Hi Oleksij, Heiner, Marc,
>>>
>>> You could also refer the solution implemented here as part of a TJA110x driver:
>>> https://source.codeaurora.org/external/autoivnsw/tja110x_linux_phydev/about/
>>
>> OK, thank you!
>>
>> Suddenly, the solution in this driver is not mainlainable. It may match on
>> ther PHYs with PHYID == 0.
>>
>> See this part of the code:
>> #define NXP_PHY_ID_TJA1102P1      (0x00000000U)
>> ...
>> 	, {
>> 	.phy_id = NXP_PHY_ID_TJA1102P1,
>> 	.name = "TJA1102_p1",
>> 	.phy_id_mask = NXP_PHY_ID_MASK,
> 
> Noooo
> 
> You cannot assume NXP is the only silicon vendor to manufacture broken
> silicon with a PHY ID of 0.

ACK, we ruled that soultion out already :)

We'll look into your and Marek's suggestions.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

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

* Re: [PATCH v1] net: phy: tja11xx: add TJA1102 support
  2020-03-03 12:55 ` Oleksij Rempel
@ 2020-03-03 14:09   ` Andrew Lunn
  2020-03-03 14:11     ` Marc Kleine-Budde
  0 siblings, 1 reply; 12+ messages in thread
From: Andrew Lunn @ 2020-03-03 14:09 UTC (permalink / raw)
  To: Oleksij Rempel
  Cc: Christian Herber, Heiner Kallweit, Florian Fainelli, Marek Vasut,
	netdev, linux-kernel, Pengutronix Kernel Team, David Jander,
	David S. Miller

> > Hi Oleksij, Heiner, Marc,
> > 
> > You could also refer the solution implemented here as part of a TJA110x driver:
> > https://source.codeaurora.org/external/autoivnsw/tja110x_linux_phydev/about/
> 
> OK, thank you!
> 
> Suddenly, the solution in this driver is not mainlainable. It may match on
> ther PHYs with PHYID == 0.
> 
> See this part of the code:
> #define NXP_PHY_ID_TJA1102P1      (0x00000000U)
> ...
> 	, {
> 	.phy_id = NXP_PHY_ID_TJA1102P1,
> 	.name = "TJA1102_p1",
> 	.phy_id_mask = NXP_PHY_ID_MASK,

Noooo

You cannot assume NXP is the only silicon vendor to manufacture broken
silicon with a PHY ID of 0.

	Andrew

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

* Re: [PATCH v1] net: phy: tja11xx: add TJA1102 support
  2020-03-03 12:42 Christian Herber
  2020-03-03 12:53 ` Marc Kleine-Budde
@ 2020-03-03 12:55 ` Oleksij Rempel
  2020-03-03 14:09   ` Andrew Lunn
  1 sibling, 1 reply; 12+ messages in thread
From: Oleksij Rempel @ 2020-03-03 12:55 UTC (permalink / raw)
  To: Christian Herber, Heiner Kallweit, Andrew Lunn, Florian Fainelli
  Cc: Marek Vasut, netdev, linux-kernel, Pengutronix Kernel Team,
	David Jander, David S. Miller



On 03.03.20 13:42, Christian Herber wrote:
>> On 03.03.2020 08:37, Oleksij Rempel wrote:
>>> TJA1102 is an dual T1 PHY chip. Both PHYs are separately addressable.
>>> PHY 0 can be identified by PHY ID. PHY 1 has no PHY ID and can be
>>> configured in device tree by setting compatible =
>>> "ethernet-phy-id0180.dc81".
>>>
>>> PHY 1 has less suported registers and functionality. For current driver
>>> it will affect only the HWMON support.
>>>
>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>>> ---
>>> drivers/net/phy/nxp-tja11xx.c | 43 +++++++++++++++++++++++++++++++++++
>>> 1 file changed, 43 insertions(+)
>>>
>>> diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
>>> index b705d0bd798b..52090cfaa54e 100644
>>> --- a/drivers/net/phy/nxp-tja11xx.c
>>> +++ b/drivers/net/phy/nxp-tja11xx.c
>>> @@ -15,6 +15,7 @@
>>> #define PHY_ID_MASK                  0xfffffff0
>>> #define PHY_ID_TJA1100                       0x0180dc40
>>> #define PHY_ID_TJA1101                       0x0180dd00
>>> +#define PHY_ID_TJA1102                       0x0180dc80
>>>
>>> #define MII_ECTRL                    17
>>> #define MII_ECTRL_LINK_CONTROL               BIT(15)
>>> @@ -190,6 +191,7 @@ static int tja11xx_config_init(struct phy_device *phydev)
>>>               return ret;
>>>       break;
>>> case PHY_ID_TJA1101:
>>> +     case PHY_ID_TJA1102:
>>>       ret = phy_set_bits(phydev, MII_COMMCFG, MII_COMMCFG_AUTO_OP);
>>>       if (ret)
>>>               return ret;
>>> @@ -337,6 +339,31 @@ static int tja11xx_probe(struct phy_device *phydev)
>>> if (!priv)
>>>       return -ENOMEM;
>>>
>>> +     /* Use the phyid to distinguish between port 0 and port 1 of the
>>> +      * TJA1102. Port 0 has a proper phyid, while port 1 reads 0.
>>> +      */
>>> +     if ((phydev->phy_id & PHY_ID_MASK) == PHY_ID_TJA1102) {
>>> +             int ret;
>>> +             u32 id;
>>> +
>>> +             ret = phy_read(phydev, MII_PHYSID1);
>>> +             if (ret < 0)
>>> +                     return ret;
>>> +
>>> +             id = ret;
>>> +             ret = phy_read(phydev, MII_PHYSID2);
>>> +             if (ret < 0)
>>> +                     return ret;
>>> +
>>> +             id |= ret << 16;
>>> +
>>> +             /* TJA1102 Port 1 has phyid 0 and doesn't support temperature
>>> +              * and undervoltage alarms.
>>> +              */
>>> +             if (id == 0)
>>> +                     return 0;
>>
>> I'm not sure I understand what you're doing here. The two ports of the chip
>> are separate PHY's on individual MDIO bus addresses?
>> Reading the PHY ID registers here seems to repeat what phylib did already
>> to populate phydev->phy_id. If port 1 has PHD ID 0 then the driver wouldn't
>> bind and tja11xx_probe() would never be called (see phy_bus_match)
>>
>>> +     }
>>> +
>>> priv->hwmon_name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
>>> if (!priv->hwmon_name)
>>>       return -ENOMEM;
>>> @@ -385,6 +412,21 @@ static struct phy_driver tja11xx_driver[] = {
>>>       .get_sset_count = tja11xx_get_sset_count,
>>>       .get_strings    = tja11xx_get_strings,
>>>       .get_stats      = tja11xx_get_stats,
>>> +     }, {
>>> +             PHY_ID_MATCH_MODEL(PHY_ID_TJA1102),
>>> +             .name           = "NXP TJA1102",
>>> +             .features       = PHY_BASIC_T1_FEATURES,
>>> +             .probe          = tja11xx_probe,
>>> +             .soft_reset     = tja11xx_soft_reset,
>>> +             .config_init    = tja11xx_config_init,
>>> +             .read_status    = tja11xx_read_status,
>>> +             .suspend        = genphy_suspend,
>>> +             .resume         = genphy_resume,
>>> +             .set_loopback   = genphy_loopback,
>>> +             /* Statistics */
>>> +             .get_sset_count = tja11xx_get_sset_count,
>>> +             .get_strings    = tja11xx_get_strings,
>>> +             .get_stats      = tja11xx_get_stats,
>>> }
>>> };
>>>
>>> @@ -393,6 +435,7 @@ module_phy_driver(tja11xx_driver);
>>> static struct mdio_device_id __maybe_unused tja11xx_tbl[] = {
>>> { PHY_ID_MATCH_MODEL(PHY_ID_TJA1100) },
>>> { PHY_ID_MATCH_MODEL(PHY_ID_TJA1101) },
>>> +     { PHY_ID_MATCH_MODEL(PHY_ID_TJA1102) },
>>> { }
>>> };
> 
> Hi Oleksij, Heiner, Marc,
> 
> You could also refer the solution implemented here as part of a TJA110x driver:
> https://source.codeaurora.org/external/autoivnsw/tja110x_linux_phydev/about/

OK, thank you!

Suddenly, the solution in this driver is not mainlainable. It may match on ther PHYs with 
PHYID == 0.

See this part of the code:
#define NXP_PHY_ID_TJA1102P1      (0x00000000U)
...
	, {
	.phy_id = NXP_PHY_ID_TJA1102P1,
	.name = "TJA1102_p1",
	.phy_id_mask = NXP_PHY_ID_MASK,


Kind regards,
Oleksij Rempel

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

* Re: [PATCH v1] net: phy: tja11xx: add TJA1102 support
  2020-03-03 12:42 Christian Herber
@ 2020-03-03 12:53 ` Marc Kleine-Budde
  2020-03-03 12:55 ` Oleksij Rempel
  1 sibling, 0 replies; 12+ messages in thread
From: Marc Kleine-Budde @ 2020-03-03 12:53 UTC (permalink / raw)
  To: Christian Herber, Heiner Kallweit, Oleksij Rempel, Andrew Lunn,
	Florian Fainelli
  Cc: Marek Vasut, netdev, linux-kernel, Pengutronix Kernel Team,
	David Jander, David S. Miller

On 3/3/20 1:42 PM, Christian Herber wrote:
> You could also refer the solution implemented here as part of a TJA110x driver:
> https://source.codeaurora.org/external/autoivnsw/tja110x_linux_phydev/about/

Do you mean this?
https://source.codeaurora.org/external/autoivnsw/tja110x_linux_phydev/tree/tja110x.c#n26

| /* load driver for TJA1102p1. It needs to be ensured,
|  * that no other mdio device with phy id 0 is present
|  */
| #define CONFIG_TJA1102_FIX

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

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

end of thread, other threads:[~2020-03-03 19:50 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-03  7:37 [PATCH v1] net: phy: tja11xx: add TJA1102 support Oleksij Rempel
2020-03-03  8:46 ` Heiner Kallweit
2020-03-03  8:56   ` Marc Kleine-Budde
2020-03-03 19:50     ` Heiner Kallweit
2020-03-03 12:18 ` Marek Vasut
2020-03-03 12:29   ` Marc Kleine-Budde
2020-03-03 13:59 ` Andrew Lunn
2020-03-03 15:36   ` Oleksij Rempel
2020-03-03 12:42 Christian Herber
2020-03-03 12:53 ` Marc Kleine-Budde
2020-03-03 12:55 ` Oleksij Rempel
2020-03-03 14:09   ` Andrew Lunn
2020-03-03 14:11     ` Marc Kleine-Budde

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).