Re: [PATCH] optee: simplify i2c access
On 08/02/21, Jorge Ramirez-Ortiz, Foundries wrote:
> On 08/02/21, Jens Wiklander wrote:
> > Hi Jorge,
> >
> > On Wed, Jan 27, 2021 at 11:41 AM Jens Wiklander
> > wrote:
> > >
> > > Hi Arnd,
> > >
> > > On Mon, Jan 25, 2021 at 12:38 PM Arnd Bergmann wrote:
> > > >
> > > > From: Arnd Bergmann
> > > >
> > > > Storing a bogus i2c_client structure on the stack adds overhead and
> > > > causes a compile-time warning:
> > > >
> > > > drivers/tee/optee/rpc.c:493:6: error: stack frame size of 1056 bytes in
> > > > function 'optee_handle_rpc' [-Werror,-Wframe-larger-than=]
> > > > void optee_handle_rpc(struct tee_context *ctx, struct optee_rpc_param
> > > > *param,
> > > >
> > > > Change the implementation of handle_rpc_func_cmd_i2c_transfer() to
> > > > open-code the i2c_transfer() call, which makes it easier to read
> > > > and avoids the warning.
> > > >
> > > > Fixes: c05210ab9757 ("drivers: optee: allow op-tee to access devices on
> > > > the i2c bus")
> > > > Signed-off-by: Arnd Bergmann
> > > > ---
> > > > drivers/tee/optee/rpc.c | 31 ---
> > > > 1 file changed, 16 insertions(+), 15 deletions(-)
> > >
> > > Looks good to me.
> > > Reviewed-by: Jens Wiklander
> >
> > Would you mind testing this?
>
> sure, doing it this morning.
>
> btw what Arnd has done - removing the unnecessary level of indirection
> - was pretty much my initial though but I thought it was easier to
> read the way I wrote it (I guess I was wrong and I obviously missed
> the stack size increase)
>
> but yes, will test
Tested on imx6ull.
Tested-by: Jorge Ramirez-Ortiz
>
> >
> > Thanks,
> > Jens
Re: [PATCH] optee: simplify i2c access
On 08/02/21, Jens Wiklander wrote:
> Hi Jorge,
>
> On Wed, Jan 27, 2021 at 11:41 AM Jens Wiklander
> wrote:
> >
> > Hi Arnd,
> >
> > On Mon, Jan 25, 2021 at 12:38 PM Arnd Bergmann wrote:
> > >
> > > From: Arnd Bergmann
> > >
> > > Storing a bogus i2c_client structure on the stack adds overhead and
> > > causes a compile-time warning:
> > >
> > > drivers/tee/optee/rpc.c:493:6: error: stack frame size of 1056 bytes in
> > > function 'optee_handle_rpc' [-Werror,-Wframe-larger-than=]
> > > void optee_handle_rpc(struct tee_context *ctx, struct optee_rpc_param
> > > *param,
> > >
> > > Change the implementation of handle_rpc_func_cmd_i2c_transfer() to
> > > open-code the i2c_transfer() call, which makes it easier to read
> > > and avoids the warning.
> > >
> > > Fixes: c05210ab9757 ("drivers: optee: allow op-tee to access devices on
> > > the i2c bus")
> > > Signed-off-by: Arnd Bergmann
> > > ---
> > > drivers/tee/optee/rpc.c | 31 ---
> > > 1 file changed, 16 insertions(+), 15 deletions(-)
> >
> > Looks good to me.
> > Reviewed-by: Jens Wiklander
>
> Would you mind testing this?
sure, doing it this morning.
btw what Arnd has done - removing the unnecessary level of indirection
- was pretty much my initial though but I thought it was easier to
read the way I wrote it (I guess I was wrong and I obviously missed
the stack size increase)
but yes, will test
>
> Thanks,
> Jens
Re: [PATCH] watchdog: qcom: Remove incorrect usage of QCOM_WDT_ENABLE_IRQ
On 26/01/21, Sai Prakash Ranjan wrote:
> As per register documentation, QCOM_WDT_ENABLE_IRQ which is BIT(1)
> of watchdog control register is wakeup interrupt enable bit and
> not related to bark interrupt at all, BIT(0) is used for that.
> So remove incorrect usage of this bit when supporting bark irq for
> pre-timeout notification. Currently with this bit set and bark
> interrupt specified, pre-timeout notification and/or watchdog
> reset/bite does not occur.
>
> Fixes: 36375491a439 ("watchdog: qcom: support pre-timeout when the bark irq
> is available")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Sai Prakash Ranjan
> ---
>
> Reading the conversations from when qcom pre-timeout support was
> added [1], Bjorn already had mentioned it was not right to touch this
> bit, not sure which SoC the pre-timeout was tested on at that time,
> but I have tested this on SDM845, SM8150, SC7180 and watchdog bark
> and bite does not occur with enabling this bit with the bark irq
> specified in DT.
this was tested on QCS404. have you validated there? unfortunately I
no longer have access to that hardware or the documentation
>
> [1] https://lore.kernel.org/linux-watchdog/20190906174009.GC11938@tuxbook-pro/
>
> ---
> drivers/watchdog/qcom-wdt.c | 13 +
> 1 file changed, 1 insertion(+), 12 deletions(-)
>
> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> index 7cf0f2ec649b..e38a87ffe5f5 100644
> --- a/drivers/watchdog/qcom-wdt.c
> +++ b/drivers/watchdog/qcom-wdt.c
> @@ -22,7 +22,6 @@ enum wdt_reg {
> };
>
> #define QCOM_WDT_ENABLE BIT(0)
> -#define QCOM_WDT_ENABLE_IRQ BIT(1)
>
> static const u32 reg_offset_data_apcs_tmr[] = {
> [WDT_RST] = 0x38,
> @@ -63,16 +62,6 @@ struct qcom_wdt *to_qcom_wdt(struct watchdog_device *wdd)
> return container_of(wdd, struct qcom_wdt, wdd);
> }
>
> -static inline int qcom_get_enable(struct watchdog_device *wdd)
> -{
> - int enable = QCOM_WDT_ENABLE;
> -
> - if (wdd->pretimeout)
> - enable |= QCOM_WDT_ENABLE_IRQ;
> -
> - return enable;
> -}
> -
> static irqreturn_t qcom_wdt_isr(int irq, void *arg)
> {
> struct watchdog_device *wdd = arg;
> @@ -91,7 +80,7 @@ static int qcom_wdt_start(struct watchdog_device *wdd)
> writel(1, wdt_addr(wdt, WDT_RST));
> writel(bark * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
> writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME));
> - writel(qcom_get_enable(wdd), wdt_addr(wdt, WDT_EN));
> + writel(QCOM_WDT_ENABLE, wdt_addr(wdt, WDT_EN));
> return 0;
> }
>
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
>
Re: [PATCH] optee: simplify i2c access
On 25/01/21, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> Storing a bogus i2c_client structure on the stack adds overhead and
> causes a compile-time warning:
>
> drivers/tee/optee/rpc.c:493:6: error: stack frame size of 1056 bytes in
> function 'optee_handle_rpc' [-Werror,-Wframe-larger-than=]
> void optee_handle_rpc(struct tee_context *ctx, struct optee_rpc_param *param,
>
> Change the implementation of handle_rpc_func_cmd_i2c_transfer() to
> open-code the i2c_transfer() call, which makes it easier to read
> and avoids the warning.
>
> Fixes: c05210ab9757 ("drivers: optee: allow op-tee to access devices on the
> i2c bus")
does fixing stack-frame compile warnings need a 'fixes' tag?
> Signed-off-by: Arnd Bergmann
> ---
> drivers/tee/optee/rpc.c | 31 ---
> 1 file changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
> index 1e3614e4798f..6cbb3643c6c4 100644
> --- a/drivers/tee/optee/rpc.c
> +++ b/drivers/tee/optee/rpc.c
> @@ -54,8 +54,9 @@ static void handle_rpc_func_cmd_get_time(struct
> optee_msg_arg *arg)
> static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
>struct optee_msg_arg *arg)
> {
> - struct i2c_client client = { 0 };
> struct tee_param *params;
> + struct i2c_adapter *adapter;
> + struct i2c_msg msg = { };
> size_t i;
> int ret = -EOPNOTSUPP;
> u8 attr[] = {
> @@ -85,48 +86,48 @@ static void handle_rpc_func_cmd_i2c_transfer(struct
> tee_context *ctx,
> goto bad;
> }
>
> - client.adapter = i2c_get_adapter(params[0].u.value.b);
> - if (!client.adapter)
> + adapter = i2c_get_adapter(params[0].u.value.b);
> + if (!adapter)
> goto bad;
>
> if (params[1].u.value.a & OPTEE_MSG_RPC_CMD_I2C_FLAGS_TEN_BIT) {
> - if (!i2c_check_functionality(client.adapter,
> + if (!i2c_check_functionality(adapter,
>I2C_FUNC_10BIT_ADDR)) {
> - i2c_put_adapter(client.adapter);
> + i2c_put_adapter(adapter);
> goto bad;
> }
>
> - client.flags = I2C_CLIENT_TEN;
> + msg.flags = I2C_M_TEN;
> }
>
> - client.addr = params[0].u.value.c;
> - snprintf(client.name, I2C_NAME_SIZE, "i2c%d", client.adapter->nr);
> + msg.addr = params[0].u.value.c;
> + msg.buf = params[2].u.memref.shm->kaddr;
> + msg.len = params[2].u.memref.size;
>
> switch (params[0].u.value.a) {
> case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
> - ret = i2c_master_recv(&client, params[2].u.memref.shm->kaddr,
> - params[2].u.memref.size);
> + msg.flags |= I2C_M_RD;
> break;
> case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR:
> - ret = i2c_master_send(&client, params[2].u.memref.shm->kaddr,
> - params[2].u.memref.size);
> break;
> default:
> - i2c_put_adapter(client.adapter);
> + i2c_put_adapter(adapter);
> goto bad;
> }
>
> + ret = i2c_transfer(adapter, &msg, 1);
> +
> if (ret < 0) {
> arg->ret = TEEC_ERROR_COMMUNICATION;
> } else {
> - params[3].u.value.a = ret;
> + params[3].u.value.a = msg.len;
> if (optee_to_msg_param(arg->params, arg->num_params, params))
> arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> else
> arg->ret = TEEC_SUCCESS;
> }
>
> - i2c_put_adapter(client.adapter);
> + i2c_put_adapter(adapter);
> kfree(params);
> return;
> bad:
> --
> 2.29.2
>
Re: [PATCH] optee: simplify i2c access
On 26/01/21, Arnd Bergmann wrote:
> On Tue, Jan 26, 2021 at 9:08 AM Jorge Ramirez-Ortiz, Foundries
> wrote:
> >
> > On 25/01/21, Arnd Bergmann wrote:
> > > From: Arnd Bergmann
> > >
> > > Storing a bogus i2c_client structure on the stack adds overhead and
> > > causes a compile-time warning:
> > >
> > > drivers/tee/optee/rpc.c:493:6: error: stack frame size of 1056 bytes in
> > > function 'optee_handle_rpc' [-Werror,-Wframe-larger-than=]
> > > void optee_handle_rpc(struct tee_context *ctx, struct optee_rpc_param
> > > *param,
> > >
> > > Change the implementation of handle_rpc_func_cmd_i2c_transfer() to
> > > open-code the i2c_transfer() call, which makes it easier to read
> > > and avoids the warning.
> > >
> > > Fixes: c05210ab9757 ("drivers: optee: allow op-tee to access devices on
> > > the i2c bus")
> >
> > does fixing stack-frame compile warnings need a 'fixes' tag?
>
> The fixes tag only describes which commit introduced the bug, it is irrelevant
> what type of bug this is.
>
> Arnd
thanks Arnd.
what compiler warnings are defined as kernel bugs? is there a list
that you use when tracking these?
Re: [PATCH] drivers: optee: i2c: add bus retry configuration
On 23/09/20, Jens Wiklander wrote:
> On Wed, Sep 23, 2020 at 01:26:31PM +0200, Jorge Ramirez-Ortiz, Foundries
> wrote:
> > On 23/09/20, Jorge Ramirez-Ortiz, Foundries wrote:
> > > On 22/09/20, Jens Wiklander wrote:
> > > > On Wed, Sep 16, 2020 at 05:27:32PM +0200, Jorge Ramirez-Ortiz wrote:
> > > > > Allow OP-TEE to specify the number of retries in the adaptor.
> > > > >
> > > > > Signed-off-by: Jorge Ramirez-Ortiz
> > > > > ---
> > > > > drivers/tee/optee/rpc.c | 7 +++
> > > > > 1 file changed, 7 insertions(+)
> > > > >
> > > > > diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
> > > > > index 1e3614e4798f..2d46a9ecb1de 100644
> > > > > --- a/drivers/tee/optee/rpc.c
> > > > > +++ b/drivers/tee/optee/rpc.c
> > > > > @@ -58,6 +58,7 @@ static void handle_rpc_func_cmd_i2c_transfer(struct
> > > > > tee_context *ctx,
> > > > > struct tee_param *params;
> > > > > size_t i;
> > > > > int ret = -EOPNOTSUPP;
> > > > > + int retries = 0;
> > > > > u8 attr[] = {
> > > > > TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> > > > > TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> > > > > @@ -102,12 +103,17 @@ static void
> > > > > handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
> > > > > client.addr = params[0].u.value.c;
> > > > > snprintf(client.name, I2C_NAME_SIZE, "i2c%d",
> > > > > client.adapter->nr);
> > > > >
> > > > > + /* cache the current value */
> > > > > + retries = client.adapter->retries;
> > > > > +
> > > > > switch (params[0].u.value.a) {
> > > > > case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
> > > > > + client.adapter->retries = params[1].u.value.b;
> > > > Do we need to take any locks befor this?
> > >
> > > no I dont think so: there is no need for bus locks when requesting a
> > > transfer via i2c_master_recv/send; the lock for the bus segment gets
> > > taken later on, when the actual transfer hppens ( __i2c_transfer())
> > >
> > > the functionality implemented in this function pretty much mimicks
> > > what is done in the normal world via /dev/i2c-X
> > > (drivers/i2c/i2c_dev.c)
> > >
> >
> > correction (of course)
> > - i2cdev_read --> i2c_master_recv
> > - i2cdev->write -->i2c_master_send
> > >
> > > and now the retry count setup on the adaptor with this commit.
> > >
> > > - i2cdev_ioctl I2C_RETRIES
>
> I don't understand. Do you mean that client.adapter->retries doesn't
> need to be protected from concurrent updates? Or is it already protected
> by some other mechanism?
yeah I probably misunderstood your comment. my bad.
um I thought that upon getting the adaptor there would be some
protection mechanism in place until it is put back; but that is not
the case. looking a bit into it I see no simple way of protecting
changes to the adaptor (at any given time any thread could get a
pointer to it) so it seems that setting the retry field is not a
guarantee that it will be applied.
>
> Cheers,
> Jens
Re: [PATCH] drivers: optee: i2c: add bus retry configuration
On 23/09/20, Jorge Ramirez-Ortiz, Foundries wrote:
> On 22/09/20, Jens Wiklander wrote:
> > On Wed, Sep 16, 2020 at 05:27:32PM +0200, Jorge Ramirez-Ortiz wrote:
> > > Allow OP-TEE to specify the number of retries in the adaptor.
> > >
> > > Signed-off-by: Jorge Ramirez-Ortiz
> > > ---
> > > drivers/tee/optee/rpc.c | 7 +++
> > > 1 file changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
> > > index 1e3614e4798f..2d46a9ecb1de 100644
> > > --- a/drivers/tee/optee/rpc.c
> > > +++ b/drivers/tee/optee/rpc.c
> > > @@ -58,6 +58,7 @@ static void handle_rpc_func_cmd_i2c_transfer(struct
> > > tee_context *ctx,
> > > struct tee_param *params;
> > > size_t i;
> > > int ret = -EOPNOTSUPP;
> > > + int retries = 0;
> > > u8 attr[] = {
> > > TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> > > TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> > > @@ -102,12 +103,17 @@ static void handle_rpc_func_cmd_i2c_transfer(struct
> > > tee_context *ctx,
> > > client.addr = params[0].u.value.c;
> > > snprintf(client.name, I2C_NAME_SIZE, "i2c%d", client.adapter->nr);
> > >
> > > + /* cache the current value */
> > > + retries = client.adapter->retries;
> > > +
> > > switch (params[0].u.value.a) {
> > > case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
> > > + client.adapter->retries = params[1].u.value.b;
> > Do we need to take any locks befor this?
>
> no I dont think so: there is no need for bus locks when requesting a
> transfer via i2c_master_recv/send; the lock for the bus segment gets
> taken later on, when the actual transfer hppens ( __i2c_transfer())
>
> the functionality implemented in this function pretty much mimicks
> what is done in the normal world via /dev/i2c-X
> (drivers/i2c/i2c_dev.c)
>
correction (of course)
- i2cdev_read --> i2c_master_recv
- i2cdev->write -->i2c_master_send
>
> and now the retry count setup on the adaptor with this commit.
>
> - i2cdev_ioctl I2C_RETRIES
>
> >
> > Cheers,
> > Jens
> >
> > > ret = i2c_master_recv(&client, params[2].u.memref.shm->kaddr,
> > > params[2].u.memref.size);
> > > break;
> > > case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR:
> > > + client.adapter->retries = params[1].u.value.b;
> > > ret = i2c_master_send(&client, params[2].u.memref.shm->kaddr,
> > > params[2].u.memref.size);
> > > break;
> > > @@ -126,6 +132,7 @@ static void handle_rpc_func_cmd_i2c_transfer(struct
> > > tee_context *ctx,
> > > arg->ret = TEEC_SUCCESS;
> > > }
> > >
> > > + client.adapter->retries = retries;
> > > i2c_put_adapter(client.adapter);
> > > kfree(params);
> > > return;
> > > --
> > > 2.17.1
> > >
Re: [PATCH] drivers: optee: i2c: add bus retry configuration
On 22/09/20, Jens Wiklander wrote:
> On Wed, Sep 16, 2020 at 05:27:32PM +0200, Jorge Ramirez-Ortiz wrote:
> > Allow OP-TEE to specify the number of retries in the adaptor.
> >
> > Signed-off-by: Jorge Ramirez-Ortiz
> > ---
> > drivers/tee/optee/rpc.c | 7 +++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
> > index 1e3614e4798f..2d46a9ecb1de 100644
> > --- a/drivers/tee/optee/rpc.c
> > +++ b/drivers/tee/optee/rpc.c
> > @@ -58,6 +58,7 @@ static void handle_rpc_func_cmd_i2c_transfer(struct
> > tee_context *ctx,
> > struct tee_param *params;
> > size_t i;
> > int ret = -EOPNOTSUPP;
> > + int retries = 0;
> > u8 attr[] = {
> > TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> > TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> > @@ -102,12 +103,17 @@ static void handle_rpc_func_cmd_i2c_transfer(struct
> > tee_context *ctx,
> > client.addr = params[0].u.value.c;
> > snprintf(client.name, I2C_NAME_SIZE, "i2c%d", client.adapter->nr);
> >
> > + /* cache the current value */
> > + retries = client.adapter->retries;
> > +
> > switch (params[0].u.value.a) {
> > case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
> > + client.adapter->retries = params[1].u.value.b;
> Do we need to take any locks befor this?
no I dont think so: there is no need for bus locks when requesting a
transfer via i2c_master_recv/send; the lock for the bus segment gets
taken later on, when the actual transfer hppens ( __i2c_transfer())
the functionality implemented in this function pretty much mimicks
what is done in the normal world via /dev/i2c-X
(drivers/i2c/i2c_dev.c)
- i2cdev_read --> i2c_master_send
- i2cdev->write -->i2c_master_recv
and now the retry count setup on the adaptor with this commit.
- i2cdev_ioctl I2C_RETRIES
>
> Cheers,
> Jens
>
> > ret = i2c_master_recv(&client, params[2].u.memref.shm->kaddr,
> > params[2].u.memref.size);
> > break;
> > case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR:
> > + client.adapter->retries = params[1].u.value.b;
> > ret = i2c_master_send(&client, params[2].u.memref.shm->kaddr,
> > params[2].u.memref.size);
> > break;
> > @@ -126,6 +132,7 @@ static void handle_rpc_func_cmd_i2c_transfer(struct
> > tee_context *ctx,
> > arg->ret = TEEC_SUCCESS;
> > }
> >
> > + client.adapter->retries = retries;
> > i2c_put_adapter(client.adapter);
> > kfree(params);
> > return;
> > --
> > 2.17.1
> >
[PATCH] drivers: optee: i2c: add bus retry configuration
Allow OP-TEE to specify the number of retries in the adaptor.
Signed-off-by: Jorge Ramirez-Ortiz
---
drivers/tee/optee/rpc.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
index 1e3614e4798f..2d46a9ecb1de 100644
--- a/drivers/tee/optee/rpc.c
+++ b/drivers/tee/optee/rpc.c
@@ -58,6 +58,7 @@ static void handle_rpc_func_cmd_i2c_transfer(struct
tee_context *ctx,
struct tee_param *params;
size_t i;
int ret = -EOPNOTSUPP;
+ int retries = 0;
u8 attr[] = {
TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
@@ -102,12 +103,17 @@ static void handle_rpc_func_cmd_i2c_transfer(struct
tee_context *ctx,
client.addr = params[0].u.value.c;
snprintf(client.name, I2C_NAME_SIZE, "i2c%d", client.adapter->nr);
+ /* cache the current value */
+ retries = client.adapter->retries;
+
switch (params[0].u.value.a) {
case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
+ client.adapter->retries = params[1].u.value.b;
ret = i2c_master_recv(&client, params[2].u.memref.shm->kaddr,
params[2].u.memref.size);
break;
case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR:
+ client.adapter->retries = params[1].u.value.b;
ret = i2c_master_send(&client, params[2].u.memref.shm->kaddr,
params[2].u.memref.size);
break;
@@ -126,6 +132,7 @@ static void handle_rpc_func_cmd_i2c_transfer(struct
tee_context *ctx,
arg->ret = TEEC_SUCCESS;
}
+ client.adapter->retries = retries;
i2c_put_adapter(client.adapter);
kfree(params);
return;
--
2.17.1
[PATCHv2] drivers: optee: fix i2c build issue
When the optee driver is compiled into the kernel while the i2c core
is configured as a module, the i2c symbols are not available.
This commit addresses the situation by disabling the i2c support for
this use case while allowing it in all other scenarios:
i2c=y, optee=y
i2c=m, optee=m
i2c=y, optee=m
i2c=m, optee=y (not supported)
Reported-by: kernel test robot
Signed-off-by: Jorge Ramirez-Ortiz
---
v2: uses IS_REACHABLE instead of macro combination
This patch applies on top of
https://git.linaro.org/people/jens.wiklander/linux-tee.git/tag/?h=optee-i2c-for-v5.10
drivers/tee/optee/rpc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
index 64a206c56264..1e3614e4798f 100644
--- a/drivers/tee/optee/rpc.c
+++ b/drivers/tee/optee/rpc.c
@@ -50,7 +50,7 @@ static void handle_rpc_func_cmd_get_time(struct optee_msg_arg
*arg)
arg->ret = TEEC_ERROR_BAD_PARAMETERS;
}
-#if IS_ENABLED(CONFIG_I2C)
+#if IS_REACHABLE(CONFIG_I2C)
static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
struct optee_msg_arg *arg)
{
--
2.17.1
Re: [PATCH] drivers: optee: fix i2c build issue
On 31/08/20, Randy Dunlap wrote:
> On 8/31/20 8:23 AM, Jorge Ramirez-Ortiz wrote:
> > When the optee driver is compiled into the kernel while the i2c core
> > is configured as a module, the i2c symbols are not available.
> >
> > This commit addresses the situation by disabling the i2c support for
> > this use case while allowing it in all other scenarios:
> >
> > i2c=y, optee=y
> > i2c=m, optee=m
> > i2c=y, optee=m
> > i2c=m, optee=y (not supported)
> >
> > Reported-by: kernel test robot
> > Signed-off-by: Jorge Ramirez-Ortiz
> > ---
> >
> > This patch applies on top of
> >
> > https://git.linaro.org/people/jens.wiklander/linux-tee.git/tag/?h=optee-i2c-for-v5.10
> >
> > drivers/tee/optee/rpc.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
> > index 64a206c56264..96e91d5f0a86 100644
> > --- a/drivers/tee/optee/rpc.c
> > +++ b/drivers/tee/optee/rpc.c
> > @@ -51,6 +51,7 @@ static void handle_rpc_func_cmd_get_time(struct
> > optee_msg_arg *arg)
> > }
> >
> > #if IS_ENABLED(CONFIG_I2C)
> > +#if !defined(CONFIG_I2C_MODULE) || defined(CONFIG_OPTEE_MODULE)
>
> Looks like
> #if IS_REACHABLE(CONFIG_I2C)
>
> should work here.
yep, much nicer. will do that instead.
>
> > static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
> > struct optee_msg_arg *arg)
> > {
> > @@ -140,6 +141,7 @@ static void handle_rpc_func_cmd_i2c_transfer(struct
> > tee_context *ctx,
> > arg->ret = TEEC_ERROR_NOT_SUPPORTED;
> > }
> > #endif
> > +#endif
> >
> > static struct wq_entry *wq_entry_get(struct optee_wait_queue *wq, u32 key)
> > {
> >
>
>
> --
> ~Randy
>
[PATCH] drivers: optee: fix i2c build issue
When the optee driver is compiled into the kernel while the i2c core
is configured as a module, the i2c symbols are not available.
This commit addresses the situation by disabling the i2c support for
this use case while allowing it in all other scenarios:
i2c=y, optee=y
i2c=m, optee=m
i2c=y, optee=m
i2c=m, optee=y (not supported)
Reported-by: kernel test robot
Signed-off-by: Jorge Ramirez-Ortiz
---
This patch applies on top of
https://git.linaro.org/people/jens.wiklander/linux-tee.git/tag/?h=optee-i2c-for-v5.10
drivers/tee/optee/rpc.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
index 64a206c56264..96e91d5f0a86 100644
--- a/drivers/tee/optee/rpc.c
+++ b/drivers/tee/optee/rpc.c
@@ -51,6 +51,7 @@ static void handle_rpc_func_cmd_get_time(struct optee_msg_arg
*arg)
}
#if IS_ENABLED(CONFIG_I2C)
+#if !defined(CONFIG_I2C_MODULE) || defined(CONFIG_OPTEE_MODULE)
static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
struct optee_msg_arg *arg)
{
@@ -140,6 +141,7 @@ static void handle_rpc_func_cmd_i2c_transfer(struct
tee_context *ctx,
arg->ret = TEEC_ERROR_NOT_SUPPORTED;
}
#endif
+#endif
static struct wq_entry *wq_entry_get(struct optee_wait_queue *wq, u32 key)
{
--
2.17.1
Re: [PATCHv9] drivers: optee: allow op-tee to access devices on the i2c bus
On 21/08/20, Jens Wiklander wrote: > On Fri, Aug 14, 2020 at 01:12:21PM +0200, Jorge Ramirez-Ortiz wrote: > > Some secure elements like NXP's SE050 sit on I2C buses. For OP-TEE to > > control this type of cryptographic devices it needs coordinated access > > to the bus, so collisions and RUNTIME_PM dont get in the way. > > > > This trampoline driver allow OP-TEE to access them. > > > > Signed-off-by: Jorge Ramirez-Ortiz > > --- > > v9: params return value must be written before optee_msg_to_param is called > > v8: review fixes: > > fix types and add TEEC_ERROR_NOT_SUPPORTED to GP errors > > v7: add support for ten bit i2c slave addressing > > v6: compile out if CONFIG_I2C not enabled > > v5: alphabetic order of includes > > v4: remove unnecessary extra line in optee_msg.h > > v3: use from/to msg param to support all types of memory > > modify OPTEE_MSG_RPC_CMD_I2C_TRANSFER message id > > > > drivers/tee/optee/optee_msg.h | 21 +++ > > drivers/tee/optee/optee_private.h | 1 + > > drivers/tee/optee/rpc.c | 95 +++ > > 3 files changed, 117 insertions(+) > > Looks good. Did you test this with the recently merged > https://github.com/OP-TEE/optee_os/pull/4033 ? > yes, I did retest and it is good. thanks! > Cheers, > Jens
[PATCHv9] drivers: optee: allow op-tee to access devices on the i2c bus
Some secure elements like NXP's SE050 sit on I2C buses. For OP-TEE to
control this type of cryptographic devices it needs coordinated access
to the bus, so collisions and RUNTIME_PM dont get in the way.
This trampoline driver allow OP-TEE to access them.
Signed-off-by: Jorge Ramirez-Ortiz
---
v9: params return value must be written before optee_msg_to_param is called
v8: review fixes:
fix types and add TEEC_ERROR_NOT_SUPPORTED to GP errors
v7: add support for ten bit i2c slave addressing
v6: compile out if CONFIG_I2C not enabled
v5: alphabetic order of includes
v4: remove unnecessary extra line in optee_msg.h
v3: use from/to msg param to support all types of memory
modify OPTEE_MSG_RPC_CMD_I2C_TRANSFER message id
drivers/tee/optee/optee_msg.h | 21 +++
drivers/tee/optee/optee_private.h | 1 +
drivers/tee/optee/rpc.c | 95 +++
3 files changed, 117 insertions(+)
diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
index 795bc19ae17a..7b2d919da2ac 100644
--- a/drivers/tee/optee/optee_msg.h
+++ b/drivers/tee/optee/optee_msg.h
@@ -419,4 +419,25 @@ struct optee_msg_arg {
*/
#define OPTEE_MSG_RPC_CMD_SHM_FREE 7
+/*
+ * Access a device on an i2c bus
+ *
+ * [in] param[0].u.value.amode: RD(0), WR(1)
+ * [in] param[0].u.value.bi2c adapter
+ * [in] param[0].u.value.ci2c chip
+ *
+ * [in] param[1].u.value.ai2c control flags
+ *
+ * [in/out] memref[2] buffer to exchange the transfer data
+ * with the secure world
+ *
+ * [out] param[3].u.value.a bytes transferred by the driver
+ */
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 21
+/* I2C master transfer modes */
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD 0
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR 1
+/* I2C master control flags */
+#define OPTEE_MSG_RPC_CMD_I2C_FLAGS_TEN_BIT BIT(0)
+
#endif /* _OPTEE_MSG_H */
diff --git a/drivers/tee/optee/optee_private.h
b/drivers/tee/optee/optee_private.h
index 8b71839a357e..564116d6cc58 100644
--- a/drivers/tee/optee/optee_private.h
+++ b/drivers/tee/optee/optee_private.h
@@ -17,6 +17,7 @@
/* Some Global Platform error codes used in this driver */
#define TEEC_SUCCESS 0x
#define TEEC_ERROR_BAD_PARAMETERS 0x0006
+#define TEEC_ERROR_NOT_SUPPORTED0x000A
#define TEEC_ERROR_COMMUNICATION 0x000E
#define TEEC_ERROR_OUT_OF_MEMORY 0x000C
#define TEEC_ERROR_SHORT_BUFFER0x0010
diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
index b4ade54d1f28..64a206c56264 100644
--- a/drivers/tee/optee/rpc.c
+++ b/drivers/tee/optee/rpc.c
@@ -7,6 +7,7 @@
#include
#include
+#include
#include
#include
#include "optee_private.h"
@@ -49,6 +50,97 @@ static void handle_rpc_func_cmd_get_time(struct
optee_msg_arg *arg)
arg->ret = TEEC_ERROR_BAD_PARAMETERS;
}
+#if IS_ENABLED(CONFIG_I2C)
+static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
+struct optee_msg_arg *arg)
+{
+ struct i2c_client client = { 0 };
+ struct tee_param *params;
+ size_t i;
+ int ret = -EOPNOTSUPP;
+ u8 attr[] = {
+ TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
+ TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
+ TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT,
+ TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT,
+ };
+
+ if (arg->num_params != ARRAY_SIZE(attr)) {
+ arg->ret = TEEC_ERROR_BAD_PARAMETERS;
+ return;
+ }
+
+ params = kmalloc_array(arg->num_params, sizeof(struct tee_param),
+ GFP_KERNEL);
+ if (!params) {
+ arg->ret = TEEC_ERROR_OUT_OF_MEMORY;
+ return;
+ }
+
+ if (optee_from_msg_param(params, arg->num_params, arg->params))
+ goto bad;
+
+ for (i = 0; i < arg->num_params; i++) {
+ if (params[i].attr != attr[i])
+ goto bad;
+ }
+
+ client.adapter = i2c_get_adapter(params[0].u.value.b);
+ if (!client.adapter)
+ goto bad;
+
+ if (params[1].u.value.a & OPTEE_MSG_RPC_CMD_I2C_FLAGS_TEN_BIT) {
+ if (!i2c_check_functionality(client.adapter,
+I2C_FUNC_10BIT_ADDR)) {
+ i2c_put_adapter(client.adapter);
+ goto bad;
+ }
+
+ client.flags = I2C_CLIENT_TEN;
+ }
+
+ client.addr = params[0].u.value.c;
+ snprintf(client.name, I2C_NAME_SIZE, "i2c%d", client.adapter->nr);
+
+ switch (params[0].u.value.a) {
+ case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
+ ret = i2c_master_recv(&client, params[2].u.memref.shm->ka
Re: [PATCHv8] drivers: optee: allow op-tee to access devices on the i2c bus
On 13/08/20, Jens Wiklander wrote: > On Wed, Aug 12, 2020 at 02:06:52PM +0200, Jorge Ramirez-Ortiz wrote: > > Some secure elements like NXP's SE050 sit on I2C buses. For OP-TEE to > > control this type of cryptographic devices it needs coordinated access > > to the bus, so collisions and RUNTIME_PM dont get in the way. > > > > This trampoline driver allow OP-TEE to access them. > > > > Signed-off-by: Jorge Ramirez-Ortiz > > --- > > > > v8: review fixes > > fix types and add TEEC_ERROR_NOT_SUPPORTED to GP errors > > v7: add support for ten bit i2c slave addressing > > v6: compile out if CONFIG_I2C not enabled > > v5: alphabetic order of includes > > v4: remove unnecessary extra line in optee_msg.h > > v3: use from/to msg param to support all types of memory > > modify OPTEE_MSG_RPC_CMD_I2C_TRANSFER message id > > > > drivers/tee/optee/optee_msg.h | 21 +++ > > drivers/tee/optee/optee_private.h | 1 + > > drivers/tee/optee/rpc.c | 96 +++ > > 3 files changed, 118 insertions(+) > > > > This looks good to me. Did you test this with the recently merged > https://github.com/OP-TEE/optee_os/pull/4024 ? I am in the process (please hold this until I can validate end to end as I was doing with the original code). I had to rebase from 3.6.0 to the tip of op-tee so it is taking me a bit longer than anticipated. Also I noticed that unfortunately a bug managed to get in that PR. The return value is on p[3] not in p[2]. See below. https://github.com/OP-TEE/optee_os/commit/30c53a72426366d0a4eb4aa396c37b8fd048a82a#r41447494 Also this version v8 also has now a problem (params get updated after calling optee_to_msg_param hence the i2c return value from the transfer does never reach optee) apologies. will fix. > > Cheers, > Jens
[PATCHv8] drivers: optee: allow op-tee to access devices on the i2c bus
Some secure elements like NXP's SE050 sit on I2C buses. For OP-TEE to
control this type of cryptographic devices it needs coordinated access
to the bus, so collisions and RUNTIME_PM dont get in the way.
This trampoline driver allow OP-TEE to access them.
Signed-off-by: Jorge Ramirez-Ortiz
---
v8: review fixes
fix types and add TEEC_ERROR_NOT_SUPPORTED to GP errors
v7: add support for ten bit i2c slave addressing
v6: compile out if CONFIG_I2C not enabled
v5: alphabetic order of includes
v4: remove unnecessary extra line in optee_msg.h
v3: use from/to msg param to support all types of memory
modify OPTEE_MSG_RPC_CMD_I2C_TRANSFER message id
drivers/tee/optee/optee_msg.h | 21 +++
drivers/tee/optee/optee_private.h | 1 +
drivers/tee/optee/rpc.c | 96 +++
3 files changed, 118 insertions(+)
diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
index 795bc19ae17a..7b2d919da2ac 100644
--- a/drivers/tee/optee/optee_msg.h
+++ b/drivers/tee/optee/optee_msg.h
@@ -419,4 +419,25 @@ struct optee_msg_arg {
*/
#define OPTEE_MSG_RPC_CMD_SHM_FREE 7
+/*
+ * Access a device on an i2c bus
+ *
+ * [in] param[0].u.value.amode: RD(0), WR(1)
+ * [in] param[0].u.value.bi2c adapter
+ * [in] param[0].u.value.ci2c chip
+ *
+ * [in] param[1].u.value.ai2c control flags
+ *
+ * [in/out] memref[2] buffer to exchange the transfer data
+ * with the secure world
+ *
+ * [out] param[3].u.value.a bytes transferred by the driver
+ */
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 21
+/* I2C master transfer modes */
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD 0
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR 1
+/* I2C master control flags */
+#define OPTEE_MSG_RPC_CMD_I2C_FLAGS_TEN_BIT BIT(0)
+
#endif /* _OPTEE_MSG_H */
diff --git a/drivers/tee/optee/optee_private.h
b/drivers/tee/optee/optee_private.h
index 8b71839a357e..564116d6cc58 100644
--- a/drivers/tee/optee/optee_private.h
+++ b/drivers/tee/optee/optee_private.h
@@ -17,6 +17,7 @@
/* Some Global Platform error codes used in this driver */
#define TEEC_SUCCESS 0x
#define TEEC_ERROR_BAD_PARAMETERS 0x0006
+#define TEEC_ERROR_NOT_SUPPORTED0x000A
#define TEEC_ERROR_COMMUNICATION 0x000E
#define TEEC_ERROR_OUT_OF_MEMORY 0x000C
#define TEEC_ERROR_SHORT_BUFFER0x0010
diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
index b4ade54d1f28..cc9ad4b68d40 100644
--- a/drivers/tee/optee/rpc.c
+++ b/drivers/tee/optee/rpc.c
@@ -7,6 +7,7 @@
#include
#include
+#include
#include
#include
#include "optee_private.h"
@@ -49,6 +50,98 @@ static void handle_rpc_func_cmd_get_time(struct
optee_msg_arg *arg)
arg->ret = TEEC_ERROR_BAD_PARAMETERS;
}
+#if IS_ENABLED(CONFIG_I2C)
+static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
+struct optee_msg_arg *arg)
+{
+ struct i2c_client client = { 0 };
+ struct tee_param *params;
+ size_t i;
+ int ret = -EOPNOTSUPP;
+ u8 attr[] = {
+ TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
+ TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
+ TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT,
+ TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT,
+ };
+
+ if (arg->num_params != ARRAY_SIZE(attr)) {
+ arg->ret = TEEC_ERROR_BAD_PARAMETERS;
+ return;
+ }
+
+ params = kmalloc_array(arg->num_params, sizeof(struct tee_param),
+ GFP_KERNEL);
+ if (!params) {
+ arg->ret = TEEC_ERROR_OUT_OF_MEMORY;
+ return;
+ }
+
+ if (optee_from_msg_param(params, arg->num_params, arg->params))
+ goto bad;
+
+ for (i = 0; i < arg->num_params; i++) {
+ if (params[i].attr != attr[i])
+ goto bad;
+ }
+
+ client.adapter = i2c_get_adapter(params[0].u.value.b);
+ if (!client.adapter)
+ goto bad;
+
+ if (params[1].u.value.a & OPTEE_MSG_RPC_CMD_I2C_FLAGS_TEN_BIT) {
+ if (!i2c_check_functionality(client.adapter,
+I2C_FUNC_10BIT_ADDR)) {
+ i2c_put_adapter(client.adapter);
+ goto bad;
+ }
+
+ client.flags = I2C_CLIENT_TEN;
+ }
+
+ client.addr = params[0].u.value.c;
+ snprintf(client.name, I2C_NAME_SIZE, "i2c%d", client.adapter->nr);
+
+ switch (params[0].u.value.a) {
+ case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
+ ret = i2c_master_recv(&client, params[2].u.memref.shm->kaddr,
+ params[2].u.memref.size);
+
Re: [PATCHv7] drivers: optee: allow op-tee to access devices on the i2c bus
On 12/08/20, Jens Wiklander wrote:
> On Tue, Aug 11, 2020 at 07:55:31PM +0200, Jorge Ramirez-Ortiz wrote:
> > Some secure elements like NXP's SE050 sit on I2C buses. For OP-TEE to
> > control this type of cryptographic devices it needs coordinated access
> > to the bus, so collisions and RUNTIME_PM dont get in the way.
> >
> > This trampoline driver allow OP-TEE to access them.
> >
> > Signed-off-by: Jorge Ramirez-Ortiz
> > ---
> > v7: add support for ten bit i2c slave addressing
> > v6: compile out if CONFIG_I2C not enabled
> > v5: alphabetic order of includes
> > v4: remove unnecessary extra line in optee_msg.h
> > v3: use from/to msg param to support all types of memory
> > modify OPTEE_MSG_RPC_CMD_I2C_TRANSFER message id
> >
> > drivers/tee/optee/optee_msg.h | 21
> > drivers/tee/optee/rpc.c | 95 +++
> > 2 files changed, 116 insertions(+)
> >
> > diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
> > index 795bc19ae17a..7b2d919da2ac 100644
> > --- a/drivers/tee/optee/optee_msg.h
> > +++ b/drivers/tee/optee/optee_msg.h
> > @@ -419,4 +419,25 @@ struct optee_msg_arg {
> > */
> > #define OPTEE_MSG_RPC_CMD_SHM_FREE 7
> >
> > +/*
> > + * Access a device on an i2c bus
> > + *
> > + * [in] param[0].u.value.amode: RD(0), WR(1)
> > + * [in] param[0].u.value.bi2c adapter
> > + * [in] param[0].u.value.ci2c chip
> > + *
> > + * [in] param[1].u.value.ai2c control flags
> > + *
> > + * [in/out] memref[2] buffer to exchange the transfer
> > data
> > + * with the secure world
> > + *
> > + * [out] param[3].u.value.a bytes transferred by the driver
> > + */
> > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 21
> > +/* I2C master transfer modes */
> > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD 0
> > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR 1
> > +/* I2C master control flags */
> > +#define OPTEE_MSG_RPC_CMD_I2C_FLAGS_TEN_BIT BIT(0)
> > +
> > #endif /* _OPTEE_MSG_H */
> > diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
> > index b4ade54d1f28..b6178761d79f 100644
> > --- a/drivers/tee/optee/rpc.c
> > +++ b/drivers/tee/optee/rpc.c
> > @@ -7,6 +7,7 @@
> >
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include "optee_private.h"
> > @@ -49,6 +50,97 @@ static void handle_rpc_func_cmd_get_time(struct
> > optee_msg_arg *arg)
> > arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> > }
> >
> > +#if IS_ENABLED(CONFIG_I2C)
> > +static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
> > +struct optee_msg_arg *arg)
> > +{
> > + struct i2c_client client = { 0 };
> > + struct tee_param *params;
> > + int i, ret = -EOPNOTSUPP;
> > + uint32_t attr[] = {
> uint32_t seems a randomly chosen type here. The
> TEE_IOCTL_PARAM_ATTR_TYPE_* defines fit in a u8.
> Consider u8 attr[] = { instead.
ok
>
> > + TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> > + TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> > + TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT,
> > + TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT,
> > + };
> > +
> > + if (arg->num_params != ARRAY_SIZE(attr)) {
> > + arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> > + return;
> > + }
> > +
> > + params = kmalloc_array(arg->num_params, sizeof(struct tee_param),
> > + GFP_KERNEL);
> > + if (!params) {
> > + arg->ret = TEEC_ERROR_OUT_OF_MEMORY;
> > + return;
> > + }
> > +
> > + if (optee_from_msg_param(params, arg->num_params, arg->params))
> > + goto bad;
> > +
> > + for (i = 0; i < arg->num_params; i++) {
> arg->num_params has an unsigned type. Consider giving i an unsigned type
> too.
ok
>
> > + if ((params[i].attr & TEE_IOCTL_PARAM_ATTR_TYPE_MASK)
> > + != attr[i])
> It looks a bit funny with the comparison at the beginning of the line,
> normally we try to leave those at the end of the line. But
> optee_from_msg_param() doesn't set any higher bits in params[i].attr so
> you could also simplify the expression as:
> if (params[i].attr !
[PATCHv7] drivers: optee: allow op-tee to access devices on the i2c bus
Some secure elements like NXP's SE050 sit on I2C buses. For OP-TEE to
control this type of cryptographic devices it needs coordinated access
to the bus, so collisions and RUNTIME_PM dont get in the way.
This trampoline driver allow OP-TEE to access them.
Signed-off-by: Jorge Ramirez-Ortiz
---
v7: add support for ten bit i2c slave addressing
v6: compile out if CONFIG_I2C not enabled
v5: alphabetic order of includes
v4: remove unnecessary extra line in optee_msg.h
v3: use from/to msg param to support all types of memory
modify OPTEE_MSG_RPC_CMD_I2C_TRANSFER message id
drivers/tee/optee/optee_msg.h | 21
drivers/tee/optee/rpc.c | 95 +++
2 files changed, 116 insertions(+)
diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
index 795bc19ae17a..7b2d919da2ac 100644
--- a/drivers/tee/optee/optee_msg.h
+++ b/drivers/tee/optee/optee_msg.h
@@ -419,4 +419,25 @@ struct optee_msg_arg {
*/
#define OPTEE_MSG_RPC_CMD_SHM_FREE 7
+/*
+ * Access a device on an i2c bus
+ *
+ * [in] param[0].u.value.amode: RD(0), WR(1)
+ * [in] param[0].u.value.bi2c adapter
+ * [in] param[0].u.value.ci2c chip
+ *
+ * [in] param[1].u.value.ai2c control flags
+ *
+ * [in/out] memref[2] buffer to exchange the transfer data
+ * with the secure world
+ *
+ * [out] param[3].u.value.a bytes transferred by the driver
+ */
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 21
+/* I2C master transfer modes */
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD 0
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR 1
+/* I2C master control flags */
+#define OPTEE_MSG_RPC_CMD_I2C_FLAGS_TEN_BIT BIT(0)
+
#endif /* _OPTEE_MSG_H */
diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
index b4ade54d1f28..b6178761d79f 100644
--- a/drivers/tee/optee/rpc.c
+++ b/drivers/tee/optee/rpc.c
@@ -7,6 +7,7 @@
#include
#include
+#include
#include
#include
#include "optee_private.h"
@@ -49,6 +50,97 @@ static void handle_rpc_func_cmd_get_time(struct
optee_msg_arg *arg)
arg->ret = TEEC_ERROR_BAD_PARAMETERS;
}
+#if IS_ENABLED(CONFIG_I2C)
+static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
+struct optee_msg_arg *arg)
+{
+ struct i2c_client client = { 0 };
+ struct tee_param *params;
+ int i, ret = -EOPNOTSUPP;
+ uint32_t attr[] = {
+ TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
+ TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
+ TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT,
+ TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT,
+ };
+
+ if (arg->num_params != ARRAY_SIZE(attr)) {
+ arg->ret = TEEC_ERROR_BAD_PARAMETERS;
+ return;
+ }
+
+ params = kmalloc_array(arg->num_params, sizeof(struct tee_param),
+ GFP_KERNEL);
+ if (!params) {
+ arg->ret = TEEC_ERROR_OUT_OF_MEMORY;
+ return;
+ }
+
+ if (optee_from_msg_param(params, arg->num_params, arg->params))
+ goto bad;
+
+ for (i = 0; i < arg->num_params; i++) {
+ if ((params[i].attr & TEE_IOCTL_PARAM_ATTR_TYPE_MASK)
+ != attr[i])
+ goto bad;
+ }
+
+ client.adapter = i2c_get_adapter(params[0].u.value.b);
+ if (!client.adapter)
+ goto bad;
+
+ if (params[1].u.value.a & OPTEE_MSG_RPC_CMD_I2C_FLAGS_TEN_BIT) {
+ if (!i2c_check_functionality(client.adapter,
+I2C_FUNC_10BIT_ADDR)) {
+ i2c_put_adapter(client.adapter);
+ goto bad;
+ }
+
+ client.flags = I2C_CLIENT_TEN;
+ }
+
+ client.addr = params[0].u.value.c;
+ snprintf(client.name, I2C_NAME_SIZE, "i2c%d", client.adapter->nr);
+
+ switch (params[0].u.value.a) {
+ case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
+ ret = i2c_master_recv(&client, params[2].u.memref.shm->kaddr,
+ params[2].u.memref.size);
+ break;
+ case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR:
+ ret = i2c_master_send(&client, params[2].u.memref.shm->kaddr,
+ params[2].u.memref.size);
+ break;
+ default:
+ i2c_put_adapter(client.adapter);
+ goto bad;
+ }
+
+ if (ret < 0) {
+ arg->ret = TEEC_ERROR_COMMUNICATION;
+ } else {
+ params[3].u.value.a = ret;
+ arg->ret = TEEC_SUCCESS;
+
+ if (optee_to_msg_param(arg->params, arg->num_params, params))
+ arg->ret = TEEC_
[PATCHv3 1/2] hwrng: optee: handle unlimited data rates
Data rates of MAX_UINT32 will schedule an unnecessary one jiffy
timeout on the call to msleep. Avoid this scenario by using 0 as the
unlimited data rate.
Signed-off-by: Jorge Ramirez-Ortiz
Reviewed-by: Sumit Garg
---
drivers/char/hw_random/optee-rng.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/char/hw_random/optee-rng.c
b/drivers/char/hw_random/optee-rng.c
index 49b2e02537dd..5bc4700c4dae 100644
--- a/drivers/char/hw_random/optee-rng.c
+++ b/drivers/char/hw_random/optee-rng.c
@@ -128,7 +128,7 @@ static int optee_rng_read(struct hwrng *rng, void *buf,
size_t max, bool wait)
data += rng_size;
read += rng_size;
- if (wait) {
+ if (wait && pvt_data->data_rate) {
if (timeout-- == 0)
return read;
msleep((1000 * (max - read)) / pvt_data->data_rate);
--
2.17.1
[PATCHv3 2/2] hwrng: optee: fix wait use case
The current code waits for data to be available before attempting a
second read. However the second read would not be executed as the
while loop will exit.
This fix does not wait if all data has been read (skips the call to
msleep(0)) and reads a second time if partial data was retrieved on
the first read.
Worth noticing that since msleep(0) schedules a one jiffy timeout is
better to skip such a call.
Signed-off-by: Jorge Ramirez-Ortiz
Reviewed-by: Sumit Garg
---
drivers/char/hw_random/optee-rng.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/char/hw_random/optee-rng.c
b/drivers/char/hw_random/optee-rng.c
index 5bc4700c4dae..a99d82949981 100644
--- a/drivers/char/hw_random/optee-rng.c
+++ b/drivers/char/hw_random/optee-rng.c
@@ -122,14 +122,14 @@ static int optee_rng_read(struct hwrng *rng, void *buf,
size_t max, bool wait)
if (max > MAX_ENTROPY_REQ_SZ)
max = MAX_ENTROPY_REQ_SZ;
- while (read == 0) {
+ while (read < max) {
rng_size = get_optee_rng_data(pvt_data, data, (max - read));
data += rng_size;
read += rng_size;
if (wait && pvt_data->data_rate) {
- if (timeout-- == 0)
+ if ((timeout-- == 0) || (read == max))
return read;
msleep((1000 * (max - read)) / pvt_data->data_rate);
} else {
--
2.17.1
[PATCHv3 2/2] hwrng: optee: fix wait use case
The current code waits for data to be available before attempting a
second read. However the second read would not be executed as the
while loop will exit.
This fix does not wait if all data has been read (skips the call to
msleep(0)) and reads a second time if partial data was retrieved on
the first read.
Worth noticing that since msleep(0) schedules a one jiffy timeout is
better to skip such a call.
Signed-off-by: Jorge Ramirez-Ortiz
Reviewed-by: Sumit Garg
---
drivers/char/hw_random/optee-rng.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/char/hw_random/optee-rng.c
b/drivers/char/hw_random/optee-rng.c
index 5bc4700c4dae..a99d82949981 100644
--- a/drivers/char/hw_random/optee-rng.c
+++ b/drivers/char/hw_random/optee-rng.c
@@ -122,14 +122,14 @@ static int optee_rng_read(struct hwrng *rng, void *buf,
size_t max, bool wait)
if (max > MAX_ENTROPY_REQ_SZ)
max = MAX_ENTROPY_REQ_SZ;
- while (read == 0) {
+ while (read < max) {
rng_size = get_optee_rng_data(pvt_data, data, (max - read));
data += rng_size;
read += rng_size;
if (wait && pvt_data->data_rate) {
- if (timeout-- == 0)
+ if ((timeout-- == 0) || (read == max))
return read;
msleep((1000 * (max - read)) / pvt_data->data_rate);
} else {
--
2.17.1
Re: [PATCHv2 2/2] hwrng: optee: fix wait use case
On 06/08/20, Sumit Garg wrote:
> On Thu, 6 Aug 2020 at 12:00, Jorge Ramirez-Ortiz, Foundries
> wrote:
> >
> > On 06/08/20, Sumit Garg wrote:
> > > On Thu, 6 Aug 2020 at 02:08, Jorge Ramirez-Ortiz, Foundries
> > > wrote:
> > > >
> > > > On 05/08/20, Sumit Garg wrote:
> > > > > Apologies for my delayed response as I was busy with some other tasks
> > > > > along with holidays.
> > > >
> > > > no pb! was just making sure this wasnt falling through some cracks.
> > > >
> > > > >
> > > > > On Fri, 24 Jul 2020 at 19:53, Jorge Ramirez-Ortiz, Foundries
> > > > > wrote:
> > > > > >
> > > > > > On 24/07/20, Sumit Garg wrote:
> > > > > > > On Thu, 23 Jul 2020 at 14:16, Jorge Ramirez-Ortiz
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > The current code waits for data to be available before
> > > > > > > > attempting a
> > > > > > > > second read. However the second read would not be executed as
> > > > > > > > the
> > > > > > > > while loop exits.
> > > > > > > >
> > > > > > > > This fix does not wait if all data has been read and reads a
> > > > > > > > second
> > > > > > > > time if only partial data was retrieved on the first read.
> > > > > > > >
> > > > > > > > This fix also does not attempt to read if not data is requested.
> > > > > > >
> > > > > > > I am not sure how this is possible, can you elaborate?
> > > > > >
> > > > > > currently, if the user sets max 0, get_optee_rng_data will
> > > > > > regardless
> > > > > > issuese a call to the secure world requesting 0 bytes from the RNG
> > > > > >
> > > > >
> > > > > This case is already handled by core API: rng_dev_read().
> > > >
> > > > ah ok good point, you are right
> > > > but yeah, there is no consequence to the actual patch.
> > > >
> > >
> > > So, at least you could get rid of the corresponding text from commit
> > > message.
> > >
> > > > >
> > > > > > with this patch, this request is avoided.
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > Signed-off-by: Jorge Ramirez-Ortiz
> > > > > > > > ---
> > > > > > > > v2: tidy up the while loop to avoid reading when no data is
> > > > > > > > requested
> > > > > > > >
> > > > > > > > drivers/char/hw_random/optee-rng.c | 4 ++--
> > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/char/hw_random/optee-rng.c
> > > > > > > > b/drivers/char/hw_random/optee-rng.c
> > > > > > > > index 5bc4700c4dae..a99d82949981 100644
> > > > > > > > --- a/drivers/char/hw_random/optee-rng.c
> > > > > > > > +++ b/drivers/char/hw_random/optee-rng.c
> > > > > > > > @@ -122,14 +122,14 @@ static int optee_rng_read(struct hwrng
> > > > > > > > *rng, void *buf, size_t max, bool wait)
> > > > > > > > if (max > MAX_ENTROPY_REQ_SZ)
> > > > > > > > max = MAX_ENTROPY_REQ_SZ;
> > > > > > > >
> > > > > > > > - while (read == 0) {
> > > > > > > > + while (read < max) {
> > > > > > > > rng_size = get_optee_rng_data(pvt_data, data,
> > > > > > > > (max - read));
> > > > > > > >
> > > > > > > > data += rng_size;
> > > > > > > > read += rng_size;
> > > > > > > >
> > > > > > > > if (wait && pvt_data->data_rate) {
> > > > > > > > - if (timeout-- == 0)
> > > > > > > > + if ((timeout-- == 0) || (read == max))
> > > > > > >
> >
Re: [PATCHv2 2/2] hwrng: optee: fix wait use case
On 06/08/20, Sumit Garg wrote:
> On Thu, 6 Aug 2020 at 02:08, Jorge Ramirez-Ortiz, Foundries
> wrote:
> >
> > On 05/08/20, Sumit Garg wrote:
> > > Apologies for my delayed response as I was busy with some other tasks
> > > along with holidays.
> >
> > no pb! was just making sure this wasnt falling through some cracks.
> >
> > >
> > > On Fri, 24 Jul 2020 at 19:53, Jorge Ramirez-Ortiz, Foundries
> > > wrote:
> > > >
> > > > On 24/07/20, Sumit Garg wrote:
> > > > > On Thu, 23 Jul 2020 at 14:16, Jorge Ramirez-Ortiz
> > > > > wrote:
> > > > > >
> > > > > > The current code waits for data to be available before attempting a
> > > > > > second read. However the second read would not be executed as the
> > > > > > while loop exits.
> > > > > >
> > > > > > This fix does not wait if all data has been read and reads a second
> > > > > > time if only partial data was retrieved on the first read.
> > > > > >
> > > > > > This fix also does not attempt to read if not data is requested.
> > > > >
> > > > > I am not sure how this is possible, can you elaborate?
> > > >
> > > > currently, if the user sets max 0, get_optee_rng_data will regardless
> > > > issuese a call to the secure world requesting 0 bytes from the RNG
> > > >
> > >
> > > This case is already handled by core API: rng_dev_read().
> >
> > ah ok good point, you are right
> > but yeah, there is no consequence to the actual patch.
> >
>
> So, at least you could get rid of the corresponding text from commit message.
>
> > >
> > > > with this patch, this request is avoided.
> > > >
> > > > >
> > > > > >
> > > > > > Signed-off-by: Jorge Ramirez-Ortiz
> > > > > > ---
> > > > > > v2: tidy up the while loop to avoid reading when no data is
> > > > > > requested
> > > > > >
> > > > > > drivers/char/hw_random/optee-rng.c | 4 ++--
> > > > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/char/hw_random/optee-rng.c
> > > > > > b/drivers/char/hw_random/optee-rng.c
> > > > > > index 5bc4700c4dae..a99d82949981 100644
> > > > > > --- a/drivers/char/hw_random/optee-rng.c
> > > > > > +++ b/drivers/char/hw_random/optee-rng.c
> > > > > > @@ -122,14 +122,14 @@ static int optee_rng_read(struct hwrng *rng,
> > > > > > void *buf, size_t max, bool wait)
> > > > > > if (max > MAX_ENTROPY_REQ_SZ)
> > > > > > max = MAX_ENTROPY_REQ_SZ;
> > > > > >
> > > > > > - while (read == 0) {
> > > > > > + while (read < max) {
> > > > > > rng_size = get_optee_rng_data(pvt_data, data, (max
> > > > > > - read));
> > > > > >
> > > > > > data += rng_size;
> > > > > > read += rng_size;
> > > > > >
> > > > > > if (wait && pvt_data->data_rate) {
> > > > > > - if (timeout-- == 0)
> > > > > > + if ((timeout-- == 0) || (read == max))
> > > > >
> > > > > If read == max, would there be any sleep?
> > > >
> > > > no but I see no reason why there should be a wait since we already have
> > > > all the data that we need; the msleep is only required when we need to
> > > > wait for the RNG to generate entropy for the number of bytes we are
> > > > requesting. if we are requesting 0 bytes, the entropy is already
> > > > available. at leat this is what makes sense to me.
> > > >
> > >
> > > Wouldn't it lead to a call as msleep(0); that means no wait as well?
> >
> > I dont understand: there is no reason to wait if read == max and this
> > patch will not wait: if read == max it calls 'return read'
> >
> > am I misunderstanding your point?
>
> What I mean is that we shouldn't require this extra check here as
> there wasn't any wait if read == max with existing implementation too.
um, I am getting confused Sumit
with the exisiting implementation (the one we aim to replace), if
get_optee_rng_data reads all the values requested on the first call (ie, read =
0) with wait set to true, the call will wait with msleep(0). Which is
unnecessary and waits for a jiffy (ie, the call to msleep 0 will schedule a one
jiffy timeout interrruptible)
with this alternative implementation, msleep(0) does not get called.
are we in synch?
>
> -Sumit
>
> >
> > >
> > > -Sumit
> > >
> > > >
> > > > >
> > > > > -Sumit
> > > > >
> > > > > > return read;
> > > > > > msleep((1000 * (max - read)) /
> > > > > > pvt_data->data_rate);
> > > > > > } else {
> > > > > > --
> > > > > > 2.17.1
> > > > > >
Re: [PATCHv2 2/2] hwrng: optee: fix wait use case
On 05/08/20, Sumit Garg wrote:
> Apologies for my delayed response as I was busy with some other tasks
> along with holidays.
no pb! was just making sure this wasnt falling through some cracks.
>
> On Fri, 24 Jul 2020 at 19:53, Jorge Ramirez-Ortiz, Foundries
> wrote:
> >
> > On 24/07/20, Sumit Garg wrote:
> > > On Thu, 23 Jul 2020 at 14:16, Jorge Ramirez-Ortiz
> > > wrote:
> > > >
> > > > The current code waits for data to be available before attempting a
> > > > second read. However the second read would not be executed as the
> > > > while loop exits.
> > > >
> > > > This fix does not wait if all data has been read and reads a second
> > > > time if only partial data was retrieved on the first read.
> > > >
> > > > This fix also does not attempt to read if not data is requested.
> > >
> > > I am not sure how this is possible, can you elaborate?
> >
> > currently, if the user sets max 0, get_optee_rng_data will regardless
> > issuese a call to the secure world requesting 0 bytes from the RNG
> >
>
> This case is already handled by core API: rng_dev_read().
ah ok good point, you are right
but yeah, there is no consequence to the actual patch.
>
> > with this patch, this request is avoided.
> >
> > >
> > > >
> > > > Signed-off-by: Jorge Ramirez-Ortiz
> > > > ---
> > > > v2: tidy up the while loop to avoid reading when no data is requested
> > > >
> > > > drivers/char/hw_random/optee-rng.c | 4 ++--
> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/char/hw_random/optee-rng.c
> > > > b/drivers/char/hw_random/optee-rng.c
> > > > index 5bc4700c4dae..a99d82949981 100644
> > > > --- a/drivers/char/hw_random/optee-rng.c
> > > > +++ b/drivers/char/hw_random/optee-rng.c
> > > > @@ -122,14 +122,14 @@ static int optee_rng_read(struct hwrng *rng, void
> > > > *buf, size_t max, bool wait)
> > > > if (max > MAX_ENTROPY_REQ_SZ)
> > > > max = MAX_ENTROPY_REQ_SZ;
> > > >
> > > > - while (read == 0) {
> > > > + while (read < max) {
> > > > rng_size = get_optee_rng_data(pvt_data, data, (max -
> > > > read));
> > > >
> > > > data += rng_size;
> > > > read += rng_size;
> > > >
> > > > if (wait && pvt_data->data_rate) {
> > > > - if (timeout-- == 0)
> > > > + if ((timeout-- == 0) || (read == max))
> > >
> > > If read == max, would there be any sleep?
> >
> > no but I see no reason why there should be a wait since we already have
> > all the data that we need; the msleep is only required when we need to
> > wait for the RNG to generate entropy for the number of bytes we are
> > requesting. if we are requesting 0 bytes, the entropy is already
> > available. at leat this is what makes sense to me.
> >
>
> Wouldn't it lead to a call as msleep(0); that means no wait as well?
I dont understand: there is no reason to wait if read == max and this
patch will not wait: if read == max it calls 'return read'
am I misunderstanding your point?
>
> -Sumit
>
> >
> > >
> > > -Sumit
> > >
> > > > return read;
> > > > msleep((1000 * (max - read)) /
> > > > pvt_data->data_rate);
> > > > } else {
> > > > --
> > > > 2.17.1
> > > >
Re: [PATCHv6] drivers: optee: allow op-tee to access devices on the i2c bus
On 05/08/20, Jens Wiklander wrote:
> On Wed, Aug 05, 2020 at 03:35:01PM +0200, Jorge Ramirez-Ortiz, Foundries
> wrote:
> > On 22/07/20, Jorge Ramirez-Ortiz wrote:
> > > Some secure elements like NXP's SE050 sit on I2C buses. For OP-TEE to
> > > control this type of cryptographic devices it needs coordinated access
> > > to the bus, so collisions and RUNTIME_PM dont get in the way.
> > >
> > > This trampoline driver allow OP-TEE to access them.
> > > Tested on imx8mm LPDDR4
> > >
> > > Signed-off-by: Jorge Ramirez-Ortiz
> > > ---
> > > v6: compile out if CONFIG_I2C not enabled
> > > v5: alphabetic order of includes
> > > v4: remove unnecessary extra line in optee_msg.h
> > > v3: use from/to msg param to support all types of memory
> > > modify OPTEE_MSG_RPC_CMD_I2C_TRANSFER message id
> > >
> > > drivers/tee/optee/optee_msg.h | 16 +++
> > > drivers/tee/optee/rpc.c | 88 +++
> > > 2 files changed, 104 insertions(+)
> > >
> > > diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
> > > index 795bc19ae17a..14b580f55356 100644
> > > --- a/drivers/tee/optee/optee_msg.h
> > > +++ b/drivers/tee/optee/optee_msg.h
> > > @@ -419,4 +419,20 @@ struct optee_msg_arg {
> > > */
> > > #define OPTEE_MSG_RPC_CMD_SHM_FREE 7
> > >
> > > +/*
> > > + * Access a device on an i2c bus
> > > + *
> > > + * [in] param[0].u.value.a mode: RD(0), WR(1)
> > > + * [in] param[0].u.value.b i2c adapter
> > > + * [in] param[0].u.value.c i2c chip
> > > + *
> > > + * [in/out] memref[1]buffer to exchange the transfer
> > > data
> > > + * with the secure world
> > > + *
> > > + * [out] param[0].u.value.a bytes transferred by the driver
> > > + */
> > > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 21
> > > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD 0
> > > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR 1
> > > +
> > > #endif /* _OPTEE_MSG_H */
> > > diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
> > > index b4ade54d1f28..5fd5c6c93896 100644
> > > --- a/drivers/tee/optee/rpc.c
> > > +++ b/drivers/tee/optee/rpc.c
> > > @@ -7,6 +7,7 @@
> > >
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > > #include "optee_private.h"
> > > @@ -49,6 +50,90 @@ static void handle_rpc_func_cmd_get_time(struct
> > > optee_msg_arg *arg)
> > > arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> > > }
> > >
> > > +#if IS_ENABLED(CONFIG_I2C)
> > > +static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
> > > + struct optee_msg_arg *arg)
> > > +{
> > > + struct i2c_client client;
> > > + struct tee_param *params;
> > > + uint32_t type;
> > > + int i, ret;
> > > + size_t len;
> > > + char *buf;
> > > + uint32_t attr[] = {
> > > + TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> > > + TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT,
> > > + TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT,
> > > + };
> > > +
> > > + if (arg->num_params != ARRAY_SIZE(attr)) {
> > > + arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> > > + return;
> > > + }
> > > +
> > > + params = kmalloc_array(arg->num_params, sizeof(struct tee_param),
> > > +GFP_KERNEL);
> > > + if (!params) {
> > > + arg->ret = TEEC_ERROR_OUT_OF_MEMORY;
> > > + return;
> > > + }
> > > +
> > > + if (optee_from_msg_param(params, arg->num_params, arg->params))
> > > + goto bad;
> > > +
> > > + for (i = 0; i < arg->num_params; i++) {
> > > + type = params[i].attr & TEE_IOCTL_PARAM_ATTR_TYPE_MASK;
> > > + if (type != attr[i])
> > > + goto bad;
> > > + }
> > > +
> > > + client.addr = params[0].u.value.c;
> > > + client.adapter = i2c_get_adapter(params[0].u.value.b);
> > > + if (!client.adapter)
> > > + goto bad;
> > > +
> > > + snprintf(client.name, I2C
Re: [PATCHv2 2/2] hwrng: optee: fix wait use case
On 23/07/20, Jorge Ramirez-Ortiz wrote:
> The current code waits for data to be available before attempting a
> second read. However the second read would not be executed as the
> while loop exits.
>
> This fix does not wait if all data has been read and reads a second
> time if only partial data was retrieved on the first read.
>
> This fix also does not attempt to read if not data is requested.
>
> Signed-off-by: Jorge Ramirez-Ortiz
> ---
> v2: tidy up the while loop to avoid reading when no data is requested
>
> drivers/char/hw_random/optee-rng.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/char/hw_random/optee-rng.c
> b/drivers/char/hw_random/optee-rng.c
> index 5bc4700c4dae..a99d82949981 100644
> --- a/drivers/char/hw_random/optee-rng.c
> +++ b/drivers/char/hw_random/optee-rng.c
> @@ -122,14 +122,14 @@ static int optee_rng_read(struct hwrng *rng, void *buf,
> size_t max, bool wait)
> if (max > MAX_ENTROPY_REQ_SZ)
> max = MAX_ENTROPY_REQ_SZ;
>
> - while (read == 0) {
> + while (read < max) {
> rng_size = get_optee_rng_data(pvt_data, data, (max - read));
>
> data += rng_size;
> read += rng_size;
>
> if (wait && pvt_data->data_rate) {
> - if (timeout-- == 0)
> + if ((timeout-- == 0) || (read == max))
> return read;
> msleep((1000 * (max - read)) / pvt_data->data_rate);
> } else {
any comments please?
Re: [PATCHv6] drivers: optee: allow op-tee to access devices on the i2c bus
On 22/07/20, Jorge Ramirez-Ortiz wrote:
> Some secure elements like NXP's SE050 sit on I2C buses. For OP-TEE to
> control this type of cryptographic devices it needs coordinated access
> to the bus, so collisions and RUNTIME_PM dont get in the way.
>
> This trampoline driver allow OP-TEE to access them.
> Tested on imx8mm LPDDR4
>
> Signed-off-by: Jorge Ramirez-Ortiz
> ---
> v6: compile out if CONFIG_I2C not enabled
> v5: alphabetic order of includes
> v4: remove unnecessary extra line in optee_msg.h
> v3: use from/to msg param to support all types of memory
> modify OPTEE_MSG_RPC_CMD_I2C_TRANSFER message id
>
> drivers/tee/optee/optee_msg.h | 16 +++
> drivers/tee/optee/rpc.c | 88 +++
> 2 files changed, 104 insertions(+)
>
> diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
> index 795bc19ae17a..14b580f55356 100644
> --- a/drivers/tee/optee/optee_msg.h
> +++ b/drivers/tee/optee/optee_msg.h
> @@ -419,4 +419,20 @@ struct optee_msg_arg {
> */
> #define OPTEE_MSG_RPC_CMD_SHM_FREE 7
>
> +/*
> + * Access a device on an i2c bus
> + *
> + * [in] param[0].u.value.a mode: RD(0), WR(1)
> + * [in] param[0].u.value.b i2c adapter
> + * [in] param[0].u.value.c i2c chip
> + *
> + * [in/out] memref[1]buffer to exchange the transfer
> data
> + * with the secure world
> + *
> + * [out] param[0].u.value.a bytes transferred by the driver
> + */
> +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 21
> +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD 0
> +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR 1
> +
> #endif /* _OPTEE_MSG_H */
> diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
> index b4ade54d1f28..5fd5c6c93896 100644
> --- a/drivers/tee/optee/rpc.c
> +++ b/drivers/tee/optee/rpc.c
> @@ -7,6 +7,7 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include "optee_private.h"
> @@ -49,6 +50,90 @@ static void handle_rpc_func_cmd_get_time(struct
> optee_msg_arg *arg)
> arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> }
>
> +#if IS_ENABLED(CONFIG_I2C)
> +static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
> + struct optee_msg_arg *arg)
> +{
> + struct i2c_client client;
> + struct tee_param *params;
> + uint32_t type;
> + int i, ret;
> + size_t len;
> + char *buf;
> + uint32_t attr[] = {
> + TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> + TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT,
> + TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT,
> + };
> +
> + if (arg->num_params != ARRAY_SIZE(attr)) {
> + arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> + return;
> + }
> +
> + params = kmalloc_array(arg->num_params, sizeof(struct tee_param),
> +GFP_KERNEL);
> + if (!params) {
> + arg->ret = TEEC_ERROR_OUT_OF_MEMORY;
> + return;
> + }
> +
> + if (optee_from_msg_param(params, arg->num_params, arg->params))
> + goto bad;
> +
> + for (i = 0; i < arg->num_params; i++) {
> + type = params[i].attr & TEE_IOCTL_PARAM_ATTR_TYPE_MASK;
> + if (type != attr[i])
> + goto bad;
> + }
> +
> + client.addr = params[0].u.value.c;
> + client.adapter = i2c_get_adapter(params[0].u.value.b);
> + if (!client.adapter)
> + goto bad;
> +
> + snprintf(client.name, I2C_NAME_SIZE, "i2c%d", client.adapter->nr);
> +
> + buf = params[1].u.memref.shm->kaddr;
> + len = params[1].u.memref.size;
> +
> + switch (params[0].u.value.a) {
> + case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
> + ret = i2c_master_recv(&client, buf, len);
> + break;
> + case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR:
> + ret = i2c_master_send(&client, buf, len);
> + break;
> + default:
> + i2c_put_adapter(client.adapter);
> + goto bad;
> + }
> +
> + if (ret >= 0) {
> + params[2].u.value.a = ret;
> + arg->ret = TEEC_SUCCESS;
> + } else {
> + arg->ret = TEEC_ERROR_COMMUNICATION;
> + }
> +
> + if (optee_to_msg_param(arg->params, arg->num_params, params))
> + arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> +
> + i2c_put_adapter(client.adapter);
> + kfree(params);
> + return;
>
Re: [PATCHv6] drivers: optee: allow op-tee to access devices on the i2c bus
On 22/07/20, Jorge Ramirez-Ortiz wrote:
> Some secure elements like NXP's SE050 sit on I2C buses. For OP-TEE to
> control this type of cryptographic devices it needs coordinated access
> to the bus, so collisions and RUNTIME_PM dont get in the way.
>
> This trampoline driver allow OP-TEE to access them.
> Tested on imx8mm LPDDR4
>
> Signed-off-by: Jorge Ramirez-Ortiz
> ---
> v6: compile out if CONFIG_I2C not enabled
> v5: alphabetic order of includes
> v4: remove unnecessary extra line in optee_msg.h
> v3: use from/to msg param to support all types of memory
> modify OPTEE_MSG_RPC_CMD_I2C_TRANSFER message id
>
> drivers/tee/optee/optee_msg.h | 16 +++
> drivers/tee/optee/rpc.c | 88 +++
> 2 files changed, 104 insertions(+)
>
> diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
> index 795bc19ae17a..14b580f55356 100644
> --- a/drivers/tee/optee/optee_msg.h
> +++ b/drivers/tee/optee/optee_msg.h
> @@ -419,4 +419,20 @@ struct optee_msg_arg {
> */
> #define OPTEE_MSG_RPC_CMD_SHM_FREE 7
>
> +/*
> + * Access a device on an i2c bus
> + *
> + * [in] param[0].u.value.a mode: RD(0), WR(1)
> + * [in] param[0].u.value.b i2c adapter
> + * [in] param[0].u.value.c i2c chip
> + *
> + * [in/out] memref[1]buffer to exchange the transfer
> data
> + * with the secure world
> + *
> + * [out] param[0].u.value.a bytes transferred by the driver
> + */
> +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 21
> +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD 0
> +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR 1
> +
> #endif /* _OPTEE_MSG_H */
> diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
> index b4ade54d1f28..5fd5c6c93896 100644
> --- a/drivers/tee/optee/rpc.c
> +++ b/drivers/tee/optee/rpc.c
> @@ -7,6 +7,7 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include "optee_private.h"
> @@ -49,6 +50,90 @@ static void handle_rpc_func_cmd_get_time(struct
> optee_msg_arg *arg)
> arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> }
>
> +#if IS_ENABLED(CONFIG_I2C)
> +static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
> + struct optee_msg_arg *arg)
> +{
> + struct i2c_client client;
> + struct tee_param *params;
> + uint32_t type;
> + int i, ret;
> + size_t len;
> + char *buf;
> + uint32_t attr[] = {
> + TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
> + TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT,
> + TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT,
> + };
> +
> + if (arg->num_params != ARRAY_SIZE(attr)) {
> + arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> + return;
> + }
> +
> + params = kmalloc_array(arg->num_params, sizeof(struct tee_param),
> +GFP_KERNEL);
> + if (!params) {
> + arg->ret = TEEC_ERROR_OUT_OF_MEMORY;
> + return;
> + }
> +
> + if (optee_from_msg_param(params, arg->num_params, arg->params))
> + goto bad;
> +
> + for (i = 0; i < arg->num_params; i++) {
> + type = params[i].attr & TEE_IOCTL_PARAM_ATTR_TYPE_MASK;
> + if (type != attr[i])
> + goto bad;
> + }
> +
> + client.addr = params[0].u.value.c;
> + client.adapter = i2c_get_adapter(params[0].u.value.b);
> + if (!client.adapter)
> + goto bad;
> +
> + snprintf(client.name, I2C_NAME_SIZE, "i2c%d", client.adapter->nr);
> +
> + buf = params[1].u.memref.shm->kaddr;
> + len = params[1].u.memref.size;
> +
> + switch (params[0].u.value.a) {
> + case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
> + ret = i2c_master_recv(&client, buf, len);
> + break;
> + case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR:
> + ret = i2c_master_send(&client, buf, len);
> + break;
> + default:
> + i2c_put_adapter(client.adapter);
> + goto bad;
> + }
> +
> + if (ret >= 0) {
> + params[2].u.value.a = ret;
> + arg->ret = TEEC_SUCCESS;
> + } else {
> + arg->ret = TEEC_ERROR_COMMUNICATION;
> + }
> +
> + if (optee_to_msg_param(arg->params, arg->num_params, params))
> + arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> +
> + i2c_put_adapter(client.adapter);
> + kfree(params);
> + return;
>
Re: [PATCHv2 2/2] hwrng: optee: fix wait use case
On 24/07/20, Jorge Ramirez-Ortiz, Foundries wrote:
> On 24/07/20, Sumit Garg wrote:
> > On Thu, 23 Jul 2020 at 14:16, Jorge Ramirez-Ortiz
> > wrote:
> > >
> > > The current code waits for data to be available before attempting a
> > > second read. However the second read would not be executed as the
> > > while loop exits.
> > >
> > > This fix does not wait if all data has been read and reads a second
> > > time if only partial data was retrieved on the first read.
> > >
> > > This fix also does not attempt to read if not data is requested.
> >
> > I am not sure how this is possible, can you elaborate?
>
> currently, if the user sets max 0, get_optee_rng_data will regardless
> issuese a call to the secure world requesting 0 bytes from the RNG
>
> with this patch, this request is avoided.
>
> >
> > >
> > > Signed-off-by: Jorge Ramirez-Ortiz
> > > ---
> > > v2: tidy up the while loop to avoid reading when no data is requested
> > >
> > > drivers/char/hw_random/optee-rng.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/char/hw_random/optee-rng.c
> > > b/drivers/char/hw_random/optee-rng.c
> > > index 5bc4700c4dae..a99d82949981 100644
> > > --- a/drivers/char/hw_random/optee-rng.c
> > > +++ b/drivers/char/hw_random/optee-rng.c
> > > @@ -122,14 +122,14 @@ static int optee_rng_read(struct hwrng *rng, void
> > > *buf, size_t max, bool wait)
> > > if (max > MAX_ENTROPY_REQ_SZ)
> > > max = MAX_ENTROPY_REQ_SZ;
> > >
> > > - while (read == 0) {
> > > + while (read < max) {
> > > rng_size = get_optee_rng_data(pvt_data, data, (max -
> > > read));
> > >
> > > data += rng_size;
> > > read += rng_size;
> > >
> > > if (wait && pvt_data->data_rate) {
> > > - if (timeout-- == 0)
> > > + if ((timeout-- == 0) || (read == max))
> >
> > If read == max, would there be any sleep?
>
> no but I see no reason why there should be a wait since we already have
> all the data that we need; the msleep is only required when we need to
> wait for the RNG to generate entropy for the number of bytes we are
> requesting. if we are requesting 0 bytes, the entropy is already
> available. at leat this is what makes sense to me.
>
>
any further comments?
> >
> > -Sumit
> >
> > > return read;
> > > msleep((1000 * (max - read)) /
> > > pvt_data->data_rate);
> > > } else {
> > > --
> > > 2.17.1
> > >
Re: [PATCHv2 2/2] hwrng: optee: fix wait use case
On 24/07/20, Sumit Garg wrote:
> On Thu, 23 Jul 2020 at 14:16, Jorge Ramirez-Ortiz wrote:
> >
> > The current code waits for data to be available before attempting a
> > second read. However the second read would not be executed as the
> > while loop exits.
> >
> > This fix does not wait if all data has been read and reads a second
> > time if only partial data was retrieved on the first read.
> >
> > This fix also does not attempt to read if not data is requested.
>
> I am not sure how this is possible, can you elaborate?
currently, if the user sets max 0, get_optee_rng_data will regardless
issuese a call to the secure world requesting 0 bytes from the RNG
with this patch, this request is avoided.
>
> >
> > Signed-off-by: Jorge Ramirez-Ortiz
> > ---
> > v2: tidy up the while loop to avoid reading when no data is requested
> >
> > drivers/char/hw_random/optee-rng.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/char/hw_random/optee-rng.c
> > b/drivers/char/hw_random/optee-rng.c
> > index 5bc4700c4dae..a99d82949981 100644
> > --- a/drivers/char/hw_random/optee-rng.c
> > +++ b/drivers/char/hw_random/optee-rng.c
> > @@ -122,14 +122,14 @@ static int optee_rng_read(struct hwrng *rng, void
> > *buf, size_t max, bool wait)
> > if (max > MAX_ENTROPY_REQ_SZ)
> > max = MAX_ENTROPY_REQ_SZ;
> >
> > - while (read == 0) {
> > + while (read < max) {
> > rng_size = get_optee_rng_data(pvt_data, data, (max - read));
> >
> > data += rng_size;
> > read += rng_size;
> >
> > if (wait && pvt_data->data_rate) {
> > - if (timeout-- == 0)
> > + if ((timeout-- == 0) || (read == max))
>
> If read == max, would there be any sleep?
no but I see no reason why there should be a wait since we already have
all the data that we need; the msleep is only required when we need to
wait for the RNG to generate entropy for the number of bytes we are
requesting. if we are requesting 0 bytes, the entropy is already
available. at leat this is what makes sense to me.
>
> -Sumit
>
> > return read;
> > msleep((1000 * (max - read)) / pvt_data->data_rate);
> > } else {
> > --
> > 2.17.1
> >
[PATCHv2 2/2] hwrng: optee: fix wait use case
The current code waits for data to be available before attempting a
second read. However the second read would not be executed as the
while loop exits.
This fix does not wait if all data has been read and reads a second
time if only partial data was retrieved on the first read.
This fix also does not attempt to read if not data is requested.
Signed-off-by: Jorge Ramirez-Ortiz
---
v2: tidy up the while loop to avoid reading when no data is requested
drivers/char/hw_random/optee-rng.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/char/hw_random/optee-rng.c
b/drivers/char/hw_random/optee-rng.c
index 5bc4700c4dae..a99d82949981 100644
--- a/drivers/char/hw_random/optee-rng.c
+++ b/drivers/char/hw_random/optee-rng.c
@@ -122,14 +122,14 @@ static int optee_rng_read(struct hwrng *rng, void *buf,
size_t max, bool wait)
if (max > MAX_ENTROPY_REQ_SZ)
max = MAX_ENTROPY_REQ_SZ;
- while (read == 0) {
+ while (read < max) {
rng_size = get_optee_rng_data(pvt_data, data, (max - read));
data += rng_size;
read += rng_size;
if (wait && pvt_data->data_rate) {
- if (timeout-- == 0)
+ if ((timeout-- == 0) || (read == max))
return read;
msleep((1000 * (max - read)) / pvt_data->data_rate);
} else {
--
2.17.1
[PATCHv2 1/2] hwrng: optee: handle unlimited data rates
Data rates of MAX_UINT32 will schedule an unnecessary one jiffy
timeout on the call to msleep. Avoid this scenario by using 0 as the
unlimited data rate.
Signed-off-by: Jorge Ramirez-Ortiz
---
drivers/char/hw_random/optee-rng.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/char/hw_random/optee-rng.c
b/drivers/char/hw_random/optee-rng.c
index 49b2e02537dd..5bc4700c4dae 100644
--- a/drivers/char/hw_random/optee-rng.c
+++ b/drivers/char/hw_random/optee-rng.c
@@ -128,7 +128,7 @@ static int optee_rng_read(struct hwrng *rng, void *buf,
size_t max, bool wait)
data += rng_size;
read += rng_size;
- if (wait) {
+ if (wait && pvt_data->data_rate) {
if (timeout-- == 0)
return read;
msleep((1000 * (max - read)) / pvt_data->data_rate);
--
2.17.1
[PATCHv6] drivers: optee: allow op-tee to access devices on the i2c bus
Some secure elements like NXP's SE050 sit on I2C buses. For OP-TEE to
control this type of cryptographic devices it needs coordinated access
to the bus, so collisions and RUNTIME_PM dont get in the way.
This trampoline driver allow OP-TEE to access them.
Tested on imx8mm LPDDR4
Signed-off-by: Jorge Ramirez-Ortiz
---
v6: compile out if CONFIG_I2C not enabled
v5: alphabetic order of includes
v4: remove unnecessary extra line in optee_msg.h
v3: use from/to msg param to support all types of memory
modify OPTEE_MSG_RPC_CMD_I2C_TRANSFER message id
drivers/tee/optee/optee_msg.h | 16 +++
drivers/tee/optee/rpc.c | 88 +++
2 files changed, 104 insertions(+)
diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
index 795bc19ae17a..14b580f55356 100644
--- a/drivers/tee/optee/optee_msg.h
+++ b/drivers/tee/optee/optee_msg.h
@@ -419,4 +419,20 @@ struct optee_msg_arg {
*/
#define OPTEE_MSG_RPC_CMD_SHM_FREE 7
+/*
+ * Access a device on an i2c bus
+ *
+ * [in] param[0].u.value.amode: RD(0), WR(1)
+ * [in] param[0].u.value.bi2c adapter
+ * [in] param[0].u.value.ci2c chip
+ *
+ * [in/out] memref[1] buffer to exchange the transfer data
+ * with the secure world
+ *
+ * [out] param[0].u.value.a bytes transferred by the driver
+ */
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 21
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD 0
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR 1
+
#endif /* _OPTEE_MSG_H */
diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
index b4ade54d1f28..5fd5c6c93896 100644
--- a/drivers/tee/optee/rpc.c
+++ b/drivers/tee/optee/rpc.c
@@ -7,6 +7,7 @@
#include
#include
+#include
#include
#include
#include "optee_private.h"
@@ -49,6 +50,90 @@ static void handle_rpc_func_cmd_get_time(struct
optee_msg_arg *arg)
arg->ret = TEEC_ERROR_BAD_PARAMETERS;
}
+#if IS_ENABLED(CONFIG_I2C)
+static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
+struct optee_msg_arg *arg)
+{
+ struct i2c_client client;
+ struct tee_param *params;
+ uint32_t type;
+ int i, ret;
+ size_t len;
+ char *buf;
+ uint32_t attr[] = {
+ TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
+ TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT,
+ TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT,
+ };
+
+ if (arg->num_params != ARRAY_SIZE(attr)) {
+ arg->ret = TEEC_ERROR_BAD_PARAMETERS;
+ return;
+ }
+
+ params = kmalloc_array(arg->num_params, sizeof(struct tee_param),
+ GFP_KERNEL);
+ if (!params) {
+ arg->ret = TEEC_ERROR_OUT_OF_MEMORY;
+ return;
+ }
+
+ if (optee_from_msg_param(params, arg->num_params, arg->params))
+ goto bad;
+
+ for (i = 0; i < arg->num_params; i++) {
+ type = params[i].attr & TEE_IOCTL_PARAM_ATTR_TYPE_MASK;
+ if (type != attr[i])
+ goto bad;
+ }
+
+ client.addr = params[0].u.value.c;
+ client.adapter = i2c_get_adapter(params[0].u.value.b);
+ if (!client.adapter)
+ goto bad;
+
+ snprintf(client.name, I2C_NAME_SIZE, "i2c%d", client.adapter->nr);
+
+ buf = params[1].u.memref.shm->kaddr;
+ len = params[1].u.memref.size;
+
+ switch (params[0].u.value.a) {
+ case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
+ ret = i2c_master_recv(&client, buf, len);
+ break;
+ case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR:
+ ret = i2c_master_send(&client, buf, len);
+ break;
+ default:
+ i2c_put_adapter(client.adapter);
+ goto bad;
+ }
+
+ if (ret >= 0) {
+ params[2].u.value.a = ret;
+ arg->ret = TEEC_SUCCESS;
+ } else {
+ arg->ret = TEEC_ERROR_COMMUNICATION;
+ }
+
+ if (optee_to_msg_param(arg->params, arg->num_params, params))
+ arg->ret = TEEC_ERROR_BAD_PARAMETERS;
+
+ i2c_put_adapter(client.adapter);
+ kfree(params);
+ return;
+bad:
+ kfree(params);
+ arg->ret = TEEC_ERROR_BAD_PARAMETERS;
+}
+#else
+static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
+struct optee_msg_arg *arg)
+{
+ arg->ret = TEEC_ERROR_COMMUNICATION;
+}
+#endif
+
static struct wq_entry *wq_entry_get(struct optee_wait_queue *wq, u32 key)
{
struct wq_entry *w;
@@ -382,6 +467,9 @@ static void handle_rpc_func_cmd(struct tee_context *ctx,
struct optee *optee,
case OPTEE_MSG_RPC_CMD_SHM_FREE:
handle_rpc_f
[PATCHv5] drivers: optee: allow op-tee to access devices on the i2c bus
Some secure elements like NXP's SE050 sit on I2C buses. For OP-TEE to
control this type of cryptographic devices it needs coordinated access
to the bus, so collisions and RUNTIME_PM dont get in the way.
This trampoline driver allow OP-TEE to access them.
Signed-off-by: Jorge Ramirez-Ortiz
---
v5: arrange includes alphabetically
v4: removed unecessary extra line in optee_msg.h
v3: use from/to msg param to support all types of memory
modify OPTEE_MSG_RPC_CMD_I2C_TRANSFER id
drivers/tee/optee/optee_msg.h | 16 +++
drivers/tee/optee/rpc.c | 80 +++
2 files changed, 96 insertions(+)
diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
index 795bc19ae17a..14b580f55356 100644
--- a/drivers/tee/optee/optee_msg.h
+++ b/drivers/tee/optee/optee_msg.h
@@ -419,4 +419,20 @@ struct optee_msg_arg {
*/
#define OPTEE_MSG_RPC_CMD_SHM_FREE 7
+/*
+ * Access a device on an i2c bus
+ *
+ * [in] param[0].u.value.amode: RD(0), WR(1)
+ * [in] param[0].u.value.bi2c adapter
+ * [in] param[0].u.value.ci2c chip
+ *
+ * [in/out] memref[1] buffer to exchange the transfer data
+ * with the secure world
+ *
+ * [out] param[0].u.value.a bytes transferred by the driver
+ */
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 21
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD 0
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR 1
+
#endif /* _OPTEE_MSG_H */
diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
index b4ade54d1f28..df6250418235 100644
--- a/drivers/tee/optee/rpc.c
+++ b/drivers/tee/optee/rpc.c
@@ -7,6 +7,7 @@
#include
#include
+#include
#include
#include
#include "optee_private.h"
@@ -49,6 +50,82 @@ static void handle_rpc_func_cmd_get_time(struct
optee_msg_arg *arg)
arg->ret = TEEC_ERROR_BAD_PARAMETERS;
}
+static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
+struct optee_msg_arg *arg)
+{
+ struct i2c_client client;
+ struct tee_param *params;
+ uint32_t type;
+ int i, ret;
+ size_t len;
+ char *buf;
+ uint32_t attr[] = {
+ TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
+ TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT,
+ TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT,
+ };
+
+ if (arg->num_params != ARRAY_SIZE(attr)) {
+ arg->ret = TEEC_ERROR_BAD_PARAMETERS;
+ return;
+ }
+
+ params = kmalloc_array(arg->num_params, sizeof(struct tee_param),
+ GFP_KERNEL);
+ if (!params) {
+ arg->ret = TEEC_ERROR_OUT_OF_MEMORY;
+ return;
+ }
+
+ if (optee_from_msg_param(params, arg->num_params, arg->params))
+ goto bad;
+
+ for (i = 0; i < arg->num_params; i++) {
+ type = params[i].attr & TEE_IOCTL_PARAM_ATTR_TYPE_MASK;
+ if (type != attr[i])
+ goto bad;
+ }
+
+ client.addr = params[0].u.value.c;
+ client.adapter = i2c_get_adapter(params[0].u.value.b);
+ if (!client.adapter)
+ goto bad;
+
+ snprintf(client.name, I2C_NAME_SIZE, "i2c%d", client.adapter->nr);
+
+ buf = params[1].u.memref.shm->kaddr;
+ len = params[1].u.memref.size;
+
+ switch (params[0].u.value.a) {
+ case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
+ ret = i2c_master_recv(&client, buf, len);
+ break;
+ case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR:
+ ret = i2c_master_send(&client, buf, len);
+ break;
+ default:
+ i2c_put_adapter(client.adapter);
+ goto bad;
+ }
+
+ if (ret >= 0) {
+ params[2].u.value.a = ret;
+ arg->ret = TEEC_SUCCESS;
+ } else {
+ arg->ret = TEEC_ERROR_COMMUNICATION;
+ }
+
+ if (optee_to_msg_param(arg->params, arg->num_params, params))
+ arg->ret = TEEC_ERROR_BAD_PARAMETERS;
+
+ i2c_put_adapter(client.adapter);
+ kfree(params);
+ return;
+bad:
+ kfree(params);
+ arg->ret = TEEC_ERROR_BAD_PARAMETERS;
+}
+
static struct wq_entry *wq_entry_get(struct optee_wait_queue *wq, u32 key)
{
struct wq_entry *w;
@@ -382,6 +459,9 @@ static void handle_rpc_func_cmd(struct tee_context *ctx,
struct optee *optee,
case OPTEE_MSG_RPC_CMD_SHM_FREE:
handle_rpc_func_cmd_shm_free(ctx, arg);
break;
+ case OPTEE_MSG_RPC_CMD_I2C_TRANSFER:
+ handle_rpc_func_cmd_i2c_transfer(ctx, arg);
+ break;
default:
handle_rpc_supp_cmd(ctx, arg);
}
--
2.17.1
[PATCHv4] drivers: optee: allow op-tee to access devices on the i2c bus
Some secure elements like NXP's SE050 sit on I2C buses. For OP-TEE to
control this type of cryptographic devices it needs coordinated access
to the bus, so collisions and RUNTIME_PM dont get in the way.
This trampoline driver allow OP-TEE to access them.
Signed-off-by: Jorge Ramirez-Ortiz
---
v4: removed unecessary extra line in optee_msg.h
v3: use from/to msg param to support all types of memory
modify OPTEE_MSG_RPC_CMD_I2C_TRANSFER id
drivers/tee/optee/optee_msg.h | 16 +++
drivers/tee/optee/rpc.c | 80 +++
2 files changed, 96 insertions(+)
diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
index 795bc19ae17a..14b580f55356 100644
--- a/drivers/tee/optee/optee_msg.h
+++ b/drivers/tee/optee/optee_msg.h
@@ -419,4 +419,20 @@ struct optee_msg_arg {
*/
#define OPTEE_MSG_RPC_CMD_SHM_FREE 7
+/*
+ * Access a device on an i2c bus
+ *
+ * [in] param[0].u.value.amode: RD(0), WR(1)
+ * [in] param[0].u.value.bi2c adapter
+ * [in] param[0].u.value.ci2c chip
+ *
+ * [in/out] memref[1] buffer to exchange the transfer data
+ * with the secure world
+ *
+ * [out] param[0].u.value.a bytes transferred by the driver
+ */
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 21
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD 0
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR 1
+
#endif /* _OPTEE_MSG_H */
diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
index b4ade54d1f28..2b2939c538f0 100644
--- a/drivers/tee/optee/rpc.c
+++ b/drivers/tee/optee/rpc.c
@@ -9,6 +9,7 @@
#include
#include
#include
+#include
#include "optee_private.h"
#include "optee_smc.h"
@@ -49,6 +50,82 @@ static void handle_rpc_func_cmd_get_time(struct
optee_msg_arg *arg)
arg->ret = TEEC_ERROR_BAD_PARAMETERS;
}
+static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
+struct optee_msg_arg *arg)
+{
+ struct i2c_client client;
+ struct tee_param *params;
+ uint32_t type;
+ int i, ret;
+ size_t len;
+ char *buf;
+ uint32_t attr[] = {
+ TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
+ TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT,
+ TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT,
+ };
+
+ if (arg->num_params != ARRAY_SIZE(attr)) {
+ arg->ret = TEEC_ERROR_BAD_PARAMETERS;
+ return;
+ }
+
+ params = kmalloc_array(arg->num_params, sizeof(struct tee_param),
+ GFP_KERNEL);
+ if (!params) {
+ arg->ret = TEEC_ERROR_OUT_OF_MEMORY;
+ return;
+ }
+
+ if (optee_from_msg_param(params, arg->num_params, arg->params))
+ goto bad;
+
+ for (i = 0; i < arg->num_params; i++) {
+ type = params[i].attr & TEE_IOCTL_PARAM_ATTR_TYPE_MASK;
+ if (type != attr[i])
+ goto bad;
+ }
+
+ client.addr = params[0].u.value.c;
+ client.adapter = i2c_get_adapter(params[0].u.value.b);
+ if (!client.adapter)
+ goto bad;
+
+ snprintf(client.name, I2C_NAME_SIZE, "i2c%d", client.adapter->nr);
+
+ buf = params[1].u.memref.shm->kaddr;
+ len = params[1].u.memref.size;
+
+ switch (params[0].u.value.a) {
+ case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
+ ret = i2c_master_recv(&client, buf, len);
+ break;
+ case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR:
+ ret = i2c_master_send(&client, buf, len);
+ break;
+ default:
+ i2c_put_adapter(client.adapter);
+ goto bad;
+ }
+
+ if (ret >= 0) {
+ params[2].u.value.a = ret;
+ arg->ret = TEEC_SUCCESS;
+ } else {
+ arg->ret = TEEC_ERROR_COMMUNICATION;
+ }
+
+ if (optee_to_msg_param(arg->params, arg->num_params, params))
+ arg->ret = TEEC_ERROR_BAD_PARAMETERS;
+
+ i2c_put_adapter(client.adapter);
+ kfree(params);
+ return;
+bad:
+ kfree(params);
+ arg->ret = TEEC_ERROR_BAD_PARAMETERS;
+}
+
static struct wq_entry *wq_entry_get(struct optee_wait_queue *wq, u32 key)
{
struct wq_entry *w;
@@ -382,6 +459,9 @@ static void handle_rpc_func_cmd(struct tee_context *ctx,
struct optee *optee,
case OPTEE_MSG_RPC_CMD_SHM_FREE:
handle_rpc_func_cmd_shm_free(ctx, arg);
break;
+ case OPTEE_MSG_RPC_CMD_I2C_TRANSFER:
+ handle_rpc_func_cmd_i2c_transfer(ctx, arg);
+ break;
default:
handle_rpc_supp_cmd(ctx, arg);
}
--
2.17.1
[PATCHv3] drivers: optee: allow op-tee to access devices on the i2c bus
Some secure elements like NXP's SE050 sit on I2C buses. For OP-TEE to
control this type of cryptographic devices it needs coordinated access
to the bus, so collisions and RUNTIME_PM dont get in the way.
This trampoline driver allow OP-TEE to access them.
Signed-off-by: Jorge Ramirez-Ortiz
---
v3: use from/to msg param to support all types of memory
modify OPTEE_MSG_RPC_CMD_I2C_TRANSFER id
drivers/tee/optee/optee_msg.h | 17
drivers/tee/optee/rpc.c | 80 +++
2 files changed, 97 insertions(+)
diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
index 795bc19ae17a..aa7cb19942b9 100644
--- a/drivers/tee/optee/optee_msg.h
+++ b/drivers/tee/optee/optee_msg.h
@@ -419,4 +419,21 @@ struct optee_msg_arg {
*/
#define OPTEE_MSG_RPC_CMD_SHM_FREE 7
+/*
+ * Access a device on an i2c bus
+ *
+ * [in] param[0].u.value.amode: RD(0), WR(1)
+ * [in] param[0].u.value.bi2c adapter
+ * [in] param[0].u.value.ci2c chip
+ *
+ * [in/out] memref[1] buffer to exchange the transfer data
+ * with the secure world
+ *
+ * [out] param[0].u.value.a bytes transferred by the driver
+ */
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 21
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD 0
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR 1
+
+
#endif /* _OPTEE_MSG_H */
diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
index b4ade54d1f28..2b2939c538f0 100644
--- a/drivers/tee/optee/rpc.c
+++ b/drivers/tee/optee/rpc.c
@@ -9,6 +9,7 @@
#include
#include
#include
+#include
#include "optee_private.h"
#include "optee_smc.h"
@@ -49,6 +50,82 @@ static void handle_rpc_func_cmd_get_time(struct
optee_msg_arg *arg)
arg->ret = TEEC_ERROR_BAD_PARAMETERS;
}
+static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
+struct optee_msg_arg *arg)
+{
+ struct i2c_client client;
+ struct tee_param *params;
+ uint32_t type;
+ int i, ret;
+ size_t len;
+ char *buf;
+ uint32_t attr[] = {
+ TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_INPUT,
+ TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT,
+ TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT,
+ };
+
+ if (arg->num_params != ARRAY_SIZE(attr)) {
+ arg->ret = TEEC_ERROR_BAD_PARAMETERS;
+ return;
+ }
+
+ params = kmalloc_array(arg->num_params, sizeof(struct tee_param),
+ GFP_KERNEL);
+ if (!params) {
+ arg->ret = TEEC_ERROR_OUT_OF_MEMORY;
+ return;
+ }
+
+ if (optee_from_msg_param(params, arg->num_params, arg->params))
+ goto bad;
+
+ for (i = 0; i < arg->num_params; i++) {
+ type = params[i].attr & TEE_IOCTL_PARAM_ATTR_TYPE_MASK;
+ if (type != attr[i])
+ goto bad;
+ }
+
+ client.addr = params[0].u.value.c;
+ client.adapter = i2c_get_adapter(params[0].u.value.b);
+ if (!client.adapter)
+ goto bad;
+
+ snprintf(client.name, I2C_NAME_SIZE, "i2c%d", client.adapter->nr);
+
+ buf = params[1].u.memref.shm->kaddr;
+ len = params[1].u.memref.size;
+
+ switch (params[0].u.value.a) {
+ case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
+ ret = i2c_master_recv(&client, buf, len);
+ break;
+ case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR:
+ ret = i2c_master_send(&client, buf, len);
+ break;
+ default:
+ i2c_put_adapter(client.adapter);
+ goto bad;
+ }
+
+ if (ret >= 0) {
+ params[2].u.value.a = ret;
+ arg->ret = TEEC_SUCCESS;
+ } else {
+ arg->ret = TEEC_ERROR_COMMUNICATION;
+ }
+
+ if (optee_to_msg_param(arg->params, arg->num_params, params))
+ arg->ret = TEEC_ERROR_BAD_PARAMETERS;
+
+ i2c_put_adapter(client.adapter);
+ kfree(params);
+ return;
+bad:
+ kfree(params);
+ arg->ret = TEEC_ERROR_BAD_PARAMETERS;
+}
+
static struct wq_entry *wq_entry_get(struct optee_wait_queue *wq, u32 key)
{
struct wq_entry *w;
@@ -382,6 +459,9 @@ static void handle_rpc_func_cmd(struct tee_context *ctx,
struct optee *optee,
case OPTEE_MSG_RPC_CMD_SHM_FREE:
handle_rpc_func_cmd_shm_free(ctx, arg);
break;
+ case OPTEE_MSG_RPC_CMD_I2C_TRANSFER:
+ handle_rpc_func_cmd_i2c_transfer(ctx, arg);
+ break;
default:
handle_rpc_supp_cmd(ctx, arg);
}
--
2.17.1
Re: [Tee-dev] [PATCH v2] drivers: optee: allow op-tee to access devices on the i2c bus
On 08/06/20, Jens Wiklander wrote:
> On Mon, Jun 01, 2020 at 09:24:46AM +0200, Jorge Ramirez-Ortiz, Foundries
> wrote:
> > On 01/06/20, Sumit Garg wrote:
> > > Hi Jorge,
> >
> > hey
> >
> > >
> > > On Mon, 1 Jun 2020 at 04:41, Jorge Ramirez-Ortiz
> > > wrote:
> > > >
> > > > Some secure elements like NXP's SE050 sit on I2C buses. For OP-TEE to
> > > > control this type of cryptographic devices it needs coordinated access
> > > > to the bus, so collisions and RUNTIME_PM dont get in the way.
> > > >
> > > > This trampoline driver allow OP-TEE to access them.
> > > >
> > >
> > > This sounds like an interesting use-case but I would like to
> > > understand how secure is this communication interface with the secure
> > > element? Like in the case of RPMB, secure world data is encrypted
> > > which flows via tee-supplicant to RPMB device.
> >
> > right, the data in the buffer should be encrypted in both directions
> > (in the case of the SE050 [1] we have the option to operate with or
> > without encryption which is what I am doing during development
> > [2]).
> >
> > But ultimately -before any product can be shipped- all comms must be
> > encrypted: this means that when OP-TEE uses the SE050 for crypto, it
> > must encrypt the data on write and decrypt what is comming from the
> > SE050 on read. I am now looking into how to enable this.
> >
> > [1] https://www.nxp.com/docs/en/data-sheet/SE050-DATASHEET.pdf
> > [2] https://github.com/ldts/optee_os/commits/se050
> This link doesn't work.
apologies, I updated the SDK release to 2.14 (latest)
please use the following:
https://github.com/ldts/optee_os/commits/se050.2.14
>
> >
> > >
> > > -Sumit
> > >
> > > > Signed-off-by: Jorge Ramirez-Ortiz
> > > > ---
> > > > drivers/tee/optee/optee_msg.h | 18 +++
> > > > drivers/tee/optee/rpc.c | 57 +++
> > > > 2 files changed, 75 insertions(+)
> > > >
> > > > diff --git a/drivers/tee/optee/optee_msg.h
> > > > b/drivers/tee/optee/optee_msg.h
> > > > index 795bc19ae17a..b6cc964fdeea 100644
> > > > --- a/drivers/tee/optee/optee_msg.h
> > > > +++ b/drivers/tee/optee/optee_msg.h
> > > > @@ -419,4 +419,22 @@ struct optee_msg_arg {
> > > > */
> > > > #define OPTEE_MSG_RPC_CMD_SHM_FREE 7
> > > >
> > > > +/*
> > > > + * Access a device on an i2c bus
> > > > + *
> > > > + * [in] param[0].u.value.amode: RD(0), WR(1)
> > > > + * [in] param[0].u.value.bi2c adapter
> > > > + * [in] param[0].u.value.ci2c chip
> > > > + *
> > > > + * [io] param[1].u.tmem.buf_ptr physical address
> > > > + * [io] param[1].u.tmem.size transfer size in bytes
> > > > + * [io] param[1].u.tmem.shm_ref shared memory reference
>
> This should be "[in/out] memref[1]" instead to be able to use
> all kinds of memory references.
>
> > > > + *
> > > > + * [out] param[0].u.value.a bytes transferred
> > > > + *
> > > > + */
> > > > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 8
> > > > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD 0
> > > > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR 1
> > > > +
> > > > #endif /* _OPTEE_MSG_H */
> > > > diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
> > > > index b4ade54d1f28..21d452805c6f 100644
> > > > --- a/drivers/tee/optee/rpc.c
> > > > +++ b/drivers/tee/optee/rpc.c
> > > > @@ -9,6 +9,7 @@
> > > > #include
> > > > #include
> > > > #include
> > > > +#include
> > > > #include "optee_private.h"
> > > > #include "optee_smc.h"
> > > >
> > > > @@ -48,6 +49,59 @@ static void handle_rpc_func_cmd_get_time(struct
> > > > optee_msg_arg *arg)
> > > > bad:
> > > > arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> > > > }
> > > > +static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
> > > > +struct optee_msg_arg *arg)
> > > > +{
> > > > + struct i2c_client client;
> > > > + struct
Re: [PATCH v2] drivers: optee: allow op-tee to access devices on the i2c bus
On 01/06/20, Jorge Ramirez-Ortiz wrote:
> Some secure elements like NXP's SE050 sit on I2C buses. For OP-TEE to
> control this type of cryptographic devices it needs coordinated access
> to the bus, so collisions and RUNTIME_PM dont get in the way.
>
> This trampoline driver allow OP-TEE to access them.
any comments please?
>
> Signed-off-by: Jorge Ramirez-Ortiz
> ---
> drivers/tee/optee/optee_msg.h | 18 +++
> drivers/tee/optee/rpc.c | 57 +++
> 2 files changed, 75 insertions(+)
>
> diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
> index 795bc19ae17a..b6cc964fdeea 100644
> --- a/drivers/tee/optee/optee_msg.h
> +++ b/drivers/tee/optee/optee_msg.h
> @@ -419,4 +419,22 @@ struct optee_msg_arg {
> */
> #define OPTEE_MSG_RPC_CMD_SHM_FREE 7
>
> +/*
> + * Access a device on an i2c bus
> + *
> + * [in] param[0].u.value.a mode: RD(0), WR(1)
> + * [in] param[0].u.value.b i2c adapter
> + * [in] param[0].u.value.c i2c chip
> + *
> + * [io] param[1].u.tmem.buf_ptr physical address
> + * [io] param[1].u.tmem.sizetransfer size in bytes
> + * [io] param[1].u.tmem.shm_ref shared memory reference
> + *
> + * [out] param[0].u.value.a bytes transferred
> + *
> + */
> +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 8
> +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD 0
> +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR 1
> +
> #endif /* _OPTEE_MSG_H */
> diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
> index b4ade54d1f28..21d452805c6f 100644
> --- a/drivers/tee/optee/rpc.c
> +++ b/drivers/tee/optee/rpc.c
> @@ -9,6 +9,7 @@
> #include
> #include
> #include
> +#include
> #include "optee_private.h"
> #include "optee_smc.h"
>
> @@ -48,6 +49,59 @@ static void handle_rpc_func_cmd_get_time(struct
> optee_msg_arg *arg)
> bad:
> arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> }
> +static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
> + struct optee_msg_arg *arg)
> +{
> + struct i2c_client client;
> + struct tee_shm *shm;
> + int i, ret;
> + char *buf;
> + uint32_t attr[] = {
> + OPTEE_MSG_ATTR_TYPE_VALUE_INPUT,
> + OPTEE_MSG_ATTR_TYPE_TMEM_INOUT,
> + OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT,
> + };
> +
> + if (arg->num_params != ARRAY_SIZE(attr))
> + goto bad;
> +
> + for (i = 0; i < ARRAY_SIZE(attr); i++)
> + if ((arg->params[i].attr & OPTEE_MSG_ATTR_TYPE_MASK) != attr[i])
> + goto bad;
> +
> + shm = (struct tee_shm *)(unsigned long)arg->params[1].u.tmem.shm_ref;
> + buf = (char *)shm->kaddr;
> +
> + client.addr = arg->params[0].u.value.c;
> + client.adapter = i2c_get_adapter(arg->params[0].u.value.b);
> + if (!client.adapter)
> + goto bad;
> +
> + snprintf(client.name, I2C_NAME_SIZE, "i2c%d", client.adapter->nr);
> +
> + switch (arg->params[0].u.value.a) {
> + case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
> + ret = i2c_master_recv(&client, buf, arg->params[1].u.tmem.size);
> + break;
> + case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR:
> + ret = i2c_master_send(&client, buf, arg->params[1].u.tmem.size);
> + break;
> + default:
> + i2c_put_adapter(client.adapter);
> + goto bad;
> + }
> +
> + if (ret >= 0) {
> + arg->params[2].u.value.a = ret;
> + arg->ret = TEEC_SUCCESS;
> + } else
> + arg->ret = TEEC_ERROR_COMMUNICATION;
> +
> + i2c_put_adapter(client.adapter);
> + return;
> +bad:
> + arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> +}
>
> static struct wq_entry *wq_entry_get(struct optee_wait_queue *wq, u32 key)
> {
> @@ -382,6 +436,9 @@ static void handle_rpc_func_cmd(struct tee_context *ctx,
> struct optee *optee,
> case OPTEE_MSG_RPC_CMD_SHM_FREE:
> handle_rpc_func_cmd_shm_free(ctx, arg);
> break;
> + case OPTEE_MSG_RPC_CMD_I2C_TRANSFER:
> + handle_rpc_func_cmd_i2c_transfer(ctx, arg);
> + break;
> default:
> handle_rpc_supp_cmd(ctx, arg);
> }
> --
> 2.17.1
>
Re: [Tee-dev] [PATCH v2] drivers: optee: allow op-tee to access devices on the i2c bus
On 01/06/20, Sumit Garg wrote:
> Hi Jorge,
hey
>
> On Mon, 1 Jun 2020 at 04:41, Jorge Ramirez-Ortiz wrote:
> >
> > Some secure elements like NXP's SE050 sit on I2C buses. For OP-TEE to
> > control this type of cryptographic devices it needs coordinated access
> > to the bus, so collisions and RUNTIME_PM dont get in the way.
> >
> > This trampoline driver allow OP-TEE to access them.
> >
>
> This sounds like an interesting use-case but I would like to
> understand how secure is this communication interface with the secure
> element? Like in the case of RPMB, secure world data is encrypted
> which flows via tee-supplicant to RPMB device.
right, the data in the buffer should be encrypted in both directions
(in the case of the SE050 [1] we have the option to operate with or
without encryption which is what I am doing during development
[2]).
But ultimately -before any product can be shipped- all comms must be
encrypted: this means that when OP-TEE uses the SE050 for crypto, it
must encrypt the data on write and decrypt what is comming from the
SE050 on read. I am now looking into how to enable this.
[1] https://www.nxp.com/docs/en/data-sheet/SE050-DATASHEET.pdf
[2] https://github.com/ldts/optee_os/commits/se050
>
> -Sumit
>
> > Signed-off-by: Jorge Ramirez-Ortiz
> > ---
> > drivers/tee/optee/optee_msg.h | 18 +++
> > drivers/tee/optee/rpc.c | 57 +++
> > 2 files changed, 75 insertions(+)
> >
> > diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
> > index 795bc19ae17a..b6cc964fdeea 100644
> > --- a/drivers/tee/optee/optee_msg.h
> > +++ b/drivers/tee/optee/optee_msg.h
> > @@ -419,4 +419,22 @@ struct optee_msg_arg {
> > */
> > #define OPTEE_MSG_RPC_CMD_SHM_FREE 7
> >
> > +/*
> > + * Access a device on an i2c bus
> > + *
> > + * [in] param[0].u.value.amode: RD(0), WR(1)
> > + * [in] param[0].u.value.bi2c adapter
> > + * [in] param[0].u.value.ci2c chip
> > + *
> > + * [io] param[1].u.tmem.buf_ptr physical address
> > + * [io] param[1].u.tmem.size transfer size in bytes
> > + * [io] param[1].u.tmem.shm_ref shared memory reference
> > + *
> > + * [out] param[0].u.value.a bytes transferred
> > + *
> > + */
> > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 8
> > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD 0
> > +#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR 1
> > +
> > #endif /* _OPTEE_MSG_H */
> > diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
> > index b4ade54d1f28..21d452805c6f 100644
> > --- a/drivers/tee/optee/rpc.c
> > +++ b/drivers/tee/optee/rpc.c
> > @@ -9,6 +9,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include "optee_private.h"
> > #include "optee_smc.h"
> >
> > @@ -48,6 +49,59 @@ static void handle_rpc_func_cmd_get_time(struct
> > optee_msg_arg *arg)
> > bad:
> > arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> > }
> > +static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
> > +struct optee_msg_arg *arg)
> > +{
> > + struct i2c_client client;
> > + struct tee_shm *shm;
> > + int i, ret;
> > + char *buf;
> > + uint32_t attr[] = {
> > + OPTEE_MSG_ATTR_TYPE_VALUE_INPUT,
> > + OPTEE_MSG_ATTR_TYPE_TMEM_INOUT,
> > + OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT,
> > + };
> > +
> > + if (arg->num_params != ARRAY_SIZE(attr))
> > + goto bad;
> > +
> > + for (i = 0; i < ARRAY_SIZE(attr); i++)
> > + if ((arg->params[i].attr & OPTEE_MSG_ATTR_TYPE_MASK) !=
> > attr[i])
> > + goto bad;
> > +
> > + shm = (struct tee_shm *)(unsigned
> > long)arg->params[1].u.tmem.shm_ref;
> > + buf = (char *)shm->kaddr;
> > +
> > + client.addr = arg->params[0].u.value.c;
> > + client.adapter = i2c_get_adapter(arg->params[0].u.value.b);
> > + if (!client.adapter)
> > + goto bad;
> > +
> > + snprintf(client.name, I2C_NAME_SIZE, "i2c%d", client.adapter->nr);
> > +
> > + switch (arg->params[0].u.value.a) {
> > + case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
> > + ret = i2c_master_recv(&client, buf,
> > arg->params[
[PATCH v2] drivers: optee: allow op-tee to access devices on the i2c bus
Some secure elements like NXP's SE050 sit on I2C buses. For OP-TEE to
control this type of cryptographic devices it needs coordinated access
to the bus, so collisions and RUNTIME_PM dont get in the way.
This trampoline driver allow OP-TEE to access them.
Signed-off-by: Jorge Ramirez-Ortiz
---
drivers/tee/optee/optee_msg.h | 18 +++
drivers/tee/optee/rpc.c | 57 +++
2 files changed, 75 insertions(+)
diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
index 795bc19ae17a..b6cc964fdeea 100644
--- a/drivers/tee/optee/optee_msg.h
+++ b/drivers/tee/optee/optee_msg.h
@@ -419,4 +419,22 @@ struct optee_msg_arg {
*/
#define OPTEE_MSG_RPC_CMD_SHM_FREE 7
+/*
+ * Access a device on an i2c bus
+ *
+ * [in] param[0].u.value.amode: RD(0), WR(1)
+ * [in] param[0].u.value.bi2c adapter
+ * [in] param[0].u.value.ci2c chip
+ *
+ * [io] param[1].u.tmem.buf_ptr physical address
+ * [io] param[1].u.tmem.size transfer size in bytes
+ * [io] param[1].u.tmem.shm_ref shared memory reference
+ *
+ * [out] param[0].u.value.a bytes transferred
+ *
+ */
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 8
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD 0
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR 1
+
#endif /* _OPTEE_MSG_H */
diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
index b4ade54d1f28..21d452805c6f 100644
--- a/drivers/tee/optee/rpc.c
+++ b/drivers/tee/optee/rpc.c
@@ -9,6 +9,7 @@
#include
#include
#include
+#include
#include "optee_private.h"
#include "optee_smc.h"
@@ -48,6 +49,59 @@ static void handle_rpc_func_cmd_get_time(struct
optee_msg_arg *arg)
bad:
arg->ret = TEEC_ERROR_BAD_PARAMETERS;
}
+static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
+struct optee_msg_arg *arg)
+{
+ struct i2c_client client;
+ struct tee_shm *shm;
+ int i, ret;
+ char *buf;
+ uint32_t attr[] = {
+ OPTEE_MSG_ATTR_TYPE_VALUE_INPUT,
+ OPTEE_MSG_ATTR_TYPE_TMEM_INOUT,
+ OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT,
+ };
+
+ if (arg->num_params != ARRAY_SIZE(attr))
+ goto bad;
+
+ for (i = 0; i < ARRAY_SIZE(attr); i++)
+ if ((arg->params[i].attr & OPTEE_MSG_ATTR_TYPE_MASK) != attr[i])
+ goto bad;
+
+ shm = (struct tee_shm *)(unsigned long)arg->params[1].u.tmem.shm_ref;
+ buf = (char *)shm->kaddr;
+
+ client.addr = arg->params[0].u.value.c;
+ client.adapter = i2c_get_adapter(arg->params[0].u.value.b);
+ if (!client.adapter)
+ goto bad;
+
+ snprintf(client.name, I2C_NAME_SIZE, "i2c%d", client.adapter->nr);
+
+ switch (arg->params[0].u.value.a) {
+ case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
+ ret = i2c_master_recv(&client, buf, arg->params[1].u.tmem.size);
+ break;
+ case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR:
+ ret = i2c_master_send(&client, buf, arg->params[1].u.tmem.size);
+ break;
+ default:
+ i2c_put_adapter(client.adapter);
+ goto bad;
+ }
+
+ if (ret >= 0) {
+ arg->params[2].u.value.a = ret;
+ arg->ret = TEEC_SUCCESS;
+ } else
+ arg->ret = TEEC_ERROR_COMMUNICATION;
+
+ i2c_put_adapter(client.adapter);
+ return;
+bad:
+ arg->ret = TEEC_ERROR_BAD_PARAMETERS;
+}
static struct wq_entry *wq_entry_get(struct optee_wait_queue *wq, u32 key)
{
@@ -382,6 +436,9 @@ static void handle_rpc_func_cmd(struct tee_context *ctx,
struct optee *optee,
case OPTEE_MSG_RPC_CMD_SHM_FREE:
handle_rpc_func_cmd_shm_free(ctx, arg);
break;
+ case OPTEE_MSG_RPC_CMD_I2C_TRANSFER:
+ handle_rpc_func_cmd_i2c_transfer(ctx, arg);
+ break;
default:
handle_rpc_supp_cmd(ctx, arg);
}
--
2.17.1
[PATCH] drivers: optee: allow op-tee to access devices on the i2c bus
Some secure elements like NXP's SE050 sit on I2C buses. For OP-TEE to
control this type of cryptographic devices it needs coordinated access
to the bus, so collisions and RUNTIME_PM dont get in the way.
This trampoline driver allow OP-TEE to access them.
Signed-off-by: Jorge Ramirez-Ortiz
---
drivers/tee/optee/optee_msg.h | 18 +++
drivers/tee/optee/rpc.c | 56 +++
2 files changed, 74 insertions(+)
diff --git a/drivers/tee/optee/optee_msg.h b/drivers/tee/optee/optee_msg.h
index 795bc19ae17a..b6cc964fdeea 100644
--- a/drivers/tee/optee/optee_msg.h
+++ b/drivers/tee/optee/optee_msg.h
@@ -419,4 +419,22 @@ struct optee_msg_arg {
*/
#define OPTEE_MSG_RPC_CMD_SHM_FREE 7
+/*
+ * Access a device on an i2c bus
+ *
+ * [in] param[0].u.value.amode: RD(0), WR(1)
+ * [in] param[0].u.value.bi2c adapter
+ * [in] param[0].u.value.ci2c chip
+ *
+ * [io] param[1].u.tmem.buf_ptr physical address
+ * [io] param[1].u.tmem.size transfer size in bytes
+ * [io] param[1].u.tmem.shm_ref shared memory reference
+ *
+ * [out] param[0].u.value.a bytes transferred
+ *
+ */
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER 8
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD 0
+#define OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR 1
+
#endif /* _OPTEE_MSG_H */
diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
index b4ade54d1f28..6c8457ac2524 100644
--- a/drivers/tee/optee/rpc.c
+++ b/drivers/tee/optee/rpc.c
@@ -9,6 +9,7 @@
#include
#include
#include
+#include
#include "optee_private.h"
#include "optee_smc.h"
@@ -48,6 +49,58 @@ static void handle_rpc_func_cmd_get_time(struct
optee_msg_arg *arg)
bad:
arg->ret = TEEC_ERROR_BAD_PARAMETERS;
}
+static void handle_rpc_func_cmd_i2c_transfer(struct tee_context *ctx,
+struct optee_msg_arg *arg)
+{
+ struct i2c_client client;
+ struct tee_shm *shm;
+ int i, ret;
+ char *buf;
+ uint32_t attr[] = {
+ OPTEE_MSG_ATTR_TYPE_VALUE_INPUT,
+ OPTEE_MSG_ATTR_TYPE_TMEM_INOUT,
+ OPTEE_MSG_ATTR_TYPE_VALUE_OUTPUT,
+ };
+
+ if (arg->num_params != ARRAY_SIZE(attr))
+ goto bad;
+
+ for (i = 0; i < ARRAY_SIZE(attr); i++)
+ if ((arg->params[i].attr & OPTEE_MSG_ATTR_TYPE_MASK) != attr[i])
+ goto bad;
+
+ shm = (struct tee_shm *)(unsigned long)arg->params[1].u.tmem.shm_ref;
+ buf = (char *)shm->kaddr;
+
+ client.addr = arg->params[0].u.value.c;
+ client.adapter = i2c_get_adapter(arg->params[0].u.value.b);
+ if (!client.adapter)
+ goto bad;
+
+ snprintf(client.name, I2C_NAME_SIZE, "i2c%d", client.adapter->nr);
+
+ switch (arg->params[0].u.value.a) {
+ case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_RD:
+ ret = i2c_master_recv(&client, buf, arg->params[1].u.tmem.size);
+ break;
+ case OPTEE_MSG_RPC_CMD_I2C_TRANSFER_WR:
+ ret = i2c_master_send(&client, buf, arg->params[1].u.tmem.size);
+ break;
+ default:
+ goto bad;
+ }
+
+ if (ret >= 0) {
+ arg->params[2].u.value.a = ret;
+ arg->ret = TEEC_SUCCESS;
+ } else
+ arg->ret = TEEC_ERROR_COMMUNICATION;
+
+ i2c_put_adapter(client.adapter);
+ return;
+bad:
+ arg->ret = TEEC_ERROR_BAD_PARAMETERS;
+}
static struct wq_entry *wq_entry_get(struct optee_wait_queue *wq, u32 key)
{
@@ -382,6 +435,9 @@ static void handle_rpc_func_cmd(struct tee_context *ctx,
struct optee *optee,
case OPTEE_MSG_RPC_CMD_SHM_FREE:
handle_rpc_func_cmd_shm_free(ctx, arg);
break;
+ case OPTEE_MSG_RPC_CMD_I2C_TRANSFER:
+ handle_rpc_func_cmd_i2c_transfer(ctx, arg);
+ break;
default:
handle_rpc_supp_cmd(ctx, arg);
}
--
2.17.1
[PATCH v2 1/5] misc: fastrpc: add mmap/unmap support
Support the allocation/deallocation of buffers mapped to the DSP.
When the memory mapped to the DSP at process creation is not enough,
the fastrpc library can extend it at runtime. This avoids having to do
large preallocations by default.
Signed-off-by: Jorge Ramirez-Ortiz
Reviewed-by: Srinivas Kandagatla
---
v2: fixes kbuild warning
cast from pointer to integer of different size
drivers/misc/fastrpc.c | 181
include/uapi/misc/fastrpc.h | 15 +++
2 files changed, 196 insertions(+)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 98603e235cf0..bacf6c20988d 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -35,6 +35,7 @@
#define INIT_FILELEN_MAX (64 * 1024 * 1024)
#define INIT_MEMLEN_MAX (8 * 1024 * 1024)
#define FASTRPC_DEVICE_NAME"fastrpc"
+#define ADSP_MMAP_ADD_PAGES 0x1000
/* Retrives number of input buffers from the scalars parameter */
#define REMOTE_SCALARS_INBUFS(sc) (((sc) >> 16) & 0x0ff)
@@ -67,6 +68,8 @@
/* Remote Method id table */
#define FASTRPC_RMID_INIT_ATTACH 0
#define FASTRPC_RMID_INIT_RELEASE 1
+#define FASTRPC_RMID_INIT_MMAP 4
+#define FASTRPC_RMID_INIT_MUNMAP 5
#define FASTRPC_RMID_INIT_CREATE 6
#define FASTRPC_RMID_INIT_CREATE_ATTR 7
#define FASTRPC_RMID_INIT_CREATE_STATIC8
@@ -90,6 +93,23 @@ struct fastrpc_remote_arg {
u64 len;
};
+struct fastrpc_mmap_rsp_msg {
+ u64 vaddr;
+};
+
+struct fastrpc_mmap_req_msg {
+ s32 pgid;
+ u32 flags;
+ u64 vaddr;
+ s32 num;
+};
+
+struct fastrpc_munmap_req_msg {
+ s32 pgid;
+ u64 vaddr;
+ u64 size;
+};
+
struct fastrpc_msg {
int pid;/* process group id */
int tid;/* thread id */
@@ -124,6 +144,9 @@ struct fastrpc_buf {
/* Lock for dma buf attachments */
struct mutex lock;
struct list_head attachments;
+ /* mmap support */
+ struct list_head node; /* list of user requested mmaps */
+ uintptr_t raddr;
};
struct fastrpc_dma_buf_attachment {
@@ -192,6 +215,7 @@ struct fastrpc_user {
struct list_head user;
struct list_head maps;
struct list_head pending;
+ struct list_head mmaps;
struct fastrpc_channel_ctx *cctx;
struct fastrpc_session_ctx *sctx;
@@ -269,6 +293,7 @@ static int fastrpc_buf_alloc(struct fastrpc_user *fl,
struct device *dev,
return -ENOMEM;
INIT_LIST_HEAD(&buf->attachments);
+ INIT_LIST_HEAD(&buf->node);
mutex_init(&buf->lock);
buf->fl = fl;
@@ -276,6 +301,7 @@ static int fastrpc_buf_alloc(struct fastrpc_user *fl,
struct device *dev,
buf->phys = 0;
buf->size = size;
buf->dev = dev;
+ buf->raddr = 0;
buf->virt = dma_alloc_coherent(dev, buf->size, (dma_addr_t *)&buf->phys,
GFP_KERNEL);
@@ -1098,6 +1124,7 @@ static int fastrpc_device_release(struct inode *inode,
struct file *file)
struct fastrpc_channel_ctx *cctx = fl->cctx;
struct fastrpc_invoke_ctx *ctx, *n;
struct fastrpc_map *map, *m;
+ struct fastrpc_buf *buf, *b;
unsigned long flags;
fastrpc_release_current_dsp_process(fl);
@@ -1119,6 +1146,11 @@ static int fastrpc_device_release(struct inode *inode,
struct file *file)
fastrpc_map_put(map);
}
+ list_for_each_entry_safe(buf, b, &fl->mmaps, node) {
+ list_del(&buf->node);
+ fastrpc_buf_free(buf);
+ }
+
fastrpc_session_free(cctx, fl->sctx);
mutex_destroy(&fl->mutex);
@@ -1143,6 +1175,7 @@ static int fastrpc_device_open(struct inode *inode,
struct file *filp)
mutex_init(&fl->mutex);
INIT_LIST_HEAD(&fl->pending);
INIT_LIST_HEAD(&fl->maps);
+ INIT_LIST_HEAD(&fl->mmaps);
INIT_LIST_HEAD(&fl->user);
fl->tgid = current->tgid;
fl->cctx = cctx;
@@ -1270,6 +1303,148 @@ static int fastrpc_invoke(struct fastrpc_user *fl, char
__user *argp)
return err;
}
+static int fastrpc_req_munmap_impl(struct fastrpc_user *fl,
+ struct fastrpc_req_munmap *req)
+{
+ struct fastrpc_invoke_args args[1] = { [0] = { 0 } };
+ struct fastrpc_buf *buf, *b;
+ struct fastrpc_munmap_req_msg req_msg;
+ struct device *dev = fl->sctx->dev;
+ int err;
+ u32 sc;
+
+ spin_lock(&fl->lock);
+ list_for_each_entry_safe(buf, b, &fl->mmaps, node) {
+ if ((buf->raddr == req->vaddrout) && (buf->size == req->size))
+ break;
+ buf = NULL;
+ }
+ spin_unlock(&fl->lock);
+
+ if (!buf)
[PATCH 5/5] misc: fastrpc: revert max init file size back to 2MB
With the integration of the mmap/unmap functionality, it is no longer necessary to allow large memory allocations upfront since they can be handled during runtime. Tested on QCS404 with CDSP Neural Processing test suite. Signed-off-by: Jorge Ramirez-Ortiz Reviewed-by: Srinivas Kandagatla --- drivers/misc/fastrpc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c index 40b48db032b5..ee6de5d9993d 100644 --- a/drivers/misc/fastrpc.c +++ b/drivers/misc/fastrpc.c @@ -32,7 +32,7 @@ #define FASTRPC_CTX_MAX (256) #define FASTRPC_INIT_HANDLE1 #define FASTRPC_CTXID_MASK (0xFF0) -#define INIT_FILELEN_MAX (64 * 1024 * 1024) +#define INIT_FILELEN_MAX (2 * 1024 * 1024) #define INIT_MEMLEN_MAX (8 * 1024 * 1024) #define FASTRPC_DEVICE_NAME"fastrpc" #define ADSP_MMAP_ADD_PAGES 0x1000 -- 2.23.0
[PATCH 0/5] misc: fastrpc: fixes and map/unmap support
Hi Greg These patches implement a few fixes identified while working on the QCS404 ML integration plus we now have support for mmap/unmap of buffers (so the process can be created with less initial memory requirements). Jorge Ramirez-Ortiz (4): misc: fastrpc: add mmap/unmap support misc: fastrpc: do not interrupt kernel calls misc: fastrpc: handle interrupted contexts misc: fastrpc: revert max init file size back to 2MB Srinivas Kandagatla (1): misc: fastrpc: fix memory leak from miscdev->name drivers/misc/fastrpc.c | 209 ++-- include/uapi/misc/fastrpc.h | 15 +++ 2 files changed, 213 insertions(+), 11 deletions(-) -- 2.23.0
[PATCH 3/5] misc: fastrpc: do not interrupt kernel calls
the DSP firmware requires some calls to be held until processing has
completed: this is to guarantee that memory continues to be
accessible.
Nevertheless, the fastrpc driver chooses not support the case were
requests need to be held for unbounded amounts of time. If such a
use-case becomes necessary, this timeout will need to be revisited.
Signed-off-by: Jorge Ramirez-Ortiz
Reviewed-by: Srinivas Kandagatla
---
drivers/misc/fastrpc.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index bc03500bfe60..d2b639dfc461 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -927,8 +927,13 @@ static int fastrpc_internal_invoke(struct fastrpc_user
*fl, u32 kernel,
if (err)
goto bail;
- /* Wait for remote dsp to respond or time out */
- err = wait_for_completion_interruptible(&ctx->work);
+ if (kernel) {
+ if (!wait_for_completion_timeout(&ctx->work, 10 * HZ))
+ err = -ETIMEDOUT;
+ } else {
+ err = wait_for_completion_interruptible(&ctx->work);
+ }
+
if (err)
goto bail;
--
2.23.0
[PATCH 4/5] misc: fastrpc: handle interrupted contexts
Buffers owned by a context that has been interrupted either by a
signal or a timeout might still be being accessed by the DSP.
delegate returning the associated memory to a later time when the
device is released.
Signed-off-by: Jorge Ramirez-Ortiz
Reviewed-by: Srinivas Kandagatla
---
drivers/misc/fastrpc.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index d2b639dfc461..40b48db032b5 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -952,12 +952,13 @@ static int fastrpc_internal_invoke(struct fastrpc_user
*fl, u32 kernel,
}
bail:
- /* We are done with this compute context, remove it from pending list */
- spin_lock(&fl->lock);
- list_del(&ctx->node);
- spin_unlock(&fl->lock);
- fastrpc_context_put(ctx);
-
+ if (err != -ERESTARTSYS && err != -ETIMEDOUT) {
+ /* We are done with this compute context */
+ spin_lock(&fl->lock);
+ list_del(&ctx->node);
+ spin_unlock(&fl->lock);
+ fastrpc_context_put(ctx);
+ }
if (err)
dev_dbg(fl->sctx->dev, "Error: Invoke Failed %d\n", err);
--
2.23.0
[PATCH 1/5] misc: fastrpc: add mmap/unmap support
Support the allocation/deallocation of buffers mapped to the DSP.
When the memory mapped to the DSP at process creation is not enough,
the fastrpc library can extend it at runtime. This avoids having to do
large preallocations by default.
Signed-off-by: Jorge Ramirez-Ortiz
Reviewed-by: Srinivas Kandagatla
---
drivers/misc/fastrpc.c | 181
include/uapi/misc/fastrpc.h | 15 +++
2 files changed, 196 insertions(+)
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 98603e235cf0..8903388993d3 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -35,6 +35,7 @@
#define INIT_FILELEN_MAX (64 * 1024 * 1024)
#define INIT_MEMLEN_MAX (8 * 1024 * 1024)
#define FASTRPC_DEVICE_NAME"fastrpc"
+#define ADSP_MMAP_ADD_PAGES 0x1000
/* Retrives number of input buffers from the scalars parameter */
#define REMOTE_SCALARS_INBUFS(sc) (((sc) >> 16) & 0x0ff)
@@ -67,6 +68,8 @@
/* Remote Method id table */
#define FASTRPC_RMID_INIT_ATTACH 0
#define FASTRPC_RMID_INIT_RELEASE 1
+#define FASTRPC_RMID_INIT_MMAP 4
+#define FASTRPC_RMID_INIT_MUNMAP 5
#define FASTRPC_RMID_INIT_CREATE 6
#define FASTRPC_RMID_INIT_CREATE_ATTR 7
#define FASTRPC_RMID_INIT_CREATE_STATIC8
@@ -90,6 +93,23 @@ struct fastrpc_remote_arg {
u64 len;
};
+struct fastrpc_mmap_rsp_msg {
+ u64 vaddr;
+};
+
+struct fastrpc_mmap_req_msg {
+ s32 pgid;
+ u32 flags;
+ u64 vaddr;
+ s32 num;
+};
+
+struct fastrpc_munmap_req_msg {
+ s32 pgid;
+ u64 vaddr;
+ u64 size;
+};
+
struct fastrpc_msg {
int pid;/* process group id */
int tid;/* thread id */
@@ -124,6 +144,9 @@ struct fastrpc_buf {
/* Lock for dma buf attachments */
struct mutex lock;
struct list_head attachments;
+ /* mmap support */
+ struct list_head node; /* list of user requested mmaps */
+ uintptr_t raddr;
};
struct fastrpc_dma_buf_attachment {
@@ -192,6 +215,7 @@ struct fastrpc_user {
struct list_head user;
struct list_head maps;
struct list_head pending;
+ struct list_head mmaps;
struct fastrpc_channel_ctx *cctx;
struct fastrpc_session_ctx *sctx;
@@ -269,6 +293,7 @@ static int fastrpc_buf_alloc(struct fastrpc_user *fl,
struct device *dev,
return -ENOMEM;
INIT_LIST_HEAD(&buf->attachments);
+ INIT_LIST_HEAD(&buf->node);
mutex_init(&buf->lock);
buf->fl = fl;
@@ -276,6 +301,7 @@ static int fastrpc_buf_alloc(struct fastrpc_user *fl,
struct device *dev,
buf->phys = 0;
buf->size = size;
buf->dev = dev;
+ buf->raddr = 0;
buf->virt = dma_alloc_coherent(dev, buf->size, (dma_addr_t *)&buf->phys,
GFP_KERNEL);
@@ -1098,6 +1124,7 @@ static int fastrpc_device_release(struct inode *inode,
struct file *file)
struct fastrpc_channel_ctx *cctx = fl->cctx;
struct fastrpc_invoke_ctx *ctx, *n;
struct fastrpc_map *map, *m;
+ struct fastrpc_buf *buf, *b;
unsigned long flags;
fastrpc_release_current_dsp_process(fl);
@@ -1119,6 +1146,11 @@ static int fastrpc_device_release(struct inode *inode,
struct file *file)
fastrpc_map_put(map);
}
+ list_for_each_entry_safe(buf, b, &fl->mmaps, node) {
+ list_del(&buf->node);
+ fastrpc_buf_free(buf);
+ }
+
fastrpc_session_free(cctx, fl->sctx);
mutex_destroy(&fl->mutex);
@@ -1143,6 +1175,7 @@ static int fastrpc_device_open(struct inode *inode,
struct file *filp)
mutex_init(&fl->mutex);
INIT_LIST_HEAD(&fl->pending);
INIT_LIST_HEAD(&fl->maps);
+ INIT_LIST_HEAD(&fl->mmaps);
INIT_LIST_HEAD(&fl->user);
fl->tgid = current->tgid;
fl->cctx = cctx;
@@ -1270,6 +1303,148 @@ static int fastrpc_invoke(struct fastrpc_user *fl, char
__user *argp)
return err;
}
+static int fastrpc_req_munmap_impl(struct fastrpc_user *fl,
+ struct fastrpc_req_munmap *req)
+{
+ struct fastrpc_invoke_args args[1] = { [0] = { 0 } };
+ struct fastrpc_buf *buf, *b;
+ struct fastrpc_munmap_req_msg req_msg;
+ struct device *dev = fl->sctx->dev;
+ int err;
+ u32 sc;
+
+ spin_lock(&fl->lock);
+ list_for_each_entry_safe(buf, b, &fl->mmaps, node) {
+ if ((buf->raddr == req->vaddrout) && (buf->size == req->size))
+ break;
+ buf = NULL;
+ }
+ spin_unlock(&fl->lock);
+
+ if (!buf) {
+ dev_err(dev, "mmap not in list\n");
+
[PATCH 2/5] misc: fastrpc: fix memory leak from miscdev->name
From: Srinivas Kandagatla Fix a memory leak in miscdev->name by using devm_variant Orignally reported by kmemleak: [] kmemleak_alloc+0x50/0x84 [] __kmalloc_track_caller+0xe8/0x168 [] kvasprintf+0x78/0x100 [] kasprintf+0x50/0x74 [] fastrpc_rpmsg_probe+0xd8/0x20c [] rpmsg_dev_probe+0xa8/0x148 [] really_probe+0x208/0x248 [] driver_probe_device+0x98/0xc0 [] __device_attach_driver+0x9c/0xac [] bus_for_each_drv+0x60/0x8c [] __device_attach+0x8c/0x100 [] device_initial_probe+0x20/0x28 [] bus_probe_device+0x34/0x7c [] device_add+0x420/0x498 [] device_register+0x24/0x2c Signed-off-by: Srinivas Kandagatla --- drivers/misc/fastrpc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c index 8903388993d3..bc03500bfe60 100644 --- a/drivers/misc/fastrpc.c +++ b/drivers/misc/fastrpc.c @@ -1599,8 +1599,8 @@ static int fastrpc_rpmsg_probe(struct rpmsg_device *rpdev) } data->miscdev.minor = MISC_DYNAMIC_MINOR; - data->miscdev.name = kasprintf(GFP_KERNEL, "fastrpc-%s", - domains[domain_id]); + data->miscdev.name = devm_kasprintf(rdev, GFP_KERNEL, "fastrpc-%s", + domains[domain_id]); data->miscdev.fops = &fastrpc_fops; err = misc_register(&data->miscdev); if (err) -- 2.23.0
[PATCH v2 2/5] clk: qcom: hfpll: register as clock provider
Make the output of the high frequency pll a clock provider.
On the QCS404 this PLL controls cpu frequency scaling.
Co-developed-by: Niklas Cassel
Signed-off-by: Niklas Cassel
Signed-off-by: Jorge Ramirez-Ortiz
Reviewed-by: Bjorn Andersson
Acked-by: Stephen Boyd
---
drivers/clk/qcom/hfpll.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/qcom/hfpll.c b/drivers/clk/qcom/hfpll.c
index a6de7101430c..e64c0fd82fe4 100644
--- a/drivers/clk/qcom/hfpll.c
+++ b/drivers/clk/qcom/hfpll.c
@@ -57,6 +57,7 @@ static int qcom_hfpll_probe(struct platform_device *pdev)
.num_parents = 1,
.ops = &clk_ops_hfpll,
};
+ int ret;
h = devm_kzalloc(dev, sizeof(*h), GFP_KERNEL);
if (!h)
@@ -79,7 +80,14 @@ static int qcom_hfpll_probe(struct platform_device *pdev)
h->clkr.hw.init = &init;
spin_lock_init(&h->lock);
- return devm_clk_register_regmap(&pdev->dev, &h->clkr);
+ ret = devm_clk_register_regmap(dev, &h->clkr);
+ if (ret) {
+ dev_err(dev, "failed to register regmap clock: %d\n", ret);
+ return ret;
+ }
+
+ return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get,
+ &h->clkr.hw);
}
static struct platform_driver qcom_hfpll_driver = {
--
2.23.0
[PATCH v2 3/5] clk: qcom: hfpll: CLK_IGNORE_UNUSED
When COMMON_CLK_DISABLED_UNUSED is set, in an effort to save power and
to keep the software model of the clock in line with reality, the
framework transverses the clock tree and disables those clocks that
were enabled by the firmware but have not been enabled by any device
driver.
If CPUFREQ is enabled, early during the system boot, it might attempt
to change the CPU frequency ("set_rate"). If the HFPLL is selected as
a provider, it will then change the rate for this clock.
As boot continues, clk_disable_unused_subtree will run. Since it wont
find a valid counter (enable_count) for a clock that is actually
enabled it will attempt to disable it which will cause the CPU to
stop. Notice that in this driver, calls to check whether the clock is
enabled are routed via the is_enabled callback which queries the
hardware.
The following commit, rather than marking the clock critical and
forcing the clock to be always enabled, addresses the above scenario
making sure the clock is not disabled but it continues to rely on the
firmware to enable the clock.
Co-developed-by: Niklas Cassel
Signed-off-by: Niklas Cassel
Signed-off-by: Jorge Ramirez-Ortiz
Reviewed-by: Bjorn Andersson
---
drivers/clk/qcom/hfpll.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/clk/qcom/hfpll.c b/drivers/clk/qcom/hfpll.c
index e64c0fd82fe4..225c675f6779 100644
--- a/drivers/clk/qcom/hfpll.c
+++ b/drivers/clk/qcom/hfpll.c
@@ -56,6 +56,13 @@ static int qcom_hfpll_probe(struct platform_device *pdev)
.parent_names = (const char *[]){ "xo" },
.num_parents = 1,
.ops = &clk_ops_hfpll,
+ /*
+* rather than marking the clock critical and forcing the clock
+* to be always enabled, we make sure that the clock is not
+* disabled: the firmware remains responsible of enabling this
+* clock (for more info check the commit log)
+*/
+ .flags = CLK_IGNORE_UNUSED,
};
int ret;
--
2.23.0
[PATCH v2 0/5] Clock changes to support cpufreq on QCS404
The following clock changes are required to enable cpufreq support on the QCS404 v2: sboyd review of v1 --- missing cover letter reorder the patchset use clk_parent data to speficy the parent clock dong ignore the clock position abi Jorge Ramirez-Ortiz (5): clk: qcom: gcc: limit GPLL0_AO_OUT operating frequency clk: qcom: hfpll: register as clock provider clk: qcom: hfpll: CLK_IGNORE_UNUSED clk: qcom: hfpll: use clk_parent_data to specify the parent clk: qcom: apcs-msm8916: get parent clock names from DT drivers/clk/qcom/apcs-msm8916.c | 15 --- drivers/clk/qcom/clk-alpha-pll.c | 8 drivers/clk/qcom/clk-alpha-pll.h | 1 + drivers/clk/qcom/gcc-qcs404.c| 2 +- drivers/clk/qcom/hfpll.c | 21 +++-- 5 files changed, 41 insertions(+), 6 deletions(-) -- 2.23.0
[PATCH v2 5/5] clk: qcom: apcs-msm8916: get parent clock names from DT
Allow accessing the parent clock names required for the driver
operation by using the device tree node.
This permits extending the driver to other platforms without having to
modify its source code.
For backwards compatibility leave previous values as default.
Co-developed-by: Niklas Cassel
Signed-off-by: Niklas Cassel
Signed-off-by: Jorge Ramirez-Ortiz
Reviewed-by: Bjorn Andersson
---
drivers/clk/qcom/apcs-msm8916.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/clk/qcom/apcs-msm8916.c b/drivers/clk/qcom/apcs-msm8916.c
index a6c89a310b18..099b028dbc20 100644
--- a/drivers/clk/qcom/apcs-msm8916.c
+++ b/drivers/clk/qcom/apcs-msm8916.c
@@ -19,7 +19,7 @@
static const u32 gpll0_a53cc_map[] = { 4, 5 };
-static const char * const gpll0_a53cc[] = {
+static const char *gpll0_a53cc[] = {
"gpll0_vote",
"a53pll",
};
@@ -50,6 +50,7 @@ static int qcom_apcs_msm8916_clk_probe(struct platform_device
*pdev)
struct regmap *regmap;
struct clk_init_data init = { };
int ret = -ENODEV;
+ const char *parents[2];
regmap = dev_get_regmap(parent, NULL);
if (!regmap) {
@@ -61,6 +62,9 @@ static int qcom_apcs_msm8916_clk_probe(struct platform_device
*pdev)
if (!a53cc)
return -ENOMEM;
+ if (of_clk_parent_fill(parent->of_node, parents, 2) == 2)
+ memcpy(gpll0_a53cc, parents, sizeof(parents));
+
init.name = "a53mux";
init.parent_names = gpll0_a53cc;
init.num_parents = ARRAY_SIZE(gpll0_a53cc);
@@ -76,10 +80,11 @@ static int qcom_apcs_msm8916_clk_probe(struct
platform_device *pdev)
a53cc->src_shift = 8;
a53cc->parent_map = gpll0_a53cc_map;
- a53cc->pclk = devm_clk_get(parent, NULL);
+ a53cc->pclk = of_clk_get(parent->of_node, 0);
if (IS_ERR(a53cc->pclk)) {
ret = PTR_ERR(a53cc->pclk);
- dev_err(dev, "failed to get clk: %d\n", ret);
+ if (ret != -EPROBE_DEFER)
+ dev_err(dev, "failed to get clk: %d\n", ret);
return ret;
}
@@ -87,6 +92,7 @@ static int qcom_apcs_msm8916_clk_probe(struct platform_device
*pdev)
ret = clk_notifier_register(a53cc->pclk, &a53cc->clk_nb);
if (ret) {
dev_err(dev, "failed to register clock notifier: %d\n", ret);
+ clk_put(a53cc->pclk);
return ret;
}
@@ -109,6 +115,8 @@ static int qcom_apcs_msm8916_clk_probe(struct
platform_device *pdev)
err:
clk_notifier_unregister(a53cc->pclk, &a53cc->clk_nb);
+ clk_put(a53cc->pclk);
+
return ret;
}
@@ -117,6 +125,7 @@ static int qcom_apcs_msm8916_clk_remove(struct
platform_device *pdev)
struct clk_regmap_mux_div *a53cc = platform_get_drvdata(pdev);
clk_notifier_unregister(a53cc->pclk, &a53cc->clk_nb);
+ clk_put(a53cc->pclk);
return 0;
}
--
2.23.0
[PATCH v2 4/5] clk: qcom: hfpll: use clk_parent_data to specify the parent
Extend support to platorms using different parents.
Co-developed-by: Niklas Cassel
Signed-off-by: Niklas Cassel
Signed-off-by: Jorge Ramirez-Ortiz
---
drivers/clk/qcom/hfpll.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/qcom/hfpll.c b/drivers/clk/qcom/hfpll.c
index 225c675f6779..5ff7f5a60620 100644
--- a/drivers/clk/qcom/hfpll.c
+++ b/drivers/clk/qcom/hfpll.c
@@ -53,7 +53,6 @@ static int qcom_hfpll_probe(struct platform_device *pdev)
struct regmap *regmap;
struct clk_hfpll *h;
struct clk_init_data init = {
- .parent_names = (const char *[]){ "xo" },
.num_parents = 1,
.ops = &clk_ops_hfpll,
/*
@@ -65,6 +64,7 @@ static int qcom_hfpll_probe(struct platform_device *pdev)
.flags = CLK_IGNORE_UNUSED,
};
int ret;
+ struct clk_parent_data pdata = { .index = 0 };
h = devm_kzalloc(dev, sizeof(*h), GFP_KERNEL);
if (!h)
@@ -83,6 +83,8 @@ static int qcom_hfpll_probe(struct platform_device *pdev)
0, &init.name))
return -ENODEV;
+ init.parent_data = &pdata;
+
h->d = &hdata;
h->clkr.hw.init = &init;
spin_lock_init(&h->lock);
--
2.23.0
[PATCH v2 1/5] clk: qcom: gcc: limit GPLL0_AO_OUT operating frequency
Limit the GPLL0_AO_OUT_MAIN operating frequency as per its hardware
specifications.
Co-developed-by: Niklas Cassel
Signed-off-by: Niklas Cassel
Signed-off-by: Jorge Ramirez-Ortiz
Reviewed-by: Bjorn Andersson
Acked-by: Stephen Boyd
---
drivers/clk/qcom/clk-alpha-pll.c | 8
drivers/clk/qcom/clk-alpha-pll.h | 1 +
drivers/clk/qcom/gcc-qcs404.c| 2 +-
3 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
index 0ced4a5a9a17..ef51f302bdf0 100644
--- a/drivers/clk/qcom/clk-alpha-pll.c
+++ b/drivers/clk/qcom/clk-alpha-pll.c
@@ -730,6 +730,14 @@ static long alpha_pll_huayra_round_rate(struct clk_hw *hw,
unsigned long rate,
return alpha_huayra_pll_round_rate(rate, *prate, &l, &a);
}
+const struct clk_ops clk_alpha_pll_fixed_ops = {
+ .enable = clk_alpha_pll_enable,
+ .disable = clk_alpha_pll_disable,
+ .is_enabled = clk_alpha_pll_is_enabled,
+ .recalc_rate = clk_alpha_pll_recalc_rate,
+};
+EXPORT_SYMBOL_GPL(clk_alpha_pll_fixed_ops);
+
const struct clk_ops clk_alpha_pll_ops = {
.enable = clk_alpha_pll_enable,
.disable = clk_alpha_pll_disable,
diff --git a/drivers/clk/qcom/clk-alpha-pll.h b/drivers/clk/qcom/clk-alpha-pll.h
index 66755f0f84fc..6b4eb74706b4 100644
--- a/drivers/clk/qcom/clk-alpha-pll.h
+++ b/drivers/clk/qcom/clk-alpha-pll.h
@@ -104,6 +104,7 @@ struct alpha_pll_config {
};
extern const struct clk_ops clk_alpha_pll_ops;
+extern const struct clk_ops clk_alpha_pll_fixed_ops;
extern const struct clk_ops clk_alpha_pll_hwfsm_ops;
extern const struct clk_ops clk_alpha_pll_postdiv_ops;
extern const struct clk_ops clk_alpha_pll_huayra_ops;
diff --git a/drivers/clk/qcom/gcc-qcs404.c b/drivers/clk/qcom/gcc-qcs404.c
index 29cf464dd2c8..18c6563889f3 100644
--- a/drivers/clk/qcom/gcc-qcs404.c
+++ b/drivers/clk/qcom/gcc-qcs404.c
@@ -330,7 +330,7 @@ static struct clk_alpha_pll gpll0_ao_out_main = {
.parent_names = (const char *[]){ "cxo" },
.num_parents = 1,
.flags = CLK_IS_CRITICAL,
- .ops = &clk_alpha_pll_ops,
+ .ops = &clk_alpha_pll_fixed_ops,
},
},
};
--
2.23.0
Re: [PATCH 1/2] watchdog: qcom: support pre-timeout when the bark irq is available
On 10/09/19 11:06:55, Guenter Roeck wrote:
> On Fri, Sep 06, 2019 at 10:54:10PM +0200, Jorge Ramirez-Ortiz wrote:
> > Use the bark interrupt as the pre-timeout notifier whenever this
> > interrupt is available.
> >
> > By default, the pretimeout notification shall occur one second earlier
> > than the timeout.
> >
> > Signed-off-by: Jorge Ramirez-Ortiz
>
> Nitpick below, otherwise:
>
> Reviewed-by: Guenter Roeck
>
> > ---
> > drivers/watchdog/qcom-wdt.c | 70 ++---
> > 1 file changed, 65 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> > index 7be7f87be28f..935c78a882a3 100644
> > --- a/drivers/watchdog/qcom-wdt.c
> > +++ b/drivers/watchdog/qcom-wdt.c
> > @@ -1,8 +1,10 @@
> > // SPDX-License-Identifier: GPL-2.0-only
> > /* Copyright (c) 2014, The Linux Foundation. All rights reserved.
> > */
> > +#include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -19,6 +21,9 @@ enum wdt_reg {
> > WDT_BITE_TIME,
> > };
> >
> > +#define QCOM_WDT_ENABLEBIT(0)
> > +#define QCOM_WDT_ENABLE_IRQBIT(1)
> > +
> > static const u32 reg_offset_data_apcs_tmr[] = {
> > [WDT_RST] = 0x38,
> > [WDT_EN] = 0x40,
> > @@ -54,15 +59,35 @@ struct qcom_wdt *to_qcom_wdt(struct watchdog_device
> > *wdd)
> > return container_of(wdd, struct qcom_wdt, wdd);
> > }
> >
> > +static inline int qcom_get_enable(struct watchdog_device *wdd)
> > +{
> > + int enable = QCOM_WDT_ENABLE;
> > +
> > + if (wdd->pretimeout)
> > + enable |= QCOM_WDT_ENABLE_IRQ;
> > +
> > + return enable;
> > +}
> > +
> > +static irqreturn_t qcom_wdt_isr(int irq, void *arg)
> > +{
> > + struct watchdog_device *wdd = arg;
> > +
> > + watchdog_notify_pretimeout(wdd);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > static int qcom_wdt_start(struct watchdog_device *wdd)
> > {
> > struct qcom_wdt *wdt = to_qcom_wdt(wdd);
> > + unsigned int bark = wdd->timeout - wdd->pretimeout;
> >
> > writel(0, wdt_addr(wdt, WDT_EN));
> > writel(1, wdt_addr(wdt, WDT_RST));
> > - writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
> > + writel(bark * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
> > writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME));
> > - writel(1, wdt_addr(wdt, WDT_EN));
> > + writel(qcom_get_enable(wdd), wdt_addr(wdt, WDT_EN));
> > return 0;
> > }
> >
> > @@ -89,6 +114,13 @@ static int qcom_wdt_set_timeout(struct watchdog_device
> > *wdd,
> > return qcom_wdt_start(wdd);
> > }
> >
> > +static int qcom_wdt_set_pretimeout(struct watchdog_device *wdd,
> > + unsigned int timeout)
> > +{
> > + wdd->pretimeout = timeout;
> > + return qcom_wdt_start(wdd);
> > +}
> > +
> > static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long
> > action,
> > void *data)
> > {
> > @@ -105,7 +137,7 @@ static int qcom_wdt_restart(struct watchdog_device
> > *wdd, unsigned long action,
> > writel(1, wdt_addr(wdt, WDT_RST));
> > writel(timeout, wdt_addr(wdt, WDT_BARK_TIME));
> > writel(timeout, wdt_addr(wdt, WDT_BITE_TIME));
> > - writel(1, wdt_addr(wdt, WDT_EN));
> > + writel(QCOM_WDT_ENABLE, wdt_addr(wdt, WDT_EN));
> >
> > /*
> > * Actually make sure the above sequence hits hardware before sleeping.
> > @@ -121,6 +153,7 @@ static const struct watchdog_ops qcom_wdt_ops = {
> > .stop = qcom_wdt_stop,
> > .ping = qcom_wdt_ping,
> > .set_timeout= qcom_wdt_set_timeout,
> > + .set_pretimeout = qcom_wdt_set_pretimeout,
> > .restart= qcom_wdt_restart,
> > .owner = THIS_MODULE,
> > };
> > @@ -133,6 +166,15 @@ static const struct watchdog_info qcom_wdt_info = {
> > .identity = KBUILD_MODNAME,
> > };
> >
> > +static const struct watchdog_info qcom_wdt_pt_info = {
> > + .options= WDIOF_KEEPALIVEPING
> > + | WDIOF_MAGICCLOSE
> > + | WDIOF_SETTIMEOUT
> > + | WDIOF_PRETIMEOUT
> > + | WDIOF_CARDRESET,
> > + .identity = K
Re: [PATCH 2/5] clk: qcom: apcs-msm8916: get parent clock names from DT
On 9/10/19 11:34, Jorge Ramirez wrote: > On 9/10/19 11:14, Stephen Boyd wrote: >> Quoting Jorge Ramirez-Ortiz, Linaro (2019-09-09 09:54:08) >>> On 09/09/19 09:17:03, Stephen Boyd wrote: >>>> But now the binding is different for the same compatible. I'd prefer we >>>> keep using devm_clk_get() and use a device pointer here and reorder the >>>> map and parent arrays instead. The clocks property shouldn't change in a >>>> way that isn't "additive" so that we maintain backwards compatibility. >>>> >>> >>> but the backwards compatibility is fully maintained - that is the main >>> reason >>> behind the change. the new stuff is that instead of hardcoding the >>> names in the source - like it is being done on the msm8916- we provide >>> the clocks in the dts node (a cleaner approach with the obvious >>> benefit of allowing new users to be added without having to modify the >>> sources). >>> >> >> This is not a backwards compatible change. >> >>>>> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi >>>>> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi >>>>> @@ -429,7 +429,8 @@ >>>>> compatible = "qcom,msm8916-apcs-kpss-global", "syscon"; >>>>> reg = <0xb011000 0x1000>; >>>>> #mbox-cells = <1>; >>>>> - clocks = <&a53pll>; >>>>> + clocks = <&gcc GPLL0_VOTE>, <&a53pll>; >>>>> + clock-names = "aux", "pll"; >>>>> #clock-cells = <0>; >>>>>}; >>>>> >>>>> >> >> Because the "clocks" property changed from >> >> <&a53pll> >> >> to >> >> <&gcc GPLL0_VOTE>, <&a53pll> >> >> and that moves pll to cell 1 instead of cell 0. >> >> > > what do you mean by backwards compatible? because this change does not > break previous clients. as per the comments I added to the code (in case this helps framing the discussion) [..] legacy bindings only defined the pll parent clock (index = 0) with no name; when both of the parents are specified in the bindings, the pll is the second one (index = 1). [..] > > > > > > >
Re: [PATCH 2/5] clk: qcom: apcs-msm8916: get parent clock names from DT
On 9/10/19 11:14, Stephen Boyd wrote: > Quoting Jorge Ramirez-Ortiz, Linaro (2019-09-09 09:54:08) >> On 09/09/19 09:17:03, Stephen Boyd wrote: >>> But now the binding is different for the same compatible. I'd prefer we >>> keep using devm_clk_get() and use a device pointer here and reorder the >>> map and parent arrays instead. The clocks property shouldn't change in a >>> way that isn't "additive" so that we maintain backwards compatibility. >>> >> >> but the backwards compatibility is fully maintained - that is the main reason >> behind the change. the new stuff is that instead of hardcoding the >> names in the source - like it is being done on the msm8916- we provide >> the clocks in the dts node (a cleaner approach with the obvious >> benefit of allowing new users to be added without having to modify the >> sources). >> > > This is not a backwards compatible change. > >>>> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi >>>> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi >>>> @@ -429,7 +429,8 @@ >>>> compatible = "qcom,msm8916-apcs-kpss-global", "syscon"; >>>> reg = <0xb011000 0x1000>; >>>> #mbox-cells = <1>; >>>> - clocks = <&a53pll>; >>>> + clocks = <&gcc GPLL0_VOTE>, <&a53pll>; >>>> + clock-names = "aux", "pll"; >>>> #clock-cells = <0>; >>>>}; >>>> >>>> > > Because the "clocks" property changed from > > <&a53pll> > > to > > <&gcc GPLL0_VOTE>, <&a53pll> > > and that moves pll to cell 1 instead of cell 0. > > what do you mean by backwards compatible? because this change does not break previous clients.
Re: [PATCH 2/5] clk: qcom: apcs-msm8916: get parent clock names from DT
On 09/09/19 09:17:03, Stephen Boyd wrote:
> Quoting Jorge Ramirez-Ortiz, Linaro (2019-09-09 07:17:40)
> > On 09/09/19 03:21:16, Stephen Boyd wrote:
> > > Quoting Jorge Ramirez-Ortiz (2019-08-26 09:45:07)
> > > > @@ -76,10 +88,11 @@ static int qcom_apcs_msm8916_clk_probe(struct
> > > > platform_device *pdev)
> > > > a53cc->src_shift = 8;
> > > > a53cc->parent_map = gpll0_a53cc_map;
> > > >
> > > > - a53cc->pclk = devm_clk_get(parent, NULL);
> > > > + a53cc->pclk = of_clk_get(parent->of_node, pll_index);
> > >
> > > Presumably the PLL was always index 0, so why are we changing it to
> > > index 1 sometimes? Seems unnecessary.
> > >
> >
> > it came as a personal preference. hope it is acceptable (I would
> > rather not change it)
> >
> > apcs-msm8916.c declares the following
> >
> > [..]
> > static const u32 gpll0_a53cc_map[] = { 4, 5 };
> > static const char *gpll0_a53cc[] = {
> >"gpll0_vote",
> > "a53pll",
> > };
> > [..]
> >
> >
> > now will be doing this
> >
> > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > @@ -429,7 +429,8 @@
> > compatible = "qcom,msm8916-apcs-kpss-global", "syscon";
> > reg = <0xb011000 0x1000>;
> > #mbox-cells = <1>;
> > - clocks = <&a53pll>;
> > + clocks = <&gcc GPLL0_VOTE>, <&a53pll>;
> > + clock-names = "aux", "pll";
> > #clock-cells = <0>;
> >};
> >
> >
> >
> > so I chose to keep the consistency between the clocks definition and
> > just change the index before calling of_clk_get.
> >
>
> But now the binding is different for the same compatible. I'd prefer we
> keep using devm_clk_get() and use a device pointer here and reorder the
> map and parent arrays instead. The clocks property shouldn't change in a
> way that isn't "additive" so that we maintain backwards compatibility.
>
but the backwards compatibility is fully maintained - that is the main reason
behind the change. the new stuff is that instead of hardcoding the
names in the source - like it is being done on the msm8916- we provide
the clocks in the dts node (a cleaner approach with the obvious
benefit of allowing new users to be added without having to modify the
sources).
Re: [PATCH 2/5] clk: qcom: apcs-msm8916: get parent clock names from DT
On 09/09/19 03:21:16, Stephen Boyd wrote:
> Quoting Jorge Ramirez-Ortiz (2019-08-26 09:45:07)
> > @@ -76,10 +88,11 @@ static int qcom_apcs_msm8916_clk_probe(struct
> > platform_device *pdev)
> > a53cc->src_shift = 8;
> > a53cc->parent_map = gpll0_a53cc_map;
> >
> > - a53cc->pclk = devm_clk_get(parent, NULL);
> > + a53cc->pclk = of_clk_get(parent->of_node, pll_index);
>
> Presumably the PLL was always index 0, so why are we changing it to
> index 1 sometimes? Seems unnecessary.
>
it came as a personal preference. hope it is acceptable (I would
rather not change it)
apcs-msm8916.c declares the following
[..]
static const u32 gpll0_a53cc_map[] = { 4, 5 };
static const char *gpll0_a53cc[] = {
"gpll0_vote",
"a53pll",
};
[..]
now will be doing this
--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
@@ -429,7 +429,8 @@
compatible = "qcom,msm8916-apcs-kpss-global", "syscon";
reg = <0xb011000 0x1000>;
#mbox-cells = <1>;
- clocks = <&a53pll>;
+ clocks = <&gcc GPLL0_VOTE>, <&a53pll>;
+ clock-names = "aux", "pll";
#clock-cells = <0>;
};
so I chose to keep the consistency between the clocks definition and
just change the index before calling of_clk_get.
[PATCH v2] mailbox: qcom-apcs: fix max_register value
The mailbox length is 0x1000 hence the max_register value is 0xFFC.
Fixes: c6a8b171ca8e ("mailbox: qcom: Convert APCS IPC driver to use
regmap")
Signed-off-by: Jorge Ramirez-Ortiz
---
v2: added Fixes tag
drivers/mailbox/qcom-apcs-ipc-mailbox.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
index 705e17a5479c..e5d6b1b70441 100644
--- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
+++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
@@ -28,7 +28,7 @@ static const struct regmap_config apcs_regmap_config = {
.reg_bits = 32,
.reg_stride = 4,
.val_bits = 32,
- .max_register = 0x1000,
+ .max_register = 0xFFC,
.fast_io = true,
};
--
2.23.0
[PATCH v2] clk: qcom: fix QCS404 TuringCC regmap
The max register is 0x23004 as per the manual (the current
max_register that this commit is fixing is actually out of bounds).
Fixes: 892df0191b29 ("clk: qcom: Add QCS404 TuringCC")
Signed-off-by: Jorge Ramirez-Ortiz
---
v2: add Fixes tag
drivers/clk/qcom/turingcc-qcs404.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/qcom/turingcc-qcs404.c
b/drivers/clk/qcom/turingcc-qcs404.c
index aa859e6ec9bd..4cfbbf5bf4d9 100644
--- a/drivers/clk/qcom/turingcc-qcs404.c
+++ b/drivers/clk/qcom/turingcc-qcs404.c
@@ -96,7 +96,7 @@ static const struct regmap_config turingcc_regmap_config = {
.reg_bits = 32,
.reg_stride = 4,
.val_bits = 32,
- .max_register = 0x3,
+ .max_register = 0x23004,
.fast_io= true,
};
--
2.23.0
[PATCH] mailbox: qcom-apcs: fix max_register value
The mailbox length is 0x1000 hence the max_register value is 0xFFC.
Signed-off-by: Jorge Ramirez-Ortiz
---
drivers/mailbox/qcom-apcs-ipc-mailbox.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
index 705e17a5479c..e5d6b1b70441 100644
--- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
+++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
@@ -28,7 +28,7 @@ static const struct regmap_config apcs_regmap_config = {
.reg_bits = 32,
.reg_stride = 4,
.val_bits = 32,
- .max_register = 0x1000,
+ .max_register = 0xFFC,
.fast_io = true,
};
--
2.23.0
[PATCH] clk: qcom: fix QCS404 TuringCC regmap
The max register is 0x23004 as per the manual (the current
max_register that this commit is fixing is actually out of bounds).
Signed-off-by: Jorge Ramirez-Ortiz
---
drivers/clk/qcom/turingcc-qcs404.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/qcom/turingcc-qcs404.c
b/drivers/clk/qcom/turingcc-qcs404.c
index aa859e6ec9bd..4cfbbf5bf4d9 100644
--- a/drivers/clk/qcom/turingcc-qcs404.c
+++ b/drivers/clk/qcom/turingcc-qcs404.c
@@ -96,7 +96,7 @@ static const struct regmap_config turingcc_regmap_config = {
.reg_bits = 32,
.reg_stride = 4,
.val_bits = 32,
- .max_register = 0x3,
+ .max_register = 0x23004,
.fast_io= true,
};
--
2.23.0
[PATCH 1/2] watchdog: qcom: support pre-timeout when the bark irq is available
Use the bark interrupt as the pre-timeout notifier whenever this
interrupt is available.
By default, the pretimeout notification shall occur one second earlier
than the timeout.
Signed-off-by: Jorge Ramirez-Ortiz
---
drivers/watchdog/qcom-wdt.c | 70 ++---
1 file changed, 65 insertions(+), 5 deletions(-)
diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
index 7be7f87be28f..935c78a882a3 100644
--- a/drivers/watchdog/qcom-wdt.c
+++ b/drivers/watchdog/qcom-wdt.c
@@ -1,8 +1,10 @@
// SPDX-License-Identifier: GPL-2.0-only
/* Copyright (c) 2014, The Linux Foundation. All rights reserved.
*/
+#include
#include
#include
+#include
#include
#include
#include
@@ -19,6 +21,9 @@ enum wdt_reg {
WDT_BITE_TIME,
};
+#define QCOM_WDT_ENABLEBIT(0)
+#define QCOM_WDT_ENABLE_IRQBIT(1)
+
static const u32 reg_offset_data_apcs_tmr[] = {
[WDT_RST] = 0x38,
[WDT_EN] = 0x40,
@@ -54,15 +59,35 @@ struct qcom_wdt *to_qcom_wdt(struct watchdog_device *wdd)
return container_of(wdd, struct qcom_wdt, wdd);
}
+static inline int qcom_get_enable(struct watchdog_device *wdd)
+{
+ int enable = QCOM_WDT_ENABLE;
+
+ if (wdd->pretimeout)
+ enable |= QCOM_WDT_ENABLE_IRQ;
+
+ return enable;
+}
+
+static irqreturn_t qcom_wdt_isr(int irq, void *arg)
+{
+ struct watchdog_device *wdd = arg;
+
+ watchdog_notify_pretimeout(wdd);
+
+ return IRQ_HANDLED;
+}
+
static int qcom_wdt_start(struct watchdog_device *wdd)
{
struct qcom_wdt *wdt = to_qcom_wdt(wdd);
+ unsigned int bark = wdd->timeout - wdd->pretimeout;
writel(0, wdt_addr(wdt, WDT_EN));
writel(1, wdt_addr(wdt, WDT_RST));
- writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
+ writel(bark * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME));
- writel(1, wdt_addr(wdt, WDT_EN));
+ writel(qcom_get_enable(wdd), wdt_addr(wdt, WDT_EN));
return 0;
}
@@ -89,6 +114,13 @@ static int qcom_wdt_set_timeout(struct watchdog_device *wdd,
return qcom_wdt_start(wdd);
}
+static int qcom_wdt_set_pretimeout(struct watchdog_device *wdd,
+ unsigned int timeout)
+{
+ wdd->pretimeout = timeout;
+ return qcom_wdt_start(wdd);
+}
+
static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action,
void *data)
{
@@ -105,7 +137,7 @@ static int qcom_wdt_restart(struct watchdog_device *wdd,
unsigned long action,
writel(1, wdt_addr(wdt, WDT_RST));
writel(timeout, wdt_addr(wdt, WDT_BARK_TIME));
writel(timeout, wdt_addr(wdt, WDT_BITE_TIME));
- writel(1, wdt_addr(wdt, WDT_EN));
+ writel(QCOM_WDT_ENABLE, wdt_addr(wdt, WDT_EN));
/*
* Actually make sure the above sequence hits hardware before sleeping.
@@ -121,6 +153,7 @@ static const struct watchdog_ops qcom_wdt_ops = {
.stop = qcom_wdt_stop,
.ping = qcom_wdt_ping,
.set_timeout= qcom_wdt_set_timeout,
+ .set_pretimeout = qcom_wdt_set_pretimeout,
.restart= qcom_wdt_restart,
.owner = THIS_MODULE,
};
@@ -133,6 +166,15 @@ static const struct watchdog_info qcom_wdt_info = {
.identity = KBUILD_MODNAME,
};
+static const struct watchdog_info qcom_wdt_pt_info = {
+ .options= WDIOF_KEEPALIVEPING
+ | WDIOF_MAGICCLOSE
+ | WDIOF_SETTIMEOUT
+ | WDIOF_PRETIMEOUT
+ | WDIOF_CARDRESET,
+ .identity = KBUILD_MODNAME,
+};
+
static void qcom_clk_disable_unprepare(void *data)
{
clk_disable_unprepare(data);
@@ -146,7 +188,7 @@ static int qcom_wdt_probe(struct platform_device *pdev)
struct device_node *np = dev->of_node;
const u32 *regs;
u32 percpu_offset;
- int ret;
+ int irq, ret;
regs = of_device_get_match_data(dev);
if (!regs) {
@@ -204,7 +246,25 @@ static int qcom_wdt_probe(struct platform_device *pdev)
return -EINVAL;
}
- wdt->wdd.info = &qcom_wdt_info;
+ /* check if there is pretimeout support */
+ irq = platform_get_irq(pdev, 0);
+ if (irq > 0) {
+ ret = devm_request_irq(dev, irq, qcom_wdt_isr,
+ IRQF_TRIGGER_RISING,
+ "wdt_bark", &wdt->wdd);
+ if (ret)
+ return ret;
+
+ wdt->wdd.info = &qcom_wdt_pt_info;
+ wdt->wdd.pretimeout = 1;
+ } else {
+ if (irq == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+
+ wdt->wdd.info = &qcom_wdt_info;
[PATCH 2/2] watchdog: qcom: remove unnecessary variable from private storage
there is no need to continue keeping the clock in private storage.
Signed-off-by: Jorge Ramirez-Ortiz
---
drivers/watchdog/qcom-wdt.c | 15 +++
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
index 935c78a882a3..e98f5a3d83ea 100644
--- a/drivers/watchdog/qcom-wdt.c
+++ b/drivers/watchdog/qcom-wdt.c
@@ -42,7 +42,6 @@ static const u32 reg_offset_data_kpss[] = {
struct qcom_wdt {
struct watchdog_device wdd;
- struct clk *clk;
unsigned long rate;
void __iomem*base;
const u32 *layout;
@@ -189,6 +188,7 @@ static int qcom_wdt_probe(struct platform_device *pdev)
const u32 *regs;
u32 percpu_offset;
int irq, ret;
+ struct clk *clk;
regs = of_device_get_match_data(dev);
if (!regs) {
@@ -215,19 +215,18 @@ static int qcom_wdt_probe(struct platform_device *pdev)
if (IS_ERR(wdt->base))
return PTR_ERR(wdt->base);
- wdt->clk = devm_clk_get(dev, NULL);
- if (IS_ERR(wdt->clk)) {
+ clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(clk)) {
dev_err(dev, "failed to get input clock\n");
- return PTR_ERR(wdt->clk);
+ return PTR_ERR(clk);
}
- ret = clk_prepare_enable(wdt->clk);
+ ret = clk_prepare_enable(clk);
if (ret) {
dev_err(dev, "failed to setup clock\n");
return ret;
}
- ret = devm_add_action_or_reset(dev, qcom_clk_disable_unprepare,
- wdt->clk);
+ ret = devm_add_action_or_reset(dev, qcom_clk_disable_unprepare, clk);
if (ret)
return ret;
@@ -239,7 +238,7 @@ static int qcom_wdt_probe(struct platform_device *pdev)
* that it would bite before a second elapses it's usefulness is
* limited. Bail if this is the case.
*/
- wdt->rate = clk_get_rate(wdt->clk);
+ wdt->rate = clk_get_rate(clk);
if (wdt->rate == 0 ||
wdt->rate > 0x1000U) {
dev_err(dev, "invalid clock rate\n");
--
2.23.0
[PATCH 0/2] qcom_wdt bark irq support
Support pre-timeout when the bark irq is avaible. This is the fifth version of the patchset addressing all the review issues to date: v5: include linux/bits.h pretimeout only enables IRQs if value != 0 remove unnecessary subtract operation add clarity to the conditional in the probe function revert the irq registration changes v4: remove unnecessary include and private variable provide macro for WDT EN register values use pretimeout as per its API intent handle EPROBE_DEFER on get_irq modify the irq registration as per pm8916_wdt v3 remove unecessary variable from the driver's private storage v2: register the pre-timeout notifier. With the second patch in the set, I took the oportunity to do some cleanup in the same code base removing an unnecesary variable from the driver's private storage. Jorge Ramirez-Ortiz (2): watchdog: qcom: support pre-timeout when the bark irq is available watchdog: qcom: remove unnecessary variable from private storage drivers/watchdog/qcom-wdt.c | 85 +++-- 1 file changed, 72 insertions(+), 13 deletions(-) -- 2.23.0
[PATCH 2/2] watchdog: pm8916_wdt: fix missing include
As per Documentation/process/submit-checklist.rst, when using a facility #include the file that defines/declares that facility. Don't depend on other header files pulling in ones that you use. Signed-off-by: Jorge Ramirez-Ortiz --- drivers/watchdog/pm8916_wdt.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/watchdog/pm8916_wdt.c b/drivers/watchdog/pm8916_wdt.c index cb5304c26ac3..b8d9df0f96f7 100644 --- a/drivers/watchdog/pm8916_wdt.c +++ b/drivers/watchdog/pm8916_wdt.c @@ -1,5 +1,6 @@ // SPDX-License-Identifier: GPL-2.0 #include +#include #include #include #include -- 2.23.0
[PATCH 1/2] watchdog: pm8916_wdt: fix pretimeout registration flow
When an IRQ is present in the dts, the probe function shall fail if
the interrupt can not be registered.
The probe function shall also be retried if getting the irq is being
deferred.
Signed-off-by: Jorge Ramirez-Ortiz
---
drivers/watchdog/pm8916_wdt.c | 16
1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/watchdog/pm8916_wdt.c b/drivers/watchdog/pm8916_wdt.c
index 2d3652004e39..cb5304c26ac3 100644
--- a/drivers/watchdog/pm8916_wdt.c
+++ b/drivers/watchdog/pm8916_wdt.c
@@ -163,9 +163,18 @@ static int pm8916_wdt_probe(struct platform_device *pdev)
irq = platform_get_irq(pdev, 0);
if (irq > 0) {
- if (devm_request_irq(dev, irq, pm8916_wdt_isr, 0, "pm8916_wdt",
-wdt))
- irq = 0;
+ err = devm_request_irq(dev, irq, pm8916_wdt_isr, 0,
+ "pm8916_wdt", wdt);
+ if (err)
+ return err;
+
+ wdt->wdev.info = &pm8916_wdt_pt_ident;
+
+ } else {
+ if (irq == -EPROBE_DEFER)
+ return -EPROBE_DEFER;
+
+ wdt->wdev.info = &pm8916_wdt_ident;
}
/* Configure watchdog to hard-reset mode */
@@ -177,7 +186,6 @@ static int pm8916_wdt_probe(struct platform_device *pdev)
return err;
}
- wdt->wdev.info = (irq > 0) ? &pm8916_wdt_pt_ident : &pm8916_wdt_ident,
wdt->wdev.ops = &pm8916_wdt_ops,
wdt->wdev.parent = dev;
wdt->wdev.min_timeout = PM8916_WDT_MIN_TIMEOUT;
--
2.23.0
Re: [PATCH v4] watchdog: qcom: support pre-timeout when the bark irq is available
On 9/6/19 19:40, Bjorn Andersson wrote:
> On Thu 05 Sep 14:00 PDT 2019, Jorge Ramirez-Ortiz wrote:
>> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> [..]
>> +static inline int qcom_get_enable(struct watchdog_device *wdd)
>> +{
>> +int enable = QCOM_WDT_ENABLE;
>> +
>> +if (wdd->info->options & WDIOF_PRETIMEOUT)
>> +enable |= QCOM_WDT_ENABLE_IRQ;
>
> Looking at downstream they conditionally write 3 to WDT_EN during
> initialization, but during suspend/resume they just set it to back to 1.
>
> So I don't think you should touch BIT(1) (which name doesn't match
> downstream or the register documentation)
writing BIT(1) on the enable register is necessary to get the interrupt
and therefore to be notified of the bark event. this can not be avoided.
>
>> +
>> +return enable;
>> +}
>> +
>> +static irqreturn_t qcom_wdt_isr(int irq, void *arg)
>> +{
>> +struct watchdog_device *wdd = arg;
>> +
>> +watchdog_notify_pretimeout(wdd);
>> +
>> +return IRQ_HANDLED;
>> +}
>> +
>> static int qcom_wdt_start(struct watchdog_device *wdd)
>> {
>> struct qcom_wdt *wdt = to_qcom_wdt(wdd);
>> +unsigned int bark = wdd->timeout;
>> +
>> +if (wdd->pretimeout)
>> +bark = bark - wdd->pretimeout;
>
> As Guenter points out, writing wdd->timeout - wdt->pretimeout to
> WDT_BARK_TIME unconditionally should do the trick.
yes
>
>>
>> writel(0, wdt_addr(wdt, WDT_EN));
>> writel(1, wdt_addr(wdt, WDT_RST));
>> -writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
>> +writel(bark * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
>> writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME));
>> -writel(1, wdt_addr(wdt, WDT_EN));
>> +writel(qcom_get_enable(wdd), wdt_addr(wdt, WDT_EN));
>> return 0;
>> }
> [..]
>> @@ -204,7 +248,17 @@ static int qcom_wdt_probe(struct platform_device *pdev)
>> return -EINVAL;
>> }
>>
>> -wdt->wdd.info = &qcom_wdt_info;
>> +irq = platform_get_irq(pdev, 0);
>> +if (irq > 0) {
>> +if (devm_request_irq(dev, irq, qcom_wdt_isr,
>> + IRQF_TRIGGER_RISING, "wdt_bark",
>> + &wdt->wdd))
>
> A failure here means that a irq was specified in DT (platform_get_irq()
> returned > 0) but you failed to acquire request it, you should fail your
> probe() when this happens.
yeah that is what I thought but since pm8916-wdt.c has recently been
merged exactly like I copied above I chose to follow to avoid arguing
about this.
anyway I'll send a patch to fix pm8916-wdt.c and then will do it that
same way on this driver.
>
>> +irq = 0;
>> +} else if (irq == -EPROBE_DEFER)
>> +return -EPROBE_DEFER;
>
> Some {} around this block please.
um, checkpatch didnt complain. anyway sure, will do
>
> Regards,
> Bjorn
>
>> +
>> +wdt->wdd.info = irq > 0 ? &qcom_wdt_pt_info : &qcom_wdt_info;
>> +wdt->wdd.pretimeout = irq > 0 ? 1 : 0;
>> wdt->wdd.ops = &qcom_wdt_ops;
>> wdt->wdd.min_timeout = 1;
>> wdt->wdd.max_timeout = 0x1000U / wdt->rate;
>> --
>> 2.23.0
>>
>
Re: [PATCH v4] watchdog: qcom: support pre-timeout when the bark irq is available
>>>
static const u32 reg_offset_data_apcs_tmr[] = {
[WDT_RST] = 0x38,
[WDT_EN] = 0x40,
@@ -54,15 +58,38 @@ struct qcom_wdt *to_qcom_wdt(struct watchdog_device
*wdd)
return container_of(wdd, struct qcom_wdt, wdd);
}
+static inline int qcom_get_enable(struct watchdog_device *wdd)
+{
+ int enable = QCOM_WDT_ENABLE;
+
+ if (wdd->info->options & WDIOF_PRETIMEOUT)
+ enable |= QCOM_WDT_ENABLE_IRQ;
+
>>>
>>> Again, the condition needs to be that pretimeout != 0,
>>> not that it is supported.
>>
>> no I dont think so. doing that would propagate a possible error in some
>> pretimeout setup code which would end up enabling an interrupt when it
>> shouldnt. so I dont think that doing that would be correct.
>>
> If the pretimeout setup code is buggy, it needs to be fixed.
the condition whether to enable the HW interrupts (IMO) should be
controlled by the DTS as part of the static configuration.
>
>> The interrupt should only be enabled if WDIOF_PRETIMEOUT is configured
>> (independently of the pretimeout value); as a matter of fact, if
>> pretimeout is 0, the interrupt will trigger at the same time than bark
>> (which is what the original code used to do).
>>
> The original code did not set bit 1 of the WDT_EN register,
sure, this is true
> and it did not set the bark time.
actually no, unless we are looking at different files, the original code
did set the bark time even though PRETIMEOUT was not enabled... so yes
bark was being set to bite. Maybe I am misunderstanding your point.
>
>> so I'd rather keep this condition unless you strongly oppose to it.
>>
>
> Please feel free to petition to Wim.
I'll change to your recomendation and repost v5 - I thought
WDIOF_PRETIMEOUT was formally correct but functionally there is little
difference (if the hardware works as expected)
thanks for all your comments Guenter.
Re: [PATCH v4] watchdog: qcom: support pre-timeout when the bark irq is available
On 9/5/19 23:19, Guenter Roeck wrote:
> On Thu, Sep 05, 2019 at 11:00:35PM +0200, Jorge Ramirez-Ortiz wrote:
>> Use the bark interrupt as the pre-timeout notifier whenever this
>> interrupt is available.
>>
>> By default, the pretimeout notification shall occur one second earlier
>> than the timeout.
>>
>> Signed-off-by: Jorge Ramirez-Ortiz
>> ---
>> v4:
>> address Guenter Roeck comments as follows:
>>remove unnecessary include and private variable
>>provide macro for WDT EN register values
>>use pretimeout as per its API intent
>>handle EPROBE_DEFER on get_irq
>> also:
>>handle the irq registration as done in pm8916_wdt.c
>> v3:
>> remove unnecesary variable added to private.
>>
>> v2:
>> register the pre-timeout notifier instead.
>>
>> v1:
>> drivers/watchdog/qcom-wdt.c | 64 ++---
>> 1 file changed, 59 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
>> index 7be7f87be28f..0f1d29eeb81d 100644
>> --- a/drivers/watchdog/qcom-wdt.c
>> +++ b/drivers/watchdog/qcom-wdt.c
>> @@ -10,6 +10,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> enum wdt_reg {
>> WDT_RST,
>> @@ -19,6 +20,9 @@ enum wdt_reg {
>> WDT_BITE_TIME,
>> };
>>
>> +#define QCOM_WDT_ENABLE BIT(0)
>> +#define QCOM_WDT_ENABLE_IRQ BIT(1)
>> +
>
> Using BIT() requires "#include ".
do you want it explicitly in the file even if it builds?
>
>> static const u32 reg_offset_data_apcs_tmr[] = {
>> [WDT_RST] = 0x38,
>> [WDT_EN] = 0x40,
>> @@ -54,15 +58,38 @@ struct qcom_wdt *to_qcom_wdt(struct watchdog_device *wdd)
>> return container_of(wdd, struct qcom_wdt, wdd);
>> }
>>
>> +static inline int qcom_get_enable(struct watchdog_device *wdd)
>> +{
>> +int enable = QCOM_WDT_ENABLE;
>> +
>> +if (wdd->info->options & WDIOF_PRETIMEOUT)
>> +enable |= QCOM_WDT_ENABLE_IRQ;
>> +
>
> Again, the condition needs to be that pretimeout != 0,
> not that it is supported.
no I dont think so. doing that would propagate a possible error in some
pretimeout setup code which would end up enabling an interrupt when it
shouldnt. so I dont think that doing that would be correct.
The interrupt should only be enabled if WDIOF_PRETIMEOUT is configured
(independently of the pretimeout value); as a matter of fact, if
pretimeout is 0, the interrupt will trigger at the same time than bark
(which is what the original code used to do).
so I'd rather keep this condition unless you strongly oppose to it.
>
>> +return enable;
>> +}
>> +
>> +static irqreturn_t qcom_wdt_isr(int irq, void *arg)
>> +{
>> +struct watchdog_device *wdd = arg;
>> +
>> +watchdog_notify_pretimeout(wdd);
>> +
>> +return IRQ_HANDLED;
>> +}
>> +
>> static int qcom_wdt_start(struct watchdog_device *wdd)
>> {
>> struct qcom_wdt *wdt = to_qcom_wdt(wdd);
>> +unsigned int bark = wdd->timeout;
>> +
>> +if (wdd->pretimeout)
>> +bark = bark - wdd->pretimeout;
>
> The if() just adds code and doesn't otherwise do any good.
yeah, was just for clarity and it is surely removed by the compiler. but
sure will remove
>
>>
>> writel(0, wdt_addr(wdt, WDT_EN));
>> writel(1, wdt_addr(wdt, WDT_RST));
>> -writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
>> +writel(bark * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
>> writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME));
>> -writel(1, wdt_addr(wdt, WDT_EN));
>> +writel(qcom_get_enable(wdd), wdt_addr(wdt, WDT_EN));
>> return 0;
>> }
>>
>> @@ -89,6 +116,13 @@ static int qcom_wdt_set_timeout(struct watchdog_device
>> *wdd,
>> return qcom_wdt_start(wdd);
>> }
>>
>> +static int qcom_wdt_set_pretimeout(struct watchdog_device *wdd,
>> + unsigned int timeout)
>> +{
>> +wdd->pretimeout = timeout;
>> +return qcom_wdt_start(wdd);
>> +}
>> +
>> static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long
>> action,
>> void *data)
>> {
>> @@ -105,7 +139,7 @@ static int qcom_wdt_restart(struct watchdog_device *wdd,
>> unsigned long action,
>>
[PATCH v4] watchdog: qcom: support pre-timeout when the bark irq is available
Use the bark interrupt as the pre-timeout notifier whenever this
interrupt is available.
By default, the pretimeout notification shall occur one second earlier
than the timeout.
Signed-off-by: Jorge Ramirez-Ortiz
---
v4:
address Guenter Roeck comments as follows:
remove unnecessary include and private variable
provide macro for WDT EN register values
use pretimeout as per its API intent
handle EPROBE_DEFER on get_irq
also:
handle the irq registration as done in pm8916_wdt.c
v3:
remove unnecesary variable added to private.
v2:
register the pre-timeout notifier instead.
v1:
drivers/watchdog/qcom-wdt.c | 64 ++---
1 file changed, 59 insertions(+), 5 deletions(-)
diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
index 7be7f87be28f..0f1d29eeb81d 100644
--- a/drivers/watchdog/qcom-wdt.c
+++ b/drivers/watchdog/qcom-wdt.c
@@ -10,6 +10,7 @@
#include
#include
#include
+#include
enum wdt_reg {
WDT_RST,
@@ -19,6 +20,9 @@ enum wdt_reg {
WDT_BITE_TIME,
};
+#define QCOM_WDT_ENABLEBIT(0)
+#define QCOM_WDT_ENABLE_IRQBIT(1)
+
static const u32 reg_offset_data_apcs_tmr[] = {
[WDT_RST] = 0x38,
[WDT_EN] = 0x40,
@@ -54,15 +58,38 @@ struct qcom_wdt *to_qcom_wdt(struct watchdog_device *wdd)
return container_of(wdd, struct qcom_wdt, wdd);
}
+static inline int qcom_get_enable(struct watchdog_device *wdd)
+{
+ int enable = QCOM_WDT_ENABLE;
+
+ if (wdd->info->options & WDIOF_PRETIMEOUT)
+ enable |= QCOM_WDT_ENABLE_IRQ;
+
+ return enable;
+}
+
+static irqreturn_t qcom_wdt_isr(int irq, void *arg)
+{
+ struct watchdog_device *wdd = arg;
+
+ watchdog_notify_pretimeout(wdd);
+
+ return IRQ_HANDLED;
+}
+
static int qcom_wdt_start(struct watchdog_device *wdd)
{
struct qcom_wdt *wdt = to_qcom_wdt(wdd);
+ unsigned int bark = wdd->timeout;
+
+ if (wdd->pretimeout)
+ bark = bark - wdd->pretimeout;
writel(0, wdt_addr(wdt, WDT_EN));
writel(1, wdt_addr(wdt, WDT_RST));
- writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
+ writel(bark * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME));
- writel(1, wdt_addr(wdt, WDT_EN));
+ writel(qcom_get_enable(wdd), wdt_addr(wdt, WDT_EN));
return 0;
}
@@ -89,6 +116,13 @@ static int qcom_wdt_set_timeout(struct watchdog_device *wdd,
return qcom_wdt_start(wdd);
}
+static int qcom_wdt_set_pretimeout(struct watchdog_device *wdd,
+ unsigned int timeout)
+{
+ wdd->pretimeout = timeout;
+ return qcom_wdt_start(wdd);
+}
+
static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action,
void *data)
{
@@ -105,7 +139,7 @@ static int qcom_wdt_restart(struct watchdog_device *wdd,
unsigned long action,
writel(1, wdt_addr(wdt, WDT_RST));
writel(timeout, wdt_addr(wdt, WDT_BARK_TIME));
writel(timeout, wdt_addr(wdt, WDT_BITE_TIME));
- writel(1, wdt_addr(wdt, WDT_EN));
+ writel(qcom_get_enable(wdd), wdt_addr(wdt, WDT_EN));
/*
* Actually make sure the above sequence hits hardware before sleeping.
@@ -121,6 +155,7 @@ static const struct watchdog_ops qcom_wdt_ops = {
.stop = qcom_wdt_stop,
.ping = qcom_wdt_ping,
.set_timeout= qcom_wdt_set_timeout,
+ .set_pretimeout = qcom_wdt_set_pretimeout,
.restart= qcom_wdt_restart,
.owner = THIS_MODULE,
};
@@ -133,6 +168,15 @@ static const struct watchdog_info qcom_wdt_info = {
.identity = KBUILD_MODNAME,
};
+static const struct watchdog_info qcom_wdt_pt_info = {
+ .options= WDIOF_KEEPALIVEPING
+ | WDIOF_MAGICCLOSE
+ | WDIOF_SETTIMEOUT
+ | WDIOF_PRETIMEOUT
+ | WDIOF_CARDRESET,
+ .identity = KBUILD_MODNAME,
+};
+
static void qcom_clk_disable_unprepare(void *data)
{
clk_disable_unprepare(data);
@@ -146,7 +190,7 @@ static int qcom_wdt_probe(struct platform_device *pdev)
struct device_node *np = dev->of_node;
const u32 *regs;
u32 percpu_offset;
- int ret;
+ int irq, ret;
regs = of_device_get_match_data(dev);
if (!regs) {
@@ -204,7 +248,17 @@ static int qcom_wdt_probe(struct platform_device *pdev)
return -EINVAL;
}
- wdt->wdd.info = &qcom_wdt_info;
+ irq = platform_get_irq(pdev, 0);
+ if (irq > 0) {
+ if (devm_request_irq(dev, irq, qcom_wdt_isr,
+IRQF_TRIGGER_RISING, "wdt_bark",
+&a
Re: [PATCH v3] watchdog: qcom: support pre-timeout when the bark irq is available
On 9/5/19 20:34, Guenter Roeck wrote: > On Thu, Sep 05, 2019 at 08:24:19PM +0200, Jorge Ramirez-Ortiz wrote: >> Use the bark interrupt as the pre-timeout notifier whenever this >> interrupt is available. >> >> By default, the pretimeout notification shall occur one second earlier >> than the timeout. >> >> Signed-off-by: Jorge Ramirez-Ortiz > > Can you please give people time to reply before flooding the mailing list > with revisions ? This is getting annoying quite rapidly. I am going to > reply to v2, as I had started on that already (and to the one without > subject before). yes, sorry about it. it is not frequent that someone gives a review so quickly though so I thought I had time to sneak in some reworks - thanks and apologies again. I am about to send v4 addressing the issues you raised. > > Guenter > . >
[PATCH v3] watchdog: qcom: support pre-timeout when the bark irq is available
Use the bark interrupt as the pre-timeout notifier whenever this
interrupt is available.
By default, the pretimeout notification shall occur one second earlier
than the timeout.
Signed-off-by: Jorge Ramirez-Ortiz
---
v3:
remove unnecesary variable added to private.
v2:
register the pre-timeout notifier instead.
v1:
use the bark irq to notify the user.
drivers/watchdog/qcom-wdt.c | 60 +
1 file changed, 55 insertions(+), 5 deletions(-)
diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
index 7be7f87be28f..847dbf79df1c 100644
--- a/drivers/watchdog/qcom-wdt.c
+++ b/drivers/watchdog/qcom-wdt.c
@@ -10,6 +10,8 @@
#include
#include
#include
+#include
+#include
enum wdt_reg {
WDT_RST,
@@ -54,15 +56,37 @@ struct qcom_wdt *to_qcom_wdt(struct watchdog_device *wdd)
return container_of(wdd, struct qcom_wdt, wdd);
}
+static inline int qcom_wdt_enable(struct watchdog_device *wdd)
+{
+ /* enable the bark interrupt */
+ if (wdd->info->options & WDIOF_PRETIMEOUT)
+ return 3;
+
+ return 1;
+}
+
+static irqreturn_t qcom_wdt_irq(int irq, void *cookie)
+{
+ struct watchdog_device *wdd = (struct watchdog_device *) cookie;
+
+ watchdog_notify_pretimeout(wdd);
+
+ return IRQ_HANDLED;
+}
+
static int qcom_wdt_start(struct watchdog_device *wdd)
{
struct qcom_wdt *wdt = to_qcom_wdt(wdd);
+ unsigned int bark = wdd->pretimeout;
+
+ if (!(wdd->info->options & WDIOF_PRETIMEOUT))
+ bark = wdd->timeout;
writel(0, wdt_addr(wdt, WDT_EN));
writel(1, wdt_addr(wdt, WDT_RST));
- writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
+ writel(bark * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME));
- writel(1, wdt_addr(wdt, WDT_EN));
+ writel(qcom_wdt_enable(wdd), wdt_addr(wdt, WDT_EN));
return 0;
}
@@ -89,6 +113,14 @@ static int qcom_wdt_set_timeout(struct watchdog_device *wdd,
return qcom_wdt_start(wdd);
}
+static int qcom_wdt_set_pretimeout(struct watchdog_device *wdd,
+ unsigned int timeout)
+{
+ wdd->pretimeout = timeout;
+
+ return 0;
+}
+
static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action,
void *data)
{
@@ -105,7 +137,7 @@ static int qcom_wdt_restart(struct watchdog_device *wdd,
unsigned long action,
writel(1, wdt_addr(wdt, WDT_RST));
writel(timeout, wdt_addr(wdt, WDT_BARK_TIME));
writel(timeout, wdt_addr(wdt, WDT_BITE_TIME));
- writel(1, wdt_addr(wdt, WDT_EN));
+ writel(qcom_wdt_enable(wdd), wdt_addr(wdt, WDT_EN));
/*
* Actually make sure the above sequence hits hardware before sleeping.
@@ -121,11 +153,12 @@ static const struct watchdog_ops qcom_wdt_ops = {
.stop = qcom_wdt_stop,
.ping = qcom_wdt_ping,
.set_timeout= qcom_wdt_set_timeout,
+ .set_pretimeout = qcom_wdt_set_pretimeout,
.restart= qcom_wdt_restart,
.owner = THIS_MODULE,
};
-static const struct watchdog_info qcom_wdt_info = {
+static struct watchdog_info qcom_wdt_info = {
.options= WDIOF_KEEPALIVEPING
| WDIOF_MAGICCLOSE
| WDIOF_SETTIMEOUT
@@ -146,7 +179,7 @@ static int qcom_wdt_probe(struct platform_device *pdev)
struct device_node *np = dev->of_node;
const u32 *regs;
u32 percpu_offset;
- int ret;
+ int irq, ret;
regs = of_device_get_match_data(dev);
if (!regs) {
@@ -222,6 +255,23 @@ static int qcom_wdt_probe(struct platform_device *pdev)
wdt->wdd.timeout = min(wdt->wdd.max_timeout, 30U);
watchdog_init_timeout(&wdt->wdd, 0, dev);
+ irq = platform_get_irq(pdev, 0);
+ if (irq >= 0) {
+ /* enable the pre-timeout notification */
+ qcom_wdt_info.options |= WDIOF_PRETIMEOUT;
+
+ ret = devm_request_irq(&pdev->dev, irq, qcom_wdt_irq,
+ IRQF_TRIGGER_RISING, "wdog_bark",
+ &wdt->wdd);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to request irq\n");
+ return ret;
+ }
+ }
+
+ if (qcom_wdt_info.options & WDIOF_PRETIMEOUT)
+ wdt->wdd.pretimeout = wdt->wdd.timeout - 1;
+
ret = devm_watchdog_register_device(dev, &wdt->wdd);
if (ret)
return ret;
--
2.23.0
[PATCH v2] watchdog: qcom: support pre-timeout when the bark irq is available
Use the bark interrupt as the pre-timeout notifier whenever this
interrupt is available.
By default, the pretimeout notification shall occur one second earlier
than the timeout.
Signed-off-by: Jorge Ramirez-Ortiz
---
drivers/watchdog/qcom-wdt.c | 63 ++---
1 file changed, 58 insertions(+), 5 deletions(-)
diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
index 7be7f87be28f..2dd36914aa82 100644
--- a/drivers/watchdog/qcom-wdt.c
+++ b/drivers/watchdog/qcom-wdt.c
@@ -10,6 +10,8 @@
#include
#include
#include
+#include
+#include
enum wdt_reg {
WDT_RST,
@@ -41,6 +43,7 @@ struct qcom_wdt {
unsigned long rate;
void __iomem*base;
const u32 *layout;
+ const struct device *dev;
};
static void __iomem *wdt_addr(struct qcom_wdt *wdt, enum wdt_reg reg)
@@ -54,15 +57,37 @@ struct qcom_wdt *to_qcom_wdt(struct watchdog_device *wdd)
return container_of(wdd, struct qcom_wdt, wdd);
}
+static inline int qcom_wdt_enable(struct qcom_wdt *wdt)
+{
+ /* enable the bark interrupt */
+ if (wdt->wdd.info->options & WDIOF_PRETIMEOUT)
+ return 3;
+
+ return 1;
+}
+
+static irqreturn_t qcom_wdt_irq(int irq, void *cookie)
+{
+ struct watchdog_device *wdd = (struct watchdog_device *) cookie;
+
+ watchdog_notify_pretimeout(wdd);
+
+ return IRQ_HANDLED;
+}
+
static int qcom_wdt_start(struct watchdog_device *wdd)
{
struct qcom_wdt *wdt = to_qcom_wdt(wdd);
+ unsigned int bark = wdd->pretimeout;
+
+ if (!(wdd->info->options & WDIOF_PRETIMEOUT))
+ bark = wdd->timeout;
writel(0, wdt_addr(wdt, WDT_EN));
writel(1, wdt_addr(wdt, WDT_RST));
- writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
+ writel(bark * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME));
- writel(1, wdt_addr(wdt, WDT_EN));
+ writel(qcom_wdt_enable(wdt), wdt_addr(wdt, WDT_EN));
return 0;
}
@@ -86,9 +111,18 @@ static int qcom_wdt_set_timeout(struct watchdog_device *wdd,
unsigned int timeout)
{
wdd->timeout = timeout;
+
return qcom_wdt_start(wdd);
}
+static int qcom_wdt_set_pretimeout(struct watchdog_device *wdd,
+ unsigned int timeout)
+{
+ wdd->pretimeout = timeout;
+
+ return 0;
+}
+
static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action,
void *data)
{
@@ -105,7 +139,7 @@ static int qcom_wdt_restart(struct watchdog_device *wdd,
unsigned long action,
writel(1, wdt_addr(wdt, WDT_RST));
writel(timeout, wdt_addr(wdt, WDT_BARK_TIME));
writel(timeout, wdt_addr(wdt, WDT_BITE_TIME));
- writel(1, wdt_addr(wdt, WDT_EN));
+ writel(qcom_wdt_enable(wdt), wdt_addr(wdt, WDT_EN));
/*
* Actually make sure the above sequence hits hardware before sleeping.
@@ -121,11 +155,12 @@ static const struct watchdog_ops qcom_wdt_ops = {
.stop = qcom_wdt_stop,
.ping = qcom_wdt_ping,
.set_timeout= qcom_wdt_set_timeout,
+ .set_pretimeout = qcom_wdt_set_pretimeout,
.restart= qcom_wdt_restart,
.owner = THIS_MODULE,
};
-static const struct watchdog_info qcom_wdt_info = {
+static struct watchdog_info qcom_wdt_info = {
.options= WDIOF_KEEPALIVEPING
| WDIOF_MAGICCLOSE
| WDIOF_SETTIMEOUT
@@ -146,7 +181,7 @@ static int qcom_wdt_probe(struct platform_device *pdev)
struct device_node *np = dev->of_node;
const u32 *regs;
u32 percpu_offset;
- int ret;
+ int irq, ret;
regs = of_device_get_match_data(dev);
if (!regs) {
@@ -210,6 +245,7 @@ static int qcom_wdt_probe(struct platform_device *pdev)
wdt->wdd.max_timeout = 0x1000U / wdt->rate;
wdt->wdd.parent = dev;
wdt->layout = regs;
+ wdt->dev = &pdev->dev;
if (readl(wdt_addr(wdt, WDT_STS)) & 1)
wdt->wdd.bootstatus = WDIOF_CARDRESET;
@@ -222,6 +258,23 @@ static int qcom_wdt_probe(struct platform_device *pdev)
wdt->wdd.timeout = min(wdt->wdd.max_timeout, 30U);
watchdog_init_timeout(&wdt->wdd, 0, dev);
+ irq = platform_get_irq(pdev, 0);
+ if (irq >= 0) {
+ /* enable the pre-timeout notification */
+ qcom_wdt_info.options |= WDIOF_PRETIMEOUT;
+
+ ret = devm_request_irq(&pdev->dev, irq, qcom_wdt_irq,
+ IRQF_TRIGGER_RISING, "wdog_bark",
+ &wdt->wdd);
+
[no subject]
Use the bark interrupt as the pre-timeout notifier whenever this
interrupt is available.
By default, the pretimeout notification shall occur one second earlier
than the timeout.
Signed-off-by: Jorge Ramirez-Ortiz
---
drivers/watchdog/qcom-wdt.c | 63 ++---
1 file changed, 58 insertions(+), 5 deletions(-)
diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
index 7be7f87be28f..2dd36914aa82 100644
--- a/drivers/watchdog/qcom-wdt.c
+++ b/drivers/watchdog/qcom-wdt.c
@@ -10,6 +10,8 @@
#include
#include
#include
+#include
+#include
enum wdt_reg {
WDT_RST,
@@ -41,6 +43,7 @@ struct qcom_wdt {
unsigned long rate;
void __iomem*base;
const u32 *layout;
+ const struct device *dev;
};
static void __iomem *wdt_addr(struct qcom_wdt *wdt, enum wdt_reg reg)
@@ -54,15 +57,37 @@ struct qcom_wdt *to_qcom_wdt(struct watchdog_device *wdd)
return container_of(wdd, struct qcom_wdt, wdd);
}
+static inline int qcom_wdt_enable(struct qcom_wdt *wdt)
+{
+ /* enable the bark interrupt */
+ if (wdt->wdd.info->options & WDIOF_PRETIMEOUT)
+ return 3;
+
+ return 1;
+}
+
+static irqreturn_t qcom_wdt_irq(int irq, void *cookie)
+{
+ struct watchdog_device *wdd = (struct watchdog_device *) cookie;
+
+ watchdog_notify_pretimeout(wdd);
+
+ return IRQ_HANDLED;
+}
+
static int qcom_wdt_start(struct watchdog_device *wdd)
{
struct qcom_wdt *wdt = to_qcom_wdt(wdd);
+ unsigned int bark = wdd->pretimeout;
+
+ if (!(wdd->info->options & WDIOF_PRETIMEOUT))
+ bark = wdd->timeout;
writel(0, wdt_addr(wdt, WDT_EN));
writel(1, wdt_addr(wdt, WDT_RST));
- writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
+ writel(bark * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME));
- writel(1, wdt_addr(wdt, WDT_EN));
+ writel(qcom_wdt_enable(wdt), wdt_addr(wdt, WDT_EN));
return 0;
}
@@ -86,9 +111,18 @@ static int qcom_wdt_set_timeout(struct watchdog_device *wdd,
unsigned int timeout)
{
wdd->timeout = timeout;
+
return qcom_wdt_start(wdd);
}
+static int qcom_wdt_set_pretimeout(struct watchdog_device *wdd,
+ unsigned int timeout)
+{
+ wdd->pretimeout = timeout;
+
+ return 0;
+}
+
static int qcom_wdt_restart(struct watchdog_device *wdd, unsigned long action,
void *data)
{
@@ -105,7 +139,7 @@ static int qcom_wdt_restart(struct watchdog_device *wdd,
unsigned long action,
writel(1, wdt_addr(wdt, WDT_RST));
writel(timeout, wdt_addr(wdt, WDT_BARK_TIME));
writel(timeout, wdt_addr(wdt, WDT_BITE_TIME));
- writel(1, wdt_addr(wdt, WDT_EN));
+ writel(qcom_wdt_enable(wdt), wdt_addr(wdt, WDT_EN));
/*
* Actually make sure the above sequence hits hardware before sleeping.
@@ -121,11 +155,12 @@ static const struct watchdog_ops qcom_wdt_ops = {
.stop = qcom_wdt_stop,
.ping = qcom_wdt_ping,
.set_timeout= qcom_wdt_set_timeout,
+ .set_pretimeout = qcom_wdt_set_pretimeout,
.restart= qcom_wdt_restart,
.owner = THIS_MODULE,
};
-static const struct watchdog_info qcom_wdt_info = {
+static struct watchdog_info qcom_wdt_info = {
.options= WDIOF_KEEPALIVEPING
| WDIOF_MAGICCLOSE
| WDIOF_SETTIMEOUT
@@ -146,7 +181,7 @@ static int qcom_wdt_probe(struct platform_device *pdev)
struct device_node *np = dev->of_node;
const u32 *regs;
u32 percpu_offset;
- int ret;
+ int irq, ret;
regs = of_device_get_match_data(dev);
if (!regs) {
@@ -210,6 +245,7 @@ static int qcom_wdt_probe(struct platform_device *pdev)
wdt->wdd.max_timeout = 0x1000U / wdt->rate;
wdt->wdd.parent = dev;
wdt->layout = regs;
+ wdt->dev = &pdev->dev;
if (readl(wdt_addr(wdt, WDT_STS)) & 1)
wdt->wdd.bootstatus = WDIOF_CARDRESET;
@@ -222,6 +258,23 @@ static int qcom_wdt_probe(struct platform_device *pdev)
wdt->wdd.timeout = min(wdt->wdd.max_timeout, 30U);
watchdog_init_timeout(&wdt->wdd, 0, dev);
+ irq = platform_get_irq(pdev, 0);
+ if (irq >= 0) {
+ /* enable the pre-timeout notification */
+ qcom_wdt_info.options |= WDIOF_PRETIMEOUT;
+
+ ret = devm_request_irq(&pdev->dev, irq, qcom_wdt_irq,
+ IRQF_TRIGGER_RISING, "wdog_bark",
+ &wdt->wdd);
+
Re: [PATCH] watchdog: qcom: add support for the bark interrupt
On 9/5/19 18:39, Bjorn Andersson wrote:
> On Thu 05 Sep 09:21 PDT 2019, Jorge Ramirez-Ortiz wrote:
>
>> Use the bark interrupt to notify the bark event. Since the bark and bite
>> timeouts are identical, increase the bite timeout by one second so
>> that the bark event can be logged to the console.
>>
>
> Afaict you should tie the bark to the "pretimeout" in the watchdog
> framework , which would allow the user to specify a pretimeout and
> configure what should happen at the bark (just a pr_alert() or panic()).
yes, you are right. will send v2 based on this.
thanks!
>
> Regards,
> Bjorn
>
>> Signed-off-by: Jorge Ramirez-Ortiz
>> ---
>> drivers/watchdog/qcom-wdt.c | 42 ++---
>> 1 file changed, 39 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
>> index 7be7f87be28f..5eaf92084b93 100644
>> --- a/drivers/watchdog/qcom-wdt.c
>> +++ b/drivers/watchdog/qcom-wdt.c
>> @@ -10,6 +10,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> enum wdt_reg {
>> WDT_RST,
>> @@ -41,6 +42,8 @@ struct qcom_wdt {
>> unsigned long rate;
>> void __iomem*base;
>> const u32 *layout;
>> +unsigned intirq;
>> +const struct device *dev;
>> };
>>
>> static void __iomem *wdt_addr(struct qcom_wdt *wdt, enum wdt_reg reg)
>> @@ -54,15 +57,37 @@ struct qcom_wdt *to_qcom_wdt(struct watchdog_device *wdd)
>> return container_of(wdd, struct qcom_wdt, wdd);
>> }
>>
>> +static inline int qcom_wdt_enable(struct qcom_wdt *wdt)
>> +{
>> +if (wdt->irq < 0)
>> +return 1;
>> +
>> +/* enable timeout with interrupt */
>> +return 3;
>> +}
>> +
>> +static irqreturn_t qcom_wdt_irq(int irq, void *cookie)
>> +{
>> +struct qcom_wdt *wdt = (struct qcom_wdt *) cookie;
>> +
>> +dev_warn(wdt->dev, "barking, one second countdown to reset\n");
>> +
>> +return IRQ_HANDLED;
>> +}
>> +
>> static int qcom_wdt_start(struct watchdog_device *wdd)
>> {
>> struct qcom_wdt *wdt = to_qcom_wdt(wdd);
>> +unsigned int bark, bite;
>> +
>> +bark = wdd->timeout;
>> +bite = wdt->irq < 0 ? bark : bark + 1;
>>
>> writel(0, wdt_addr(wdt, WDT_EN));
>> writel(1, wdt_addr(wdt, WDT_RST));
>> -writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
>> -writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME));
>> -writel(1, wdt_addr(wdt, WDT_EN));
>> +writel(bark * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
>> +writel(bite * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME));
>> +writel(qcom_wdt_enable(wdt), wdt_addr(wdt, WDT_EN));
>> return 0;
>> }
>>
>> @@ -210,10 +235,21 @@ static int qcom_wdt_probe(struct platform_device *pdev)
>> wdt->wdd.max_timeout = 0x1000U / wdt->rate;
>> wdt->wdd.parent = dev;
>> wdt->layout = regs;
>> +wdt->dev = &pdev->dev;
>>
>> if (readl(wdt_addr(wdt, WDT_STS)) & 1)
>> wdt->wdd.bootstatus = WDIOF_CARDRESET;
>>
>> +wdt->irq = platform_get_irq(pdev, 0);
>> +if (wdt->irq >= 0) {
>> +ret = devm_request_irq(&pdev->dev, wdt->irq, qcom_wdt_irq,
>> + IRQF_TRIGGER_RISING, "wdog_bark", wdt);
>> +if (ret) {
>> +dev_err(&pdev->dev, "failed to request irq\n");
>> +return ret;
>> +}
>> +}
>> +
>> /*
>> * If 'timeout-sec' unspecified in devicetree, assume a 30 second
>> * default, unless the max timeout is less than 30 seconds, then use
>> --
>> 2.23.0
>>
>
[PATCH] watchdog: qcom: add support for the bark interrupt
Use the bark interrupt to notify the bark event. Since the bark and bite
timeouts are identical, increase the bite timeout by one second so
that the bark event can be logged to the console.
Signed-off-by: Jorge Ramirez-Ortiz
---
drivers/watchdog/qcom-wdt.c | 42 ++---
1 file changed, 39 insertions(+), 3 deletions(-)
diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
index 7be7f87be28f..5eaf92084b93 100644
--- a/drivers/watchdog/qcom-wdt.c
+++ b/drivers/watchdog/qcom-wdt.c
@@ -10,6 +10,7 @@
#include
#include
#include
+#include
enum wdt_reg {
WDT_RST,
@@ -41,6 +42,8 @@ struct qcom_wdt {
unsigned long rate;
void __iomem*base;
const u32 *layout;
+ unsigned intirq;
+ const struct device *dev;
};
static void __iomem *wdt_addr(struct qcom_wdt *wdt, enum wdt_reg reg)
@@ -54,15 +57,37 @@ struct qcom_wdt *to_qcom_wdt(struct watchdog_device *wdd)
return container_of(wdd, struct qcom_wdt, wdd);
}
+static inline int qcom_wdt_enable(struct qcom_wdt *wdt)
+{
+ if (wdt->irq < 0)
+ return 1;
+
+ /* enable timeout with interrupt */
+ return 3;
+}
+
+static irqreturn_t qcom_wdt_irq(int irq, void *cookie)
+{
+ struct qcom_wdt *wdt = (struct qcom_wdt *) cookie;
+
+ dev_warn(wdt->dev, "barking, one second countdown to reset\n");
+
+ return IRQ_HANDLED;
+}
+
static int qcom_wdt_start(struct watchdog_device *wdd)
{
struct qcom_wdt *wdt = to_qcom_wdt(wdd);
+ unsigned int bark, bite;
+
+ bark = wdd->timeout;
+ bite = wdt->irq < 0 ? bark : bark + 1;
writel(0, wdt_addr(wdt, WDT_EN));
writel(1, wdt_addr(wdt, WDT_RST));
- writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
- writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME));
- writel(1, wdt_addr(wdt, WDT_EN));
+ writel(bark * wdt->rate, wdt_addr(wdt, WDT_BARK_TIME));
+ writel(bite * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME));
+ writel(qcom_wdt_enable(wdt), wdt_addr(wdt, WDT_EN));
return 0;
}
@@ -210,10 +235,21 @@ static int qcom_wdt_probe(struct platform_device *pdev)
wdt->wdd.max_timeout = 0x1000U / wdt->rate;
wdt->wdd.parent = dev;
wdt->layout = regs;
+ wdt->dev = &pdev->dev;
if (readl(wdt_addr(wdt, WDT_STS)) & 1)
wdt->wdd.bootstatus = WDIOF_CARDRESET;
+ wdt->irq = platform_get_irq(pdev, 0);
+ if (wdt->irq >= 0) {
+ ret = devm_request_irq(&pdev->dev, wdt->irq, qcom_wdt_irq,
+ IRQF_TRIGGER_RISING, "wdog_bark", wdt);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to request irq\n");
+ return ret;
+ }
+ }
+
/*
* If 'timeout-sec' unspecified in devicetree, assume a 30 second
* default, unless the max timeout is less than 30 seconds, then use
--
2.23.0
Re: [PATCH 1/5] clk: qcom: gcc: limit GPLL0_AO_OUT operating frequency
On 8/26/19 18:45, Jorge Ramirez-Ortiz wrote:
> Limit the GPLL0_AO_OUT_MAIN operating frequency as per its hardware
> specifications.
>
> Co-developed-by: Niklas Cassel
> Signed-off-by: Niklas Cassel
> Signed-off-by: Jorge Ramirez-Ortiz
> Reviewed-by: Bjorn Andersson
> Acked-by: Stephen Boyd
> ---
> drivers/clk/qcom/clk-alpha-pll.c | 8
> drivers/clk/qcom/clk-alpha-pll.h | 1 +
> drivers/clk/qcom/gcc-qcs404.c| 2 +-
> 3 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/qcom/clk-alpha-pll.c
> b/drivers/clk/qcom/clk-alpha-pll.c
> index 055318f97991..9228b7b1f56e 100644
> --- a/drivers/clk/qcom/clk-alpha-pll.c
> +++ b/drivers/clk/qcom/clk-alpha-pll.c
> @@ -878,6 +878,14 @@ static long clk_trion_pll_round_rate(struct clk_hw *hw,
> unsigned long rate,
> return clamp(rate, min_freq, max_freq);
> }
>
> +const struct clk_ops clk_alpha_pll_fixed_ops = {
> + .enable = clk_alpha_pll_enable,
> + .disable = clk_alpha_pll_disable,
> + .is_enabled = clk_alpha_pll_is_enabled,
> + .recalc_rate = clk_alpha_pll_recalc_rate,
> +};
> +EXPORT_SYMBOL_GPL(clk_alpha_pll_fixed_ops);
> +
> const struct clk_ops clk_alpha_pll_ops = {
> .enable = clk_alpha_pll_enable,
> .disable = clk_alpha_pll_disable,
> diff --git a/drivers/clk/qcom/clk-alpha-pll.h
> b/drivers/clk/qcom/clk-alpha-pll.h
> index 15f27f4b06df..c28eb1a08c0c 100644
> --- a/drivers/clk/qcom/clk-alpha-pll.h
> +++ b/drivers/clk/qcom/clk-alpha-pll.h
> @@ -109,6 +109,7 @@ struct alpha_pll_config {
> };
>
> extern const struct clk_ops clk_alpha_pll_ops;
> +extern const struct clk_ops clk_alpha_pll_fixed_ops;
> extern const struct clk_ops clk_alpha_pll_hwfsm_ops;
> extern const struct clk_ops clk_alpha_pll_postdiv_ops;
> extern const struct clk_ops clk_alpha_pll_huayra_ops;
> diff --git a/drivers/clk/qcom/gcc-qcs404.c b/drivers/clk/qcom/gcc-qcs404.c
> index e12c04c09a6a..567140709c7d 100644
> --- a/drivers/clk/qcom/gcc-qcs404.c
> +++ b/drivers/clk/qcom/gcc-qcs404.c
> @@ -330,7 +330,7 @@ static struct clk_alpha_pll gpll0_ao_out_main = {
> .parent_names = (const char *[]){ "cxo" },
> .num_parents = 1,
> .flags = CLK_IS_CRITICAL,
> - .ops = &clk_alpha_pll_ops,
> + .ops = &clk_alpha_pll_fixed_ops,
> },
> },
> };
>
just a quick follow up, is this series being picked-up?
Re: [PATCH 1/6] dt-bindings: mailbox: qcom: Add clock-name optional property
On 8/26/19 18:48, Jorge Ramirez-Ortiz wrote:
> When the APCS clock is registered (platform dependent), it retrieves
> its parent names from hardcoded values in the driver.
>
> The following commit allows the DT node to provide such clock names to
> the platform data based clock driver therefore avoiding having to
> explicitly embed those names in the clock driver source code.
>
> Co-developed-by: Niklas Cassel
> Signed-off-by: Niklas Cassel
> Signed-off-by: Jorge Ramirez-Ortiz
> Reviewed-by: Rob Herring
> Reviewed-by: Bjorn Andersson
> ---
> .../mailbox/qcom,apcs-kpss-global.txt | 24 ---
> 1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git
> a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt
> b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt
> index 1232fc9fc709..b69310322b09 100644
> --- a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt
> +++ b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt
> @@ -18,10 +18,11 @@ platforms.
> Usage: required
> Value type:
> Definition: must specify the base address and size of the global block
> +
> - clocks:
> - Usage: required if #clocks-cells property is present
> - Value type:
> - Definition: phandle to the input PLL, which feeds the APCS mux/divider
> + Usage: required if #clock-names property is present
> + Value type:
> + Definition: phandles to the two parent clocks of the clock driver.
>
> - #mbox-cells:
> Usage: required
> @@ -33,6 +34,12 @@ platforms.
> Value type:
> Definition: as described in clock.txt, must be 0
>
> +- clock-names:
> + Usage: required if the platform data based clock driver needs to
> + retrieve the parent clock names from device tree.
> + This will requires two mandatory clocks to be defined.
> + Value type:
> + Definition: must be "aux" and "pll"
>
> = EXAMPLE
> The following example describes the APCS HMSS found in MSM8996 and part of
> the
> @@ -65,3 +72,14 @@ Below is another example of the APCS binding on MSM8916
> platforms:
> clocks = <&a53pll>;
> #clock-cells = <0>;
> };
> +
> +Below is another example of the APCS binding on QCS404 platforms:
> +
> + apcs_glb: mailbox@b011000 {
> + compatible = "qcom,qcs404-apcs-apps-global", "syscon";
> + reg = <0x0b011000 0x1000>;
> + #mbox-cells = <1>;
> + clocks = <&gcc GCC_GPLL0_AO_OUT_MAIN>, <&apcs_hfpll>;
> + clock-names = "aux", "pll";
> + #clock-cells = <0>;
> + };
>
just a quick follow up, is this series being picked-up?
Re: [PATCH v4 3/4] dt-bindings: Add Qualcomm USB SuperSpeed PHY bindings
On 9/4/19 01:34, Bjorn Andersson wrote:
> On Tue 03 Sep 14:45 PDT 2019, Stephen Boyd wrote:
>
>> Quoting Jack Pham (2019-09-03 10:39:24)
>>> On Mon, Sep 02, 2019 at 08:23:04AM +0200, Jorge Ramirez wrote:
>>>> On 8/30/19 20:28, Stephen Boyd wrote:
>>>>> Quoting Bjorn Andersson (2019-08-30 09:45:20)
>>>>>> On Fri 30 Aug 09:01 PDT 2019, Stephen Boyd wrote:
>>>>>>
>>>>>>>>>
>>>>>>>>> The USB-C connector is attached both to the HS and SS PHYs, so I think
>>>>>>>>> you should represent this external to this node and use of_graph to
>>>>>>>>> query it.
>>>>>>>>
>>>>>>>> but AFAICS we wont be able to retrieve the vbux-supply from an external
>>>>>>>> node (that interface does not exist).
>>>>>>>>
>>>>>>>> rob, do you have a suggestion?
>>>>>>>
>>>>>>> Shouldn't the vbus supply be in the phy? Or is this a situation where
>>>>>>> the phy itself doesn't have the vbus supply going to it because the PMIC
>>>>>>> gets in the way and handles the vbus for the connector by having the SoC
>>>>>>> communicate with the PMIC about when to turn the vbus on and off, etc?
>>>>>>>
>>>>>>
>>>>>> That's correct, the VBUS comes out of the PMIC and goes directly to the
>>>>>> connector.
>>>>>>
>>>>>> The additional complicating factor here is that the connector is wired
>>>>>> to a USB2 phy as well, so we need to wire up detection and vbus control
>>>>>> to both of them - but I think this will be fine, if we can only figure
>>>>>> out a sane way of getting hold of the vbus-supply.
>>>>>>
>>>>>
>>>>> Does it really matter to describe this situation though? Maybe it's
>>>>> simpler to throw the vbus supply into the phy and control it from the
>>>>> phy driver, even if it never really goes there. Or put it into the
>>>>> toplevel usb controller?
>>>>>
>>>> that would work for me - the connector definition seemed a better way to
>>>> explain the connectivity but since we cant retrieve the supply from the
>>>> external node is not of much functional use.
>>>>
>>>> but please let me know how to proceed. shall I add the supply back to
>>>> the phy?
>>
>> So does the vbus actually go to the phy? I thought it never went there
>> and the power for the phy was different (and possibly lower in voltage).
>>
>
> No, the PHYs use different - lower voltage - supplies to operate. VBUS
> is coming from a 5V supply straight to the connector and plug-detect
> logic (which is passive in this design).
>
>>>
>>> Putting it in the toplevel usb node makes sense to me, since that's
>>> usually the driver that knows when it's switching into host mode and
>>> needs to turn on VBUS. The dwc3-qcom driver & bindings currently don't
>>> do this but there's precedent in a couple of the other dwc3 "glues"--see
>>> Documentation/devicetree/bindings/usb/{amlogic\,dwc3,omap-usb}.txt
>>>
>>> One exception is if the PMIC is also USB-PD capable and can do power
>>> role swap, in which case the VBUS control needs to be done by the TCPM,
>>> so that'd be a case where having vbus-supply in the connector node might
>>> make more sense.
>>>
>>
>> The other way is to implement the code to get the vbus supply out of a
>> connector. Then any driver can do the work if it knows it needs to and
>> we don't have to care that the vbus isn't going somewhere. I suppose
>> that would need an of_regulator_get() sort of API that can get the
>> regulator out of there? Or to make the connector into a struct device
>> that can get the regulator out per some generic connector driver and
>> then pass it through to the USB controller when it asks for it. Maybe
>> try to prototype that out?
>>
>
> The examples given in the DT bindings describes the connector as a child
> of a PMIC, with of_graph somehow tying it to the various inputs. But in
> these examples vbus is handled by implicitly inside the MFD, where
> extcon is informed about the plug event they toggle vbus as well.
>
> In our case we have a extcon-usb-gpio to detect mode, which per Jorge's
> proposal will trickle down to the PHY and become a regulator calls on
> either some external regulator or more typically one of the chargers in
> the system.
>
>
> So if we come up with a struct device for the connector and some API for
> toggling the vbus we're going to have to fairly abstract entities
> representing pretty much the same thing - and in a design with a mux we
> would have a different setup.
I am a bit unclear - not sure if we have gone full circle on this
subject. what is then the direction to get this merged?
I did have look last week and the level of effort to support regulators
on external nodes is not neglectable meaning that I might not have the
time to deliver that feature (perhaps someone else wishes to take over?)
>
> Regards,
> Bjorn
>
Re: [PATCH v4 3/4] dt-bindings: Add Qualcomm USB SuperSpeed PHY bindings
On 8/30/19 20:28, Stephen Boyd wrote:
> Quoting Bjorn Andersson (2019-08-30 09:45:20)
>> On Fri 30 Aug 09:01 PDT 2019, Stephen Boyd wrote:
>>
>>> Quoting Jorge Ramirez (2019-08-29 00:03:48)
>>>> On 2/23/19 17:52, Bjorn Andersson wrote:
>>>>> On Thu 07 Feb 03:17 PST 2019, Jorge Ramirez-Ortiz wrote:
>>>>>> +
>>>>>> +Required child nodes:
>>>>>> +
>>>>>> +- usb connector node as defined in bindings/connector/usb-connector.txt
>>>>>> + containing the property vbus-supply.
>>>>>> +
>>>>>> +Example:
>>>>>> +
>>>>>> +usb3_phy: usb3-phy@78000 {
>>>>>> +compatible = "qcom,snps-usb-ssphy";
>>>>>> +reg = <0x78000 0x400>;
>>>>>> +#phy-cells = <0>;
>>>>>> +clocks = <&rpmcc RPM_SMD_LN_BB_CLK>,
>>>>>> + <&gcc GCC_USB_HS_PHY_CFG_AHB_CLK>,
>>>>>> + <&gcc GCC_USB3_PHY_PIPE_CLK>;
>>>>>> +clock-names = "ref", "phy", "pipe";
>>>>>> +resets = <&gcc GCC_USB3_PHY_BCR>,
>>>>>> + <&gcc GCC_USB3PHY_PHY_BCR>;
>>>>>> +reset-names = "com", "phy";
>>>>>> +vdd-supply = <&vreg_l3_1p05>;
>>>>>> +vdda1p8-supply = <&vreg_l5_1p8>;
>>>>>> +usb3_c_connector: usb3-c-connector {
>>>
>>> Node name should be 'connector', not usb3-c-connector.
>>>
>>
>> It probably has to be usb-c-connector, because we have a
>> micro-usb-connector on the same board.
>
> Ok. Or connector@1 and connector@2? Our toplevel node container story is
> sort of sad because we have to play tricks with node names. But in the
> example, just connector I presume?
>
>>
>>>>>
>>>>> The USB-C connector is attached both to the HS and SS PHYs, so I think
>>>>> you should represent this external to this node and use of_graph to
>>>>> query it.
>>>>
>>>> but AFAICS we wont be able to retrieve the vbux-supply from an external
>>>> node (that interface does not exist).
>>>>
>>>> rob, do you have a suggestion?
>>>
>>> Shouldn't the vbus supply be in the phy? Or is this a situation where
>>> the phy itself doesn't have the vbus supply going to it because the PMIC
>>> gets in the way and handles the vbus for the connector by having the SoC
>>> communicate with the PMIC about when to turn the vbus on and off, etc?
>>>
>>
>> That's correct, the VBUS comes out of the PMIC and goes directly to the
>> connector.
>>
>> The additional complicating factor here is that the connector is wired
>> to a USB2 phy as well, so we need to wire up detection and vbus control
>> to both of them - but I think this will be fine, if we can only figure
>> out a sane way of getting hold of the vbus-supply.
>>
>
> Does it really matter to describe this situation though? Maybe it's
> simpler to throw the vbus supply into the phy and control it from the
> phy driver, even if it never really goes there. Or put it into the
> toplevel usb controller?
>
that would work for me - the connector definition seemed a better way to
explain the connectivity but since we cant retrieve the supply from the
external node is not of much functional use.
but please let me know how to proceed. shall I add the supply back to
the phy?
[PATCH 2/2] arm64: dts: qcom: qcs404: add the watchdog node
Allows QCS404 based designs to enable watchdog support
Signed-off-by: Jorge Ramirez-Ortiz
---
arch/arm64/boot/dts/qcom/qcs404.dtsi | 6 ++
1 file changed, 6 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi
b/arch/arm64/boot/dts/qcom/qcs404.dtsi
index 131d8046d3be..17d4dd54c53a 100644
--- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
+++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
@@ -875,6 +875,12 @@
#mbox-cells = <1>;
};
+ watchdog@b017000 {
+ compatible = "qcom,kpss-wdt";
+ reg = <0x0b017000 0x1000>;
+ clocks = <&sleep_clk>;
+ };
+
timer@b12 {
#address-cells = <1>;
#size-cells = <1>;
--
2.22.0
[PATCH 1/2] arm64: dts: qcom: qcs404: add sleep clk fixed rate oscillator
This fixed rate clock is required for the operation of some devices
(ie watchdog).
Signed-off-by: Jorge Ramirez-Ortiz
---
arch/arm64/boot/dts/qcom/qcs404.dtsi | 6 ++
1 file changed, 6 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi
b/arch/arm64/boot/dts/qcom/qcs404.dtsi
index a97eeb4569c0..131d8046d3be 100644
--- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
+++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
@@ -22,6 +22,12 @@
#clock-cells = <0>;
clock-frequency = <1920>;
};
+
+ sleep_clk: sleep-clk {
+ compatible = "fixed-clock";
+ #clock-cells = <0>;
+ clock-frequency = <32768>;
+ };
};
cpus {
--
2.22.0
Re: [PATCH 1/2] mbox: qcom: add APCS child device for QCS404
On 8/29/19 00:31, Stephen Boyd wrote:
> Quoting Jorge Ramirez-Ortiz (2019-08-26 09:46:24)
>> There is clock controller functionality in the APCS hardware block of
>> qcs404 devices similar to msm8916.
>>
>> Co-developed-by: Niklas Cassel
>> Signed-off-by: Niklas Cassel
>> Signed-off-by: Jorge Ramirez-Ortiz
>> Reviewed-by: Bjorn Andersson
>> ---
>> drivers/mailbox/qcom-apcs-ipc-mailbox.c | 8 +---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
>> b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
>> index 705e17a5479c..76e1ad433b3f 100644
>> --- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
>> +++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
>> @@ -89,7 +89,11 @@ static int qcom_apcs_ipc_probe(struct platform_device
>> *pdev)
>> return ret;
>> }
>>
>> - if (of_device_is_compatible(np, "qcom,msm8916-apcs-kpss-global")) {
>> + platform_set_drvdata(pdev, apcs);
>
> Why did this move? It's required in the child driver or something now?
> Is it a Fixes sort of change?
>
>> +
>> + if (of_device_is_compatible(np, "qcom,msm8916-apcs-kpss-global") ||
>> + of_device_is_compatible(np, "qcom,qcs404-apcs-apps-global")) {
>
> Maybe this should be a compatible list instead of two calls to
> of_device_is_compatible().
>
>> +
>> apcs->clk = platform_device_register_data(&pdev->dev,
>>
>> "qcom-apcs-msm8916-clk",
>> -1, NULL, 0);
>> @@ -97,8 +101,6 @@ static int qcom_apcs_ipc_probe(struct platform_device
>> *pdev)
>> dev_err(&pdev->dev, "failed to register APCS clk\n");
>> }
>>
>> - platform_set_drvdata(pdev, apcs);
>> -
>> return 0;
>> }
>>
>
thanks stephen. posted v2
[PATCH v2 2/2] mbox: qcom: replace integer with valid macro
Use the correct macro when registering the platform device.
Co-developed-by: Niklas Cassel
Signed-off-by: Niklas Cassel
Signed-off-by: Jorge Ramirez-Ortiz
Reviewed-by: Bjorn Andersson
---
drivers/mailbox/qcom-apcs-ipc-mailbox.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
index d3676fd3cf94..7870edb4405b 100644
--- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
+++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
@@ -96,7 +96,8 @@ static int qcom_apcs_ipc_probe(struct platform_device *pdev)
if (of_match_device(apcs_clk_match_table, &pdev->dev)) {
apcs->clk = platform_device_register_data(&pdev->dev,
"qcom-apcs-msm8916-clk",
- -1, NULL, 0);
+ PLATFORM_DEVID_NONE,
+ NULL, 0);
if (IS_ERR(apcs->clk))
dev_err(&pdev->dev, "failed to register APCS clk\n");
}
--
2.22.0
[PATCH v2 1/2] mbox: qcom: add APCS child device for QCS404
There is clock controller functionality in the APCS hardware block of
qcs404 devices similar to msm8916.
Co-developed-by: Niklas Cassel
Signed-off-by: Niklas Cassel
Signed-off-by: Jorge Ramirez-Ortiz
---
drivers/mailbox/qcom-apcs-ipc-mailbox.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
index 705e17a5479c..d3676fd3cf94 100644
--- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
+++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
@@ -47,7 +47,6 @@ static const struct mbox_chan_ops qcom_apcs_ipc_ops = {
static int qcom_apcs_ipc_probe(struct platform_device *pdev)
{
- struct device_node *np = pdev->dev.of_node;
struct qcom_apcs_ipc *apcs;
struct regmap *regmap;
struct resource *res;
@@ -55,6 +54,11 @@ static int qcom_apcs_ipc_probe(struct platform_device *pdev)
void __iomem *base;
unsigned long i;
int ret;
+ const struct of_device_id apcs_clk_match_table[] = {
+ { .compatible = "qcom,msm8916-apcs-kpss-global", },
+ { .compatible = "qcom,qcs404-apcs-apps-global", },
+ {}
+ };
apcs = devm_kzalloc(&pdev->dev, sizeof(*apcs), GFP_KERNEL);
if (!apcs)
@@ -89,7 +93,7 @@ static int qcom_apcs_ipc_probe(struct platform_device *pdev)
return ret;
}
- if (of_device_is_compatible(np, "qcom,msm8916-apcs-kpss-global")) {
+ if (of_match_device(apcs_clk_match_table, &pdev->dev)) {
apcs->clk = platform_device_register_data(&pdev->dev,
"qcom-apcs-msm8916-clk",
-1, NULL, 0);
--
2.22.0
[PATCH v2 0/2] mbox changes for QCS404 DVFS
These are the mailbox changes required to enable CPU frequency scaling on Qualcomm's QCS404. v2: sboyd review replace if statement with a of_match_device dont modify platform_set_drvdata Jorge Ramirez-Ortiz (2): mbox: qcom: add APCS child device for QCS404 mbox: qcom: replace integer with valid macro drivers/mailbox/qcom-apcs-ipc-mailbox.c | 11 --- 1 file changed, 8 insertions(+), 3 deletions(-) -- 2.22.0
Re: [PATCH v4 3/4] dt-bindings: Add Qualcomm USB SuperSpeed PHY bindings
On 2/23/19 17:52, Bjorn Andersson wrote:
> On Thu 07 Feb 03:17 PST 2019, Jorge Ramirez-Ortiz wrote:
>
>> Binding description for Qualcomm's Synopsys 1.0.0 SuperSpeed phy
>> controller embedded in QCS404.
>>
>> Based on Sriharsha Allenki's original
>> definitions.
>>
>> Signed-off-by: Jorge Ramirez-Ortiz
>> ---
>> .../bindings/phy/qcom,snps-usb-ssphy.txt | 79 +++
>> 1 file changed, 79 insertions(+)
>> create mode 100644
>> Documentation/devicetree/bindings/phy/qcom,snps-usb-ssphy.txt
>>
>> diff --git a/Documentation/devicetree/bindings/phy/qcom,snps-usb-ssphy.txt
>> b/Documentation/devicetree/bindings/phy/qcom,snps-usb-ssphy.txt
>> new file mode 100644
>> index ..354e6f9cef62
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/qcom,snps-usb-ssphy.txt
>> @@ -0,0 +1,79 @@
>> +Qualcomm Synopsys 1.0.0 SS phy controller
>> +===
>> +
>> +Qualcomm 1.0.0 SS phy controller supports SuperSpeed USB connectivity on
>> +some Qualcomm platforms.
>> +
>> +Required properties:
>> +
>> +- compatible:
>> +Value type:
>> +Definition: Should contain "qcom,snps-usb-ssphy".
>
> Per Rob's request make this:
>
> Should contain "qcom,qcs404-snps-usb-ssphy" and "qcom,snps-usb-ssphy"
ok
>
> You can then leave the driver matching on qcom,snps-usb-ssphy for now
> and if we ever find this to be incompatible with other platforms we can
> make the driver match on the platform-specific compatible.
ok
>
>> +
>> +- reg:
>> +Value type:
>> +Definition: USB PHY base address and length of the register map.
>> +
>> +- #phy-cells:
>> +Value type:
>> +Definition: Should be 0. See phy/phy-bindings.txt for details.
>> +
>> +- clocks:
>> +Value type:
>> +Definition: See clock-bindings.txt section "consumers". List of
>> + three clock specifiers for reference, phy core and
>> + pipe clocks.
>> +
>> +- clock-names:
>> +Value type:
>> +Definition: Names of the clocks in 1-1 correspondence with the "clocks"
>> + property. Must contain "ref", "phy" and "pipe".
>> +
>> +- vdd-supply:
>> +Value type:
>> +Definition: phandle to the regulator VDD supply node.
>> +
>> +- vdda1p8-supply:
>> +Value type:
>> +Definition: phandle to the regulator 1.8V supply node.
>> +
>> +Optional properties:
>> +
>> +- resets:
>> +Value type:
>> +Definition: See reset.txt section "consumers". Specifiers for COM and
>> + PHY resets.
>> +
>> +- reset-names:
>> +Value type:
>> +Definition: Names of the resets in 1-1 correspondence with the "resets"
>> + property. Must contain "com" and "phy" if the property is
>> + specified.
>> +
>> +Required child nodes:
>> +
>> +- usb connector node as defined in bindings/connector/usb-connector.txt
>> + containing the property vbus-supply.
>> +
>> +Example:
>> +
>> +usb3_phy: usb3-phy@78000 {
>> +compatible = "qcom,snps-usb-ssphy";
>> +reg = <0x78000 0x400>;
>> +#phy-cells = <0>;
>> +clocks = <&rpmcc RPM_SMD_LN_BB_CLK>,
>> + <&gcc GCC_USB_HS_PHY_CFG_AHB_CLK>,
>> + <&gcc GCC_USB3_PHY_PIPE_CLK>;
>> +clock-names = "ref", "phy", "pipe";
>> +resets = <&gcc GCC_USB3_PHY_BCR>,
>> + <&gcc GCC_USB3PHY_PHY_BCR>;
>> +reset-names = "com", "phy";
>> +vdd-supply = <&vreg_l3_1p05>;
>> +vdda1p8-supply = <&vreg_l5_1p8>;
>> +usb3_c_connector: usb3-c-connector {
>
> The USB-C connector is attached both to the HS and SS PHYs, so I think
> you should represent this external to this node and use of_graph to
> query it.
but AFAICS we wont be able to retrieve the vbux-supply from an external
node (that interface does not exist).
rob, do you have a suggestion?
>
> So the connector should look similar to example 2 in
> connector/usb-connector.txt.
>
> Regards,
> Bjorn
>
>> +compatible = "usb-c-connector";
>> +label = "USB-C";
>> +type = "micro";
>> +vbus-supply = <&usb3_vbus_reg>;
>> +};
>> +};
>> --
>> 2.20.1
>>
>
Re: [PATCH 4/5] misc: fastrpc: fix double refcounting on dmabuf
On 8/28/19 10:48, Srinivas Kandagatla wrote: > > > On 28/08/2019 08:50, Jorge Ramirez wrote: >> On 8/27/19 23:45, Srinivas Kandagatla wrote: >>> On 23/08/2019 16:23, Jorge Ramirez-Ortiz wrote: >>>> can you add me as a co-author to this patch please? >>> No problem I can do that if you feel so! >> yes please. thanks! >> >>>> since I spent about a day doing the analysis, sent you a fix that >>>> maintained the API used by the library and explained you how to >>>> reproduce the issue I think it is just fair. > the fact that the api >>>> could be be modified and the fix be done a bit >>>> differently- free dma buf ioctl removed- seems just a minor >>>> implementation detail to me. >>> No, that's not true, this is a clear fastrpc design issue. >> IMO the ioctls defines the contract with userspace and the contract >> establishes that userspace must call deallocate. the kernel wrongly >> implemented to that contract since it doesn't handle the cases where >> userspace can't send the release calls which leads to memory leaks. this >> is what I meant by and implementation issue. >> >> if we had many fastrpc users, rolling out the design change that you >> propose - removing an ioctl- would definitively have an impact. But >> since that is not yet the case, there is not doubt that your patch makes >> more sense. > > Exactly before it make a way into other projects! > >> >> but my point was that there is not a huge gap in efforts between doing >> one or the other. > > Thats not the point, point is about right fix! but I disagree with you about what 'right' means. in this context, for you "right" meant potentially breaking some users and implement the best possible kernel design. for me, it meant continue to obey at the agreed ioctl interface to not disturb the users. but as I said, since there is not a significant pool of fastrpc users, breaking backward compatibility with the fastrpc library is not important hence why I agree that removing the ioctl was the better choice (on this particular case). > >> >>> Userspace is already doing a refcount via mmap/unmap on that dmabuf fd, >>> having an additional api adds another level of refcount which is totally >>> redundant and is the root cause for this leak. >> yes it is redundant but is not the root cause for this leak. the root >> cause is that the driver doesnt handle the case where userspace didnt or >> was not able to call release (and that is no more than adding allocated >> buffers to a list and clean on exit) > > I don't agree with you on that. We should not take an extra refcount in > first place in driver. of course taking an extra refcount is functionally pointless. But that was a design decision that imposed something on the user. and the kernel can certainly work with that 'silly' design decision by tracking the memory in the driver. is the right thing to do to keep less than ideal designs in the kernel to support agreed user interface? IMO it depends on the use case to my eyes the design was obviously wrong, I never questioned that...that was very clear when I started tracing the code. perhaps, rather than work around it, I should have considered that removing the ioctl wouldnt be a big deal to anyone. so I would have send two patches instead of the one I sent you 1. fix leak (keep track of allocated dma buffers and make sure everything is released on exit) 2. remove unnecessary ioctl warning users. > > let me explain it one more time! cmon, I did understand it before we engaged in this discussion :) > > dmabuf has to be mmaped in userspace app before it is used, and > "Memory mappings that were created in the process shall be unmapped > before the process is destroyed" so the refcount is taken care by > mmap/unmap automatically. I would like to leave the discussion here if that is ok with you (I clearly understand your POV but I feel I am not doing a good job at sharing my thoughts...we can do that offline if you want) > > --srini > > >> >
Re: [PATCH 4/5] misc: fastrpc: fix double refcounting on dmabuf
On 8/27/19 23:45, Srinivas Kandagatla wrote: > > On 23/08/2019 16:23, Jorge Ramirez-Ortiz wrote: >> can you add me as a co-author to this patch please? > > No problem I can do that if you feel so! yes please. thanks! > >> since I spent about a day doing the analysis, sent you a fix that >> maintained the API used by the library and explained you how to >> reproduce the issue I think it is just fair. > the fact that the api >> could be be modified and the fix be done a bit >> differently- free dma buf ioctl removed- seems just a minor >> implementation detail to me. > > No, that's not true, this is a clear fastrpc design issue. IMO the ioctls defines the contract with userspace and the contract establishes that userspace must call deallocate. the kernel wrongly implemented to that contract since it doesn't handle the cases where userspace can't send the release calls which leads to memory leaks. this is what I meant by and implementation issue. if we had many fastrpc users, rolling out the design change that you propose - removing an ioctl- would definitively have an impact. But since that is not yet the case, there is not doubt that your patch makes more sense. but my point was that there is not a huge gap in efforts between doing one or the other. > > Userspace is already doing a refcount via mmap/unmap on that dmabuf fd, > having an additional api adds another level of refcount which is totally > redundant and is the root cause for this leak. yes it is redundant but is not the root cause for this leak. the root cause is that the driver doesnt handle the case where userspace didnt or was not able to call release (and that is no more than adding allocated buffers to a list and clean on exit) > > > --srini
Re: [PATCH v4 00/13] Support CPU frequency scaling on QCS404
On 8/26/19 08:54, Jorge Ramirez wrote: > On 7/31/19 22:29, Jorge Ramirez-Ortiz wrote: >> The following patchset enables CPU frequency scaling support on the >> QCS404 (with dynamic voltage scaling). >> >> It is important to notice that this functionality will be superseded >> by Core Power Reduction (CPR), a more accurate form of AVS found on >> certain Qualcomm SoCs. >> >> Some of the changes required to support CPR do conflict with the >> configuration required for CPUFreq. >> >> In particular, the following commit for CPR - already merged - will >> need to be reverted in order to enable CPUFreq. >> >>Author: Jorge Ramirez-Ortiz >>Date: Thu Jul 25 12:41:36 2019 +0200 >>cpufreq: Add qcs404 to cpufreq-dt-platdev blacklist >> >> Patch 8 "clk: qcom: hfpll: CLK_IGNORE_UNUSED" is a bit controversial; >> in this platform, this PLL provides the clock signal to a CPU >> core. But in others it might not. >> >> We opted for the minimal ammount of changes without affecting the >> default functionality: simply bypassing the COMMON_CLK_DISABLE_UNUSED >> framework and letting the firwmare chose whether to enable or disable >> the clock at boot. However maybe a DT property and marking the clock >> as critical would be more appropriate for this PLL. we'd appreciate the >> maintainer's input on this topic. >> >> v2: >>- dts: ms8916: apcs mux/divider: new bindings >> (the driver can still support the old bindings) >> >>- qcs404.dtsi >> fix apcs-hfpll definition >> fix cpu_opp_table definition >> >>- GPLL0_AO_OUT operating frequency >> define new alpha_pll_fixed_ops to limit the operating frequency >> >> v3: >> - qcom-apcs-ipc-mailbox >> replace goto to ease readability >> >> - apcs-msm8916.c >> rework patch to use of_clk_parent_fill >> >> - hfpll.c >> add relevant comments to the code >> >> - qcs404.dtsi >> add voltage scaling support >> >> v4: >> - squash OPP definition and DVFS enablement in dts >>(patches 10 and 13 in previous version) >> >> - qcom-apcs-ipc-mailbox >>replace return condition for readability >> >> - answer one question on CLK_IGNORE_UNUSED in mailing list >> >> Jorge Ramirez-Ortiz, Niklas Cassel (13): >> clk: qcom: gcc: limit GPLL0_AO_OUT operating frequency >> mbox: qcom: add APCS child device for QCS404 >> mbox: qcom: replace integer with valid macro >> dt-bindings: mailbox: qcom: Add clock-name optional property >> clk: qcom: apcs-msm8916: get parent clock names from DT >> clk: qcom: hfpll: get parent clock names from DT >> clk: qcom: hfpll: register as clock provider >> clk: qcom: hfpll: CLK_IGNORE_UNUSED >> arm64: dts: qcom: msm8916: Add the clocks for the APCS mux/divider >> arm64: dts: qcom: qcs404: Add HFPLL node >> arm64: dts: qcom: qcs404: Add the clocks for APCS mux/divider >> arm64: dts: qcom: qcs404: Add DVFS support >> arm64: defconfig: Enable HFPLL >> >> .../mailbox/qcom,apcs-kpss-global.txt | 24 +-- >> arch/arm64/boot/dts/qcom/msm8916.dtsi | 3 +- >> arch/arm64/boot/dts/qcom/qcs404.dtsi | 43 +++ >> arch/arm64/configs/defconfig | 1 + >> drivers/clk/qcom/apcs-msm8916.c | 23 -- >> drivers/clk/qcom/clk-alpha-pll.c | 8 >> drivers/clk/qcom/clk-alpha-pll.h | 1 + >> drivers/clk/qcom/gcc-qcs404.c | 2 +- >> drivers/clk/qcom/hfpll.c | 25 ++- >> drivers/mailbox/qcom-apcs-ipc-mailbox.c | 11 +++-- >> 10 files changed, 128 insertions(+), 13 deletions(-) >> > > any feedback on this set? > > TIA > trying to ease the maintainers task, I have resent the series split in three individual sets: - device tree - clk - mbox for full functionality obviously all of them are required please let me know if there is anything else I can do.
[PATCH 3/6] arm64: dts: qcom: qcs404: Add HFPLL node
The high frequency pll functionality is required to enable CPU
frequency scaling operation.
Co-developed-by: Niklas Cassel
Signed-off-by: Niklas Cassel
Signed-off-by: Jorge Ramirez-Ortiz
Reviewed-by: Bjorn Andersson
---
arch/arm64/boot/dts/qcom/qcs404.dtsi | 9 +
1 file changed, 9 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi
b/arch/arm64/boot/dts/qcom/qcs404.dtsi
index a97eeb4569c0..75ea356a3fb0 100644
--- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
+++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
@@ -869,6 +869,15 @@
#mbox-cells = <1>;
};
+ apcs_hfpll: clock-controller@b016000 {
+ compatible = "qcom,hfpll";
+ reg = <0x0b016000 0x30>;
+ #clock-cells = <0>;
+ clock-output-names = "apcs_hfpll";
+ clocks = <&xo_board>;
+ clock-names = "xo";
+ };
+
timer@b12 {
#address-cells = <1>;
#size-cells = <1>;
--
2.22.0
[PATCH 5/6] arm64: dts: qcom: qcs404: Add DVFS support
Support dynamic voltage and frequency scaling on qcs404.
CPUFreq will soon be superseded by Core Power Reduction (CPR, a form
of Adaptive Voltage Scaling found on some Qualcomm SoCs like the
qcs404).
Due to the CPR upstreaming already being in progress - and some
commits already merged - the following commit will need to be
reverted to enable CPUFreq support
Author: Jorge Ramirez-Ortiz
Date: Thu Jul 25 12:41:36 2019 +0200
cpufreq: Add qcs404 to cpufreq-dt-platdev blacklist
Co-developed-by: Niklas Cassel
Signed-off-by: Niklas Cassel
Signed-off-by: Jorge Ramirez-Ortiz
Reviewed-by: Bjorn Andersson
---
arch/arm64/boot/dts/qcom/qcs404.dtsi | 31
1 file changed, 31 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi
b/arch/arm64/boot/dts/qcom/qcs404.dtsi
index 34360b2d3e0d..e425e54e1af9 100644
--- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
+++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
@@ -36,6 +36,10 @@
cpu-idle-states = <&CPU_SLEEP_0>;
next-level-cache = <&L2_0>;
#cooling-cells = <2>;
+ clocks = <&apcs_glb>;
+ operating-points-v2 = <&cpu_opp_table>;
+ cpu-supply = <&pms405_s3>;
+
};
CPU1: cpu@101 {
@@ -46,6 +50,9 @@
cpu-idle-states = <&CPU_SLEEP_0>;
next-level-cache = <&L2_0>;
#cooling-cells = <2>;
+ clocks = <&apcs_glb>;
+ operating-points-v2 = <&cpu_opp_table>;
+ cpu-supply = <&pms405_s3>;
};
CPU2: cpu@102 {
@@ -56,6 +63,9 @@
cpu-idle-states = <&CPU_SLEEP_0>;
next-level-cache = <&L2_0>;
#cooling-cells = <2>;
+ clocks = <&apcs_glb>;
+ operating-points-v2 = <&cpu_opp_table>;
+ cpu-supply = <&pms405_s3>;
};
CPU3: cpu@103 {
@@ -66,6 +76,9 @@
cpu-idle-states = <&CPU_SLEEP_0>;
next-level-cache = <&L2_0>;
#cooling-cells = <2>;
+ clocks = <&apcs_glb>;
+ operating-points-v2 = <&cpu_opp_table>;
+ cpu-supply = <&pms405_s3>;
};
L2_0: l2-cache {
@@ -88,6 +101,24 @@
};
};
+ cpu_opp_table: cpu-opp-table {
+ compatible = "operating-points-v2";
+ opp-shared;
+
+ opp-109440 {
+ opp-hz = /bits/ 64 <109440>;
+ opp-microvolt = <1224000 1224000 1224000>;
+ };
+ opp-124800 {
+ opp-hz = /bits/ 64 <124800>;
+ opp-microvolt = <1288000 1288000 1288000>;
+ };
+ opp-140160 {
+ opp-hz = /bits/ 64 <140160>;
+ opp-microvolt = <1384000 1384000 1384000>;
+ };
+ };
+
firmware {
scm: scm {
compatible = "qcom,scm-qcs404", "qcom,scm";
--
2.22.0
[PATCH 6/6] arm64: defconfig: Enable HFPLL
The high frequency pll is required on compatible Qualcomm SoCs to support the CPU frequency scaling feature. Co-developed-by: Niklas Cassel Signed-off-by: Niklas Cassel Signed-off-by: Jorge Ramirez-Ortiz Reviewed-by: Bjorn Andersson --- arch/arm64/configs/defconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig index 74f82901aa2d..af13602c3987 100644 --- a/arch/arm64/configs/defconfig +++ b/arch/arm64/configs/defconfig @@ -703,6 +703,7 @@ CONFIG_MSM_GCC_8998=y CONFIG_QCS_GCC_404=y CONFIG_SDM_GCC_845=y CONFIG_SM_GCC_8150=y +CONFIG_QCOM_HFPLL=y CONFIG_HWSPINLOCK=y CONFIG_HWSPINLOCK_QCOM=y CONFIG_ARM_MHU=y -- 2.22.0
[PATCH 1/6] dt-bindings: mailbox: qcom: Add clock-name optional property
When the APCS clock is registered (platform dependent), it retrieves
its parent names from hardcoded values in the driver.
The following commit allows the DT node to provide such clock names to
the platform data based clock driver therefore avoiding having to
explicitly embed those names in the clock driver source code.
Co-developed-by: Niklas Cassel
Signed-off-by: Niklas Cassel
Signed-off-by: Jorge Ramirez-Ortiz
Reviewed-by: Rob Herring
Reviewed-by: Bjorn Andersson
---
.../mailbox/qcom,apcs-kpss-global.txt | 24 ---
1 file changed, 21 insertions(+), 3 deletions(-)
diff --git
a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt
b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt
index 1232fc9fc709..b69310322b09 100644
--- a/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt
+++ b/Documentation/devicetree/bindings/mailbox/qcom,apcs-kpss-global.txt
@@ -18,10 +18,11 @@ platforms.
Usage: required
Value type:
Definition: must specify the base address and size of the global block
+
- clocks:
- Usage: required if #clocks-cells property is present
- Value type:
- Definition: phandle to the input PLL, which feeds the APCS mux/divider
+ Usage: required if #clock-names property is present
+ Value type:
+ Definition: phandles to the two parent clocks of the clock driver.
- #mbox-cells:
Usage: required
@@ -33,6 +34,12 @@ platforms.
Value type:
Definition: as described in clock.txt, must be 0
+- clock-names:
+ Usage: required if the platform data based clock driver needs to
+ retrieve the parent clock names from device tree.
+ This will requires two mandatory clocks to be defined.
+ Value type:
+ Definition: must be "aux" and "pll"
= EXAMPLE
The following example describes the APCS HMSS found in MSM8996 and part of the
@@ -65,3 +72,14 @@ Below is another example of the APCS binding on MSM8916
platforms:
clocks = <&a53pll>;
#clock-cells = <0>;
};
+
+Below is another example of the APCS binding on QCS404 platforms:
+
+ apcs_glb: mailbox@b011000 {
+ compatible = "qcom,qcs404-apcs-apps-global", "syscon";
+ reg = <0x0b011000 0x1000>;
+ #mbox-cells = <1>;
+ clocks = <&gcc GCC_GPLL0_AO_OUT_MAIN>, <&apcs_hfpll>;
+ clock-names = "aux", "pll";
+ #clock-cells = <0>;
+ };
--
2.22.0
[PATCH 4/6] arm64: dts: qcom: qcs404: Add the clocks for APCS mux/divider
Specify the clocks that feed the APCS mux/divider instead of using
default hardcoded values in the source code.
Co-developed-by: Niklas Cassel
Signed-off-by: Niklas Cassel
Signed-off-by: Jorge Ramirez-Ortiz
Reviewed-by: Bjorn Andersson
---
arch/arm64/boot/dts/qcom/qcs404.dtsi | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi
b/arch/arm64/boot/dts/qcom/qcs404.dtsi
index 75ea356a3fb0..34360b2d3e0d 100644
--- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
+++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
@@ -867,6 +867,9 @@
compatible = "qcom,qcs404-apcs-apps-global", "syscon";
reg = <0x0b011000 0x1000>;
#mbox-cells = <1>;
+ clocks = <&gcc GCC_GPLL0_AO_OUT_MAIN>, <&apcs_hfpll>;
+ clock-names = "aux", "pll";
+ #clock-cells = <0>;
};
apcs_hfpll: clock-controller@b016000 {
--
2.22.0
[PATCH 2/6] arm64: dts: qcom: msm8916: Add the clocks for the APCS mux/divider
Specify the clocks that feed the APCS mux/divider instead of using default hardcoded values in the source code. The driver still supports the previous bindings; however with this update it we allow the msm8916 to access the parent clock names required by the driver operation using the device tree node. Signed-off-by: Jorge Ramirez-Ortiz Reviewed-by: Bjorn Andersson --- arch/arm64/boot/dts/qcom/msm8916.dtsi | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi index 5ea9fb8f2f87..96dc7a12aa94 100644 --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi @@ -429,7 +429,8 @@ compatible = "qcom,msm8916-apcs-kpss-global", "syscon"; reg = <0xb011000 0x1000>; #mbox-cells = <1>; - clocks = <&a53pll>; + clocks = <&gcc GPLL0_VOTE>, <&a53pll>; + clock-names = "aux", "pll"; #clock-cells = <0>; }; -- 2.22.0
[PATCH 1/2] mbox: qcom: add APCS child device for QCS404
There is clock controller functionality in the APCS hardware block of
qcs404 devices similar to msm8916.
Co-developed-by: Niklas Cassel
Signed-off-by: Niklas Cassel
Signed-off-by: Jorge Ramirez-Ortiz
Reviewed-by: Bjorn Andersson
---
drivers/mailbox/qcom-apcs-ipc-mailbox.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
index 705e17a5479c..76e1ad433b3f 100644
--- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c
+++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c
@@ -89,7 +89,11 @@ static int qcom_apcs_ipc_probe(struct platform_device *pdev)
return ret;
}
- if (of_device_is_compatible(np, "qcom,msm8916-apcs-kpss-global")) {
+ platform_set_drvdata(pdev, apcs);
+
+ if (of_device_is_compatible(np, "qcom,msm8916-apcs-kpss-global") ||
+ of_device_is_compatible(np, "qcom,qcs404-apcs-apps-global")) {
+
apcs->clk = platform_device_register_data(&pdev->dev,
"qcom-apcs-msm8916-clk",
-1, NULL, 0);
@@ -97,8 +101,6 @@ static int qcom_apcs_ipc_probe(struct platform_device *pdev)
dev_err(&pdev->dev, "failed to register APCS clk\n");
}
- platform_set_drvdata(pdev, apcs);
-
return 0;
}
--
2.22.0
[PATCH 2/2] mbox: qcom: replace integer with valid macro
Use the correct macro when registering the platform device. Co-developed-by: Niklas Cassel Signed-off-by: Niklas Cassel Signed-off-by: Jorge Ramirez-Ortiz Reviewed-by: Bjorn Andersson --- drivers/mailbox/qcom-apcs-ipc-mailbox.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/mailbox/qcom-apcs-ipc-mailbox.c b/drivers/mailbox/qcom-apcs-ipc-mailbox.c index 76e1ad433b3f..dc198802bdf7 100644 --- a/drivers/mailbox/qcom-apcs-ipc-mailbox.c +++ b/drivers/mailbox/qcom-apcs-ipc-mailbox.c @@ -96,7 +96,8 @@ static int qcom_apcs_ipc_probe(struct platform_device *pdev) apcs->clk = platform_device_register_data(&pdev->dev, "qcom-apcs-msm8916-clk", - -1, NULL, 0); + PLATFORM_DEVID_NONE, + NULL, 0); if (IS_ERR(apcs->clk)) dev_err(&pdev->dev, "failed to register APCS clk\n"); } -- 2.22.0
