Date:   Tue, 2 Nov 2021 16:32:02 +0530
From:   Rajendra Nayak <rnayak@...eaurora.org>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Bjorn Andersson <bjorn.andersson@...aro.org>,
        Andy Gross <agross@...nel.org>,
        LinusW <linus.walleij@...aro.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Prasad Sodagudi <psodagud@...eaurora.org>
Subject: Re: [PATCH v2 1/2] pinctrl: qcom: Add egpio feature support



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

Powered by blists - more mailing lists