lkml.org 
[lkml]   [2019]   [Apr]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] pinctrl: intel: save HOSTSW_OWN register over suspend/resume
    On Tue, Apr 02, 2019 at 02:16:19PM +0800, Chris Chiu wrote:
    > On Mon, Apr 1, 2019 at 8:23 PM Andy Shevchenko
    > <andriy.shevchenko@intel.com> wrote:
    > > On Mon, Apr 01, 2019 at 06:41:57PM +0800, Chris Chiu wrote:

    > Thanks for the comment. My first version did mimic the logic of the interrupt
    > mask restore but it was based on the DMI quirk. It saves HOSTSW_OWN
    > for each padgroup and restores them all after resume if DMI info matched.
    >
    > What really confused me is how to do this specifically for a requested GPIO
    > pin. So here's my new proposed patch. Please suggests if there's any better
    > idea. Thanks.

    > struct intel_community_context {
    > u32 *intmask;
    > + u32 *hostown;

    This is okay.

    > };

    > +#ifdef CONFIG_PM_SLEEP
    > +static void intel_save_hostown(struct intel_pinctrl *pctrl, unsigned int pin);
    > +#endif
    > +

    No need for this...

    > /* Disable TX buffer and enable RX (this will be input) */
    > __intel_gpio_set_direction(padcfg0, true);
    > +#ifdef CONFIG_PM_SLEEP
    > + intel_save_hostown(pctrl, pin);
    > +#endif

    ...and for this.
    Just save all of them at ->suspend()

    > for (i = 0; i < pctrl->ncommunities; i++) {
    > struct intel_community *community = &pctrl->communities[i];
    > - u32 *intmask;
    > + u32 *intmask, *hostown;
    >
    > intmask = devm_kcalloc(pctrl->dev, community->ngpps,
    > sizeof(*intmask), GFP_KERNEL);
    > @@ -1292,6 +1299,13 @@ static int intel_pinctrl_pm_init(struct
    > intel_pinctrl *pctrl)
    > return -ENOMEM;
    >
    > communities[i].intmask = intmask;
    > +
    > + hostown = devm_kcalloc(pctrl->dev, community->ngpps,
    > + sizeof(*hostown), GFP_KERNEL);
    > + if (!hostown)
    > + return -ENOMEM;
    > +
    > + communities[i].hostown= hostown;

    This is good.

    > }

    > +static void intel_save_hostown(struct intel_pinctrl *pctrl, unsigned int pin)
    > +{
    > + const struct intel_community *community;
    > + const struct intel_padgroup *padgrp;
    > + int i;
    > +
    > + community = intel_get_community(pctrl, pin);
    > + if (!community)
    > + return;
    > + if (!community->hostown_offset)
    > + return;
    > +
    > + padgrp = intel_community_get_padgroup(community, pin);
    > + if (!padgrp)
    > + return;
    > +
    > + for (i = 0; i < pctrl->ncommunities; i++) {
    > + const struct intel_community *comm = &pctrl->communities[i];
    > + int j;
    > +
    > + for (j = 0; j < comm->ngpps; j++) {
    > + const struct intel_padgroup *pgrp = &comm->gpps[j];
    > +
    > + if (padgrp == pgrp) {
    > + struct intel_community_context *communities;
    > + void __iomem *base;
    > +
    > + communities = pctrl->context.communities;
    > + base = community->regs +
    > community->hostown_offset;
    > + communities[i].hostown[j] = readl(base + j * 4);
    > + break;
    > + }
    > + }
    > + }

    > + return;

    Useless.

    > +}

    This is too complicated. Just add

    base = community->regs + community->hostown_offset;
    for (gpp = 0; gpp < community->ngpps; gpp++)
    communities[i].hostown[gpp] = readl(base + gpp * 4);

    into ->suspend() loop.

    > + base = community->regs + community->hostown_offset;
    > + for (gpp = 0; gpp < community->ngpps; gpp++) {
    > + if (communities[i].hostown[gpp] &&
    > + communities[i].hostown[gpp] != readl(base
    > + gpp * 4)) {
    > + writel(communities[i].hostown[gpp],
    > base + gpp * 4);
    > + dev_warn(dev, "hostown changed after resume\n");
    > + dev_dbg(dev, "restored hostown %d/%u
    > %#08x\n", i, gpp,
    > + readl(base + gpp * 4));
    > + }
    > + }

    Instead you may need to loop over each pin in the part of the group related to
    one 32-bit HOSTSW_OWN register (i.e. 8, see PADOWN_*() macros in the driver),
    check if it's requested and break a loop. If loop index is off-by-one a limit,
    nothing to do, otherwise restore hostown register.

    More pedantic approach is to collect the mask inside the loop and apply it.

    The check function name is gpiochip_is_requested().

    (One of Intel's drivers which is using that at ->resume() is
    drivers/gpio/gpio-lynxpoint.c)

    P.S. I prefer pedantic approach. The simplification one is showed in order to
    give you an idea.

    --
    With Best Regards,
    Andy Shevchenko


    \
     
     \ /
      Last update: 2019-04-02 13:59    [W:2.241 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site