Re: [PATCH V8 1/1] i2c: i2c-qcom-geni: Add shutdown callback for i2c
Hi Stephen,
On 2021-02-24 12:36, Stephen Boyd wrote:
Quoting ro...@codeaurora.org (2021-02-18 06:15:17)
Hi Stephen,
On 2021-01-13 12:24, Stephen Boyd wrote:
> Quoting Roja Rani Yarubandi (2021-01-08 07:05:45)
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c
>> b/drivers/i2c/busses/i2c-qcom-geni.c
>> index 214b4c913a13..c3f584795911 100644
>> --- a/drivers/i2c/busses/i2c-qcom-geni.c
>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>> @@ -375,6 +375,32 @@ static void geni_i2c_tx_msg_cleanup(struct
>> geni_i2c_dev *gi2c,
>> }
>> }
>>
>> +static void geni_i2c_stop_xfer(struct geni_i2c_dev *gi2c)
>> +{
>> + int ret;
>> + u32 geni_status;
>> + struct i2c_msg *cur;
>> +
>> + /* Resume device, as runtime suspend can happen anytime during
>> transfer */
>> + ret = pm_runtime_get_sync(gi2c->se.dev);
>> + if (ret < 0) {
>> + dev_err(gi2c->se.dev, "Failed to resume device: %d\n",
>> ret);
>> + return;
>> + }
>> +
>> + geni_status = readl_relaxed(gi2c->se.base + SE_GENI_STATUS);
>> + if (geni_status & M_GENI_CMD_ACTIVE) {
>> + cur = gi2c->cur;
>
> Why don't we need to hold the spinlock gi2c::lock here?
>
I am not seeing any race here. May I know which race are you
suspecting
here?
Sorry there are long delays between posting and replies to my review
comments. It takes me some time to remember what we're talking about
because this patch has dragged on for many months.
Sorry for the delayed responses.
So my understanding is that gi2c::lock protects the 'cur' pointer. I
imagine this scenario might go bad
CPU0 CPU1
geni_i2c_stop_xfer()
... geni_i2c_rx_one_msg()
gi2c->cur = cur1;
cur = gi2c->cur;
... geni_i2c_tx_one_msg()
gi2c->cur = cur2;
geni_i2c_abort_xfer()
if (cur->flags & I2C_M_RD)
It's almost like we should combine the geni_i2c_abort_xfer() logic with
the rx/tx message cleanup functions so that it's all done under one
lock. Unfortunately it's complicated by the fact that there are various
completion waiting timeouts involved. Fun!
Thanks for the explanation. Fixed this possible race by protecting
gi2c->cur
and calling geni_i2c_abort_xfer() with adding another parameter to
differentiate
from which sequence is the geni_i2c_abort_xfer() called and handle the
spin_lock/spin_unlock accordingly inside geni_i2c_abort_xfer()
But even after all that, I don't see how the geni_i2c_stop_xfer() puts
a
stop to future calls to geni_i2c_rx_one_msg() or geni_i2c_tx_one_msg().
Now handled this by adding a bool variable "stop_xfer" in geni_i2c_dev
struct,
used to put stop to upcoming geni_i2c_rx_one_msg() and
geni_i2c_tx_one_msg() calls
once we receive the shutdown call.
The hardware isn't disabled from what I can tell. The irq isn't
disabled, the clks aren't turned off, etc. What is to stop an i2c
device
from trying to use the bus after this shutdown function is called? If
anything, this function looks like a "flush", where we flush out any
pending transfer. Where's the "plug" operation that prevents any future
operations from following this call?
We are turning off clocks and disabling irq in
geni_i2c_runtime_suspend().
IIUC about shutdown sequence, during "remove" we will unplug the device
with opposite
calls to "probe's" plug operations example i2c_del_adapter(). For
"shutdown", as system
is going to shutdown, there is no need of unplug operations to be done.
BTW, I see this is merged upstream. That's great, but it seems broken.
Please fix it or revert it out.
>> + geni_i2c_abort_xfer(gi2c);
>> + if (cur->flags & I2C_M_RD)
>> + geni_i2c_rx_msg_cleanup(gi2c, cur);
>> + else
>> + geni_i2c_tx_msg_cleanup(gi2c, cur);
>> + }
>> +
>> + pm_runtime_put_sync_suspend(gi2c->se.dev);
>> +}
>> +
>> static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct
>> i2c_msg *msg,
>> u32 m_param)
>> {
Thanks,
Roja
Re: [PATCH 3/3] i2c: i2c-qcom-geni: Add support for 'assigned-performance-states'
On 2021-02-12 14:51, ro...@codeaurora.org wrote: On 2021-01-20 19:01, Ulf Hansson wrote: On Tue, 19 Jan 2021 at 12:05, Viresh Kumar wrote: On 19-01-21, 12:02, Ulf Hansson wrote: > As a matter of fact this was quite recently discussed [1], which also > pointed out some issues when using the "required-opps" in combination, > but perhaps that got resolved? Viresh? Perhaps we never did anything there .. Okay. Looks like we should pick up that discussion again, to conclude on how to move forward. Soft Reminder! Request Viresh, Uffe to discuss on the way forward. -- viresh Kind regards Uffe - Roja
Re: [PATCH V3 1/2] soc: qcom-geni-se: Cleanup the code to remove proxy votes
On 2021-03-23 15:00, Greg KH wrote:
On Mon, Mar 22, 2021 at 04:34:28PM +0530, Roja Rani Yarubandi wrote:
This reverts commit 048eb908a1f2 ("soc: qcom-geni-se: Add interconnect
support to fix earlycon crash")
ICC core and platforms drivers supports sync_state feature with
commit 7d3b0b0d8184 ("interconnect: qcom: Use icc_sync_state") which
ensures that the default ICC BW votes from the bootloader is not
removed until all it's consumers are probes.
The proxy votes were needed in case other QUP child drivers
I2C, SPI probes before UART, they can turn off the QUP-CORE clock
which is shared resources for all QUP driver, this causes unclocked
access to HW from earlycon.
Given above support from ICC there is no longer need to maintain
proxy votes on QUP-CORE ICC node from QUP wrapper driver for early
console usecase, the default votes won't be removed until real
console is probed.
Signed-off-by: Roja Rani Yarubandi
Signed-off-by: Akash Asthana
Reviewed-by: Matthias Kaehlcke
Should this have a "Fixes:" tag, and also be cc: sta...@vger.kernel.org
so that it will be properly backported?
If so, please add and resend.
Okay, will add and resend.
thanks,
greg k-h
Re: [PATCH V2 1/2] soc: qcom-geni-se: Cleanup the code to remove proxy votes
On 2021-03-18 22:43, Matthias Kaehlcke wrote:
On Thu, Mar 18, 2021 at 04:40:08PM +0530, Roja Rani Yarubandi wrote:
ICC core and platforms drivers supports sync_state feature, which
ensures that the default ICC BW votes from the bootloader is not
removed until all it's consumers are probes.
The proxy votes were needed in case other QUP child drivers
I2C, SPI probes before UART, they can turn off the QUP-CORE clock
which is shared resources for all QUP driver, this causes unclocked
access to HW from earlycon.
Given above support from ICC there is no longer need to maintain
proxy votes on QUP-CORE ICC node from QUP wrapper driver for early
console usecase, the default votes won't be removed until real
console is probed.
Signed-off-by: Roja Rani Yarubandi
Signed-off-by: Akash Asthana
I suggest to mention that this is essentially a revert of commit
048eb908a1f2 ("soc: qcom-geni-se: Add interconnect support to fix
earlycon crash"). This makes the life of reviewers easier and it's
also good to have the reference in the git history.
Ok.
You could also mention commit 7d3b0b0d8184 ("interconnect: qcom:
Use icc_sync_state") in the intro.
Ok.
I tried to test by first reproducing the original issue without
'sync_state' in the ICC, but wasn't successful, probably something
changed in the boot/ICC timing in the meantime ¯\_(ツ)_/¯.
Need to remove runtime auto suspend support from SPI/I2C as well, as it
was
masking the issue by delaying to turn off the resources by 250ms.
Reviewed-by: Matthias Kaehlcke
Re: [PATCH V8 1/1] i2c: i2c-qcom-geni: Add shutdown callback for i2c
Hi Stephen,
On 2021-01-13 12:24, Stephen Boyd wrote:
Quoting Roja Rani Yarubandi (2021-01-08 07:05:45)
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c
b/drivers/i2c/busses/i2c-qcom-geni.c
index 214b4c913a13..c3f584795911 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -375,6 +375,32 @@ static void geni_i2c_tx_msg_cleanup(struct
geni_i2c_dev *gi2c,
}
}
+static void geni_i2c_stop_xfer(struct geni_i2c_dev *gi2c)
+{
+ int ret;
+ u32 geni_status;
+ struct i2c_msg *cur;
+
+ /* Resume device, as runtime suspend can happen anytime during
transfer */
+ ret = pm_runtime_get_sync(gi2c->se.dev);
+ if (ret < 0) {
+ dev_err(gi2c->se.dev, "Failed to resume device: %d\n",
ret);
+ return;
+ }
+
+ geni_status = readl_relaxed(gi2c->se.base + SE_GENI_STATUS);
+ if (geni_status & M_GENI_CMD_ACTIVE) {
+ cur = gi2c->cur;
Why don't we need to hold the spinlock gi2c::lock here?
I am not seeing any race here. May I know which race are you suspecting
here?
+ geni_i2c_abort_xfer(gi2c);
+ if (cur->flags & I2C_M_RD)
+ geni_i2c_rx_msg_cleanup(gi2c, cur);
+ else
+ geni_i2c_tx_msg_cleanup(gi2c, cur);
+ }
+
+ pm_runtime_put_sync_suspend(gi2c->se.dev);
+}
+
static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct
i2c_msg *msg,
u32 m_param)
{
Re: [PATCH 3/3] i2c: i2c-qcom-geni: Add support for 'assigned-performance-states'
On 2021-01-20 19:01, Ulf Hansson wrote: On Tue, 19 Jan 2021 at 12:05, Viresh Kumar wrote: On 19-01-21, 12:02, Ulf Hansson wrote: > As a matter of fact this was quite recently discussed [1], which also > pointed out some issues when using the "required-opps" in combination, > but perhaps that got resolved? Viresh? Perhaps we never did anything there .. Okay. Looks like we should pick up that discussion again, to conclude on how to move forward. Soft Reminder! -- viresh Kind regards Uffe - Roja
Re: [PATCH V7 2/2] i2c: i2c-qcom-geni: Add shutdown callback for i2c
Hi Wolfram,
On 2021-01-05 20:57, Wolfram Sang wrote:
+ geni_status = readl_relaxed(gi2c->se.base + SE_GENI_STATUS);
+ if (!(geni_status & M_GENI_CMD_ACTIVE))
+ goto out;
+
+ cur = gi2c->cur;
+ geni_i2c_abort_xfer(gi2c);
+ if (cur->flags & I2C_M_RD)
+ geni_i2c_rx_msg_cleanup(gi2c, cur);
+ else
+ geni_i2c_tx_msg_cleanup(gi2c, cur);
+out:
+ pm_runtime_put_sync_suspend(gi2c->se.dev);
+}
The use of 'goto' is not needed here IMHO. I think:
if (geni_status & M_GENI_CMD_ACTIVE) {
do_the_stuff
}
pm_runtime_put_sync_suspend(...);
is more readable, in fact.
In context to the previous comment [1], I have implemented this way.
But, yeah anything is fine for me.
Also, I don't think we really need the 'cur'
variable and just use 'gi2c->cur' but that's very minor and you can
keep
it if you like it.
In geni_i2c_abort_xfer() function gi2c->cur will be made NULL, so
copying it before to "cur" is needed here.
Reset looks good!
[1]
https://patchwork.kernel.org/project/linux-arm-msm/patch/20200820103522.26242-3-ro...@codeaurora.org/#23560541
Thanks,
Roja
Re: [RESEND PATCH V6 1/2] i2c: i2c-qcom-geni: Store DMA mapping data in geni_i2c_dev struct
On 2020-12-09 18:29, Akash Asthana wrote:
Hi Roja,
On 12/3/2020 4:01 PM, Roja Rani Yarubandi wrote:
Store DMA mapping data in geni_i2c_dev struct to enhance DMA mapping
data scope. For example during shutdown callback to unmap DMA mapping,
this stored DMA mapping data can be used to call geni_se_tx_dma_unprep
and geni_se_rx_dma_unprep functions.
Add two helper functions geni_i2c_rx_msg_cleanup and
geni_i2c_tx_msg_cleanup to unwrap the things after rx/tx FIFO/DMA
transfers, so that the same can be used in geni_i2c_stop_xfer()
function during shutdown callback.
Signed-off-by: Roja Rani Yarubandi
---
Changes in V5:
- As per Stephen's comments separated this patch from shutdown
callback patch, gi2c->cur = NULL is not removed from
geni_i2c_abort_xfer(), and made a copy of gi2c->cur and passed
to cleanup functions.
Changes in V6:
- Added spin_lock/unlock in geni_i2c_rx_msg_cleanup() and
geni_i2c_tx_msg_cleanup() functions.
drivers/i2c/busses/i2c-qcom-geni.c | 69
+++---
1 file changed, 53 insertions(+), 16 deletions(-)
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c
b/drivers/i2c/busses/i2c-qcom-geni.c
index dce75b85253c..bfbc80f65006 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -86,6 +86,9 @@ struct geni_i2c_dev {
u32 clk_freq_out;
const struct geni_i2c_clk_fld *clk_fld;
int suspended;
+ void *dma_buf;
+ size_t xfer_len;
+ dma_addr_t dma_addr;
};
struct geni_i2c_err_log {
@@ -348,14 +351,49 @@ static void geni_i2c_tx_fsm_rst(struct
geni_i2c_dev *gi2c)
dev_err(gi2c->se.dev, "Timeout resetting TX_FSM\n");
}
+static void geni_i2c_rx_msg_cleanup(struct geni_i2c_dev *gi2c,
+struct i2c_msg *cur)
+{
+ struct geni_se *se = >se;
+ unsigned long flags;
+
+ spin_lock_irqsave(>lock, flags);
+ gi2c->cur_rd = 0;
+ if (gi2c->dma_buf) {
+ if (gi2c->err)
+ geni_i2c_rx_fsm_rst(gi2c);
Which race we are trying to avoid here by holding spinlock?
Thought that race might occur with "cur" here.
We cannot call any sleeping API by holding spinlock,
geni_i2c_rx_fsm_rst calls *wait-for-completion*, which is a sleeping
call.
Fixed this.
+ geni_se_rx_dma_unprep(se, gi2c->dma_addr, gi2c->xfer_len);
+ i2c_put_dma_safe_msg_buf(gi2c->dma_buf, cur, !gi2c->err);
+ }
+ spin_unlock_irqrestore(>lock, flags);
+}
+
+static void geni_i2c_tx_msg_cleanup(struct geni_i2c_dev *gi2c,
+struct i2c_msg *cur)
+{
+ struct geni_se *se = >se;
+ unsigned long flags;
+
+ spin_lock_irqsave(>lock, flags);
+ gi2c->cur_wr = 0;
+ if (gi2c->dma_buf) {
+ if (gi2c->err)
+ geni_i2c_tx_fsm_rst(gi2c);
Same here
Regards,
Akash
+ geni_se_tx_dma_unprep(se, gi2c->dma_addr, gi2c->xfer_len);
+ i2c_put_dma_safe_msg_buf(gi2c->dma_buf, cur, !gi2c->err);
+ }
+ spin_unlock_irqrestore(>lock, flags);
+}
+
static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct
i2c_msg *msg,
u32 m_param)
{
- dma_addr_t rx_dma;
+ dma_addr_t rx_dma = 0;
unsigned long time_left;
void *dma_buf = NULL;
struct geni_se *se = >se;
size_t len = msg->len;
+ struct i2c_msg *cur;
if (!of_machine_is_compatible("lenovo,yoga-c630"))
dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
@@ -372,19 +410,18 @@ static int geni_i2c_rx_one_msg(struct
geni_i2c_dev *gi2c, struct i2c_msg *msg,
geni_se_select_mode(se, GENI_SE_FIFO);
i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
dma_buf = NULL;
+ } else {
+ gi2c->xfer_len = len;
+ gi2c->dma_addr = rx_dma;
+ gi2c->dma_buf = dma_buf;
}
+ cur = gi2c->cur;
time_left = wait_for_completion_timeout(>done, XFER_TIMEOUT);
if (!time_left)
geni_i2c_abort_xfer(gi2c);
- gi2c->cur_rd = 0;
- if (dma_buf) {
- if (gi2c->err)
- geni_i2c_rx_fsm_rst(gi2c);
- geni_se_rx_dma_unprep(se, rx_dma, len);
- i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err);
- }
+ geni_i2c_rx_msg_cleanup(gi2c, cur);
return gi2c->err;
}
@@ -392,11 +429,12 @@ 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)
{
- dma_addr_t tx_dma;
+ dma_addr_t tx_dma = 0;
unsigned long time_left;
void *dma_buf = NULL;
struct geni_se *se = >se;
size_t len = msg->len;
+ struct i2c_msg *cur;
if
Re: [PATCH V5 3/3] i2c: i2c-qcom-geni: Add shutdown callback for i2c
Hi Stephen,
On 2020-10-03 07:09, Stephen Boyd wrote:
Quoting Roja Rani Yarubandi (2020-10-01 01:44:25)
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c
b/drivers/i2c/busses/i2c-qcom-geni.c
index aee2a1dd2c62..56d3fbfe7eb6 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -380,6 +380,36 @@ static void geni_i2c_tx_msg_cleanup(struct
geni_i2c_dev *gi2c,
}
}
+static void geni_i2c_stop_xfer(struct geni_i2c_dev *gi2c)
+{
+ int ret;
+ u32 geni_status;
+ unsigned long flags;
+ struct i2c_msg *cur;
+
+ /* Resume device, runtime suspend can happen anytime during
transfer */
+ ret = pm_runtime_get_sync(gi2c->se.dev);
+ if (ret < 0) {
+ dev_err(gi2c->se.dev, "Failed to resume device: %d\n",
ret);
+ return;
+ }
+
+ spin_lock_irqsave(>lock, flags);
We grab the lock here.
+ geni_status = readl_relaxed(gi2c->se.base + SE_GENI_STATUS);
+ if (!(geni_status & M_GENI_CMD_ACTIVE))
+ goto out;
+
+ cur = gi2c->cur;
+ geni_i2c_abort_xfer(gi2c);
But it looks like this function takes the lock again? Did you test this
with lockdep enabled? It should hang even without lockdep, so it seems
like this path of code has not been tested.
Tested with lockdep enabled, and fixed the unsafe lock order issue here.
And to be more proper moved spin_lock/unlock to cleanup functions.
+ if (cur->flags & I2C_M_RD)
+ geni_i2c_rx_msg_cleanup(gi2c, cur);
+ else
+ geni_i2c_tx_msg_cleanup(gi2c, cur);
+ spin_unlock_irqrestore(>lock, flags);
+out:
+ pm_runtime_put_sync_suspend(gi2c->se.dev);
+}
+
static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct
i2c_msg *msg,
u32 m_param)
{
Re: [PATCH V4] i2c: i2c-qcom-geni: Add shutdown callback for i2c
Hi Stephen,
On 2020-09-18 01:53, Stephen Boyd wrote:
Quoting Roja Rani Yarubandi (2020-09-17 05:25:58)
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c
b/drivers/i2c/busses/i2c-qcom-geni.c
index dead5db3315a..b0d8043c8cb2 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -86,6 +86,10 @@ struct geni_i2c_dev {
u32 clk_freq_out;
const struct geni_i2c_clk_fld *clk_fld;
int suspended;
+ void *dma_buf;
+ size_t xfer_len;
+ dma_addr_t tx_dma;
+ dma_addr_t rx_dma;
Do we need both tx_dma and rx_dma? Seems that we use cur->flags to
figure out if the transfer is tx or rx so we could have juat dma_buf
and
dma_addr here?
Okay.
};
struct geni_i2c_err_log {
@@ -307,7 +311,6 @@ static void geni_i2c_abort_xfer(struct
geni_i2c_dev *gi2c)
spin_lock_irqsave(>lock, flags);
geni_i2c_err(gi2c, GENI_TIMEOUT);
- gi2c->cur = NULL;
This looks concerning. We're moving this out from under the spinlock.
The irq handler in this driver seems to hold the spinlock all the time
while processing and this function grabs it here to keep cur consistent
when aborting the transfer due to a timeout. Otherwise it looks like
the
irqhandler can race with this and try to complete the transfer while
it's being torn down here.
geni_se_abort_m_cmd(>se);
spin_unlock_irqrestore(>lock, flags);
do {
@@ -349,10 +352,62 @@ static void geni_i2c_tx_fsm_rst(struct
geni_i2c_dev *gi2c)
dev_err(gi2c->se.dev, "Timeout resetting TX_FSM\n");
}
+static void geni_i2c_rx_msg_cleanup(struct geni_i2c_dev *gi2c)
So maybe pass cur to this function?
Sorry, i did not understand why to pass cur to this function?
+{
+ struct geni_se *se = >se;
+
+ gi2c->cur_rd = 0;
+ if (gi2c->dma_buf) {
+ if (gi2c->err)
+ geni_i2c_rx_fsm_rst(gi2c);
+ geni_se_rx_dma_unprep(se, gi2c->rx_dma,
gi2c->xfer_len);
+ i2c_put_dma_safe_msg_buf(gi2c->dma_buf, gi2c->cur,
!gi2c->err);
+ }
+}
+
+static void geni_i2c_tx_msg_cleanup(struct geni_i2c_dev *gi2c)
And this one?
+{
+ struct geni_se *se = >se;
+
+ gi2c->cur_wr = 0;
+ if (gi2c->dma_buf) {
+ if (gi2c->err)
+ geni_i2c_tx_fsm_rst(gi2c);
+ geni_se_tx_dma_unprep(se, gi2c->tx_dma,
gi2c->xfer_len);
+ i2c_put_dma_safe_msg_buf(gi2c->dma_buf, gi2c->cur,
!gi2c->err);
+ }
+}
+
+static void geni_i2c_stop_xfer(struct geni_i2c_dev *gi2c)
+{
+ int ret;
+ u32 geni_status;
+
+ /* Resume device, as runtime suspend can happen anytime during
transfer */
+ ret = pm_runtime_get_sync(gi2c->se.dev);
+ if (ret < 0) {
+ dev_err(gi2c->se.dev, "Failed to resume device: %d\n",
ret);
+ return;
+ }
+
+ geni_status = readl_relaxed(gi2c->se.base + SE_GENI_STATUS);
And this probably needs to hold the lock?
+ if (!(geni_status & M_GENI_CMD_ACTIVE))
+ goto out;
+
+ geni_i2c_abort_xfer(gi2c);
+ if (gi2c->cur->flags & I2C_M_RD)
+ geni_i2c_rx_msg_cleanup(gi2c);
+ else
+ geni_i2c_tx_msg_cleanup(gi2c);
+ gi2c->cur = NULL;
until here?
Okay.
+out:
+ pm_runtime_put_sync_suspend(gi2c->se.dev);
+}
+
static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct
i2c_msg *msg,
u32 m_param)
{
- dma_addr_t rx_dma;
+ dma_addr_t rx_dma = 0;
unsigned long time_left;
void *dma_buf = NULL;
struct geni_se *se = >se;
@@ -372,6 +427,10 @@ static int geni_i2c_rx_one_msg(struct
geni_i2c_dev *gi2c, struct i2c_msg *msg,
geni_se_select_mode(se, GENI_SE_FIFO);
i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
dma_buf = NULL;
+ } else {
+ gi2c->xfer_len = len;
+ gi2c->rx_dma = rx_dma;
+ gi2c->dma_buf = dma_buf;
}
geni_se_setup_m_cmd(se, I2C_READ, m_param);
@@ -380,13 +439,7 @@ static int geni_i2c_rx_one_msg(struct
geni_i2c_dev *gi2c, struct i2c_msg *msg,
if (!time_left)
geni_i2c_abort_xfer(gi2c);
- gi2c->cur_rd = 0;
- if (dma_buf) {
- if (gi2c->err)
- geni_i2c_rx_fsm_rst(gi2c);
- geni_se_rx_dma_unprep(se, rx_dma, len);
- i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err);
- }
+ geni_i2c_rx_msg_cleanup(gi2c);
return gi2c->err;
}
It may make sense to extract the cleanup stuff into another patch. Then
have a patch after that which does the shutdown hook. So three patches
total.
Okay, I will make separate patches, one for cleanup and another for
shutdown hook.
diff --git a/drivers/soc/qcom/qcom-geni-se.c
b/drivers/soc/qcom/qcom-geni-se.c
index d0e4f520cff8..0216b38c1e9a 100644
Re: [PATCH V3] i2c: i2c-qcom-geni: Add shutdown callback for i2c
Hi Stephen,
On 2020-09-09 01:00, Stephen Boyd wrote:
Why is dri-devel on here? And linaro-mm-sig?
Ok, I will remove these lists.
Quoting Roja Rani Yarubandi (2020-09-07 06:07:31)
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c
b/drivers/i2c/busses/i2c-qcom-geni.c
index dead5db3315a..b3609760909f 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
struct geni_i2c_err_log {
@@ -384,7 +387,8 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev
*gi2c, struct i2c_msg *msg,
if (dma_buf) {
if (gi2c->err)
geni_i2c_rx_fsm_rst(gi2c);
- geni_se_rx_dma_unprep(se, rx_dma, len);
+ geni_se_rx_dma_unprep(se, gi2c->rx_dma, len);
+ gi2c->rx_dma = (dma_addr_t)NULL;
i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err);
}
@@ -394,12 +398,12 @@ 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)
{
- dma_addr_t tx_dma;
unsigned long time_left;
void *dma_buf = NULL;
struct geni_se *se = >se;
size_t len = msg->len;
+ gi2c->xfer_len = len;
if (!of_machine_is_compatible("lenovo,yoga-c630"))
dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
@@ -410,7 +414,7 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev
*gi2c, struct i2c_msg *msg,
writel_relaxed(len, se->base + SE_I2C_TX_TRANS_LEN);
- if (dma_buf && geni_se_tx_dma_prep(se, dma_buf, len, _dma))
{
+ if (dma_buf && geni_se_tx_dma_prep(se, dma_buf, len,
>tx_dma)) {
geni_se_select_mode(se, GENI_SE_FIFO);
i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
dma_buf = NULL;
@@ -429,7 +433,8 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev
*gi2c, struct i2c_msg *msg,
if (dma_buf) {
if (gi2c->err)
geni_i2c_tx_fsm_rst(gi2c);
- geni_se_tx_dma_unprep(se, tx_dma, len);
+ geni_se_tx_dma_unprep(se, gi2c->tx_dma, len);
+ gi2c->tx_dma = (dma_addr_t)NULL;
i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err);
}
@@ -479,6 +484,51 @@ static int geni_i2c_xfer(struct i2c_adapter
*adap,
return ret;
}
+static void geni_i2c_stop_xfer(struct geni_i2c_dev *gi2c)
+{
+ int ret;
+ u32 dma;
+ u32 val;
+ u32 geni_status;
+ struct geni_se *se = >se;
+
+ ret = pm_runtime_get_sync(gi2c->se.dev);
+ if (ret < 0) {
+ dev_err(gi2c->se.dev, "Failed to resume device: %d\n",
ret);
Is this print really necessary? Doesn't PM core already print this sort
of information?
PM core will not print any such information.
Here we wanted to know our driver's pm runtime resume is successful.
+ return;
+ }
+
+ geni_status = readl_relaxed(gi2c->se.base + SE_GENI_STATUS);
+ if (geni_status & M_GENI_CMD_ACTIVE) {
Please try to de-indent all this.
Okay.
if (!(geni_status & M_GENI_CMD_ACTIVE))
goto out;
+ geni_i2c_abort_xfer(gi2c);
+ dma = readl_relaxed(se->base + SE_GENI_DMA_MODE_EN);
+ if (dma) {
if (!dma)
goto out;
+ val = readl_relaxed(gi2c->se.base +
SE_DMA_DEBUG_REG0);
+ if (val & DMA_TX_ACTIVE) {
+ gi2c->cur_wr = 0;
+ if (gi2c->err)
+ geni_i2c_tx_fsm_rst(gi2c);
+ if (gi2c->tx_dma) {
+ geni_se_tx_dma_unprep(se,
+gi2c->tx_dma,
gi2c->xfer_len);
+ gi2c->tx_dma =
(dma_addr_t)NULL;
Almost nobody does this. In fact, grep shows me one hit in the kernel.
If nobody else is doing it something is probably wrong. When would dma
mode be active and tx_dma not be set to something that should be
stopped? If it really is necessary I suppose we should assign this to
DMA_MAPPING_ERROR instead of casting NULL. Then the check above for
tx_dma being valid can be dropped because geni_se_tx_dma_unprep()
already checks for a valid mapping before doing anything. But really,
we
should probably be tracking the dma buffer mapped to the CPU as well as
the dma address that was used for the mapping. Not storing both is a
problem, see below.
You are correct, setting gi2c->tx_dma to NULL and check for tx_dma is
not required. Will correct this.
+ }
+ } else if (val & DMA_RX_ACTIVE) {
+ gi2c->cur_rd = 0;
+ if (gi2c->err)
+
Re: [PATCH V2 2/2] i2c: i2c-qcom-geni: Add shutdown callback for i2c
On 2020-08-26 17:26, Akash Asthana wrote:
Hi Roja,
On 8/20/2020 4:05 PM, Roja Rani Yarubandi wrote:
If the hardware is still accessing memory after SMMU translation
is disabled (as part of smmu shutdown callback), then the
IOVAs (I/O virtual address) which it was using will go on the bus
as the physical addresses which will result in unknown crashes
like NoC/interconnect errors.
So, implement shutdown callback to i2c driver to unmap DMA mappings
during system "reboot" or "shutdown".
Signed-off-by: Roja Rani Yarubandi
---
Changes in V2:
- As per Stephen's comments added seperate function for stop
transfer,
fixed minor nitpicks.
drivers/i2c/busses/i2c-qcom-geni.c | 43
++
include/linux/qcom-geni-se.h | 5
2 files changed, 48 insertions(+)
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c
b/drivers/i2c/busses/i2c-qcom-geni.c
index 1fda5c7c2cfc..d07f2f33bb75 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -486,6 +486,28 @@ static int geni_i2c_xfer(struct i2c_adapter
*adap,
return ret;
}
+static void geni_i2c_stop_xfer(struct geni_i2c_dev *gi2c)
+{
+ u32 val;
+ struct geni_se *se = >se;
+
+ val = readl_relaxed(gi2c->se.base + SE_DMA_DEBUG_REG0);
+ if (val & DMA_TX_ACTIVE) {
+ geni_i2c_abort_xfer(gi2c);
+ gi2c->cur_wr = 0;
+ if (gi2c->err)
+ geni_i2c_tx_fsm_rst(gi2c);
+ geni_se_tx_dma_unprep(se, gi2c->tx_dma, gi2c->xfer_len);
+ }
should be 'else if', because TX and RX can't happen parallel.
+ if (val & DMA_RX_ACTIVE) {
+ geni_i2c_abort_xfer(gi2c);
+ gi2c->cur_rd = 0;
+ if (gi2c->err)
+ geni_i2c_rx_fsm_rst(gi2c);
+ geni_se_rx_dma_unprep(se, gi2c->rx_dma, gi2c->xfer_len);
+ }
+}
+
static u32 geni_i2c_func(struct i2c_adapter *adap)
{
return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL &
~I2C_FUNC_SMBUS_QUICK);
@@ -617,6 +639,26 @@ static int geni_i2c_remove(struct platform_device
*pdev)
return 0;
}
+static void geni_i2c_shutdown(struct platform_device *pdev)
+{
+ int ret;
+ u32 dma;
+ struct geni_i2c_dev *gi2c = platform_get_drvdata(pdev);
+ struct geni_se *se = >se;
+
+ ret = pm_runtime_get_sync(gi2c->se.dev);
+ if (ret < 0) {
+ dev_err(gi2c->se.dev, "Failed to resume device: %d\n", ret);
+ return;
+ }
+
+ dma = readl_relaxed(se->base + SE_GENI_DMA_MODE_EN);
Wrt to current issue context it may be suffice to stop just DMA mode
transfers but why not stop all mode of active transfer during shutdown
in a generic way.
Regards,
Akash
Okay, I will include FIFO transfer case also in stop_xfer
+ if (dma)
+ geni_i2c_stop_xfer(gi2c);
+
+ pm_runtime_put_sync_suspend(gi2c->se.dev);
+}
+
static int __maybe_unused geni_i2c_runtime_suspend(struct device
*dev)
{
int ret;
@@ -677,6 +719,7 @@ MODULE_DEVICE_TABLE(of, geni_i2c_dt_match);
static struct platform_driver geni_i2c_driver = {
.probe = geni_i2c_probe,
.remove = geni_i2c_remove,
+ .shutdown = geni_i2c_shutdown,
.driver = {
.name = "geni_i2c",
.pm = _i2c_pm_ops,
diff --git a/include/linux/qcom-geni-se.h
b/include/linux/qcom-geni-se.h
index dd464943f717..c3c016496d98 100644
--- a/include/linux/qcom-geni-se.h
+++ b/include/linux/qcom-geni-se.h
@@ -77,6 +77,7 @@ struct geni_se {
#define SE_DMA_RX_FSM_RST 0xd58
#define SE_HW_PARAM_0 0xe24
#define SE_HW_PARAM_1 0xe28
+#define SE_DMA_DEBUG_REG0 0xe40
/* GENI_FORCE_DEFAULT_REG fields */
#define FORCE_DEFAULT BIT(0)
@@ -207,6 +208,10 @@ struct geni_se {
#define RX_GENI_CANCEL_IRQBIT(11)
#define RX_GENI_GP_IRQ_EXTGENMASK(13, 12)
+/* SE_DMA_DEBUG_REG0 Register fields */
+#define DMA_TX_ACTIVE BIT(0)
+#define DMA_RX_ACTIVE BIT(1)
+
/* SE_HW_PARAM_0 fields */
#define TX_FIFO_WIDTH_MSK GENMASK(29, 24)
#define TX_FIFO_WIDTH_SHFT24
Re: [PATCH V2 1/2] i2c: i2c-qcom-geni: Store DMA mapping data in geni_i2c_dev struct
On 2020-08-26 17:25, Akash Asthana wrote:
Hi Roja,
On 8/20/2020 4:05 PM, Roja Rani Yarubandi wrote:
Store DMA mapping data in geni_i2c_dev struct to enhance DMA mapping
data scope. For example during shutdown callback to unmap DMA mapping,
this stored DMA mapping data can be used to call geni_se_tx_dma_unprep
and geni_se_rx_dma_unprep functions.
Signed-off-by: Roja Rani Yarubandi
---
Changes in V2:
- As per Stephen's comments, changed commit text, fixed minor
nitpicks.
drivers/i2c/busses/i2c-qcom-geni.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c
b/drivers/i2c/busses/i2c-qcom-geni.c
index 7f130829bf01..1fda5c7c2cfc 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -86,6 +86,9 @@ struct geni_i2c_dev {
u32 clk_freq_out;
const struct geni_i2c_clk_fld *clk_fld;
int suspended;
+ dma_addr_t tx_dma;
+ dma_addr_t rx_dma;
+ size_t xfer_len;
};
struct geni_i2c_err_log {
@@ -358,6 +361,7 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev
*gi2c, struct i2c_msg *msg,
struct geni_se *se = >se;
size_t len = msg->len;
+ gi2c->xfer_len = msg->len;
nit: gi2c->xfer = len, for tx_one_msg as well.
Regards,
Akash
Okay
if (!of_machine_is_compatible("lenovo,yoga-c630"))
dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
@@ -384,6 +388,7 @@ static int geni_i2c_rx_one_msg(struct
geni_i2c_dev *gi2c, struct i2c_msg *msg,
if (dma_buf) {
if (gi2c->err)
geni_i2c_rx_fsm_rst(gi2c);
+ gi2c->rx_dma = rx_dma;
geni_se_rx_dma_unprep(se, rx_dma, len);
i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err);
}
@@ -400,6 +405,7 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev
*gi2c, struct i2c_msg *msg,
struct geni_se *se = >se;
size_t len = msg->len;
+ gi2c->xfer_len = msg->len;
if (!of_machine_is_compatible("lenovo,yoga-c630"))
dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
@@ -429,6 +435,7 @@ static int geni_i2c_tx_one_msg(struct
geni_i2c_dev *gi2c, struct i2c_msg *msg,
if (dma_buf) {
if (gi2c->err)
geni_i2c_tx_fsm_rst(gi2c);
+ gi2c->tx_dma = tx_dma;
geni_se_tx_dma_unprep(se, tx_dma, len);
i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err);
}
Re: [PATCH V2 2/2] i2c: i2c-qcom-geni: Add shutdown callback for i2c
On 2020-08-21 05:52, Stephen Boyd wrote:
Quoting Roja Rani Yarubandi (2020-08-20 03:35:22)
If the hardware is still accessing memory after SMMU translation
is disabled (as part of smmu shutdown callback), then the
IOVAs (I/O virtual address) which it was using will go on the bus
as the physical addresses which will result in unknown crashes
like NoC/interconnect errors.
So, implement shutdown callback to i2c driver to unmap DMA mappings
during system "reboot" or "shutdown".
Signed-off-by: Roja Rani Yarubandi
---
I'd still put a Fixes tag because it's fixing the driver when someone
runs shutdown.
Okay, will add fixes tag.
Changes in V2:
- As per Stephen's comments added seperate function for stop
transfer,
fixed minor nitpicks.
drivers/i2c/busses/i2c-qcom-geni.c | 43
++
include/linux/qcom-geni-se.h | 5
2 files changed, 48 insertions(+)
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c
b/drivers/i2c/busses/i2c-qcom-geni.c
index 1fda5c7c2cfc..d07f2f33bb75 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -486,6 +486,28 @@ static int geni_i2c_xfer(struct i2c_adapter
*adap,
return ret;
}
+static void geni_i2c_stop_xfer(struct geni_i2c_dev *gi2c)
+{
+ u32 val;
+ struct geni_se *se = >se;
+
+ val = readl_relaxed(gi2c->se.base + SE_DMA_DEBUG_REG0);
+ if (val & DMA_TX_ACTIVE) {
+ geni_i2c_abort_xfer(gi2c);
+ gi2c->cur_wr = 0;
+ if (gi2c->err)
+ geni_i2c_tx_fsm_rst(gi2c);
+ geni_se_tx_dma_unprep(se, gi2c->tx_dma,
gi2c->xfer_len);
+ }
+ if (val & DMA_RX_ACTIVE) {
+ geni_i2c_abort_xfer(gi2c);
+ gi2c->cur_rd = 0;
+ if (gi2c->err)
+ geni_i2c_rx_fsm_rst(gi2c);
+ geni_se_rx_dma_unprep(se, gi2c->rx_dma,
gi2c->xfer_len);
+ }
+}
+
static u32 geni_i2c_func(struct i2c_adapter *adap)
{
return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL &
~I2C_FUNC_SMBUS_QUICK);
@@ -617,6 +639,26 @@ static int geni_i2c_remove(struct platform_device
*pdev)
return 0;
}
+static void geni_i2c_shutdown(struct platform_device *pdev)
+{
+ int ret;
+ u32 dma;
+ struct geni_i2c_dev *gi2c = platform_get_drvdata(pdev);
+ struct geni_se *se = >se;
+
+ ret = pm_runtime_get_sync(gi2c->se.dev);
+ if (ret < 0) {
+ dev_err(gi2c->se.dev, "Failed to resume device: %d\n",
ret);
+ return;
+ }
+
+ dma = readl_relaxed(se->base + SE_GENI_DMA_MODE_EN);
+ if (dma)
+ geni_i2c_stop_xfer(gi2c);
Any reason the if (dma) check isn't inside geni_i2c_stop_xfer()?
Checking for dma and then bailing out early should work and keep the
logic in one function. Then the pm_runtime_sync() call could go in
there
too and if (!dma) goto out. This assumes that we're going to call
geni_i2c_stop_xfer() from somewhere else, like if a transfer times out
or something?
Okay, will do the changes.
+
+ pm_runtime_put_sync_suspend(gi2c->se.dev);
+}
+
static int __maybe_unused geni_i2c_runtime_suspend(struct device
*dev)
{
int ret;
Re: [PATCH V2 1/2] i2c: i2c-qcom-geni: Store DMA mapping data in geni_i2c_dev struct
On 2020-08-21 05:48, Stephen Boyd wrote: Quoting Roja Rani Yarubandi (2020-08-20 03:35:21) Store DMA mapping data in geni_i2c_dev struct to enhance DMA mapping data scope. For example during shutdown callback to unmap DMA mapping, this stored DMA mapping data can be used to call geni_se_tx_dma_unprep and geni_se_rx_dma_unprep functions. Signed-off-by: Roja Rani Yarubandi --- Can this be squashed with the next patch? I don't see how this stands on its own. Okay.
Re: [PATCH 2/2] i2c: i2c-qcom-geni: Add shutdown callback for i2c
On 2020-08-19 09:13, Stephen Boyd wrote:
Quoting Roja Rani Yarubandi (2020-08-14 02:55:40)
If the hardware is still accessing memory after SMMU translation
is disabled(as part of smmu shutdown callback), then the
Put a space before (
Ok.
IOVAs(I/O virtual address) which it was using will go on the bus
Put a space before (
as the physical addresses which will result in unknown crashes
like NoC/interconnect errors.
So, adding shutdown callback to i2c driver to unmap DMA mappings
during system "reboot" or "shutdown".
Deserves a Fixes: tag if it's fixing a problem, which it looks like it
is.
It is not fixing a problem. We are anticipating a problem and
implementing
Shutdown callback.
Signed-off-by: Roja Rani Yarubandi
---
drivers/i2c/busses/i2c-qcom-geni.c | 36
++
include/linux/qcom-geni-se.h | 5 +
I'd prefer this is squashed with the previous patch. The first patch
doesn't really stand on its own anyway.
Sorry, I did not understand.
First patch can be compiled independently.
2 files changed, 41 insertions(+)
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c
b/drivers/i2c/busses/i2c-qcom-geni.c
index 53ca41f76080..749c225f95c4 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -613,6 +613,41 @@ static int geni_i2c_remove(struct platform_device
*pdev)
return 0;
}
+static void geni_i2c_shutdown(struct platform_device *pdev)
+{
+ int ret;
+ struct geni_i2c_dev *gi2c = platform_get_drvdata(pdev);
+ struct geni_se *se = >se;
+ u32 dma;
+ u32 dma_dbg_reg;
Typically this is just called 'val'.
Ok.
+
+ ret = pm_runtime_get_sync(gi2c->se.dev);
+ if (ret < 0) {
+ dev_err(gi2c->se.dev, "Failed to resume device:%d\n",
ret);
Maybe just write: "Failed to resume device\n"? Not sure anyone cares
what the error code is. And if they did, it would be connected to the
colon so it will be hard to read. Add a space after colon if you want
to
keep the return value please.
Ok, will add space after colon.
+ return;
+ }
+
+ dma = readl_relaxed(se->base + SE_GENI_DMA_MODE_EN);
+ if (dma) {
+ dma_dbg_reg = readl_relaxed(gi2c->se.base +
SE_DMA_DEBUG_REG0);
+ if (dma_dbg_reg & DMA_TX_ACTIVE) {
+ geni_i2c_abort_xfer(gi2c);
+ gi2c->cur_wr = 0;
+ if (gi2c->err)
+ geni_i2c_tx_fsm_rst(gi2c);
+ geni_se_tx_dma_unprep(se, gi2c->tx_dma,
gi2c->xfer_len);
+ }
+ if (dma_dbg_reg & DMA_RX_ACTIVE) {
+ geni_i2c_abort_xfer(gi2c);
+ gi2c->cur_rd = 0;
+ if (gi2c->err)
+ geni_i2c_rx_fsm_rst(gi2c);
+ geni_se_rx_dma_unprep(se, gi2c->rx_dma,
gi2c->xfer_len);
+ }
Can this be a function? It sort of seems like we should be doing the
same sort of stuff if we're canceling a DMA transaction in flight.
Ok. Will make the changes.
+ }
+ pm_runtime_put_sync_suspend(gi2c->se.dev);
+}
+
static int __maybe_unused geni_i2c_runtime_suspend(struct device
*dev)
{
int ret;
diff --git a/include/linux/qcom-geni-se.h
b/include/linux/qcom-geni-se.h
index dd464943f717..acad69be747d 100644
--- a/include/linux/qcom-geni-se.h
+++ b/include/linux/qcom-geni-se.h
@@ -77,6 +77,7 @@ struct geni_se {
#define SE_DMA_RX_FSM_RST 0xd58
#define SE_HW_PARAM_0 0xe24
#define SE_HW_PARAM_1 0xe28
+#define SE_DMA_DEBUG_REG0 0xe40
/* GENI_FORCE_DEFAULT_REG fields */
#define FORCE_DEFAULT BIT(0)
@@ -207,6 +208,10 @@ struct geni_se {
#define RX_GENI_CANCEL_IRQ BIT(11)
#define RX_GENI_GP_IRQ_EXT GENMASK(13, 12)
+/* DMA DEBUG Register fields */
Please follow other style, ie. SE_DMA_DEBUG_REG0 fields
Ok.
+#define DMA_TX_ACTIVE BIT(0)
+#define DMA_RX_ACTIVE BIT(1)
+
/* SE_HW_PARAM_0 fields */
#define TX_FIFO_WIDTH_MSK GENMASK(29, 24)
#define TX_FIFO_WIDTH_SHFT 24
Re: [PATCH 1/2] i2c: i2c-qcom-geni: Add tx_dma, rx_dma and xfer_len to geni_i2c_dev struct
Hi Stephen,
Thanks for reviewing the patches.
On 2020-08-19 09:09, Stephen Boyd wrote:
Quoting Roja Rani Yarubandi (2020-08-14 02:55:39)
Adding tx_dma, rx_dma and xfer length in geni_i2c_dev struct to
store DMA mapping data to enhance its scope. For example during
shutdown callback to unmap DMA mapping, these new struct members
can be used as part of geni_se_tx_dma_unprep and
geni_se_rx_dma_unprep calls.
Please read about how to write commit text from kernel docs[1]. Hint,
use imperative mood.
Ok, will update the commit text.
Signed-off-by: Roja Rani Yarubandi
---
drivers/i2c/busses/i2c-qcom-geni.c | 23 +--
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c
b/drivers/i2c/busses/i2c-qcom-geni.c
index 7f130829bf01..53ca41f76080 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -86,6 +86,9 @@ struct geni_i2c_dev {
u32 clk_freq_out;
const struct geni_i2c_clk_fld *clk_fld;
int suspended;
+ dma_addr_t tx_dma;
+ dma_addr_t rx_dma;
+ u32 xfer_len;
Why not size_t?
Will change it to size_t.
};
struct geni_i2c_err_log {
@@ -352,12 +355,11 @@ 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)
{
- dma_addr_t rx_dma;
unsigned long time_left;
void *dma_buf = NULL;
struct geni_se *se = >se;
- size_t len = msg->len;
+ gi2c->xfer_len = msg->len;
I'd prefer to keep the local variable and then have
len = gi2c->xfer_len = msg->len;
Ok, will keep the local variable.
if (!of_machine_is_compatible("lenovo,yoga-c630"))
dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
@@ -366,9 +368,10 @@ static int geni_i2c_rx_one_msg(struct
geni_i2c_dev *gi2c, struct i2c_msg *msg,
else
geni_se_select_mode(se, GENI_SE_FIFO);
- writel_relaxed(len, se->base + SE_I2C_RX_TRANS_LEN);
+ writel_relaxed(gi2c->xfer_len, se->base +
SE_I2C_RX_TRANS_LEN);
So that all this doesn't have to change.
- if (dma_buf && geni_se_rx_dma_prep(se, dma_buf, len, _dma))
{
+ if (dma_buf && geni_se_rx_dma_prep(se, dma_buf,
gi2c->xfer_len,
+ >rx_dma)) {
geni_se_select_mode(se, GENI_SE_FIFO);
i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
dma_buf = NULL;
@@ -384,7 +387,7 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev
*gi2c, struct i2c_msg *msg,
if (dma_buf) {
if (gi2c->err)
geni_i2c_rx_fsm_rst(gi2c);
- geni_se_rx_dma_unprep(se, rx_dma, len);
+ geni_se_rx_dma_unprep(se, gi2c->rx_dma,
gi2c->xfer_len);
i2c_put_dma_safe_msg_buf(dma_buf, msg, !gi2c->err);
}
@@ -394,12 +397,11 @@ 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)
{
- dma_addr_t tx_dma;
unsigned long time_left;
void *dma_buf = NULL;
struct geni_se *se = >se;
- size_t len = msg->len;
+ gi2c->xfer_len = msg->len;
Same comment.
Ok.
if (!of_machine_is_compatible("lenovo,yoga-c630"))
dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
[1]
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes
Thanks,
Roja
