[V5] net: phy: tja11xx: Add TJA11xx PHY driver

Message ID	20190517235123.32261-1-marex@denx.de (mailing list archive)
State	Not Applicable
Headers	show Return-Path: <linux-hwmon-owner@kernel.org> From: Marek Vasut <marex@denx.de> To: netdev@vger.kernel.org Cc: Marek Vasut <marex@denx.de>, Andrew Lunn <andrew@lunn.ch>, Florian Fainelli <f.fainelli@gmail.com>, Guenter Roeck <linux@roeck-us.net>, Heiner Kallweit <hkallweit1@gmail.com>, Jean Delvare <jdelvare@suse.com>, linux-hwmon@vger.kernel.org Subject: [PATCH V5] net: phy: tja11xx: Add TJA11xx PHY driver Date: Sat, 18 May 2019 01:51:23 +0200 Message-Id: <20190517235123.32261-1-marex@denx.de> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-hwmon-owner@vger.kernel.org Precedence: bulk
Series	[V5] net: phy: tja11xx: Add TJA11xx PHY driver \| expand [V5] net: phy: tja11xx: Add TJA11xx PHY driver

Marek Vasut May 17, 2019, 11:51 p.m. UTC

Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
BroadRReach 100BaseT1 PHYs used in automotive.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Jean Delvare <jdelvare@suse.com>
Cc: linux-hwmon@vger.kernel.org
---
V2: - Use phy_modify(), phy_{set,clear}_bits()
    - Drop enable argument of tja11xx_enable_link_control()
    - Use PHY_BASIC_T1_FEATURES and dont modify supported/advertised
      features in config_init callback
    - Use genphy_soft_reset() instead of opencoding the reset sequence.
    - Drop the aneg parts, since the PHY datasheet claims it does not
      support aneg
V3: - Replace clr with mask
    - Add hwmon support
    - Check commstat in tja11xx_read_status() only if link is up
    - Use PHY_ID_MATCH_MODEL()
V4: - Use correct bit in tja11xx_hwmon_read() hwmon_temp_crit_alarm
    - Use ENOMEM if devm_kstrdup() fails
    - Check $type in tja11xx_hwmon_read() in addition to $attr
V5: - Drop assignment of phydev->irq,pause,asym_pause
---
 drivers/net/phy/Kconfig       |   6 +
 drivers/net/phy/Makefile      |   1 +
 drivers/net/phy/nxp-tja11xx.c | 423 ++++++++++++++++++++++++++++++++++
 3 files changed, 430 insertions(+)
 create mode 100644 drivers/net/phy/nxp-tja11xx.c

Andrew Lunn May 18, 2019, 2:14 p.m. UTC | #1

On Sat, May 18, 2019 at 01:51:23AM +0200, Marek Vasut wrote:
> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
> BroadRReach 100BaseT1 PHYs used in automotive.

Hi Marek

