Re: [PATCH v5 04/10] drivers: qcom: rpmh: add RPMH helper functions

2018-04-13 Thread Stephen Boyd 
Quoting Bjorn Andersson (2018-04-10 17:01:20)
> On Thu 05 Apr 09:18 PDT 2018, Lina Iyer wrote:
> 
> >  
> > diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
> > new file mode 100644
> > index ..e3c7491e7baf
> > --- /dev/null
> > +++ b/drivers/soc/qcom/rpmh.c
> > @@ -0,0 +1,253 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include 
> > +
> > +#include "rpmh-internal.h"
> > +
> > +#define RPMH_MAX_MBOXES  2
> > +#define RPMH_TIMEOUT_MS  1
> 
> Just define this in jiffies and you don't need to do msecs_to_jiffies()
> every time you use it.
> 

Put the msecs_to_jiffies() here if you want. The compiler will constant
fold it to the final value, and then we get the same time in seconds no
matter what the jiffies frequency is configured for.

Re: [PATCH v5 04/10] drivers: qcom: rpmh: add RPMH helper functions

2018-04-13 Thread Stephen Boyd 
Quoting Bjorn Andersson (2018-04-10 17:01:20)
> On Thu 05 Apr 09:18 PDT 2018, Lina Iyer wrote:
> 
> >  
> > diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
> > new file mode 100644
> > index ..e3c7491e7baf
> > --- /dev/null
> > +++ b/drivers/soc/qcom/rpmh.c
> > @@ -0,0 +1,253 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include 
> > +
> > +#include "rpmh-internal.h"
> > +
> > +#define RPMH_MAX_MBOXES  2
> > +#define RPMH_TIMEOUT_MS  1
> 
> Just define this in jiffies and you don't need to do msecs_to_jiffies()
> every time you use it.
> 

Put the msecs_to_jiffies() here if you want. The compiler will constant
fold it to the final value, and then we get the same time in seconds no
matter what the jiffies frequency is configured for.

Re: [PATCH v5 04/10] drivers: qcom: rpmh: add RPMH helper functions

2018-04-11 Thread Lina Iyer 

On Tue, Apr 10 2018 at 20:23 -0600, Stephen Boyd wrote:

Quoting Lina Iyer (2018-04-09 08:36:31)

On Fri, Apr 06 2018 at 19:21 -0600, Stephen Boyd wrote:
>Quoting Lina Iyer (2018-04-05 09:18:28)
>> diff --git a/include/soc/qcom/rpmh.h b/include/soc/qcom/rpmh.h
>> new file mode 100644
>> index ..95334d4c1ede
>> --- /dev/null
>> +++ b/include/soc/qcom/rpmh.h
>> @@ -0,0 +1,34 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#ifndef __SOC_QCOM_RPMH_H__
>> +#define __SOC_QCOM_RPMH_H__
>> +
>> +#include 
>> +#include 
>> +
>> +struct rpmh_client;
>> +
>> +#if IS_ENABLED(CONFIG_QCOM_RPMH)
>> +int rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
>> +  const struct tcs_cmd *cmd, u32 n);
>> +
>> +struct rpmh_client *rpmh_get_client(struct platform_device *pdev);
>> +
>> +void rpmh_release(struct rpmh_client *rc);
>
>Please get rid of this 'client' layer and fold it into the rpmh driver.
>Everything that uses the rpmh_client is a child device of the rpmh
>device so they should be able to just pass in their device pointer as
>their 'handle' and have the rpmh driver take that, get the parent device
>pointer, and pull an rpmh_drv structure out of there. The 'common' code
>can go into the base rpmh driver and get used from there and then we
>don't have to hop between two files to see how rpmh is used by the
>consumers. Code complexity goes down this way.

That would be not be a good idea. This layer is not just providing an
API interface. There is resource buffering, handling of memory for
requests and downstream quirks and debug going on in this layer. It
would be unwise to clobber the hardware centric rpmh-rsc layer. If you
look at the series as a whole, you would understand why this is
necessary. I plan to build more on top of these patches in the future as
we add support for system low power modes. The complexity doesn't go
away, it just thrown in to another file, which is already decently
sized.

I could try to use the device as a handle, and internally work on
getting the drv and other information from it, if that helps. But I do
not want to clobber these two files together. It doesn't help
maintainability.


