Re: [PATCH] net: phy: add qca8081 ethernet phy driver

[Jie Luo <luoj@codeaurora.org>]

On 8/16/2021 10:01 PM, Russell King (Oracle) wrote:
> On Mon, Aug 16, 2021 at 07:34:40PM +0800, Luo Jie wrote:
>> +/* PMA control */
>> +#define QCA808X_PHY_MMD1_PMA_CONTROL			0x0
>> +#define QCA808X_PMA_CONTROL_SPEED_MASK			(BIT(13) | BIT(6))
>> +#define QCA808X_PMA_CONTROL_2500M			(BIT(13) | BIT(6))
>> +#define QCA808X_PMA_CONTROL_1000M			BIT(6)
>> +#define QCA808X_PMA_CONTROL_100M			BIT(13)
>> +#define QCA808X_PMA_CONTROL_10M				0x0
> I don't think this is (a) correct, (b) any different from the IEEE 802.3
> standard definitions. Please use the standard definitions in
> include/uapi/linux/mdio.h.
thanks Russell, will update it in the next patch.
>
>> +/* PMA capable */
>> +#define QCA808X_PHY_MMD1_PMA_CAP_REG			0x4
>> +#define QCA808X_STATUS_2500T_FD_CAPS			BIT(13)
> Again, this is IEEE 802.3 standard, nothing special here. As this is
> a standard bit, include/uapi/linux/mdio.h should be augmented with
> it rather than introducing private definitions. Note that this is
> _not_ 2500T but merely indicates that the "PMA/PMD is capable of
> operating at 2.5 Gb/s". IEEE 802.3 makes no mention of the underlying
> technology used to achieve 2.5Gbps.
thanks for the comments, will update it to use the standard register for 
the 2.5G capability.
>> +/* PMA type */
>> +#define QCA808X_PHY_MMD1_PMA_TYPE			0x7
>> +#define QCA808X_PMA_TYPE_MASK				GENMASK(5, 0)
>> +#define QCA808X_PMA_TYPE_2500M				0x30
>> +#define QCA808X_PMA_TYPE_1000M				0xc
>> +#define QCA808X_PMA_TYPE_100M				0xe
>> +#define QCA808X_PMA_TYPE_10M				0xf
> This is the PMA type selection register... another IEEE 802.3 standard
> register. You omit the underlying technology for these definitions.
>  From the IEEE 802.3 standard:
>
> 0x30 2.5GBASE-T PMA
> 0x0f 10BASE-T PMA/PMD
> 0x0e 100BASE-TX PMA/PMD
> 0x0c 1000BASE-T PMA/PMD
>
> and we have definitions for all these already:
>
> #define MDIO_PMA_CTRL2_1000BT           0x000c  /* 1000BASE-T type */
> #define MDIO_PMA_CTRL2_100BTX           0x000e  /* 100BASE-TX type */
> #define MDIO_PMA_CTRL2_10BT             0x000f  /* 10BASE-T type */
> #define MDIO_PMA_CTRL2_2_5GBT           0x0030  /* 2.5GBaseT type */
>
> Please make use of these.
will use it in the next patch.
>
>> +/* AN 2.5G */
>> +#define QCA808X_PHY_MMD7_AUTONEGOTIATION_CONTROL	0x20
>> +#define QCA808X_ADVERTISE_2500FULL			BIT(7)
>> +#define QCA808X_FAST_RETRAIN_2500BT			BIT(5)
>> +#define QCA808X_ADV_LOOP_TIMING				BIT(0)
>> +
>> +/* Fast retrain related registers */
>> +#define QCA808X_PHY_MMD1_FAST_RETRAIN_STATUS_CTL	0x93
>> +#define QCA808X_FAST_RETRAIN_CTRL			0x1
> I suspect I'm going to find that the above are standard registers
> as well.
>
> I think I'll also ask that once you've corrected the above, that you
> also look to see which of the Clause 45 generic support functions you
> can make use of in this driver, and switch it over to those - and then
> post a revised version.
>
> Thanks.

thanks Russell for review comments, will refer to the standard registers 
and update

it in the next patch.

>