| Message ID | 20200303073715.32301-1-o.rempel@pengutronix.de |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v1] net: phy: tja11xx: add TJA1102 support | expand |
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) }, > { } > }; > >
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
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 ?
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
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
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
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
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) }, { } };
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(+)