Using the device as a handle is a good start. Let's see how it looks
once that part of the code gets replaced. I still fail to see how buffer
management and requests are any different from poking the hardware, but
OK. Maybe if this was a TCS "library" on top of the rpmh hardware
interface?

This is essentially a TCS library.

-- Lina

Re: [PATCH v5 04/10] drivers: qcom: rpmh: add RPMH helper functions

2018-04-11 Thread Lina Iyer 

On Tue, Apr 10 2018 at 20:23 -0600, Stephen Boyd wrote:

Quoting Lina Iyer (2018-04-09 08:36:31)

On Fri, Apr 06 2018 at 19:21 -0600, Stephen Boyd wrote:
>Quoting Lina Iyer (2018-04-05 09:18:28)
>> diff --git a/include/soc/qcom/rpmh.h b/include/soc/qcom/rpmh.h
>> new file mode 100644
>> index ..95334d4c1ede
>> --- /dev/null
>> +++ b/include/soc/qcom/rpmh.h
>> @@ -0,0 +1,34 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#ifndef __SOC_QCOM_RPMH_H__
>> +#define __SOC_QCOM_RPMH_H__
>> +
>> +#include 
>> +#include 
>> +
>> +struct rpmh_client;
>> +
>> +#if IS_ENABLED(CONFIG_QCOM_RPMH)
>> +int rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
>> +  const struct tcs_cmd *cmd, u32 n);
>> +
>> +struct rpmh_client *rpmh_get_client(struct platform_device *pdev);
>> +
>> +void rpmh_release(struct rpmh_client *rc);
>
>Please get rid of this 'client' layer and fold it into the rpmh driver.
>Everything that uses the rpmh_client is a child device of the rpmh
>device so they should be able to just pass in their device pointer as
>their 'handle' and have the rpmh driver take that, get the parent device
>pointer, and pull an rpmh_drv structure out of there. The 'common' code
>can go into the base rpmh driver and get used from there and then we
>don't have to hop between two files to see how rpmh is used by the
>consumers. Code complexity goes down this way.

That would be not be a good idea. This layer is not just providing an
API interface. There is resource buffering, handling of memory for
requests and downstream quirks and debug going on in this layer. It
would be unwise to clobber the hardware centric rpmh-rsc layer. If you
look at the series as a whole, you would understand why this is
necessary. I plan to build more on top of these patches in the future as
we add support for system low power modes. The complexity doesn't go
away, it just thrown in to another file, which is already decently
sized.

I could try to use the device as a handle, and internally work on
getting the drv and other information from it, if that helps. But I do
not want to clobber these two files together. It doesn't help
maintainability.


Using the device as a handle is a good start. Let's see how it looks
once that part of the code gets replaced. I still fail to see how buffer
management and requests are any different from poking the hardware, but
OK. Maybe if this was a TCS "library" on top of the rpmh hardware
interface?

This is essentially a TCS library.

-- Lina

Re: [PATCH v5 04/10] drivers: qcom: rpmh: add RPMH helper functions

