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

[Selvamani Rajagopal]

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@xxxxxxxxxxxxx
> <Parthiban.Veerasooran@xxxxxxxxxxxxx>
> Sent: Friday, May 10, 2024 4:22 AM
> To: andrew@xxxxxxx
> Cc: davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx;
> pabeni@xxxxxxxxxx; horms@xxxxxxxxxx; saeedm@xxxxxxxxxx;
> anthony.l.nguyen@xxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; corbet@xxxxxxx; linux-doc@xxxxxxxxxxxxxxx;
> robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx;
> conor+dt@xxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> Horatiu.Vultur@xxxxxxxxxxxxx; ruanjinjie@xxxxxxxxxx;
> Steen.Hegelund@xxxxxxxxxxxxx; vladimir.oltean@xxxxxxx;
> UNGLinuxDriver@xxxxxxxxxxxxx;
> Thorsten.Kummermehr@xxxxxxxxxxxxx; Piergiorgio Beruto
> <Pier.Beruto@xxxxxxxxxx>; Selvamani Rajagopal
> <Selvamani.Rajagopal@xxxxxxxxxx>; Nicolas.Ferre@xxxxxxxxxxxxx;
> benjamin.bigler@xxxxxxxxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxx 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
> >