Re: [PATCH net-next v3 06/12] net: ethernet: oa_tc6: implement internal PHY initialization

From: Parthiban.Veerasooran
Date: Tue Apr 16 2024 - 07:04:00 EST

Hi Andrew,

On 15/04/24 6:45 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Fri, Apr 12, 2024 at 10:43:15AM +0000, Parthiban.Veerasooran@xxxxxxxxxxxxx wrote:
>> Hi Andrew,
>>
>> After implementing the new APIs oa_tc6_mdiobus_read_c45() and
>> oa_tc6_mdiobus_write_c45(), I tried testing. But, for some reason these
>> APIs are never get called by phy_read_mmd() or phy_write_mmd() as those
>> APIs are checking for phydev->is_c45 flag for calling this new c45 APIs
>> which is not set in this case.
>>
>> If the phydev is found via c22, phylib does not set phydev->is_c45, and
>> everything ends up going indirect.
>
> We don't have a clean separation between C22/C45 register space and
> C22/C45 MDIO bus protocols. As you said, if the PHY is discovered via
> C22 bus protocol it assumes it uses C22 protocol.
>
Thanks for confirming my understanding.
> Those PHYs which do support C45, and in particular, features which
> need C45, all call genphy_c45_pma_read_abilities() in there
> .get_features function to add in C45 features. However, it will
> continue using C45 over C22 because genphy_c45_pma_read_abilities()
> only really says the device has C45 registers, not C45 protocol.
>
OK.
> I can see two different things you can try.
>
> 1) set .read_mmd and .write_mmd in the PHY driver to phy_read_mmd()
> and phy_write_mmd(). That is not great however, since each vendor
> is likely to have their own PHY driver.
>
I tried this approach and it works as expected. Means whenever there is
a c45 register access, it directly uses the
oa_tc6_read_c45()/oa_tc6_write_c45() functions. Herewith I have attached
the patch
(v4-0006-net-ethernet-oa_tc6-implement-internal-PHY-initia.patch) which
has this new implementation for your reference. Is this you expected?
Can you comment on this?
> 2) Add a helper to set is_c45. This however has side effects you might
> not want. e.g. auto-neg will be performed via C45, not C22. This
> might not matter for a T1 PHY where is think auto-neg is not
> actually used. But i don't see anything in TC6 which limits it to
> T1, i could well imagine a T2 or T4 PHY being used with full
> auto-neg.
>
I tried this approach by setting up is_c45 flag when I use
phy_read_mmd() function but ended up with the kernel call trace
(c45_kernel_call_trace.png) attached here for your reference.
> We really do need to separate C45 registers from C45 protocol in the
> phylib core to cleanly solve this.
OK. In my opinion, until having the above implementation in the phylib
core, shall we go with the first approach [1] I tried out as it is
fulfilling our requirement? What do you think?

Best regards,
Parthiban V
>
> Andrew
>

Attachment: c45_kernel_call_trace.png
Description: c45_kernel_call_trace.png

From 8cb916820a5d69a18c68b76b3d1bb164cb821afa Mon Sep 17 00:00:00 2001
From: Parthiban Veerasooran <Parthiban.Veerasooran@xxxxxxxxxxxxx>
Date: Tue, 16 Apr 2024 14:25:13 +0530
Subject: [PATCH net-next v4 6/6] net: ethernet: oa_tc6: implement internal PHY
initialization

Internal PHY is initialized as per the PHY register capability supported
by the MAC-PHY. Direct PHY Register Access Capability indicates if PHY
registers are directly accessible within the SPI register memory space.
Indirect PHY Register Access Capability indicates if PHY registers are
indirectly accessible through the MDIO/MDC registers MDIOACCn defined in
OPEN Alliance specification. Currently the direct register access is only
supported.

Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@xxxxxxxxxxxxx>
---
drivers/net/ethernet/oa_tc6.c | 220 +++++++++++++++++++++++++++++++-
drivers/net/phy/microchip_t1s.c | 20 +++
include/linux/oa_tc6.h | 4 +-
include/uapi/linux/mdio.h | 1 +
4 files changed, 243 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
index 850765574d30..043137663852 100644
--- a/drivers/net/ethernet/oa_tc6.c
+++ b/drivers/net/ethernet/oa_tc6.c
@@ -7,9 +7,15 @@

#include <linux/bitfield.h>
#include <linux/iopoll.h>
+#include <linux/mdio.h>
+#include <linux/phy.h>
#include <linux/oa_tc6.h>