2018-04-10 Thread Stephen Boyd 
Quoting Lina Iyer (2018-04-09 08:36:31)
> On Fri, Apr 06 2018 at 19:21 -0600, Stephen Boyd wrote:
> >Quoting Lina Iyer (2018-04-05 09:18:28)
> >> diff --git a/include/soc/qcom/rpmh.h b/include/soc/qcom/rpmh.h
> >> new file mode 100644
> >> index ..95334d4c1ede
> >> --- /dev/null
> >> +++ b/include/soc/qcom/rpmh.h
> >> @@ -0,0 +1,34 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +/*
> >> + * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> >> + */
> >> +
> >> +#ifndef __SOC_QCOM_RPMH_H__
> >> +#define __SOC_QCOM_RPMH_H__
> >> +
> >> +#include 
> >> +#include 
> >> +
> >> +struct rpmh_client;
> >> +
> >> +#if IS_ENABLED(CONFIG_QCOM_RPMH)
> >> +int rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
> >> +  const struct tcs_cmd *cmd, u32 n);
> >> +
> >> +struct rpmh_client *rpmh_get_client(struct platform_device *pdev);
> >> +
> >> +void rpmh_release(struct rpmh_client *rc);
> >
> >Please get rid of this 'client' layer and fold it into the rpmh driver.
> >Everything that uses the rpmh_client is a child device of the rpmh
> >device so they should be able to just pass in their device pointer as
> >their 'handle' and have the rpmh driver take that, get the parent device
> >pointer, and pull an rpmh_drv structure out of there. The 'common' code
> >can go into the base rpmh driver and get used from there and then we
> >don't have to hop between two files to see how rpmh is used by the
> >consumers. Code complexity goes down this way.
> 
> That would be not be a good idea. This layer is not just providing an
> API interface. There is resource buffering, handling of memory for
> requests and downstream quirks and debug going on in this layer. It
> would be unwise to clobber the hardware centric rpmh-rsc layer. If you
> look at the series as a whole, you would understand why this is
> necessary. I plan to build more on top of these patches in the future as
> we add support for system low power modes. The complexity doesn't go
> away, it just thrown in to another file, which is already decently
> sized.
> 
> I could try to use the device as a handle, and internally work on
> getting the drv and other information from it, if that helps. But I do
> not want to clobber these two files together. It doesn't help
> maintainability.

Using the device as a handle is a good start. Let's see how it looks
once that part of the code gets replaced. I still fail to see how buffer
management and requests are any different from poking the hardware, but
OK. Maybe if this was a TCS "library" on top of the rpmh hardware
interface?

Re: [PATCH v5 04/10] drivers: qcom: rpmh: add RPMH helper functions

2018-04-10 Thread Stephen Boyd 
Quoting Lina Iyer (2018-04-09 08:36:31)
> On Fri, Apr 06 2018 at 19:21 -0600, Stephen Boyd wrote:
> >Quoting Lina Iyer (2018-04-05 09:18:28)
> >> diff --git a/include/soc/qcom/rpmh.h b/include/soc/qcom/rpmh.h
> >> new file mode 100644
> >> index ..95334d4c1ede
> >> --- /dev/null
> >> +++ b/include/soc/qcom/rpmh.h
> >> @@ -0,0 +1,34 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +/*
> >> + * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> >> + */
> >> +
> >> +#ifndef __SOC_QCOM_RPMH_H__
> >> +#define __SOC_QCOM_RPMH_H__
> >> +
> >> +#include 
> >> +#include 
> >> +
> >> +struct rpmh_client;
> >> +
> >> +#if IS_ENABLED(CONFIG_QCOM_RPMH)
> >> +int rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
> >> +  const struct tcs_cmd *cmd, u32 n);
> >> +
> >> +struct rpmh_client *rpmh_get_client(struct platform_device *pdev);
> >> +
> >> +void rpmh_release(struct rpmh_client *rc);
> >
> >Please get rid of this 'client' layer and fold it into the rpmh driver.
> >Everything that uses the rpmh_client is a child device of the rpmh
> >device so they should be able to just pass in their device pointer as
> >their 'handle' and have the rpmh driver take that, get the parent device
> >pointer, and pull an rpmh_drv structure out of there. The 'common' code
> >can go into the base rpmh driver and get used from there and then we
> >don't have to hop between two files to see how rpmh is used by the
> >consumers. Code complexity goes down this way.
> 
> That would be not be a good idea. This layer is not just providing an
> API interface. There is resource buffering, handling of memory for
> requests and downstream quirks and debug going on in this layer. It
> would be unwise to clobber the hardware centric rpmh-rsc layer. If you
> look at the series as a whole, you would understand why this is
> necessary. I plan to build more on top of these patches in the future as
> we add support for system low power modes. The complexity doesn't go
> away, it just thrown in to another file, which is already decently
> sized.
> 
> I could try to use the device as a handle, and internally work on
> getting the drv and other information from it, if that helps. But I do
> not want to clobber these two files together. It doesn't help
> maintainability.

Using the device as a handle is a good start. Let's see how it looks
once that part of the code gets replaced. I still fail to see how buffer
management and requests are any different from poking the hardware, but
OK. Maybe if this was a TCS "library" on top of the rpmh hardware
interface?

Re: [PATCH v5 04/10] drivers: qcom: rpmh: add RPMH helper functions

