| Message ID | 1635250056-20274-1-git-send-email-rnayak@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/2] pinctrl: qcom: Add egpio feature support | expand |
On Tue 26 Oct 05:07 PDT 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. > > 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; You're returning here with pctrl->lock held and irqs disabled. > + } else { > + val &= ~mask; > + val |= i << g->mux_bit; > + /* Check if egpio present and enable that feature */ I never remember if egpio_enable means apss or lpass, so I think this comment would be better as: /* Claim ownership of pin if egpio capable */ > + if (egpio_func && (val & BIT(g->egpio_present))) Can't you drop the parenthesis around the second expression? Regards, Bjorn > + 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 > */ > struct msm_pinctrl_soc_data { > const struct pinctrl_pin_desc *pins; > @@ -136,6 +139,7 @@ struct msm_pinctrl_soc_data { > unsigned int nwakeirq_map; > bool wakeirq_dual_edge_errata; > unsigned int gpio_func; > + unsigned int egpio_func; > }; > > extern const struct dev_pm_ops msm_pinctrl_dev_pm_ops; > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation >
Quoting Rajendra Nayak (2021-10-26 05:07:35) > 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> > --- Does this supersede adding support for lpass pinctrl in this series[1]? [1] https://lore.kernel.org/r/1635342097-2726-1-git-send-email-srivasam@codeaurora.org
On 10/29/2021 4:31 AM, Bjorn Andersson wrote: > On Tue 26 Oct 05:07 PDT 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. >> >> 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; > > You're returning here with pctrl->lock held and irqs disabled. argh, right. I will fix that and repost. I wonder if I should just drop that error handling completely, we wouldn't end up here unless the platform driver wrongly populates a pin which does not support egpio with a egpio function. > >> + } else { >> + val &= ~mask; >> + val |= i << g->mux_bit; >> + /* Check if egpio present and enable that feature */ > > I never remember if egpio_enable means apss or lpass, so I think this > comment would be better as: > > /* Claim ownership of pin if egpio capable */ :) makes sense > >> + if (egpio_func && (val & BIT(g->egpio_present))) > > Can't you drop the parenthesis around the second expression? yes, will do, thanks for the review.
On 10/29/2021 12:24 PM, Stephen Boyd wrote: > Quoting Rajendra Nayak (2021-10-26 05:07:35) >> 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> >> --- > > Does this supersede adding support for lpass pinctrl in this series[1]? No, the driver in [1] actually manages the LPASS TLMM instance, while this patch makes it possible for the 'same' pins to be managed by the SoC TLMM instance. On sc7280 SoC for instance GPIO144-158 maps to LPI-GPIO-0-14, and GPIO159-174 maps to SSC-GPIO-0-15. > [1] https://lore.kernel.org/r/1635342097-2726-1-git-send-email-srivasam@codeaurora.org >
Quoting Rajendra Nayak (2021-10-29 03:19:04) > > > On 10/29/2021 12:24 PM, Stephen Boyd wrote: > > Quoting Rajendra Nayak (2021-10-26 05:07:35) > >> 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> > >> --- > > > > Does this supersede adding support for lpass pinctrl in this series[1]? > > No, the driver in [1] actually manages the LPASS TLMM instance, while this patch > makes it possible for the 'same' pins to be managed by the SoC TLMM instance. > On sc7280 SoC for instance GPIO144-158 maps to LPI-GPIO-0-14, and GPIO159-174 > maps to SSC-GPIO-0-15. How do we make sure that the LPASS pins are actually muxed out of the SoC and not blocked by eGPIO in this driver muxing out the pin as a gpio? Do they avoid conflicting with each other somehow?
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. 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? ...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
On 10/30/2021 12:35 AM, Stephen Boyd wrote: > Quoting Rajendra Nayak (2021-10-29 03:19:04) >> >> >> On 10/29/2021 12:24 PM, Stephen Boyd wrote: >>> Quoting Rajendra Nayak (2021-10-26 05:07:35) >>>> 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> >>>> --- >>> >>> Does this supersede adding support for lpass pinctrl in this series[1]? >> >> No, the driver in [1] actually manages the LPASS TLMM instance, while this patch >> makes it possible for the 'same' pins to be managed by the SoC TLMM instance. >> On sc7280 SoC for instance GPIO144-158 maps to LPI-GPIO-0-14, and GPIO159-174 >> maps to SSC-GPIO-0-15. > > How do we make sure that the LPASS pins are actually muxed out of the > SoC and not blocked by eGPIO in this driver muxing out the pin as a > gpio? Do they avoid conflicting with each other somehow? No, currently they don't. The default value of egpio_enable is 0, so if SoC TLMM grabs it first and sets it to 1 and then the LPASS TLMM tries to grab it, perhaps it can find out and throw and error, though I don;t think it does that today. I am not sure how this can be caught when its the other way round (LPASS grabs it first)
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 >
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 */ struct msm_pinctrl_soc_data { const struct pinctrl_pin_desc *pins; @@ -136,6 +139,7 @@ struct msm_pinctrl_soc_data { unsigned int nwakeirq_map; bool wakeirq_dual_edge_errata; unsigned int gpio_func; + unsigned int egpio_func; }; extern const struct dev_pm_ops msm_pinctrl_dev_pm_ops;