| Message ID | 1415659966-16200-2-git-send-email-bjorn.andersson@sonymobile.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 10 Nov 2014, Bjorn Andersson wrote: > Resources exposed from the RPM have an "active state" that is used during > normal operations and a "sleep state" that is used for HW assisted sleep > modes. Expose this in the api to let client drivers set the "sleep > state" as well. I assume you have users lined up which will request a sleeping state? > Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com> > --- > drivers/mfd/qcom_rpm.c | 9 +++++---- > drivers/regulator/qcom_rpm-regulator.c | 1 + > include/linux/mfd/qcom_rpm.h | 5 ++++- > 3 files changed, 10 insertions(+), 5 deletions(-) > > diff --git a/drivers/mfd/qcom_rpm.c b/drivers/mfd/qcom_rpm.c > index 0dd7a6fe..f696328 100644 > --- a/drivers/mfd/qcom_rpm.c > +++ b/drivers/mfd/qcom_rpm.c > @@ -67,7 +67,6 @@ struct qcom_rpm { > #define RPM_ACK_SELECTOR 23 > #define RPM_SELECT_SIZE 7 > > -#define RPM_ACTIVE_STATE BIT(0) > #define RPM_NOTIFICATION BIT(30) > #define RPM_REJECTED BIT(31) > > @@ -332,7 +331,10 @@ static const struct of_device_id qcom_rpm_of_match[] = { > }; > MODULE_DEVICE_TABLE(of, qcom_rpm_of_match); > > -int qcom_rpm_write(struct qcom_rpm *rpm, int resource, u32 *buf, size_t count) > +int qcom_rpm_write(struct qcom_rpm *rpm, > + int state, > + int resource, > + u32 *buf, size_t count) > { > const struct qcom_rpm_resource *res; > const struct qcom_rpm_data *data = rpm->data; > @@ -359,8 +361,7 @@ int qcom_rpm_write(struct qcom_rpm *rpm, int resource, u32 *buf, size_t count) > RPM_CTRL_REG(rpm, RPM_REQ_SELECT + i)); > } > > - writel_relaxed(RPM_ACTIVE_STATE, > - RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT)); > + writel_relaxed(BIT(state), RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT)); How are the state bits organised? > reinit_completion(&rpm->ack); > regmap_write(rpm->ipc_regmap, rpm->ipc_offset, BIT(rpm->ipc_bit)); > diff --git a/drivers/regulator/qcom_rpm-regulator.c b/drivers/regulator/qcom_rpm-regulator.c > index b55cd5b..4fc1c7e 100644 > --- a/drivers/regulator/qcom_rpm-regulator.c > +++ b/drivers/regulator/qcom_rpm-regulator.c > @@ -198,6 +198,7 @@ static int rpm_reg_write(struct qcom_rpm_reg *vreg, > vreg->val[req->word] |= value << req->shift; > > return qcom_rpm_write(vreg->rpm, > + RPM_ACTIVE_STATE, > vreg->resource, > vreg->val, > vreg->parts->request_len); > diff --git a/include/linux/mfd/qcom_rpm.h b/include/linux/mfd/qcom_rpm.h > index a60798d..f0e70b2 100644 > --- a/include/linux/mfd/qcom_rpm.h > +++ b/include/linux/mfd/qcom_rpm.h > @@ -5,6 +5,9 @@ > > struct qcom_rpm; > > -int qcom_rpm_write(struct qcom_rpm *rpm, int resource, u32 *buf, size_t count); > +#define RPM_ACTIVE_STATE 0 > +#define RPM_SLEEP_STATE 1 > + > +int qcom_rpm_write(struct qcom_rpm *rpm, int state, int resource, u32 *buf, size_t count); > > #endif
On Tue 11 Nov 04:04 PST 2014, Lee Jones wrote: > On Mon, 10 Nov 2014, Bjorn Andersson wrote: > > > Resources exposed from the RPM have an "active state" that is used during > > normal operations and a "sleep state" that is used for HW assisted sleep > > modes. Expose this in the api to let client drivers set the "sleep > > state" as well. > > I assume you have users lined up which will request a sleeping state? > All users of this interface (regulators, clocks and bus scaling) will have to be able to specify this. [..] > > @@ -359,8 +361,7 @@ int qcom_rpm_write(struct qcom_rpm *rpm, int resource, u32 *buf, size_t count) > > RPM_CTRL_REG(rpm, RPM_REQ_SELECT + i)); > > } > > > > - writel_relaxed(RPM_ACTIVE_STATE, > > - RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT)); > > + writel_relaxed(BIT(state), RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT)); > > How are the state bits organised? > BIT(0) is active mode, BIT(1) is sleep mode, as specified below. I could add some sanity checking here if you would like to. [..] > > diff --git a/include/linux/mfd/qcom_rpm.h b/include/linux/mfd/qcom_rpm.h > > index a60798d..f0e70b2 100644 > > --- a/include/linux/mfd/qcom_rpm.h > > +++ b/include/linux/mfd/qcom_rpm.h > > +#define RPM_ACTIVE_STATE 0 > > +#define RPM_SLEEP_STATE 1 Regards, Bjorn
On Tue, 11 Nov 2014, Bjorn Andersson wrote: > On Tue 11 Nov 04:04 PST 2014, Lee Jones wrote: > > > On Mon, 10 Nov 2014, Bjorn Andersson wrote: > > > > > Resources exposed from the RPM have an "active state" that is used during > > > normal operations and a "sleep state" that is used for HW assisted sleep > > > modes. Expose this in the api to let client drivers set the "sleep > > > state" as well. > > > > I assume you have users lined up which will request a sleeping state? > > > > All users of this interface (regulators, clocks and bus scaling) will have to > be able to specify this. > > [..] > > > @@ -359,8 +361,7 @@ int qcom_rpm_write(struct qcom_rpm *rpm, int resource, u32 *buf, size_t count) > > > RPM_CTRL_REG(rpm, RPM_REQ_SELECT + i)); > > > } > > > > > > - writel_relaxed(RPM_ACTIVE_STATE, > > > - RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT)); > > > + writel_relaxed(BIT(state), RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT)); > > > > How are the state bits organised? > > > > BIT(0) is active mode, BIT(1) is sleep mode, as specified below. I could add > some sanity checking here if you would like to. I'm just double checking that you know what that means. BIT(0) == b01 BIT(1) == b10 It seems strange to represent a single boolean state over 2 bits. Also, what happens if b11 or b00 occurs? > [..] > > > diff --git a/include/linux/mfd/qcom_rpm.h b/include/linux/mfd/qcom_rpm.h > > > index a60798d..f0e70b2 100644 > > > --- a/include/linux/mfd/qcom_rpm.h > > > +++ b/include/linux/mfd/qcom_rpm.h > > > +#define RPM_ACTIVE_STATE 0 > > > +#define RPM_SLEEP_STATE 1 > > Regards, > Bjorn
On Wed, Nov 12 2014 at 02:52 -0700, Lee Jones wrote: >On Tue, 11 Nov 2014, Bjorn Andersson wrote: > >> On Tue 11 Nov 04:04 PST 2014, Lee Jones wrote: >> >> > On Mon, 10 Nov 2014, Bjorn Andersson wrote: >> > > > > + writel_relaxed(BIT(state), RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT)); >> > >> > How are the state bits organised? >> > >> >> BIT(0) is active mode, BIT(1) is sleep mode, as specified below. I could add >> some sanity checking here if you would like to. > >I'm just double checking that you know what that means. > >BIT(0) == b01 >BIT(1) == b10 > >It seems strange to represent a single boolean state over 2 bits. > >Also, what happens if b11 or b00 occurs? > Lee is correct, it should be 0 for Active and 1 for Sleep set. Lina
On Wed 12 Nov 06:45 PST 2014, Lina Iyer wrote: > On Wed, Nov 12 2014 at 02:52 -0700, Lee Jones wrote: > >On Tue, 11 Nov 2014, Bjorn Andersson wrote: > > > >> On Tue 11 Nov 04:04 PST 2014, Lee Jones wrote: > >> > >> > On Mon, 10 Nov 2014, Bjorn Andersson wrote: > >> > > > > > > + writel_relaxed(BIT(state), RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT)); > >> > > >> > How are the state bits organised? > >> > > >> > >> BIT(0) is active mode, BIT(1) is sleep mode, as specified below. I could add > >> some sanity checking here if you would like to. > > > >I'm just double checking that you know what that means. > > > >BIT(0) == b01 > >BIT(1) == b10 > > > >It seems strange to represent a single boolean state over 2 bits. > > > >Also, what happens if b11 or b00 occurs? > > > Lee is correct, it should be 0 for Active and 1 for Sleep set. > In the caf msm-3.4 tree the regulator core will call e.g. vreg_set_voltage() that will call vreg_set(), that will call msm_rpm_set(MSM_RPM_CTX_SET_0,...). That comes from the following: enum { MSM_RPM_CTX_SET_0, MSM_RPM_CTX_SET_SLEEP, MSM_RPM_CTX_SET_COUNT, MSM_RPM_CTX_NOTIFICATION = 30, MSM_RPM_CTX_REJECTED = 31, }; So there's your 0 and 1. msm_rpm_set() calls msm_rpm_set_common() that calls msm_rpm_set_exclusive() that contains these two statements: uint32_t ctx_mask = msm_rpm_get_ctx_mask(ctx); ... msm_rpm_write(MSM_RPM_PAGE_CTRL, target_ctrl(MSM_RPM_CTRL_REQ_CTX_0), ctx_mask); And we have: static inline uint32_t msm_rpm_get_ctx_mask(unsigned int ctx) { return 1UL << ctx; } So, as far as I can see it should be BIT(state) here. But there's a lot of code and a lot of indirections here and I've been tricked by it before, so please let me know if I got something wrong on the way. Regards, Bjorn
On Wed 12 Nov 01:52 PST 2014, Lee Jones wrote: > On Tue, 11 Nov 2014, Bjorn Andersson wrote: > > > On Tue 11 Nov 04:04 PST 2014, Lee Jones wrote: > > > > > On Mon, 10 Nov 2014, Bjorn Andersson wrote: > > > > > > > Resources exposed from the RPM have an "active state" that is used during > > > > normal operations and a "sleep state" that is used for HW assisted sleep > > > > modes. Expose this in the api to let client drivers set the "sleep > > > > state" as well. > > > > > > I assume you have users lined up which will request a sleeping state? > > > > > > > All users of this interface (regulators, clocks and bus scaling) will have to > > be able to specify this. > > > > [..] > > > > @@ -359,8 +361,7 @@ int qcom_rpm_write(struct qcom_rpm *rpm, int resource, u32 *buf, size_t count) > > > > RPM_CTRL_REG(rpm, RPM_REQ_SELECT + i)); > > > > } > > > > > > > > - writel_relaxed(RPM_ACTIVE_STATE, > > > > - RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT)); > > > > + writel_relaxed(BIT(state), RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT)); > > > > > > How are the state bits organised? > > > > > > > BIT(0) is active mode, BIT(1) is sleep mode, as specified below. I could add > > some sanity checking here if you would like to. > > I'm just double checking that you know what that means. > > BIT(0) == b01 > BIT(1) == b10 > > It seems strange to represent a single boolean state over 2 bits. > I'm aware of this and find most parts of this solution to be strange. > Also, what happens if b11 or b00 occurs? > Had to pull out the firmware for the other side; it boils down to a count of trailing zeros, with 0 giving 0, that is then turned back into and enum where ACTIVE is 0 and SLEEP is 1. This is then sanity checked to be less than 2. So the possible values and results would be: 00 => ACTIVE (seems invalid but "accidentally" accepted) 01 => ACTIVE 10 => SLEEP 11 => ACTIVE In one of the later versions of the RPM (not applicable for any of these platforms) 2 seems to be a valid choice as well; but there it's passed as an u32 and not a bitmask. Regards, Bjorn
On Wed, Nov 12 2014 at 12:23 -0700, Bjorn Andersson wrote: >On Wed 12 Nov 06:45 PST 2014, Lina Iyer wrote: > >> On Wed, Nov 12 2014 at 02:52 -0700, Lee Jones wrote: >> >On Tue, 11 Nov 2014, Bjorn Andersson wrote: >> > >> >> On Tue 11 Nov 04:04 PST 2014, Lee Jones wrote: >> >> >> >> > On Mon, 10 Nov 2014, Bjorn Andersson wrote: >> >> > >> >> > > > + writel_relaxed(BIT(state), RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT)); >> >> > >> >> > How are the state bits organised? >> >> > >> >> >> >> BIT(0) is active mode, BIT(1) is sleep mode, as specified below. I could add >> >> some sanity checking here if you would like to. >> > >> >I'm just double checking that you know what that means. >> > >> >BIT(0) == b01 >> >BIT(1) == b10 >> > >> >It seems strange to represent a single boolean state over 2 bits. >> > >> >Also, what happens if b11 or b00 occurs? >> > >> Lee is correct, it should be 0 for Active and 1 for Sleep set. >> > >In the caf msm-3.4 tree the regulator core will call e.g. vreg_set_voltage() >that will call vreg_set(), that will call msm_rpm_set(MSM_RPM_CTX_SET_0,...). >That comes from the following: > >enum { > MSM_RPM_CTX_SET_0, > MSM_RPM_CTX_SET_SLEEP, > MSM_RPM_CTX_SET_COUNT, > > MSM_RPM_CTX_NOTIFICATION = 30, > MSM_RPM_CTX_REJECTED = 31, >}; > >So there's your 0 and 1. > >msm_rpm_set() calls msm_rpm_set_common() that calls msm_rpm_set_exclusive() >that contains these two statements: > > uint32_t ctx_mask = msm_rpm_get_ctx_mask(ctx); > ... > msm_rpm_write(MSM_RPM_PAGE_CTRL, target_ctrl(MSM_RPM_CTRL_REQ_CTX_0), ctx_mask); > >And we have: > >static inline uint32_t msm_rpm_get_ctx_mask(unsigned int ctx) >{ > return 1UL << ctx; >} > >So, as far as I can see it should be BIT(state) here. But there's a lot of code >and a lot of indirections here and I've been tricked by it before, so please >let me know if I got something wrong on the way. Sorry, I retract my earlier objection. This is how it is, strangely. > >Regards, >Bjorn
diff --git a/drivers/mfd/qcom_rpm.c b/drivers/mfd/qcom_rpm.c index 0dd7a6fe..f696328 100644 --- a/drivers/mfd/qcom_rpm.c +++ b/drivers/mfd/qcom_rpm.c @@ -67,7 +67,6 @@ struct qcom_rpm { #define RPM_ACK_SELECTOR 23 #define RPM_SELECT_SIZE 7 -#define RPM_ACTIVE_STATE BIT(0) #define RPM_NOTIFICATION BIT(30) #define RPM_REJECTED BIT(31) @@ -332,7 +331,10 @@ static const struct of_device_id qcom_rpm_of_match[] = { }; MODULE_DEVICE_TABLE(of, qcom_rpm_of_match); -int qcom_rpm_write(struct qcom_rpm *rpm, int resource, u32 *buf, size_t count) +int qcom_rpm_write(struct qcom_rpm *rpm, + int state, + int resource, + u32 *buf, size_t count) { const struct qcom_rpm_resource *res; const struct qcom_rpm_data *data = rpm->data; @@ -359,8 +361,7 @@ int qcom_rpm_write(struct qcom_rpm *rpm, int resource, u32 *buf, size_t count) RPM_CTRL_REG(rpm, RPM_REQ_SELECT + i)); } - writel_relaxed(RPM_ACTIVE_STATE, - RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT)); + writel_relaxed(BIT(state), RPM_CTRL_REG(rpm, RPM_REQUEST_CONTEXT)); reinit_completion(&rpm->ack); regmap_write(rpm->ipc_regmap, rpm->ipc_offset, BIT(rpm->ipc_bit)); diff --git a/drivers/regulator/qcom_rpm-regulator.c b/drivers/regulator/qcom_rpm-regulator.c index b55cd5b..4fc1c7e 100644 --- a/drivers/regulator/qcom_rpm-regulator.c +++ b/drivers/regulator/qcom_rpm-regulator.c @@ -198,6 +198,7 @@ static int rpm_reg_write(struct qcom_rpm_reg *vreg, vreg->val[req->word] |= value << req->shift; return qcom_rpm_write(vreg->rpm, + RPM_ACTIVE_STATE, vreg->resource, vreg->val, vreg->parts->request_len); diff --git a/include/linux/mfd/qcom_rpm.h b/include/linux/mfd/qcom_rpm.h index a60798d..f0e70b2 100644 --- a/include/linux/mfd/qcom_rpm.h +++ b/include/linux/mfd/qcom_rpm.h @@ -5,6 +5,9 @@ struct qcom_rpm; -int qcom_rpm_write(struct qcom_rpm *rpm, int resource, u32 *buf, size_t count); +#define RPM_ACTIVE_STATE 0 +#define RPM_SLEEP_STATE 1 + +int qcom_rpm_write(struct qcom_rpm *rpm, int state, int resource, u32 *buf, size_t count); #endif
Resources exposed from the RPM have an "active state" that is used during normal operations and a "sleep state" that is used for HW assisted sleep modes. Expose this in the api to let client drivers set the "sleep state" as well. Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com> --- drivers/mfd/qcom_rpm.c | 9 +++++---- drivers/regulator/qcom_rpm-regulator.c | 1 + include/linux/mfd/qcom_rpm.h | 5 ++++- 3 files changed, 10 insertions(+), 5 deletions(-)