2018-04-10 Thread Bjorn Andersson 
On Thu 05 Apr 09:18 PDT 2018, Lina Iyer wrote:

> Sending RPMH requests and waiting for response from the controller
> through a callback is common functionality across all platform drivers.
> To simplify drivers, add a library functions to create RPMH client and
> send resource state requests.
> 
> rpmh_write() is a synchronous blocking call that can be used to send
> active state requests.
> 
> Signed-off-by: Lina Iyer 
> ---
> 
> Changes in v4:
>   - use const struct tcs_cmd in API
>   - remove wait count from this patch
>   - changed -EFAULT to -EINVAL
> ---
>  drivers/soc/qcom/Makefile|   4 +-
>  drivers/soc/qcom/rpmh-internal.h |   2 +
>  drivers/soc/qcom/rpmh-rsc.c  |   7 ++
>  drivers/soc/qcom/rpmh.c  | 253 
> +++
>  include/soc/qcom/rpmh.h  |  34 ++
>  5 files changed, 299 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/soc/qcom/rpmh.c
>  create mode 100644 include/soc/qcom/rpmh.h
> 
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index cb6300f6a8e9..bb395c3202ca 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -7,7 +7,9 @@ obj-$(CONFIG_QCOM_PM) +=  spm.o
>  obj-$(CONFIG_QCOM_QMI_HELPERS)   += qmi_helpers.o
>  qmi_helpers-y+= qmi_encdec.o qmi_interface.o
>  obj-$(CONFIG_QCOM_RMTFS_MEM) += rmtfs_mem.o
> -obj-$(CONFIG_QCOM_RPMH)  +=  rpmh-rsc.o
> +obj-$(CONFIG_QCOM_RPMH)  += qcom_rpmh.o
> +qcom_rpmh-y  += rpmh-rsc.o
> +qcom_rpmh-y  += rpmh.o
>  obj-$(CONFIG_QCOM_SMD_RPM)   += smd-rpm.o
>  obj-$(CONFIG_QCOM_SMEM) +=   smem.o
>  obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
> diff --git a/drivers/soc/qcom/rpmh-internal.h 
> b/drivers/soc/qcom/rpmh-internal.h
> index aa73ec4b3e42..125f9faec536 100644
> --- a/drivers/soc/qcom/rpmh-internal.h
> +++ b/drivers/soc/qcom/rpmh-internal.h
> @@ -86,4 +86,6 @@ struct rsc_drv {
>  
>  int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg);
>  
> +void rpmh_tx_done(const struct tcs_request *msg, int r);
> +
>  #endif /* __RPM_INTERNAL_H__ */
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index f604101a4fc2..c5cde917dba6 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -252,6 +252,8 @@ static void tcs_notify_tx_done(unsigned long data)
>   struct rsc_drv *drv = (struct rsc_drv *)data;
>   struct tcs_response *resp;
>   unsigned long flags;
> + const struct tcs_request *msg;
> + int err;
>  
>   for (;;) {
>   spin_lock_irqsave(>drv_lock, flags);
> @@ -264,7 +266,10 @@ static void tcs_notify_tx_done(unsigned long data)
>   list_del(>list);
>   spin_unlock_irqrestore(>drv_lock, flags);
>   trace_rpmh_notify_tx_done(drv, resp);
> + msg = resp->msg;
> + err = resp->err;
>   free_response(resp);
> + rpmh_tx_done(msg, err);
>   }
>  }
>  
> @@ -554,6 +559,8 @@ static int rpmh_rsc_probe(struct platform_device *pdev)
>   /* Enable the active TCS to send requests immediately */
>   write_tcs_reg(drv, RSC_DRV_IRQ_ENABLE, 0, 0, drv->tcs[ACTIVE_TCS].mask);
>  
> + dev_set_drvdata(>dev, drv);
> +
>   return devm_of_platform_populate(>dev);
>  }
>  
> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
> new file mode 100644
> index ..e3c7491e7baf
> --- /dev/null
> +++ b/drivers/soc/qcom/rpmh.c
> @@ -0,0 +1,253 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +#include "rpmh-internal.h"
> +
> +#define RPMH_MAX_MBOXES  2
> +#define RPMH_TIMEOUT_MS  1

Just define this in jiffies and you don't need to do msecs_to_jiffies()
every time you use it.

