Re: [PATCH net-next v2 5/9] net: ethernet: oa_tc6: implement internal PHY initialization

[Parthiban.Veerasooran]

Hi Andrew,

On 24/10/23 6:26 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> +     /* Minimum supported Chunk Payload Size */
>>        mincps = FIELD_GET(MINCPS, regval);
>> +     /* Cut-Through Capability */
>>        ctc = (regval & CTC) ? true : false;
> 
> These comment should be in the patch which added these, not here.
Ah yes. Will correct it.
> 
>> +     /* Direct PHY Register Access Capability */
>> +     dprac = (regval & DPRAC) ? true : false;
>> +     /* Indirect PHY Register access Capability */
>> +     iprac = (regval & IPRAC) ? true : false;
>>
>>        regval = 0;
>>        oa_node = of_get_child_by_name(spi->dev.of_node, "oa-tc6");
>> @@ -242,7 +257,7 @@ static int oa_tc6_configure(struct oa_tc6 *tc6)
>>                        if (tc6->cps < mincps)
>>                                return -ENODEV;
>>                } else {
>> -                     tc6->cps = 64;
>> +                     tc6->cps = OA_TC6_MAX_CPS;
> 
> This also should of been in an earlier patch.
Yes, will correct it.
> 
>>                }
>>                if (of_property_present(oa_node, "oa-txcte")) {
>>                        /* Return error if the tx cut through mode is configured
>> @@ -266,8 +281,26 @@ static int oa_tc6_configure(struct oa_tc6 *tc6)
>>                        regval |= PROTE;
>>                        tc6->prote = true;
>>                }
>> +             if (of_property_present(oa_node, "oa-dprac")) {
>> +                     /* Return error if the direct phy register access mode
>> +                      * is configured but it is not supported by MAC-PHY.
>> +                      */
>> +                     if (dprac)
>> +                             tc6->dprac = true;
>> +                     else
>> +                             return -ENODEV;
>> +             }
> 
> This is not in the binding. Why do we even need to be able to
> configure it. Direct is faster, so use it is available. If not, use
> indirect. And if both dprac and iproc are false, dev_err() and
> -ENODEV.
Ok, I will remove this option even in the next patch and will go with 
the option read from the capability register (STDCAP).
> 
>> +static int oa_tc6_mdiobus_read(struct mii_bus *bus, int phy_id, int idx)
> 
> It would be good to put direct in the name. If somebody implements
> indirect, it will make the naming easier.
Yes sure.
> 
>> +{
>> +     struct oa_tc6 *tc6 = bus->priv;
>> +     u32 regval;
>> +     bool ret;
>> +
>> +     ret = oa_tc6_read_register(tc6, 0xFF00 | (idx & 0xFF), &regval);
>> +     if (ret)
>> +             return -ENODEV;
>> +
>> +     return regval;
>> +}
>> +
>> +static int oa_tc6_mdiobus_write(struct mii_bus *bus, int phy_id, int idx,
>> +                             u16 val)
>> +{
>> +     struct oa_tc6 *tc6 = bus->priv;
>> +
>> +     return oa_tc6_write_register(tc6, 0xFF00 | (idx & 0xFF), val);
>> +}
>> +
>> +static int oa_tc6_phy_init(struct oa_tc6 *tc6)
>> +{
>> +     int ret;
>> +
>> +     if (tc6->dprac) {
> 
> You can avoid the indentation by first checking indirect is the only
> choice, and doing a dev_err() followed by return -ENODEV.
Ah ok, will do it.
> 
>> +             tc6->mdiobus = mdiobus_alloc();
>> +             if (!tc6->mdiobus) {
>> +                     netdev_err(tc6->netdev, "MDIO bus alloc failed\n");
>> +                     return -ENODEV;
>> +             }
>> +
>> +             tc6->mdiobus->phy_mask = ~(u32)BIT(1);
> 
> Does the standard define this ? BIT(1), not BIT(0)?
Ok, I think here is a typo. Will correct it.
> 
>>   /**
>>    * oa_tc6_init - allocates and intializes oa_tc6 structure.
>>    * @spi: device with which data will be exchanged.
>> - * @prote: control data (register) read/write protection enable/disable.
> 
> Something else which should of been in the previous patch. Please look
> through this patch and find all the other instances.
Yes sure. Will correct it.
> 
>> + * @netdev: network device to use.
>>    *
>>    * Returns pointer reference to the oa_tc6 structure if all the memory
>>    * allocation success 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;
>>
>> @@ -395,15 +521,19 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
>>        if (!tc6)
>>                return NULL;
>>
>> +     /* Allocate memory for the control tx buffer used for SPI transfer. */
>>        tc6->ctrl_tx_buf = devm_kzalloc(&spi->dev, TC6_CTRL_BUF_SIZE, GFP_KERNEL);
>>        if (!tc6->ctrl_tx_buf)
>>                return NULL;
>>
>> +     /* Allocate memory for the control rx buffer used for SPI transfer. */
>>        tc6->ctrl_rx_buf = devm_kzalloc(&spi->dev, TC6_CTRL_BUF_SIZE, GFP_KERNEL);
>>        if (!tc6->ctrl_rx_buf)
>>                return NULL;
>>
>>        tc6->spi = spi;
>> +     tc6->netdev = netdev;
>> +     SET_NETDEV_DEV(netdev, &spi->dev);
>>
>>        /* Perform MAC-PHY software reset */
>>        if (oa_tc6_sw_reset(tc6)) {
>> @@ -417,10 +547,27 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
>>                return NULL;
>>        }
>>
>> +     /* Initialize PHY */
>> +     if (oa_tc6_phy_init(tc6)) {
>> +             dev_err(&spi->dev, "PHY initialization failed\n");
>> +             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/include/linux/oa_tc6.h b/include/linux/oa_tc6.h
>> index 378636fd9ca8..36b729c384ac 100644
>> --- a/include/linux/oa_tc6.h
>> +++ b/include/linux/oa_tc6.h
>> @@ -5,54 +5,59 @@
>>    * Author: Parthiban Veerasooran <parthiban.veerasooran@xxxxxxxxxxxxx>
>>    */
>>
>> +#include <linux/etherdevice.h>
>>   #include <linux/spi/spi.h>
>>
>>   /* Control header */
>> -#define CTRL_HDR_DNC         BIT(31)         /* Data-Not-Control */
>> -#define CTRL_HDR_HDRB                BIT(30)         /* Received Header Bad */
>> -#define CTRL_HDR_WNR         BIT(29)         /* Write-Not-Read */
>> -#define CTRL_HDR_AID         BIT(28)         /* Address Increment Disable */
>> -#define CTRL_HDR_MMS         GENMASK(27, 24) /* Memory Map Selector */
>> -#define CTRL_HDR_ADDR                GENMASK(23, 8)  /* Address */
>> -#define CTRL_HDR_LEN         GENMASK(7, 1)   /* Length */
>> -#define CTRL_HDR_P           BIT(0)          /* Parity Bit */
>> +#define CTRL_HDR_DNC BIT(31)         /* Data-Not-Control */
>> +#define CTRL_HDR_HDRB        BIT(30)         /* Received Header Bad */
>> +#define CTRL_HDR_WNR BIT(29)         /* Write-Not-Read */
>> +#define CTRL_HDR_AID BIT(28)         /* Address Increment Disable */
>> +#define CTRL_HDR_MMS GENMASK(27, 24) /* Memory Map Selector */
>> +#define CTRL_HDR_ADDR        GENMASK(23, 8)  /* Address */
>> +#define CTRL_HDR_LEN GENMASK(7, 1)   /* Length */
>> +#define CTRL_HDR_P   BIT(0)          /* Parity Bit */
> 
> Please don't change the whitespace like this.
Ah yes, will correct it.

Best Regards,
Parthiban V
> 
>         Andrew