[V2] net: phy: tja11xx: Add IRQ support to the driver

Commit Message

Marek Vasut May 28, 2019, 7:23 p.m. UTC

Add support for handling the TJA11xx PHY IRQ signal.

Signed-off-by: Marek Vasut <[e-mail]>
Cc: Andrew Lunn <[e-mail]>
Cc: Florian Fainelli <[e-mail]>
Cc: Guenter Roeck <[e-mail]>
Cc: Heiner Kallweit <[e-mail]>
Cc: Jean Delvare <[e-mail]>
Cc: [e-mail]
---
V2: - Define each bit of the MII_INTEN register and a mask
    - Drop IRQ acking from tja11xx_config_intr()
---
 drivers/net/phy/nxp-tja11xx.c | 48 +++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

Comments

Heiner Kallweit May 28, 2019, 7:28 p.m. UTC | #1

On 28.05.2019 21:23, Marek Vasut wrote:
> Add support for handling the TJA11xx PHY IRQ signal.
> 
> Signed-off-by: Marek Vasut <[e-mail]>
> Cc: Andrew Lunn <[e-mail]>
> Cc: Florian Fainelli <[e-mail]>
> Cc: Guenter Roeck <[e-mail]>
> Cc: Heiner Kallweit <[e-mail]>
> Cc: Jean Delvare <[e-mail]>
> Cc: [e-mail]
> ---
> V2: - Define each bit of the MII_INTEN register and a mask
>     - Drop IRQ acking from tja11xx_config_intr()
> ---
>  drivers/net/phy/nxp-tja11xx.c | 48 +++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
> index b705d0bd798b..b41af609607d 100644
> --- a/drivers/net/phy/nxp-tja11xx.c
> +++ b/drivers/net/phy/nxp-tja11xx.c
> @@ -40,6 +40,29 @@
>  #define MII_INTSRC_TEMP_ERR		BIT(1)
>  #define MII_INTSRC_UV_ERR		BIT(3)
>  
> +#define MII_INTEN			22
> +#define MII_INTEN_PWON_EN		BIT(15)
> +#define MII_INTEN_WAKEUP_EN		BIT(14)
> +#define MII_INTEN_PHY_INIT_FAIL_EN	BIT(11)
> +#define MII_INTEN_LINK_STATUS_FAIL_EN	BIT(10)
> +#define MII_INTEN_LINK_STATUS_UP_EN	BIT(9)
> +#define MII_INTEN_SYM_ERR_EN		BIT(8)
> +#define MII_INTEN_TRAINING_FAILED_EN	BIT(7)
> +#define MII_INTEN_SQI_WARNING_EN	BIT(6)
> +#define MII_INTEN_CONTROL_ERR_EN	BIT(5)
> +#define MII_INTEN_UV_ERR_EN		BIT(3)
> +#define MII_INTEN_UV_RECOVERY_EN	BIT(2)
> +#define MII_INTEN_TEMP_ERR_EN		BIT(1)
> +#define MII_INTEN_SLEEP_ABORT_EN	BIT(0)
> +#define MII_INTEN_MASK							\
> +	(MII_INTEN_PWON_EN | MII_INTEN_WAKEUP_EN |			\
> +	MII_INTEN_PHY_INIT_FAIL_EN | MII_INTEN_LINK_STATUS_FAIL_EN |	\
> +	MII_INTEN_LINK_STATUS_UP_EN | MII_INTEN_SYM_ERR_EN |		\
> +	MII_INTEN_TRAINING_FAILED_EN | MII_INTEN_SQI_WARNING_EN |	\
> +	MII_INTEN_CONTROL_ERR_EN | MII_INTEN_UV_ERR_EN |		\
> +	MII_INTEN_UV_RECOVERY_EN | MII_INTEN_TEMP_ERR_EN |		\
> +	MII_INTEN_SLEEP_ABORT_EN)

Why do you enable all these interrupt sources? As I said, phylib needs
link change info only.