> +
> +#define DEFINE_RPMH_MSG_ONSTACK(rc, s, q, name)  \
> + struct rpmh_request name = {\
> + .msg = {\
> + .state = s, \
> + .cmds = name.cmd,   \
> + .num_cmds = 0,  \
> + .wait_for_compl = true, \
> + },  \
> + .cmd = { { 0 } },   \
> + .completion = q,\
> + .rc = rc,   \
> + }
> +
> +/**
> + * struct rpmh_request: the message to be sent to rpmh-rsc
> + *
> + * @msg: the request
> + * @cmd: the payload that will be part of the @msg
> + * @completion: triggered when request is done
> + * @err: err return

Re: [PATCH v5 04/10] drivers: qcom: rpmh: add RPMH helper functions

2018-04-10 Thread Bjorn Andersson 
On Thu 05 Apr 09:18 PDT 2018, Lina Iyer wrote:

> Sending RPMH requests and waiting for response from the controller
> through a callback is common functionality across all platform drivers.
> To simplify drivers, add a library functions to create RPMH client and
> send resource state requests.
> 
> rpmh_write() is a synchronous blocking call that can be used to send
> active state requests.
> 
> Signed-off-by: Lina Iyer 
> ---
> 
> Changes in v4:
>   - use const struct tcs_cmd in API
>   - remove wait count from this patch
>   - changed -EFAULT to -EINVAL
> ---
>  drivers/soc/qcom/Makefile|   4 +-
>  drivers/soc/qcom/rpmh-internal.h |   2 +
>  drivers/soc/qcom/rpmh-rsc.c  |   7 ++
>  drivers/soc/qcom/rpmh.c  | 253 
> +++
>  include/soc/qcom/rpmh.h  |  34 ++
>  5 files changed, 299 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/soc/qcom/rpmh.c
>  create mode 100644 include/soc/qcom/rpmh.h
> 
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index cb6300f6a8e9..bb395c3202ca 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -7,7 +7,9 @@ obj-$(CONFIG_QCOM_PM) +=  spm.o
>  obj-$(CONFIG_QCOM_QMI_HELPERS)   += qmi_helpers.o
>  qmi_helpers-y+= qmi_encdec.o qmi_interface.o
>  obj-$(CONFIG_QCOM_RMTFS_MEM) += rmtfs_mem.o
> -obj-$(CONFIG_QCOM_RPMH)  +=  rpmh-rsc.o
> +obj-$(CONFIG_QCOM_RPMH)  += qcom_rpmh.o
> +qcom_rpmh-y  += rpmh-rsc.o
> +qcom_rpmh-y  += rpmh.o
>  obj-$(CONFIG_QCOM_SMD_RPM)   += smd-rpm.o
>  obj-$(CONFIG_QCOM_SMEM) +=   smem.o
>  obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
> diff --git a/drivers/soc/qcom/rpmh-internal.h 
> b/drivers/soc/qcom/rpmh-internal.h
> index aa73ec4b3e42..125f9faec536 100644
> --- a/drivers/soc/qcom/rpmh-internal.h
> +++ b/drivers/soc/qcom/rpmh-internal.h
> @@ -86,4 +86,6 @@ struct rsc_drv {
>  
>  int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg);
>  
> +void rpmh_tx_done(const struct tcs_request *msg, int r);
> +
>  #endif /* __RPM_INTERNAL_H__ */
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index f604101a4fc2..c5cde917dba6 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -252,6 +252,8 @@ static void tcs_notify_tx_done(unsigned long data)
>   struct rsc_drv *drv = (struct rsc_drv *)data;
>   struct tcs_response *resp;
>   unsigned long flags;
> + const struct tcs_request *msg;
> + int err;
>  
>   for (;;) {
>   spin_lock_irqsave(>drv_lock, flags);
> @@ -264,7 +266,10 @@ static void tcs_notify_tx_done(unsigned long data)
>   list_del(>list);
>   spin_unlock_irqrestore(>drv_lock, flags);
>   trace_rpmh_notify_tx_done(drv, resp);
> + msg = resp->msg;
> + err = resp->err;
>   free_response(resp);
> + rpmh_tx_done(msg, err);
>   }
>  }
>  
> @@ -554,6 +559,8 @@ static int rpmh_rsc_probe(struct platform_device *pdev)
>   /* Enable the active TCS to send requests immediately */
>   write_tcs_reg(drv, RSC_DRV_IRQ_ENABLE, 0, 0, drv->tcs[ACTIVE_TCS].mask);
>  
> + dev_set_drvdata(>dev, drv);
> +
>   return devm_of_platform_populate(>dev);
>  }
>  
> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
> new file mode 100644
> index ..e3c7491e7baf
> --- /dev/null
> +++ b/drivers/soc/qcom/rpmh.c
> @@ -0,0 +1,253 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +#include "rpmh-internal.h"
> +
> +#define RPMH_MAX_MBOXES  2
> +#define RPMH_TIMEOUT_MS  1