/* OPEN Alliance TC6 registers */
+/* Standard Capabilities Register */
+#define OA_TC6_REG_STDCAP 0x0002
+#define STDCAP_DIRECT_PHY_REG_ACCESS BIT(8)
+
/* Reset Control and Status Register */
#define OA_TC6_REG_RESET 0x0003
#define RESET_SWRESET BIT(0) /* Software Reset */
@@ -25,6 +31,10 @@
#define INT_MASK0_RX_BUFFER_OVERFLOW_ERR_MASK BIT(3)
#define INT_MASK0_TX_PROTOCOL_ERR_MASK BIT(0)

+/* PHY Clause 22 and 29 registers base address and mask */
+#define OA_TC6_PHY_STD_REG_ADDR_BASE 0xFF00
+#define OA_TC6_PHY_STD_REG_ADDR_MASK 0x3F
+
/* Control command header */
#define OA_TC6_CTRL_HEADER_DATA_NOT_CTRL BIT(31)
#define OA_TC6_CTRL_HEADER_WRITE_NOT_READ BIT(29)
@@ -33,6 +43,15 @@
#define OA_TC6_CTRL_HEADER_LENGTH GENMASK(7, 1)
#define OA_TC6_CTRL_HEADER_PARITY BIT(0)

+/* PHY â?? Clause 45 registers memory map selector (MMS) as per table 6 in the
+ * OPEN Alliance specification.
+ */
+#define OA_TC6_PHY_C45_PCS_MMS2 2 /* MMD 3 */
+#define OA_TC6_PHY_C45_PMA_PMD_MMS3 3 /* MMD 1 */
+#define OA_TC6_PHY_C45_VS_PLCA_MMS4 4 /* MMD 31 */
+#define OA_TC6_PHY_C45_AUTO_NEG_MMS5 5 /* MMD 7 */
+#define OA_TC6_PHY_C45_POWER_UNIT_MMS6 6 /* MMD 13 */
+
#define OA_TC6_CTRL_HEADER_SIZE 4
#define OA_TC6_CTRL_REG_VALUE_SIZE 4
#define OA_TC6_CTRL_IGNORED_SIZE 4
@@ -46,6 +65,10 @@

/* Internal structure for MAC-PHY drivers */
struct oa_tc6 {
+ struct device *dev;
+ struct net_device *netdev;
+ struct phy_device *phydev;
+ struct mii_bus *mdiobus;
struct spi_device *spi;
struct mutex spi_ctrl_lock; /* Protects spi control transfer */
void *spi_ctrl_tx_buf;
@@ -298,6 +321,181 @@ int oa_tc6_write_register(struct oa_tc6 *tc6, u32 address, u32 value)
}
EXPORT_SYMBOL_GPL(oa_tc6_write_register);

