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

Selvamani Rajagopal
2024-05-24T22:57:05Z

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
> <Parthiban.Veerasooran@microchip.com>
> 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
> <Pier.Beruto@onsemi.com>; Selvamani Rajagopal
> <Selvamani.Rajagopal@onsemi.com>; 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:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> >
> > On Thu, May 09, 2024 at 01:04:52PM +0000,
> Parthiban.Veerasooran@microchip.com wrote:
> >> Hi Andrew,
> >>
> >> On 08/05/24 10:34 pm, Andrew Lunn wrote:
> >>> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> >>>
> >>>> Yes. I tried this test. It works as expected.
> >>>
> >>>> Each LAN8651 received approximately 3Mbps with lot of
> "Receive buffer
> >>>> 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
> >>>> 10Mbps on each.
> >>>
> >>> Thanks for testing this.
> >>>
> >>> This also shows the "Receive buffer overflow error" needs to go
> away.
> >>> 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
> >> 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.
> >>
> >> 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.
> >>
> >> 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
> >
> > Andrew
> >