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

On 8/16/2021 10:01 PM, Russell King (Oracle) wrote:

On Mon, Aug 16, 2021 at 07:34:40PM +0800, Luo Jie wrote:

quoted

+/* 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.

quoted

+/* 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.

quoted

+/* 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.

quoted

+/* 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.