We have a production-level laptop (Lenovo Yoga C630) which is exhibiting
a rather horrific bug. When I2C HID devices are being scanned for at
boot-time the QCom Geni based I2C (Serial Engine) attempts to use DMA.
When it does, the laptop reboots and the user never sees the OS.
The beautiful thing about this approach is that, *if* the Geni SE DMA
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* will not work - but they do not work now anyway.
Fixes: 8bc529b25354 ("soc: qcom: geni: Add support for ACPI")
Signed-off-by: Lee Jones <[email protected]>
Reviewed-by: Vinod Koul <[email protected]>
---
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..8822dea82980 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-no-dma"))
+ 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-no-dma"))
+ 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
On Thu, 05 Sep 2019, Wolfram Sang wrote:
>
> > Fixes: 8bc529b25354 ("soc: qcom: geni: Add support for ACPI")
>
> Are you sure? From visual inspection, I don't see a correlation between
> this commit and the fix here.
This patch should have been part of the commit, or at the very least,
part of the set, alluded to above. Unfortunately, I was carrying
Bjorn's hack which simply returned early from geni_se_rx_dma_prep()
with an error, so it masked the issue.
> > Signed-off-by: Lee Jones <[email protected]>
> > Reviewed-by: Vinod Koul <[email protected]>
> > ---
> > 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..8822dea82980 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-no-dma"))
> > + 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-no-dma"))
> > + dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
> > +
> > if (dma_buf)
> > geni_se_select_mode(se, GENI_SE_DMA);
> > else
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
> Fixes: 8bc529b25354 ("soc: qcom: geni: Add support for ACPI")
Are you sure? From visual inspection, I don't see a correlation between
this commit and the fix here.
> Signed-off-by: Lee Jones <[email protected]>
> Reviewed-by: Vinod Koul <[email protected]>
> ---
> 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..8822dea82980 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-no-dma"))
> + 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-no-dma"))
> + 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
>
On Thu, Sep 05, 2019 at 10:28:16AM +0100, Lee Jones wrote:
> On Thu, 05 Sep 2019, Wolfram Sang wrote:
>
> >
> > > Fixes: 8bc529b25354 ("soc: qcom: geni: Add support for ACPI")
> >
> > Are you sure? From visual inspection, I don't see a correlation between
> > this commit and the fix here.
>
> This patch should have been part of the commit, or at the very least,
> part of the set, alluded to above. Unfortunately, I was carrying
> Bjorn's hack which simply returned early from geni_se_rx_dma_prep()
> with an error, so it masked the issue.
I still don't see why this basic ACPI enabling code (not touching DMA
but only clocks and pinctrl) causes and additional handling for DMA. Am
I overlooking something obvious?
On Thu, 05 Sep 2019, Wolfram Sang wrote:
> On Thu, Sep 05, 2019 at 10:28:16AM +0100, Lee Jones wrote:
> > On Thu, 05 Sep 2019, Wolfram Sang wrote:
> >
> > >
> > > > Fixes: 8bc529b25354 ("soc: qcom: geni: Add support for ACPI")
> > >
> > > Are you sure? From visual inspection, I don't see a correlation between
> > > this commit and the fix here.
> >
> > This patch should have been part of the commit, or at the very least,
> > part of the set, alluded to above. Unfortunately, I was carrying
> > Bjorn's hack which simply returned early from geni_se_rx_dma_prep()
> > with an error, so it masked the issue.
>
> I still don't see why this basic ACPI enabling code (not touching DMA
> but only clocks and pinctrl) causes and additional handling for DMA. Am
> I overlooking something obvious?
Please ignore, I'm discussing with another patch in mind.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog