From: Rajendra Nayak <rnayak@codeaurora.org>
To: Doug Anderson <dianders@chromium.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>,
Andy Gross <agross@kernel.org>, LinusW <linus.walleij@linaro.org>,
linux-arm-msm <linux-arm-msm@vger.kernel.org>,
"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
LKML <linux-kernel@vger.kernel.org>,
Prasad Sodagudi <psodagud@codeaurora.org>
Subject: Re: [PATCH v2 1/2] pinctrl: qcom: Add egpio feature support
Date: Tue, 2 Nov 2021 16:32:02 +0530 [thread overview]
Message-ID: <27bf8521-f4d2-13b1-8e14-ceacbbeabb1b@codeaurora.org> (raw)
In-Reply-To: <CAD=FV=UoTFzZn5h_VNrwrt2E5P2k9WmqZ7nXFLDHyMzUvSzhUA@mail.gmail.com>
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
prev parent reply other threads:[~2021-11-02 11:02 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-26 12:07 [PATCH v2 1/2] pinctrl: qcom: Add egpio feature support Rajendra Nayak
2021-10-26 12:07 ` [PATCH v2 2/2] pinctrl: qcom: sc7280: Add egpio support Rajendra Nayak
2021-10-28 23:03 ` Bjorn Andersson
2021-10-28 23:01 ` [PATCH v2 1/2] pinctrl: qcom: Add egpio feature support Bjorn Andersson
2021-10-29 9:58 ` Rajendra Nayak
2021-10-29 6:54 ` Stephen Boyd
2021-10-29 10:19 ` Rajendra Nayak
2021-10-29 19:05 ` Stephen Boyd
2021-11-02 10:38 ` Rajendra Nayak
2021-11-01 21:04 ` Doug Anderson
2021-11-02 11:02 ` Rajendra Nayak [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=27bf8521-f4d2-13b1-8e14-ceacbbeabb1b@codeaurora.org \
--to=rnayak@codeaurora.org \
--cc=agross@kernel.org \
--cc=bjorn.andersson@linaro.org \
--cc=dianders@chromium.org \
--cc=linus.walleij@linaro.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=psodagud@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).