> +	}, {
> +		PHY_ID_MATCH_MODEL(PHY_ID_TJA1101),
> +		.name		= "NXP TJA1101",
> +		.features       = PHY_BASIC_T1_FEATURES,

One thing i would like to do before this patch goes in is define
ETHTOOL_LINK_MODE_100baseT1_Full_BIT in ethtool.h, and use it here.
We could not do it earlier because were ran out of bits. But with
PHYLIB now using bitmaps, rather than u32, we can.

Once net-next reopens i will submit a patch adding it.

I also see in the data sheet we should be able to correct detect its
features using register 15. So we should extend
genphy_read_abilities(). That will allow us to avoid changing
PHY_BASIC_T1_FEATURES and possibly breaking backwards compatibility of
other T1 PHY which currently say they are plain 100BaseT.

      Andrew

Marek Vasut May 18, 2019, 4:50 p.m. UTC | #2

On 5/18/19 4:14 PM, Andrew Lunn wrote:
> On Sat, May 18, 2019 at 01:51:23AM +0200, Marek Vasut wrote:
>> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
>> BroadRReach 100BaseT1 PHYs used in automotive.
> 
> Hi Marek

Hello Andrew,

>> +	}, {
>> +		PHY_ID_MATCH_MODEL(PHY_ID_TJA1101),
>> +		.name		= "NXP TJA1101",
>> +		.features       = PHY_BASIC_T1_FEATURES,
> 
> One thing i would like to do before this patch goes in is define
> ETHTOOL_LINK_MODE_100baseT1_Full_BIT in ethtool.h, and use it here.
> We could not do it earlier because were ran out of bits. But with
> PHYLIB now using bitmaps, rather than u32, we can.
> 
> Once net-next reopens i will submit a patch adding it.

I can understand blocking patches from being applied if they have review
problems or need to be updated on some existing or even posted feature.
But blocking a patch because some future yet-to-be-developed patch is a
bit odd.

Besides, this sounds more like a cleanup which can very well be done
later. It will surely be done for the other PHY drivers too.

> I also see in the data sheet we should be able to correct detect its
> features using register 15. So we should extend
> genphy_read_abilities().

Which bits do you refer to ?

Anyway, this is something which can be done in a subsequent patch, I
don't see a reason for blocking hardware enablement because of this.

> That will allow us to avoid changing
> PHY_BASIC_T1_FEATURES and possibly breaking backwards compatibility of
> other T1 PHY which currently say they are plain 100BaseT.

What sort of backward compatibility ?

Andrew Lunn May 18, 2019, 8:12 p.m. UTC | #3

On Sat, May 18, 2019 at 06:50:48PM +0200, Marek Vasut wrote:
> On 5/18/19 4:14 PM, Andrew Lunn wrote:
> > On Sat, May 18, 2019 at 01:51:23AM +0200, Marek Vasut wrote:
> >> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
> >> BroadRReach 100BaseT1 PHYs used in automotive.
> > 
> > Hi Marek
> 
> Hello Andrew,
> 
> >> +	}, {
> >> +		PHY_ID_MATCH_MODEL(PHY_ID_TJA1101),
> >> +		.name		= "NXP TJA1101",
> >> +		.features       = PHY_BASIC_T1_FEATURES,
> > 
> > One thing i would like to do before this patch goes in is define
> > ETHTOOL_LINK_MODE_100baseT1_Full_BIT in ethtool.h, and use it here.
> > We could not do it earlier because were ran out of bits. But with
> > PHYLIB now using bitmaps, rather than u32, we can.
> > 
> > Once net-next reopens i will submit a patch adding it.
> 
> I can understand blocking patches from being applied if they have review
> problems or need to be updated on some existing or even posted feature.
> But blocking a patch because some future yet-to-be-developed patch is a
> bit odd.

Hi Marek

What i'm trying to avoid is an ABI change. By using
PHY_BASIC_T1_FEATURES you are saying the device support 100BaseT. It
does not. It supports 100BaseT1. I want to add 100BaseT1 first, so
your PHY does not change from 100BaseT to 100BaseT1, which could be
considered an ABI change.

I'm not suggesting blocking your patch for a long time. I'm already
2/3 of the way doing the work. At the latest, i expect to have patches
submitted in the next few days. And then your driver can go in, using
this. So by end of next week, your driver can be in.

> > I also see in the data sheet we should be able to correct detect its
> > features using register 15. So we should extend
> > genphy_read_abilities().
> 
> Which bits do you refer to ?

Register 15, bit 7. This indicates the PHY can do 100BaseT1. I want to
double check with the 802.3 standard, but i expect this is part of the
standard.

	Andrew

Marek Vasut May 18, 2019, 8:46 p.m. UTC | #4

On 5/18/19 10:12 PM, Andrew Lunn wrote:
> On Sat, May 18, 2019 at 06:50:48PM +0200, Marek Vasut wrote:
>> On 5/18/19 4:14 PM, Andrew Lunn wrote:
>>> On Sat, May 18, 2019 at 01:51:23AM +0200, Marek Vasut wrote:
>>>> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
>>>> BroadRReach 100BaseT1 PHYs used in automotive.
>>>
>>> Hi Marek
>>
>> Hello Andrew,
>>
>>>> +	}, {
>>>> +		PHY_ID_MATCH_MODEL(PHY_ID_TJA1101),
>>>> +		.name		= "NXP TJA1101",
>>>> +		.features       = PHY_BASIC_T1_FEATURES,
>>>
>>> One thing i would like to do before this patch goes in is define
>>> ETHTOOL_LINK_MODE_100baseT1_Full_BIT in ethtool.h, and use it here.
>>> We could not do it earlier because were ran out of bits. But with
>>> PHYLIB now using bitmaps, rather than u32, we can.
>>>
>>> Once net-next reopens i will submit a patch adding it.
>>
>> I can understand blocking patches from being applied if they have review
>> problems or need to be updated on some existing or even posted feature.
>> But blocking a patch because some future yet-to-be-developed patch is a
>> bit odd.
> 
> Hi Marek
> 
> What i'm trying to avoid is an ABI change. By using
> PHY_BASIC_T1_FEATURES you are saying the device support 100BaseT. It
> does not. It supports 100BaseT1. I want to add 100BaseT1 first, so
> your PHY does not change from 100BaseT to 100BaseT1, which could be
> considered an ABI change.

I would expect PHY_BASIC_T1_FEATURES , with T1 in the middle , would
imply 100baseT1 , but maybe that's a misnomer .

> I'm not suggesting blocking your patch for a long time. I'm already
> 2/3 of the way doing the work. At the latest, i expect to have patches
> submitted in the next few days. And then your driver can go in, using
> this. So by end of next week, your driver can be in.

But someone has to review your patches too, so that would add another
few weeks.

>>> I also see in the data sheet we should be able to correct detect its
>>> features using register 15. So we should extend
>>> genphy_read_abilities().
>>
>> Which bits do you refer to ?
> 
> Register 15, bit 7. This indicates the PHY can do 100BaseT1. I want to
> double check with the 802.3 standard, but i expect this is part of the
> standard.

The PHY does only 100BaseT1 .

Florian Fainelli May 20, 2019, 5:21 p.m. UTC | #5

On May 18, 2019 9:50:48 AM PDT, Marek Vasut <marex@denx.de> wrote:
>On 5/18/19 4:14 PM, Andrew Lunn wrote:
>> On Sat, May 18, 2019 at 01:51:23AM +0200, Marek Vasut wrote:
>>> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are
>special
>>> BroadRReach 100BaseT1 PHYs used in automotive.
>> 
>> Hi Marek
>
>Hello Andrew,
>
>>> +	}, {
>>> +		PHY_ID_MATCH_MODEL(PHY_ID_TJA1101),
>>> +		.name		= "NXP TJA1101",
>>> +		.features       = PHY_BASIC_T1_FEATURES,
>> 
>> One thing i would like to do before this patch goes in is define
>> ETHTOOL_LINK_MODE_100baseT1_Full_BIT in ethtool.h, and use it here.
>> We could not do it earlier because were ran out of bits. But with
>> PHYLIB now using bitmaps, rather than u32, we can.
>> 
>> Once net-next reopens i will submit a patch adding it.
>
>I can understand blocking patches from being applied if they have
>review
>problems or need to be updated on some existing or even posted feature.
>But blocking a patch because some future yet-to-be-developed patch is a
>bit odd.
>

The net-next tree is currently closed which means there is ample time
for you and Andrew to work offline and have you submit both Andrew's
cleanup patches as well as this very one as a patch series when net-next
opens back up giving both of you your own cookies. Once submitted it
won't take weeks to get merged more like hours given David's typical
review and patch acceptance time.
--
Florian

Marek Vasut May 22, 2019, 9:48 p.m. UTC | #6

On 5/18/19 4:14 PM, Andrew Lunn wrote:
> On Sat, May 18, 2019 at 01:51:23AM +0200, Marek Vasut wrote:
>> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
>> BroadRReach 100BaseT1 PHYs used in automotive.
> 
> Hi Marek
> 
>> +	}, {
>> +		PHY_ID_MATCH_MODEL(PHY_ID_TJA1101),
>> +		.name		= "NXP TJA1101",
>> +		.features       = PHY_BASIC_T1_FEATURES,
> 
> One thing i would like to do before this patch goes in is define
> ETHTOOL_LINK_MODE_100baseT1_Full_BIT in ethtool.h, and use it here.
> We could not do it earlier because were ran out of bits. But with
> PHYLIB now using bitmaps, rather than u32, we can.

So now that "[PATCH net-next 0/2] net: phy: T1 support" is posted, does
this patch require any change ? I don't think it does ?

[...]

Andrew Lunn May 22, 2019, 10:14 p.m. UTC | #7

On Wed, May 22, 2019 at 11:48:06PM +0200, Marek Vasut wrote:
> On 5/18/19 4:14 PM, Andrew Lunn wrote:
> > On Sat, May 18, 2019 at 01:51:23AM +0200, Marek Vasut wrote:
> >> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
> >> BroadRReach 100BaseT1 PHYs used in automotive.
> > 
> > Hi Marek
> > 
> >> +	}, {
> >> +		PHY_ID_MATCH_MODEL(PHY_ID_TJA1101),
> >> +		.name		= "NXP TJA1101",
> >> +		.features       = PHY_BASIC_T1_FEATURES,
> > 
> > One thing i would like to do before this patch goes in is define
> > ETHTOOL_LINK_MODE_100baseT1_Full_BIT in ethtool.h, and use it here.
> > We could not do it earlier because were ran out of bits. But with
> > PHYLIB now using bitmaps, rather than u32, we can.
> 
> So now that "[PATCH net-next 0/2] net: phy: T1 support" is posted, does
> this patch require any change ? I don't think it does ?

Hi Marek

In the end, no it does not need any changes because of my patchset.

   Andrew

Marek Vasut May 22, 2019, 10:42 p.m. UTC | #8

On 5/23/19 12:14 AM, Andrew Lunn wrote:
> On Wed, May 22, 2019 at 11:48:06PM +0200, Marek Vasut wrote:
>> On 5/18/19 4:14 PM, Andrew Lunn wrote:
>>> On Sat, May 18, 2019 at 01:51:23AM +0200, Marek Vasut wrote:
>>>> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
>>>> BroadRReach 100BaseT1 PHYs used in automotive.
>>>
>>> Hi Marek
>>>
>>>> +	}, {
>>>> +		PHY_ID_MATCH_MODEL(PHY_ID_TJA1101),
>>>> +		.name		= "NXP TJA1101",
>>>> +		.features       = PHY_BASIC_T1_FEATURES,
>>>
>>> One thing i would like to do before this patch goes in is define
>>> ETHTOOL_LINK_MODE_100baseT1_Full_BIT in ethtool.h, and use it here.
>>> We could not do it earlier because were ran out of bits. But with
>>> PHYLIB now using bitmaps, rather than u32, we can.
>>
>> So now that "[PATCH net-next 0/2] net: phy: T1 support" is posted, does
>> this patch require any change ? I don't think it does ?
> 
> Hi Marek
> 
> In the end, no it does not need any changes because of my patchset.

OK, thanks.

Florian Fainelli May 23, 2019, 2:38 a.m. UTC | #9

+Vladimir,

On 5/17/2019 4:51 PM, Marek Vasut wrote:
> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
> BroadRReach 100BaseT1 PHYs used in automotive.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Cc: Jean Delvare <jdelvare@suse.com>
> Cc: linux-hwmon@vger.kernel.org

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

Vladimir Oltean May 23, 2019, 7:48 p.m. UTC | #10

On Thu, 23 May 2019 at 05:39, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> +Vladimir,
>
> On 5/17/2019 4:51 PM, Marek Vasut wrote:
> > Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
> > BroadRReach 100BaseT1 PHYs used in automotive.
> >
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Andrew Lunn <andrew@lunn.ch>
> > Cc: Florian Fainelli <f.fainelli@gmail.com>
> > Cc: Guenter Roeck <linux@roeck-us.net>
> > Cc: Heiner Kallweit <hkallweit1@gmail.com>
> > Cc: Jean Delvare <jdelvare@suse.com>
> > Cc: linux-hwmon@vger.kernel.org
>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> --
> Florian

My only feedback is: keep up the good work!

-Vladimir

Marek Vasut May 24, 2019, 11:41 a.m. UTC | #11

On 5/23/19 9:48 PM, Vladimir Oltean wrote:
> On Thu, 23 May 2019 at 05:39, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>> +Vladimir,
>>
>> On 5/17/2019 4:51 PM, Marek Vasut wrote:
>>> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
>>> BroadRReach 100BaseT1 PHYs used in automotive.
>>>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> Cc: Andrew Lunn <andrew@lunn.ch>
>>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>>> Cc: Guenter Roeck <linux@roeck-us.net>
>>> Cc: Heiner Kallweit <hkallweit1@gmail.com>
>>> Cc: Jean Delvare <jdelvare@suse.com>
>>> Cc: linux-hwmon@vger.kernel.org
>>
>> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
>> --
>> Florian
> 
> My only feedback is: keep up the good work!

Well, it seems this patch is flagged in patchwork as "changes requested"
. I don't know what changes are requested though :-(

Andrew Lunn May 24, 2019, 1:52 p.m. UTC | #12

> Well, it seems this patch is flagged in patchwork as "changes requested"
> . I don't know what changes are requested though :-(

Hi Marek

The patch was submitted while net-next as closed. That is pretty much
an automatic reject.

Please submit it again, and add on the review tags you have received.
It should then get accepted.

   Andrew

Marek Vasut May 24, 2019, 2:23 p.m. UTC | #13

On 5/24/19 3:52 PM, Andrew Lunn wrote:
>> Well, it seems this patch is flagged in patchwork as "changes requested"
>> . I don't know what changes are requested though :-(
> 
> Hi Marek

Hi,

> The patch was submitted while net-next as closed. That is pretty much
> an automatic reject.
> 
> Please submit it again, and add on the review tags you have received.
> It should then get accepted.

Would be nice to get some sort of notification.

Are all these subtleties documented anywhere ?
Is there some calendar when to send / not-send patches ?

Andrew Lunn May 24, 2019, 2:28 p.m. UTC | #14

> Are all these subtleties documented anywhere ?

The netdev FAQ.

    Andrew

Guenter Roeck May 27, 2019, 3:44 p.m. UTC | #15

On 5/17/19 4:51 PM, Marek Vasut wrote:
> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
> BroadRReach 100BaseT1 PHYs used in automotive.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Cc: Jean Delvare <jdelvare@suse.com>
> Cc: linux-hwmon@vger.kernel.org
> ---
> V2: - Use phy_modify(), phy_{set,clear}_bits()
>      - Drop enable argument of tja11xx_enable_link_control()
>      - Use PHY_BASIC_T1_FEATURES and dont modify supported/advertised
>        features in config_init callback
>      - Use genphy_soft_reset() instead of opencoding the reset sequence.
>      - Drop the aneg parts, since the PHY datasheet claims it does not
>        support aneg
> V3: - Replace clr with mask
>      - Add hwmon support
>      - Check commstat in tja11xx_read_status() only if link is up
>      - Use PHY_ID_MATCH_MODEL()
> V4: - Use correct bit in tja11xx_hwmon_read() hwmon_temp_crit_alarm
>      - Use ENOMEM if devm_kstrdup() fails
>      - Check $type in tja11xx_hwmon_read() in addition to $attr
> V5: - Drop assignment of phydev->irq,pause,asym_pause
> ---
>   drivers/net/phy/Kconfig       |   6 +
>   drivers/net/phy/Makefile      |   1 +
>   drivers/net/phy/nxp-tja11xx.c | 423 ++++++++++++++++++++++++++++++++++
>   3 files changed, 430 insertions(+)
>   create mode 100644 drivers/net/phy/nxp-tja11xx.c
> 
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index d6299710d634..31478d8b3c0c 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -415,6 +415,12 @@ config NATIONAL_PHY
>   	---help---
>   	  Currently supports the DP83865 PHY.
>   
> +config NXP_TJA11XX_PHY
> +	tristate "NXP TJA11xx PHYs support"
> +	depends on HWMON
> +	---help---
> +	  Currently supports the NXP TJA1100 and TJA1101 PHY.
> +
>   config QSEMI_PHY
>   	tristate "Quality Semiconductor PHYs"
>   	---help---
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index 27d7f9f3b0de..bac339e09042 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -82,6 +82,7 @@ obj-$(CONFIG_MICROCHIP_PHY)	+= microchip.o
>   obj-$(CONFIG_MICROCHIP_T1_PHY)	+= microchip_t1.o
>   obj-$(CONFIG_MICROSEMI_PHY)	+= mscc.o
>   obj-$(CONFIG_NATIONAL_PHY)	+= national.o
> +obj-$(CONFIG_NXP_TJA11XX_PHY)	+= nxp-tja11xx.o
>   obj-$(CONFIG_QSEMI_PHY)		+= qsemi.o
>   obj-$(CONFIG_REALTEK_PHY)	+= realtek.o
>   obj-$(CONFIG_RENESAS_PHY)	+= uPD60620.o
> diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
> new file mode 100644
> index 000000000000..11b8701e78fd
> --- /dev/null
> +++ b/drivers/net/phy/nxp-tja11xx.c
> @@ -0,0 +1,423 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* NXP TJA1100 BroadRReach PHY driver
> + *
> + * Copyright (C) 2018 Marek Vasut <marex@denx.de>
> + */
> +#include <linux/delay.h>
> +#include <linux/ethtool.h>
> +#include <linux/kernel.h>
> +#include <linux/mii.h>
> +#include <linux/module.h>
> +#include <linux/phy.h>
> +#include <linux/hwmon.h>
> +#include <linux/bitfield.h>
> +
> +#define PHY_ID_MASK			0xfffffff0
> +#define PHY_ID_TJA1100			0x0180dc40
> +#define PHY_ID_TJA1101			0x0180dd00
> +
> +#define MII_ECTRL			17
> +#define MII_ECTRL_LINK_CONTROL		BIT(15)
> +#define MII_ECTRL_POWER_MODE_MASK	GENMASK(14, 11)
> +#define MII_ECTRL_POWER_MODE_NO_CHANGE	(0x0 << 11)
> +#define MII_ECTRL_POWER_MODE_NORMAL	(0x3 << 11)
> +#define MII_ECTRL_POWER_MODE_STANDBY	(0xc << 11)
> +#define MII_ECTRL_CONFIG_EN		BIT(2)
> +#define MII_ECTRL_WAKE_REQUEST		BIT(0)
> +
> +#define MII_CFG1			18
> +#define MII_CFG1_AUTO_OP		BIT(14)
> +#define MII_CFG1_SLEEP_CONFIRM		BIT(6)
> +#define MII_CFG1_LED_MODE_MASK		GENMASK(5, 4)
> +#define MII_CFG1_LED_MODE_LINKUP	0
> +#define MII_CFG1_LED_ENABLE		BIT(3)
> +
> +#define MII_CFG2			19
> +#define MII_CFG2_SLEEP_REQUEST_TO	GENMASK(1, 0)
> +#define MII_CFG2_SLEEP_REQUEST_TO_16MS	0x3
> +
> +#define MII_INTSRC			21
> +#define MII_INTSRC_TEMP_ERR		BIT(1)
> +#define MII_INTSRC_UV_ERR		BIT(3)
> +
> +#define MII_COMMSTAT			23
> +#define MII_COMMSTAT_LINK_UP		BIT(15)
> +
> +#define MII_GENSTAT			24
> +#define MII_GENSTAT_PLL_LOCKED		BIT(14)
> +
> +#define MII_COMMCFG			27
> +#define MII_COMMCFG_AUTO_OP		BIT(15)
> +
> +struct tja11xx_priv {
> +	char		*hwmon_name;
> +	struct device	*hwmon_dev;
> +};
> +
> +struct tja11xx_phy_stats {
> +	const char	*string;
> +	u8		reg;
> +	u8		off;
> +	u16		mask;
> +};
> +
> +static struct tja11xx_phy_stats tja11xx_hw_stats[] = {
> +	{ "phy_symbol_error_count", 20, 0, GENMASK(15, 0) },
> +	{ "phy_polarity_detect", 25, 6, BIT(6) },
> +	{ "phy_open_detect", 25, 7, BIT(7) },
> +	{ "phy_short_detect", 25, 8, BIT(8) },
> +	{ "phy_rem_rcvr_count", 26, 0, GENMASK(7, 0) },
> +	{ "phy_loc_rcvr_count", 26, 8, GENMASK(15, 8) },
> +};
> +
> +static int tja11xx_check(struct phy_device *phydev, u8 reg, u16 mask, u16 set)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < 200; i++) {
> +		ret = phy_read(phydev, reg);
> +		if (ret < 0)
> +			return ret;
> +
> +		if ((ret & mask) == set)
> +			return 0;
> +
> +		usleep_range(100, 150);
> +	}
> +
> +	return -ETIMEDOUT;
> +}
> +
> +static int phy_modify_check(struct phy_device *phydev, u8 reg,
> +			    u16 mask, u16 set)
> +{
> +	int ret;
> +
> +	ret = phy_modify(phydev, reg, mask, set);
> +	if (ret)
> +		return ret;
> +
> +	return tja11xx_check(phydev, reg, mask, set);
> +}
> +
> +static int tja11xx_enable_reg_write(struct phy_device *phydev)
> +{
> +	return phy_set_bits(phydev, MII_ECTRL, MII_ECTRL_CONFIG_EN);
> +}
> +
> +static int tja11xx_enable_link_control(struct phy_device *phydev)
> +{
> +	return phy_set_bits(phydev, MII_ECTRL, MII_ECTRL_LINK_CONTROL);
> +}
> +
> +static int tja11xx_wakeup(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = phy_read(phydev, MII_ECTRL);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (ret & MII_ECTRL_POWER_MODE_MASK) {
> +	case MII_ECTRL_POWER_MODE_NO_CHANGE:
> +		break;
> +	case MII_ECTRL_POWER_MODE_NORMAL:
> +		ret = phy_set_bits(phydev, MII_ECTRL, MII_ECTRL_WAKE_REQUEST);
> +		if (ret)
> +			return ret;
> +
> +		ret = phy_clear_bits(phydev, MII_ECTRL, MII_ECTRL_WAKE_REQUEST);
> +		if (ret)
> +			return ret;
> +		break;
> +	case MII_ECTRL_POWER_MODE_STANDBY:
> +		ret = phy_modify_check(phydev, MII_ECTRL,
> +				       MII_ECTRL_POWER_MODE_MASK,
> +				       MII_ECTRL_POWER_MODE_STANDBY);
> +		if (ret)
> +			return ret;
> +
> +		ret = phy_modify(phydev, MII_ECTRL, MII_ECTRL_POWER_MODE_MASK,
> +				 MII_ECTRL_POWER_MODE_NORMAL);
> +		if (ret)
> +			return ret;
> +
> +		ret = phy_modify_check(phydev, MII_GENSTAT,
> +				       MII_GENSTAT_PLL_LOCKED,
> +				       MII_GENSTAT_PLL_LOCKED);
> +		if (ret)
> +			return ret;
> +
> +		return tja11xx_enable_link_control(phydev);
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int tja11xx_soft_reset(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = tja11xx_enable_reg_write(phydev);
> +	if (ret)
> +		return ret;
> +
> +	return genphy_soft_reset(phydev);
> +}
> +
> +static int tja11xx_config_init(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = tja11xx_enable_reg_write(phydev);
> +	if (ret)
> +		return ret;
> +
> +	phydev->autoneg = AUTONEG_DISABLE;
> +	phydev->speed = SPEED_100;
> +	phydev->duplex = DUPLEX_FULL;
> +
> +	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);
> +		if (ret)
> +			return ret;
> +		break;
> +	case PHY_ID_TJA1101:
> +		ret = phy_set_bits(phydev, MII_COMMCFG, MII_COMMCFG_AUTO_OP);
> +		if (ret)
> +			return ret;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = phy_clear_bits(phydev, MII_CFG1, MII_CFG1_SLEEP_CONFIRM);
> +	if (ret)
> +		return ret;
> +
> +	ret = phy_modify(phydev, MII_CFG2, MII_CFG2_SLEEP_REQUEST_TO,
> +			 MII_CFG2_SLEEP_REQUEST_TO_16MS);
> +	if (ret)
> +		return ret;
> +
> +	ret = tja11xx_wakeup(phydev);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* ACK interrupts by reading the status register */
> +	ret = phy_read(phydev, MII_INTSRC);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int tja11xx_read_status(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = genphy_update_link(phydev);
> +	if (ret)
> +		return ret;
> +
> +	if (phydev->link) {
> +		ret = phy_read(phydev, MII_COMMSTAT);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (!(ret & MII_COMMSTAT_LINK_UP))
> +			phydev->link = 0;
> +	}
> +
> +	return 0;
> +}
> +
> +static int tja11xx_get_sset_count(struct phy_device *phydev)
> +{
> +	return ARRAY_SIZE(tja11xx_hw_stats);
> +}
> +
> +static void tja11xx_get_strings(struct phy_device *phydev, u8 *data)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(tja11xx_hw_stats); i++) {
> +		strncpy(data + i * ETH_GSTRING_LEN,
> +			tja11xx_hw_stats[i].string, ETH_GSTRING_LEN);
> +	}
> +}
> +
> +static void tja11xx_get_stats(struct phy_device *phydev,
> +			      struct ethtool_stats *stats, u64 *data)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(tja11xx_hw_stats); i++) {
> +		ret = phy_read(phydev, tja11xx_hw_stats[i].reg);
> +		if (ret < 0)
> +			data[i] = U64_MAX;
> +		else {
> +			data[i] = ret & tja11xx_hw_stats[i].mask;
> +			data[i] >>= tja11xx_hw_stats[i].off;
> +		}
> +	}
> +}
> +
> +static int tja11xx_hwmon_read(struct device *dev,
> +			      enum hwmon_sensor_types type,
> +			      u32 attr, int channel, long *value)
> +{
> +	struct phy_device *phydev = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (type == hwmon_in && attr == hwmon_in_lcrit_alarm) {
> +		ret = phy_read(phydev, MII_INTSRC);
> +		if (ret < 0)
> +			return ret;
> +
> +		*value = !!(ret & MII_INTSRC_TEMP_ERR);
> +		return 0;
> +	}
> +
> +	if (type == hwmon_temp && attr == hwmon_temp_crit_alarm) {
> +		ret = phy_read(phydev, MII_INTSRC);
> +		if (ret < 0)
> +			return ret;
> +
> +		*value = !!(ret & MII_INTSRC_UV_ERR);
> +		return 0;
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static umode_t tja11xx_hwmon_is_visible(const void *data,
> +					enum hwmon_sensor_types type,
> +					u32 attr, int channel)
> +{
> +	if (type == hwmon_in && attr == hwmon_in_lcrit_alarm)
> +		return 0444;
> +
> +	if (type == hwmon_temp && attr == hwmon_temp_crit_alarm)
> +		return 0444;
> +
> +	return 0;
> +}
> +
> +static u32 tja11xx_hwmon_in_config[] = {
> +	HWMON_I_LCRIT_ALARM,
> +	0
> +};
> +
> +static const struct hwmon_channel_info tja11xx_hwmon_in = {
> +	.type		= hwmon_in,
> +	.config		= tja11xx_hwmon_in_config,
> +};
> +
> +static u32 tja11xx_hwmon_temp_config[] = {
> +	HWMON_T_CRIT_ALARM,
> +	0
> +};
> +
> +static const struct hwmon_channel_info tja11xx_hwmon_temp = {
> +	.type		= hwmon_temp,
> +	.config		= tja11xx_hwmon_temp_config,
> +};
> +
> +static const struct hwmon_channel_info *tja11xx_hwmon_info[] = {
> +	&tja11xx_hwmon_in,
> +	&tja11xx_hwmon_temp,
> +	NULL
> +};
> +
You might want to consider using the new HWMON_CHANNEL_INFO() macro
to simplify above boilerplate code.

Guenter

> +static const struct hwmon_ops tja11xx_hwmon_hwmon_ops = {
> +	.is_visible	= tja11xx_hwmon_is_visible,
> +	.read		= tja11xx_hwmon_read,
> +};
> +
> +static const struct hwmon_chip_info tja11xx_hwmon_chip_info = {
> +	.ops		= &tja11xx_hwmon_hwmon_ops,
> +	.info		= tja11xx_hwmon_info,
> +};
> +
> +static int tja11xx_probe(struct phy_device *phydev)
> +{
> +	struct device *dev = &phydev->mdio.dev;
> +	struct tja11xx_priv *priv;
> +	int i;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->hwmon_name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
> +	if (!priv->hwmon_name)
> +		return -ENOMEM;
> +
> +	for (i = 0; priv->hwmon_name[i]; i++)
> +		if (hwmon_is_bad_char(priv->hwmon_name[i]))
> +			priv->hwmon_name[i] = '_';
> +
> +	priv->hwmon_dev =
> +		devm_hwmon_device_register_with_info(dev, priv->hwmon_name,
> +						     phydev,
> +						     &tja11xx_hwmon_chip_info,
> +						     NULL);
> +
> +	return PTR_ERR_OR_ZERO(priv->hwmon_dev);
> +}
> +
> +static struct phy_driver tja11xx_driver[] = {
> +	{
> +		PHY_ID_MATCH_MODEL(PHY_ID_TJA1100),
> +		.name		= "NXP TJA1100",
> +		.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,
> +	}, {
> +		PHY_ID_MATCH_MODEL(PHY_ID_TJA1101),
> +		.name		= "NXP TJA1101",
> +		.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,
> +	}
> +};
> +
> +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) },
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(mdio, tja11xx_tbl);
> +
> +MODULE_AUTHOR("Marek Vasut <marex@denx.de>");
> +MODULE_DESCRIPTION("NXP TJA11xx BoardR-Reach PHY driver");
> +MODULE_LICENSE("GPL");
>

