| Message ID | 20240418125648.372526-1-Parthiban.Veerasooran@microchip.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add support for OPEN Alliance 10BASE-T1x MACPHY Serial Interface | expand |
On Thu, Apr 18, 2024 at 06:26:36PM +0530, Parthiban Veerasooran wrote: > This patch series contain the below updates, > - Adds support for OPEN Alliance 10BASE-T1x MACPHY Serial Interface in the > net/ethernet/oa_tc6.c. > Link to the spec: > ----------------- > https://opensig.org/download/document/OPEN_Alliance_10BASET1x_MAC-PHY_Serial_Interface_V1.1.pdf > > - Adds driver support for Microchip LAN8650/1 Rev.B1 10BASE-T1S MACPHY > Ethernet driver in the net/ethernet/microchip/lan865x/lan865x.c. > Link to the product: > -------------------- > https://www.microchip.com/en-us/product/lan8650 I will get around to reviewing this soon, i promise. To the OnSemi people: Have you tried your driver/device using this framework? It would be good to have a second vendors device using it, just to show no vendor specific code has slipped into the framework. Thanks Andrew
> Testing Details: > ---------------- > The driver performance was tested using iperf3 in the below two setups > separately. > > Setup 1: > -------- > Node 0 - Raspberry Pi 4 with LAN8650 MAC-PHY > Node 1 - Raspberry Pi 4 with EVB-LAN8670-USB USB Stick > > Setup 2: > -------- > Node 0 - SAMA7G54-EK with LAN8650 MAC-PHY > Node 1 - Raspberry Pi 4 with EVB-LAN8670-USB USB Stick Would it be possible to chain these two setups together by adding two USB dongles to one of the Ri 4s? If i remember correctly, there were reports of issues when two devices were using the framework at once. Thanks Andrew
On Mon, Apr 22, 2024 at 10:08:23PM +0200, Andrew Lunn wrote: > > Testing Details: > > ---------------- > > The driver performance was tested using iperf3 in the below two setups > > separately. > > > > Setup 1: > > -------- > > Node 0 - Raspberry Pi 4 with LAN8650 MAC-PHY > > Node 1 - Raspberry Pi 4 with EVB-LAN8670-USB USB Stick > > > > Setup 2: > > -------- > > Node 0 - SAMA7G54-EK with LAN8650 MAC-PHY > > Node 1 - Raspberry Pi 4 with EVB-LAN8670-USB USB Stick > > Would it be possible to chain these two setups together by adding two > USB dongles to one of the Ri 4s? If i remember correctly, there were > reports of issues when two devices were using the framework at once. Sorry, that makes no sense. Your USB device is unlikely to be doing USB->SPI->MAC-PHY. Do you have two LAN8650 MAC-PHY you can connect to one host? Andrew
On Thu, Apr 18, 2024 at 06:26:36PM +0530, Parthiban Veerasooran wrote: > This patch series contain the below updates, > - Adds support for OPEN Alliance 10BASE-T1x MACPHY Serial Interface in the > net/ethernet/oa_tc6.c. > Link to the spec: > ----------------- > https://opensig.org/download/document/OPEN_Alliance_10BASET1x_MAC-PHY_Serial_Interface_V1.1.pdf > > - Adds driver support for Microchip LAN8650/1 Rev.B1 10BASE-T1S MACPHY > Ethernet driver in the net/ethernet/microchip/lan865x/lan865x.c. > Link to the product: > -------------------- > https://www.microchip.com/en-us/product/lan8650 The patchset did not apply cleanly to net-next: https://patchwork.kernel.org/project/netdevbpf/list/?series=&submitter=206056&state=*&q=&archive=&delegate= Which means it did not get any of the standard automatic testing :-( Andrew
Hi Andrew, On 23/04/24 4:53 am, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Mon, Apr 22, 2024 at 10:08:23PM +0200, Andrew Lunn wrote: >>> Testing Details: >>> ---------------- >>> The driver performance was tested using iperf3 in the below two setups >>> separately. >>> >>> Setup 1: >>> -------- >>> Node 0 - Raspberry Pi 4 with LAN8650 MAC-PHY >>> Node 1 - Raspberry Pi 4 with EVB-LAN8670-USB USB Stick >>> >>> Setup 2: >>> -------- >>> Node 0 - SAMA7G54-EK with LAN8650 MAC-PHY >>> Node 1 - Raspberry Pi 4 with EVB-LAN8670-USB USB Stick >> >> Would it be possible to chain these two setups together by adding two >> USB dongles to one of the Ri 4s? If i remember correctly, there were >> reports of issues when two devices were using the framework at once. > > Sorry, that makes no sense. Your USB device is unlikely to be doing > USB->SPI->MAC-PHY. Do you have two LAN8650 MAC-PHY you can connect to > one host? Yes. I tried this test. It works as expected. Setup: ------ ------------------- --------------------------- | RPI4 1 | | RPI4 2 | | --------------- | N/W 1| ----------------------- | | | 1st LAN8651 | |<---->| | 1st EVB-LAN8670-USB | | | --------------- | | ----------------------- | | | | | | --------------- | N/W 2| ----------------------- | | | 2nd LAN8651 | |<---->| | 2nd EVB-LAN8670-USB | | | --------------- | | ----------------------- | ------------------- --------------------------- |---> 1st LAN8651 ---> IP: 192.168.5.100 RPI4 1 | |---> 2nd LAN8651 ---> IP: 192.168.5.100 |---> 1st EVB-LAN8670-USB ---> IP: 192.168.5.101 RPI4 2 | |---> 2nd EVB-LAN8670-USB ---> IP: 192.168.6.101 Test case 1: ------------ RPI4 1 (Sender): ---------------- $ iperf3 -c 192.168.5.101 -u -b 5M -i 1 -t 0 -p 5001 $ iperf3 -c 192.168.6.101 -u -b 5M -i 1 -t 0 -p 5002 RPI4 2 (Receiver): ------------------ $ iperf3 -s -p 5001 RPI4 3 (Receiver): ------------------ $ iperf3 -s -p 5002 Result on RPI4 side: -------------------- Each LAN8651 transmits 5Mbps without any error. Test case 2: ------------ RPI4 1 (Sender): ---------------- $ iperf3 -c 192.168.5.101 -u -b 10M -i 1 -t 0 -p 5001 $ iperf3 -c 192.168.6.101 -u -b 10M -i 1 -t 0 -p 5002 RPI4 2 (Receiver): ------------------ $ iperf3 -s -p 5001 RPI4 3 (Receiver): ------------------ $ iperf3 -s -p 5002 Result on RPI4 side: -------------------- Each LAN8651 transmitting approximately 5 or 6Mbps without any error. Test case 3: ----------------------- RPI4 1 (Receiver): ---------------- $ iperf3 -s -p 5001 $ iperf3 -s -p 5002 RPI4 2 (Sender): ---------------- $ iperf3 -c 192.168.5.100 -u -b 5M -i 1 -t 0 -p 5001 RPI4 3 (Sender): ---------------- $ iperf3 -c 192.168.6.100 -u -b 5M -i 1 -t 0 -p 5002 Result on RPI4 side: -------------------- Each LAN8651 received 5Mbps without any error. Test case 4: ----------------------- RPI4 1 (Receiver): ------------------ $ iperf3 -s -p 5001 $ iperf3 -s -p 5002 RPI4 2 (Sender): ---------------- $ iperf3 -c 192.168.5.100 -u -b 10M -i 1 -t 0 -p 5001 RPI4 3 (Sender): ---------------- $ iperf3 -c 192.168.6.100 -u -b 10M -i 1 -t 0 -p 5002 Result on RPI4 side: -------------------- 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. Please let me know if I have any misunderstanding here or you need more details on the test cases. Best regards, Parthiban V > > Andrew >
> 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. Andrew
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? Best regards, Parthiban V > > Andrew >
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. Andrew
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 >
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 > >
> 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 That is a bad design. Vendor specific PHY registers should be in MMS 4 which is MMD 31, where the PHY driver can access them. Table 6 says: "PHY – Vendor Specific" for MMS 4, so clearly that is where the standards committee expected PHY vendor registers to be. Anyway, does the PHY driver actually need to access MMS 12? Or can the MAC driver do it? That is the same question i asked Ramón about the Microchip part. We really should avoid layering violations as much as we can, and we should not have the framework make it easy to violate layering. We want all such horrible hacks hidden in the MAC driver which needs such horrible hacks because of bad design. Andrew
Hi Andrew, In reality, it is not the PHY having register in MMS12, and not even the MAC. These are really "chip-specific" registers, unrelated to networking (e.g., GPIOs, HW diagnostics, etc.). They are in MMS12 exactly because they cannot be considered PHY functions, nor MAC functions. And we have PHY specific registers in MMS4, just as you said. Although, I think it is a good idea anyway to allow the MACPHY drivers to hook into / extend the MDIO access functions. If anything, because of the hacks you mentioned. But also to allow vendor-specific extensions. Makes sense? Thanks, Piergiorgio -----Original Message----- From: Andrew Lunn <andrew@lunn.ch> Sent: 24 May, 2024 23:27 To: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com> Cc: Parthiban.Veerasooran@microchip.com; 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>; 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. > 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 That is a bad design. Vendor specific PHY registers should be in MMS 4 which is MMD 31, where the PHY driver can access them. Table 6 says: "PHY – Vendor Specific" for MMS 4, so clearly that is where the standards committee expected PHY vendor registers to be. Anyway, does the PHY driver actually need to access MMS 12? Or can the MAC driver do it? That is the same question i asked Ramón about the Microchip part. We really should avoid layering violations as much as we can, and we should not have the framework make it easy to violate layering. We want all such horrible hacks hidden in the MAC driver which needs such horrible hacks because of bad design. Andrew
> In reality, it is not the PHY having register in MMS12, and not even > the MAC. These are really "chip-specific" registers, unrelated to > networking (e.g., GPIOs, HW diagnostics, etc.). Having a GPIO driver within the MAC driver is O.K. For hardware diagnostics you should be using devlink, which many MAC drivers have. So i don't see a need for the PHY driver to access MMS 12. Anyway, we can do a real review when you post your code. > Although, I think it is a good idea anyway to allow the MACPHY > drivers to hook into / extend the MDIO access functions. If > anything, because of the hacks you mentioned. But also to allow > vendor-specific extensions. But we don't want vendor specific extensions. OS 101, the OS is there to make all hardware look the same. And in general, it is not often that vendors actually come up with anything unique. And if they do, and it is useful, other vendors will copy it. So rather than doing vendor specific extensions, you should be thinking about how to export it in a way which is common across multiple vendors. Andrew
> Having a GPIO driver within the MAC driver is O.K. For hardware diagnostics you should be using devlink, which many MAC drivers have. So i don't see a need for the PHY driver to access MMS 12. What I was trying to say is that I actually agree the PHY driver does not need to access MMS12. But the MAC driver might need to access MMS-es for vendor specific stuff. In our case, there is a model specific register we need to access during probe. > But we don't want vendor specific extensions. OS 101, the OS is there to make all hardware look the same. And in general, it is not often that vendors actually come up with anything unique. And if they do, and it is useful, other vendors will copy it. So rather than doing vendor specific extensions, you should be thinking about how to export it in a way which is common across multiple vendors. Fair enough, let's keep it for "hacks" then. Still, I think there are features that -initially- are kind of vendor specific, but in the long run they turn into standards or de-facto standards. I assume we want to help this happening (step-wise), don't we? For example, one big feature I think at some point we should understand how to deal with, is topology discovery for multi-drop. Maybe you've heard about it already, but in short it is a feature that allows one PHY to measure the distance (or rather, the propagation delay) to another node on the same multi-drop segment. Knowing the cable Tpd (~5ns/m), this allows you to get also the physical distance. It is a cool feature that has been also standardized in OPEN alliance and many vendors are already implemented it. As for other stuff, we put a lot of effort in convincing the committees to standardize the registers too, and fortunately we did it. The "problem" with topology discovery is that it involves both physical layer functions (to activate the functions that do the actual measurement) but also "upper layer" protocols to communicate with the nodes and carry on the actual measurement. For this "upper layer" stuff we did not define a standard. In my view, we should probably create some PHY extensions in the kernel to activate the physical layer part, leaving the "protocol" to the userland. May I ask your opinion? Thanks, Piergiorgio
On Fri, May 24, 2024 at 10:08:54PM +0000, Piergiorgio Beruto wrote: > > Having a GPIO driver within the MAC driver is O.K. For hardware diagnostics you should be using devlink, which many MAC drivers have. So i don't see a need for the PHY driver to access MMS 12. > But the MAC driver might need to access MMS-es for vendor specific > stuff. In our case, there is a model specific register we need to > access during probe. Which is fine, and currently supported. Look at the Microchip driver, it access some registers at startup. The framwork just provides the core of moving packets around, and MDIO access. The rest is up to MAC driver. > Fair enough, let's keep it for "hacks" then. Still, I think there > are features that -initially- are kind of vendor specific, but in > the long run they turn into standards or de-facto standards. > I assume we want to help this happening (step-wise), don't we? Maybe, but maybe not. We have been developing MAC drivers for over 25 years. There are a number of mechanisms for exporting things to user space. We have to see if your features fit one of them. > For example, one big feature I think at some point we should > understand how to deal with, is topology discovery for multi-drop. > Maybe you've heard about it already, but in short it is a feature > that allows one PHY to measure the distance (or rather, the > propagation delay) to another node on the same multi-drop segment. > Knowing the cable Tpd (~5ns/m), this allows you to get also the physical distance. We already have something very similar to this. Cable testing results which make use of Time Domain Reflectromitery. I would look at re-using the API as much as possible. > In my view, we should probably create some PHY extensions in the > kernel to activate the physical layer part, leaving the "protocol" > to the userland. May I ask your opinion? Please look at how cable testing works. Andrew
Hello Andrew, I was reading back into the MACPHY specifications in OPEN Alliance, and it seems like MMS 10 to MMS 15 are actually allowed as vendor specific registers. See page 50. The specifications further say that vendor specific registers of the PHY that would normally be in MMD30-31 (ie, excluding the PLCA registers and the other OPEN standard registers) would go into MMS10 to MMS15. So I'm wondering, why is it bad to have vendor specific registers into MMD10 to MMD15? I think the framework should allow non-standard stuff to be mapped into these, no? Thanks, Piergiorgio -----Original Message----- From: Andrew Lunn <andrew@lunn.ch> Sent: 24 May, 2024 23:55 To: Piergiorgio Beruto <Pier.Beruto@onsemi.com> Cc: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>; Parthiban.Veerasooran@microchip.com; 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; 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. > In reality, it is not the PHY having register in MMS12, and not even > the MAC. These are really "chip-specific" registers, unrelated to > networking (e.g., GPIOs, HW diagnostics, etc.). Having a GPIO driver within the MAC driver is O.K. For hardware diagnostics you should be using devlink, which many MAC drivers have. So i don't see a need for the PHY driver to access MMS 12. Anyway, we can do a real review when you post your code. > Although, I think it is a good idea anyway to allow the MACPHY drivers > to hook into / extend the MDIO access functions. If anything, because > of the hacks you mentioned. But also to allow vendor-specific > extensions. But we don't want vendor specific extensions. OS 101, the OS is there to make all hardware look the same. And in general, it is not often that vendors actually come up with anything unique. And if they do, and it is useful, other vendors will copy it. So rather than doing vendor specific extensions, you should be thinking about how to export it in a way which is common across multiple vendors. Andrew
On Thu, May 30, 2024 at 09:43:56AM +0000, Piergiorgio Beruto wrote: > Hello Andrew, > > I was reading back into the MACPHY specifications in OPEN Alliance, and it seems like MMS 10 to MMS 15 are actually allowed as vendor specific registers. See page 50. > The specifications further say that vendor specific registers of the PHY that would normally be in MMD30-31 (ie, excluding the PLCA registers and the other OPEN standard registers) would go into MMS10 to MMS15. > > So I'm wondering, why is it bad to have vendor specific registers into MMD10 to MMD15? > I think the framework should allow non-standard stuff to be mapped into these, no? From an architecture perspicuity, PHY vendor specific registers should be in the PHY register address space. MAC vendor specific registers should be in the MAC register address space. It seems like the Microchip device has some PHY vendor specific registers in the MAC address space. That is bad. Both your and Microchip device is a single piece of silicon. But i doubt there is anything in the standard which actually requires this. The PHY could be discrete, on the end of an MDIO bus and an MII bus. That is the typical design for the last 30 years, and what linux is built around. The MAC should not assume anything about the PHY, the PHY should not assume anything about the MAC, because they are interchangeable. The framework does allow you to poke any register anywhere. But i would strongly avoid breaking the layering, it is going to cause you long term maintenance problems, and is ugly. Andrew
> From an architecture perspicuity, PHY vendor specific registers should be in the PHY register address space. MAC vendor specific registers should be in the MAC register address space. I agree 100%. The registers I'm talking about are not PHY or MAC specific. They are related to other functions. For example, configuring pins to output a clock, an SFD indication, a LED, or other. Some devices also can configure events driven by the PTP timer to toggle GPIOs, capture the timer on rising/falling edge of a GPIO or similar. > It seems like the Microchip device has some PHY vendor specific registers in the MAC address space. That is bad. I was not aware of that. I can tell you this is not my case. > Both your and Microchip device is a single piece of silicon. But i doubt there is anything in the standard which actually requires this. The PHY could be discrete, on the end of an MDIO bus and an MII bus. That is the typical design for the last 30 years, and what linux is built around. The MAC should not assume anything about the PHY, the PHY should not assume anything about the MAC, because they are interchangeable. Yes, to me the MAC and PHY must be separate entities even for the OPEN Alliance TC14/6 protocol. Besides, you can stuff whatever PHY within your MACPHY chip (e.g. 10BASE-T1L). In "my" driver you need to specify in the DTS the kind of net device and the PHY as a handle. > The framework does allow you to poke any register anywhere. But i would strongly avoid breaking the layering, it is going to cause you long term maintenance problems, and is ugly. On that we agree. Maybe I was just misunderstanding the earlier conversation where I thought you would not allow specific drivers to access MMS other than 0,1,4 and the ones that map to MMDs. If this is not the case, then I think I'm good. Thanks! --Pier
> On that we agree. Maybe I was just misunderstanding the earlier > conversation where I thought you would not allow specific drivers to > access MMS other than 0,1,4 and the ones that map to MMDs. I would be disappointed to see a driver access registers which is not in its address space. The LED driver can be in the MAC driver, or the PHY driver, depending on where its registers are. The PTP driver can be in the MAC driver, or the PHY driver, depending on where its registers are. Hopefully you don't have anything which is split over both addresses spaces. Andrew
Hi All, First of all, I thank all of you for the comments and response. In my opinion, the framework what we have in this patch series will support all the necessary features to enable basic 10Base-T1S Ethernet communication and also we tested this with Microchip LAN8650/1. If it is not supporting for other vendor's devices, then please let me know we can add necessary changes to support them. The basic idea with this patch series is to baseline an initial version which basically supports 10Base-T1S Ethernet communication. I agree we may have some more further features to be implemented in the framework but they can be done later once we have a basic version mainlined. We can't have all things together in the 1st version of the patch series which will create unnecessary deviations from our focus. So I would request all of you to give your comments on the existing implementation in the patch series to improve better. Once this version is mainlined we will discuss further to implement further features supported. I feel the current discussion doesn't have any impact on the existing implementation which supports basic 10Base-T1S Ethernet communication. Thanks for your understanding. Please let me know if you have any opinion on this. Best regards, Parthiban V On 30/05/24 3:13 pm, Piergiorgio Beruto wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Hello Andrew, > > I was reading back into the MACPHY specifications in OPEN Alliance, and it seems like MMS 10 to MMS 15 are actually allowed as vendor specific registers. See page 50. > The specifications further say that vendor specific registers of the PHY that would normally be in MMD30-31 (ie, excluding the PLCA registers and the other OPEN standard registers) would go into MMS10 to MMS15. > > So I'm wondering, why is it bad to have vendor specific registers into MMD10 to MMD15? > I think the framework should allow non-standard stuff to be mapped into these, no? > > Thanks, > Piergiorgio > > -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: 24 May, 2024 23:55 > To: Piergiorgio Beruto <Pier.Beruto@onsemi.com> > Cc: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>; Parthiban.Veerasooran@microchip.com; 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; 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. > >> In reality, it is not the PHY having register in MMS12, and not even >> the MAC. These are really "chip-specific" registers, unrelated to >> networking (e.g., GPIOs, HW diagnostics, etc.). > > Having a GPIO driver within the MAC driver is O.K. For hardware diagnostics you should be using devlink, which many MAC drivers have. So i don't see a need for the PHY driver to access MMS 12. > > Anyway, we can do a real review when you post your code. > >> Although, I think it is a good idea anyway to allow the MACPHY drivers >> to hook into / extend the MDIO access functions. If anything, because >> of the hacks you mentioned. But also to allow vendor-specific >> extensions. > > But we don't want vendor specific extensions. OS 101, the OS is there to make all hardware look the same. And in general, it is not often that vendors actually come up with anything unique. And if they do, and it is useful, other vendors will copy it. So rather than doing vendor specific extensions, you should be thinking about how to export it in a way which is common across multiple vendors. > > Andrew
> So I would request all of you to give your comments on the existing > implementation in the patch series to improve better. Once this version > is mainlined we will discuss further to implement further features > supported. I feel the current discussion doesn't have any impact on the > existing implementation which supports basic 10Base-T1S Ethernet > communication. Agreed. Lets focus on what we have now. https://patchwork.kernel.org/project/netdevbpf/patch/20240418125648.372526-2-Parthiban.Veerasooran@microchip.com/ Version 4 failed to apply. So we are missing all the CI tests. We need a v5 which cleanly applies to net-next in order for those tests to run. I think we should disable vendor interrupts by default, since we currently have no way to handle them. I had a quick look at the comments on the patches. I don't think we have any other big issues not agreed on. So please post a v5 with them all addressed and we will see what the CI says. Piergiorgio, if you have any real problems getting basic support for your device working with this framework, now would be a good time to raise the problems. Andrew
Hi Andrew, We're currently working on re-factoring our driver onto the framework. I will make sure we can give you a feedback ASAP. We're also trying to asses the performance difference between what we have now and what we can achieve after re-factorng. Thanks, Piergiorgio -----Original Message----- From: Andrew Lunn <andrew@lunn.ch> Sent: 31 May, 2024 14:37 To: Parthiban.Veerasooran@microchip.com Cc: Piergiorgio Beruto <Pier.Beruto@onsemi.com>; Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>; 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; Nicolas.Ferre@microchip.com; benjamin.bigler@bernformulastudent.ch; Viliam Vozar <Viliam.Vozar@onsemi.com>; Arndt Schuebel <Arndt.Schuebel@onsemi.com> 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. > So I would request all of you to give your comments on the existing > implementation in the patch series to improve better. Once this > version is mainlined we will discuss further to implement further > features supported. I feel the current discussion doesn't have any > impact on the existing implementation which supports basic 10Base-T1S > Ethernet communication. Agreed. Lets focus on what we have now. https://urldefense.com/v3/__https://patchwork.kernel.org/project/netdevbpf/patch/20240418125648.372526-2-Parthiban.Veerasooran@microchip.com/__;!!KkVubWw!n9QOIA72sKA9z72UFogHeBRnA8Hse9gmIqzNv27f7Tc-4dYH1KA__DfMSmln-uBotO-bnw3PC2qXbfRn$ Version 4 failed to apply. So we are missing all the CI tests. We need a v5 which cleanly applies to net-next in order for those tests to run. I think we should disable vendor interrupts by default, since we currently have no way to handle them. I had a quick look at the comments on the patches. I don't think we have any other big issues not agreed on. So please post a v5 with them all addressed and we will see what the CI says. Piergiorgio, if you have any real problems getting basic support for your device working with this framework, now would be a good time to raise the problems. Andrew
Hi Andrew, On 31/05/24 6:07 pm, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> So I would request all of you to give your comments on the existing >> implementation in the patch series to improve better. Once this version >> is mainlined we will discuss further to implement further features >> supported. I feel the current discussion doesn't have any impact on the >> existing implementation which supports basic 10Base-T1S Ethernet >> communication. > > Agreed. Lets focus on what we have now. Great. Thanks for your opinion on this. > > https://patchwork.kernel.org/project/netdevbpf/patch/20240418125648.372526-2-Parthiban.Veerasooran@microchip.com/ > > Version 4 failed to apply. So we are missing all the CI tests. We need > a v5 which cleanly applies to net-next in order for those tests to > run. Sure I will start preparing the v5 now. > > I think we should disable vendor interrupts by default, since we > currently have no way to handle them. OK, I will remove the patch which enables the interrupts. > > I had a quick look at the comments on the patches. I don't think we > have any other big issues not agreed on. So please post a v5 with them > all addressed and we will see what the CI says. Sure, thanks for the confirmation. Best regards, Parthiban V > > Piergiorgio, if you have any real problems getting basic support for > your device working with this framework, now would be a good time to > raise the problems. > > Andrew
Hi Piergiorgio, On 31/05/24 8:43 pm, Piergiorgio Beruto wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Hi Andrew, > We're currently working on re-factoring our driver onto the framework. > I will make sure we can give you a feedback ASAP. > > We're also trying to asses the performance difference between what we have now and what we can achieve after re-factorng. That's cool. As Andrew commented in the other email, let me know the feedback if you have any to be addressed in the v5 patch series to support the basic communication. Best regards, Parthiban V > > Thanks, > Piergiorgio > > -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: 31 May, 2024 14:37 > To: Parthiban.Veerasooran@microchip.com > Cc: Piergiorgio Beruto <Pier.Beruto@onsemi.com>; Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>; 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; Nicolas.Ferre@microchip.com; benjamin.bigler@bernformulastudent.ch; Viliam Vozar <Viliam.Vozar@onsemi.com>; Arndt Schuebel <Arndt.Schuebel@onsemi.com> > 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. > >> So I would request all of you to give your comments on the existing >> implementation in the patch series to improve better. Once this >> version is mainlined we will discuss further to implement further >> features supported. I feel the current discussion doesn't have any >> impact on the existing implementation which supports basic 10Base-T1S >> Ethernet communication. > > Agreed. Lets focus on what we have now. > > https://urldefense.com/v3/__https://patchwork.kernel.org/project/netdevbpf/patch/20240418125648.372526-2-Parthiban.Veerasooran@microchip.com/__;!!KkVubWw!n9QOIA72sKA9z72UFogHeBRnA8Hse9gmIqzNv27f7Tc-4dYH1KA__DfMSmln-uBotO-bnw3PC2qXbfRn$ > > Version 4 failed to apply. So we are missing all the CI tests. We need a v5 which cleanly applies to net-next in order for those tests to run. > > I think we should disable vendor interrupts by default, since we currently have no way to handle them. > > I had a quick look at the comments on the patches. I don't think we have any other big issues not agreed on. So please post a v5 with them all addressed and we will see what the CI says. > > Piergiorgio, if you have any real problems getting basic support for your device working with this framework, now would be a good time to raise the problems. > > Andrew
Parthiban/Andrew,
Couple of requests / suggestions after completing the integration of our drivers to the current framework.
1) Can we move memory map selector definitions (OA_TC6_PHY_C45_PCS_MMS2 and other 4 definitions) to the header file
include/linux/oa_tc6.h?
Also, if possible, could we add the MMS0, MMS1?. Our driver is using them. Of course, we could add it when we submit our driver.
2) If it not too late to ask, Is it possible to move interrupt handler to vendor's code? ( oa_tc6_macphy_isr ). Instead, we could provide a function
oa_tc6_wakeup(priv->oa_tc6) in oa_tc6.c, so that vendors' ISR code can use them to notify the work queue tc6->spi_wq.
This way, it will provide vendors' code an ability to deal with some of the interrupts. For example, our code deals with PHYINT bit. In the current
framework, interrupts are handled in the common framework (oa_tc6.c) and status bits are cleared there.
I believe that change would require vendors to call "request_irq", which should be OK.
Sincerely
Selva
> -----Original Message-----
> From: Parthiban.Veerasooran@microchip.com
> <Parthiban.Veerasooran@microchip.com>
> Sent: Sunday, June 2, 2024 11:56 PM
> To: Piergiorgio Beruto <Pier.Beruto@onsemi.com>; andrew@lunn.ch
> Cc: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>;
> 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;
> Nicolas.Ferre@microchip.com;
> benjamin.bigler@bernformulastudent.ch; Viliam Vozar
> <Viliam.Vozar@onsemi.com>; Arndt Schuebel
> <Arndt.Schuebel@onsemi.com>
> 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 Piergiorgio,
>
> On 31/05/24 8:43 pm, Piergiorgio Beruto wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> >
> > Hi Andrew,
> > We're currently working on re-factoring our driver onto the
> framework.
> > I will make sure we can give you a feedback ASAP.
> >
> > We're also trying to asses the performance difference between what
> we have now and what we can achieve after re-factorng.
> That's cool. As Andrew commented in the other email, let me know the
> feedback if you have any to be addressed in the v5 patch series to
> support the basic communication.
>
> Best regards,
> Parthiban V
> >
> > Thanks,
> > Piergiorgio
> >
> > -----Original Message-----
> > From: Andrew Lunn <andrew@lunn.ch>
> > Sent: 31 May, 2024 14:37
> > To: Parthiban.Veerasooran@microchip.com
> > Cc: Piergiorgio Beruto <Pier.Beruto@onsemi.com>; Selvamani
> Rajagopal <Selvamani.Rajagopal@onsemi.com>;
> 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;
> Nicolas.Ferre@microchip.com;
> benjamin.bigler@bernformulastudent.ch; Viliam Vozar
> <Viliam.Vozar@onsemi.com>; Arndt Schuebel
> <Arndt.Schuebel@onsemi.com>
> > 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.
> >
> >> So I would request all of you to give your comments on the existing
> >> implementation in the patch series to improve better. Once this
> >> version is mainlined we will discuss further to implement further
> >> features supported. I feel the current discussion doesn't have any
> >> impact on the existing implementation which supports basic 10Base-
> T1S
> >> Ethernet communication.
> >
> > Agreed. Lets focus on what we have now.
> >
> >
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%
> 2Furldefense.com%2Fv3%2F__https%3A%2F%2Fpatchwork.kernel.org
> %2Fproject%2Fnetdevbpf%2Fpatch%2F20240418125648.372526-2-
> Parthiban.Veerasooran%40microchip.com%2F__%3B!!KkVubWw!n9Q
> OIA72sKA9z72UFogHeBRnA8Hse9gmIqzNv27f7Tc-
> 4dYH1KA__DfMSmln-uBotO-
> bnw3PC2qXbfRn%24&data=05%7C02%7CSelvamani.Rajagopal%40on
> semi.com%7Cc9784216cb1143af435408dc839a3822%7C04e1674b7
> af54d13a08264fc6e42384c%7C1%7C0%7C638529945611367664%7
> CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2lu
> MzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=eT
> wuSpyx1KIPuOXV3krpjqElcbhU0zBoTmPjj0srUTs%3D&reserved=0
> >
> > Version 4 failed to apply. So we are missing all the CI tests. We need a
> v5 which cleanly applies to net-next in order for those tests to run.
> >
> > I think we should disable vendor interrupts by default, since we
> currently have no way to handle them.
> >
> > I had a quick look at the comments on the patches. I don't think we
> have any other big issues not agreed on. So please post a v5 with them
> all addressed and we will see what the CI says.
> >
> > Piergiorgio, if you have any real problems getting basic support for
> your device working with this framework, now would be a good time to
> raise the problems.
> >
> > Andrew
On Wed, Jun 05, 2024 at 09:40:12PM +0000, Selvamani Rajagopal wrote: > Parthiban/Andrew, > > Couple of requests / suggestions after completing the integration of our drivers to the current framework. Please configure your email client to wrap lines at about 78 characters. > > 1) Can we move memory map selector definitions (OA_TC6_PHY_C45_PCS_MMS2 and other 4 definitions) to the header file > include/linux/oa_tc6.h? > Also, if possible, could we add the MMS0, MMS1?. Our driver is using them. Of course, we could add it when we submit our driver. Interesting. So you have vendor registers outside of MMS 10-15? Or do you need to access standard registers? I would prefer to see your use cases before deciding this. If you want to access standard registers, you are probably doing stuff other vendors also want to do, so we should add a helper in the framework. 2) If it not too late to ask, Is it possible to move interrupt > handler to vendor's code? I would say no, not at the moment. What we can do in the future is allow a driver to register a function to handle the vendor interrupts, leaving the framework to handle the standard interrupts, and chain into the specific driver vendor interrupt handler when a vendor interrupt it indicated. > This way, it will provide vendors' code an ability to deal with some > of the interrupts. For example, our code deals with PHYINT bit. Please explain what you are doing here? What are you doing which the framework does not cover. Andrew
> -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Wednesday, June 5, 2024 4:43 PM > To: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com> > Cc: Parthiban.Veerasooran@microchip.com; Piergiorgio Beruto > <Pier.Beruto@onsemi.com>; 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; > Nicolas.Ferre@microchip.com; > benjamin.bigler@bernformulastudent.ch; Viliam Vozar > <Viliam.Vozar@onsemi.com>; Arndt Schuebel > <Arndt.Schuebel@onsemi.com> > 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. > > > On Wed, Jun 05, 2024 at 09:40:12PM +0000, Selvamani Rajagopal > wrote: > > Parthiban/Andrew, > > > > Couple of requests / suggestions after completing the integration of > our drivers to the current framework. > > Please configure your email client to wrap lines at about 78 > characters. > I believe my client is configured to wrap at 70th characters. Not sure why it is not doing it. > > > > > 1) Can we move memory map selector definitions > (OA_TC6_PHY_C45_PCS_MMS2 and other 4 definitions) to the header > file > > include/linux/oa_tc6.h? > > Also, if possible, could we add the MMS0, MMS1?. Our driver is > using them. Of course, we could add it when we submit our driver. > > Interesting. So you have vendor registers outside of MMS 10-15? This is not about vendor registers. The current oa_tc6 defines MMS selector values for 2, 3, 4, 5, 6. I am asking, if 0, 1 can be added, which are meant for "Standard Control and Status" and MAC respectively, according to MMS assignment table 6 on OA standard. > > Or do you need to access standard registers? I would prefer to see > your use cases before deciding this. If you want to access standard > registers, you are probably doing stuff other vendors also want to do, > so we should add a helper in the framework. > > 2) If it not too late to ask, Is it possible to move interrupt > > handler to vendor's code? > > I would say no, not at the moment. > > What we can do in the future is allow a driver to register a function > to handle the vendor interrupts, leaving the framework to handle the > standard interrupts, and chain into the specific driver vendor > interrupt handler when a vendor interrupt it indicated. > > > This way, it will provide vendors' code an ability to deal with some > > of the interrupts. For example, our code deals with PHYINT bit. > > Please explain what you are doing here? What are you doing which the > framework does not cover. One example I can think of is, to handle PHYINT status bit that may be set in STATUS0 register. Another example could be, to give a vendor flexibility to not to use interrupt mode. FYI: Our driver uses interrupts. So, this is not the main reason. > > Andrew
> I believe my client is configured to wrap at 70th characters. > Not sure why it is not doing it. It could be you also send a MIME obfuscated copy which is not wrapped correctly? > > > 1) Can we move memory map selector definitions > > (OA_TC6_PHY_C45_PCS_MMS2 and other 4 definitions) to the header > > file > > > include/linux/oa_tc6.h? > > > Also, if possible, could we add the MMS0, MMS1?. Our driver is > > using them. Of course, we could add it when we submit our driver. > > > > Interesting. So you have vendor registers outside of MMS 10-15? > > This is not about vendor registers. The current oa_tc6 defines > MMS selector values for 2, 3, 4, 5, 6. I am asking, if 0, 1 can be added, > which are meant for "Standard Control and Status" and MAC respectively, > according to MMS assignment table 6 on OA standard. But why would a MAC driver need access to those? Everything using those registers should be defined in the standard. So the framework should handle them. > One example I can think of is, to handle PHYINT status bit > that may be set in STATUS0 register. Another example could be, > to give a vendor flexibility to not to use interrupt mode. But that is part of the standard. Why would a driver need to do anything, the framework should handle PHYINT, calling phy_mac_interrupt(phydev). I really think you need to post patches. We can then discuss each use case, and i can give you concrete feedback. But in general, if it is part of the standard it should be in the framework. Support for features which are not part of the standard, and workarounds for where a device violates the standard, should be in the MAC driver, or the PHY driver. Andrew
> -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Thursday, June 6, 2024 6:12 AM > To: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com> > Cc: Parthiban.Veerasooran@microchip.com; Piergiorgio Beruto > <Pier.Beruto@onsemi.com>; 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; Nicolas.Ferre@microchip.com; > benjamin.bigler@bernformulastudent.ch; Viliam Vozar > <Viliam.Vozar@onsemi.com>; Arndt Schuebel > <Arndt.Schuebel@onsemi.com> > 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. > > > I believe my client is configured to wrap at 70th characters. > > Not sure why it is not doing it. > > > It could be you also send a MIME obfuscated copy which is not wrapped > correctly? > > > > > 1) Can we move memory map selector definitions > > > (OA_TC6_PHY_C45_PCS_MMS2 and other 4 definitions) to the header > > > file > > > > include/linux/oa_tc6.h? > > > > Also, if possible, could we add the MMS0, MMS1?. Our driver is > > > using them. Of course, we could add it when we submit our driver. > > > > > > Interesting. So you have vendor registers outside of MMS 10-15? > > > > This is not about vendor registers. The current oa_tc6 defines > > MMS selector values for 2, 3, 4, 5, 6. I am asking, if 0, 1 can be added, > > which are meant for "Standard Control and Status" and MAC > respectively, > > according to MMS assignment table 6 on OA standard. > > But why would a MAC driver need access to those? Everything using > those registers should be defined in the standard. So the framework > should handle them. > > > One example I can think of is, to handle PHYINT status bit > > that may be set in STATUS0 register. Another example could be, > > to give a vendor flexibility to not to use interrupt mode. > > But that is part of the standard. Why would a driver need to do > anything, the framework should handle PHYINT, calling > phy_mac_interrupt(phydev). > > I really think you need to post patches. We can then discuss each use > case, and i can give you concrete feedback. True. That would be better. Will work on the patches so that it is clear. > > But in general, if it is part of the standard it should be in the > framework. Support for features which are not part of the standard, > and workarounds for where a device violates the standard, should be in > the MAC driver, or the PHY driver. > > Andrew
This patch series contain the below updates, - Adds support for OPEN Alliance 10BASE-T1x MACPHY Serial Interface in the net/ethernet/oa_tc6.c. Link to the spec: ----------------- https://opensig.org/download/document/OPEN_Alliance_10BASET1x_MAC-PHY_Serial_Interface_V1.1.pdf - Adds driver support for Microchip LAN8650/1 Rev.B1 10BASE-T1S MACPHY Ethernet driver in the net/ethernet/microchip/lan865x/lan865x.c. Link to the product: -------------------- https://www.microchip.com/en-us/product/lan8650 Testing Details: ---------------- The driver performance was tested using iperf3 in the below two setups separately. Setup 1: -------- Node 0 - Raspberry Pi 4 with LAN8650 MAC-PHY Node 1 - Raspberry Pi 4 with EVB-LAN8670-USB USB Stick Setup 2: -------- Node 0 - SAMA7G54-EK with LAN8650 MAC-PHY Node 1 - Raspberry Pi 4 with EVB-LAN8670-USB USB Stick Achieved maximum of 9.4 Mbps. Some systems like Raspberry Pi 4 need performance mode enabled to get the proper clock speed for SPI. Refer below link for more details. https://github.com/raspberrypi/linux/issues/3381#issuecomment-1144723750 Changes: v2: - Removed RFC tag. - OA TC6 framework configured in the Kconfig and Makefile to compile as a module. - Kerneldoc headers added for all the API methods exposed to MAC driver. - Odd parity calculation logic updated from the below link, https://elixir.bootlin.com/linux/latest/source/lib/bch.c#L348 - Control buffer memory allocation moved to the initial function. - struct oa_tc6 implemented as an obaque structure. - Removed kthread for handling mac-phy interrupt instead threaded irq is used. - Removed interrupt implementation for soft reset handling instead of that polling has been implemented. - Registers name in the defines changed according to the specification document. - Registers defines are arranged in the order of offset and followed by register fields. - oa_tc6_write_register() implemented for writing a single register and oa_tc6_write_registers() implemented for writing multiple registers. - oa_tc6_read_register() implemented for reading a single register and oa_tc6_read_registers() implemented for reading multiple registers. - Removed DRV_VERSION macro as git hash provided by ethtool. - Moved MDIO bus registration and PHY initialization to the OA TC6 lib. - Replaced lan865x_set/get_link_ksettings() functions with phy_ethtool_ksettings_set/get() functions. - MAC-PHY's standard capability register values checked against the user configured values. - Removed unnecessary parameters validity check in various places. - Removed MAC address configuration in the lan865x_net_open() function as it is done in the lan865x_probe() function already. - Moved standard registers and proprietary vendor registers to the respective files. - Added proper subject prefixes for the DT bindings. - Moved OA specific properties to a separate DT bindings and corrected the types & mistakes in the DT bindings. - Inherited OA specific DT bindings to the LAN865x specific DT bindings. - Removed sparse warnings in all the places. - Used net_err_ratelimited() for printing the error messages. - oa_tc6_process_rx_chunks() function and the content of oa_tc6_handler() function are split into small functions. - Used proper macros provided by network layer for calculating the MAX_ETH_LEN. - Return value of netif_rx() function handled properly. - Removed unnecessary NULL initialization of skb in the oa_tc6_rx_eth_ready() function removed. - Local variables declaration ordered in reverse xmas tree notation. v3: - Completely redesigned all the patches. - Control and data interface patches are divided into multiple small patches. - Device driver APIs added in the oa-tc6-framework.rst file. - Code readability improved in all the patches. - Defined macros wherever is possible. - Changed RESETC to STATUS0_RESETC for improving the readability. - Removed OA specific DT bindings. - Used default configurations defined in the OA spec. - All variables are named properly as per OA spec for more redability. - Bigger functions are split into multiple smaller functions. - DT binding check is done. - Phy mask is removed in phy scanning. - Used NET_RX_DROP to compare the rx packet submission status. - Indentation in the Kconfig file corrected. - Removed CONFIG_OF and CONFIG_ACPI ifdefs. - Removed MODULE_ALIAS(). v4: - Fixed indentation in oa-tc6-framework.rst file. - Replaced ENODEV error code with EPROTO in the oa_tc6_check_ctrl_write_reply and oa_tc6_check_ctrl_read_reply() functions. - Renamed oa_tc6_read_sw_reset_status() function as oa_tc6_read_status0(). - Changed software reset polling delay as 1ms and polling timeout as 1s. - Implemented clause 45 registers direct access. - Replaced ENODEV error code with ENOMEM in the oa_tc6_mdiobus_register() function. - Changed transmit skbs queue size as 2. - Added skb_linearize() function to convert contiguous packet data. - Checked kthread_should_stop() in the oa_tc6_spi_thread_handler() function before proceeding for the oa_tc6_try_spi_transfer(). - Removed netdev_err() print in the oa_tc6_allocate_rx_skb() function. - Added spi-peripheral-props reference in the dt-bindings. - Changed the fallback order in the dt-bindings. - Replaced netif_start_queue() with netif_wake_queue(). - Empty data transfer performed in the oa_tc6_init() function to clear the reset complete interrupt. - ZARFE bit in the CONFIG0 register is set to 1 to avoid lan865x halt based on the recommendation in the lan865x errata. Parthiban Veerasooran (12): Documentation: networking: add OPEN Alliance 10BASE-T1x MAC-PHY serial interface net: ethernet: oa_tc6: implement register write operation net: ethernet: oa_tc6: implement register read operation net: ethernet: oa_tc6: implement software reset net: ethernet: oa_tc6: implement error interrupts unmasking net: ethernet: oa_tc6: implement internal PHY initialization net: ethernet: oa_tc6: enable open alliance tc6 data communication net: ethernet: oa_tc6: implement transmit path to transfer tx ethernet frames net: ethernet: oa_tc6: implement receive path to receive rx ethernet frames net: ethernet: oa_tc6: implement mac-phy interrupt microchip: lan865x: add driver support for Microchip's LAN865X MAC-PHY dt-bindings: net: add Microchip's LAN865X 10BASE-T1S MACPHY .../bindings/net/microchip,lan865x.yaml | 80 + Documentation/networking/oa-tc6-framework.rst | 491 ++++++ MAINTAINERS | 15 + drivers/net/ethernet/Kconfig | 11 + drivers/net/ethernet/Makefile | 1 + drivers/net/ethernet/microchip/Kconfig | 1 + drivers/net/ethernet/microchip/Makefile | 1 + .../net/ethernet/microchip/lan865x/Kconfig | 19 + .../net/ethernet/microchip/lan865x/Makefile | 6 + .../net/ethernet/microchip/lan865x/lan865x.c | 384 +++++ drivers/net/ethernet/oa_tc6.c | 1321 +++++++++++++++++ drivers/net/phy/microchip_t1s.c | 30 + include/linux/oa_tc6.h | 23 + include/uapi/linux/mdio.h | 1 + 14 files changed, 2384 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/microchip,lan865x.yaml create mode 100644 Documentation/networking/oa-tc6-framework.rst create mode 100644 drivers/net/ethernet/microchip/lan865x/Kconfig create mode 100644 drivers/net/ethernet/microchip/lan865x/Makefile create mode 100644 drivers/net/ethernet/microchip/lan865x/lan865x.c create mode 100644 drivers/net/ethernet/oa_tc6.c create mode 100644 include/linux/oa_tc6.h