| Message ID | 1512586115-4698-1-git-send-email-ykaneko0929@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: gpio-rcar: Support S2RAM | expand |
Hi Kaneko-san, On Wed, Dec 6, 2017 at 7:48 PM, Yoshihiro Kaneko <ykaneko0929@gmail.com> wrote: > From: Hien Dang <hien.dang.eb@renesas.com> > > This patch adds an implementation that saves and restores the state of > GPIO configuration on suspend and resume. > > Signed-off-by: Hien Dang <hien.dang.eb@renesas.com> > Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com> > Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com> Thanks for your patch! > --- a/drivers/gpio/gpio-rcar.c > +++ b/drivers/gpio/gpio-rcar.c > @@ -31,6 +31,16 @@ > #include <linux/spinlock.h> > #include <linux/slab.h> > > +struct gpio_rcar_bank_info { > + bool iointsel; > + bool inoutsel; > + bool outdt; > + bool active_high_rising_edge; > + bool level_trigger; > + bool both; > + bool intmsk; > +}; So for each GPIO, you save 7 bools = 7 bytes. > struct gpio_rcar_priv { > void __iomem *base; > spinlock_t lock; > @@ -41,6 +51,7 @@ struct gpio_rcar_priv { > unsigned int irq_parent; > bool has_both_edge_trigger; > bool needs_clk; > + struct gpio_rcar_bank_info bank_info[32]; That's 32 x 7 = 224 bytes in total. What about just using 7 u32s instead, one for each register to save? That way you only need 7 x 4 = 28 bytes, and you can probably optimize the code to just save/restore the whole register at once. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 06, 2017 at 08:01:52PM +0100, Geert Uytterhoeven wrote: > Hi Kaneko-san, > > On Wed, Dec 6, 2017 at 7:48 PM, Yoshihiro Kaneko <ykaneko0929@gmail.com> wrote: > > From: Hien Dang <hien.dang.eb@renesas.com> > > > > This patch adds an implementation that saves and restores the state of > > GPIO configuration on suspend and resume. > > > > Signed-off-by: Hien Dang <hien.dang.eb@renesas.com> > > Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com> > > Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com> > > Thanks for your patch! > > > --- a/drivers/gpio/gpio-rcar.c > > +++ b/drivers/gpio/gpio-rcar.c > > @@ -31,6 +31,16 @@ > > #include <linux/spinlock.h> > > #include <linux/slab.h> > > > > +struct gpio_rcar_bank_info { > > + bool iointsel; > > + bool inoutsel; > > + bool outdt; > > + bool active_high_rising_edge; > > + bool level_trigger; > > + bool both; > > + bool intmsk; > > +}; > > So for each GPIO, you save 7 bools = 7 bytes. s/save/use/ ? > > struct gpio_rcar_priv { > > void __iomem *base; > > spinlock_t lock; > > @@ -41,6 +51,7 @@ struct gpio_rcar_priv { > > unsigned int irq_parent; > > bool has_both_edge_trigger; > > bool needs_clk; > > + struct gpio_rcar_bank_info bank_info[32]; > > That's 32 x 7 = 224 bytes in total. > > What about just using 7 u32s instead, one for each register to save? > That way you only need 7 x 4 = 28 bytes, and you can probably optimize > the code to just save/restore the whole register at once. So the suggestion is to use a u32 instead of struct gpio_rcar_bank_info, and for each field of struct gpio_rcar_bank_info use a bit in the u32? If so, probably one could go a step further and use a u8 as there are currently only 7 fields, thus using 32 x 1 = 32 bytes rather than 32 x 4 = 128 bytes. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Simon, On Thu, Dec 7, 2017 at 10:08 AM, Simon Horman <horms@verge.net.au> wrote: > On Wed, Dec 06, 2017 at 08:01:52PM +0100, Geert Uytterhoeven wrote: >> On Wed, Dec 6, 2017 at 7:48 PM, Yoshihiro Kaneko <ykaneko0929@gmail.com> wrote: >> > From: Hien Dang <hien.dang.eb@renesas.com> >> > >> > This patch adds an implementation that saves and restores the state of >> > GPIO configuration on suspend and resume. >> > >> > Signed-off-by: Hien Dang <hien.dang.eb@renesas.com> >> > Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com> >> > Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com> >> >> Thanks for your patch! >> >> > --- a/drivers/gpio/gpio-rcar.c >> > +++ b/drivers/gpio/gpio-rcar.c >> > @@ -31,6 +31,16 @@ >> > #include <linux/spinlock.h> >> > #include <linux/slab.h> >> > >> > +struct gpio_rcar_bank_info { >> > + bool iointsel; >> > + bool inoutsel; >> > + bool outdt; >> > + bool active_high_rising_edge; >> > + bool level_trigger; >> > + bool both; >> > + bool intmsk; >> > +}; >> >> So for each GPIO, you save 7 bools = 7 bytes. > > s/save/use/ ? Right, use 1 bool to save 1 bit ;-) >> > struct gpio_rcar_priv { >> > void __iomem *base; >> > spinlock_t lock; >> > @@ -41,6 +51,7 @@ struct gpio_rcar_priv { >> > unsigned int irq_parent; >> > bool has_both_edge_trigger; >> > bool needs_clk; >> > + struct gpio_rcar_bank_info bank_info[32]; >> >> That's 32 x 7 = 224 bytes in total. >> >> What about just using 7 u32s instead, one for each register to save? >> That way you only need 7 x 4 = 28 bytes, and you can probably optimize >> the code to just save/restore the whole register at once. > > So the suggestion is to use a u32 instead of struct gpio_rcar_bank_info, > and for each field of struct gpio_rcar_bank_info use a bit in the u32? > > If so, probably one could go a step further and use a u8 as there are > currently only 7 fields, thus using 32 x 1 = 32 bytes rather than > 32 x 4 = 128 bytes. I think you misunderstood. The patch has one gpio_rcar_bank_info for each GPIO. Each bank has 7 bits (bools), one for each register. Indexing is done through bank_info[<gpio>].<reg>. Saving/restoring bits requires converting from hardware register layout to stored layout ("transposing a 32 x 7 matrix to a 7 x 32 matrix"). I proposed 7 u32s, one for each register, storing the similar bits for all 32 GPIOs. So indexing is reversed, becoming regs[<reg>] & BIT(<gpio>), which is similar to how the data is stored in hardware registers. Storing all bits related to a single register in a single u32 may allow to save/restore all bits of the register in a single operation. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 7, 2017 at 10:21 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >>> > struct gpio_rcar_priv { >>> > void __iomem *base; >>> > spinlock_t lock; >>> > @@ -41,6 +51,7 @@ struct gpio_rcar_priv { >>> > unsigned int irq_parent; >>> > bool has_both_edge_trigger; >>> > bool needs_clk; >>> > + struct gpio_rcar_bank_info bank_info[32]; >>> >>> That's 32 x 7 = 224 bytes in total. >>> >>> What about just using 7 u32s instead, one for each register to save? >>> That way you only need 7 x 4 = 28 bytes, and you can probably optimize >>> the code to just save/restore the whole register at once. >> >> So the suggestion is to use a u32 instead of struct gpio_rcar_bank_info, >> and for each field of struct gpio_rcar_bank_info use a bit in the u32? >> >> If so, probably one could go a step further and use a u8 as there are >> currently only 7 fields, thus using 32 x 1 = 32 bytes rather than >> 32 x 4 = 128 bytes. > > I think you misunderstood. > The patch has one gpio_rcar_bank_info for each GPIO. > Each bank has 7 bits (bools), one for each register. > Indexing is done through bank_info[<gpio>].<reg>. > Saving/restoring bits requires converting from hardware register layout to > stored layout ("transposing a 32 x 7 matrix to a 7 x 32 matrix"). > > I proposed 7 u32s, one for each register, storing the similar bits for all > 32 GPIOs. > So indexing is reversed, becoming regs[<reg>] & BIT(<gpio>), which is > similar to how the data is stored in hardware registers. > Storing all bits related to a single register in a single u32 may allow to > save/restore all bits of the register in a single operation. More clarification: it's the difference between "int array[7][32]" and "int array[32][7]". Both store the same amount of data. But if the hardware uses the former organization, you want to save/restore using the same organization, else it requires an expensive transformation. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 07, 2017 at 10:26:53AM +0100, Geert Uytterhoeven wrote: > On Thu, Dec 7, 2017 at 10:21 AM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: > >>> > struct gpio_rcar_priv { > >>> > void __iomem *base; > >>> > spinlock_t lock; > >>> > @@ -41,6 +51,7 @@ struct gpio_rcar_priv { > >>> > unsigned int irq_parent; > >>> > bool has_both_edge_trigger; > >>> > bool needs_clk; > >>> > + struct gpio_rcar_bank_info bank_info[32]; > >>> > >>> That's 32 x 7 = 224 bytes in total. > >>> > >>> What about just using 7 u32s instead, one for each register to save? > >>> That way you only need 7 x 4 = 28 bytes, and you can probably optimize > >>> the code to just save/restore the whole register at once. > >> > >> So the suggestion is to use a u32 instead of struct gpio_rcar_bank_info, > >> and for each field of struct gpio_rcar_bank_info use a bit in the u32? > >> > >> If so, probably one could go a step further and use a u8 as there are > >> currently only 7 fields, thus using 32 x 1 = 32 bytes rather than > >> 32 x 4 = 128 bytes. > > > > I think you misunderstood. > > The patch has one gpio_rcar_bank_info for each GPIO. > > Each bank has 7 bits (bools), one for each register. > > Indexing is done through bank_info[<gpio>].<reg>. > > Saving/restoring bits requires converting from hardware register layout to > > stored layout ("transposing a 32 x 7 matrix to a 7 x 32 matrix"). > > > > I proposed 7 u32s, one for each register, storing the similar bits for all > > 32 GPIOs. > > So indexing is reversed, becoming regs[<reg>] & BIT(<gpio>), which is > > similar to how the data is stored in hardware registers. > > Storing all bits related to a single register in a single u32 may allow to > > save/restore all bits of the register in a single operation. > > More clarification: it's the difference between "int array[7][32]" and > "int array[32][7]". Both store the same amount of data. > But if the hardware uses the former organization, you want to > save/restore using the same organization, else it requires an expensive > transformation. Thanks, you are correct that I misunderstood. I understand now. Kaneko-san, could you take a look at switching this around and posting an RFT? -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Simon-san, Geert-san, I am sorry for my late reply. 2017-12-07 19:27 GMT+09:00 Simon Horman <horms@verge.net.au>: > On Thu, Dec 07, 2017 at 10:26:53AM +0100, Geert Uytterhoeven wrote: >> On Thu, Dec 7, 2017 at 10:21 AM, Geert Uytterhoeven >> <geert@linux-m68k.org> wrote: >> >>> > struct gpio_rcar_priv { >> >>> > void __iomem *base; >> >>> > spinlock_t lock; >> >>> > @@ -41,6 +51,7 @@ struct gpio_rcar_priv { >> >>> > unsigned int irq_parent; >> >>> > bool has_both_edge_trigger; >> >>> > bool needs_clk; >> >>> > + struct gpio_rcar_bank_info bank_info[32]; >> >>> >> >>> That's 32 x 7 = 224 bytes in total. >> >>> >> >>> What about just using 7 u32s instead, one for each register to save? >> >>> That way you only need 7 x 4 = 28 bytes, and you can probably optimize >> >>> the code to just save/restore the whole register at once. >> >> >> >> So the suggestion is to use a u32 instead of struct gpio_rcar_bank_info, >> >> and for each field of struct gpio_rcar_bank_info use a bit in the u32? >> >> >> >> If so, probably one could go a step further and use a u8 as there are >> >> currently only 7 fields, thus using 32 x 1 = 32 bytes rather than >> >> 32 x 4 = 128 bytes. >> > >> > I think you misunderstood. >> > The patch has one gpio_rcar_bank_info for each GPIO. >> > Each bank has 7 bits (bools), one for each register. >> > Indexing is done through bank_info[<gpio>].<reg>. >> > Saving/restoring bits requires converting from hardware register layout to >> > stored layout ("transposing a 32 x 7 matrix to a 7 x 32 matrix"). >> > >> > I proposed 7 u32s, one for each register, storing the similar bits for all >> > 32 GPIOs. >> > So indexing is reversed, becoming regs[<reg>] & BIT(<gpio>), which is >> > similar to how the data is stored in hardware registers. >> > Storing all bits related to a single register in a single u32 may allow to >> > save/restore all bits of the register in a single operation. >> >> More clarification: it's the difference between "int array[7][32]" and >> "int array[32][7]". Both store the same amount of data. >> But if the hardware uses the former organization, you want to >> save/restore using the same organization, else it requires an expensive >> transformation. > > Thanks, you are correct that I misunderstood. > I understand now. > > Kaneko-san, could you take a look at switching this around and posting an RFT? Sure, will do. Thanks, Kaneko -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" 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/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c index e76de57..c99a2c5 100644 --- a/drivers/gpio/gpio-rcar.c +++ b/drivers/gpio/gpio-rcar.c @@ -31,6 +31,16 @@ #include <linux/spinlock.h> #include <linux/slab.h> +struct gpio_rcar_bank_info { + bool iointsel; + bool inoutsel; + bool outdt; + bool active_high_rising_edge; + bool level_trigger; + bool both; + bool intmsk; +}; + struct gpio_rcar_priv { void __iomem *base; spinlock_t lock; @@ -41,6 +51,7 @@ struct gpio_rcar_priv { unsigned int irq_parent; bool has_both_edge_trigger; bool needs_clk; + struct gpio_rcar_bank_info bank_info[32]; }; #define IOINTSEL 0x00 /* General IO/Interrupt Switching Register */ @@ -415,6 +426,83 @@ static int gpio_rcar_parse_dt(struct gpio_rcar_priv *p, unsigned int *npins) return 0; } +#ifdef CONFIG_PM_SLEEP +static int gpio_rcar_suspend(struct device *dev) +{ + struct gpio_rcar_priv *p = dev_get_drvdata(dev); + int offset; + u32 bit_mask; + struct gpio_rcar_bank_info *bank_info; + + for (offset = 0; offset < p->gpio_chip.ngpio; offset++) { + bank_info = &p->bank_info[offset]; + bit_mask = BIT(offset); + bank_info->iointsel = !!(gpio_rcar_read(p, IOINTSEL) & + bit_mask); + + /* I/O pin */ + if (!bank_info->iointsel) { + bank_info->inoutsel = !!(gpio_rcar_read(p, INOUTSEL) & + bit_mask); + bank_info->outdt = !!(gpio_rcar_read(p, OUTDT) & + bit_mask); + /* Interrupt pin */ + } else { + bank_info->intmsk = !!(gpio_rcar_read(p, INTMSK) & + bit_mask); + bank_info->active_high_rising_edge = + !(!!(gpio_rcar_read(p, POSNEG) & bit_mask)); + bank_info->level_trigger = + !(!!(gpio_rcar_read(p, EDGLEVEL) & bit_mask)); + bank_info->both = !!(gpio_rcar_read(p, BOTHEDGE) & + bit_mask); + } + } + + return 0; +} + +static int gpio_rcar_resume(struct device *dev) +{ + struct gpio_rcar_priv *p = dev_get_drvdata(dev); + int offset; + struct gpio_rcar_bank_info *bank_info; + + for (offset = 0; offset < p->gpio_chip.ngpio; offset++) { + bank_info = &p->bank_info[offset]; + /* I/O pin */ + if (!bank_info->iointsel) { + if (bank_info->inoutsel) + gpio_rcar_direction_output(&p->gpio_chip, + offset, + bank_info->outdt); + else + gpio_rcar_direction_input(&p->gpio_chip, + offset); + /* Interrupt pin */ + } else { + gpio_rcar_config_interrupt_input_mode( + p, + offset, + bank_info->active_high_rising_edge, + bank_info->level_trigger, + bank_info->both); + + if (bank_info->intmsk) + gpio_rcar_write(p, MSKCLR, BIT(offset)); + } + } + + return 0; +} + +static SIMPLE_DEV_PM_OPS(gpio_rcar_pm_ops, + gpio_rcar_suspend, gpio_rcar_resume); +#define DEV_PM_OPS (&gpio_rcar_pm_ops) +#else +#define DEV_PM_OPS NULL +#endif /* CONFIG_PM_SLEEP*/ + static int gpio_rcar_probe(struct platform_device *pdev) { struct gpio_rcar_priv *p; @@ -536,6 +624,7 @@ static int gpio_rcar_remove(struct platform_device *pdev) .remove = gpio_rcar_remove, .driver = { .name = "gpio_rcar", + .pm = DEV_PM_OPS, .of_match_table = of_match_ptr(gpio_rcar_of_table), } };