Just define this in jiffies and you don't need to do msecs_to_jiffies()
every time you use it.

> +
> +#define DEFINE_RPMH_MSG_ONSTACK(rc, s, q, name)  \
> + struct rpmh_request name = {\
> + .msg = {\
> + .state = s, \
> + .cmds = name.cmd,   \
> + .num_cmds = 0,  \
> + .wait_for_compl = true, \
> + },  \
> + .cmd = { { 0 } },   \
> + .completion = q,\
> + .rc = rc,   \
> + }
> +
> +/**
> + * struct rpmh_request: the message to be sent to rpmh-rsc
> + *
> + * @msg: the request
> + * @cmd: the payload that will be part of the @msg
> + * @completion: triggered when request is done
> + * @err: err return from the controller
>

Re: [PATCH v5 04/10] drivers: qcom: rpmh: add RPMH helper functions

2018-04-09 Thread Lina Iyer 

On Fri, Apr 06 2018 at 19:21 -0600, Stephen Boyd wrote:

Quoting Lina Iyer (2018-04-05 09:18:28)

diff --git a/include/soc/qcom/rpmh.h b/include/soc/qcom/rpmh.h
new file mode 100644
index ..95334d4c1ede
--- /dev/null
+++ b/include/soc/qcom/rpmh.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef __SOC_QCOM_RPMH_H__
+#define __SOC_QCOM_RPMH_H__
+
+#include 
+#include 
+
+struct rpmh_client;
+
+#if IS_ENABLED(CONFIG_QCOM_RPMH)
+int rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
+  const struct tcs_cmd *cmd, u32 n);
+
+struct rpmh_client *rpmh_get_client(struct platform_device *pdev);
+
+void rpmh_release(struct rpmh_client *rc);


Please get rid of this 'client' layer and fold it into the rpmh driver.
Everything that uses the rpmh_client is a child device of the rpmh
device so they should be able to just pass in their device pointer as
their 'handle' and have the rpmh driver take that, get the parent device
pointer, and pull an rpmh_drv structure out of there. The 'common' code
can go into the base rpmh driver and get used from there and then we
don't have to hop between two files to see how rpmh is used by the
consumers. Code complexity goes down this way.


That would be not be a good idea. This layer is not just providing an
API interface. There is resource buffering, handling of memory for
requests and downstream quirks and debug going on in this layer. It
would be unwise to clobber the hardware centric rpmh-rsc layer. If you
look at the series as a whole, you would understand why this is
necessary. I plan to build more on top of these patches in the future as
we add support for system low power modes. The complexity doesn't go
away, it just thrown in to another file, which is already decently
sized.

I could try to use the device as a handle, and internally work on
getting the drv and other information from it, if that helps. But I do
not want to clobber these two files together. It doesn't help
maintainability.

Thanks,
Lina

Re: [PATCH v5 04/10] drivers: qcom: rpmh: add RPMH helper functions

2018-04-09 Thread Lina Iyer 

On Fri, Apr 06 2018 at 19:21 -0600, Stephen Boyd wrote:

Quoting Lina Iyer (2018-04-05 09:18:28)

diff --git a/include/soc/qcom/rpmh.h b/include/soc/qcom/rpmh.h
new file mode 100644
index ..95334d4c1ede
--- /dev/null
+++ b/include/soc/qcom/rpmh.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
+ */
+
+#ifndef __SOC_QCOM_RPMH_H__
+#define __SOC_QCOM_RPMH_H__
+
+#include 
+#include 
+
+struct rpmh_client;
+
+#if IS_ENABLED(CONFIG_QCOM_RPMH)
+int rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
+  const struct tcs_cmd *cmd, u32 n);
+
+struct rpmh_client *rpmh_get_client(struct platform_device *pdev);
+
+void rpmh_release(struct rpmh_client *rc);


