[PATCH 1/2] i2c: qcom-geni: Provide an option to select FIFO processing

[PATCH 1/2] i2c: qcom-geni: Provide an option to select FIFO processing

[Lee Jones]

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"))
+		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

[PATCH 2/2] dt-bindings: soc: qcom: Provide option to select FIFO mode

[Lee Jones]

Used when DMA is not available or the best option.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
index dab7ca9f250c..b0e71c07e604 100644
--- a/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
+++ b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
@@ -40,6 +40,7 @@ Required properties:
 Optional property:
 - clock-frequency:	Desired I2C bus clock frequency in Hz.
 			When missing default to 100000Hz.
+- qcom,geni-se-fifo:	Selects FIFO processing - as opposed to DMA.
 
 Child nodes should conform to I2C bus binding as described in i2c.txt.
 
-- 
2.17.1

Re: [PATCH 1/2] i2c: qcom-geni: Provide an option to select FIFO processing

[Vinod Koul]

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

Re: [PATCH 2/2] dt-bindings: soc: qcom: Provide option to select FIFO mode

[Vinod Koul]

On 04-09-19, 12:36, Lee Jones wrote:
> Used when DMA is not available or the best option.

Ah binding is the second patch, I would assume think that this should be
first patch :)

Nevertheless looks good to me.

Reviewed-by: Vinod Koul <vkoul@kernel.org>

> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
> index dab7ca9f250c..b0e71c07e604 100644
> --- a/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
> @@ -40,6 +40,7 @@ Required properties:
>  Optional property:
>  - clock-frequency:	Desired I2C bus clock frequency in Hz.
>  			When missing default to 100000Hz.
> +- qcom,geni-se-fifo:	Selects FIFO processing - as opposed to DMA.
>  
>  Child nodes should conform to I2C bus binding as described in i2c.txt.
>  
> -- 
> 2.17.1

-- 
~Vinod

Re: [PATCH 1/2] i2c: qcom-geni: Provide an option to select FIFO processing

[Vinod Koul]

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

Re: [PATCH 1/2] i2c: qcom-geni: Provide an option to select FIFO processing

[Lee Jones]

On Wed, 04 Sep 2019, Vinod Koul wrote:

> 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

In this set. :)

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/2] i2c: qcom-geni: Provide an option to select FIFO processing

[Bjorn Andersson]

On Wed 04 Sep 04:36 PDT 2019, Lee Jones wrote:

The subject implies that we select FIFO mode instead of DMA, but that's
not really true, because with DMA enabled we still fall back to FIFO for
messages below 32 bytes. 

So what this does it to disable DMA, which neither the subject or the DT
property describes.

Also missing is a description of why this is needed.

Regards,
Bjorn

> 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"))
> +		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
> 

Re: [PATCH 1/2] i2c: qcom-geni: Provide an option to select FIFO processing

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 3020 bytes --]

On Wed, Sep 04, 2019 at 01:35:48PM -0700, Bjorn Andersson wrote:
> On Wed 04 Sep 04:36 PDT 2019, Lee Jones wrote:
> 
> The subject implies that we select FIFO mode instead of DMA, but that's
> not really true, because with DMA enabled we still fall back to FIFO for
> messages below 32 bytes. 
> 
> So what this does it to disable DMA, which neither the subject or the DT
> property describes.
> 
> Also missing is a description of why this is needed.

Yes.

I am willing to help to get this resolved soonish. However, I have
issues with the approach.

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
binding for this workaround seems like a no-go to me. We have to live
with this binding forever. Sidenote: I could think of a generic
'disable-dma' which could be reused everywhere but we probably won't get
that upstream that late in the cycle.

Is there no other way to disable DMA which is local to this driver so we
can easily revert the workaround later?

> 
> Regards,
> Bjorn
> 
> > 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"))
> > +		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
> > 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] i2c: qcom-geni: Provide an option to select FIFO processing

[Lee Jones]

On Wed, 04 Sep 2019, Wolfram Sang wrote:

> On Wed, Sep 04, 2019 at 01:35:48PM -0700, Bjorn Andersson wrote:
> > On Wed 04 Sep 04:36 PDT 2019, Lee Jones wrote:
> > 
> > The subject implies that we select FIFO mode instead of DMA, but that's
> > not really true, because with DMA enabled we still fall back to FIFO for
> > messages below 32 bytes. 

Do you mean, we fall back to DMA?

> > So what this does it to disable DMA, which neither the subject or the DT
> > property describes.
> > 
> > Also missing is a description of why this is needed.
> 
> Yes.
> 
> I am willing to help to get this resolved soonish. However, I have
> issues with the approach.
> 
> 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
> binding for this workaround seems like a no-go to me. We have to live
> with this binding forever. Sidenote: I could think of a generic
> 'disable-dma' which could be reused everywhere but we probably won't get
> that upstream that late in the cycle.
> 
> 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).

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* won't work, but they don't work now
anyway.

NB: QCom have also made it pretty clear that DTBs *must* match their
kernel version.  I know this is controversial amongst DT purists, but
it's still how QCom operate.

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.

With regards to the nomenclature, my original suggestion was
'qcom,geni-se-no-dma'.  Would that better suit your request?

[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/soc/qcom/qcom-geni-se.c#n644

> > > 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"))
> > > +		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



-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/2] i2c: qcom-geni: Provide an option to select FIFO processing

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1963 bytes --]

Hi Lee,

> > 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?

> > 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.

> The beautiful thing about this approach is that, *if* the Geni SE DMA

I'd say 'advantage' instead of 'beautiful' ;)

> 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.

> 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.

> 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?

Kind regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] i2c: qcom-geni: Provide an option to select FIFO processing

[Lee Jones]

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

Re: [PATCH 1/2] i2c: qcom-geni: Provide an option to select FIFO processing

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1279 bytes --]


> > So, are there investigations running why this reboot happens?
> 
> Yes, but they have been running for months, literally.

I see. This is good to know. Just so I also know where we are with this.

> > 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?

Why not? Using of_machine_is_compatible() makes the patch 100% self
contained (no extra binding needed). It will work wherever the machine
description fits.

> > 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 ... ;)

It seems to work mostly, I won't deny that. But we don't know if the
buggy situation can be triggered on these platforms as well by something
else later. We just don't know.

> > 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.

Glad you like it.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]