| Message ID | 20190904113613.14997-1-lee.jones@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] i2c: qcom-geni: Provide an option to select FIFO processing | expand |
On 04-09-19, 12:36, Lee Jones wrote: > Signed-off-by: Lee Jones <lee.jones@linaro.org> > --- > drivers/i2c/busses/i2c-qcom-geni.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c > index a89bfce5388e..dfdbce067827 100644 > --- a/drivers/i2c/busses/i2c-qcom-geni.c > +++ b/drivers/i2c/busses/i2c-qcom-geni.c > @@ -353,13 +353,16 @@ static void geni_i2c_tx_fsm_rst(struct geni_i2c_dev *gi2c) > static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, > u32 m_param) > { > + struct device_node *np = gi2c->se.dev->of_node; > dma_addr_t rx_dma; > unsigned long time_left; > - void *dma_buf; > + void *dma_buf = NULL; > struct geni_se *se = &gi2c->se; > size_t len = msg->len; > > - dma_buf = i2c_get_dma_safe_msg_buf(msg, 32); > + if (!of_property_read_bool(np, "qcom,geni-se-fifo")) Where is this property documented, I dont see anything in linux-next for today > + dma_buf = i2c_get_dma_safe_msg_buf(msg, 32); > + > if (dma_buf) > geni_se_select_mode(se, GENI_SE_DMA); > else > @@ -392,13 +395,16 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, > static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, > u32 m_param) > { > + struct device_node *np = gi2c->se.dev->of_node; > dma_addr_t tx_dma; > unsigned long time_left; > - void *dma_buf; > + void *dma_buf = NULL; > struct geni_se *se = &gi2c->se; > size_t len = msg->len; > > - dma_buf = i2c_get_dma_safe_msg_buf(msg, 32); > + if (!of_property_read_bool(np, "qcom,geni-se-fifo")) > + dma_buf = i2c_get_dma_safe_msg_buf(msg, 32); > + > if (dma_buf) > geni_se_select_mode(se, GENI_SE_DMA); > else > -- > 2.17.1 -- ~Vinod
On 04-09-19, 12:36, Lee Jones wrote: > Signed-off-by: Lee Jones <lee.jones@linaro.org> Reviewed-by: Vinod Koul <vkoul@kernel.org> > --- > drivers/i2c/busses/i2c-qcom-geni.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c > index a89bfce5388e..dfdbce067827 100644 > --- a/drivers/i2c/busses/i2c-qcom-geni.c > +++ b/drivers/i2c/busses/i2c-qcom-geni.c > @@ -353,13 +353,16 @@ static void geni_i2c_tx_fsm_rst(struct geni_i2c_dev *gi2c) > static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, > u32 m_param) > { > + struct device_node *np = gi2c->se.dev->of_node; > dma_addr_t rx_dma; > unsigned long time_left; > - void *dma_buf; > + void *dma_buf = NULL; > struct geni_se *se = &gi2c->se; > size_t len = msg->len; > > - dma_buf = i2c_get_dma_safe_msg_buf(msg, 32); > + if (!of_property_read_bool(np, "qcom,geni-se-fifo")) > + dma_buf = i2c_get_dma_safe_msg_buf(msg, 32); > + > if (dma_buf) > geni_se_select_mode(se, GENI_SE_DMA); > else > @@ -392,13 +395,16 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, > static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, > u32 m_param) > { > + struct device_node *np = gi2c->se.dev->of_node; > dma_addr_t tx_dma; > unsigned long time_left; > - void *dma_buf; > + void *dma_buf = NULL; > struct geni_se *se = &gi2c->se; > size_t len = msg->len; > > - dma_buf = i2c_get_dma_safe_msg_buf(msg, 32); > + if (!of_property_read_bool(np, "qcom,geni-se-fifo")) > + dma_buf = i2c_get_dma_safe_msg_buf(msg, 32); > + > if (dma_buf) > geni_se_select_mode(se, GENI_SE_DMA); > else > -- > 2.17.1 -- ~Vinod
On Thu, 05 Sep 2019, Wolfram Sang wrote: > > > It looks like a workaround to me. It would be interesting to hear which > > > I2C client breaks with DMA and if it's driver can't be fixed somehow > > > instead. But even if we agree on a workaround short term, adding a > > So, are there investigations running why this reboot happens? Yes, but they have been running for months, literally. Unfortunately, since these are production level platforms, all of the usual low-level debugging avenues (JTAG) have been closed off. Also, only a very small number of people have access to documentation, but even those who are in possession are stumped. Andy Gross did have one idea as to what might be happening, but it turned out to be a red herring. > > > Is there no other way to disable DMA which is local to this driver so we > > > can easily revert the workaround later? > > > > This is the most local low-impact solution (nomenclature aside). > > I disagree. You could use of_machine_is_compatible() and disable DMA for > that machine. Less impact because we save the workaround binding. That could also work. > > The beautiful thing about this approach is that, *if* the Geni SE DMA > > I'd say 'advantage' instead of 'beautiful' ;) Okay, "the advantage thing about ..." ;) > > ever starts working, we can remove the C code and any old properties > > left in older DTs just become NOOP. Older kernels with newer DTs > > (less of a priority) *still* won't work, but they don't work now > > anyway. > > Which is a clear disadvantage of that solution. It won't fix older > kernels. My suggestion above should fix them, too. Not sure how this is possible. Unless you mean LTS? > > The offending line can be found at [0]. There is no obvious bug to > > fix and this code obviously works well on some of the hardware > > platforms using it. But on our platform (Lenovo Yoga C630 - QCom > > SMD850) that final command, which initiates the DMA transaction, ends > > up rebooting the machine. > > Unless we know why the reboot happens on your platform, I'd be careful > with saying "work obviously well" on other platforms. Someone must have tested it? Surely ... ;) > > With regards to the nomenclature, my original suggestion was > > 'qcom,geni-se-no-dma'. Would that better suit your request? > > My suggestion: > > For 5.3, use of_machine_is_compatible() and we backport that. For later, > try to find out the root cause and fix it. If that can't be done, try to > set up a generic "disable-dma" property and use it. > > What do you think about that? Sounds okay to me. Let me code that up. -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c index a89bfce5388e..dfdbce067827 100644 --- a/drivers/i2c/busses/i2c-qcom-geni.c +++ b/drivers/i2c/busses/i2c-qcom-geni.c @@ -353,13 +353,16 @@ static void geni_i2c_tx_fsm_rst(struct geni_i2c_dev *gi2c) static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, u32 m_param) { + struct device_node *np = gi2c->se.dev->of_node; dma_addr_t rx_dma; unsigned long time_left; - void *dma_buf; + void *dma_buf = NULL; struct geni_se *se = &gi2c->se; size_t len = msg->len; - dma_buf = i2c_get_dma_safe_msg_buf(msg, 32); + if (!of_property_read_bool(np, "qcom,geni-se-fifo")) + dma_buf = i2c_get_dma_safe_msg_buf(msg, 32); + if (dma_buf) geni_se_select_mode(se, GENI_SE_DMA); else @@ -392,13 +395,16 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg, u32 m_param) { + struct device_node *np = gi2c->se.dev->of_node; dma_addr_t tx_dma; unsigned long time_left; - void *dma_buf; + void *dma_buf = NULL; struct geni_se *se = &gi2c->se; size_t len = msg->len; - dma_buf = i2c_get_dma_safe_msg_buf(msg, 32); + if (!of_property_read_bool(np, "qcom,geni-se-fifo")) + dma_buf = i2c_get_dma_safe_msg_buf(msg, 32); + if (dma_buf) geni_se_select_mode(se, GENI_SE_DMA); else
Signed-off-by: Lee Jones <lee.jones@linaro.org> --- drivers/i2c/busses/i2c-qcom-geni.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) -- 2.17.1