Marek Vasut May 28, 2019, 6:17 p.m. UTC | #16

On 5/27/19 5:44 PM, Guenter Roeck wrote:
> On 5/17/19 4:51 PM, Marek Vasut wrote:
>> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
>> BroadRReach 100BaseT1 PHYs used in automotive.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Andrew Lunn <andrew@lunn.ch>
>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>> Cc: Guenter Roeck <linux@roeck-us.net>
>> Cc: Heiner Kallweit <hkallweit1@gmail.com>
>> Cc: Jean Delvare <jdelvare@suse.com>
>> Cc: linux-hwmon@vger.kernel.org

[...]

>> +static u32 tja11xx_hwmon_in_config[] = {
>> +    HWMON_I_LCRIT_ALARM,
>> +    0
>> +};
>> +
>> +static const struct hwmon_channel_info tja11xx_hwmon_in = {
>> +    .type        = hwmon_in,
>> +    .config        = tja11xx_hwmon_in_config,
>> +};
>> +
>> +static u32 tja11xx_hwmon_temp_config[] = {
>> +    HWMON_T_CRIT_ALARM,
>> +    0
>> +};
>> +
>> +static const struct hwmon_channel_info tja11xx_hwmon_temp = {
>> +    .type        = hwmon_temp,
>> +    .config        = tja11xx_hwmon_temp_config,
>> +};
>> +
>> +static const struct hwmon_channel_info *tja11xx_hwmon_info[] = {
>> +    &tja11xx_hwmon_in,
>> +    &tja11xx_hwmon_temp,
>> +    NULL
>> +};
>> +
> You might want to consider using the new HWMON_CHANNEL_INFO() macro
> to simplify above boilerplate code.
Patch posted