> +
>  #define MII_COMMSTAT			23
>  #define MII_COMMSTAT_LINK_UP		BIT(15)
>  
> @@ -239,6 +262,25 @@ static int tja11xx_read_status(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int tja11xx_config_intr(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
> +		ret = phy_write(phydev, MII_INTEN, MII_INTEN_MASK);
> +	else
> +		ret = phy_write(phydev, MII_INTEN, 0);
> +
> +	return ret < 0 ? ret : 0;

phy_write returns only 0 or negative errno. You don't need
variable ret.

> +}
> +
> +static int tja11xx_ack_interrupt(struct phy_device *phydev)
> +{
> +	int ret = phy_read(phydev, MII_INTSRC);
> +
> +	return ret < 0 ? ret : 0;
> +}
> +
>  static int tja11xx_get_sset_count(struct phy_device *phydev)
>  {
>  	return ARRAY_SIZE(tja11xx_hw_stats);
> @@ -366,6 +408,9 @@ static struct phy_driver tja11xx_driver[] = {
>  		.suspend	= genphy_suspend,
>  		.resume		= genphy_resume,
>  		.set_loopback   = genphy_loopback,
> +		/* IRQ related */
> +		.config_intr	= tja11xx_config_intr,
> +		.ack_interrupt	= tja11xx_ack_interrupt,
>  		/* Statistics */
>  		.get_sset_count = tja11xx_get_sset_count,
>  		.get_strings	= tja11xx_get_strings,
> @@ -381,6 +426,9 @@ static struct phy_driver tja11xx_driver[] = {
>  		.suspend	= genphy_suspend,
>  		.resume		= genphy_resume,
>  		.set_loopback   = genphy_loopback,
> +		/* IRQ related */
> +		.config_intr	= tja11xx_config_intr,
> +		.ack_interrupt	= tja11xx_ack_interrupt,
>  		/* Statistics */
>  		.get_sset_count = tja11xx_get_sset_count,
>  		.get_strings	= tja11xx_get_strings,
>

Marek Vasut May 28, 2019, 7:31 p.m. UTC | #2

On 5/28/19 9:28 PM, Heiner Kallweit wrote:
> On 28.05.2019 21:23, Marek Vasut wrote:
>> Add support for handling the TJA11xx PHY IRQ signal.
>>
>> Signed-off-by: Marek Vasut <[e-mail]>
>> Cc: Andrew Lunn <[e-mail]>
>> Cc: Florian Fainelli <[e-mail]>
>> Cc: Guenter Roeck <[e-mail]>
>> Cc: Heiner Kallweit <[e-mail]>
>> Cc: Jean Delvare <[e-mail]>
>> Cc: [e-mail]
>> ---
>> V2: - Define each bit of the MII_INTEN register and a mask
>>     - Drop IRQ acking from tja11xx_config_intr()
>> ---
>>  drivers/net/phy/nxp-tja11xx.c | 48 +++++++++++++++++++++++++++++++++++
>>  1 file changed, 48 insertions(+)
>>
>> diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
>> index b705d0bd798b..b41af609607d 100644
>> --- a/drivers/net/phy/nxp-tja11xx.c
>> +++ b/drivers/net/phy/nxp-tja11xx.c
>> @@ -40,6 +40,29 @@
>>  #define MII_INTSRC_TEMP_ERR		BIT(1)
>>  #define MII_INTSRC_UV_ERR		BIT(3)
>>  
>> +#define MII_INTEN			22
>> +#define MII_INTEN_PWON_EN		BIT(15)
>> +#define MII_INTEN_WAKEUP_EN		BIT(14)
>> +#define MII_INTEN_PHY_INIT_FAIL_EN	BIT(11)
>> +#define MII_INTEN_LINK_STATUS_FAIL_EN	BIT(10)
>> +#define MII_INTEN_LINK_STATUS_UP_EN	BIT(9)
>> +#define MII_INTEN_SYM_ERR_EN		BIT(8)
>> +#define MII_INTEN_TRAINING_FAILED_EN	BIT(7)
>> +#define MII_INTEN_SQI_WARNING_EN	BIT(6)
>> +#define MII_INTEN_CONTROL_ERR_EN	BIT(5)
>> +#define MII_INTEN_UV_ERR_EN		BIT(3)
>> +#define MII_INTEN_UV_RECOVERY_EN	BIT(2)
>> +#define MII_INTEN_TEMP_ERR_EN		BIT(1)
>> +#define MII_INTEN_SLEEP_ABORT_EN	BIT(0)
>> +#define MII_INTEN_MASK							\
>> +	(MII_INTEN_PWON_EN | MII_INTEN_WAKEUP_EN |			\
>> +	MII_INTEN_PHY_INIT_FAIL_EN | MII_INTEN_LINK_STATUS_FAIL_EN |	\
>> +	MII_INTEN_LINK_STATUS_UP_EN | MII_INTEN_SYM_ERR_EN |		\
>> +	MII_INTEN_TRAINING_FAILED_EN | MII_INTEN_SQI_WARNING_EN |	\
>> +	MII_INTEN_CONTROL_ERR_EN | MII_INTEN_UV_ERR_EN |		\
>> +	MII_INTEN_UV_RECOVERY_EN | MII_INTEN_TEMP_ERR_EN |		\
>> +	MII_INTEN_SLEEP_ABORT_EN)
> 
> Why do you enable all these interrupt sources? As I said, phylib needs
> link change info only.

Because I need them to reliably detect that the link state changed.

>> +
>>  #define MII_COMMSTAT			23
>>  #define MII_COMMSTAT_LINK_UP		BIT(15)
>>  
>> @@ -239,6 +262,25 @@ static int tja11xx_read_status(struct phy_device *phydev)
>>  	return 0;
>>  }
>>  
>> +static int tja11xx_config_intr(struct phy_device *phydev)
>> +{
>> +	int ret;
>> +
>> +	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
>> +		ret = phy_write(phydev, MII_INTEN, MII_INTEN_MASK);
>> +	else
>> +		ret = phy_write(phydev, MII_INTEN, 0);
>> +
>> +	return ret < 0 ? ret : 0;
> 
> phy_write returns only 0 or negative errno. You don't need
> variable ret.

OK

Heiner Kallweit May 28, 2019, 7:35 p.m. UTC | #3

On 28.05.2019 21:31, Marek Vasut wrote:
> On 5/28/19 9:28 PM, Heiner Kallweit wrote:
>> On 28.05.2019 21:23, Marek Vasut wrote:
>>> Add support for handling the TJA11xx PHY IRQ signal.
>>>
>>> Signed-off-by: Marek Vasut <[e-mail]>
>>> Cc: Andrew Lunn <[e-mail]>
>>> Cc: Florian Fainelli <[e-mail]>
>>> Cc: Guenter Roeck <[e-mail]>
>>> Cc: Heiner Kallweit <[e-mail]>
>>> Cc: Jean Delvare <[e-mail]>
>>> Cc: [e-mail]
>>> ---
>>> V2: - Define each bit of the MII_INTEN register and a mask
>>>     - Drop IRQ acking from tja11xx_config_intr()
>>> ---
>>>  drivers/net/phy/nxp-tja11xx.c | 48 +++++++++++++++++++++++++++++++++++
>>>  1 file changed, 48 insertions(+)
>>>
>>> diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
>>> index b705d0bd798b..b41af609607d 100644
>>> --- a/drivers/net/phy/nxp-tja11xx.c
>>> +++ b/drivers/net/phy/nxp-tja11xx.c
>>> @@ -40,6 +40,29 @@
>>>  #define MII_INTSRC_TEMP_ERR		BIT(1)
>>>  #define MII_INTSRC_UV_ERR		BIT(3)
>>>  
>>> +#define MII_INTEN			22
>>> +#define MII_INTEN_PWON_EN		BIT(15)
>>> +#define MII_INTEN_WAKEUP_EN		BIT(14)
>>> +#define MII_INTEN_PHY_INIT_FAIL_EN	BIT(11)
>>> +#define MII_INTEN_LINK_STATUS_FAIL_EN	BIT(10)
>>> +#define MII_INTEN_LINK_STATUS_UP_EN	BIT(9)
>>> +#define MII_INTEN_SYM_ERR_EN		BIT(8)
>>> +#define MII_INTEN_TRAINING_FAILED_EN	BIT(7)
>>> +#define MII_INTEN_SQI_WARNING_EN	BIT(6)
>>> +#define MII_INTEN_CONTROL_ERR_EN	BIT(5)
>>> +#define MII_INTEN_UV_ERR_EN		BIT(3)
>>> +#define MII_INTEN_UV_RECOVERY_EN	BIT(2)
>>> +#define MII_INTEN_TEMP_ERR_EN		BIT(1)
>>> +#define MII_INTEN_SLEEP_ABORT_EN	BIT(0)
>>> +#define MII_INTEN_MASK							\
>>> +	(MII_INTEN_PWON_EN | MII_INTEN_WAKEUP_EN |			\
>>> +	MII_INTEN_PHY_INIT_FAIL_EN | MII_INTEN_LINK_STATUS_FAIL_EN |	\
>>> +	MII_INTEN_LINK_STATUS_UP_EN | MII_INTEN_SYM_ERR_EN |		\
>>> +	MII_INTEN_TRAINING_FAILED_EN | MII_INTEN_SQI_WARNING_EN |	\
>>> +	MII_INTEN_CONTROL_ERR_EN | MII_INTEN_UV_ERR_EN |		\
>>> +	MII_INTEN_UV_RECOVERY_EN | MII_INTEN_TEMP_ERR_EN |		\
>>> +	MII_INTEN_SLEEP_ABORT_EN)
>>
>> Why do you enable all these interrupt sources? As I said, phylib needs
>> link change info only.
> 
> Because I need them to reliably detect that the link state changed.
> 

Hmm, e.g. this one MII_INTEN_TEMP_ERR_EN doesn't seem to be related
to a link status change. Name sounds like it just reports exceeding
a temperature threshold.

>>> +
>>>  #define MII_COMMSTAT			23
>>>  #define MII_COMMSTAT_LINK_UP		BIT(15)
>>>  
>>> @@ -239,6 +262,25 @@ static int tja11xx_read_status(struct phy_device *phydev)
>>>  	return 0;
>>>  }
>>>  
>>> +static int tja11xx_config_intr(struct phy_device *phydev)
>>> +{
>>> +	int ret;
>>> +
>>> +	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
>>> +		ret = phy_write(phydev, MII_INTEN, MII_INTEN_MASK);
>>> +	else
>>> +		ret = phy_write(phydev, MII_INTEN, 0);
>>> +
>>> +	return ret < 0 ? ret : 0;
>>
>> phy_write returns only 0 or negative errno. You don't need
>> variable ret.
> 
> OK
>

Marek Vasut May 28, 2019, 7:46 p.m. UTC | #4

On 5/28/19 9:35 PM, Heiner Kallweit wrote:
> On 28.05.2019 21:31, Marek Vasut wrote:
>> On 5/28/19 9:28 PM, Heiner Kallweit wrote:
>>> On 28.05.2019 21:23, Marek Vasut wrote:
>>>> Add support for handling the TJA11xx PHY IRQ signal.
>>>>
>>>> Signed-off-by: Marek Vasut <[e-mail]>
>>>> Cc: Andrew Lunn <[e-mail]>
>>>> Cc: Florian Fainelli <[e-mail]>
>>>> Cc: Guenter Roeck <[e-mail]>
>>>> Cc: Heiner Kallweit <[e-mail]>
>>>> Cc: Jean Delvare <[e-mail]>
>>>> Cc: [e-mail]
>>>> ---
>>>> V2: - Define each bit of the MII_INTEN register and a mask
>>>>     - Drop IRQ acking from tja11xx_config_intr()
>>>> ---
>>>>  drivers/net/phy/nxp-tja11xx.c | 48 +++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 48 insertions(+)
>>>>
>>>> diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
>>>> index b705d0bd798b..b41af609607d 100644
>>>> --- a/drivers/net/phy/nxp-tja11xx.c
>>>> +++ b/drivers/net/phy/nxp-tja11xx.c
>>>> @@ -40,6 +40,29 @@
>>>>  #define MII_INTSRC_TEMP_ERR		BIT(1)
>>>>  #define MII_INTSRC_UV_ERR		BIT(3)
>>>>  
>>>> +#define MII_INTEN			22
>>>> +#define MII_INTEN_PWON_EN		BIT(15)
>>>> +#define MII_INTEN_WAKEUP_EN		BIT(14)
>>>> +#define MII_INTEN_PHY_INIT_FAIL_EN	BIT(11)
>>>> +#define MII_INTEN_LINK_STATUS_FAIL_EN	BIT(10)
>>>> +#define MII_INTEN_LINK_STATUS_UP_EN	BIT(9)
>>>> +#define MII_INTEN_SYM_ERR_EN		BIT(8)
>>>> +#define MII_INTEN_TRAINING_FAILED_EN	BIT(7)
>>>> +#define MII_INTEN_SQI_WARNING_EN	BIT(6)
>>>> +#define MII_INTEN_CONTROL_ERR_EN	BIT(5)
>>>> +#define MII_INTEN_UV_ERR_EN		BIT(3)
>>>> +#define MII_INTEN_UV_RECOVERY_EN	BIT(2)
>>>> +#define MII_INTEN_TEMP_ERR_EN		BIT(1)
>>>> +#define MII_INTEN_SLEEP_ABORT_EN	BIT(0)
>>>> +#define MII_INTEN_MASK							\
>>>> +	(MII_INTEN_PWON_EN | MII_INTEN_WAKEUP_EN |			\
>>>> +	MII_INTEN_PHY_INIT_FAIL_EN | MII_INTEN_LINK_STATUS_FAIL_EN |	\
>>>> +	MII_INTEN_LINK_STATUS_UP_EN | MII_INTEN_SYM_ERR_EN |		\
>>>> +	MII_INTEN_TRAINING_FAILED_EN | MII_INTEN_SQI_WARNING_EN |	\
>>>> +	MII_INTEN_CONTROL_ERR_EN | MII_INTEN_UV_ERR_EN |		\
>>>> +	MII_INTEN_UV_RECOVERY_EN | MII_INTEN_TEMP_ERR_EN |		\
>>>> +	MII_INTEN_SLEEP_ABORT_EN)
>>>
>>> Why do you enable all these interrupt sources? As I said, phylib needs
>>> link change info only.
>>
>> Because I need them to reliably detect that the link state changed.
>>
> 
> Hmm, e.g. this one MII_INTEN_TEMP_ERR_EN doesn't seem to be related
> to a link status change. Name sounds like it just reports exceeding
> a temperature threshold.

It's PHY over-temperature. Whether it tears the link down or not is not
clear.

Andrew Lunn May 28, 2019, 7:58 p.m. UTC | #5

On Tue, May 28, 2019 at 09:46:47PM +0200, Marek Vasut wrote:
> On 5/28/19 9:35 PM, Heiner Kallweit wrote:
> > On 28.05.2019 21:31, Marek Vasut wrote:
> >> On 5/28/19 9:28 PM, Heiner Kallweit wrote:
> >>> On 28.05.2019 21:23, Marek Vasut wrote:
> >>>> Add support for handling the TJA11xx PHY IRQ signal.
> >>>>
> >>>> Signed-off-by: Marek Vasut <[e-mail]>
> >>>> Cc: Andrew Lunn <[e-mail]>
> >>>> Cc: Florian Fainelli <[e-mail]>
> >>>> Cc: Guenter Roeck <[e-mail]>
> >>>> Cc: Heiner Kallweit <[e-mail]>
> >>>> Cc: Jean Delvare <[e-mail]>
> >>>> Cc: [e-mail]
> >>>> ---
> >>>> V2: - Define each bit of the MII_INTEN register and a mask
> >>>>     - Drop IRQ acking from tja11xx_config_intr()
> >>>> ---
> >>>>  drivers/net/phy/nxp-tja11xx.c | 48 +++++++++++++++++++++++++++++++++++
> >>>>  1 file changed, 48 insertions(+)
> >>>>
> >>>> diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
> >>>> index b705d0bd798b..b41af609607d 100644
> >>>> --- a/drivers/net/phy/nxp-tja11xx.c
> >>>> +++ b/drivers/net/phy/nxp-tja11xx.c
 > >>>> @@ -40,6 +40,29 @@
> >>>>  #define MII_INTSRC_TEMP_ERR		BIT(1)
> >>>>  #define MII_INTSRC_UV_ERR		BIT(3)
> >>>>  
> >>>> +#define MII_INTEN			22
> >>>> +#define MII_INTEN_PWON_EN		BIT(15)
> >>>> +#define MII_INTEN_WAKEUP_EN		BIT(14)
> >>>> +#define MII_INTEN_PHY_INIT_FAIL_EN	BIT(11)
> >>>> +#define MII_INTEN_LINK_STATUS_FAIL_EN	BIT(10)
> >>>> +#define MII_INTEN_LINK_STATUS_UP_EN	BIT(9)
> >>>> +#define MII_INTEN_SYM_ERR_EN		BIT(8)
> >>>> +#define MII_INTEN_TRAINING_FAILED_EN	BIT(7)
> >>>> +#define MII_INTEN_SQI_WARNING_EN	BIT(6)
> >>>> +#define MII_INTEN_CONTROL_ERR_EN	BIT(5)
> >>>> +#define MII_INTEN_UV_ERR_EN		BIT(3)
> >>>> +#define MII_INTEN_UV_RECOVERY_EN	BIT(2)
> >>>> +#define MII_INTEN_TEMP_ERR_EN		BIT(1)
> >>>> +#define MII_INTEN_SLEEP_ABORT_EN	BIT(0)
> >>>> +#define MII_INTEN_MASK							\
> >>>> +	(MII_INTEN_PWON_EN | MII_INTEN_WAKEUP_EN |			\
> >>>> +	MII_INTEN_PHY_INIT_FAIL_EN | MII_INTEN_LINK_STATUS_FAIL_EN |	\
> >>>> +	MII_INTEN_LINK_STATUS_UP_EN | MII_INTEN_SYM_ERR_EN |		\
> >>>> +	MII_INTEN_TRAINING_FAILED_EN | MII_INTEN_SQI_WARNING_EN |	\
> >>>> +	MII_INTEN_CONTROL_ERR_EN | MII_INTEN_UV_ERR_EN |		\
> >>>> +	MII_INTEN_UV_RECOVERY_EN | MII_INTEN_TEMP_ERR_EN |		\
> >>>> +	MII_INTEN_SLEEP_ABORT_EN)
> >>>
> >>> Why do you enable all these interrupt sources? As I said, phylib needs
> >>> link change info only.
> >>
> >> Because I need them to reliably detect that the link state changed.

Hi Marek

That statement suggests you started with just bits 10 and 9 and it
failed to detect some sort of link up/down event? What was missed?

       Andrew

Marek Vasut May 28, 2019, 8 p.m. UTC | #6

On 5/28/19 9:58 PM, Andrew Lunn wrote:
> On Tue, May 28, 2019 at 09:46:47PM +0200, Marek Vasut wrote:
>> On 5/28/19 9:35 PM, Heiner Kallweit wrote:
>>> On 28.05.2019 21:31, Marek Vasut wrote:
>>>> On 5/28/19 9:28 PM, Heiner Kallweit wrote:
>>>>> On 28.05.2019 21:23, Marek Vasut wrote:
>>>>>> Add support for handling the TJA11xx PHY IRQ signal.
>>>>>>
>>>>>> Signed-off-by: Marek Vasut <[e-mail]>
>>>>>> Cc: Andrew Lunn <[e-mail]>
>>>>>> Cc: Florian Fainelli <[e-mail]>
>>>>>> Cc: Guenter Roeck <[e-mail]>
>>>>>> Cc: Heiner Kallweit <[e-mail]>
>>>>>> Cc: Jean Delvare <[e-mail]>
>>>>>> Cc: [e-mail]
>>>>>> ---
>>>>>> V2: - Define each bit of the MII_INTEN register and a mask
>>>>>>     - Drop IRQ acking from tja11xx_config_intr()
>>>>>> ---
>>>>>>  drivers/net/phy/nxp-tja11xx.c | 48 +++++++++++++++++++++++++++++++++++
>>>>>>  1 file changed, 48 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
>>>>>> index b705d0bd798b..b41af609607d 100644
>>>>>> --- a/drivers/net/phy/nxp-tja11xx.c
>>>>>> +++ b/drivers/net/phy/nxp-tja11xx.c
>  > >>>> @@ -40,6 +40,29 @@
>>>>>>  #define MII_INTSRC_TEMP_ERR		BIT(1)
>>>>>>  #define MII_INTSRC_UV_ERR		BIT(3)
>>>>>>  
>>>>>> +#define MII_INTEN			22
>>>>>> +#define MII_INTEN_PWON_EN		BIT(15)
>>>>>> +#define MII_INTEN_WAKEUP_EN		BIT(14)
>>>>>> +#define MII_INTEN_PHY_INIT_FAIL_EN	BIT(11)
>>>>>> +#define MII_INTEN_LINK_STATUS_FAIL_EN	BIT(10)
>>>>>> +#define MII_INTEN_LINK_STATUS_UP_EN	BIT(9)
>>>>>> +#define MII_INTEN_SYM_ERR_EN		BIT(8)
>>>>>> +#define MII_INTEN_TRAINING_FAILED_EN	BIT(7)
>>>>>> +#define MII_INTEN_SQI_WARNING_EN	BIT(6)
>>>>>> +#define MII_INTEN_CONTROL_ERR_EN	BIT(5)
>>>>>> +#define MII_INTEN_UV_ERR_EN		BIT(3)
>>>>>> +#define MII_INTEN_UV_RECOVERY_EN	BIT(2)
>>>>>> +#define MII_INTEN_TEMP_ERR_EN		BIT(1)
>>>>>> +#define MII_INTEN_SLEEP_ABORT_EN	BIT(0)
>>>>>> +#define MII_INTEN_MASK							\
>>>>>> +	(MII_INTEN_PWON_EN | MII_INTEN_WAKEUP_EN |			\
>>>>>> +	MII_INTEN_PHY_INIT_FAIL_EN | MII_INTEN_LINK_STATUS_FAIL_EN |	\
>>>>>> +	MII_INTEN_LINK_STATUS_UP_EN | MII_INTEN_SYM_ERR_EN |		\
>>>>>> +	MII_INTEN_TRAINING_FAILED_EN | MII_INTEN_SQI_WARNING_EN |	\
>>>>>> +	MII_INTEN_CONTROL_ERR_EN | MII_INTEN_UV_ERR_EN |		\
>>>>>> +	MII_INTEN_UV_RECOVERY_EN | MII_INTEN_TEMP_ERR_EN |		\
>>>>>> +	MII_INTEN_SLEEP_ABORT_EN)
>>>>>
>>>>> Why do you enable all these interrupt sources? As I said, phylib needs
>>>>> link change info only.
>>>>
>>>> Because I need them to reliably detect that the link state changed.
> 
> Hi Marek
> 
> That statement suggests you started with just bits 10 and 9 and it
> failed to detect some sort of link up/down event? What was missed?

The link detection on the TJA1100 (not TJA1101) seems unstable at best,
so I better use all the interrupt sources to nudge the PHY subsystem and
have it check the link change.

Andrew Lunn May 28, 2019, 9:22 p.m. UTC | #7

> The link detection on the TJA1100 (not TJA1101) seems unstable at best,
> so I better use all the interrupt sources to nudge the PHY subsystem and
> have it check the link change.

Then it sounds like you should just ignore interrupts and stay will
polling for the TJA1100.

	Andrew

Marek Vasut May 28, 2019, 9:33 p.m. UTC | #8

On 5/28/19 11:22 PM, Andrew Lunn wrote:
>> The link detection on the TJA1100 (not TJA1101) seems unstable at best,
>> so I better use all the interrupt sources to nudge the PHY subsystem and
>> have it check the link change.
> 
> Then it sounds like you should just ignore interrupts and stay will
> polling for the TJA1100.

Polling for the link status change is slow(er) than the IRQ driven
operation, so I would much rather use the interrupts.

Andrew Lunn May 29, 2019, 11:29 p.m. UTC | #9

On Tue, May 28, 2019 at 11:33:33PM +0200, Marek Vasut wrote:
> On 5/28/19 11:22 PM, Andrew Lunn wrote:
> >> The link detection on the TJA1100 (not TJA1101) seems unstable at best,
> >> so I better use all the interrupt sources to nudge the PHY subsystem and
> >> have it check the link change.
> > 
> > Then it sounds like you should just ignore interrupts and stay will
> > polling for the TJA1100.
> 
> Polling for the link status change is slow(er) than the IRQ driven
> operation, so I would much rather use the interrupts.

I agree about the speed, but it seems like interrupts on this PHY are
not so reliable. Polling always works. But unfortunately, you cannot
have both interrupts and polling to fix up problems when interrupts
fail. Your call, do you think interrupts really do work?

If you say that tja1101 works as expected, then please just use the
link up/down bits for it.

     Andrew

Marek Vasut May 29, 2019, 11:46 p.m. UTC | #10

On 5/30/19 1:29 AM, Andrew Lunn wrote:
> On Tue, May 28, 2019 at 11:33:33PM +0200, Marek Vasut wrote:
>> On 5/28/19 11:22 PM, Andrew Lunn wrote:
>>>> The link detection on the TJA1100 (not TJA1101) seems unstable at best,
>>>> so I better use all the interrupt sources to nudge the PHY subsystem and
>>>> have it check the link change.
>>>
>>> Then it sounds like you should just ignore interrupts and stay will
>>> polling for the TJA1100.
>>
>> Polling for the link status change is slow(er) than the IRQ driven
>> operation, so I would much rather use the interrupts.
> 
> I agree about the speed, but it seems like interrupts on this PHY are
> not so reliable. Polling always works. But unfortunately, you cannot
> have both interrupts and polling to fix up problems when interrupts
> fail. Your call, do you think interrupts really do work?

It works fine for me this way. And mind you, it's only the TJA1100
that's flaky, the TJA1101 is better.

> If you say that tja1101 works as expected, then please just use the
> link up/down bits for it.

I still don't know which bits really trigger link status changes, so I'd
like to play it safe and just trigger on all of them.

Marek Vasut June 13, 2019, 3:42 p.m. UTC | #11

On 5/30/19 1:46 AM, Marek Vasut wrote:
> On 5/30/19 1:29 AM, Andrew Lunn wrote:
>> On Tue, May 28, 2019 at 11:33:33PM +0200, Marek Vasut wrote:
>>> On 5/28/19 11:22 PM, Andrew Lunn wrote:
>>>>> The link detection on the TJA1100 (not TJA1101) seems unstable at best,
>>>>> so I better use all the interrupt sources to nudge the PHY subsystem and
>>>>> have it check the link change.
>>>>
>>>> Then it sounds like you should just ignore interrupts and stay will
>>>> polling for the TJA1100.
>>>
>>> Polling for the link status change is slow(er) than the IRQ driven
>>> operation, so I would much rather use the interrupts.
>>
>> I agree about the speed, but it seems like interrupts on this PHY are
>> not so reliable. Polling always works. But unfortunately, you cannot
>> have both interrupts and polling to fix up problems when interrupts
>> fail. Your call, do you think interrupts really do work?
> 
> It works fine for me this way. And mind you, it's only the TJA1100
> that's flaky, the TJA1101 is better.
> 
>> If you say that tja1101 works as expected, then please just use the
>> link up/down bits for it.
> 
> I still don't know which bits really trigger link status changes, so I'd
> like to play it safe and just trigger on all of them.

So what do we do here ?

Andrew Lunn June 17, 2019, 5:16 p.m. UTC | #12

On Thu, Jun 13, 2019 at 05:42:53PM +0200, Marek Vasut wrote:
> On 5/30/19 1:46 AM, Marek Vasut wrote:
> > On 5/30/19 1:29 AM, Andrew Lunn wrote:
> >> On Tue, May 28, 2019 at 11:33:33PM +0200, Marek Vasut wrote:
> >>> On 5/28/19 11:22 PM, Andrew Lunn wrote:
> >>>>> The link detection on the TJA1100 (not TJA1101) seems unstable at best,
> >>>>> so I better use all the interrupt sources to nudge the PHY subsystem and
> >>>>> have it check the link change.
> >>>>
> >>>> Then it sounds like you should just ignore interrupts and stay will
> >>>> polling for the TJA1100.
> >>>
> >>> Polling for the link status change is slow(er) than the IRQ driven
> >>> operation, so I would much rather use the interrupts.
> >>
> >> I agree about the speed, but it seems like interrupts on this PHY are
> >> not so reliable. Polling always works. But unfortunately, you cannot
> >> have both interrupts and polling to fix up problems when interrupts
> >> fail. Your call, do you think interrupts really do work?
> > 
> > It works fine for me this way. And mind you, it's only the TJA1100
> > that's flaky, the TJA1101 is better.
> > 
> >> If you say that tja1101 works as expected, then please just use the
> >> link up/down bits for it.
> > 
> > I still don't know which bits really trigger link status changes, so I'd
> > like to play it safe and just trigger on all of them.
> 
> So what do we do here ?

Hi Marek

My personal preference would be to just enable what is needed. But
I won't block a patch which enables everything.

  Andrew

Marek Vasut June 17, 2019, 5:43 p.m. UTC | #13

On 6/17/19 7:16 PM, Andrew Lunn wrote:
> On Thu, Jun 13, 2019 at 05:42:53PM +0200, Marek Vasut wrote:
>> On 5/30/19 1:46 AM, Marek Vasut wrote:
>>> On 5/30/19 1:29 AM, Andrew Lunn wrote:
>>>> On Tue, May 28, 2019 at 11:33:33PM +0200, Marek Vasut wrote:
>>>>> On 5/28/19 11:22 PM, Andrew Lunn wrote:
>>>>>>> The link detection on the TJA1100 (not TJA1101) seems unstable at best,
>>>>>>> so I better use all the interrupt sources to nudge the PHY subsystem and
>>>>>>> have it check the link change.
>>>>>>
>>>>>> Then it sounds like you should just ignore interrupts and stay will
>>>>>> polling for the TJA1100.
>>>>>
>>>>> Polling for the link status change is slow(er) than the IRQ driven
>>>>> operation, so I would much rather use the interrupts.
>>>>
>>>> I agree about the speed, but it seems like interrupts on this PHY are
>>>> not so reliable. Polling always works. But unfortunately, you cannot
>>>> have both interrupts and polling to fix up problems when interrupts
>>>> fail. Your call, do you think interrupts really do work?
>>>
>>> It works fine for me this way. And mind you, it's only the TJA1100
>>> that's flaky, the TJA1101 is better.
>>>
>>>> If you say that tja1101 works as expected, then please just use the
>>>> link up/down bits for it.
>>>
>>> I still don't know which bits really trigger link status changes, so I'd
>>> like to play it safe and just trigger on all of them.
>>
>> So what do we do here ?
> 
> Hi Marek
> 
> My personal preference would be to just enable what is needed. But
> I won't block a patch which enables everything.

Thanks. I don't know exactly what is needed , but I know that if I
enable everything, it works fine. And I'm not getting an interrupt storm
either, so it's probably OKish.

Patch

diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
index b705d0bd798b..b41af609607d 100644
--- a/drivers/net/phy/nxp-tja11xx.c
+++ b/drivers/net/phy/nxp-tja11xx.c
@@ -40,6 +40,29 @@ 
 #define MII_INTSRC_TEMP_ERR		BIT(1)
 #define MII_INTSRC_UV_ERR		BIT(3)
 
+#define MII_INTEN			22
+#define MII_INTEN_PWON_EN		BIT(15)
+#define MII_INTEN_WAKEUP_EN		BIT(14)
+#define MII_INTEN_PHY_INIT_FAIL_EN	BIT(11)
+#define MII_INTEN_LINK_STATUS_FAIL_EN	BIT(10)
+#define MII_INTEN_LINK_STATUS_UP_EN	BIT(9)
+#define MII_INTEN_SYM_ERR_EN		BIT(8)
+#define MII_INTEN_TRAINING_FAILED_EN	BIT(7)
+#define MII_INTEN_SQI_WARNING_EN	BIT(6)
+#define MII_INTEN_CONTROL_ERR_EN	BIT(5)
+#define MII_INTEN_UV_ERR_EN		BIT(3)
+#define MII_INTEN_UV_RECOVERY_EN	BIT(2)
+#define MII_INTEN_TEMP_ERR_EN		BIT(1)
+#define MII_INTEN_SLEEP_ABORT_EN	BIT(0)
+#define MII_INTEN_MASK							\
+	(MII_INTEN_PWON_EN | MII_INTEN_WAKEUP_EN |			\
+	MII_INTEN_PHY_INIT_FAIL_EN | MII_INTEN_LINK_STATUS_FAIL_EN |	\
+	MII_INTEN_LINK_STATUS_UP_EN | MII_INTEN_SYM_ERR_EN |		\
+	MII_INTEN_TRAINING_FAILED_EN | MII_INTEN_SQI_WARNING_EN |	\
+	MII_INTEN_CONTROL_ERR_EN | MII_INTEN_UV_ERR_EN |		\
+	MII_INTEN_UV_RECOVERY_EN | MII_INTEN_TEMP_ERR_EN |		\
+	MII_INTEN_SLEEP_ABORT_EN)
+
 #define MII_COMMSTAT			23
 #define MII_COMMSTAT_LINK_UP		BIT(15)
 
@@ -239,6 +262,25 @@  static int tja11xx_read_status(struct phy_device *phydev)
 	return 0;
 }
 
+static int tja11xx_config_intr(struct phy_device *phydev)
+{
+	int ret;
+
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+		ret = phy_write(phydev, MII_INTEN, MII_INTEN_MASK);
+	else
+		ret = phy_write(phydev, MII_INTEN, 0);
+
+	return ret < 0 ? ret : 0;
+}
+
+static int tja11xx_ack_interrupt(struct phy_device *phydev)
+{
+	int ret = phy_read(phydev, MII_INTSRC);
+
+	return ret < 0 ? ret : 0;
+}
+
 static int tja11xx_get_sset_count(struct phy_device *phydev)
 {
 	return ARRAY_SIZE(tja11xx_hw_stats);
@@ -366,6 +408,9 @@  static struct phy_driver tja11xx_driver[] = {
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
 		.set_loopback   = genphy_loopback,
+		/* IRQ related */
+		.config_intr	= tja11xx_config_intr,
+		.ack_interrupt	= tja11xx_ack_interrupt,
 		/* Statistics */
 		.get_sset_count = tja11xx_get_sset_count,
 		.get_strings	= tja11xx_get_strings,
@@ -381,6 +426,9 @@  static struct phy_driver tja11xx_driver[] = {
 		.suspend	= genphy_suspend,
 		.resume		= genphy_resume,
 		.set_loopback   = genphy_loopback,
+		/* IRQ related */
+		.config_intr	= tja11xx_config_intr,
+		.ack_interrupt	= tja11xx_ack_interrupt,
 		/* Statistics */
 		.get_sset_count = tja11xx_get_sset_count,
 		.get_strings	= tja11xx_get_strings,