* [PATCH v1] pinctrl: intel: Allow to request locked pins
@ 2019-07-26 20:08 Andy Shevchenko
2019-08-06 11:01 ` Mika Westerberg
2019-08-06 14:27 ` Linus Walleij
0 siblings, 2 replies; 5+ messages in thread
From: Andy Shevchenko @ 2019-07-26 20:08 UTC (permalink / raw)
To: Mika Westerberg, linux-gpio, Linus Walleij; +Cc: Andy Shevchenko
Some firmwares would like to protect pins from being modified by OS
and at the same time provide them to OS as a resource. So, the driver
in such circumstances may request pin and may not change its state.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/pinctrl/intel/pinctrl-intel.c | 55 +++++++++++++++++++--------
1 file changed, 39 insertions(+), 16 deletions(-)
diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 3a945997b8eb..567fe43b238f 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -220,22 +220,30 @@ static bool intel_pad_acpi_mode(struct intel_pinctrl *pctrl, unsigned int pin)
return !(readl(hostown) & BIT(gpp_offset));
}
-static bool intel_pad_locked(struct intel_pinctrl *pctrl, unsigned int pin)
+enum {
+ PAD_UNLOCKED = 0,
+ PAD_LOCKED = 1,
+ PAD_LOCKED_TX = 2,
+ PAD_LOCKED_FULL = PAD_LOCKED | PAD_LOCKED_TX,
+};
+
+static int intel_pad_locked(struct intel_pinctrl *pctrl, unsigned int pin)
{
struct intel_community *community;
const struct intel_padgroup *padgrp;
unsigned int offset, gpp_offset;
u32 value;
+ int ret = PAD_UNLOCKED;
community = intel_get_community(pctrl, pin);
if (!community)
- return true;
+ return PAD_LOCKED_FULL;
if (!community->padcfglock_offset)
- return false;
+ return PAD_UNLOCKED;
padgrp = intel_community_get_padgroup(community, pin);
if (!padgrp)
- return true;
+ return PAD_LOCKED_FULL;
gpp_offset = padgroup_offset(padgrp, pin);
@@ -244,23 +252,27 @@ static bool intel_pad_locked(struct intel_pinctrl *pctrl, unsigned int pin)
* the pad is considered unlocked. Any other case means that it is
* either fully or partially locked and we don't touch it.
*/
- offset = community->padcfglock_offset + padgrp->reg_num * 8;
+ offset = community->padcfglock_offset + 0 + padgrp->reg_num * 8;
value = readl(community->regs + offset);
if (value & BIT(gpp_offset))
- return true;
+ ret |= PAD_LOCKED;
offset = community->padcfglock_offset + 4 + padgrp->reg_num * 8;
value = readl(community->regs + offset);
if (value & BIT(gpp_offset))
- return true;
+ ret |= PAD_LOCKED_TX;
- return false;
+ return ret;
+}
+
+static bool intel_pad_is_unlocked(struct intel_pinctrl *pctrl, unsigned int pin)
+{
+ return (intel_pad_locked(pctrl, pin) & PAD_LOCKED) == PAD_UNLOCKED;
}
static bool intel_pad_usable(struct intel_pinctrl *pctrl, unsigned int pin)
{
- return intel_pad_owned_by_host(pctrl, pin) &&
- !intel_pad_locked(pctrl, pin);
+ return intel_pad_owned_by_host(pctrl, pin) && intel_pad_is_unlocked(pctrl, pin);
}
static int intel_get_groups_count(struct pinctrl_dev *pctldev)
@@ -294,7 +306,8 @@ static void intel_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
void __iomem *padcfg;
u32 cfg0, cfg1, mode;
- bool locked, acpi;
+ int locked;
+ bool acpi;
if (!intel_pad_owned_by_host(pctrl, pin)) {
seq_puts(s, "not available");
@@ -322,11 +335,16 @@ static void intel_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
if (locked || acpi) {
seq_puts(s, " [");
- if (locked) {
+ if (locked)
seq_puts(s, "LOCKED");
- if (acpi)
- seq_puts(s, ", ");
- }
+ if ((locked & PAD_LOCKED_FULL) == PAD_LOCKED_TX)
+ seq_puts(s, " TX");
+ else if ((locked & PAD_LOCKED_FULL) == PAD_LOCKED_FULL)
+ seq_puts(s, " FULL");
+
+ if (locked && acpi)
+ seq_puts(s, ", ");
+
if (acpi)
seq_puts(s, "ACPI");
seq_puts(s, "]");
@@ -448,11 +466,16 @@ static int intel_gpio_request_enable(struct pinctrl_dev *pctldev,
raw_spin_lock_irqsave(&pctrl->lock, flags);
- if (!intel_pad_usable(pctrl, pin)) {
+ if (!intel_pad_owned_by_host(pctrl, pin)) {
raw_spin_unlock_irqrestore(&pctrl->lock, flags);
return -EBUSY;
}
+ if (!intel_pad_is_unlocked(pctrl, pin)) {
+ raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+ return 0;
+ }
+
padcfg0 = intel_get_padcfg(pctrl, pin, PADCFG0);
intel_gpio_set_gpio_mode(padcfg0);
/* Disable TX buffer and enable RX (this will be input) */
--
2.20.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v1] pinctrl: intel: Allow to request locked pins
2019-07-26 20:08 [PATCH v1] pinctrl: intel: Allow to request locked pins Andy Shevchenko
@ 2019-08-06 11:01 ` Mika Westerberg
2019-08-06 11:45 ` Andy Shevchenko
2019-08-06 14:27 ` Linus Walleij
1 sibling, 1 reply; 5+ messages in thread
From: Mika Westerberg @ 2019-08-06 11:01 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: linux-gpio, Linus Walleij
On Fri, Jul 26, 2019 at 11:08:30PM +0300, Andy Shevchenko wrote:
> Some firmwares would like to protect pins from being modified by OS
> and at the same time provide them to OS as a resource. So, the driver
> in such circumstances may request pin and may not change its state.
This is definitely good idea.
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> drivers/pinctrl/intel/pinctrl-intel.c | 55 +++++++++++++++++++--------
> 1 file changed, 39 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index 3a945997b8eb..567fe43b238f 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -220,22 +220,30 @@ static bool intel_pad_acpi_mode(struct intel_pinctrl *pctrl, unsigned int pin)
> return !(readl(hostown) & BIT(gpp_offset));
> }
>
> -static bool intel_pad_locked(struct intel_pinctrl *pctrl, unsigned int pin)
> +enum {
> + PAD_UNLOCKED = 0,
> + PAD_LOCKED = 1,
> + PAD_LOCKED_TX = 2,
> + PAD_LOCKED_FULL = PAD_LOCKED | PAD_LOCKED_TX,
> +};
> +
> +static int intel_pad_locked(struct intel_pinctrl *pctrl, unsigned int pin)
> {
> struct intel_community *community;
> const struct intel_padgroup *padgrp;
> unsigned int offset, gpp_offset;
> u32 value;
> + int ret = PAD_UNLOCKED;
>
> community = intel_get_community(pctrl, pin);
> if (!community)
> - return true;
> + return PAD_LOCKED_FULL;
> if (!community->padcfglock_offset)
> - return false;
> + return PAD_UNLOCKED;
>
> padgrp = intel_community_get_padgroup(community, pin);
> if (!padgrp)
> - return true;
> + return PAD_LOCKED_FULL;
>
> gpp_offset = padgroup_offset(padgrp, pin);
>
> @@ -244,23 +252,27 @@ static bool intel_pad_locked(struct intel_pinctrl *pctrl, unsigned int pin)
> * the pad is considered unlocked. Any other case means that it is
> * either fully or partially locked and we don't touch it.
I think you should update the above comment as well.
> */
> - offset = community->padcfglock_offset + padgrp->reg_num * 8;
> + offset = community->padcfglock_offset + 0 + padgrp->reg_num * 8;
> value = readl(community->regs + offset);
> if (value & BIT(gpp_offset))
> - return true;
> + ret |= PAD_LOCKED;
>
> offset = community->padcfglock_offset + 4 + padgrp->reg_num * 8;
> value = readl(community->regs + offset);
> if (value & BIT(gpp_offset))
> - return true;
> + ret |= PAD_LOCKED_TX;
>
> - return false;
> + return ret;
> +}
> +
> +static bool intel_pad_is_unlocked(struct intel_pinctrl *pctrl, unsigned int pin)
> +{
> + return (intel_pad_locked(pctrl, pin) & PAD_LOCKED) == PAD_UNLOCKED;
> }
>
> static bool intel_pad_usable(struct intel_pinctrl *pctrl, unsigned int pin)
> {
> - return intel_pad_owned_by_host(pctrl, pin) &&
> - !intel_pad_locked(pctrl, pin);
> + return intel_pad_owned_by_host(pctrl, pin) && intel_pad_is_unlocked(pctrl, pin);
> }
>
> static int intel_get_groups_count(struct pinctrl_dev *pctldev)
> @@ -294,7 +306,8 @@ static void intel_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
> struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> void __iomem *padcfg;
> u32 cfg0, cfg1, mode;
> - bool locked, acpi;
> + int locked;
> + bool acpi;
>
> if (!intel_pad_owned_by_host(pctrl, pin)) {
> seq_puts(s, "not available");
> @@ -322,11 +335,16 @@ static void intel_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
>
> if (locked || acpi) {
> seq_puts(s, " [");
> - if (locked) {
> + if (locked)
> seq_puts(s, "LOCKED");
> - if (acpi)
> - seq_puts(s, ", ");
> - }
> + if ((locked & PAD_LOCKED_FULL) == PAD_LOCKED_TX)
> + seq_puts(s, " TX");
> + else if ((locked & PAD_LOCKED_FULL) == PAD_LOCKED_FULL)
> + seq_puts(s, " FULL");
> +
> + if (locked && acpi)
> + seq_puts(s, ", ");
> +
> if (acpi)
> seq_puts(s, "ACPI");
> seq_puts(s, "]");
> @@ -448,11 +466,16 @@ static int intel_gpio_request_enable(struct pinctrl_dev *pctldev,
>
> raw_spin_lock_irqsave(&pctrl->lock, flags);
>
> - if (!intel_pad_usable(pctrl, pin)) {
> + if (!intel_pad_owned_by_host(pctrl, pin)) {
> raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> return -EBUSY;
> }
>
> + if (!intel_pad_is_unlocked(pctrl, pin)) {
> + raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> + return 0;
Hmm, if I'm reading this right it still does not allow requesting locked
pins. What I'm missing here?
> + }
> +
> padcfg0 = intel_get_padcfg(pctrl, pin, PADCFG0);
> intel_gpio_set_gpio_mode(padcfg0);
> /* Disable TX buffer and enable RX (this will be input) */
> --
> 2.20.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1] pinctrl: intel: Allow to request locked pins
2019-08-06 11:01 ` Mika Westerberg
@ 2019-08-06 11:45 ` Andy Shevchenko
2019-08-06 11:47 ` Mika Westerberg
0 siblings, 1 reply; 5+ messages in thread
From: Andy Shevchenko @ 2019-08-06 11:45 UTC (permalink / raw)
To: Mika Westerberg; +Cc: linux-gpio, Linus Walleij
On Tue, Aug 06, 2019 at 02:01:26PM +0300, Mika Westerberg wrote:
> On Fri, Jul 26, 2019 at 11:08:30PM +0300, Andy Shevchenko wrote:
> > Some firmwares would like to protect pins from being modified by OS
> > and at the same time provide them to OS as a resource. So, the driver
> > in such circumstances may request pin and may not change its state.
>
> This is definitely good idea.
Thanks for review, my answers below.
> > * the pad is considered unlocked. Any other case means that it is
> > * either fully or partially locked and we don't touch it.
>
> I think you should update the above comment as well.
Will do for v2.
> > raw_spin_lock_irqsave(&pctrl->lock, flags);
> >
> > - if (!intel_pad_usable(pctrl, pin)) {
> > + if (!intel_pad_owned_by_host(pctrl, pin)) {
> > raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> > return -EBUSY;
> > }
> >
> > + if (!intel_pad_is_unlocked(pctrl, pin)) {
> > + raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> > + return 0;
>
> Hmm, if I'm reading this right it still does not allow requesting locked
> pins. What I'm missing here?
We do not return an error code here, so pin is left requested but pad
configuration register untouched.
> > + }
> > +
> > padcfg0 = intel_get_padcfg(pctrl, pin, PADCFG0);
> > intel_gpio_set_gpio_mode(padcfg0);
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1] pinctrl: intel: Allow to request locked pins
2019-08-06 11:45 ` Andy Shevchenko
@ 2019-08-06 11:47 ` Mika Westerberg
0 siblings, 0 replies; 5+ messages in thread
From: Mika Westerberg @ 2019-08-06 11:47 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: linux-gpio, Linus Walleij
On Tue, Aug 06, 2019 at 02:45:17PM +0300, Andy Shevchenko wrote:
> On Tue, Aug 06, 2019 at 02:01:26PM +0300, Mika Westerberg wrote:
> > On Fri, Jul 26, 2019 at 11:08:30PM +0300, Andy Shevchenko wrote:
> > > Some firmwares would like to protect pins from being modified by OS
> > > and at the same time provide them to OS as a resource. So, the driver
> > > in such circumstances may request pin and may not change its state.
> >
> > This is definitely good idea.
>
> Thanks for review, my answers below.
>
> > > * the pad is considered unlocked. Any other case means that it is
> > > * either fully or partially locked and we don't touch it.
> >
> > I think you should update the above comment as well.
>
> Will do for v2.
>
> > > raw_spin_lock_irqsave(&pctrl->lock, flags);
> > >
> > > - if (!intel_pad_usable(pctrl, pin)) {
> > > + if (!intel_pad_owned_by_host(pctrl, pin)) {
> > > raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> > > return -EBUSY;
> > > }
> > >
> > > + if (!intel_pad_is_unlocked(pctrl, pin)) {
> > > + raw_spin_unlock_irqrestore(&pctrl->lock, flags);
> > > + return 0;
> >
> > Hmm, if I'm reading this right it still does not allow requesting locked
> > pins. What I'm missing here?
>
> We do not return an error code here, so pin is left requested but pad
> configuration register untouched.
Indeed, now it is clear. Thanks for explaining :)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1] pinctrl: intel: Allow to request locked pins
2019-07-26 20:08 [PATCH v1] pinctrl: intel: Allow to request locked pins Andy Shevchenko
2019-08-06 11:01 ` Mika Westerberg
@ 2019-08-06 14:27 ` Linus Walleij
1 sibling, 0 replies; 5+ messages in thread
From: Linus Walleij @ 2019-08-06 14:27 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Mika Westerberg, open list:GPIO SUBSYSTEM
Hi Andy,
On Fri, Jul 26, 2019 at 10:08 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Some firmwares would like to protect pins from being modified by OS
> and at the same time provide them to OS as a resource. So, the driver
> in such circumstances may request pin and may not change its state.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Interesting patch!
> +enum {
> + PAD_UNLOCKED = 0,
> + PAD_LOCKED = 1,
> + PAD_LOCKED_TX = 2,
> + PAD_LOCKED_FULL = PAD_LOCKED | PAD_LOCKED_TX,
> +};
Please add some kerneldoc explaining what these different lock
states are. My head is spinning. Locked? Locked TX? Locked full?
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-08-06 14:27 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-26 20:08 [PATCH v1] pinctrl: intel: Allow to request locked pins Andy Shevchenko
2019-08-06 11:01 ` Mika Westerberg
2019-08-06 11:45 ` Andy Shevchenko
2019-08-06 11:47 ` Mika Westerberg
2019-08-06 14:27 ` Linus Walleij
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).