RE: [PATCH net-next v4 00/12] Add support for OPEN Alliance 10BASE-T1x MACPHY Serial Interface

Andrew/Parthiban,

Sorry I couldn't get back earlier on this subject as I just had to time review the organization of the code with respect to our device. 
I have a proposal for the way MDIO APIs are implemented in common code (oa_tc6.c).

At present, mii_bus structure is in common code without any visibility to vendor specific code. Though this is a good design to get consistent
behavior between different vendors, we can't customize the functions stored in  read, write, read_c45, and write_c45 function pointers.

In our MDIO functions, we do certain things based on PHY ID, also our driver deal with vendor specific register, MMS 12 (refer Table 6 in section 9.1
Open Alliance MAC-PHY Serial interface specification document.)

I hope it is not uncommon to expect the ability to implement vendor specific mdio read/write. At present, there is no alternative
to functions like oa_tc6_mdiobus_read, oa_tc6_mdiobus_write, oa_tc6_mdiobus_read_c45, and oa_tc6_mdiobus_write_c45.

I am sure we can resolve this issue in many ways.  One way is to provide a public function to populate mii_bus pointer (tc6->mdiobus
in the code) from the vendor driver, or provide a way the pass when we call oa_tc6_init.

Vendors who don't require such customization can use existing functionality. Those who require customization, can have extra code
before calling this standard MDIO functions.

Sincerely
Selva

-----Original Message-----
From: Parthiban.Veerasooran@microchip.com
[off-list ref]
Sent: Friday, May 10, 2024 4:22 AM
To: andrew@lunn.ch
Cc: davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
pabeni@redhat.com; horms@kernel.org; saeedm@nvidia.com;
anthony.l.nguyen@intel.com; netdev@vger.kernel.org; linux-
kernel@vger.kernel.org; corbet@lwn.net; linux-doc@vger.kernel.org;
robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org;
conor+dt@kernel.org; devicetree@vger.kernel.org;
Horatiu.Vultur@microchip.com; ruanjinjie@huawei.com;
Steen.Hegelund@microchip.com; vladimir.oltean@nxp.com;
UNGLinuxDriver@microchip.com;
Thorsten.Kummermehr@microchip.com; Piergiorgio Beruto
[off-list ref]; Selvamani Rajagopal
[off-list ref]; Nicolas.Ferre@microchip.com;
benjamin.bigler@bernformulastudent.ch
Subject: Re: [PATCH net-next v4 00/12] Add support for OPEN Alliance
10BASE-T1x MACPHY Serial Interface

[External Email]: This email arrived from an external source - Please
exercise caution when opening any attachments or clicking on links.

Hi Andrew,

On 10/05/24 2:09 am, Andrew Lunn wrote:

quoted

EXTERNAL EMAIL: Do not click links or open attachments unless you

know the content is safe

quoted

On Thu, May 09, 2024 at 01:04:52PM +0000,

Parthiban.Veerasooran@microchip.com wrote:

quoted

quoted

Hi Andrew,

On 08/05/24 10:34 pm, Andrew Lunn wrote:

quoted

EXTERNAL EMAIL: Do not click links or open attachments unless you

know the content is safe

quoted

quoted

quoted

quoted

Yes. I tried this test. It works as expected.

quoted

     Each LAN8651 received approximately 3Mbps with lot of

"Receive buffer

quoted

quoted

quoted

quoted

overflow error". I think it is expected as the single SPI master has to
serve both LAN8651 at the same time and both LAN8651 will be

receiving

quoted

quoted

quoted

quoted

10Mbps on each.

Thanks for testing this.

This also shows the "Receive buffer overflow error" needs to go

away.

quoted

quoted

quoted

Either we don't care at all, and should not enable the interrupt, or
we do care and should increment a counter.

Thanks for your comments. I think, I would go for your 2nd proposal
because having "Receive buffer overflow error" enabled will indicate

the

quoted

quoted

cause of the poor performance.

Already we have,
tc6->netdev->stats.rx_dropped++;
to increment the rx dropped counter in case of receive buffer

overflow.

quoted

quoted

May be we can remove the print,
net_err_ratelimited("%s: Receive buffer overflow error\n",
tc6->netdev->name);
as it might lead to additional poor performance by adding some

delay.

quoted

quoted

Could you please provide your opinion on this?

This is your code. Ideally you should decide. I will only add review
comments if i think it is wrong. Any can decide between any correct
option.

Sure, thanks for your advice. Let me stick with the above proposal until
I get any others opinion.

Best regards,
Parthiban V

quoted

         Andrew