lkml.org 
[lkml]   [2023]   [Jan]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH net-next v2 5/5] net: phy: Add driver for Motorcomm yt8531 gigabit ethernet phy
From
Hi Andrew,

On 2023/1/28 23:42, Andrew Lunn wrote:
> On Sat, Jan 28, 2023 at 11:13:14AM +0800, Frank Sae wrote:
>> Add a driver for the motorcomm yt8531 gigabit ethernet phy. We have
>> verified the driver on AM335x platform with yt8531 board. On the
>> board, yt8531 gigabit ethernet phy works in utp mode, RGMII
>> interface, supports 1000M/100M/10M speeds, and wol(magic package).
>>
>> Signed-off-by: Frank Sae <Frank.Sae@motor-comm.com>
>> ---
>> drivers/net/phy/Kconfig | 2 +-
>> drivers/net/phy/motorcomm.c | 204 +++++++++++++++++++++++++++++++++++-
>> 2 files changed, 203 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
>> index f5df2edc94a5..dc2f7d0b0cd8 100644
>> --- a/drivers/net/phy/Kconfig
>> +++ b/drivers/net/phy/Kconfig
>> @@ -257,7 +257,7 @@ config MOTORCOMM_PHY
>> tristate "Motorcomm PHYs"
>> help
>> Enables support for Motorcomm network PHYs.
>> - Currently supports the YT8511, YT8521, YT8531S Gigabit Ethernet PHYs.
>> + Currently supports the YT8511, YT8521, YT8531, YT8531S Gigabit Ethernet PHYs.
>
> This is O.K. for now, but when you add the next PHY, please do this in
> some other way, because it does not scale. Maybe just say YT85xx?
>
>>
>> config NATIONAL_PHY
>> tristate "National Semiconductor PHYs"
>> diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
>> index 9559fc52814f..f1fc912738e0 100644
>> --- a/drivers/net/phy/motorcomm.c
>> +++ b/drivers/net/phy/motorcomm.c
>> @@ -1,6 +1,6 @@
>> // SPDX-License-Identifier: GPL-2.0+
>> /*
>> - * Motorcomm 8511/8521/8531S PHY driver.
>> + * Motorcomm 8511/8521/8531/8531S PHY driver.
>> *
>> * Author: Peter Geis <pgwipeout@gmail.com>
>> * Author: Frank <Frank.Sae@motor-comm.com>
>> @@ -14,6 +14,7 @@
>>
>> #define PHY_ID_YT8511 0x0000010a
>> #define PHY_ID_YT8521 0x0000011A
>> +#define PHY_ID_YT8531 0x4f51e91b
>> #define PHY_ID_YT8531S 0x4F51E91A
>>
>> /* YT8521/YT8531S Register Overview
>> @@ -517,6 +518,68 @@ static int ytphy_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
>> return phy_restore_page(phydev, old_page, ret);
>> }
>>
>> +static int yt8531_set_wol(struct phy_device *phydev,
>> + struct ethtool_wolinfo *wol)
>> +{
>> + struct net_device *p_attached_dev;
>> + const u16 mac_addr_reg[] = {
>> + YTPHY_WOL_MACADDR2_REG,
>> + YTPHY_WOL_MACADDR1_REG,
>> + YTPHY_WOL_MACADDR0_REG,
>> + };
>> + const u8 *mac_addr;
>> + u16 mask, val;
>> + int ret;
>> + u8 i;
>> +
>> + if (wol->wolopts & WAKE_MAGIC) {
>> + p_attached_dev = phydev->attached_dev;
>> + if (!p_attached_dev)
>> + return -ENODEV;
>> +
>> + mac_addr = (const u8 *)p_attached_dev->dev_addr;
>> + if (!is_valid_ether_addr(mac_addr))
>> + return -EINVAL;
>
> Have you ever seen that happen? It suggests the MAC driver has a bug,
> not validating its MAC address.

I have never seen that happen.
Do you mean that I should change the code from

+ if (wol->wolopts & WAKE_MAGIC) {
+ p_attached_dev = phydev->attached_dev;
+ if (!p_attached_dev)
+ return -ENODEV;
+
+ mac_addr = (const u8 *)p_attached_dev->dev_addr;
+ if (!is_valid_ether_addr(mac_addr))
+ return -EINVAL;

to

+ if (wol->wolopts & WAKE_MAGIC) {
+ mac_addr = phydev->attached_dev->dev_addr;

?

> Also, does the PHY actually care? Will the firmware crash if given a
> bad MAC address?
>

The PHY actually is not care this. The firmware is not crash if given a
bad MAC address.

> Andrew

\
 
 \ /
  Last update: 2023-03-27 00:00    [W:0.048 / U:0.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site