Message-ID: <296aa172-404e-414a-a56a-ca82b3e90397@microchip.com>
Date:   Tue, 31 Oct 2023 11:04:12 +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: [PATCH net-next v2 8/9] microchip: lan865x: add driver support
 for Microchip's LAN865X MACPHY

Hi Andrew,

On 24/10/23 8:03 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> +static int lan865x_set_hw_macaddr(struct net_device *netdev)
>> +{
>> +     u32 regval;
>> +     bool ret;
>> +     struct lan865x_priv *priv = netdev_priv(netdev);
>> +     const u8 *mac = netdev->dev_addr;
>> +
>> +     ret = oa_tc6_read_register(priv->tc6, LAN865X_MAC_NCR, &regval);
>> +     if (ret)
>> +             goto error_mac;
>> +     if ((regval & LAN865X_TXEN) | (regval & LAN865X_RXEN)) {
> 
> Would testing netif_running(netdev) be enough? That is a more common
> test you see. In fact, you already have that in
> lan865x_set_mac_address(). And in lan865x_probe() why would the
> hardware be enabled?
Ah yes, this check is not needed at all. Will correct it.
> 
> 
>> +             if (netif_msg_drv(priv))
>> +                     netdev_warn(netdev, "Hardware must be disabled for MAC setting\n");
>> +             return -EBUSY;
>> +     }
>> +     /* MAC address setting */
>> +     regval = (mac[3] << 24) | (mac[2] << 16) | (mac[1] << 8) | mac[0];
>> +     ret = oa_tc6_write_register(priv->tc6, LAN865X_MAC_SAB1, regval);
>> +     if (ret)
>> +             goto error_mac;
>> +
>> +     regval = (mac[5] << 8) | mac[4];
>> +     ret = oa_tc6_write_register(priv->tc6, LAN865X_MAC_SAT1, regval);
>> +     if (ret)
>> +             goto error_mac;
>> +
>> +     return 0;
>> +
>> +error_mac:
>> +     return -ENODEV;
> 
> oa_tc6_write_register() should return an error code, so return it.
Yes, noted. Will do it.
> 
>> +static int lan865x_set_mac_address(struct net_device *netdev, void *addr)
>> +{
>> +     struct sockaddr *address = addr;
>> +
>> +     if (netif_running(netdev))
>> +             return -EBUSY;
>> +
>> +     eth_hw_addr_set(netdev, address->sa_data);
>> +
>> +     return lan865x_set_hw_macaddr(netdev);
> 
> You should ideally only update the netdev MAC address, if you managed
> to change the value in the hardware.
Ah ok, then it is supposed to be like below, isn't it?

static int lan865x_set_mac_address(struct net_device *netdev, void *addr)
{
	struct sockaddr *address = addr;
	int ret;

	if (netif_running(netdev))
		return -EBUSY;

	ret = lan865x_set_hw_macaddr(netdev);
	if (ret)
		return ret;

	eth_hw_addr_set(netdev, address->sa_data);

	return 0;
}
> 
>> +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, LAN865X_MAC_HRT, hash_hi)) {
>> +                     if (netif_msg_timer(priv))
>> +                             netdev_err(netdev, "Failed to write reg_hashh");
>> +                     return;
>> +             }
>> +             if (oa_tc6_write_register(priv->tc6, LAN865X_MAC_HRB, hash_lo)) {
>> +                     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, LAN865X_MAC_HRT, regval)) {
>> +                     if (netif_msg_timer(priv))
>> +                             netdev_err(netdev, "Failed to write reg_hashh");
>> +                     return;
>> +             }
>> +             if (oa_tc6_write_register(priv->tc6, LAN865X_MAC_HRB, regval)) {
>> +                     if (netif_msg_timer(priv))
>> +                             netdev_err(netdev, "Failed to write reg_hashl");
>> +                     return;
>> +             }
>> +     }
>> +     if (oa_tc6_write_register(priv->tc6, LAN865X_MAC_NCFGR, regval)) {
>> +             if (netif_msg_timer(priv))
>> +                     netdev_err(netdev, "Failed to enable promiscuous mode");
>> +     }
>> +}
> 
> Another of those big functions which could be lots of simple functions
> each doing one thing.
Sure, will split into multiple functions.
> 
>> +/* Configures the number of bytes allocated to each buffer in the
>> + * transmit/receive queue. LAN865x supports only 64 and 32 bytes cps and also 64
>> + * is the default value. So it is enough to configure the queue buffer size only
>> + * for 32 bytes. Generally cps can't be changed during run time and also it is
>> + * configured in the device tree. The values for the Tx/Rx queue buffer size are
>> + * taken from the LAN865x datasheet.
>> + */
> 
> Its unclear why this needs to be a callback. Why not just set it after
> oa_tc6_init() returns?
oa_tc6_init() function internally calls oa_tc6_configure() function to 
configure different settings. At the end it writes SYNC bit to CONFIG0 
register which enables the MAC-PHY to start the data transfer. So the 
buffer configuration should be done prior to that. Since this is part of 
the configuration this callback is implemented.
> 
>> +static void lan865x_remove(struct spi_device *spi)
>> +{
>> +     struct lan865x_priv *priv = spi_get_drvdata(spi);
>> +
>> +     oa_tc6_exit(priv->tc6);
>> +     unregister_netdev(priv->netdev);
> 
> Is that the correct order? Seems like you should unregister the netdev
> first.
Ah yes, will correct it.
> 
>> +#ifdef CONFIG_ACPI
>> +static const struct acpi_device_id lan865x_acpi_ids[] = {
>> +     { .id = "LAN865X",
>> +     },
>> +     {},
> 
> Does this work? You don't have access to the device tree properties.
Going to remove all the OA specific configurations in the next revisions.

Best Regards,
Parthiban V
> 
>       Andrew
> 

Powered by blists - more mailing lists