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), ®val);
>> + 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