+static int oa_tc6_check_phy_reg_direct_access_capability(struct oa_tc6 *tc6)
+{
+ u32 regval;
+ int ret;
+
+ ret = oa_tc6_read_register(tc6, OA_TC6_REG_STDCAP, &regval);
+ if (ret)
+ return ret;
+
+ if (!(regval & STDCAP_DIRECT_PHY_REG_ACCESS))
+ return -ENODEV;
+
+ return 0;
+}
+
+static void oa_tc6_handle_link_change(struct net_device *netdev)
+{
+ phy_print_status(netdev->phydev);
+}
+
+static int oa_tc6_mdiobus_direct_read(struct mii_bus *bus, int addr, int regnum)
+{
+ struct oa_tc6 *tc6 = bus->priv;
+ u32 regval;
+ bool ret;
+
+ ret = oa_tc6_read_register(tc6, OA_TC6_PHY_STD_REG_ADDR_BASE |
+ (regnum & OA_TC6_PHY_STD_REG_ADDR_MASK),
+ &regval);
+ if (ret)
+ return -ENODEV;
+
+ return regval;
+}
+
+static int oa_tc6_mdiobus_direct_write(struct mii_bus *bus, int addr, int regnum,
+ u16 val)
+{
+ struct oa_tc6 *tc6 = bus->priv;
+
+ return oa_tc6_write_register(tc6, OA_TC6_PHY_STD_REG_ADDR_BASE |
+ (regnum & OA_TC6_PHY_STD_REG_ADDR_MASK),
+ val);
+}
+
+static int oa_tc6_get_phy_c45_mms(int devnum)
+{
+ switch (devnum) {
+ case MDIO_MMD_PCS:
+ return OA_TC6_PHY_C45_PCS_MMS2;
+ case MDIO_MMD_PMAPMD:
+ return OA_TC6_PHY_C45_PMA_PMD_MMS3;
+ case MDIO_MMD_VEND2:
+ return OA_TC6_PHY_C45_VS_PLCA_MMS4;
+ case MDIO_MMD_AN:
+ return OA_TC6_PHY_C45_AUTO_NEG_MMS5;
+ case MDIO_MMD_POWER_UNIT:
+ return OA_TC6_PHY_C45_POWER_UNIT_MMS6;
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static int oa_tc6_mdiobus_read_c45(struct mii_bus *bus, int addr, int devnum,
+ int regnum)
+{
+ struct oa_tc6 *tc6 = bus->priv;
+ u32 regval;
+ int ret;
+
+ ret = oa_tc6_get_phy_c45_mms(devnum);
+ if (ret < 0)
+ return ret;
+
+ ret = oa_tc6_read_register(tc6, (ret << 16) | regnum, &regval);
+ if (ret)
+ return ret;
+
+ return regval;
+}
+
+static int oa_tc6_mdiobus_write_c45(struct mii_bus *bus, int addr, int devnum,
+ int regnum, u16 val)
+{
+ struct oa_tc6 *tc6 = bus->priv;
+ int ret;
+
+ ret = oa_tc6_get_phy_c45_mms(devnum);
+ if (ret < 0)
+ return ret;
+
+ return oa_tc6_write_register(tc6, (ret << 16) | regnum, val);
+}
+
+static int oa_tc6_mdiobus_register(struct oa_tc6 *tc6)
+{
+ int ret;
+
+ tc6->mdiobus = mdiobus_alloc();
+ if (!tc6->mdiobus) {
+ netdev_err(tc6->netdev, "MDIO bus alloc failed\n");
+ return -ENOMEM;
+ }
+
+ tc6->mdiobus->priv = tc6;
+ tc6->mdiobus->read = oa_tc6_mdiobus_direct_read;
+ tc6->mdiobus->write = oa_tc6_mdiobus_direct_write;
+ tc6->mdiobus->read_c45 = oa_tc6_mdiobus_read_c45;
+ tc6->mdiobus->write_c45 = oa_tc6_mdiobus_write_c45;
+ tc6->mdiobus->name = "oa-tc6-mdiobus";
+ tc6->mdiobus->parent = tc6->dev;
+
+ snprintf(tc6->mdiobus->id, ARRAY_SIZE(tc6->mdiobus->id), "%s",
+ dev_name(&tc6->spi->dev));
+
+ ret = mdiobus_register(tc6->mdiobus);
+ if (ret) {
+ netdev_err(tc6->netdev, "Could not register MDIO bus\n");
+ mdiobus_free(tc6->mdiobus);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void oa_tc6_mdiobus_unregister(struct oa_tc6 *tc6)
+{
+ mdiobus_unregister(tc6->mdiobus);
+ mdiobus_free(tc6->mdiobus);
+}
+
+static int oa_tc6_phy_init(struct oa_tc6 *tc6)
+{
+ int ret;
+
+ ret = oa_tc6_check_phy_reg_direct_access_capability(tc6);
+ if (ret) {
+ netdev_err(tc6->netdev,
+ "Direct PHY register access is not supported by the MAC-PHY\n");
+ return ret;
+ }
+
+ ret = oa_tc6_mdiobus_register(tc6);
+ if (ret)
+ return ret;
+
+ tc6->phydev = phy_find_first(tc6->mdiobus);
+ if (!tc6->phydev) {
+ netdev_err(tc6->netdev, "No PHY found\n");
+ oa_tc6_mdiobus_unregister(tc6);
+ return -ENODEV;
+ }
+
+ tc6->phydev->is_internal = true;
+ ret = phy_connect_direct(tc6->netdev, tc6->phydev,
+ &oa_tc6_handle_link_change,
+ PHY_INTERFACE_MODE_INTERNAL);
+ if (ret) {
+ netdev_err(tc6->netdev, "Can't attach PHY to %s\n",
+ tc6->mdiobus->id);
+ oa_tc6_mdiobus_unregister(tc6);
+ return ret;
+ }
+
+ phy_attached_info(tc6->netdev->phydev);
+
+ return 0;
+}
+
+static void oa_tc6_phy_exit(struct oa_tc6 *tc6)
+{
+ phy_disconnect(tc6->phydev);
+ oa_tc6_mdiobus_unregister(tc6);
+}
+
static int oa_tc6_read_status0(struct oa_tc6 *tc6)
{
u32 regval;
@@ -351,11 +549,12 @@ static int oa_tc6_unmask_macphy_error_interrupts(struct oa_tc6 *tc6)
/**
* oa_tc6_init - allocates and initializes oa_tc6 structure.
* @spi: device with which data will be exchanged.
+ * @netdev: network device interface structure.
*
* Returns pointer reference to the oa_tc6 structure if the MAC-PHY
* initialization is successful otherwise NULL.
*/
-struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
+struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev)
{
struct oa_tc6 *tc6;
int ret;
@@ -365,6 +564,8 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
return NULL;

tc6->spi = spi;
+ tc6->netdev = netdev;
+ SET_NETDEV_DEV(netdev, &spi->dev);
mutex_init(&tc6->spi_ctrl_lock);

/* Set the SPI controller to pump at realtime priority */
@@ -395,10 +596,27 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
return NULL;
}

+ ret = oa_tc6_phy_init(tc6);
+ if (ret) {
+ dev_err(&tc6->spi->dev,
+ "MAC internal PHY initialization failed: %d\n", ret);
+ return NULL;
+ }
+
return tc6;
}
EXPORT_SYMBOL_GPL(oa_tc6_init);

+/**
+ * oa_tc6_exit - exit function.
+ * @tc6: oa_tc6 struct.
+ */
+void oa_tc6_exit(struct oa_tc6 *tc6)
+{
+ oa_tc6_phy_exit(tc6);
+}
+EXPORT_SYMBOL_GPL(oa_tc6_exit);
+
MODULE_DESCRIPTION("OPEN Alliance 10BASEâ??T1x MACâ??PHY Serial Interface Lib");
MODULE_AUTHOR("Parthiban Veerasooran <parthiban.veerasooran@xxxxxxxxxxxxx>");
MODULE_LICENSE("GPL");
diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
index 534ca7d1b061..dbb229c6f602 100644
--- a/drivers/net/phy/microchip_t1s.c
+++ b/drivers/net/phy/microchip_t1s.c
@@ -268,6 +268,24 @@ static int lan86xx_read_status(struct phy_device *phydev)
return 0;
}

+static int lan865x_phy_read_mmd(struct phy_device *phydev, int devnum,
+ u16 regnum)
+{
+ struct mii_bus *bus = phydev->mdio.bus;
+ int addr = phydev->mdio.addr;
+
+ return bus->read_c45(bus, addr, devnum, regnum);
+}
+
+static int lan865x_phy_write_mmd(struct phy_device *phydev, int devnum,
+ u16 regnum, u16 val)
+{
+ struct mii_bus *bus = phydev->mdio.bus;
+ int addr = phydev->mdio.addr;
+
+ return bus->write_c45(bus, addr, devnum, regnum, val);
+}
+
static struct phy_driver microchip_t1s_driver[] = {
{
PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVB1),
@@ -285,6 +303,8 @@ static struct phy_driver microchip_t1s_driver[] = {
.features = PHY_BASIC_T1S_P2MP_FEATURES,
.config_init = lan865x_revb0_config_init,
.read_status = lan86xx_read_status,
+ .read_mmd = lan865x_phy_read_mmd,
+ .write_mmd = lan865x_phy_write_mmd,
.get_plca_cfg = genphy_c45_plca_get_cfg,
.set_plca_cfg = genphy_c45_plca_set_cfg,
.get_plca_status = genphy_c45_plca_get_status,
diff --git a/include/linux/oa_tc6.h b/include/linux/oa_tc6.h
index 85aeecf87306..606ba9f1e663 100644
--- a/include/linux/oa_tc6.h
+++ b/include/linux/oa_tc6.h
@@ -7,11 +7,13 @@
* Author: Parthiban Veerasooran <parthiban.veerasooran@xxxxxxxxxxxxx>
*/

+#include <linux/etherdevice.h>
#include <linux/spi/spi.h>

struct oa_tc6;

-struct oa_tc6 *oa_tc6_init(struct spi_device *spi);
+struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev);
+void oa_tc6_exit(struct oa_tc6 *tc6);
int oa_tc6_write_register(struct oa_tc6 *tc6, u32 address, u32 value);
int oa_tc6_write_registers(struct oa_tc6 *tc6, u32 address, u32 value[],
u8 length);
diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h
index c0c8ec995b06..f0d3f268240d 100644
--- a/include/uapi/linux/mdio.h
+++ b/include/uapi/linux/mdio.h
@@ -23,6 +23,7 @@
#define MDIO_MMD_DTEXS 5 /* DTE Extender Sublayer */
#define MDIO_MMD_TC 6 /* Transmission Convergence */
#define MDIO_MMD_AN 7 /* Auto-Negotiation */
+#define MDIO_MMD_POWER_UNIT 13 /* PHY Power Unit */
#define MDIO_MMD_C22EXT 29 /* Clause 22 extension */
#define MDIO_MMD_VEND1 30 /* Vendor specific 1 */
#define MDIO_MMD_VEND2 31 /* Vendor specific 2 */
--
2.34.1