Re: [PATCH net-next 1/2] net: phy: marvell: add new m88e1510 LED configuration

[shenjian (K)]

å 2019/2/14 12:06, Florian Fainelli åé:
> 
> 
> On 2/13/2019 8:31 PM, Jian Shen wrote:
>> The default m88e1510 LED configuration is 0x1177, used LED[0]
>> for 1000M link, LED[1] for 100M link, and LED[2] for active.
>> But for our boards, we want to use 0x1040, which use LED[0] for
>> link, and LED[1] for active.
>>
>> This patch adds a new m88e1510 LED configuration for it.
> 
> There appears to be a precedent with the DNS323 flag that was defined
> for the same purpose, but this unfortunately does not scale we cannot
> have every new platform come up with its own LED configuration without
> having a more structured approach to representing the LED configuration.
> 
> Maybe we can encode the desired LED behavior in a more generic way and
> utilize the 32 flag bits available to denote a selection, e.g.:
> 
> MARVELL_PHY_FLAG_LED0_100M	BIT(3)
> MARVELL_PHY_FLAG_LED0_1000M	BIT(4)
> 
> etc.
> 
> or maybe even better would be to expose the LEDs using the standard LEDs
> class subsystem and allow configuring different triggers. We have some
> amount of support for PHY LEDs already in tree, but AFAIR what we do not
> have support for is a "hardware blinking" trigger which those LEDs are.
> 
Thanks Florian.

I agree that we should use a generic way.
Maybe I understood incorrectly, for "utilize the 32 flag bits available to denote a selection",
do you mean to define a serial flags for each usage of each led ?
For each led can be configured to several usages, include link/10M link/100M link/1000M link/active/...,
it needs a lot bits for these combination, e.g:

#define MARVELL_PHY_FLAG_LED0_LINK		BIT(2)
#define MARVELL_PHY_FLAG_LED0_LINK_100M		BIT(3)
#define MARVELL_PHY_FLAG_LED0_LINK_1000M	BIT(4)
#define MARVELL_PHY_FLAG_LED0_ACTIVE		BIT(5)
#define MARVELL_PHY_FLAG_LED1_LINK		BIT(6)
#define MARVELL_PHY_FLAG_LED1_LINK_100M		BIT(7)
#define MARVELL_PHY_FLAG_LED1_LINK_1000M	BIT(8)
#define MARVELL_PHY_FLAG_LED1_ACTIVE		BIT(9)
...

Jian Shen
>>
>> Signed-off-by: Jian Shen <shenjian15@xxxxxxxxxx>
>> ---
>>  drivers/net/phy/marvell.c   | 22 +++++++++++++++++++++-
>>  include/linux/marvell_phy.h |  1 +
>>  2 files changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
>> index 3ccba37..c195286 100644
>> --- a/drivers/net/phy/marvell.c
>> +++ b/drivers/net/phy/marvell.c
>> @@ -128,6 +128,10 @@
>>  #define MII_PHY_LED_CTRL	        16
>>  #define MII_88E1121_PHY_LED_DEF		0x0030
>>  #define MII_88E1510_PHY_LED_DEF		0x1177
>> +#define MII_88E1510_PHY_HNS3_LED_DEF	0x1040
>> +
>> +#define MII_88E1510_PHY_LED_POLARITY_CTRL	0x11
>> +#define MII_88E1510_PHY_HNS3_LED_POLARITY	0x4415
>>  
>>  #define MII_M1011_PHY_STATUS		0x11
>>  #define MII_M1011_PHY_STATUS_1000	0x8000
>> @@ -619,12 +623,19 @@ static void marvell_config_led(struct phy_device *phydev)
>>  		def_config = MII_88E1121_PHY_LED_DEF;
>>  		break;
>>  	/* Default PHY LED config:
>> +	 * For hns3:
>> +	 * LED[0] .. Link
>> +	 * LED[1] .. Activity
>> +	 * For others:
>>  	 * LED[0] .. 1000Mbps Link
>>  	 * LED[1] .. 100Mbps Link
>>  	 * LED[2] .. Blink, Activity
>>  	 */
>>  	case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1510):
>> -		def_config = MII_88E1510_PHY_LED_DEF;
>> +		if (phydev->dev_flags & MARVELL_PHY_M1510_HNS3_LEDS)
>> +			def_config = MII_88E1510_PHY_HNS3_LED_DEF;
>> +		else
>> +			def_config = MII_88E1510_PHY_LED_DEF;
>>  		break;
>>  	default:
>>  		return;
>> @@ -634,6 +645,15 @@ static void marvell_config_led(struct phy_device *phydev)
>>  			      def_config);
>>  	if (err < 0)
>>  		phydev_warn(phydev, "Fail to config marvell phy LED.\n");
>> +
>> +	if (phydev->dev_flags & MARVELL_PHY_M1510_HNS3_LEDS) {
>> +		err = phy_write_paged(phydev, MII_MARVELL_LED_PAGE,
>> +				      MII_88E1510_PHY_LED_POLARITY_CTRL,
>> +				      MII_88E1510_PHY_HNS3_LED_POLARITY);
>> +		if (err < 0)
>> +			phydev_warn(phydev,
>> +				    "Fail to config marvell phy LED polarity.\n");
>> +	}
>>  }
>>  
>>  static int marvell_config_init(struct phy_device *phydev)
>> diff --git a/include/linux/marvell_phy.h b/include/linux/marvell_phy.h
>> index 1eb6f24..99e0bbb 100644
>> --- a/include/linux/marvell_phy.h
>> +++ b/include/linux/marvell_phy.h
>> @@ -32,5 +32,6 @@
>>  /* struct phy_device dev_flags definitions */
>>  #define MARVELL_PHY_M1145_FLAGS_RESISTANCE	0x00000001
>>  #define MARVELL_PHY_M1118_DNS323_LEDS		0x00000002
>> +#define MARVELL_PHY_M1510_HNS3_LEDS		0x00000004
>>  
>>  #endif /* _MARVELL_PHY_H */
>>
>