This patch series add the qca8081 ethernet phy driver support, which
improve the wol feature, leverage at803x phy driver and add the cdt
feature.
Changes in v1:
* merge qca8081 phy driver into at803x.
* add cdt feature.
* leverage at803x phy driver helpers.
Luo Jie (3):
net: phy: improve the wol feature of at803x
net: phy: add qca8081 ethernet phy driver
net: phy: add cdt feature of qca8081 phy
drivers/net/phy/at803x.c | 632 ++++++++++++++++++++++++++++++++++-----
1 file changed, 557 insertions(+), 75 deletions(-)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
wol is controlled by bit 5 of reg 3.8012, which should be
configured by set_wol of phy_driver.
Signed-off-by: Luo Jie <[email protected]>
---
drivers/net/phy/at803x.c | 50 +++++++++++++++++++++++-----------------
1 file changed, 29 insertions(+), 21 deletions(-)
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index 5d62b85a4024..ecae26f11aa4 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -70,10 +70,14 @@
#define AT803X_CDT_STATUS_DELTA_TIME_MASK GENMASK(7, 0)
#define AT803X_LED_CONTROL 0x18
-#define AT803X_DEVICE_ADDR 0x03
+/* WOL control */
+#define AT803X_PHY_MMD3_WOL_CTRL 0x8012
+#define AT803X_WOL_EN BIT(5)
+
#define AT803X_LOC_MAC_ADDR_0_15_OFFSET 0x804C
#define AT803X_LOC_MAC_ADDR_16_31_OFFSET 0x804B
#define AT803X_LOC_MAC_ADDR_32_47_OFFSET 0x804A
+
#define AT803X_REG_CHIP_CONFIG 0x1f
#define AT803X_BT_BX_REG_SEL 0x8000
@@ -328,12 +332,6 @@ static int at803x_set_wol(struct phy_device *phydev,
struct net_device *ndev = phydev->attached_dev;
const u8 *mac;
int ret;
- u32 value;
- unsigned int i, offsets[] = {
- AT803X_LOC_MAC_ADDR_32_47_OFFSET,
- AT803X_LOC_MAC_ADDR_16_31_OFFSET,
- AT803X_LOC_MAC_ADDR_0_15_OFFSET,
- };
if (!ndev)
return -ENODEV;
@@ -344,23 +342,30 @@ static int at803x_set_wol(struct phy_device *phydev,
if (!is_valid_ether_addr(mac))
return -EINVAL;
- for (i = 0; i < 3; i++)
- phy_write_mmd(phydev, AT803X_DEVICE_ADDR, offsets[i],
- mac[(i * 2) + 1] | (mac[(i * 2)] << 8));
+ phy_write_mmd(phydev, MDIO_MMD_PCS, AT803X_LOC_MAC_ADDR_32_47_OFFSET,
+ mac[1] | (mac[0] << 8));
+ phy_write_mmd(phydev, MDIO_MMD_PCS, AT803X_LOC_MAC_ADDR_16_31_OFFSET,
+ mac[3] | (mac[2] << 8));
+ phy_write_mmd(phydev, MDIO_MMD_PCS, AT803X_LOC_MAC_ADDR_0_15_OFFSET,
+ mac[5] | (mac[4] << 8));
- value = phy_read(phydev, AT803X_INTR_ENABLE);
- value |= AT803X_INTR_ENABLE_WOL;
- ret = phy_write(phydev, AT803X_INTR_ENABLE, value);
+ /* clear the pending interrupt */
+ phy_read(phydev, AT803X_INTR_STATUS);
+
+ ret = phy_modify(phydev, AT803X_INTR_ENABLE, 0, AT803X_INTR_ENABLE_WOL);
if (ret)
return ret;
- value = phy_read(phydev, AT803X_INTR_STATUS);
+
+ ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, AT803X_PHY_MMD3_WOL_CTRL,
+ 0, AT803X_WOL_EN);
+
} else {
- value = phy_read(phydev, AT803X_INTR_ENABLE);
- value &= (~AT803X_INTR_ENABLE_WOL);
- ret = phy_write(phydev, AT803X_INTR_ENABLE, value);
+ ret = phy_modify(phydev, AT803X_INTR_ENABLE, AT803X_INTR_ENABLE_WOL, 0);
if (ret)
return ret;
- value = phy_read(phydev, AT803X_INTR_STATUS);
+
+ ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, AT803X_PHY_MMD3_WOL_CTRL,
+ AT803X_WOL_EN, 0);
}
return ret;
@@ -369,13 +374,16 @@ static int at803x_set_wol(struct phy_device *phydev,
static void at803x_get_wol(struct phy_device *phydev,
struct ethtool_wolinfo *wol)
{
- u32 value;
+ int ret;
wol->supported = WAKE_MAGIC;
wol->wolopts = 0;
- value = phy_read(phydev, AT803X_INTR_ENABLE);
- if (value & AT803X_INTR_ENABLE_WOL)
+ ret = phy_read_mmd(phydev, MDIO_MMD_PCS, AT803X_PHY_MMD3_WOL_CTRL);
+ if (ret < 0)
+ return;
+
+ if (ret & AT803X_WOL_EN)
wol->wolopts |= WAKE_MAGIC;
}
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On Mon, Aug 30, 2021 at 07:07:31PM +0800, Luo Jie wrote:
> wol is controlled by bit 5 of reg 3.8012, which should be
> configured by set_wol of phy_driver.
>
> Signed-off-by: Luo Jie <[email protected]>
> ---
> drivers/net/phy/at803x.c | 50 +++++++++++++++++++++++-----------------
> 1 file changed, 29 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index 5d62b85a4024..ecae26f11aa4 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -70,10 +70,14 @@
> #define AT803X_CDT_STATUS_DELTA_TIME_MASK GENMASK(7, 0)
> #define AT803X_LED_CONTROL 0x18
>
> -#define AT803X_DEVICE_ADDR 0x03
> +/* WOL control */
> +#define AT803X_PHY_MMD3_WOL_CTRL 0x8012
> +#define AT803X_WOL_EN BIT(5)
> +
> #define AT803X_LOC_MAC_ADDR_0_15_OFFSET 0x804C
> #define AT803X_LOC_MAC_ADDR_16_31_OFFSET 0x804B
> #define AT803X_LOC_MAC_ADDR_32_47_OFFSET 0x804A
> +
> #define AT803X_REG_CHIP_CONFIG 0x1f
> #define AT803X_BT_BX_REG_SEL 0x8000
>
> @@ -328,12 +332,6 @@ static int at803x_set_wol(struct phy_device *phydev,
> struct net_device *ndev = phydev->attached_dev;
> const u8 *mac;
> int ret;
> - u32 value;
> - unsigned int i, offsets[] = {
> - AT803X_LOC_MAC_ADDR_32_47_OFFSET,
> - AT803X_LOC_MAC_ADDR_16_31_OFFSET,
> - AT803X_LOC_MAC_ADDR_0_15_OFFSET,
> - };
>
> if (!ndev)
> return -ENODEV;
> @@ -344,23 +342,30 @@ static int at803x_set_wol(struct phy_device *phydev,
> if (!is_valid_ether_addr(mac))
> return -EINVAL;
>
> - for (i = 0; i < 3; i++)
> - phy_write_mmd(phydev, AT803X_DEVICE_ADDR, offsets[i],
> - mac[(i * 2) + 1] | (mac[(i * 2)] << 8));
> + phy_write_mmd(phydev, MDIO_MMD_PCS, AT803X_LOC_MAC_ADDR_32_47_OFFSET,
> + mac[1] | (mac[0] << 8));
> + phy_write_mmd(phydev, MDIO_MMD_PCS, AT803X_LOC_MAC_ADDR_16_31_OFFSET,
> + mac[3] | (mac[2] << 8));
> + phy_write_mmd(phydev, MDIO_MMD_PCS, AT803X_LOC_MAC_ADDR_0_15_OFFSET,
> + mac[5] | (mac[4] << 8));
Please try to keep your changes minimal. It looks like all you really
need is to replace AT803X_DEVICE_ADDR with MDIO_MMD_PCS. Everything
else is O.K. Maybe make offset a const?
Making the change more complex than it needs to be makes it harder to
review.
>
> - value = phy_read(phydev, AT803X_INTR_ENABLE);
> - value |= AT803X_INTR_ENABLE_WOL;
> - ret = phy_write(phydev, AT803X_INTR_ENABLE, value);
So that it be replaced with a phy_modify().
> + /* clear the pending interrupt */
> + phy_read(phydev, AT803X_INTR_STATUS);
But where did this come from?
> +
> + ret = phy_modify(phydev, AT803X_INTR_ENABLE, 0, AT803X_INTR_ENABLE_WOL);
> if (ret)
> return ret;
> - value = phy_read(phydev, AT803X_INTR_STATUS);
> +
> + ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, AT803X_PHY_MMD3_WOL_CTRL,
> + 0, AT803X_WOL_EN);
> +
> } else {
> - value = phy_read(phydev, AT803X_INTR_ENABLE);
> - value &= (~AT803X_INTR_ENABLE_WOL);
> - ret = phy_write(phydev, AT803X_INTR_ENABLE, value);
> + ret = phy_modify(phydev, AT803X_INTR_ENABLE, AT803X_INTR_ENABLE_WOL, 0);
This makes sense
> if (ret)
> return ret;
> - value = phy_read(phydev, AT803X_INTR_STATUS);
> +
> + ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, AT803X_PHY_MMD3_WOL_CTRL,
> + AT803X_WOL_EN, 0);
But where did this come from?
I could be wrong, but i get the feeling you just replaced the code
with what you have in your new driver, rather than step by step
improve this code.
Please break this patch up into a number of patches:
AT803X_DEVICE_ADDR with MDIO_MMD_PCS
read/write to modify.
Other patches for the remaining changes, if actually required, with a
good explanation of why they are needed.
Andrew
On 8/30/2021 9:32 PM, Andrew Lunn wrote:
> On Mon, Aug 30, 2021 at 07:07:31PM +0800, Luo Jie wrote:
>> wol is controlled by bit 5 of reg 3.8012, which should be
>> configured by set_wol of phy_driver.
>>
>> Signed-off-by: Luo Jie <[email protected]>
>> ---
>> drivers/net/phy/at803x.c | 50 +++++++++++++++++++++++-----------------
>> 1 file changed, 29 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
>> index 5d62b85a4024..ecae26f11aa4 100644
>> --- a/drivers/net/phy/at803x.c
>> +++ b/drivers/net/phy/at803x.c
>> @@ -70,10 +70,14 @@
>> #define AT803X_CDT_STATUS_DELTA_TIME_MASK GENMASK(7, 0)
>> #define AT803X_LED_CONTROL 0x18
>>
>> -#define AT803X_DEVICE_ADDR 0x03
>> +/* WOL control */
>> +#define AT803X_PHY_MMD3_WOL_CTRL 0x8012
>> +#define AT803X_WOL_EN BIT(5)
>> +
>> #define AT803X_LOC_MAC_ADDR_0_15_OFFSET 0x804C
>> #define AT803X_LOC_MAC_ADDR_16_31_OFFSET 0x804B
>> #define AT803X_LOC_MAC_ADDR_32_47_OFFSET 0x804A
>> +
>> #define AT803X_REG_CHIP_CONFIG 0x1f
>> #define AT803X_BT_BX_REG_SEL 0x8000
>>
>> @@ -328,12 +332,6 @@ static int at803x_set_wol(struct phy_device *phydev,
>> struct net_device *ndev = phydev->attached_dev;
>> const u8 *mac;
>> int ret;
>> - u32 value;
>> - unsigned int i, offsets[] = {
>> - AT803X_LOC_MAC_ADDR_32_47_OFFSET,
>> - AT803X_LOC_MAC_ADDR_16_31_OFFSET,
>> - AT803X_LOC_MAC_ADDR_0_15_OFFSET,
>> - };
>>
>> if (!ndev)
>> return -ENODEV;
>> @@ -344,23 +342,30 @@ static int at803x_set_wol(struct phy_device *phydev,
>> if (!is_valid_ether_addr(mac))
>> return -EINVAL;
>>
>> - for (i = 0; i < 3; i++)
>> - phy_write_mmd(phydev, AT803X_DEVICE_ADDR, offsets[i],
>> - mac[(i * 2) + 1] | (mac[(i * 2)] << 8));
>> + phy_write_mmd(phydev, MDIO_MMD_PCS, AT803X_LOC_MAC_ADDR_32_47_OFFSET,
>> + mac[1] | (mac[0] << 8));
>> + phy_write_mmd(phydev, MDIO_MMD_PCS, AT803X_LOC_MAC_ADDR_16_31_OFFSET,
>> + mac[3] | (mac[2] << 8));
>> + phy_write_mmd(phydev, MDIO_MMD_PCS, AT803X_LOC_MAC_ADDR_0_15_OFFSET,
>> + mac[5] | (mac[4] << 8));
> Please try to keep your changes minimal. It looks like all you really
> need is to replace AT803X_DEVICE_ADDR with MDIO_MMD_PCS. Everything
> else is O.K. Maybe make offset a const?
>
> Making the change more complex than it needs to be makes it harder to
> review.
thanks Andrew for the comments, will keep this changes minimal in the
next patch series.
>
>>
>> - value = phy_read(phydev, AT803X_INTR_ENABLE);
>> - value |= AT803X_INTR_ENABLE_WOL;
>> - ret = phy_write(phydev, AT803X_INTR_ENABLE, value);
> So that it be replaced with a phy_modify().
yes, it's replaced with phy_modify().
>
>
>> + /* clear the pending interrupt */
>> + phy_read(phydev, AT803X_INTR_STATUS);
> But where did this come from?
this code is not new added, which is existed code for at803x phy driver.
>
>> +
>> + ret = phy_modify(phydev, AT803X_INTR_ENABLE, 0, AT803X_INTR_ENABLE_WOL);
>> if (ret)
>> return ret;
>> - value = phy_read(phydev, AT803X_INTR_STATUS);
>> +
>> + ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, AT803X_PHY_MMD3_WOL_CTRL,
>> + 0, AT803X_WOL_EN);
>> +
>> } else {
>> - value = phy_read(phydev, AT803X_INTR_ENABLE);
>> - value &= (~AT803X_INTR_ENABLE_WOL);
>> - ret = phy_write(phydev, AT803X_INTR_ENABLE, value);
>> + ret = phy_modify(phydev, AT803X_INTR_ENABLE, AT803X_INTR_ENABLE_WOL, 0);
> This makes sense
>
>> if (ret)
>> return ret;
>> - value = phy_read(phydev, AT803X_INTR_STATUS);
>> +
>> + ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, AT803X_PHY_MMD3_WOL_CTRL,
>> + AT803X_WOL_EN, 0);
> But where did this come from?
For wol_set, i add the AT803X_WOL_EN configuration, which is necessary
for the wol feature.
>
> I could be wrong, but i get the feeling you just replaced the code
> with what you have in your new driver, rather than step by step
> improve this code.
>
> Please break this patch up into a number of patches:
>
> AT803X_DEVICE_ADDR with MDIO_MMD_PCS
> read/write to modify.
>
> Other patches for the remaining changes, if actually required, with a
> good explanation of why they are needed.
>
> Andrew
Hi Andrew, thanks for the suggestions. Will break the changes into
minimal patches, and check the at803x phy driver in detail to
improve the driver in the next patch series.