| Message ID | 20190726200830.52728-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] pinctrl: intel: Allow to request locked pins | expand |
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
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);
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 :)
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
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) */
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(-)