[...]

Andrew Lunn May 28, 2019, 7:34 p.m. UTC | #17

On Mon, May 27, 2019 at 08:44:24AM -0700, Guenter Roeck wrote:
> >+static u32 tja11xx_hwmon_in_config[] = {
> >+	HWMON_I_LCRIT_ALARM,
> >+	0
> >+};
> >+
> >+static const struct hwmon_channel_info tja11xx_hwmon_in = {
> >+	.type		= hwmon_in,
> >+	.config		= tja11xx_hwmon_in_config,
> >+};
> >+
> >+static u32 tja11xx_hwmon_temp_config[] = {
> >+	HWMON_T_CRIT_ALARM,
> >+	0
> >+};
> >+
> >+static const struct hwmon_channel_info tja11xx_hwmon_temp = {
> >+	.type		= hwmon_temp,
> >+	.config		= tja11xx_hwmon_temp_config,
> >+};
> >+
> >+static const struct hwmon_channel_info *tja11xx_hwmon_info[] = {
> >+	&tja11xx_hwmon_in,
> >+	&tja11xx_hwmon_temp,
> >+	NULL
> >+};
> >+
> You might want to consider using the new HWMON_CHANNEL_INFO() macro
> to simplify above boilerplate code.

Hi Guenter

That is a nice simplification. Could you run the semantic patch
in drivers/net/phy and submit the results.

Thanks
	Andrew

[V5] net: phy: tja11xx: Add TJA11xx PHY driver

Commit Message

Comments

Patch