Date: Fri, 24 May 2024 20:52:22 +0000
From: Selvamani Rajagopal <Selvamani.Rajagopal@...emi.com>
To: "Parthiban.Veerasooran@...rochip.com"
	<Parthiban.Veerasooran@...rochip.com>,
        "andrew@...n.ch" <andrew@...n.ch>
CC: "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com"
	<edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "horms@...nel.org"
	<horms@...nel.org>,
        "saeedm@...dia.com" <saeedm@...dia.com>,
        "anthony.l.nguyen@...el.com" <anthony.l.nguyen@...el.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "corbet@....net" <corbet@....net>,
        "linux-doc@...r.kernel.org"
	<linux-doc@...r.kernel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "krzysztof.kozlowski+dt@...aro.org" <krzysztof.kozlowski+dt@...aro.org>,
        "conor+dt@...nel.org" <conor+dt@...nel.org>,
        "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>,
        "Horatiu.Vultur@...rochip.com"
	<Horatiu.Vultur@...rochip.com>,
        "ruanjinjie@...wei.com"
	<ruanjinjie@...wei.com>,
        "Steen.Hegelund@...rochip.com"
	<Steen.Hegelund@...rochip.com>,
        "vladimir.oltean@....com"
	<vladimir.oltean@....com>,
        "UNGLinuxDriver@...rochip.com"
	<UNGLinuxDriver@...rochip.com>,
        "Thorsten.Kummermehr@...rochip.com"
	<Thorsten.Kummermehr@...rochip.com>,
        Piergiorgio Beruto
	<Pier.Beruto@...emi.com>,
        "Nicolas.Ferre@...rochip.com"
	<Nicolas.Ferre@...rochip.com>,
        "benjamin.bigler@...nformulastudent.ch"
	<benjamin.bigler@...nformulastudent.ch>
Subject: 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@...rochip.com
> <Parthiban.Veerasooran@...rochip.com>
> Sent: Friday, May 10, 2024 4:22 AM
> To: andrew@...n.ch
> Cc: davem@...emloft.net; edumazet@...gle.com; kuba@...nel.org;
> pabeni@...hat.com; horms@...nel.org; saeedm@...dia.com;
> anthony.l.nguyen@...el.com; netdev@...r.kernel.org; linux-
> kernel@...r.kernel.org; corbet@....net; linux-doc@...r.kernel.org;
> robh+dt@...nel.org; krzysztof.kozlowski+dt@...aro.org;
> conor+dt@...nel.org; devicetree@...r.kernel.org;
> Horatiu.Vultur@...rochip.com; ruanjinjie@...wei.com;
> Steen.Hegelund@...rochip.com; vladimir.oltean@....com;
> UNGLinuxDriver@...rochip.com;
> Thorsten.Kummermehr@...rochip.com; Piergiorgio Beruto
> <Pier.Beruto@...emi.com>; Selvamani Rajagopal
> <Selvamani.Rajagopal@...emi.com>; Nicolas.Ferre@...rochip.com;
> benjamin.bigler@...nformulastudent.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@...rochip.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
> >

Powered by blists - more mailing lists