Message-ID: <248dc5fd-f860-507b-6b3e-4be62cd91b2a@microchip.com>
Date: Tue, 19 Sep 2023 09:18:07 +0000
From: <Parthiban.Veerasooran@...rochip.com>
To: <andrew@...n.ch>
CC: <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<pabeni@...hat.com>, <robh+dt@...nel.org>,
	<krzysztof.kozlowski+dt@...aro.org>, <conor+dt@...nel.org>, <corbet@....net>,
	<Steen.Hegelund@...rochip.com>, <rdunlap@...radead.org>, <horms@...nel.org>,
	<casper.casan@...il.com>, <netdev@...r.kernel.org>,
	<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<linux-doc@...r.kernel.org>, <Horatiu.Vultur@...rochip.com>,
	<Woojung.Huh@...rochip.com>, <Nicolas.Ferre@...rochip.com>,
	<UNGLinuxDriver@...rochip.com>, <Thorsten.Kummermehr@...rochip.com>
Subject: Re: [RFC PATCH net-next 5/6] microchip: lan865x: add driver support
 for Microchip's LAN865X MACPHY

Hi Andrew,

On 14/09/23 7:21 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> +#define DRV_VERSION          "0.1"
> 
> This is pointless. The ethtool code will fill in the git hash which is
> a much more useful value to have.
Ah ok, so I can also remove the below code updating this info in the 
lan865x_get_drvinfo() function.

