From: Jie Luo <luoj@codeaurora.org>
To: "Russell King (Oracle)" <linux@armlinux.org.uk>
Cc: andrew@lunn.ch, hkallweit1@gmail.com, davem@davemloft.net,
kuba@kernel.org, linux-kernel@vger.kernel.org,
netdev@vger.kernel.org, sricharan@codeaurora.org
Subject: Re: [PATCH] net: phy: add qca8081 ethernet phy driver
Date: Tue, 17 Aug 2021 21:16:10 +0800 [thread overview]
Message-ID: <5f517769-8987-385d-1a0c-48c3e808636c@codeaurora.org> (raw)
In-Reply-To: <20210816140103.GK22278@shell.armlinux.org.uk>
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.
>
next prev parent reply other threads:[~2021-08-17 13:17 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-16 11:34 [PATCH] net: phy: add qca8081 ethernet phy driver Luo Jie
2021-08-16 13:56 ` Andrew Lunn
2021-08-17 13:10 ` Jie Luo
2021-08-17 13:33 ` Andrew Lunn
2021-08-18 13:21 ` Jie Luo
2021-08-17 14:32 ` Russell King (Oracle)
2021-08-18 7:41 ` Michael Walle
2021-08-18 13:34 ` Jie Luo
2021-08-18 17:09 ` Andrew Lunn
2021-08-19 10:30 ` Jie Luo
2021-08-16 14:01 ` Russell King (Oracle)
2021-08-17 13:16 ` Jie Luo [this message]
2021-08-16 15:27 ` Heiner Kallweit
2021-08-17 14:27 ` Jie Luo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5f517769-8987-385d-1a0c-48c3e808636c@codeaurora.org \
--to=luoj@codeaurora.org \
--cc=andrew@lunn.ch \
--cc=davem@davemloft.net \
--cc=hkallweit1@gmail.com \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=netdev@vger.kernel.org \
--cc=sricharan@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.