Re: [PATCH v2 1/2] pinctrl: qcom: Add egpio feature support
From: Rajendra Nayak
Date: Tue Nov 02 2021 - 07:02:39 EST
On 11/2/2021 2:34 AM, Doug Anderson wrote:
Hi,
On Tue, Oct 26, 2021 at 5:09 AM Rajendra Nayak <rnayak@xxxxxxxxxxxxxx> wrote:
From: Prasad Sodagudi <psodagud@xxxxxxxxxxxxxx>
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.
To support this we add a new function 'egpio' which can be used to
set the egpio_enable to 0, for any other TLMM controlled functions
we set the egpio_enable to 1.
Signed-off-by: Prasad Sodagudi <psodagud@xxxxxxxxxxxxxx>
Signed-off-by: Rajendra Nayak <rnayak@xxxxxxxxxxxxxx>
---
drivers/pinctrl/qcom/pinctrl-msm.c | 17 +++++++++++++++--
drivers/pinctrl/qcom/pinctrl-msm.h | 4 ++++
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 8476a8a..bfdba3a 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -185,6 +185,7 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
unsigned int irq = irq_find_mapping(gc->irq.domain, group);
struct irq_data *d = irq_get_irq_data(irq);
unsigned int gpio_func = pctrl->soc->gpio_func;
+ unsigned int egpio_func = pctrl->soc->egpio_func;
const struct msm_pingroup *g;
unsigned long flags;
u32 val, mask;
@@ -218,8 +219,20 @@ static int msm_pinmux_set_mux(struct pinctrl_dev *pctldev,
raw_spin_lock_irqsave(&pctrl->lock, flags);
val = msm_readl_ctl(pctrl, g);
- val &= ~mask;
- val |= i << g->mux_bit;
+
+ if (egpio_func && i == egpio_func) {
+ if (val & BIT(g->egpio_present))
+ val &= ~BIT(g->egpio_enable);
+ else
+ return -EINVAL;
+ } else {
+ val &= ~mask;
+ val |= i << g->mux_bit;
+ /* Check if egpio present and enable that feature */
+ if (egpio_func && (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..b7110ac 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;
@@ -119,6 +121,7 @@ struct msm_gpio_wakeirq_map {
* to be aware that their parent can't handle dual
* edge interrupts.
* @gpio_func: Which function number is GPIO (usually 0).
+ * @egpio_func: Which function number is eGPIO
nit: in the above, document that this is actually a _virtual_ number.
In other words it doesn't actually map to any real hardware register
setting. Also maybe document 0 here means that eGPIO isn't supported
on this SoC. ...and lastly, all the other entries in this docstring
end with a ".". Something roughly like this:
* @egpio_func: If non-zero then this SoC supports eGPIO. Even though in
hardware this is a mux 1-level above the TLMM, we'll treat
it as if this is just another mux state of the TLMM. Since
it doesn't really map to hardware, we'll allocate a virtual
function number for eGPIO and any time we see that function
number used we'll treat it as a request to mux away from
our TLMM towards another owner.
Thanks Doug, this sounds perfect, I'll copy-paste it :)
Thinking about this made me look a little closer at your virtual
function number, though. On sc7280 (in the next patch) you chose
function "9" as GPIO. Things smell a little strange, though.
Apparently sc7280 was already setup for a non-virtual "function 9"
since "nfuncs" was 10. Was this just a fortunate bug that kept you
from having to touch all the sc7280 PINGROUP definitions in the next
patch, or is there actually a true "function 9" somewhere in the
hardware that we might want to someday add to Linux? If so, should we
pick eGPIO as 10?
Right, I did start off thinking I would need to add a new function, and
deal with changing all the PINGROUP definitions, and worry about all the
stuff that you mentioned below, but then I realized function 9 was actually
unused on all sc7280 pins (and I did look at the couple other SoCs usptream
which support egpio which also had the same pattern) so decided to just reuse
function 9 and avoid dealing with all the mess that would result
with adding a new virtual function ¯\_(ツ)_/¯
...and then, looking further, what would happen if we picked eGPIO 10?
Should "nfuncs" include this virtual number, or not? If "nfuncs"
_does_ include this number and it bumps you over to the next
"order_base_2" then the mask calculated by msm_pinmux_set_mux() will
be wrong. If "nfuncs" _doesn't_ include this number then we should
probably document that fact, and (I suppose) change sc7280's "nfuncs"
down to 9?
-Doug
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation