* [PATCH] pinctrl: qcom: Add egpio feature support @ 2021-09-17 6:37 Rajendra Nayak 2021-09-17 16:00 ` Doug Anderson 2021-09-20 0:44 ` Bjorn Andersson 0 siblings, 2 replies; 10+ messages in thread From: Rajendra Nayak @ 2021-09-17 6:37 UTC (permalink / raw) To: bjorn.andersson, agross, linus.walleij Cc: linux-arm-msm, linux-gpio, linux-kernel, Prasad Sodagudi, Rajendra Nayak From: Prasad Sodagudi <psodagud@codeaurora.org> egpio is a scheme which allows special power Island Domain IOs (LPASS,SSC) to be reused as regular chip GPIOs by muxing regular TLMM functions with Island Domain functions. With this scheme, an IO can be controlled both by the cpu running linux and the Island processor. This provides great flexibility to re-purpose the Island IOs for regular TLMM usecases. 2 new bits are added to ctl_reg, egpio_present is a read only bit which shows if egpio feature is available or not on a given gpio. egpio_enable is the read/write bit and only effective if egpio_present is 1. Once its set, the Island IO is controlled from Chip TLMM. egpio_enable when set to 0 means the GPIO is used as Island Domain IO. The support exists on most recent qcom SoCs, and we add support for sm8150/sm8250/sm8350 and sc7280 as part of this patch. Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org> [rnayak: rewrite commit log, minor rebase] Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> --- drivers/pinctrl/qcom/pinctrl-msm.c | 4 ++++ drivers/pinctrl/qcom/pinctrl-msm.h | 2 ++ drivers/pinctrl/qcom/pinctrl-sc7280.c | 2 ++ drivers/pinctrl/qcom/pinctrl-sm8150.c | 2 ++ drivers/pinctrl/qcom/pinctrl-sm8250.c | 2 ++ drivers/pinctrl/qcom/pinctrl-sm8350.c | 2 ++ 6 files changed, 14 insertions(+) diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c index 8476a8a..f4a2343 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm.c +++ b/drivers/pinctrl/qcom/pinctrl-msm.c @@ -220,6 +220,10 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev, val = msm_readl_ctl(pctrl, g); val &= ~mask; val |= i << g->mux_bit; + /* Check if egpio present and enable that feature */ + if (val & BIT(g->egpio_present)) + val |= BIT(g->egpio_enable); + msm_writel_ctl(val, pctrl, g); raw_spin_unlock_irqrestore(&pctrl->lock, flags); diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h index e31a516..3635b31 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm.h +++ b/drivers/pinctrl/qcom/pinctrl-msm.h @@ -77,6 +77,8 @@ struct msm_pingroup { unsigned drv_bit:5; unsigned od_bit:5; + unsigned egpio_enable:5; + unsigned egpio_present:5; unsigned oe_bit:5; unsigned in_bit:5; unsigned out_bit:5; diff --git a/drivers/pinctrl/qcom/pinctrl-sc7280.c b/drivers/pinctrl/qcom/pinctrl-sc7280.c index afddf6d..607d459 100644 --- a/drivers/pinctrl/qcom/pinctrl-sc7280.c +++ b/drivers/pinctrl/qcom/pinctrl-sc7280.c @@ -43,6 +43,8 @@ .mux_bit = 2, \ .pull_bit = 0, \ .drv_bit = 6, \ + .egpio_enable = 12, \ + .egpio_present = 11, \ .oe_bit = 9, \ .in_bit = 0, \ .out_bit = 1, \ diff --git a/drivers/pinctrl/qcom/pinctrl-sm8150.c b/drivers/pinctrl/qcom/pinctrl-sm8150.c index 7359bae..63a625a 100644 --- a/drivers/pinctrl/qcom/pinctrl-sm8150.c +++ b/drivers/pinctrl/qcom/pinctrl-sm8150.c @@ -56,6 +56,8 @@ enum { .mux_bit = 2, \ .pull_bit = 0, \ .drv_bit = 6, \ + .egpio_enable = 12, \ + .egpio_present = 11, \ .oe_bit = 9, \ .in_bit = 0, \ .out_bit = 1, \ diff --git a/drivers/pinctrl/qcom/pinctrl-sm8250.c b/drivers/pinctrl/qcom/pinctrl-sm8250.c index af144e7..ad4fd94 100644 --- a/drivers/pinctrl/qcom/pinctrl-sm8250.c +++ b/drivers/pinctrl/qcom/pinctrl-sm8250.c @@ -57,6 +57,8 @@ enum { .mux_bit = 2, \ .pull_bit = 0, \ .drv_bit = 6, \ + .egpio_enable = 12, \ + .egpio_present = 11, \ .oe_bit = 9, \ .in_bit = 0, \ .out_bit = 1, \ diff --git a/drivers/pinctrl/qcom/pinctrl-sm8350.c b/drivers/pinctrl/qcom/pinctrl-sm8350.c index 4d8f863..bb436dc 100644 --- a/drivers/pinctrl/qcom/pinctrl-sm8350.c +++ b/drivers/pinctrl/qcom/pinctrl-sm8350.c @@ -46,6 +46,8 @@ .mux_bit = 2, \ .pull_bit = 0, \ .drv_bit = 6, \ + .egpio_enable = 12, \ + .egpio_present = 11, \ .oe_bit = 9, \ .in_bit = 0, \ .out_bit = 1, \ -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] pinctrl: qcom: Add egpio feature support 2021-09-17 6:37 [PATCH] pinctrl: qcom: Add egpio feature support Rajendra Nayak @ 2021-09-17 16:00 ` Doug Anderson 2021-09-20 0:44 ` Bjorn Andersson 1 sibling, 0 replies; 10+ messages in thread From: Doug Anderson @ 2021-09-17 16:00 UTC (permalink / raw) To: Rajendra Nayak Cc: Bjorn Andersson, Andy Gross, LinusW, linux-arm-msm, open list:GPIO SUBSYSTEM, LKML, Prasad Sodagudi Hi, On Thu, Sep 16, 2021 at 11:38 PM Rajendra Nayak <rnayak@codeaurora.org> wrote: > > From: Prasad Sodagudi <psodagud@codeaurora.org> > > egpio is a scheme which allows special power Island Domain IOs > (LPASS,SSC) to be reused as regular chip GPIOs by muxing regular > TLMM functions with Island Domain functions. > With this scheme, an IO can be controlled both by the cpu running > linux and the Island processor. This provides great flexibility to > re-purpose the Island IOs for regular TLMM usecases. > > 2 new bits are added to ctl_reg, egpio_present is a read only bit > which shows if egpio feature is available or not on a given gpio. > egpio_enable is the read/write bit and only effective if egpio_present > is 1. Once its set, the Island IO is controlled from Chip TLMM. > egpio_enable when set to 0 means the GPIO is used as Island Domain IO. > > The support exists on most recent qcom SoCs, and we add support > for sm8150/sm8250/sm8350 and sc7280 as part of this patch. > > Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org> > [rnayak: rewrite commit log, minor rebase] > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > --- > drivers/pinctrl/qcom/pinctrl-msm.c | 4 ++++ > drivers/pinctrl/qcom/pinctrl-msm.h | 2 ++ > drivers/pinctrl/qcom/pinctrl-sc7280.c | 2 ++ > drivers/pinctrl/qcom/pinctrl-sm8150.c | 2 ++ > drivers/pinctrl/qcom/pinctrl-sm8250.c | 2 ++ > drivers/pinctrl/qcom/pinctrl-sm8350.c | 2 ++ > 6 files changed, 14 insertions(+) > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > index 8476a8a..f4a2343 100644 > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > @@ -220,6 +220,10 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev, > val = msm_readl_ctl(pctrl, g); > val &= ~mask; > val |= i << g->mux_bit; > + /* Check if egpio present and enable that feature */ nit: blank line above the comment? > + if (val & BIT(g->egpio_present)) > + val |= BIT(g->egpio_enable); Thinking about this on systems that don't support egpio, g->egpio_present will be 0, right? BIT(0) = 1, so I guess this is equivalent to the code below on non-eGPIO systems: if (val & 1) val |= 1; I guess that's not the end of the world (it's a noop) and I can't think of anything better. In theory you could add a boolean "egpio_used" or you could just assume egpio is used if "egpio_present" != "egpio_enable", but all of those seem like a waste. Maybe just change the comment to something like: /* * Check if egpio present and enable that feature. For SoCs that * don't support egpio `egpio_present` will equal `egpio_enable` (they * will both be zero) and the statement below will be a no-op. */ Another question I have is: don't we need a way to turn off egpio_enable? Maybe this should be something like this: if (val & BIT(g->egpio_present)) { if (i == gpio_func) val |= BIT(g->egpio_enable); else val &= ~BIT(g->egpio_enable); } ...oh, but then you probably _do_ need to check if "egpio_present" != "egpio_enable" since the clearing of the bit won't be a no-op on non-egpio SoCs. -Doug ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pinctrl: qcom: Add egpio feature support 2021-09-17 6:37 [PATCH] pinctrl: qcom: Add egpio feature support Rajendra Nayak 2021-09-17 16:00 ` Doug Anderson @ 2021-09-20 0:44 ` Bjorn Andersson 2021-09-21 10:39 ` Rajendra Nayak 1 sibling, 1 reply; 10+ messages in thread From: Bjorn Andersson @ 2021-09-20 0:44 UTC (permalink / raw) To: Rajendra Nayak Cc: agross, linus.walleij, linux-arm-msm, linux-gpio, linux-kernel, Prasad Sodagudi On Fri 17 Sep 01:37 CDT 2021, Rajendra Nayak wrote: > From: Prasad Sodagudi <psodagud@codeaurora.org> > > egpio is a scheme which allows special power Island Domain IOs > (LPASS,SSC) to be reused as regular chip GPIOs by muxing regular > TLMM functions with Island Domain functions. > With this scheme, an IO can be controlled both by the cpu running > linux and the Island processor. This provides great flexibility to > re-purpose the Island IOs for regular TLMM usecases. > > 2 new bits are added to ctl_reg, egpio_present is a read only bit > which shows if egpio feature is available or not on a given gpio. > egpio_enable is the read/write bit and only effective if egpio_present > is 1. Once its set, the Island IO is controlled from Chip TLMM. > egpio_enable when set to 0 means the GPIO is used as Island Domain IO. > > The support exists on most recent qcom SoCs, and we add support > for sm8150/sm8250/sm8350 and sc7280 as part of this patch. > I was under the impression that this feature would allow you to repurpose pins for use either by the remote island or by apps. But if I understand your proposal, you check to see if the pin is "egpio capable" for a pin and if so just sets the bit - muxing it to apps (or the island?). It seems reasonable that this would be another pinmux state for these pins, rather than just flipping them all in one or the other direction. PS. When I spoke with Prasad about this a couple of years ago, I think we talked about representing this as a pinconf property, but it seems to make more sense to me now that it would be a pinmux state. Regards, Bjorn > Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org> > [rnayak: rewrite commit log, minor rebase] > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > --- > drivers/pinctrl/qcom/pinctrl-msm.c | 4 ++++ > drivers/pinctrl/qcom/pinctrl-msm.h | 2 ++ > drivers/pinctrl/qcom/pinctrl-sc7280.c | 2 ++ > drivers/pinctrl/qcom/pinctrl-sm8150.c | 2 ++ > drivers/pinctrl/qcom/pinctrl-sm8250.c | 2 ++ > drivers/pinctrl/qcom/pinctrl-sm8350.c | 2 ++ > 6 files changed, 14 insertions(+) > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > index 8476a8a..f4a2343 100644 > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > @@ -220,6 +220,10 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev, > val = msm_readl_ctl(pctrl, g); > val &= ~mask; > val |= i << g->mux_bit; > + /* Check if egpio present and enable that feature */ > + if (val & BIT(g->egpio_present)) > + val |= BIT(g->egpio_enable); > + > msm_writel_ctl(val, pctrl, g); > > raw_spin_unlock_irqrestore(&pctrl->lock, flags); > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h > index e31a516..3635b31 100644 > --- a/drivers/pinctrl/qcom/pinctrl-msm.h > +++ b/drivers/pinctrl/qcom/pinctrl-msm.h > @@ -77,6 +77,8 @@ struct msm_pingroup { > unsigned drv_bit:5; > > unsigned od_bit:5; > + unsigned egpio_enable:5; > + unsigned egpio_present:5; > unsigned oe_bit:5; > unsigned in_bit:5; > unsigned out_bit:5; > diff --git a/drivers/pinctrl/qcom/pinctrl-sc7280.c b/drivers/pinctrl/qcom/pinctrl-sc7280.c > index afddf6d..607d459 100644 > --- a/drivers/pinctrl/qcom/pinctrl-sc7280.c > +++ b/drivers/pinctrl/qcom/pinctrl-sc7280.c > @@ -43,6 +43,8 @@ > .mux_bit = 2, \ > .pull_bit = 0, \ > .drv_bit = 6, \ > + .egpio_enable = 12, \ > + .egpio_present = 11, \ > .oe_bit = 9, \ > .in_bit = 0, \ > .out_bit = 1, \ > diff --git a/drivers/pinctrl/qcom/pinctrl-sm8150.c b/drivers/pinctrl/qcom/pinctrl-sm8150.c > index 7359bae..63a625a 100644 > --- a/drivers/pinctrl/qcom/pinctrl-sm8150.c > +++ b/drivers/pinctrl/qcom/pinctrl-sm8150.c > @@ -56,6 +56,8 @@ enum { > .mux_bit = 2, \ > .pull_bit = 0, \ > .drv_bit = 6, \ > + .egpio_enable = 12, \ > + .egpio_present = 11, \ > .oe_bit = 9, \ > .in_bit = 0, \ > .out_bit = 1, \ > diff --git a/drivers/pinctrl/qcom/pinctrl-sm8250.c b/drivers/pinctrl/qcom/pinctrl-sm8250.c > index af144e7..ad4fd94 100644 > --- a/drivers/pinctrl/qcom/pinctrl-sm8250.c > +++ b/drivers/pinctrl/qcom/pinctrl-sm8250.c > @@ -57,6 +57,8 @@ enum { > .mux_bit = 2, \ > .pull_bit = 0, \ > .drv_bit = 6, \ > + .egpio_enable = 12, \ > + .egpio_present = 11, \ > .oe_bit = 9, \ > .in_bit = 0, \ > .out_bit = 1, \ > diff --git a/drivers/pinctrl/qcom/pinctrl-sm8350.c b/drivers/pinctrl/qcom/pinctrl-sm8350.c > index 4d8f863..bb436dc 100644 > --- a/drivers/pinctrl/qcom/pinctrl-sm8350.c > +++ b/drivers/pinctrl/qcom/pinctrl-sm8350.c > @@ -46,6 +46,8 @@ > .mux_bit = 2, \ > .pull_bit = 0, \ > .drv_bit = 6, \ > + .egpio_enable = 12, \ > + .egpio_present = 11, \ > .oe_bit = 9, \ > .in_bit = 0, \ > .out_bit = 1, \ > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pinctrl: qcom: Add egpio feature support 2021-09-20 0:44 ` Bjorn Andersson @ 2021-09-21 10:39 ` Rajendra Nayak 2021-09-21 15:56 ` Linus Walleij 2021-09-21 16:26 ` Bjorn Andersson 0 siblings, 2 replies; 10+ messages in thread From: Rajendra Nayak @ 2021-09-21 10:39 UTC (permalink / raw) To: Bjorn Andersson Cc: agross, linus.walleij, linux-arm-msm, linux-gpio, linux-kernel, Prasad Sodagudi On 9/20/2021 6:14 AM, Bjorn Andersson wrote: > On Fri 17 Sep 01:37 CDT 2021, Rajendra Nayak wrote: > >> From: Prasad Sodagudi <psodagud@codeaurora.org> >> >> egpio is a scheme which allows special power Island Domain IOs >> (LPASS,SSC) to be reused as regular chip GPIOs by muxing regular >> TLMM functions with Island Domain functions. >> With this scheme, an IO can be controlled both by the cpu running >> linux and the Island processor. This provides great flexibility to >> re-purpose the Island IOs for regular TLMM usecases. >> >> 2 new bits are added to ctl_reg, egpio_present is a read only bit >> which shows if egpio feature is available or not on a given gpio. >> egpio_enable is the read/write bit and only effective if egpio_present >> is 1. Once its set, the Island IO is controlled from Chip TLMM. >> egpio_enable when set to 0 means the GPIO is used as Island Domain IO. >> >> The support exists on most recent qcom SoCs, and we add support >> for sm8150/sm8250/sm8350 and sc7280 as part of this patch. >> > > I was under the impression that this feature would allow you to > repurpose pins for use either by the remote island or by apps. thats right, you can repurpose the pins for usage by apps by setting the egpio_enable to 1, when set to 0 its owned by the island processor. > > But if I understand your proposal, you check to see if the pin is > "egpio capable" for a pin and if so just sets the bit - muxing it to > apps (or the island?). Right, so if there is a request for a egpio-capable pin, the driver flips the ownership. Are you suggesting having some kind of checks to determine who should own it? > It seems reasonable that this would be another pinmux state for these > pins, rather than just flipping them all in one or the other direction. hmm, I don't understand. This is not a pinmux state, its a switch to decide the ownership. These egpio pins have regulator mux functions, some for apps, some for the island processor, they might not always be used as gpios. > PS. When I spoke with Prasad about this a couple of years ago, I think > we talked about representing this as a pinconf property, but it seems to > make more sense to me now that it would be a pinmux state. > > Regards, > Bjorn > >> Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org> >> [rnayak: rewrite commit log, minor rebase] >> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> >> --- >> drivers/pinctrl/qcom/pinctrl-msm.c | 4 ++++ >> drivers/pinctrl/qcom/pinctrl-msm.h | 2 ++ >> drivers/pinctrl/qcom/pinctrl-sc7280.c | 2 ++ >> drivers/pinctrl/qcom/pinctrl-sm8150.c | 2 ++ >> drivers/pinctrl/qcom/pinctrl-sm8250.c | 2 ++ >> drivers/pinctrl/qcom/pinctrl-sm8350.c | 2 ++ >> 6 files changed, 14 insertions(+) >> >> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c >> index 8476a8a..f4a2343 100644 >> --- a/drivers/pinctrl/qcom/pinctrl-msm.c >> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c >> @@ -220,6 +220,10 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev, >> val = msm_readl_ctl(pctrl, g); >> val &= ~mask; >> val |= i << g->mux_bit; >> + /* Check if egpio present and enable that feature */ >> + if (val & BIT(g->egpio_present)) >> + val |= BIT(g->egpio_enable); >> + >> msm_writel_ctl(val, pctrl, g); >> >> raw_spin_unlock_irqrestore(&pctrl->lock, flags); >> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h >> index e31a516..3635b31 100644 >> --- a/drivers/pinctrl/qcom/pinctrl-msm.h >> +++ b/drivers/pinctrl/qcom/pinctrl-msm.h >> @@ -77,6 +77,8 @@ struct msm_pingroup { >> unsigned drv_bit:5; >> >> unsigned od_bit:5; >> + unsigned egpio_enable:5; >> + unsigned egpio_present:5; >> unsigned oe_bit:5; >> unsigned in_bit:5; >> unsigned out_bit:5; >> diff --git a/drivers/pinctrl/qcom/pinctrl-sc7280.c b/drivers/pinctrl/qcom/pinctrl-sc7280.c >> index afddf6d..607d459 100644 >> --- a/drivers/pinctrl/qcom/pinctrl-sc7280.c >> +++ b/drivers/pinctrl/qcom/pinctrl-sc7280.c >> @@ -43,6 +43,8 @@ >> .mux_bit = 2, \ >> .pull_bit = 0, \ >> .drv_bit = 6, \ >> + .egpio_enable = 12, \ >> + .egpio_present = 11, \ >> .oe_bit = 9, \ >> .in_bit = 0, \ >> .out_bit = 1, \ >> diff --git a/drivers/pinctrl/qcom/pinctrl-sm8150.c b/drivers/pinctrl/qcom/pinctrl-sm8150.c >> index 7359bae..63a625a 100644 >> --- a/drivers/pinctrl/qcom/pinctrl-sm8150.c >> +++ b/drivers/pinctrl/qcom/pinctrl-sm8150.c >> @@ -56,6 +56,8 @@ enum { >> .mux_bit = 2, \ >> .pull_bit = 0, \ >> .drv_bit = 6, \ >> + .egpio_enable = 12, \ >> + .egpio_present = 11, \ >> .oe_bit = 9, \ >> .in_bit = 0, \ >> .out_bit = 1, \ >> diff --git a/drivers/pinctrl/qcom/pinctrl-sm8250.c b/drivers/pinctrl/qcom/pinctrl-sm8250.c >> index af144e7..ad4fd94 100644 >> --- a/drivers/pinctrl/qcom/pinctrl-sm8250.c >> +++ b/drivers/pinctrl/qcom/pinctrl-sm8250.c >> @@ -57,6 +57,8 @@ enum { >> .mux_bit = 2, \ >> .pull_bit = 0, \ >> .drv_bit = 6, \ >> + .egpio_enable = 12, \ >> + .egpio_present = 11, \ >> .oe_bit = 9, \ >> .in_bit = 0, \ >> .out_bit = 1, \ >> diff --git a/drivers/pinctrl/qcom/pinctrl-sm8350.c b/drivers/pinctrl/qcom/pinctrl-sm8350.c >> index 4d8f863..bb436dc 100644 >> --- a/drivers/pinctrl/qcom/pinctrl-sm8350.c >> +++ b/drivers/pinctrl/qcom/pinctrl-sm8350.c >> @@ -46,6 +46,8 @@ >> .mux_bit = 2, \ >> .pull_bit = 0, \ >> .drv_bit = 6, \ >> + .egpio_enable = 12, \ >> + .egpio_present = 11, \ >> .oe_bit = 9, \ >> .in_bit = 0, \ >> .out_bit = 1, \ >> -- >> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member >> of Code Aurora Forum, hosted by The Linux Foundation >> -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pinctrl: qcom: Add egpio feature support 2021-09-21 10:39 ` Rajendra Nayak @ 2021-09-21 15:56 ` Linus Walleij 2021-09-21 16:26 ` Bjorn Andersson 1 sibling, 0 replies; 10+ messages in thread From: Linus Walleij @ 2021-09-21 15:56 UTC (permalink / raw) To: Rajendra Nayak Cc: Bjorn Andersson, Andy Gross, MSM, open list:GPIO SUBSYSTEM, linux-kernel, Prasad Sodagudi On Tue, Sep 21, 2021 at 12:39 PM Rajendra Nayak <rnayak@codeaurora.org> wrote: > On 9/20/2021 6:14 AM, Bjorn Andersson wrote: > > It seems reasonable that this would be another pinmux state for these > > pins, rather than just flipping them all in one or the other direction. > > hmm, I don't understand. This is not a pinmux state, its a switch to decide > the ownership. > These egpio pins have regulator mux functions, some for apps, some for the > island processor, they might not always be used as gpios. The pinmux API is for muliplexing a pin between owners, and that is what is going on is it not? If the bit(s) select different processors that is changing the owner and creating a usage matrix, e.g. TLMM_GPIO ISLAND_GPIO TLMM_I2C ISLAND_I2C etc etc They become different muxing states nevertheless. How to encode them, such as (a) bitfields in the enums, (b) more cells in the device tree or (c) ... that is another question. Shoehorning it into pinconf is not the answer IMO. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pinctrl: qcom: Add egpio feature support 2021-09-21 10:39 ` Rajendra Nayak 2021-09-21 15:56 ` Linus Walleij @ 2021-09-21 16:26 ` Bjorn Andersson 2021-09-30 9:46 ` Rajendra Nayak 1 sibling, 1 reply; 10+ messages in thread From: Bjorn Andersson @ 2021-09-21 16:26 UTC (permalink / raw) To: Rajendra Nayak Cc: agross, linus.walleij, linux-arm-msm, linux-gpio, linux-kernel, Prasad Sodagudi On Tue 21 Sep 03:39 PDT 2021, Rajendra Nayak wrote: > > > On 9/20/2021 6:14 AM, Bjorn Andersson wrote: > > On Fri 17 Sep 01:37 CDT 2021, Rajendra Nayak wrote: > > > > > From: Prasad Sodagudi <psodagud@codeaurora.org> > > > > > > egpio is a scheme which allows special power Island Domain IOs > > > (LPASS,SSC) to be reused as regular chip GPIOs by muxing regular > > > TLMM functions with Island Domain functions. > > > With this scheme, an IO can be controlled both by the cpu running > > > linux and the Island processor. This provides great flexibility to > > > re-purpose the Island IOs for regular TLMM usecases. > > > > > > 2 new bits are added to ctl_reg, egpio_present is a read only bit > > > which shows if egpio feature is available or not on a given gpio. > > > egpio_enable is the read/write bit and only effective if egpio_present > > > is 1. Once its set, the Island IO is controlled from Chip TLMM. > > > egpio_enable when set to 0 means the GPIO is used as Island Domain IO. > > > > > > The support exists on most recent qcom SoCs, and we add support > > > for sm8150/sm8250/sm8350 and sc7280 as part of this patch. > > > > > > > I was under the impression that this feature would allow you to > > repurpose pins for use either by the remote island or by apps. > > thats right, you can repurpose the pins for usage by apps by setting > the egpio_enable to 1, when set to 0 its owned by the island processor. Good. > > > > But if I understand your proposal, you check to see if the pin is > > "egpio capable" for a pin and if so just sets the bit - muxing it to > > apps (or the island?). > > Right, so if there is a request for a egpio-capable pin, the driver > flips the ownership. Are you suggesting having some kind of checks to determine > who should own it? > I see, I missed that nuance. So Linux will steal any pins that are mentioned in DT. But that would mean that you're relying on someone else to ensure that this bit is cleared for the other pins and you would not be able to explicitly flip the state back to island mode in runtime. I would prefer that this was more explicit. > > It seems reasonable that this would be another pinmux state for these > > pins, rather than just flipping them all in one or the other direction. > > hmm, I don't understand. This is not a pinmux state, its a switch to decide > the ownership. But does it mux the pin to the island, or does it state that the island is now in charge of the associated TLMM registers? If it's muxing the pin to the island, then it's conceptually just a special mux state that we can represent in DT as another pinmux function. > These egpio pins have regulator mux functions, some for apps, some for the > island processor, they might not always be used as gpios. Right, so if egpio = 1 for a pin, then it works like any other pin that's handled by the pinctrl-msm driver, with whatever muxing options are available for the given pin. But what happens when egpio = 0? Is the TLMM decoupled from the physical pin, or does the island use the TLMM as well? If it's not using the TLMM, do we need the TLMM to be in some particular state? What I'm proposing is that if the egpio is simply a first-line mux and the TLMM isn't used when the pin is muxed towards the island. Then we could add a custom 'function = "egpio"' muxing state and you could per pin mux things explicitly, and possibly dynamically, to be routed to the island. Regards, Bjorn > > PS. When I spoke with Prasad about this a couple of years ago, I think > > we talked about representing this as a pinconf property, but it seems to > > make more sense to me now that it would be a pinmux state. > > > > Regards, > > Bjorn > > > > > Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org> > > > [rnayak: rewrite commit log, minor rebase] > > > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > > > --- > > > drivers/pinctrl/qcom/pinctrl-msm.c | 4 ++++ > > > drivers/pinctrl/qcom/pinctrl-msm.h | 2 ++ > > > drivers/pinctrl/qcom/pinctrl-sc7280.c | 2 ++ > > > drivers/pinctrl/qcom/pinctrl-sm8150.c | 2 ++ > > > drivers/pinctrl/qcom/pinctrl-sm8250.c | 2 ++ > > > drivers/pinctrl/qcom/pinctrl-sm8350.c | 2 ++ > > > 6 files changed, 14 insertions(+) > > > > > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > > > index 8476a8a..f4a2343 100644 > > > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > > > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > > > @@ -220,6 +220,10 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev, > > > val = msm_readl_ctl(pctrl, g); > > > val &= ~mask; > > > val |= i << g->mux_bit; > > > + /* Check if egpio present and enable that feature */ > > > + if (val & BIT(g->egpio_present)) > > > + val |= BIT(g->egpio_enable); > > > + > > > msm_writel_ctl(val, pctrl, g); > > > raw_spin_unlock_irqrestore(&pctrl->lock, flags); > > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h > > > index e31a516..3635b31 100644 > > > --- a/drivers/pinctrl/qcom/pinctrl-msm.h > > > +++ b/drivers/pinctrl/qcom/pinctrl-msm.h > > > @@ -77,6 +77,8 @@ struct msm_pingroup { > > > unsigned drv_bit:5; > > > unsigned od_bit:5; > > > + unsigned egpio_enable:5; > > > + unsigned egpio_present:5; > > > unsigned oe_bit:5; > > > unsigned in_bit:5; > > > unsigned out_bit:5; > > > diff --git a/drivers/pinctrl/qcom/pinctrl-sc7280.c b/drivers/pinctrl/qcom/pinctrl-sc7280.c > > > index afddf6d..607d459 100644 > > > --- a/drivers/pinctrl/qcom/pinctrl-sc7280.c > > > +++ b/drivers/pinctrl/qcom/pinctrl-sc7280.c > > > @@ -43,6 +43,8 @@ > > > .mux_bit = 2, \ > > > .pull_bit = 0, \ > > > .drv_bit = 6, \ > > > + .egpio_enable = 12, \ > > > + .egpio_present = 11, \ > > > .oe_bit = 9, \ > > > .in_bit = 0, \ > > > .out_bit = 1, \ > > > diff --git a/drivers/pinctrl/qcom/pinctrl-sm8150.c b/drivers/pinctrl/qcom/pinctrl-sm8150.c > > > index 7359bae..63a625a 100644 > > > --- a/drivers/pinctrl/qcom/pinctrl-sm8150.c > > > +++ b/drivers/pinctrl/qcom/pinctrl-sm8150.c > > > @@ -56,6 +56,8 @@ enum { > > > .mux_bit = 2, \ > > > .pull_bit = 0, \ > > > .drv_bit = 6, \ > > > + .egpio_enable = 12, \ > > > + .egpio_present = 11, \ > > > .oe_bit = 9, \ > > > .in_bit = 0, \ > > > .out_bit = 1, \ > > > diff --git a/drivers/pinctrl/qcom/pinctrl-sm8250.c b/drivers/pinctrl/qcom/pinctrl-sm8250.c > > > index af144e7..ad4fd94 100644 > > > --- a/drivers/pinctrl/qcom/pinctrl-sm8250.c > > > +++ b/drivers/pinctrl/qcom/pinctrl-sm8250.c > > > @@ -57,6 +57,8 @@ enum { > > > .mux_bit = 2, \ > > > .pull_bit = 0, \ > > > .drv_bit = 6, \ > > > + .egpio_enable = 12, \ > > > + .egpio_present = 11, \ > > > .oe_bit = 9, \ > > > .in_bit = 0, \ > > > .out_bit = 1, \ > > > diff --git a/drivers/pinctrl/qcom/pinctrl-sm8350.c b/drivers/pinctrl/qcom/pinctrl-sm8350.c > > > index 4d8f863..bb436dc 100644 > > > --- a/drivers/pinctrl/qcom/pinctrl-sm8350.c > > > +++ b/drivers/pinctrl/qcom/pinctrl-sm8350.c > > > @@ -46,6 +46,8 @@ > > > .mux_bit = 2, \ > > > .pull_bit = 0, \ > > > .drv_bit = 6, \ > > > + .egpio_enable = 12, \ > > > + .egpio_present = 11, \ > > > .oe_bit = 9, \ > > > .in_bit = 0, \ > > > .out_bit = 1, \ > > > -- > > > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > > > of Code Aurora Forum, hosted by The Linux Foundation > > > > > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pinctrl: qcom: Add egpio feature support 2021-09-21 16:26 ` Bjorn Andersson @ 2021-09-30 9:46 ` Rajendra Nayak 2021-09-30 14:55 ` Bjorn Andersson 0 siblings, 1 reply; 10+ messages in thread From: Rajendra Nayak @ 2021-09-30 9:46 UTC (permalink / raw) To: Bjorn Andersson Cc: agross, linus.walleij, linux-arm-msm, linux-gpio, linux-kernel, Prasad Sodagudi On 9/21/2021 9:56 PM, Bjorn Andersson wrote: > On Tue 21 Sep 03:39 PDT 2021, Rajendra Nayak wrote: > >> >> >> On 9/20/2021 6:14 AM, Bjorn Andersson wrote: >>> On Fri 17 Sep 01:37 CDT 2021, Rajendra Nayak wrote: >>> >>>> From: Prasad Sodagudi <psodagud@codeaurora.org> >>>> >>>> egpio is a scheme which allows special power Island Domain IOs >>>> (LPASS,SSC) to be reused as regular chip GPIOs by muxing regular >>>> TLMM functions with Island Domain functions. >>>> With this scheme, an IO can be controlled both by the cpu running >>>> linux and the Island processor. This provides great flexibility to >>>> re-purpose the Island IOs for regular TLMM usecases. >>>> >>>> 2 new bits are added to ctl_reg, egpio_present is a read only bit >>>> which shows if egpio feature is available or not on a given gpio. >>>> egpio_enable is the read/write bit and only effective if egpio_present >>>> is 1. Once its set, the Island IO is controlled from Chip TLMM. >>>> egpio_enable when set to 0 means the GPIO is used as Island Domain IO. >>>> >>>> The support exists on most recent qcom SoCs, and we add support >>>> for sm8150/sm8250/sm8350 and sc7280 as part of this patch. >>>> >>> >>> I was under the impression that this feature would allow you to >>> repurpose pins for use either by the remote island or by apps. >> >> thats right, you can repurpose the pins for usage by apps by setting >> the egpio_enable to 1, when set to 0 its owned by the island processor. > > Good. > >>> >>> But if I understand your proposal, you check to see if the pin is >>> "egpio capable" for a pin and if so just sets the bit - muxing it to >>> apps (or the island?). >> >> Right, so if there is a request for a egpio-capable pin, the driver >> flips the ownership. Are you suggesting having some kind of checks to determine >> who should own it? >> > > I see, I missed that nuance. So Linux will steal any pins that are > mentioned in DT. But that would mean that you're relying on someone else > to ensure that this bit is cleared for the other pins and you would not > be able to explicitly flip the state back to island mode in runtime. > > I would prefer that this was more explicit. > >>> It seems reasonable that this would be another pinmux state for these >>> pins, rather than just flipping them all in one or the other direction. >> >> hmm, I don't understand. This is not a pinmux state, its a switch to decide >> the ownership. > > But does it mux the pin to the island, or does it state that the island > is now in charge of the associated TLMM registers? The island processor does not access the APPS TLMM register space, it has its own TLMM register space that it configures. APPS TLMM registers control its mux/conf settings and Island TLMM registers controls its mux/conf. So essentially there are 2 sets of registers to control the same pin. This bit is more like a top level mux which decides what register settings take affect. > If it's muxing the pin to the island, then it's conceptually just a > special mux state that we can represent in DT as another pinmux > function. > >> These egpio pins have regulator mux functions, some for apps, some for the >> island processor, they might not always be used as gpios. > > Right, so if egpio = 1 for a pin, then it works like any other pin > that's handled by the pinctrl-msm driver, with whatever muxing options > are available for the given pin. > > But what happens when egpio = 0? Is the TLMM decoupled from the physical > pin, or does the island use the TLMM as well? > > If it's not using the TLMM, do we need the TLMM to be in some particular > state? > > > What I'm proposing is that if the egpio is simply a first-line mux and > the TLMM isn't used when the pin is muxed towards the island. Then we > could add a custom 'function = "egpio"' muxing state and you could per > pin mux things explicitly, and possibly dynamically, to be routed to the > island. > > Regards, > Bjorn > >>> PS. When I spoke with Prasad about this a couple of years ago, I think >>> we talked about representing this as a pinconf property, but it seems to >>> make more sense to me now that it would be a pinmux state. >>> >>> Regards, >>> Bjorn >>> >>>> Signed-off-by: Prasad Sodagudi <psodagud@codeaurora.org> >>>> [rnayak: rewrite commit log, minor rebase] >>>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> >>>> --- >>>> drivers/pinctrl/qcom/pinctrl-msm.c | 4 ++++ >>>> drivers/pinctrl/qcom/pinctrl-msm.h | 2 ++ >>>> drivers/pinctrl/qcom/pinctrl-sc7280.c | 2 ++ >>>> drivers/pinctrl/qcom/pinctrl-sm8150.c | 2 ++ >>>> drivers/pinctrl/qcom/pinctrl-sm8250.c | 2 ++ >>>> drivers/pinctrl/qcom/pinctrl-sm8350.c | 2 ++ >>>> 6 files changed, 14 insertions(+) >>>> >>>> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c >>>> index 8476a8a..f4a2343 100644 >>>> --- a/drivers/pinctrl/qcom/pinctrl-msm.c >>>> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c >>>> @@ -220,6 +220,10 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev, >>>> val = msm_readl_ctl(pctrl, g); >>>> val &= ~mask; >>>> val |= i << g->mux_bit; >>>> + /* Check if egpio present and enable that feature */ >>>> + if (val & BIT(g->egpio_present)) >>>> + val |= BIT(g->egpio_enable); >>>> + >>>> msm_writel_ctl(val, pctrl, g); >>>> raw_spin_unlock_irqrestore(&pctrl->lock, flags); >>>> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h >>>> index e31a516..3635b31 100644 >>>> --- a/drivers/pinctrl/qcom/pinctrl-msm.h >>>> +++ b/drivers/pinctrl/qcom/pinctrl-msm.h >>>> @@ -77,6 +77,8 @@ struct msm_pingroup { >>>> unsigned drv_bit:5; >>>> unsigned od_bit:5; >>>> + unsigned egpio_enable:5; >>>> + unsigned egpio_present:5; >>>> unsigned oe_bit:5; >>>> unsigned in_bit:5; >>>> unsigned out_bit:5; >>>> diff --git a/drivers/pinctrl/qcom/pinctrl-sc7280.c b/drivers/pinctrl/qcom/pinctrl-sc7280.c >>>> index afddf6d..607d459 100644 >>>> --- a/drivers/pinctrl/qcom/pinctrl-sc7280.c >>>> +++ b/drivers/pinctrl/qcom/pinctrl-sc7280.c >>>> @@ -43,6 +43,8 @@ >>>> .mux_bit = 2, \ >>>> .pull_bit = 0, \ >>>> .drv_bit = 6, \ >>>> + .egpio_enable = 12, \ >>>> + .egpio_present = 11, \ >>>> .oe_bit = 9, \ >>>> .in_bit = 0, \ >>>> .out_bit = 1, \ >>>> diff --git a/drivers/pinctrl/qcom/pinctrl-sm8150.c b/drivers/pinctrl/qcom/pinctrl-sm8150.c >>>> index 7359bae..63a625a 100644 >>>> --- a/drivers/pinctrl/qcom/pinctrl-sm8150.c >>>> +++ b/drivers/pinctrl/qcom/pinctrl-sm8150.c >>>> @@ -56,6 +56,8 @@ enum { >>>> .mux_bit = 2, \ >>>> .pull_bit = 0, \ >>>> .drv_bit = 6, \ >>>> + .egpio_enable = 12, \ >>>> + .egpio_present = 11, \ >>>> .oe_bit = 9, \ >>>> .in_bit = 0, \ >>>> .out_bit = 1, \ >>>> diff --git a/drivers/pinctrl/qcom/pinctrl-sm8250.c b/drivers/pinctrl/qcom/pinctrl-sm8250.c >>>> index af144e7..ad4fd94 100644 >>>> --- a/drivers/pinctrl/qcom/pinctrl-sm8250.c >>>> +++ b/drivers/pinctrl/qcom/pinctrl-sm8250.c >>>> @@ -57,6 +57,8 @@ enum { >>>> .mux_bit = 2, \ >>>> .pull_bit = 0, \ >>>> .drv_bit = 6, \ >>>> + .egpio_enable = 12, \ >>>> + .egpio_present = 11, \ >>>> .oe_bit = 9, \ >>>> .in_bit = 0, \ >>>> .out_bit = 1, \ >>>> diff --git a/drivers/pinctrl/qcom/pinctrl-sm8350.c b/drivers/pinctrl/qcom/pinctrl-sm8350.c >>>> index 4d8f863..bb436dc 100644 >>>> --- a/drivers/pinctrl/qcom/pinctrl-sm8350.c >>>> +++ b/drivers/pinctrl/qcom/pinctrl-sm8350.c >>>> @@ -46,6 +46,8 @@ >>>> .mux_bit = 2, \ >>>> .pull_bit = 0, \ >>>> .drv_bit = 6, \ >>>> + .egpio_enable = 12, \ >>>> + .egpio_present = 11, \ >>>> .oe_bit = 9, \ >>>> .in_bit = 0, \ >>>> .out_bit = 1, \ >>>> -- >>>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member >>>> of Code Aurora Forum, hosted by The Linux Foundation >>>> >> >> -- >> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member >> of Code Aurora Forum, hosted by The Linux Foundation -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pinctrl: qcom: Add egpio feature support 2021-09-30 9:46 ` Rajendra Nayak @ 2021-09-30 14:55 ` Bjorn Andersson 2021-10-01 5:34 ` Rajendra Nayak 0 siblings, 1 reply; 10+ messages in thread From: Bjorn Andersson @ 2021-09-30 14:55 UTC (permalink / raw) To: Rajendra Nayak Cc: agross, linus.walleij, linux-arm-msm, linux-gpio, linux-kernel, Prasad Sodagudi On Thu 30 Sep 02:46 PDT 2021, Rajendra Nayak wrote: > > > On 9/21/2021 9:56 PM, Bjorn Andersson wrote: > > On Tue 21 Sep 03:39 PDT 2021, Rajendra Nayak wrote: > > > > > > > > > > > On 9/20/2021 6:14 AM, Bjorn Andersson wrote: > > > > On Fri 17 Sep 01:37 CDT 2021, Rajendra Nayak wrote: > > > > > > > > > From: Prasad Sodagudi <psodagud@codeaurora.org> > > > > > > > > > > egpio is a scheme which allows special power Island Domain IOs > > > > > (LPASS,SSC) to be reused as regular chip GPIOs by muxing regular > > > > > TLMM functions with Island Domain functions. > > > > > With this scheme, an IO can be controlled both by the cpu running > > > > > linux and the Island processor. This provides great flexibility to > > > > > re-purpose the Island IOs for regular TLMM usecases. > > > > > > > > > > 2 new bits are added to ctl_reg, egpio_present is a read only bit > > > > > which shows if egpio feature is available or not on a given gpio. > > > > > egpio_enable is the read/write bit and only effective if egpio_present > > > > > is 1. Once its set, the Island IO is controlled from Chip TLMM. > > > > > egpio_enable when set to 0 means the GPIO is used as Island Domain IO. > > > > > > > > > > The support exists on most recent qcom SoCs, and we add support > > > > > for sm8150/sm8250/sm8350 and sc7280 as part of this patch. > > > > > > > > > > > > > I was under the impression that this feature would allow you to > > > > repurpose pins for use either by the remote island or by apps. > > > > > > thats right, you can repurpose the pins for usage by apps by setting > > > the egpio_enable to 1, when set to 0 its owned by the island processor. > > > > Good. > > > > > > > > > > But if I understand your proposal, you check to see if the pin is > > > > "egpio capable" for a pin and if so just sets the bit - muxing it to > > > > apps (or the island?). > > > > > > Right, so if there is a request for a egpio-capable pin, the driver > > > flips the ownership. Are you suggesting having some kind of checks to determine > > > who should own it? > > > > > > > I see, I missed that nuance. So Linux will steal any pins that are > > mentioned in DT. But that would mean that you're relying on someone else > > to ensure that this bit is cleared for the other pins and you would not > > be able to explicitly flip the state back to island mode in runtime. > > > > I would prefer that this was more explicit. > > > > > > It seems reasonable that this would be another pinmux state for these > > > > pins, rather than just flipping them all in one or the other direction. > > > > > > hmm, I don't understand. This is not a pinmux state, its a switch to decide > > > the ownership. > > > > But does it mux the pin to the island, or does it state that the island > > is now in charge of the associated TLMM registers? > > The island processor does not access the APPS TLMM register space, it has its > own TLMM register space that it configures. APPS TLMM registers control its > mux/conf settings and Island TLMM registers controls its mux/conf. So essentially > there are 2 sets of registers to control the same pin. > This bit is more like a top level mux which decides what register settings > take affect. > "One mux to rule them all" :) When we switch this mux towards the Island TLMM, do we need to configure the APPS TLMM in a particular way, or does the state of that not matter? Would it be reasonable to say that when muxed towards the island the apps should always be in gpio mux with some predetermined properties, to save power? To reiterate, as proposed, mentioning a egpio-capable pin in the apps DTS will cause it to be muxed to the APSS TLMM. But I'm not convinced that we don't have scenarios where one might want to dynamically mux the pin between island and apss tlmm. My suggestion is that even that it's two independent muxes controlled in the apps tlmm, we'd express them in the same pinmux, i.e. we'd have something like: some-local-state { pins = "gpio1"; function = "gpio"; output-high; }; some-remote-state { pins = "gpio1"; function = "island"; /* or just egpio... ? */ }; One case I imaging where this could be useful is to allow Linux to configure a known state of pins when the island isn't running, from the remoteproc driver and then flip it over to island mode before booting the remote. Regards, Bjorn ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pinctrl: qcom: Add egpio feature support 2021-09-30 14:55 ` Bjorn Andersson @ 2021-10-01 5:34 ` Rajendra Nayak 2021-10-04 20:43 ` Bjorn Andersson 0 siblings, 1 reply; 10+ messages in thread From: Rajendra Nayak @ 2021-10-01 5:34 UTC (permalink / raw) To: Bjorn Andersson Cc: agross, linus.walleij, linux-arm-msm, linux-gpio, linux-kernel, Prasad Sodagudi On 9/30/2021 8:25 PM, Bjorn Andersson wrote: > On Thu 30 Sep 02:46 PDT 2021, Rajendra Nayak wrote: > >> >> >> On 9/21/2021 9:56 PM, Bjorn Andersson wrote: >>> On Tue 21 Sep 03:39 PDT 2021, Rajendra Nayak wrote: >>> >>>> >>>> >>>> On 9/20/2021 6:14 AM, Bjorn Andersson wrote: >>>>> On Fri 17 Sep 01:37 CDT 2021, Rajendra Nayak wrote: >>>>> >>>>>> From: Prasad Sodagudi <psodagud@codeaurora.org> >>>>>> >>>>>> egpio is a scheme which allows special power Island Domain IOs >>>>>> (LPASS,SSC) to be reused as regular chip GPIOs by muxing regular >>>>>> TLMM functions with Island Domain functions. >>>>>> With this scheme, an IO can be controlled both by the cpu running >>>>>> linux and the Island processor. This provides great flexibility to >>>>>> re-purpose the Island IOs for regular TLMM usecases. >>>>>> >>>>>> 2 new bits are added to ctl_reg, egpio_present is a read only bit >>>>>> which shows if egpio feature is available or not on a given gpio. >>>>>> egpio_enable is the read/write bit and only effective if egpio_present >>>>>> is 1. Once its set, the Island IO is controlled from Chip TLMM. >>>>>> egpio_enable when set to 0 means the GPIO is used as Island Domain IO. >>>>>> >>>>>> The support exists on most recent qcom SoCs, and we add support >>>>>> for sm8150/sm8250/sm8350 and sc7280 as part of this patch. >>>>>> >>>>> >>>>> I was under the impression that this feature would allow you to >>>>> repurpose pins for use either by the remote island or by apps. >>>> >>>> thats right, you can repurpose the pins for usage by apps by setting >>>> the egpio_enable to 1, when set to 0 its owned by the island processor. >>> >>> Good. >>> >>>>> >>>>> But if I understand your proposal, you check to see if the pin is >>>>> "egpio capable" for a pin and if so just sets the bit - muxing it to >>>>> apps (or the island?). >>>> >>>> Right, so if there is a request for a egpio-capable pin, the driver >>>> flips the ownership. Are you suggesting having some kind of checks to determine >>>> who should own it? >>>> >>> >>> I see, I missed that nuance. So Linux will steal any pins that are >>> mentioned in DT. But that would mean that you're relying on someone else >>> to ensure that this bit is cleared for the other pins and you would not >>> be able to explicitly flip the state back to island mode in runtime. >>> >>> I would prefer that this was more explicit. >>> >>>>> It seems reasonable that this would be another pinmux state for these >>>>> pins, rather than just flipping them all in one or the other direction. >>>> >>>> hmm, I don't understand. This is not a pinmux state, its a switch to decide >>>> the ownership. >>> >>> But does it mux the pin to the island, or does it state that the island >>> is now in charge of the associated TLMM registers? >> >> The island processor does not access the APPS TLMM register space, it has its >> own TLMM register space that it configures. APPS TLMM registers control its >> mux/conf settings and Island TLMM registers controls its mux/conf. So essentially >> there are 2 sets of registers to control the same pin. >> This bit is more like a top level mux which decides what register settings >> take affect. >> > > "One mux to rule them all" :) > > When we switch this mux towards the Island TLMM, do we need to configure > the APPS TLMM in a particular way, or does the state of that not matter? No APPS TLMM settings should be needed, the state of that does not matter. > Would it be reasonable to say that when muxed towards the island the > apps should always be in gpio mux with some predetermined properties, to > save power? No, the the register settings in APPS TLMM are nop/dont care when egpio_enable is 0. > To reiterate, as proposed, mentioning a egpio-capable pin in the apps > DTS will cause it to be muxed to the APSS TLMM. But I'm not convinced > that we don't have scenarios where one might want to dynamically mux the > pin between island and apss tlmm. > > My suggestion is that even that it's two independent muxes controlled in > the apps tlmm, we'd express them in the same pinmux, i.e. we'd have > something like: > > some-local-state { > pins = "gpio1"; > function = "gpio"; > output-high; > }; so this would set the function to gpio in the APPS TLMM and set the egpio_enable = 1? which was also what the original $SUBJECT patch did. > some-remote-state { > pins = "gpio1"; > function = "island"; /* or just egpio... ? */ > }; Here we add a new function to the pin and that's used to set the egpio_enable to 0? > One case I imaging where this could be useful is to allow Linux to > configure a known state of pins when the island isn't running, from the > remoteproc driver and then flip it over to island mode before booting > the remote. So we save power during boot up until the island processor comes up? So fwik when we boot linux its either configured to boot the island processor or not. Are you talking about some scenario where the island processor comes up on demand and goes down when not used? -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pinctrl: qcom: Add egpio feature support 2021-10-01 5:34 ` Rajendra Nayak @ 2021-10-04 20:43 ` Bjorn Andersson 0 siblings, 0 replies; 10+ messages in thread From: Bjorn Andersson @ 2021-10-04 20:43 UTC (permalink / raw) To: Rajendra Nayak Cc: agross, linus.walleij, linux-arm-msm, linux-gpio, linux-kernel, Prasad Sodagudi On Thu 30 Sep 22:34 PDT 2021, Rajendra Nayak wrote: > > On 9/30/2021 8:25 PM, Bjorn Andersson wrote: > > On Thu 30 Sep 02:46 PDT 2021, Rajendra Nayak wrote: > > > > > > > > > > > On 9/21/2021 9:56 PM, Bjorn Andersson wrote: > > > > On Tue 21 Sep 03:39 PDT 2021, Rajendra Nayak wrote: > > > > > > > > > > > > > > > > > > > On 9/20/2021 6:14 AM, Bjorn Andersson wrote: > > > > > > On Fri 17 Sep 01:37 CDT 2021, Rajendra Nayak wrote: > > > > > > > > > > > > > From: Prasad Sodagudi <psodagud@codeaurora.org> > > > > > > > > > > > > > > egpio is a scheme which allows special power Island Domain IOs > > > > > > > (LPASS,SSC) to be reused as regular chip GPIOs by muxing regular > > > > > > > TLMM functions with Island Domain functions. > > > > > > > With this scheme, an IO can be controlled both by the cpu running > > > > > > > linux and the Island processor. This provides great flexibility to > > > > > > > re-purpose the Island IOs for regular TLMM usecases. > > > > > > > > > > > > > > 2 new bits are added to ctl_reg, egpio_present is a read only bit > > > > > > > which shows if egpio feature is available or not on a given gpio. > > > > > > > egpio_enable is the read/write bit and only effective if egpio_present > > > > > > > is 1. Once its set, the Island IO is controlled from Chip TLMM. > > > > > > > egpio_enable when set to 0 means the GPIO is used as Island Domain IO. > > > > > > > > > > > > > > The support exists on most recent qcom SoCs, and we add support > > > > > > > for sm8150/sm8250/sm8350 and sc7280 as part of this patch. > > > > > > > > > > > > > > > > > > > I was under the impression that this feature would allow you to > > > > > > repurpose pins for use either by the remote island or by apps. > > > > > > > > > > thats right, you can repurpose the pins for usage by apps by setting > > > > > the egpio_enable to 1, when set to 0 its owned by the island processor. > > > > > > > > Good. > > > > > > > > > > > > > > > > But if I understand your proposal, you check to see if the pin is > > > > > > "egpio capable" for a pin and if so just sets the bit - muxing it to > > > > > > apps (or the island?). > > > > > > > > > > Right, so if there is a request for a egpio-capable pin, the driver > > > > > flips the ownership. Are you suggesting having some kind of checks to determine > > > > > who should own it? > > > > > > > > > > > > > I see, I missed that nuance. So Linux will steal any pins that are > > > > mentioned in DT. But that would mean that you're relying on someone else > > > > to ensure that this bit is cleared for the other pins and you would not > > > > be able to explicitly flip the state back to island mode in runtime. > > > > > > > > I would prefer that this was more explicit. > > > > > > > > > > It seems reasonable that this would be another pinmux state for these > > > > > > pins, rather than just flipping them all in one or the other direction. > > > > > > > > > > hmm, I don't understand. This is not a pinmux state, its a switch to decide > > > > > the ownership. > > > > > > > > But does it mux the pin to the island, or does it state that the island > > > > is now in charge of the associated TLMM registers? > > > > > > The island processor does not access the APPS TLMM register space, it has its > > > own TLMM register space that it configures. APPS TLMM registers control its > > > mux/conf settings and Island TLMM registers controls its mux/conf. So essentially > > > there are 2 sets of registers to control the same pin. > > > This bit is more like a top level mux which decides what register settings > > > take affect. > > > > > > > "One mux to rule them all" :) > > > > When we switch this mux towards the Island TLMM, do we need to configure > > the APPS TLMM in a particular way, or does the state of that not matter? > > No APPS TLMM settings should be needed, the state of that does not matter. > > > Would it be reasonable to say that when muxed towards the island the > > apps should always be in gpio mux with some predetermined properties, to > > save power? > > No, the the register settings in APPS TLMM are nop/dont care when egpio_enable is 0. > That's good. > > To reiterate, as proposed, mentioning a egpio-capable pin in the apps > > DTS will cause it to be muxed to the APSS TLMM. But I'm not convinced > > that we don't have scenarios where one might want to dynamically mux the > > pin between island and apss tlmm. > > > > My suggestion is that even that it's two independent muxes controlled in > > the apps tlmm, we'd express them in the same pinmux, i.e. we'd have > > something like: > > > > some-local-state { > > pins = "gpio1"; > > function = "gpio"; > > output-high; > > }; > > so this would set the function to gpio in the APPS TLMM and set the egpio_enable = 1? > which was also what the original $SUBJECT patch did. > Right, the effect for any of the today defined functions would be the same as the patch you proposed. > > some-remote-state { > > pins = "gpio1"; > > function = "island"; /* or just egpio... ? */ > > }; > > Here we add a new function to the pin and that's used to set the egpio_enable to 0? > Yes. > > One case I imaging where this could be useful is to allow Linux to > > configure a known state of pins when the island isn't running, from the > > remoteproc driver and then flip it over to island mode before booting > > the remote. > > So we save power during boot up until the island processor comes up? > So fwik when we boot linux its either configured to boot the island processor > or not. Are you talking about some scenario where the island processor comes > up on demand and goes down when not used? > Perhaps I'm just paranoid, but I do like the idea of being able to explicitly describe the "island configuration" in DT, rather than relying on an implicit "if I don't define the node it's probably in island mode". I was in particular thinking about the case where the remote stops, but I guess that should only happen very shortly before the remote is restarted again. The other thing that your proposed patch rely on is that all egpio-capable pins are left with egpio_enable = 0 by the bootloader, because the only way to flip a egpio_enable = 1 to a 0 is to recompile and re-sign the bootloader. Regards, Bjorn ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-10-04 20:41 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-17 6:37 [PATCH] pinctrl: qcom: Add egpio feature support Rajendra Nayak 2021-09-17 16:00 ` Doug Anderson 2021-09-20 0:44 ` Bjorn Andersson 2021-09-21 10:39 ` Rajendra Nayak 2021-09-21 15:56 ` Linus Walleij 2021-09-21 16:26 ` Bjorn Andersson 2021-09-30 9:46 ` Rajendra Nayak 2021-09-30 14:55 ` Bjorn Andersson 2021-10-01 5:34 ` Rajendra Nayak 2021-10-04 20:43 ` Bjorn Andersson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).