strscpy(info->version, DRV_VERSION, sizeof(info->version));
> 
>> +static void lan865x_handle_link_change(struct net_device *netdev)
>> +{
>> +     struct lan865x_priv *priv = netdev_priv(netdev);
>> +
>> +     phy_print_status(priv->phydev);
>> +}
>> +
>> +static int lan865x_mdiobus_read(struct mii_bus *bus, int phy_id, int idx)
>> +{
>> +     struct lan865x_priv *priv = bus->priv;
>> +     u32 regval;
>> +     bool ret;
>> +
>> +     ret = oa_tc6_read_register(priv->tc6, 0xFF00 | (idx & 0xFF), &regval, 1);
>> +     if (ret)
>> +             return -ENODEV;
>> +
>> +     return regval;
>> +}
>> +
>> +static int lan865x_mdiobus_write(struct mii_bus *bus, int phy_id, int idx,
>> +                              u16 regval)
>> +{
>> +     struct lan865x_priv *priv = bus->priv;
>> +     u32 value = regval;
>> +     bool ret;
>> +
>> +     ret = oa_tc6_write_register(priv->tc6, 0xFF00 | (idx & 0xFF), &value, 1);
>> +     if (ret)
>> +             return -ENODEV;
>> +
>> +     return 0;
>> +}
>> +
>> +static int lan865x_phy_init(struct lan865x_priv *priv)
>> +{
>> +     int ret;
>> +
>> +     priv->mdiobus = mdiobus_alloc();
>> +     if (!priv->mdiobus) {
>> +             netdev_err(priv->netdev, "MDIO bus alloc failed\n");
>> +             return -ENODEV;
>> +     }
>> +
>> +     priv->mdiobus->phy_mask = ~(u32)BIT(1);
>> +     priv->mdiobus->priv = priv;
>> +     priv->mdiobus->read = lan865x_mdiobus_read;
>> +     priv->mdiobus->write = lan865x_mdiobus_write;
> 
> The MDIO bus is part of the standard. So i would expect this to be in
> the library. From what i remember, there are two different ways to
> implement MDIO, either via the PHY registers being directly mapped
> into the register space, or indirect like this. And i think there is a
> status bit somewhere which tells you which is implemented? So please
> move this code into the library, but check the status bit and return
> ENODEV if the silicon does not actually implement this access method.
Sure, I can move this part to oa_tc6 lib. If I understand you correctly 
you are talking about the Standard Capabilities Register (0x0002) 
defined in the OPEN Alliance 10BASE-T1x MAC-PHY Serial Interface spec 
right? If so, the 9th bit of this register tells about Indirect PHY 
Register access Capability. Did you mean this bit? If so, this bit 
describes the below,

IPRAC - Indirect PHY Register Access Capability. Indicates if PHY 
registers are indirectly accessible through the MDIO/MDC registers MDIOACCn.

The MDIOACCn – MDIO Access Register 0-7 is defined under Register Memory 
Map 0 - Standard Control and Status Registers of the OA spec. As you 
said, some vendors may implement this and some may not be. But still we 
have PHY clause 45 registers indirect access through clause 22 registers 
namely 0xd and 0xe. Using them we will be able to create a MDIO virtual 
bus to access PHY clause 45 registers. We already have all the required 
API's provided by Linux for this approach.

https://elixir.bootlin.com/linux/v6.6-rc2/source/drivers/net/phy/phy-core.c#L529

Microchip's LAN865x doesn't support OA provided indirect access of PHY 
registers. But it supports PHY registers indirect access through clause 
22 registers namely 0xd and 0xe. Also it supports direct PHY register 
access within the SPI register memory space.

As the PLCA registers are already standardized and mainlined it will be 
more straight forward if the driver implements the PHY register indirect 
access using clause 22. Otherwise there will be an extra effort for 
mapping those PLCA registers into SPI memory space to access them 
directly. Also every vendor will have their own SPI memory space to map.

https://elixir.bootlin.com/linux/v6.6-rc2/source/drivers/net/phy/mdio-open-alliance.h

So my proposal would be, I stick with this standard implementation of 
indirect PHY register access through clause 22 registers 0xd and 0xe. I 
believe almost all the vendors will have this access. Later we will add 
this feature tested (Since Microchip's LAN865x doesn't support this, I 
can't test) if there is a vendor supports this. What do you think?
> 
>> +static int
>> +lan865x_set_link_ksettings(struct net_device *netdev,
>> +                        const struct ethtool_link_ksettings *cmd)
>> +{
>> +     struct lan865x_priv *priv = netdev_priv(netdev);
>> +     int ret = 0;
>> +
>> +     if (cmd->base.autoneg != AUTONEG_DISABLE ||
>> +         cmd->base.speed != SPEED_10 || cmd->base.duplex != DUPLEX_HALF) {
>> +             if (netif_msg_link(priv))
>> +                     netdev_warn(netdev, "Unsupported link setting");
>> +             ret = -EOPNOTSUPP;
>> +     } else {
>> +             if (netif_msg_link(priv))
>> +                     netdev_warn(netdev, "Hardware must be disabled to set link mode");
>> +             ret = -EBUSY;
>> +     }
>> +     return ret;
> 
> I would expect to see a call to phy_ethtool_ksettings_set()
> here. phylib should be able to do some of the validation.
Ah ok, doing the below will make the life easier.
.set_link_ksettings	= phy_ethtool_set_link_ksettings,
> 
>> +}
>> +
>> +static int
>> +lan865x_get_link_ksettings(struct net_device *netdev,
>> +                        struct ethtool_link_ksettings *cmd)
>> +{
>> +     ethtool_link_ksettings_zero_link_mode(cmd, supported);
>> +     ethtool_link_ksettings_add_link_mode(cmd, supported, 10baseT_Half);
>> +     ethtool_link_ksettings_add_link_mode(cmd, supported, TP);
>> +
>> +     cmd->base.speed = SPEED_10;
>> +     cmd->base.duplex = DUPLEX_HALF;
>> +     cmd->base.port  = PORT_TP;
>> +     cmd->base.autoneg = AUTONEG_DISABLE;
>> +
>> +     return 0;
> 
> phy_ethtool_ksettings_get().
Yes,
.get_link_ksettings	= phy_ethtool_get_link_ksettings,
> 
> I also think this can be moved along with the MDIO bus and PHY
> handling into the library.
Ok will do that.
> 
>> +static int lan865x_set_mac_address(struct net_device *netdev, void *addr)
>> +{
>> +     struct sockaddr *address = addr;
>> +
>> +     if (netif_running(netdev))
>> +             return -EBUSY;
>> +     if (!is_valid_ether_addr(address->sa_data))
>> +             return -EADDRNOTAVAIL;
> 
> Does the core allow an invalid MAC address be passed to the driver?
Ah yes, it is not needed here.
> 
>> +
>> +     eth_hw_addr_set(netdev, address->sa_data);
>> +     return lan865x_set_hw_macaddr(netdev);
>> +}
>> +
>> +static u32 lan865x_hash(u8 addr[ETH_ALEN])
>> +{
>> +     return (ether_crc(ETH_ALEN, addr) >> 26) & 0x3f;
>> +}
>> +
>> +static void lan865x_set_multicast_list(struct net_device *netdev)
>> +{
>> +     struct lan865x_priv *priv = netdev_priv(netdev);
>> +     u32 regval = 0;
>> +
>> +     if (netdev->flags & IFF_PROMISC) {
>> +             /* Enabling promiscuous mode */
>> +             regval |= MAC_PROMISCUOUS_MODE;
>> +             regval &= (~MAC_MULTICAST_MODE);
>> +             regval &= (~MAC_UNICAST_MODE);
>> +     } else if (netdev->flags & IFF_ALLMULTI) {
>> +             /* Enabling all multicast mode */
>> +             regval &= (~MAC_PROMISCUOUS_MODE);
>> +             regval |= MAC_MULTICAST_MODE;
>> +             regval &= (~MAC_UNICAST_MODE);
>> +     } else if (!netdev_mc_empty(netdev)) {
>> +             /* Enabling specific multicast addresses */
>> +             struct netdev_hw_addr *ha;
>> +             u32 hash_lo = 0;
>> +             u32 hash_hi = 0;
>> +
>> +             netdev_for_each_mc_addr(ha, netdev) {
>> +                     u32 bit_num = lan865x_hash(ha->addr);
>> +                     u32 mask = 1 << (bit_num & 0x1f);
>> +
>> +                     if (bit_num & 0x20)
>> +                             hash_hi |= mask;
>> +                     else
>> +                             hash_lo |= mask;
>> +             }
>> +             if (oa_tc6_write_register(priv->tc6, REG_MAC_HASHH, &hash_hi, 1)) {
>> +                     if (netif_msg_timer(priv))
>> +                             netdev_err(netdev, "Failed to write reg_hashh");
>> +                     return;
>> +             }
>> +             if (oa_tc6_write_register(priv->tc6, REG_MAC_HASHL, &hash_lo, 1)) {
>> +                     if (netif_msg_timer(priv))
>> +                             netdev_err(netdev, "Failed to write reg_hashl");
>> +                     return;
>> +             }
>> +             regval &= (~MAC_PROMISCUOUS_MODE);
>> +             regval &= (~MAC_MULTICAST_MODE);
>> +             regval |= MAC_UNICAST_MODE;
>> +     } else {
>> +             /* enabling local mac address only */
>> +             if (oa_tc6_write_register(priv->tc6, REG_MAC_HASHH, &regval, 1)) {
>> +                     if (netif_msg_timer(priv))
>> +                             netdev_err(netdev, "Failed to write reg_hashh");
>> +                     return;
>> +             }
>> +             if (oa_tc6_write_register(priv->tc6, REG_MAC_HASHL, &regval, 1)) {
>> +                     if (netif_msg_timer(priv))
>> +                             netdev_err(netdev, "Failed to write reg_hashl");
>> +                     return;
>> +             }
>> +     }
>> +     if (oa_tc6_write_register(priv->tc6, REG_MAC_NW_CONFIG, &regval, 1)) {
>> +             if (netif_msg_timer(priv))
>> +                     netdev_err(netdev, "Failed to enable promiscuous mode");
>> +     }
>> +}
>> +
>> +static netdev_tx_t lan865x_send_packet(struct sk_buff *skb,
>> +                                    struct net_device *netdev)
>> +{
>> +     struct lan865x_priv *priv = netdev_priv(netdev);
>> +
>> +     return oa_tc6_send_eth_pkt(priv->tc6, skb);
>> +}
>> +
>> +static int lan865x_hw_disable(struct lan865x_priv *priv)
>> +{
>> +     u32 regval = NW_DISABLE;
>> +
>> +     if (oa_tc6_write_register(priv->tc6, REG_MAC_NW_CTRL, &regval, 1))
>> +             return -ENODEV;
>> +
>> +     return 0;
>> +}
>> +
>> +static int lan865x_net_close(struct net_device *netdev)
>> +{
>> +     struct lan865x_priv *priv = netdev_priv(netdev);
>> +     int ret;
>> +
>> +     netif_stop_queue(netdev);
>> +     ret = lan865x_hw_disable(priv);
>> +     if (ret) {
>> +             if (netif_msg_ifup(priv))
>> +                     netdev_err(netdev, "Failed to disable the hardware\n");
>> +             return ret;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static int lan865x_hw_enable(struct lan865x_priv *priv)
>> +{
>> +     u32 regval = NW_TX_STATUS | NW_RX_STATUS;
>> +
>> +     if (oa_tc6_write_register(priv->tc6, REG_MAC_NW_CTRL, &regval, 1))
>> +             return -ENODEV;
>> +
>> +     return 0;
>> +}
>> +
>> +static int lan865x_net_open(struct net_device *netdev)
>> +{
>> +     struct lan865x_priv *priv = netdev_priv(netdev);
>> +     int ret;
>> +
>> +     if (!is_valid_ether_addr(netdev->dev_addr)) {
>> +             if (netif_msg_ifup(priv))
>> +                     netdev_err(netdev, "Invalid MAC address %pm", netdev->dev_addr);
>> +             return -EADDRNOTAVAIL;
> 
> Using a random MAC address is the normal workaround for not having a
> valid MAC address via OTP flash etc.
Ah ok, you mean to use eth_hw_addr_random(netdev) instead of returning 
error.
> 
> 
>> +static int lan865x_get_dt_data(struct lan865x_priv *priv)
>> +{
>> +     struct spi_device *spi = priv->spi;
>> +     int ret;
>> +
>> +     if (of_property_present(spi->dev.of_node, "oa-chunk-size")) {
>> +             ret = of_property_read_u32(spi->dev.of_node, "oa-chunk-size",
>> +                                        &priv->cps);
>> +             if (ret < 0)
>> +                     return ret;
>> +     } else {
>> +             priv->cps = 64;
>> +             dev_info(&spi->dev, "Property oa-chunk-size is not found in dt and proceeding with the size 64\n");
>> +     }
>> +
>> +     if (of_property_present(spi->dev.of_node, "oa-tx-cut-through"))
>> +             priv->txcte = true;
>> +     else
>> +             dev_info(&spi->dev, "Property oa-tx-cut-through is not found in dt and proceeding with tx store and forward mode\n");
> 
> Please remove all these dev_info() prints. The device tree binding
> should make it clear what the defaults are when not specified in DT.
Ah ok, will remove them and update device tree binding.
> 
>> +
>> +     if (of_property_present(spi->dev.of_node, "oa-rx-cut-through"))
>> +             priv->rxcte = true;
>> +     else
>> +             dev_info(&spi->dev, "Property oa-rx-cut-through is not found in dt and proceeding with rx store and forward mode\n");
>> +
>> +     if (of_property_present(spi->dev.of_node, "oa-protected"))
>> +             priv->protected = true;
>> +     else
>> +             dev_info(&spi->dev, "Property oa-protected is not found in dt and proceeding with protection enabled\n");
> 
> Which of these are proprietary properties, and which are part of the
> standard? Please move parsing all the standard properties into the
> library.
Ah ok good idea. Will do that.
> 
>> +static int lan865x_probe(struct spi_device *spi)
>> +{
> 
> ...
> 
>> +
>> +     phy_start(priv->phydev);
>> +     return 0;
> 
> phy_start() is normally done in open, not probe.
Ok will move it.

Best Regards,
Parthiban V
> 
>              Andrew

Powered by blists - more mailing lists