| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6856a839-0fa0-1240-47cd-ae8536294bcd@codeaurora.org>
Date: Tue, 17 Aug 2021 21:10:44 +0800
From: Jie Luo <luoj@...eaurora.org>
To: Andrew Lunn <andrew@...n.ch>
Cc: hkallweit1@...il.com, linux@...linux.org.uk, davem@...emloft.net,
kuba@...nel.org, linux-kernel@...r.kernel.org,
netdev@...r.kernel.org, sricharan@...eaurora.org
Subject: Re: [PATCH] net: phy: add qca8081 ethernet phy driver
On 8/16/2021 9:56 PM, Andrew Lunn wrote:
> On Mon, Aug 16, 2021 at 07:34:40PM +0800, Luo Jie wrote:
>> qca8081 is industry’s lowest cost and power 1-port 2.5G/1G Ethernet PHY
>> chip, which implements SGMII/SGMII+ for interface to SoC.
> Hi Luo
>
> No Marketing claims in the commit message please. Even if it is
> correct now, it will soon be wrong with newer generations of
> devices.
>
> And what is SGMII+? Please reference a document. Is it actually
> 2500BaseX?
Hi Andrew,
thanks for the comments, will remove the claims in the next patch.
SGMII+ is for 2500BaseX, which is same as SGMII, but the clock frequency
of SGMII+ is 2.5 times SGMII.
>> Signed-off-by: Luo Jie <luoj@...eaurora.org>
>> ---
>> drivers/net/phy/Kconfig | 6 +
>> drivers/net/phy/Makefile | 1 +
>> drivers/net/phy/qca808x.c | 573 ++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 580 insertions(+)
>> create mode 100644 drivers/net/phy/qca808x.c
>>
>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
>> index c56f703ae998..26cb1c2ffd17 100644
>> --- a/drivers/net/phy/Kconfig
>> +++ b/drivers/net/phy/Kconfig
>> @@ -343,3 +343,9 @@ endif # PHYLIB
>> config MICREL_KS8995MA
>> tristate "Micrel KS8995MA 5-ports 10/100 managed Ethernet switch"
>> depends on SPI
>> +
>> +config QCA808X_PHY
>> + tristate "Qualcomm Atheros QCA808X PHYs"
>> + depends on REGULATOR
>> + help
>> + Currently supports the QCA8081 model
> This file is sorted on the tristate text. So it should appear directly
> after AT803X_PHY.
will update it in the next patch.
>
>> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
>> index 172bb193ae6a..9ef477d79588 100644
>> --- a/drivers/net/phy/Makefile
>> +++ b/drivers/net/phy/Makefile
>> @@ -84,3 +84,4 @@ obj-$(CONFIG_STE10XP) += ste10Xp.o
>> obj-$(CONFIG_TERANETICS_PHY) += teranetics.o
>> obj-$(CONFIG_VITESSE_PHY) += vitesse.o
>> obj-$(CONFIG_XILINX_GMII2RGMII) += xilinx_gmii2rgmii.o
>> +obj-$(CONFIG_QCA808X_PHY) += qca808x.o
> And this file is sorted by CONFIG_ so should be after
> CONFIG_NXP_TJA11XX_PHY.
>
> Keeping things sorted reduces the likelyhood of a merge conflict.
will update it in the next patch.
>
>> +#include <linux/module.h>
>> +#include <linux/etherdevice.h>
>> +#include <linux/phy.h>
>> +#include <linux/bitfield.h>
>> +
>> +#define QCA8081_PHY_ID 0x004DD101
>> +
>> +/* MII special status */
>> +#define QCA808X_PHY_SPEC_STATUS 0x11
>> +#define QCA808X_STATUS_FULL_DUPLEX BIT(13)
>> +#define QCA808X_STATUS_LINK_PASS BIT(10)
>> +#define QCA808X_STATUS_SPEED_MASK GENMASK(9, 7)
>> +#define QCA808X_STATUS_SPEED_100MBS 1
>> +#define QCA808X_STATUS_SPEED_1000MBS 2
>> +#define QCA808X_STATUS_SPEED_2500MBS 4
>> +
>> +/* MII interrupt enable & status */
>> +#define QCA808X_PHY_INTR_MASK 0x12
>> +#define QCA808X_PHY_INTR_STATUS 0x13
>> +#define QCA808X_INTR_ENABLE_FAST_RETRAIN_FAIL BIT(15)
>> +#define QCA808X_INTR_ENABLE_SPEED_CHANGED BIT(14)
>> +#define QCA808X_INTR_ENABLE_DUPLEX_CHANGED BIT(13)
>> +#define QCA808X_INTR_ENABLE_PAGE_RECEIVED BIT(12)
>> +#define QCA808X_INTR_ENABLE_LINK_FAIL BIT(11)
>> +#define QCA808X_INTR_ENABLE_LINK_SUCCESS BIT(10)
>> +#define QCA808X_INTR_ENABLE_POE BIT(1)
>> +#define QCA808X_INTR_ENABLE_WOL BIT(0)
>> +
>> +/* MII DBG address & data */
>> +#define QCA808X_PHY_DEBUG_ADDR 0x1d
>> +#define QCA808X_PHY_DEBUG_DATA 0x1e
>> +
> A lot of these registers look the same as the at803x. So i'm thinking
> you should merge these two drivers. There is a lot of code which is
> identical, or very similar, which you can share.
Hi Andrew,
qca8081 supports IEEE1588 feature, the IEEE1588 code may be submitted in
the near future,
so it may be a good idea to keep it out from at803x code.
>
>> +static int qca808x_get_2500caps(struct phy_device *phydev)
>> +{
>> + int phy_data;
>> +
>> + phy_data = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, QCA808X_PHY_MMD1_PMA_CAP_REG);
>> +
>> + return (phy_data & QCA808X_STATUS_2500T_FD_CAPS) ? 1 : 0;
> So the PHY ignores the standard and does not set bit
> MDIO_PMA_NG_EXTABLE_2_5GBT in MDIO_MMD_PMAPMD, MDIO_PMA_NG_EXTABLE ?
> Please compare the registers against the standards. If they are
> actually being followed, please you the linux names for these
> registers, and try to use the generic code.
will update it to use the generic code in the next patch.
>
>> +static int qca808x_phy_ms_random_seed_set(struct phy_device *phydev)
>> +{
>> + u16 seed_value = (prandom_u32() % QCA808X_MASTER_SLAVE_SEED_RANGE) << 2;
>> +
>> + return qca808x_debug_reg_modify(phydev, QCA808X_PHY_DEBUG_LOCAL_SEED,
>> + QCA808X_MASTER_SLAVE_SEED_CFG, seed_value);
>> +}
>> +
>> +static int qca808x_phy_ms_seed_enable(struct phy_device *phydev, bool enable)
>> +{
>> + u16 seed_enable = 0;
>> +
>> + if (enable)
>> + seed_enable = QCA808X_MASTER_SLAVE_SEED_ENABLE;
>> +
>> + return qca808x_debug_reg_modify(phydev, QCA808X_PHY_DEBUG_LOCAL_SEED,
>> + QCA808X_MASTER_SLAVE_SEED_ENABLE, seed_enable);
>> +}
> This is interesting. I've not seen any other PHY does this. is there
> documentation? Is the datasheet available?
this piece of code is for configuring the random seed to a lower value
to make the PHY linked
as the SLAVE mode for fixing some IOT issue, for master/slave
auto-negotiation, please refer to
https://www.ieee802.org/3/an/public/jul04/lynskey_2_0704.pdf.
>
>> +static int qca808x_soft_reset(struct phy_device *phydev)
>> +{
>> + int ret;
>> +
>> + ret = genphy_soft_reset(phydev);
>> + if (ret < 0)
>> + return ret;
>> +
>> + if (phydev->autoneg == AUTONEG_DISABLE) {
>> + ret = qca808x_speed_forced(phydev);
>> + if (ret)
>> + return ret;
> That is unusual. After a reset, you would expect the config_aneg()
> function to be called, and it should set things like this. Why is it
> needed?
>
> I don't see anything handling the host side. Generally, devices like
> this use SGMII for 10/100/1G. When 2.5G is in use they swap their host
> interface to 2500BaseX. See mv3310_update_interface() as an example.
>
> Andrew
thanks Andrew for the comments. will remove qca808x_speed_forced from
reset function,
and add the phydev interface update in the next patch.
Powered by blists - more mailing lists