Please get rid of this 'client' layer and fold it into the rpmh driver.
Everything that uses the rpmh_client is a child device of the rpmh
device so they should be able to just pass in their device pointer as
their 'handle' and have the rpmh driver take that, get the parent device
pointer, and pull an rpmh_drv structure out of there. The 'common' code
can go into the base rpmh driver and get used from there and then we
don't have to hop between two files to see how rpmh is used by the
consumers. Code complexity goes down this way.


That would be not be a good idea. This layer is not just providing an
API interface. There is resource buffering, handling of memory for
requests and downstream quirks and debug going on in this layer. It
would be unwise to clobber the hardware centric rpmh-rsc layer. If you
look at the series as a whole, you would understand why this is
necessary. I plan to build more on top of these patches in the future as
we add support for system low power modes. The complexity doesn't go
away, it just thrown in to another file, which is already decently
sized.

I could try to use the device as a handle, and internally work on
getting the drv and other information from it, if that helps. But I do
not want to clobber these two files together. It doesn't help
maintainability.

Thanks,
Lina

Re: [PATCH v5 04/10] drivers: qcom: rpmh: add RPMH helper functions

2018-04-06 Thread Stephen Boyd 
Quoting Lina Iyer (2018-04-05 09:18:28)
> diff --git a/include/soc/qcom/rpmh.h b/include/soc/qcom/rpmh.h
> new file mode 100644
> index ..95334d4c1ede
> --- /dev/null
> +++ b/include/soc/qcom/rpmh.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#ifndef __SOC_QCOM_RPMH_H__
> +#define __SOC_QCOM_RPMH_H__
> +
> +#include 
> +#include 
> +
> +struct rpmh_client;
> +
> +#if IS_ENABLED(CONFIG_QCOM_RPMH)
> +int rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
> +  const struct tcs_cmd *cmd, u32 n);
> +
> +struct rpmh_client *rpmh_get_client(struct platform_device *pdev);
> +
> +void rpmh_release(struct rpmh_client *rc);

Please get rid of this 'client' layer and fold it into the rpmh driver.
Everything that uses the rpmh_client is a child device of the rpmh
device so they should be able to just pass in their device pointer as
their 'handle' and have the rpmh driver take that, get the parent device
pointer, and pull an rpmh_drv structure out of there. The 'common' code
can go into the base rpmh driver and get used from there and then we
don't have to hop between two files to see how rpmh is used by the
consumers. Code complexity goes down this way.

Re: [PATCH v5 04/10] drivers: qcom: rpmh: add RPMH helper functions

2018-04-06 Thread Stephen Boyd 
Quoting Lina Iyer (2018-04-05 09:18:28)
> diff --git a/include/soc/qcom/rpmh.h b/include/soc/qcom/rpmh.h
> new file mode 100644
> index ..95334d4c1ede
> --- /dev/null
> +++ b/include/soc/qcom/rpmh.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#ifndef __SOC_QCOM_RPMH_H__
> +#define __SOC_QCOM_RPMH_H__
> +
> +#include 
> +#include 
> +
> +struct rpmh_client;
> +
> +#if IS_ENABLED(CONFIG_QCOM_RPMH)
> +int rpmh_write(struct rpmh_client *rc, enum rpmh_state state,
> +  const struct tcs_cmd *cmd, u32 n);
> +
> +struct rpmh_client *rpmh_get_client(struct platform_device *pdev);
> +
> +void rpmh_release(struct rpmh_client *rc);

Please get rid of this 'client' layer and fold it into the rpmh driver.
Everything that uses the rpmh_client is a child device of the rpmh
device so they should be able to just pass in their device pointer as
their 'handle' and have the rpmh driver take that, get the parent device
pointer, and pull an rpmh_drv structure out of there. The 'common' code
can go into the base rpmh driver and get used from there and then we
don't have to hop between two files to see how rpmh is used by the
consumers. Code complexity goes down this way.