| Message ID | 1522095312-23249-1-git-send-email-m-karicheri2@ti.com |
|---|---|
| Headers | show |
| Series | Add support for netcp driver on K2G SoC | expand |
On Mon, Mar 26, 2018 at 04:15:10PM -0400, Murali Karicheri wrote: > K2G SoC uses 2u cpsw h/w. It uses RGMII instead of SGMII to interface with > Phy. This patch enhances the driver to check RGMII status instead of SGMII > status for link state determination. Also map all of the vlan priorities > to zero as the packet DMA is enabled to receive only flow id 0 which maps > to priority zero. > > Additionally, When a phy with rgmii interface requires internal delay, the > same is set in the phy driver. To support such phy devices, add a phy-mode > handling code in the driver using of_get_phy_mode() and pass the obtained > phy mode to of_phy_connect() Hi Murali Please break this patch up. One patch should do one thing. That makes it easy to review. There are too many things going on at once here. Andrew -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 26, 2018 at 04:15:09PM -0400, Murali Karicheri wrote: > This patch provide APIs to allow client drivers to support > probe deferral. On K2G SoC, devices can be probed only > after the ti_sci_pm_domains driver is probed and ready. > As drivers may get probed at different order, any driver > that depends on knav dma and qmss drivers, for example > netcp network driver, needs to defer probe until > knav devices are probed and ready to service. To do this, > add an API to query the device ready status from the knav > dma and qmss devices. Hi Murali Shouldn't you really re-write this to be a dma driver? You would then do something like of_dma_request_slave_channel() in the ethernet driver probe function. That probably correctly returns EPROBE_DEFER. Andrew -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/26/2018 04:28 PM, Andrew Lunn wrote: > On Mon, Mar 26, 2018 at 04:15:10PM -0400, Murali Karicheri wrote: >> K2G SoC uses 2u cpsw h/w. It uses RGMII instead of SGMII to interface with >> Phy. This patch enhances the driver to check RGMII status instead of SGMII >> status for link state determination. Also map all of the vlan priorities >> to zero as the packet DMA is enabled to receive only flow id 0 which maps >> to priority zero. >> >> Additionally, When a phy with rgmii interface requires internal delay, the >> same is set in the phy driver. To support such phy devices, add a phy-mode >> handling code in the driver using of_get_phy_mode() and pass the obtained >> phy mode to of_phy_connect() > > Hi Murali > > Please break this patch up. One patch should do one thing. That makes > it easy to review. There are too many things going on at once here. > > Andrew > Hello Andrew, Thanks for the comment. But I am not sure how to break this up as this is an enhancement to the driver to support a newer version of the cspw hardware. Without all these pieces together, the driver can't function. Probably I can make the below break up based on different functions added. Let me know if this looks good to you. Beware that patch #2 and #3 are small patches and majority of code change will be in patch #1 which has to go together. Patch #1. Add support new link interface, RGMII_LINK_MAC_PHY, for K2G - Most of the code is for this Patch #2. Add support for configuring phy_mode - This just add phy_mode handling code Patch #3. map all vlan priorities to flow id 0
Hello Andrew, On 03/26/2018 04:48 PM, Andrew Lunn wrote: > On Mon, Mar 26, 2018 at 04:15:09PM -0400, Murali Karicheri wrote: >> This patch provide APIs to allow client drivers to support >> probe deferral. On K2G SoC, devices can be probed only >> after the ti_sci_pm_domains driver is probed and ready. >> As drivers may get probed at different order, any driver >> that depends on knav dma and qmss drivers, for example >> netcp network driver, needs to defer probe until >> knav devices are probed and ready to service. To do this, >> add an API to query the device ready status from the knav >> dma and qmss devices. > > Hi Murali > > Shouldn't you really re-write this to be a dma driver? You would then > do something like of_dma_request_slave_channel() in the ethernet > driver probe function. That probably correctly returns EPROBE_DEFER. > > Andrew > Could you please elaborate? These knav dma and qmss drivers are introduced to support packet DMA hardware available in Keystone NetCP which couldn't be implemented using the DMA APIs available at the time this driver was introduced. Another reason was that the performance was really bad. We had an internal implementation based on DMA API before which couldn't be upstreamed at that time due to the reason that we were mis-using the API for this driver. So we introduced these knav_dma driver to support NetCP. We don't have any plan to re-write the driver at this time. If your question is about EPROBE_DEFER being returned from an existing knav_dma API and using the return code to achieve probe defer instead of introducing these APIs, I can take a look into that and respond. So please clarify.
> Hello Andrew, > > Thanks for the comment. But I am not sure how to break this up as this is > an enhancement to the driver to support a newer version of the cspw > hardware. Without all these pieces together, the driver can't > function. Hi Murali A few things to consider. 1) You can introduce new features needed by the new hardware one piece at a time. The new hardware will then work when all the pieces have been added. This is fine, the new hardware never worked before, so you are not breaking anything. 2) Your changes could break the old hardware. By having lots of small changes, you can do a git bissect and find which of the small changes broke it. 3) It is much easier to review 10 small obviously correct patches than one huge complex patch, which is not obvious. > Probably I can make the below break up based on different functions > added. Let me know if this looks good to you. Beware that patch #2 and > #3 are small patches and majority of code change will be in patch #1 > which has to go together. > > Patch #1. Add support new link interface, RGMII_LINK_MAC_PHY, for K2G > - Most of the code is for this > Patch #2. Add support for configuring phy_mode > - This just add phy_mode handling code > Patch #3. map all vlan priorities to flow id 0 That sounds better. Thanks Andrew -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Could you please elaborate? These knav dma and qmss drivers are > introduced to support packet DMA hardware available in Keystone > NetCP which couldn't be implemented using the DMA APIs available > at the time this driver was introduced. Another reason was that > the performance was really bad. We had an internal implementation > based on DMA API before which couldn't be upstreamed at that time > due to the reason that we were mis-using the API for this driver. > So we introduced these knav_dma driver to support NetCP. We don't > have any plan to re-write the driver at this time. > > If your question is about EPROBE_DEFER being returned from an > existing knav_dma API and using the return code to achieve probe > defer instead of introducing these APIs, I can take a look into > that and respond. So please clarify. Hi Murali So if i understood you right, at the time these drivers were written, the linux DMA API did not do what you wanted. You could hack something together by using the API wrongly, but that could not be mainlined. So rather than fixing the DMA API to make it work for this hardware, you ignored it, and made up your own API? This API now has its own problems, it does not correctly handle ordering? So you are hacking your own API further. Does the Linux DMA API correctly handle probing order issues? Has the Linux DMA API evolved so that it now does do what is needed by your hardware? If this was an old hardware which is slowly going away, it would not be an issue. But it seems like there are new variants of the hardware being released. So maybe you should go back and re-write the DMA driver, rather than paper over the cracks? Andrew -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Andrew, On 03/27/2018 10:03 AM, Andrew Lunn wrote: >> Could you please elaborate? These knav dma and qmss drivers are >> introduced to support packet DMA hardware available in Keystone >> NetCP which couldn't be implemented using the DMA APIs available >> at the time this driver was introduced. Another reason was that >> the performance was really bad. We had an internal implementation >> based on DMA API before which couldn't be upstreamed at that time >> due to the reason that we were mis-using the API for this driver. >> So we introduced these knav_dma driver to support NetCP. We don't >> have any plan to re-write the driver at this time. >> >> If your question is about EPROBE_DEFER being returned from an >> existing knav_dma API and using the return code to achieve probe >> defer instead of introducing these APIs, I can take a look into >> that and respond. So please clarify. > > Hi Murali > > So if i understood you right, at the time these drivers were written, > the linux DMA API did not do what you wanted. You could hack something > together by using the API wrongly, but that could not be mainlined. So > rather than fixing the DMA API to make it work for this hardware, you > ignored it, and made up your own API? This API now has its own > problems, it does not correctly handle ordering? So you are hacking > your own API further. > > Does the Linux DMA API correctly handle probing order issues? Has the > Linux DMA API evolved so that it now does do what is needed by your > hardware? > Thanks once again for your review and response! I don't think dma API was meant to support hardware like pkt dma and was the reason quoted when this driver was introduced and the same is valid even today. AFAIK, Without hacking the API, we will not be able to support our driver even today. Besides Keystone itself is an old platform that is matured and have been there for long. K2G SoC was also introduced almost 3 years ago and we were late to port it to upstream due to various reasons. We now have the SoC and most of the drivers upstreamed and this is a missing driver to support networking. Given that we don't have any other new devices planned from this SoC familty in the future, and the platform itself is old and matured, I don't think the extra effort needed to explore DMA API usage and re-write the driver is justified. knav dma API is a TI SoC specific driver API and IMO, it should be fine\ to extend it to support probe deferral. I have also looked at the driver and I don't see any other way to handle this since the channels are allocated and used in ndo_open() and EPROBE_DEFER is not useful here. Btw, I will re-write patch 3/5 as you have suggested as there is scope for adding one more patch besides what I have mentioned in my response. I will be sending v2 of the series soon with your comment on 3/5 addressed. Regards, Murali > If this was an old hardware which is slowly going away, it would not > be an issue. But it seems like there are new variants of the hardware > being released. So maybe you should go back and re-write the DMA > driver, rather than paper over the cracks? > > Andrew >