Re: [net-next PATCH 2/5] soc: ti: K2G: provide APIs to support driver probe deferral
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 > -- Murali Karicheri Linux Kernel, Keystone
Re: [net-next PATCH 2/5] soc: ti: K2G: provide APIs to support driver probe deferral
> 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
Re: [net-next PATCH 2/5] soc: ti: K2G: provide APIs to support driver probe deferral
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. -- Murali Karicheri Linux Kernel, Keystone
Re: [net-next PATCH 2/5] soc: ti: K2G: provide APIs to support driver probe deferral
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
[net-next PATCH 2/5] soc: ti: K2G: provide APIs to support driver probe deferral
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.
Signed-off-by: Murali Karicheri
---
drivers/soc/ti/knav_dma.c| 8
drivers/soc/ti/knav_qmss_queue.c | 8
include/linux/soc/ti/knav_dma.h | 12
include/linux/soc/ti/knav_qmss.h | 1 +
4 files changed, 29 insertions(+)
diff --git a/drivers/soc/ti/knav_dma.c b/drivers/soc/ti/knav_dma.c
index 026182d..224d7dd 100644
--- a/drivers/soc/ti/knav_dma.c
+++ b/drivers/soc/ti/knav_dma.c
@@ -134,6 +134,13 @@ struct knav_dma_chan {
static struct knav_dma_pool_device *kdev;
+static bool device_ready;
+bool knav_dma_device_ready(void)
+{
+ return device_ready;
+}
+EXPORT_SYMBOL_GPL(knav_dma_device_ready);
+
static bool check_config(struct knav_dma_chan *chan, struct knav_dma_cfg *cfg)
{
if (!memcmp(&chan->cfg, cfg, sizeof(*cfg)))
@@ -773,6 +780,7 @@ static int knav_dma_probe(struct platform_device *pdev)
debugfs_create_file("knav_dma", S_IFREG | S_IRUGO, NULL, NULL,
&knav_dma_debug_ops);
+ device_ready = true;
return ret;
}
diff --git a/drivers/soc/ti/knav_qmss_queue.c b/drivers/soc/ti/knav_qmss_queue.c
index 5b2c4e9..f0e67aa 100644
--- a/drivers/soc/ti/knav_qmss_queue.c
+++ b/drivers/soc/ti/knav_qmss_queue.c
@@ -74,6 +74,13 @@
*/
const char *knav_acc_firmwares[] = {"ks2_qmss_pdsp_acc48.bin"};
+static bool device_ready;
+bool knav_qmss_device_ready(void)
+{
+ return device_ready;
+}
+EXPORT_SYMBOL_GPL(knav_qmss_device_ready);
+
/**
* knav_queue_notify: qmss queue notfier call
*
@@ -1849,6 +1856,7 @@ static int knav_queue_probe(struct platform_device *pdev)
debugfs_create_file("qmss", S_IFREG | S_IRUGO, NULL, NULL,
&knav_queue_debug_ops);
+ device_ready = true;
return 0;
err:
diff --git a/include/linux/soc/ti/knav_dma.h b/include/linux/soc/ti/knav_dma.h
index 66693bc..7127ec3 100644
--- a/include/linux/soc/ti/knav_dma.h
+++ b/include/linux/soc/ti/knav_dma.h
@@ -167,6 +167,8 @@ struct knav_dma_desc {
void *knav_dma_open_channel(struct device *dev, const char *name,
struct knav_dma_cfg *config);
void knav_dma_close_channel(void *channel);
+int knav_dma_get_flow(void *channel);
+bool knav_dma_device_ready(void);
#else
static inline void *knav_dma_open_channel(struct device *dev, const char *name,
struct knav_dma_cfg *config)
@@ -176,6 +178,16 @@ static inline void *knav_dma_open_channel(struct device
*dev, const char *name,
static inline void knav_dma_close_channel(void *channel)
{}
+static inline int knav_dma_get_flow(void *channel)
+{
+ return -EINVAL;
+}
+
+static inline bool knav_dma_device_ready(void)
+{
+ return false;
+}
+
#endif
#endif /* __SOC_TI_KEYSTONE_NAVIGATOR_DMA_H__ */
diff --git a/include/linux/soc/ti/knav_qmss.h b/include/linux/soc/ti/knav_qmss.h
index 9f0ebb3b..9745df6 100644
--- a/include/linux/soc/ti/knav_qmss.h
+++ b/include/linux/soc/ti/knav_qmss.h
@@ -86,5 +86,6 @@ int knav_pool_desc_map(void *ph, void *desc, unsigned size,
void *knav_pool_desc_unmap(void *ph, dma_addr_t dma, unsigned dma_sz);
dma_addr_t knav_pool_desc_virt_to_dma(void *ph, void *virt);
void *knav_pool_desc_dma_to_virt(void *ph, dma_addr_t dma);
+bool knav_qmss_device_ready(void);
#endif /* __SOC_TI_KNAV_QMSS_H__ */
--
1.9.1
