lkml.org 
[lkml]   [2021]   [Nov]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 1/2] pinctrl: qcom: Add egpio feature support
From
Date


On 11/2/2021 2:34 AM, Doug Anderson wrote:
> Hi,
>
> On Tue, Oct 26, 2021 at 5:09 AM 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.
>>
>> 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@codeaurora.org>
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> ---
>> 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

\
 
 \ /
  Last update: 2021-11-02 12:03    [W:0.089 / U:0.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site