| Message ID | 1368724352-10849-2-git-send-email-dianders@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Doug, Thanks for the patch. See my comments inline. On Thursday 16 of May 2013 10:12:31 Doug Anderson wrote: > The GPIO states need to be restored after s2r and this is not currently > supported in the pinctrl driver. This patch saves the gpio states before > suspend and restores them after resume. > > Logic and commenting for samsung_pinctrl_resume_noirq() is heavily > borrowed from the old samsung_gpio_pm_2bit_resume(), which seemed to > do this a reasonable way. > > Patch originally from Prathyush K <prathyush.k@samsung.com> but > rewritten by Doug Anderson <dianders@chromium.org>. > > Signed-off-by: Prathyush K <prathyush.k@samsung.com> > Signed-off-by: Doug Anderson <dianders@chromium.org> > --- > drivers/pinctrl/pinctrl-samsung.c | 199 > ++++++++++++++++++++++++++++++++++++++ > drivers/pinctrl/pinctrl-samsung.h | 11 +++ > 2 files changed, 210 insertions(+) > > diff --git a/drivers/pinctrl/pinctrl-samsung.c > b/drivers/pinctrl/pinctrl-samsung.c index 9763668..0891667 100644 > --- a/drivers/pinctrl/pinctrl-samsung.c > +++ b/drivers/pinctrl/pinctrl-samsung.c > @@ -964,6 +964,204 @@ static int samsung_pinctrl_probe(struct > platform_device *pdev) return 0; > } > > +#ifdef CONFIG_PM > + > +/** > + * samsung_pinctrl_resume_noirq - save pinctrl state for suspend > + * > + * Save data for all banks handled by this device. > + */ > +static int samsung_pinctrl_suspend_noirq(struct device *dev) > +{ > + struct samsung_pinctrl_drv_data *drvdata = dev_get_drvdata(dev); > + struct samsung_pin_ctrl *ctrl = drvdata->ctrl; > + void __iomem * const virt_base = drvdata->virt_base; Nit: This const is ugly :) . Is it needed for anything? > + int i; > + > + for (i = 0; i < ctrl->nr_banks; i++) { > + struct samsung_pin_bank *bank = &ctrl->pin_banks[i]; > + const struct samsung_pin_bank_type *type = bank->type; > + void __iomem * const reg = virt_base + bank->pctl_offset; Nit: This one is not pretty either. > + bank->pm_save.con = readl(reg + > + type- >reg_offset[PINCFG_TYPE_FUNC]); > + if (type->fld_width[PINCFG_TYPE_FUNC] > 4) What is this condition supposed to check? > + bank->pm_save.con |= (u64)readl(reg + 4 + > + type->reg_offset[PINCFG_TYPE_FUNC]) << 32; This looks ugly. Whatever is going on here, wouldn't it be better to use separate field, like con2 or something? > + bank->pm_save.dat = readl(reg + > + type- >reg_offset[PINCFG_TYPE_DAT]); > + bank->pm_save.pud = readl(reg + > + type- >reg_offset[PINCFG_TYPE_PUD]); > + bank->pm_save.drv = readl(reg + > + type- >reg_offset[PINCFG_TYPE_DRV]); > + > + if (type->fld_width[PINCFG_TYPE_CON_PDN]) { > + bank->pm_save.conpdn = readl(reg + > + type->reg_offset[PINCFG_TYPE_CON_PDN]); > + bank->pm_save.pudpdn = readl(reg + > + type->reg_offset[PINCFG_TYPE_PUD_PDN]); > + } I wonder if you couldn't do all the saving here in a single loop over all pin control types, like: unsigned int offsets = bank->type->reg_offsets; unsigned int widths = bank->type->fld_width; for (i = 0; i < PINCFG_TYPE_NUM; ++i) if (widths[i]) bank->pm_save[i] = readl(reg + offsets[i]); The only thing not handled by this loop is second CON registers in banks with two of them. I can't think of any better solution for this other than just adding a special case after the loop. > + dev_dbg(dev, "Save %s @ %p (con %#010llx)\n", > + bank->name, reg, bank->pm_save.con); > + } > + > + return 0; > +} > + > +/** > + * is_sfn - test whether a pin config represents special function. > + * > + * Test whether the given masked+shifted bits of an GPIO configuration > + * are one of the SFN (special function) modes. > + */ > +static inline int is_sfn(u32 con) > +{ > + return con >= 2; > +} > + > +/** > + * is_in - test if the given masked+shifted GPIO configuration is an > input. + */ > +static inline int is_in(u32 con) > +{ > + return con == 0; > +} > + > +/** > + * is_out - test if the given masked+shifted GPIO configuration is an > output. + */ > +static inline int is_out(u32 con) > +{ > + return con == 1; > +} > + > +/** > + * samsung_pinctrl_resume_noirq - restore pinctrl state from suspend > + * > + * Restore one of the GPIO banks that was saved during suspend. This is > + * not as simple as once thought, due to the possibility of glitches + > * from the order that the CON and DAT registers are set in. > + * > + * The three states the pin can be are {IN,OUT,SFN} which gives us 9 > + * combinations of changes to check. Three of these, if the pin stays > + * in the same configuration can be discounted. This leaves us with > + * the following: > + * > + * { IN => OUT } Change DAT first > + * { IN => SFN } Change CON first > + * { OUT => SFN } Change CON first, so new data will not glitch > + * { OUT => IN } Change CON first, so new data will not glitch > + * { SFN => IN } Change CON first > + * { SFN => OUT } Change DAT first, so new data will not glitch [1] > + * > + * We do not currently deal with the UP registers as these control > + * weak resistors, so a small delay in change should not need to bring > + * these into the calculations. > + * > + * [1] this assumes that writing to a pin DAT whilst in SFN will set > the + * state for when it is next output. > + */ > +static int samsung_pinctrl_resume_noirq(struct device *dev) > +{ > + struct samsung_pinctrl_drv_data *drvdata = dev_get_drvdata(dev); > + struct samsung_pin_ctrl *ctrl = drvdata->ctrl; > + void __iomem * const virt_base = drvdata->virt_base; > + int i; > + > + for (i = 0; i < ctrl->nr_banks; i++) { > + const struct samsung_pin_bank *bank = &ctrl->pin_banks[i]; > + void __iomem * const reg = virt_base + bank->pctl_offset; > + const struct samsung_pin_bank_type *type = bank->type; > + const u8 func_offset = type->reg_offset[PINCFG_TYPE_FUNC]; > + const u32 func_width = type->fld_width[PINCFG_TYPE_FUNC]; > + const u32 func_mask = (1 << func_width) - 1; I'm constantly seeing so many consts in this patch. Is this constraint relevant here in any way? Git grep doesn't show too many instances of this construct used for local variables around the kernel. > + const bool is64bit = type->fld_width[PINCFG_TYPE_FUNC] > 4; Again this check. Is it supposed to handle banks with two CON registers? If yes, it is incorrect, as they have 4 bits per pin. > + const u64 to_con = bank->pm_save.con; > + u64 from_con = readl(reg + func_offset); > + u64 change_mask = 0; > + u64 to_write; > + int pin; > + > + if (is64bit) > + from_con |= (u64)readl(reg + 4 + func_offset) << 32; Same comment as in save. > + > + /* Easy--the PUD and DRV can go first */ > + writel(bank->pm_save.pud, > + reg + type->reg_offset[PINCFG_TYPE_PUD]); > + writel(bank->pm_save.drv, > + reg + type->reg_offset[PINCFG_TYPE_DRV]); > + > + /* > + * Create a change_mask of all the items that need to have > + * their CON value changed before their DAT value, so that > + * we minimise the work between the two settings. > + */ > + for (pin = 0; pin < bank->nr_pins; pin++) { This is a bit more tricky that in case of save, so CON register as a special case is justified here. > + u32 shift = pin * func_width; > + u32 from_func = (from_con >> shift) & func_mask; > + u32 to_func = (to_con >> shift) & func_mask; > + > + /* If there is no change, then skip */ > + if (from_func == to_func) > + continue; > + > + /* If both are special function, then skip */ > + if (is_sfn(from_func) && is_sfn(to_func)) > + continue; > + > + /* Change is IN => OUT, do not change now */ > + if (is_in(from_func) && is_out(to_func)) > + continue; > + > + /* Change is SFN => OUT, do not change now */ > + if (is_sfn(from_func) && is_out(to_func)) > + continue; > + > + /* > + * We should now be at the case of: > + * IN=>SFN, OUT=>SFN, OUT=>IN, SFN=>IN. > + */ > + change_mask |= (func_mask << shift); > + } > + > + /* Write the new CON settings */ > + to_write = (from_con & ~change_mask) | (to_con & change_mask); > + writel((u32)to_write, reg + func_offset); > + if (is64bit) > + writel((u32)(to_write >> 32), reg + 4 + func_offset); > + > + /* Now change any items that require DAT,CON */ > + writel(bank->pm_save.dat, > + reg + type->reg_offset[PINCFG_TYPE_DAT]); > + writel((u32)to_con, reg + func_offset); > + if (is64bit) > + writel((u32)(to_con >> 32), reg + 4 + to_con); > + > + if (type->fld_width[PINCFG_TYPE_CON_PDN]) { > + writel(bank->pm_save.conpdn, > + reg + type- >reg_offset[PINCFG_TYPE_CON_PDN]); > + writel(bank->pm_save.pudpdn, > + reg + type- >reg_offset[PINCFG_TYPE_PUD_PDN]); > + } Now as I think of it, do CON_PDN and PUD_PDN really need to be saved? I couldn't find in the documentation if they are preserved or lost in sleep mode. Do you have some information on this? My experiments on Exynos4210 and 4x12 showed that they are preserved, but I don't know what about other SoCs. > + dev_dbg(dev, "Restore %s@%p (%#010llx=>%#010llx ch %#010llx)\n", > + bank->name, reg, from_con, to_con, change_mask); > + } > + > + return 0; > +} I wonder if the whole restoration procedure couldn't be simplified. I don't remember my version being so complicated, but I don't have my patch on my screen at the moment, so I might be wrong on this. > +#else > +#define samsung_pinctrl_suspend_noirq NULL > +#define samsung_pinctrl_resume_noirq NULL > +#endif > + > +static const struct dev_pm_ops samsung_pinctrl_dev_pm_ops = { > + .suspend_noirq = samsung_pinctrl_suspend_noirq, > + .resume_noirq = samsung_pinctrl_resume_noirq, > +}; I'm not sure if resume_noirq is really early enough. Some drivers might already need correct pin configuration in their resume_noirq callback. In my patch I have used syscore_ops to register very late suspend and very early resume callbacks for the whole pinctrl-samsung driver and a global list of registered pin controllers, that is iterated over in suspend and resume. > static const struct of_device_id samsung_pinctrl_dt_match[] = { > #ifdef CONFIG_PINCTRL_EXYNOS > { .compatible = "samsung,exynos4210-pinctrl", > @@ -987,6 +1185,7 @@ static struct platform_driver > samsung_pinctrl_driver = { .name = "samsung-pinctrl", > .owner = THIS_MODULE, > .of_match_table = of_match_ptr(samsung_pinctrl_dt_match), > + .pm = &samsung_pinctrl_dev_pm_ops, > }, > }; > > diff --git a/drivers/pinctrl/pinctrl-samsung.h > b/drivers/pinctrl/pinctrl-samsung.h index 7c7f9eb..c9a7b6e 100644 > --- a/drivers/pinctrl/pinctrl-samsung.h > +++ b/drivers/pinctrl/pinctrl-samsung.h > @@ -127,6 +127,7 @@ struct samsung_pin_bank_type { > * @gpio_chip: GPIO chip of the bank. > * @grange: linux gpio pin range supported by this bank. > * @slock: spinlock protecting bank registers > + * @pm_save: saved register values during suspend > */ > struct samsung_pin_bank { > struct samsung_pin_bank_type *type; > @@ -144,6 +145,16 @@ struct samsung_pin_bank { > struct gpio_chip gpio_chip; > struct pinctrl_gpio_range grange; > spinlock_t slock; > +#ifdef CONFIG_PM > + struct { > + u64 con; > + u32 dat; > + u32 pud; > + u32 drv; > + u32 conpdn; > + u32 pudpdn; This could be changed into an array. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Tomasz, Thanks for the review! I'll get a new patch out either today or tomorrow... On Thu, May 16, 2013 at 12:19 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: >> +/** >> + * samsung_pinctrl_resume_noirq - save pinctrl state for suspend >> + * >> + * Save data for all banks handled by this device. >> + */ >> +static int samsung_pinctrl_suspend_noirq(struct device *dev) >> +{ >> + struct samsung_pinctrl_drv_data *drvdata = dev_get_drvdata(dev); >> + struct samsung_pin_ctrl *ctrl = drvdata->ctrl; >> + void __iomem * const virt_base = drvdata->virt_base; > > Nit: This const is ugly :) . Is it needed for anything? Not anything really. I just got in the habit of adding them for variables that are simple shorthand variables: AKA I'm only creating this variable to avoid typing some long complicated thing below. It's a hint to someone reading the code that they don't need to think about it. I have also occasionally caught bugs by doing this. ...but I can understand the dislike. I'll remove this and other similar (but keep const pointers). >> + if (type->fld_width[PINCFG_TYPE_FUNC] > 4) > > What is this condition supposed to check? It is supposed to be checking whether there are two CON registers in use. ...oh, but that's probably not the right way to do it now that I think about it. I need to check (bank->nr_pins * type->fld_width[PINCFG_TYPE_FUNC]). I will fix. >> + bank->pm_save.con |= (u64)readl(reg + 4 + >> + type->reg_offset[PINCFG_TYPE_FUNC]) > << 32; > > This looks ugly. Whatever is going on here, wouldn't it be better to use > separate field, like con2 or something? Probably. The resume code seemed cleaner with a 64-bit value, but I think I could make it nearly as clean with two 32-bit ones by using a helper function. > I wonder if you couldn't do all the saving here in a single loop over all > pin control types, like: > > unsigned int offsets = bank->type->reg_offsets; > unsigned int widths = bank->type->fld_width; > > for (i = 0; i < PINCFG_TYPE_NUM; ++i) > if (widths[i]) > bank->pm_save[i] = readl(reg + offsets[i]); > > The only thing not handled by this loop is second CON registers in banks > with two of them. I can't think of any better solution for this other than > just adding a special case after the loop. Yes, that would work. I think it wasn't possible when I first wrote the code against an older code base that didn't have the arrays. I can give it a shot if it doesn't make restore too bad... > Now as I think of it, do CON_PDN and PUD_PDN really need to be saved? I > couldn't find in the documentation if they are preserved or lost in sleep > mode. Do you have some information on this? I remember it being important. Running a test now. Yes, it's important on exynos5250. As an example: [ 62.220000] samsung-pinctrl 11400000.pinctrl: Restore gpa2@f0048040 (0x22220000=>0x22123333 ch 0x0000ffff) [ 62.220000] samsung-pinctrl 11400000.pinctrl: Restore gpb0@f0048060 CON_PDN (0x00000000=>0x000002aa) [ 62.220000] samsung-pinctrl 11400000.pinctrl: Restore gpb0@f0048060 PUD_PDN (0x00000000=>0x00000155) > I wonder if the whole restoration procedure couldn't be simplified. I > don't remember my version being so complicated, but I don't have my patch > on my screen at the moment, so I might be wrong on this. I debated about this a bunch. Perhaps I should just delete it. I saw that it was there in the old "2-bit" code and it also seemed quite reasonable, so I kept it. Things seem to work OK without it, but most things are pretty tolerant to their lines glitching (or even driving high to low for a short period of time). ...but your question made me check again. From previous experimentation I'm pretty certain that most pins on the exynos are held in the powerdown state even during early bootup of the SoC. The hope is that they are released from powerdown _after_ the GPIO init is called. If not then we're already glitching somewhat as we transition from powerdown state to default state before this function finally gets called. Looking at exynos, that's probably done in exynos_pm_resume(), maybe by mucking with the pad retention options? Oh, that probably means taht no_irq() is too late and that I need to figure out how to get my code called earlier... The exynos_pm_resume() is called by syscore. >> +#else >> +#define samsung_pinctrl_suspend_noirq NULL >> +#define samsung_pinctrl_resume_noirq NULL >> +#endif >> + >> +static const struct dev_pm_ops samsung_pinctrl_dev_pm_ops = { >> + .suspend_noirq = samsung_pinctrl_suspend_noirq, >> + .resume_noirq = samsung_pinctrl_resume_noirq, >> +}; > > I'm not sure if resume_noirq is really early enough. Some drivers might > already need correct pin configuration in their resume_noirq callback. > > In my patch I have used syscore_ops to register very late suspend and very > early resume callbacks for the whole pinctrl-samsung driver and a global > list of registered pin controllers, that is iterated over in suspend and > resume. OK, as per above syscore is actually important. ...and in fact I need to figure out how to ensure that this is called _before_ exynos_pm_resume(), which is also syscore. I guess I also need to stash an array of devices in an global somewhere, since syscore passes no params... -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Donnerstag, 16. Mai 2013, 21:19:20 schrieb Tomasz Figa: [...] > > + > > + if (type->fld_width[PINCFG_TYPE_CON_PDN]) { > > + bank->pm_save.conpdn = readl(reg + > > + type->reg_offset[PINCFG_TYPE_CON_PDN]); > > + bank->pm_save.pudpdn = readl(reg + > > + type->reg_offset[PINCFG_TYPE_PUD_PDN]); > > + } > > I wonder if you couldn't do all the saving here in a single loop over all > pin control types, like: > > unsigned int offsets = bank->type->reg_offsets; > unsigned int widths = bank->type->fld_width; > > for (i = 0; i < PINCFG_TYPE_NUM; ++i) > if (widths[i]) > bank->pm_save[i] = readl(reg + offsets[i]); > > The only thing not handled by this loop is second CON registers in banks > with two of them. I can't think of any better solution for this other than > just adding a special case after the loop. doing this in the loop over the pinctrl types like Tomasz suggests, also nicely fixes the problem of s3c24xx [0] only having FUNC, DAT and PUD and some (gpa) not even having the PUD, which was not checked in the original patch. Heiko [0] patch is with Kgene currently, so should make it into 3.11 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 16 of May 2013 13:32:48 Doug Anderson wrote: > Tomasz, > > Thanks for the review! I'll get a new patch out either today or > tomorrow... OK. I will be fine to go with your patches, after addressing the comments. In the end it's good that you posted them, as reviewing them allowed me to find even better ways of doing some things than I had in mine ;) . [snip] > > > I wonder if you couldn't do all the saving here in a single loop over > > all> > > pin control types, like: > > unsigned int offsets = bank->type->reg_offsets; > > unsigned int widths = bank->type->fld_width; > > > > for (i = 0; i < PINCFG_TYPE_NUM; ++i) > > > > if (widths[i]) > > > > bank->pm_save[i] = readl(reg + offsets[i]); > > > > The only thing not handled by this loop is second CON registers in > > banks with two of them. I can't think of any better solution for this > > other than just adding a special case after the loop. > > Yes, that would work. I think it wasn't possible when I first wrote > the code against an older code base that didn't have the arrays. I > can give it a shot if it doesn't make restore too bad... OK. I wonder if resume couldn't somehow benefit from this as well, but it probably depends on how much it can be altered without breaking the anti- glitch functionality. > > Now as I think of it, do CON_PDN and PUD_PDN really need to be saved? > > I > > couldn't find in the documentation if they are preserved or lost in > > sleep mode. Do you have some information on this? > > I remember it being important. Running a test now. Yes, it's > important on exynos5250. As an example: > > [ 62.220000] samsung-pinctrl 11400000.pinctrl: Restore gpa2@f0048040 > (0x22220000=>0x22123333 ch 0x0000ffff) > [ 62.220000] samsung-pinctrl 11400000.pinctrl: Restore gpb0@f0048060 > CON_PDN (0x00000000=>0x000002aa) > [ 62.220000] samsung-pinctrl 11400000.pinctrl: Restore gpb0@f0048060 > PUD_PDN (0x00000000=>0x00000155) OK. It's good to know. > > I wonder if the whole restoration procedure couldn't be simplified. I > > don't remember my version being so complicated, but I don't have my > > patch on my screen at the moment, so I might be wrong on this. > > I debated about this a bunch. Perhaps I should just delete it. I saw > that it was there in the old "2-bit" code and it also seemed quite > reasonable, so I kept it. Things seem to work OK without it, but most > things are pretty tolerant to their lines glitching (or even driving > high to low for a short period of time). > > ...but your question made me check again. > > From previous experimentation I'm pretty certain that most pins on the > exynos are held in the powerdown state even during early bootup of the > SoC. The hope is that they are released from powerdown _after_ the > GPIO init is called. If not then we're already glitching somewhat as > we transition from powerdown state to default state before this > function finally gets called. > > Looking at exynos, that's probably done in exynos_pm_resume(), maybe > by mucking with the pad retention options? > > Oh, that probably means taht no_irq() is too late and that I need to > figure out how to get my code called earlier... The > exynos_pm_resume() is called by syscore. How all of this works is basically a good question. I couldn't find any mention about pins switching from power down to normal mode in the documentation, but maybe there is a small side note somewhere, which I could miss. On S3C6410, for example, there are two modes. State is switched to power down mode automatically, but can be switched out either automatically on wake-up (exact timing is unknown to me) or by clearing a special bit, depending on value of other special bit. IMHO this is rather important, so we should find out how it work on other SoCs and make the code account for it. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Tomasz, On Thu, May 16, 2013 at 2:27 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > OK. I will be fine to go with your patches, after addressing the comments. > In the end it's good that you posted them, as reviewing them allowed me to > find even better ways of doing some things than I had in mine ;) . Yes. I often find that the best way to review code is to think about how I would implement it myself. Certainly I think we've ended up with something better / less buggy this way. ;) > How all of this works is basically a good question. I couldn't find any > mention about pins switching from power down to normal mode in the > documentation, but maybe there is a small side note somewhere, which I > could miss. > > On S3C6410, for example, there are two modes. State is switched to power > down mode automatically, but can be switched out either automatically on > wake-up (exact timing is unknown to me) or by clearing a special bit, > depending on value of other special bit. > > IMHO this is rather important, so we should find out how it work on other > SoCs and make the code account for it. Agreed that it's important. ...but it's also good not to have tons of complexity when it's not needed. It sounds like S3C6410 could be handled OK by just using the special bits and waiting to take things out of power down mode. ...thinking about it, all SoCs that have power down modes (which you _must_ have if your pinctrl state is lost across a low power) would be slightly broken if they didn't have a bit to switch out of power down mode. Otherwise you're asking for at least some type of glitch because you'll end up in the default state of pins for a little while during resume. That's not to say that there aren't broken SoCs out there and it's entirely possible that people even designed systems around them (knowing that the default state of each pin after wakeup is not harmful to whatever is connected to that pin). If there are any cases like this then they would need the special code like my V1 patch had. Do you know of any SoCs like this that we need to support on kernel 3.10 and higher? I'm planning on going back to the "simpler" code for my next patchset unless I can find a problem with it. -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 16 of May 2013 14:51:53 Doug Anderson wrote: > Tomasz, > > On Thu, May 16, 2013 at 2:27 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > > OK. I will be fine to go with your patches, after addressing the > > comments. In the end it's good that you posted them, as reviewing > > them allowed me to find even better ways of doing some things than I > > had in mine ;) . > Yes. I often find that the best way to review code is to think about > how I would implement it myself. Certainly I think we've ended up > with something better / less buggy this way. ;) > > > How all of this works is basically a good question. I couldn't find > > any > > mention about pins switching from power down to normal mode in the > > documentation, but maybe there is a small side note somewhere, which I > > could miss. > > > > On S3C6410, for example, there are two modes. State is switched to > > power down mode automatically, but can be switched out either > > automatically on wake-up (exact timing is unknown to me) or by > > clearing a special bit, depending on value of other special bit. > > > > IMHO this is rather important, so we should find out how it work on > > other SoCs and make the code account for it. > > Agreed that it's important. ...but it's also good not to have tons of > complexity when it's not needed. It sounds like S3C6410 could be > handled OK by just using the special bits and waiting to take things > out of power down mode. > > ...thinking about it, all SoCs that have power down modes (which you > _must_ have if your pinctrl state is lost across a low power) would be > slightly broken if they didn't have a bit to switch out of power down > mode. Otherwise you're asking for at least some type of glitch > because you'll end up in the default state of pins for a little while > during resume. > > That's not to say that there aren't broken SoCs out there and it's > entirely possible that people even designed systems around them > (knowing that the default state of each pin after wakeup is not > harmful to whatever is connected to that pin). If there are any cases > like this then they would need the special code like my V1 patch had. > Do you know of any SoCs like this that we need to support on kernel > 3.10 and higher? Hmm, I just checked documentation of S3C2440 and S3C2416 they seem to retain GPIO settings completely in sleep mode. This would mean that they don't require any suspend/resume support in pinctrl driver. Heiko, can you confirm this? Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Freitag, 17. Mai 2013, 00:08:34 schrieb Tomasz Figa: > On Thursday 16 of May 2013 14:51:53 Doug Anderson wrote: > > Tomasz, > > > > On Thu, May 16, 2013 at 2:27 PM, Tomasz Figa <tomasz.figa@gmail.com> > > wrote: > > > OK. I will be fine to go with your patches, after addressing the > > > comments. In the end it's good that you posted them, as reviewing > > > them allowed me to find even better ways of doing some things than I > > > had in mine ;) . > > > > Yes. I often find that the best way to review code is to think about > > how I would implement it myself. Certainly I think we've ended up > > with something better / less buggy this way. ;) > > > > > How all of this works is basically a good question. I couldn't find > > > any > > > mention about pins switching from power down to normal mode in the > > > documentation, but maybe there is a small side note somewhere, which I > > > could miss. > > > > > > On S3C6410, for example, there are two modes. State is switched to > > > power down mode automatically, but can be switched out either > > > automatically on wake-up (exact timing is unknown to me) or by > > > clearing a special bit, depending on value of other special bit. > > > > > > IMHO this is rather important, so we should find out how it work on > > > other SoCs and make the code account for it. > > > > Agreed that it's important. ...but it's also good not to have tons of > > complexity when it's not needed. It sounds like S3C6410 could be > > handled OK by just using the special bits and waiting to take things > > out of power down mode. > > > > ...thinking about it, all SoCs that have power down modes (which you > > _must_ have if your pinctrl state is lost across a low power) would be > > slightly broken if they didn't have a bit to switch out of power down > > mode. Otherwise you're asking for at least some type of glitch > > because you'll end up in the default state of pins for a little while > > during resume. > > > > That's not to say that there aren't broken SoCs out there and it's > > entirely possible that people even designed systems around them > > (knowing that the default state of each pin after wakeup is not > > harmful to whatever is connected to that pin). If there are any cases > > like this then they would need the special code like my V1 patch had. > > Do you know of any SoCs like this that we need to support on kernel > > 3.10 and higher? > > Hmm, I just checked documentation of S3C2440 and S3C2416 they seem to > retain GPIO settings completely in sleep mode. This would mean that they > don't require any suspend/resume support in pinctrl driver. Heiko, can you > confirm this? Hmm, my system does not have a working suspend right now, but looking at the legacy code (mach-s3c24xx/pm.c, etc) tells me that the gpio banks were never saved during suspend. And as there were (and still are) systems with working suspend around, I'd assume that you're correct that the pins retain their state. Is the same true for the s3c64xx, as I didn't find any gpio suspend handling for it either. Heiko -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 17 of May 2013 00:30:38 Heiko Stübner wrote: > Am Freitag, 17. Mai 2013, 00:08:34 schrieb Tomasz Figa: > > On Thursday 16 of May 2013 14:51:53 Doug Anderson wrote: > > > Tomasz, > > > > > > On Thu, May 16, 2013 at 2:27 PM, Tomasz Figa <tomasz.figa@gmail.com> > > > > wrote: > > > > OK. I will be fine to go with your patches, after addressing the > > > > comments. In the end it's good that you posted them, as reviewing > > > > them allowed me to find even better ways of doing some things than > > > > I > > > > had in mine ;) . > > > > > > Yes. I often find that the best way to review code is to think > > > about > > > how I would implement it myself. Certainly I think we've ended up > > > with something better / less buggy this way. ;) > > > > > > > How all of this works is basically a good question. I couldn't > > > > find > > > > any > > > > mention about pins switching from power down to normal mode in the > > > > documentation, but maybe there is a small side note somewhere, > > > > which I > > > > could miss. > > > > > > > > On S3C6410, for example, there are two modes. State is switched to > > > > power down mode automatically, but can be switched out either > > > > automatically on wake-up (exact timing is unknown to me) or by > > > > clearing a special bit, depending on value of other special bit. > > > > > > > > IMHO this is rather important, so we should find out how it work > > > > on > > > > other SoCs and make the code account for it. > > > > > > Agreed that it's important. ...but it's also good not to have tons > > > of > > > complexity when it's not needed. It sounds like S3C6410 could be > > > handled OK by just using the special bits and waiting to take things > > > out of power down mode. > > > > > > ...thinking about it, all SoCs that have power down modes (which you > > > _must_ have if your pinctrl state is lost across a low power) would > > > be > > > slightly broken if they didn't have a bit to switch out of power > > > down > > > mode. Otherwise you're asking for at least some type of glitch > > > because you'll end up in the default state of pins for a little > > > while > > > during resume. > > > > > > That's not to say that there aren't broken SoCs out there and it's > > > entirely possible that people even designed systems around them > > > (knowing that the default state of each pin after wakeup is not > > > harmful to whatever is connected to that pin). If there are any > > > cases > > > like this then they would need the special code like my V1 patch > > > had. > > > Do you know of any SoCs like this that we need to support on kernel > > > 3.10 and higher? > > > > Hmm, I just checked documentation of S3C2440 and S3C2416 they seem to > > retain GPIO settings completely in sleep mode. This would mean that > > they don't require any suspend/resume support in pinctrl driver. > > Heiko, can you confirm this? > > Hmm, my system does not have a working suspend right now, but looking at > the legacy code (mach-s3c24xx/pm.c, etc) tells me that the gpio banks > were never saved during suspend. > > And as there were (and still are) systems with working suspend around, > I'd assume that you're correct that the pins retain their state. > > Is the same true for the s3c64xx, as I didn't find any gpio suspend > handling for it either. Seems like I need some sleep, as I'm already starting to overlook large blobs of code. Originally, GPIO suspend/resume handlers have been configured in drivers/gpio/gpio-samsung.c, by setting pm field of samsung_gpio_chip struct to point to appropriate samsung_gpio_pm struct, which contains pointers to save and resume callbacks. In result, samsung_gpio_pm_2bit_* or samsung_gpio_pm_4bit_* have been used, depending on bank type, on all SoCs. Now since the documentation states that wake-up reset doesn't reset GPIO registers (at least on S3C2440 and S3C2416), I wonder what is the correct way of handling them. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Tomasz, On Thu, May 16, 2013 at 3:56 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > Seems like I need some sleep, as I'm already starting to overlook large > blobs of code. > > Originally, GPIO suspend/resume handlers have been configured in > drivers/gpio/gpio-samsung.c, by setting pm field of samsung_gpio_chip > struct to point to appropriate samsung_gpio_pm struct, which contains > pointers to save and resume callbacks. > > In result, samsung_gpio_pm_2bit_* or samsung_gpio_pm_4bit_* have been > used, depending on bank type, on all SoCs. > > Now since the documentation states that wake-up reset doesn't reset GPIO > registers (at least on S3C2440 and S3C2416), I wonder what is the correct > way of handling them. If state of these registers isn't lost on those SoCs then running the save/restore shouldn't _hurt_ though, right? If you can run the old GPIO code on one of those systems and do a suspend/resume you could check... -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Tomasz, On Thu, May 16, 2013 at 4:10 PM, Doug Anderson <dianders@chromium.org> wrote: > If state of these registers isn't lost on those SoCs then running the > save/restore shouldn't _hurt_ though, right? If you can run the old > GPIO code on one of those systems and do a suspend/resume you could > check... I think it's been too long of a day for me, too. I just thought about this and realized that there is no "powerdown" registers for the GPX banks on exynos5250. ...and they don't lose their state at sleep! ...so maybe a reasonable thing to do would be to skip save/restore in any case where there are no powerdown registers? You can see a printout in my case: [ 412.840000] gpx0 @ f004ac00 (con 0x30000110 => 0x30000110) [ 412.840000] gpx1 @ f004ac20 (con 0x1f10ff10 => 0x1f10ff10) [ 412.840000] gpx2 @ f004ac40 (con 0x1f000f0f => 0x1f000f0f) [ 412.840000] gpx3 @ f004ac60 (con 0x00f00f01 => 0x00f00f01) -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pinctrl/pinctrl-samsung.c b/drivers/pinctrl/pinctrl-samsung.c index 9763668..0891667 100644 --- a/drivers/pinctrl/pinctrl-samsung.c +++ b/drivers/pinctrl/pinctrl-samsung.c @@ -964,6 +964,204 @@ static int samsung_pinctrl_probe(struct platform_device *pdev) return 0; } +#ifdef CONFIG_PM + +/** + * samsung_pinctrl_resume_noirq - save pinctrl state for suspend + * + * Save data for all banks handled by this device. + */ +static int samsung_pinctrl_suspend_noirq(struct device *dev) +{ + struct samsung_pinctrl_drv_data *drvdata = dev_get_drvdata(dev); + struct samsung_pin_ctrl *ctrl = drvdata->ctrl; + void __iomem * const virt_base = drvdata->virt_base; + int i; + + for (i = 0; i < ctrl->nr_banks; i++) { + struct samsung_pin_bank *bank = &ctrl->pin_banks[i]; + const struct samsung_pin_bank_type *type = bank->type; + void __iomem * const reg = virt_base + bank->pctl_offset; + + bank->pm_save.con = readl(reg + + type->reg_offset[PINCFG_TYPE_FUNC]); + if (type->fld_width[PINCFG_TYPE_FUNC] > 4) + bank->pm_save.con |= (u64)readl(reg + 4 + + type->reg_offset[PINCFG_TYPE_FUNC]) << 32; + bank->pm_save.dat = readl(reg + + type->reg_offset[PINCFG_TYPE_DAT]); + bank->pm_save.pud = readl(reg + + type->reg_offset[PINCFG_TYPE_PUD]); + bank->pm_save.drv = readl(reg + + type->reg_offset[PINCFG_TYPE_DRV]); + + if (type->fld_width[PINCFG_TYPE_CON_PDN]) { + bank->pm_save.conpdn = readl(reg + + type->reg_offset[PINCFG_TYPE_CON_PDN]); + bank->pm_save.pudpdn = readl(reg + + type->reg_offset[PINCFG_TYPE_PUD_PDN]); + } + + dev_dbg(dev, "Save %s @ %p (con %#010llx)\n", + bank->name, reg, bank->pm_save.con); + } + + return 0; +} + +/** + * is_sfn - test whether a pin config represents special function. + * + * Test whether the given masked+shifted bits of an GPIO configuration + * are one of the SFN (special function) modes. + */ +static inline int is_sfn(u32 con) +{ + return con >= 2; +} + +/** + * is_in - test if the given masked+shifted GPIO configuration is an input. + */ +static inline int is_in(u32 con) +{ + return con == 0; +} + +/** + * is_out - test if the given masked+shifted GPIO configuration is an output. + */ +static inline int is_out(u32 con) +{ + return con == 1; +} + +/** + * samsung_pinctrl_resume_noirq - restore pinctrl state from suspend + * + * Restore one of the GPIO banks that was saved during suspend. This is + * not as simple as once thought, due to the possibility of glitches + * from the order that the CON and DAT registers are set in. + * + * The three states the pin can be are {IN,OUT,SFN} which gives us 9 + * combinations of changes to check. Three of these, if the pin stays + * in the same configuration can be discounted. This leaves us with + * the following: + * + * { IN => OUT } Change DAT first + * { IN => SFN } Change CON first + * { OUT => SFN } Change CON first, so new data will not glitch + * { OUT => IN } Change CON first, so new data will not glitch + * { SFN => IN } Change CON first + * { SFN => OUT } Change DAT first, so new data will not glitch [1] + * + * We do not currently deal with the UP registers as these control + * weak resistors, so a small delay in change should not need to bring + * these into the calculations. + * + * [1] this assumes that writing to a pin DAT whilst in SFN will set the + * state for when it is next output. + */ +static int samsung_pinctrl_resume_noirq(struct device *dev) +{ + struct samsung_pinctrl_drv_data *drvdata = dev_get_drvdata(dev); + struct samsung_pin_ctrl *ctrl = drvdata->ctrl; + void __iomem * const virt_base = drvdata->virt_base; + int i; + + for (i = 0; i < ctrl->nr_banks; i++) { + const struct samsung_pin_bank *bank = &ctrl->pin_banks[i]; + void __iomem * const reg = virt_base + bank->pctl_offset; + const struct samsung_pin_bank_type *type = bank->type; + const u8 func_offset = type->reg_offset[PINCFG_TYPE_FUNC]; + const u32 func_width = type->fld_width[PINCFG_TYPE_FUNC]; + const u32 func_mask = (1 << func_width) - 1; + const bool is64bit = type->fld_width[PINCFG_TYPE_FUNC] > 4; + const u64 to_con = bank->pm_save.con; + u64 from_con = readl(reg + func_offset); + u64 change_mask = 0; + u64 to_write; + int pin; + + if (is64bit) + from_con |= (u64)readl(reg + 4 + func_offset) << 32; + + /* Easy--the PUD and DRV can go first */ + writel(bank->pm_save.pud, + reg + type->reg_offset[PINCFG_TYPE_PUD]); + writel(bank->pm_save.drv, + reg + type->reg_offset[PINCFG_TYPE_DRV]); + + /* + * Create a change_mask of all the items that need to have + * their CON value changed before their DAT value, so that + * we minimise the work between the two settings. + */ + for (pin = 0; pin < bank->nr_pins; pin++) { + u32 shift = pin * func_width; + u32 from_func = (from_con >> shift) & func_mask; + u32 to_func = (to_con >> shift) & func_mask; + + /* If there is no change, then skip */ + if (from_func == to_func) + continue; + + /* If both are special function, then skip */ + if (is_sfn(from_func) && is_sfn(to_func)) + continue; + + /* Change is IN => OUT, do not change now */ + if (is_in(from_func) && is_out(to_func)) + continue; + + /* Change is SFN => OUT, do not change now */ + if (is_sfn(from_func) && is_out(to_func)) + continue; + + /* + * We should now be at the case of: + * IN=>SFN, OUT=>SFN, OUT=>IN, SFN=>IN. + */ + change_mask |= (func_mask << shift); + } + + /* Write the new CON settings */ + to_write = (from_con & ~change_mask) | (to_con & change_mask); + writel((u32)to_write, reg + func_offset); + if (is64bit) + writel((u32)(to_write >> 32), reg + 4 + func_offset); + + /* Now change any items that require DAT,CON */ + writel(bank->pm_save.dat, + reg + type->reg_offset[PINCFG_TYPE_DAT]); + writel((u32)to_con, reg + func_offset); + if (is64bit) + writel((u32)(to_con >> 32), reg + 4 + to_con); + + if (type->fld_width[PINCFG_TYPE_CON_PDN]) { + writel(bank->pm_save.conpdn, + reg + type->reg_offset[PINCFG_TYPE_CON_PDN]); + writel(bank->pm_save.pudpdn, + reg + type->reg_offset[PINCFG_TYPE_PUD_PDN]); + } + + dev_dbg(dev, "Restore %s@%p (%#010llx=>%#010llx ch %#010llx)\n", + bank->name, reg, from_con, to_con, change_mask); + } + + return 0; +} + +#else +#define samsung_pinctrl_suspend_noirq NULL +#define samsung_pinctrl_resume_noirq NULL +#endif + +static const struct dev_pm_ops samsung_pinctrl_dev_pm_ops = { + .suspend_noirq = samsung_pinctrl_suspend_noirq, + .resume_noirq = samsung_pinctrl_resume_noirq, +}; + static const struct of_device_id samsung_pinctrl_dt_match[] = { #ifdef CONFIG_PINCTRL_EXYNOS { .compatible = "samsung,exynos4210-pinctrl", @@ -987,6 +1185,7 @@ static struct platform_driver samsung_pinctrl_driver = { .name = "samsung-pinctrl", .owner = THIS_MODULE, .of_match_table = of_match_ptr(samsung_pinctrl_dt_match), + .pm = &samsung_pinctrl_dev_pm_ops, }, }; diff --git a/drivers/pinctrl/pinctrl-samsung.h b/drivers/pinctrl/pinctrl-samsung.h index 7c7f9eb..c9a7b6e 100644 --- a/drivers/pinctrl/pinctrl-samsung.h +++ b/drivers/pinctrl/pinctrl-samsung.h @@ -127,6 +127,7 @@ struct samsung_pin_bank_type { * @gpio_chip: GPIO chip of the bank. * @grange: linux gpio pin range supported by this bank. * @slock: spinlock protecting bank registers + * @pm_save: saved register values during suspend */ struct samsung_pin_bank { struct samsung_pin_bank_type *type; @@ -144,6 +145,16 @@ struct samsung_pin_bank { struct gpio_chip gpio_chip; struct pinctrl_gpio_range grange; spinlock_t slock; +#ifdef CONFIG_PM + struct { + u64 con; + u32 dat; + u32 pud; + u32 drv; + u32 conpdn; + u32 pudpdn; + } pm_save; +#endif }; /**