From: Claudiu Beznea <[email protected]>
pinctrl-rzg2l driver is used on RZ/G3S which support deep sleep states
where power to most of the SoC components is turned off.
For this add suspend/resume support. This involves saving and restoring
configured registers along with disabling clock in case there is no pin
configured as wakeup sources.
To save/restore registers 2 caches were allocated: one for GPIO pins and
one for dedicated pins.
On suspend path the pin controller registers are saved and if none of the
pins are configured as wakeup sources the pinctrl clock is disabled.
Otherwise it remains on.
On resume path the configuration is done as follows:
1/ setup PFCs by writing to registers on pin based accesses
2/ setup GPIOs by writing to registers on port based accesses and
following configuration steps specified in hardware manual
3/ setup dedicated pins by writing to registers on port based accesses
4/ setup interrupts.
Because interrupt signals are routed to IA55 interrupt controller and
IA55 interrupt controller resumes before pin controller, patch restores
also the configured interrupts just after pin settings are restored to
avoid invalid interrupts while resuming.
Signed-off-by: Claudiu Beznea <[email protected]>
---
drivers/pinctrl/renesas/pinctrl-rzg2l.c | 397 +++++++++++++++++++++++-
1 file changed, 393 insertions(+), 4 deletions(-)
diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
index 818dccdd70da..09d9b448d819 100644
--- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
@@ -149,6 +149,33 @@
#define RZG2L_TINT_IRQ_START_INDEX 9
#define RZG2L_PACK_HWIRQ(t, i) (((t) << 16) | (i))
+/* Read/write 8 bits register */
+#define RZG2L_PCTRL_REG_ACCESS8(_read, _addr, _val) \
+ do { \
+ if (_read) \
+ _val = readb(_addr); \
+ else \
+ writeb(_val, _addr); \
+ } while (0)
+
+/* Read/write 16 bits register */
+#define RZG2L_PCTRL_REG_ACCESS16(_read, _addr, _val) \
+ do { \
+ if (_read) \
+ _val = readw(_addr); \
+ else \
+ writew(_val, _addr); \
+ } while (0)
+
+/* Read/write 32 bits register */
+#define RZG2L_PCTRL_REG_ACCESS32(_read, _addr, _val) \
+ do { \
+ if (_read) \
+ _val = readl(_addr); \
+ else \
+ writel(_val, _addr); \
+ } while (0)
+
/**
* struct rzg2l_register_offsets - specific register offsets
* @pwpr: PWPR register offset
@@ -241,6 +268,32 @@ struct rzg2l_pinctrl_pin_settings {
u16 drive_strength_ua;
};
+/**
+ * struct rzg2l_pinctrl_reg_cache - register cache structure (to be used in suspend/resume)
+ * @p: P registers cache
+ * @pm: PM registers cache
+ * @pmc: PMC registers cache
+ * @pfc: PFC registers cache
+ * @iolh: IOLH registers cache
+ * @ien: IEN registers cache
+ * @sd_ch: SD_CH registers cache
+ * @eth_poc: ET_POC registers cache
+ * @eth_mode: ETH_MODE register cache
+ * @qspi: QSPI registers cache
+ */
+struct rzg2l_pinctrl_reg_cache {
+ u8 *p;
+ u16 *pm;
+ u8 *pmc;
+ u32 *pfc;
+ u32 *iolh[2];
+ u32 *ien[2];
+ u32 sd_ch[2];
+ u32 eth_poc[2];
+ u32 eth_mode;
+ u32 qspi;
+};
+
struct rzg2l_pinctrl {
struct pinctrl_dev *pctl;
struct pinctrl_desc desc;
@@ -250,6 +303,8 @@ struct rzg2l_pinctrl {
void __iomem *base;
struct device *dev;
+ struct clk *clk;
+
struct gpio_chip gpio_chip;
struct pinctrl_gpio_range gpio_range;
DECLARE_BITMAP(tint_slot, RZG2L_TINT_MAX_INTERRUPT);
@@ -260,6 +315,9 @@ struct rzg2l_pinctrl {
struct mutex mutex; /* serialize adding groups and functions */
struct rzg2l_pinctrl_pin_settings *settings;
+ struct rzg2l_pinctrl_reg_cache *cache;
+ struct rzg2l_pinctrl_reg_cache *dedicated_cache;
+ atomic_t wakeup_source;
};
static const u16 available_ps[] = { 1800, 2500, 3300 };
@@ -1880,6 +1938,19 @@ static void rzg2l_gpio_irq_print_chip(struct irq_data *data, struct seq_file *p)
seq_printf(p, dev_name(gc->parent));
}
+static int rzg2l_gpio_irq_set_wake(struct irq_data *data, unsigned int on)
+{
+ struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+ struct rzg2l_pinctrl *pctrl = container_of(gc, struct rzg2l_pinctrl, gpio_chip);
+
+ if (on)
+ atomic_inc(&pctrl->wakeup_source);
+ else
+ atomic_dec(&pctrl->wakeup_source);
+
+ return 0;
+}
+
static const struct irq_chip rzg2l_gpio_irqchip = {
.name = "rzg2l-gpio",
.irq_disable = rzg2l_gpio_irq_disable,
@@ -1890,6 +1961,7 @@ static const struct irq_chip rzg2l_gpio_irqchip = {
.irq_eoi = rzg2l_gpio_irqc_eoi,
.irq_print_chip = rzg2l_gpio_irq_print_chip,
.irq_set_affinity = irq_chip_set_affinity_parent,
+ .irq_set_wake = rzg2l_gpio_irq_set_wake,
.flags = IRQCHIP_IMMUTABLE,
GPIOCHIP_IRQ_RESOURCE_HELPERS,
};
@@ -1937,6 +2009,35 @@ static int rzg2l_gpio_populate_parent_fwspec(struct gpio_chip *chip,
return 0;
}
+static void rzg2l_gpio_irq_restore(struct rzg2l_pinctrl *pctrl)
+{
+ struct irq_domain *domain = pctrl->gpio_chip.irq.domain;
+
+ for (unsigned int i = 0; i < RZG2L_TINT_MAX_INTERRUPT; i++) {
+ struct irq_data *data;
+ unsigned int virq;
+
+ if (!pctrl->hwirq[i])
+ continue;
+
+ virq = irq_find_mapping(domain, pctrl->hwirq[i]);
+ if (!virq) {
+ dev_crit(pctrl->dev, "Failed to find IRQ mapping for hwirq %u\n",
+ pctrl->hwirq[i]);
+ continue;
+ }
+
+ data = irq_domain_get_irq_data(domain, virq);
+ if (!data) {
+ dev_crit(pctrl->dev, "Failed to get IRQ data for virq=%u\n", virq);
+ continue;
+ }
+
+ if (!irqd_irq_disabled(data))
+ rzg2l_gpio_irq_enable(data);
+ }
+}
+
static void rzg2l_gpio_irq_domain_free(struct irq_domain *domain, unsigned int virq,
unsigned int nr_irqs)
{
@@ -1985,6 +2086,68 @@ static void rzg2l_init_irq_valid_mask(struct gpio_chip *gc,
}
}
+static int rzg2l_pinctrl_reg_cache_alloc(struct rzg2l_pinctrl *pctrl)
+{
+ u32 nports = pctrl->data->n_port_pins / RZG2L_PINS_PER_PORT;
+ struct rzg2l_pinctrl_reg_cache *cache, *dedicated_cache;
+
+ cache = devm_kzalloc(pctrl->dev, sizeof(*cache), GFP_KERNEL);
+ if (!cache)
+ return -ENOMEM;
+
+ dedicated_cache = devm_kzalloc(pctrl->dev, sizeof(*dedicated_cache), GFP_KERNEL);
+ if (!dedicated_cache)
+ return -ENOMEM;
+
+ cache->p = devm_kcalloc(pctrl->dev, nports, sizeof(*cache->p), GFP_KERNEL);
+ if (!cache->p)
+ return -ENOMEM;
+
+ cache->pm = devm_kcalloc(pctrl->dev, nports, sizeof(*cache->pm), GFP_KERNEL);
+ if (!cache->pm)
+ return -ENOMEM;
+
+ cache->pmc = devm_kcalloc(pctrl->dev, nports, sizeof(*cache->pmc), GFP_KERNEL);
+ if (!cache->pmc)
+ return -ENOMEM;
+
+ cache->pfc = devm_kcalloc(pctrl->dev, nports, sizeof(*cache->pfc), GFP_KERNEL);
+ if (!cache->pfc)
+ return -ENOMEM;
+
+ for (u8 i = 0; i < 2; i++) {
+ u32 n_dedicated_pins = pctrl->data->n_dedicated_pins;
+
+ cache->iolh[i] = devm_kcalloc(pctrl->dev, nports, sizeof(*cache->iolh[i]),
+ GFP_KERNEL);
+ if (!cache->iolh[i])
+ return -ENOMEM;
+
+ cache->ien[i] = devm_kcalloc(pctrl->dev, nports, sizeof(*cache->ien[i]),
+ GFP_KERNEL);
+ if (!cache->ien[i])
+ return -ENOMEM;
+
+ /* Allocate dedicated cache. */
+ dedicated_cache->iolh[i] = devm_kcalloc(pctrl->dev, n_dedicated_pins,
+ sizeof(*dedicated_cache->iolh[i]),
+ GFP_KERNEL);
+ if (!dedicated_cache->iolh[i])
+ return -ENOMEM;
+
+ dedicated_cache->ien[i] = devm_kcalloc(pctrl->dev, n_dedicated_pins,
+ sizeof(*dedicated_cache->ien[i]),
+ GFP_KERNEL);
+ if (!dedicated_cache->ien[i])
+ return -ENOMEM;
+ }
+
+ pctrl->cache = cache;
+ pctrl->dedicated_cache = dedicated_cache;
+
+ return 0;
+}
+
static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl)
{
struct device_node *np = pctrl->dev->of_node;
@@ -2125,6 +2288,10 @@ static int rzg2l_pinctrl_register(struct rzg2l_pinctrl *pctrl)
}
}
+ ret = rzg2l_pinctrl_reg_cache_alloc(pctrl);
+ if (ret)
+ return ret;
+
ret = devm_pinctrl_register_and_init(pctrl->dev, &pctrl->desc, pctrl,
&pctrl->pctl);
if (ret) {
@@ -2150,7 +2317,6 @@ static int rzg2l_pinctrl_register(struct rzg2l_pinctrl *pctrl)
static int rzg2l_pinctrl_probe(struct platform_device *pdev)
{
struct rzg2l_pinctrl *pctrl;
- struct clk *clk;
int ret;
BUILD_BUG_ON(ARRAY_SIZE(r9a07g044_gpio_configs) * RZG2L_PINS_PER_PORT >
@@ -2176,14 +2342,16 @@ static int rzg2l_pinctrl_probe(struct platform_device *pdev)
if (IS_ERR(pctrl->base))
return PTR_ERR(pctrl->base);
- clk = devm_clk_get_enabled(pctrl->dev, NULL);
- if (IS_ERR(clk))
- return dev_err_probe(pctrl->dev, PTR_ERR(clk),
+ pctrl->clk = devm_clk_get_enabled(pctrl->dev, NULL);
+ if (IS_ERR(pctrl->clk)) {
+ return dev_err_probe(pctrl->dev, PTR_ERR(pctrl->clk),
"failed to enable GPIO clk\n");
+ }
spin_lock_init(&pctrl->lock);
spin_lock_init(&pctrl->bitmap_lock);
mutex_init(&pctrl->mutex);
+ atomic_set(&pctrl->wakeup_source, 0);
platform_set_drvdata(pdev, pctrl);
@@ -2195,6 +2363,222 @@ static int rzg2l_pinctrl_probe(struct platform_device *pdev)
return 0;
}
+static void rzg2l_pinctrl_pm_setup_regs(struct rzg2l_pinctrl *pctrl, bool suspend)
+{
+ u32 nports = pctrl->data->n_port_pins / RZG2L_PINS_PER_PORT;
+ struct rzg2l_pinctrl_reg_cache *cache = pctrl->cache;
+
+ for (u32 port = 0; port < nports; port++) {
+ bool has_iolh, has_ien;
+ u32 off, caps;
+ u8 pincnt;
+ u64 cfg;
+
+ cfg = pctrl->data->port_pin_configs[port];
+ off = RZG2L_PIN_CFG_TO_PORT_OFFSET(cfg);
+ pincnt = hweight8(FIELD_GET(PIN_CFG_PIN_MAP_MASK, cfg));
+
+ caps = FIELD_GET(PIN_CFG_MASK, cfg);
+ has_iolh = !!(caps & (PIN_CFG_IOLH_A | PIN_CFG_IOLH_B | PIN_CFG_IOLH_C));
+ has_ien = !!(caps & PIN_CFG_IEN);
+
+ if (suspend)
+ RZG2L_PCTRL_REG_ACCESS32(suspend, pctrl->base + PFC(off), cache->pfc[port]);
+
+ /*
+ * Now cache the registers or set them in the order suggested by
+ * HW manual (section "Operation for GPIO Function").
+ */
+ RZG2L_PCTRL_REG_ACCESS8(suspend, pctrl->base + PMC(off), cache->pmc[port]);
+ if (has_iolh) {
+ RZG2L_PCTRL_REG_ACCESS32(suspend, pctrl->base + IOLH(off),
+ cache->iolh[0][port]);
+ if (pincnt >= 4) {
+ RZG2L_PCTRL_REG_ACCESS32(suspend, pctrl->base + IOLH(off) + 4,
+ cache->iolh[1][port]);
+ }
+ }
+
+ RZG2L_PCTRL_REG_ACCESS16(suspend, pctrl->base + PM(off), cache->pm[port]);
+ RZG2L_PCTRL_REG_ACCESS8(suspend, pctrl->base + P(off), cache->p[port]);
+
+ if (has_ien) {
+ RZG2L_PCTRL_REG_ACCESS32(suspend, pctrl->base + IEN(off),
+ cache->ien[0][port]);
+ if (pincnt >= 4) {
+ RZG2L_PCTRL_REG_ACCESS32(suspend, pctrl->base + IEN(off) + 4,
+ cache->ien[1][port]);
+ }
+ }
+ }
+}
+
+static void rzg2l_pinctrl_pm_setup_dedicated_regs(struct rzg2l_pinctrl *pctrl, bool suspend)
+{
+ struct rzg2l_pinctrl_reg_cache *cache = pctrl->dedicated_cache;
+
+ /*
+ * Make sure entries in pctrl->data->n_dedicated_pins[] having the same
+ * port offset are close together.
+ */
+ for (u32 i = 0, caps = 0; i < pctrl->data->n_dedicated_pins; i++) {
+ bool has_iolh, has_ien;
+ u32 off, next_off = 0;
+ u64 cfg, next_cfg;
+ u8 pincnt;
+
+ cfg = pctrl->data->dedicated_pins[i].config;
+ off = RZG2L_PIN_CFG_TO_PORT_OFFSET(cfg);
+ if (i + 1 < pctrl->data->n_dedicated_pins) {
+ next_cfg = pctrl->data->dedicated_pins[i + 1].config;
+ next_off = RZG2L_PIN_CFG_TO_PORT_OFFSET(next_cfg);
+ }
+
+ if (off == next_off) {
+ /* Gather caps of all port pins. */
+ caps |= FIELD_GET(PIN_CFG_MASK, cfg);
+ continue;
+ }
+
+ /* And apply them in a single shot. */
+ has_iolh = !!(caps & (PIN_CFG_IOLH_A | PIN_CFG_IOLH_B | PIN_CFG_IOLH_C));
+ has_ien = !!(caps & PIN_CFG_IEN);
+ pincnt = hweight8(FIELD_GET(RZG2L_SINGLE_PIN_BITS_MASK, cfg));
+
+ if (has_iolh) {
+ RZG2L_PCTRL_REG_ACCESS32(suspend, pctrl->base + IOLH(off),
+ cache->iolh[0][i]);
+ }
+ if (has_ien) {
+ RZG2L_PCTRL_REG_ACCESS32(suspend, pctrl->base + IEN(off),
+ cache->ien[0][i]);
+ }
+
+ if (pincnt >= 4) {
+ if (has_iolh) {
+ RZG2L_PCTRL_REG_ACCESS32(suspend,
+ pctrl->base + IOLH(off) + 4,
+ cache->iolh[1][i]);
+ }
+ if (has_ien) {
+ RZG2L_PCTRL_REG_ACCESS32(suspend,
+ pctrl->base + IEN(off) + 4,
+ cache->ien[1][i]);
+ }
+ }
+ caps = 0;
+ }
+}
+
+static void rzg2l_pinctrl_pm_setup_pfc(struct rzg2l_pinctrl *pctrl)
+{
+ u32 nports = pctrl->data->n_port_pins / RZG2L_PINS_PER_PORT;
+ const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg;
+ const struct rzg2l_register_offsets *regs = &hwcfg->regs;
+
+ /* Set the PWPR register to allow PFC register to write. */
+ writel(0x0, pctrl->base + regs->pwpr); /* B0WI=0, PFCWE=0 */
+ writel(PWPR_PFCWE, pctrl->base + regs->pwpr); /* B0WI=0, PFCWE=1 */
+
+ /* Restore port registers. */
+ for (u32 port = 0; port < nports; port++) {
+ unsigned long pinmap;
+ u8 pmc = 0, max_pin;
+ u32 off, pfc = 0;
+ u64 cfg;
+ u16 pm;
+ u8 pin;
+
+ cfg = pctrl->data->port_pin_configs[port];
+ off = RZG2L_PIN_CFG_TO_PORT_OFFSET(cfg);
+ pinmap = FIELD_GET(PIN_CFG_PIN_MAP_MASK, cfg);
+ max_pin = fls(pinmap);
+
+ pm = readw(pctrl->base + PM(off));
+ for_each_set_bit(pin, &pinmap, max_pin) {
+ struct rzg2l_pinctrl_reg_cache *cache = pctrl->cache;
+
+ /* Nothing to do if PFC was not configured before. */
+ if (!(cache->pmc[port] & BIT(pin)))
+ continue;
+
+ /* Set pin to 'Non-use (Hi-Z input protection)' */
+ pm &= ~(PM_MASK << (pin * 2));
+ writew(pm, pctrl->base + PM(off));
+
+ /* Temporarily switch to GPIO mode with PMC register */
+ pmc &= ~BIT(pin);
+ writeb(pmc, pctrl->base + PMC(off));
+
+ /* Select Pin function mode. */
+ pfc &= ~(PFC_MASK << (pin * 4));
+ pfc |= (cache->pfc[port] & (PFC_MASK << (pin * 4)));
+ writel(pfc, pctrl->base + PFC(off));
+
+ /* Switch to Peripheral pin function. */
+ pmc |= BIT(pin);
+ writeb(pmc, pctrl->base + PMC(off));
+ }
+ }
+
+ /* Set the PWPR register to be write-protected. */
+ writel(0x0, pctrl->base + regs->pwpr); /* B0WI=0, PFCWE=0 */
+ writel(PWPR_B0WI, pctrl->base + regs->pwpr); /* B0WI=1, PFCWE=0 */
+}
+
+static int rzg2l_pinctrl_suspend_noirq(struct device *dev)
+{
+ struct rzg2l_pinctrl *pctrl = dev_get_drvdata(dev);
+ const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg;
+ const struct rzg2l_register_offsets *regs = &hwcfg->regs;
+ struct rzg2l_pinctrl_reg_cache *cache = pctrl->cache;
+
+ rzg2l_pinctrl_pm_setup_regs(pctrl, true);
+ rzg2l_pinctrl_pm_setup_dedicated_regs(pctrl, true);
+
+ for (u8 i = 0; i < 2; i++) {
+ cache->sd_ch[i] = readl(pctrl->base + SD_CH(regs->sd_ch, i));
+ cache->eth_poc[i] = readl(pctrl->base + ETH_POC(regs->eth_poc, i));
+ }
+
+ cache->qspi = readl(pctrl->base + QSPI);
+ cache->eth_mode = readl(pctrl->base + ETH_MODE);
+
+ if (!atomic_read(&pctrl->wakeup_source))
+ clk_disable_unprepare(pctrl->clk);
+
+ return 0;
+}
+
+static int rzg2l_pinctrl_resume_noirq(struct device *dev)
+{
+ struct rzg2l_pinctrl *pctrl = dev_get_drvdata(dev);
+ const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg;
+ const struct rzg2l_register_offsets *regs = &hwcfg->regs;
+ struct rzg2l_pinctrl_reg_cache *cache = pctrl->cache;
+ int ret;
+
+ if (!atomic_read(&pctrl->wakeup_source)) {
+ ret = clk_prepare_enable(pctrl->clk);
+ if (ret)
+ return ret;
+ }
+
+ writel(cache->qspi, pctrl->base + QSPI);
+ writel(cache->eth_mode, pctrl->base + ETH_MODE);
+ for (u8 i = 0; i < 2; i++) {
+ writel(cache->sd_ch[i], pctrl->base + SD_CH(regs->sd_ch, i));
+ writel(cache->eth_poc[i], pctrl->base + ETH_POC(regs->eth_poc, i));
+ }
+
+ rzg2l_pinctrl_pm_setup_pfc(pctrl);
+ rzg2l_pinctrl_pm_setup_regs(pctrl, false);
+ rzg2l_pinctrl_pm_setup_dedicated_regs(pctrl, false);
+ rzg2l_gpio_irq_restore(pctrl);
+
+ return 0;
+}
+
static const struct rzg2l_hwcfg rzg2l_hwcfg = {
.regs = {
.pwpr = 0x3014,
@@ -2291,10 +2675,15 @@ static const struct of_device_id rzg2l_pinctrl_of_table[] = {
{ /* sentinel */ }
};
+static const struct dev_pm_ops rzg2l_pinctrl_pm_ops = {
+ NOIRQ_SYSTEM_SLEEP_PM_OPS(rzg2l_pinctrl_suspend_noirq, rzg2l_pinctrl_resume_noirq)
+};
+
static struct platform_driver rzg2l_pinctrl_driver = {
.driver = {
.name = DRV_NAME,
.of_match_table = of_match_ptr(rzg2l_pinctrl_of_table),
+ .pm = pm_sleep_ptr(&rzg2l_pinctrl_pm_ops),
},
.probe = rzg2l_pinctrl_probe,
};
--
2.39.2
On 08.02.2024 15:56, Claudiu wrote:
> From: Claudiu Beznea <[email protected]>
>
> pinctrl-rzg2l driver is used on RZ/G3S which support deep sleep states
> where power to most of the SoC components is turned off.
>
> For this add suspend/resume support. This involves saving and restoring
> configured registers along with disabling clock in case there is no pin
> configured as wakeup sources.
>
> To save/restore registers 2 caches were allocated: one for GPIO pins and
> one for dedicated pins.
>
> On suspend path the pin controller registers are saved and if none of the
> pins are configured as wakeup sources the pinctrl clock is disabled.
> Otherwise it remains on.
>
> On resume path the configuration is done as follows:
> 1/ setup PFCs by writing to registers on pin based accesses
> 2/ setup GPIOs by writing to registers on port based accesses and
> following configuration steps specified in hardware manual
> 3/ setup dedicated pins by writing to registers on port based accesses
> 4/ setup interrupts.
>
> Because interrupt signals are routed to IA55 interrupt controller and
> IA55 interrupt controller resumes before pin controller, patch restores
> also the configured interrupts just after pin settings are restored to
> avoid invalid interrupts while resuming.
>
> Signed-off-by: Claudiu Beznea <[email protected]>
> ---
[ ... ]
>
> +/**
> + * struct rzg2l_pinctrl_reg_cache - register cache structure (to be used in suspend/resume)
> + * @p: P registers cache
> + * @pm: PM registers cache
> + * @pmc: PMC registers cache
> + * @pfc: PFC registers cache
> + * @iolh: IOLH registers cache
> + * @ien: IEN registers cache
> + * @sd_ch: SD_CH registers cache
> + * @eth_poc: ET_POC registers cache
> + * @eth_mode: ETH_MODE register cache
> + * @qspi: QSPI registers cache
> + */
> +struct rzg2l_pinctrl_reg_cache {
> + u8 *p;
> + u16 *pm;
> + u8 *pmc;
> + u32 *pfc;
> + u32 *iolh[2];
> + u32 *ien[2];
> + u32 sd_ch[2];
> + u32 eth_poc[2];
> + u32 eth_mode;
> + u32 qspi;
I missed it, u8 should be enough for these.
[ ... ]
Hi Claudiu,
On Thu, Feb 8, 2024 at 6:59 PM Claudiu <[email protected]> wrote:
> From: Claudiu Beznea <[email protected]>
>
> pinctrl-rzg2l driver is used on RZ/G3S which support deep sleep states
> where power to most of the SoC components is turned off.
>
> For this add suspend/resume support. This involves saving and restoring
> configured registers along with disabling clock in case there is no pin
> configured as wakeup sources.
>
> To save/restore registers 2 caches were allocated: one for GPIO pins and
> one for dedicated pins.
>
> On suspend path the pin controller registers are saved and if none of the
> pins are configured as wakeup sources the pinctrl clock is disabled.
> Otherwise it remains on.
>
> On resume path the configuration is done as follows:
> 1/ setup PFCs by writing to registers on pin based accesses
> 2/ setup GPIOs by writing to registers on port based accesses and
> following configuration steps specified in hardware manual
> 3/ setup dedicated pins by writing to registers on port based accesses
> 4/ setup interrupts.
>
> Because interrupt signals are routed to IA55 interrupt controller and
> IA55 interrupt controller resumes before pin controller, patch restores
> also the configured interrupts just after pin settings are restored to
> avoid invalid interrupts while resuming.
>
> Signed-off-by: Claudiu Beznea <[email protected]>
Thanks for your patch!
In my review below, I am focussing on the wake-up part, as that is
usually the hardest part to get right.
> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -260,6 +315,9 @@ struct rzg2l_pinctrl {
> struct mutex mutex; /* serialize adding groups and functions */
>
> struct rzg2l_pinctrl_pin_settings *settings;
> + struct rzg2l_pinctrl_reg_cache *cache;
> + struct rzg2l_pinctrl_reg_cache *dedicated_cache;
> + atomic_t wakeup_source;
I'd call this wakeup_path, as the wake-up source is the ultimate device
that triggers the GPIO.
> };
>
> static const u16 available_ps[] = { 1800, 2500, 3300 };
> @@ -1880,6 +1938,19 @@ static void rzg2l_gpio_irq_print_chip(struct irq_data *data, struct seq_file *p)
> seq_printf(p, dev_name(gc->parent));
> }
>
> +static int rzg2l_gpio_irq_set_wake(struct irq_data *data, unsigned int on)
> +{
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
> + struct rzg2l_pinctrl *pctrl = container_of(gc, struct rzg2l_pinctrl, gpio_chip);
> +
I think you also have to call irq_set_irq_wake(pctrl->hwirq[...]) here.
Cfr. drivers/gpio/gpio-rcar.c (which is simpler, as it has a single interrupt
parent, instead of a parent irq_domain with multiple interrupts).
> + if (on)
> + atomic_inc(&pctrl->wakeup_source);
> + else
> + atomic_dec(&pctrl->wakeup_source);
> +
> + return 0;
> +}
> +
> static const struct irq_chip rzg2l_gpio_irqchip = {
> .name = "rzg2l-gpio",
> .irq_disable = rzg2l_gpio_irq_disable,
> +static int rzg2l_pinctrl_suspend_noirq(struct device *dev)
> +{
> + struct rzg2l_pinctrl *pctrl = dev_get_drvdata(dev);
> + const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg;
> + const struct rzg2l_register_offsets *regs = &hwcfg->regs;
> + struct rzg2l_pinctrl_reg_cache *cache = pctrl->cache;
> +
> + rzg2l_pinctrl_pm_setup_regs(pctrl, true);
> + rzg2l_pinctrl_pm_setup_dedicated_regs(pctrl, true);
> +
> + for (u8 i = 0; i < 2; i++) {
> + cache->sd_ch[i] = readl(pctrl->base + SD_CH(regs->sd_ch, i));
> + cache->eth_poc[i] = readl(pctrl->base + ETH_POC(regs->eth_poc, i));
> + }
> +
> + cache->qspi = readl(pctrl->base + QSPI);
> + cache->eth_mode = readl(pctrl->base + ETH_MODE);
> +
> + if (!atomic_read(&pctrl->wakeup_source))
> + clk_disable_unprepare(pctrl->clk);
While you handle the module clock yourself, I think there is still merit
in calling device_set_wakeup_path(dev) when the clock is kept enabled.
BTW, is there any need to save the registers when pinctrl is part of
the wake-up path, and its module clock is not disabled?
> +
> + return 0;
> +}
> +
> +static int rzg2l_pinctrl_resume_noirq(struct device *dev)
> +{
> + struct rzg2l_pinctrl *pctrl = dev_get_drvdata(dev);
> + const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg;
> + const struct rzg2l_register_offsets *regs = &hwcfg->regs;
> + struct rzg2l_pinctrl_reg_cache *cache = pctrl->cache;
> + int ret;
> +
> + if (!atomic_read(&pctrl->wakeup_source)) {
> + ret = clk_prepare_enable(pctrl->clk);
> + if (ret)
> + return ret;
> + }
Is there any need to restore the registers when pinctrl is part of
the wake-up path, and its module clock was not disabled?
> +
> + writel(cache->qspi, pctrl->base + QSPI);
> + writel(cache->eth_mode, pctrl->base + ETH_MODE);
> + for (u8 i = 0; i < 2; i++) {
> + writel(cache->sd_ch[i], pctrl->base + SD_CH(regs->sd_ch, i));
> + writel(cache->eth_poc[i], pctrl->base + ETH_POC(regs->eth_poc, i));
> + }
> +
> + rzg2l_pinctrl_pm_setup_pfc(pctrl);
> + rzg2l_pinctrl_pm_setup_regs(pctrl, false);
> + rzg2l_pinctrl_pm_setup_dedicated_regs(pctrl, false);
> + rzg2l_gpio_irq_restore(pctrl);
> +
> + return 0;
> +}
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg
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
Hi, Geert,
On 12.02.2024 17:06, Geert Uytterhoeven wrote:
> Hi Claudiu,
>
> On Thu, Feb 8, 2024 at 6:59 PM Claudiu <[email protected]> wrote:
>> From: Claudiu Beznea <[email protected]>
>>
>> pinctrl-rzg2l driver is used on RZ/G3S which support deep sleep states
>> where power to most of the SoC components is turned off.
>>
>> For this add suspend/resume support. This involves saving and restoring
>> configured registers along with disabling clock in case there is no pin
>> configured as wakeup sources.
>>
>> To save/restore registers 2 caches were allocated: one for GPIO pins and
>> one for dedicated pins.
>>
>> On suspend path the pin controller registers are saved and if none of the
>> pins are configured as wakeup sources the pinctrl clock is disabled.
>> Otherwise it remains on.
>>
>> On resume path the configuration is done as follows:
>> 1/ setup PFCs by writing to registers on pin based accesses
>> 2/ setup GPIOs by writing to registers on port based accesses and
>> following configuration steps specified in hardware manual
>> 3/ setup dedicated pins by writing to registers on port based accesses
>> 4/ setup interrupts.
>>
>> Because interrupt signals are routed to IA55 interrupt controller and
>> IA55 interrupt controller resumes before pin controller, patch restores
>> also the configured interrupts just after pin settings are restored to
>> avoid invalid interrupts while resuming.
>>
>> Signed-off-by: Claudiu Beznea <[email protected]>
>
> Thanks for your patch!
>
> In my review below, I am focussing on the wake-up part, as that is
> usually the hardest part to get right.
>
>> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
>> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
>> @@ -260,6 +315,9 @@ struct rzg2l_pinctrl {
>> struct mutex mutex; /* serialize adding groups and functions */
>>
>> struct rzg2l_pinctrl_pin_settings *settings;
>> + struct rzg2l_pinctrl_reg_cache *cache;
>> + struct rzg2l_pinctrl_reg_cache *dedicated_cache;
>> + atomic_t wakeup_source;
>
> I'd call this wakeup_path, as the wake-up source is the ultimate device
> that triggers the GPIO.
OK!
>
>> };
>>
>> static const u16 available_ps[] = { 1800, 2500, 3300 };
>> @@ -1880,6 +1938,19 @@ static void rzg2l_gpio_irq_print_chip(struct irq_data *data, struct seq_file *p)
>> seq_printf(p, dev_name(gc->parent));
>> }
>>
>> +static int rzg2l_gpio_irq_set_wake(struct irq_data *data, unsigned int on)
>> +{
>> + struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
>> + struct rzg2l_pinctrl *pctrl = container_of(gc, struct rzg2l_pinctrl, gpio_chip);
>> +
>
> I think you also have to call irq_set_irq_wake(pctrl->hwirq[...]) here.
> Cfr. drivers/gpio/gpio-rcar.c (which is simpler, as it has a single interrupt
> parent, instead of a parent irq_domain with multiple interrupts).
I had it in my initial implementation (done long time ago) but I don't
remember why I removed it. I'll re-add it anyway.
>
>> + if (on)
>> + atomic_inc(&pctrl->wakeup_source);
>> + else
>> + atomic_dec(&pctrl->wakeup_source);
>> +
>> + return 0;
>> +}
>> +
>> static const struct irq_chip rzg2l_gpio_irqchip = {
>> .name = "rzg2l-gpio",
>> .irq_disable = rzg2l_gpio_irq_disable,
>
>
>> +static int rzg2l_pinctrl_suspend_noirq(struct device *dev)
>> +{
>> + struct rzg2l_pinctrl *pctrl = dev_get_drvdata(dev);
>> + const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg;
>> + const struct rzg2l_register_offsets *regs = &hwcfg->regs;
>> + struct rzg2l_pinctrl_reg_cache *cache = pctrl->cache;
>> +
>> + rzg2l_pinctrl_pm_setup_regs(pctrl, true);
>> + rzg2l_pinctrl_pm_setup_dedicated_regs(pctrl, true);
>> +
>> + for (u8 i = 0; i < 2; i++) {
>> + cache->sd_ch[i] = readl(pctrl->base + SD_CH(regs->sd_ch, i));
>> + cache->eth_poc[i] = readl(pctrl->base + ETH_POC(regs->eth_poc, i));
>> + }
>> +
>> + cache->qspi = readl(pctrl->base + QSPI);
>> + cache->eth_mode = readl(pctrl->base + ETH_MODE);
>> +
>> + if (!atomic_read(&pctrl->wakeup_source))
>> + clk_disable_unprepare(pctrl->clk);
>
> While you handle the module clock yourself, I think there is still merit
> in calling device_set_wakeup_path(dev) when the clock is kept enabled.
Ok, I'll explore it.
>
> BTW, is there any need to save the registers when pinctrl is part of
> the wake-up path, and its module clock is not disabled?
Yes, for scenarios where the pinctrl is part of the wake-up path but the
system is going a deep sleep state where pinctrl registers will be lost anyway.
Same for the resume path.
Thank you for your review,
Claudiu Beznea
>
>> +
>> + return 0;
>> +}
>> +
>> +static int rzg2l_pinctrl_resume_noirq(struct device *dev)
>> +{
>> + struct rzg2l_pinctrl *pctrl = dev_get_drvdata(dev);
>> + const struct rzg2l_hwcfg *hwcfg = pctrl->data->hwcfg;
>> + const struct rzg2l_register_offsets *regs = &hwcfg->regs;
>> + struct rzg2l_pinctrl_reg_cache *cache = pctrl->cache;
>> + int ret;
>> +
>> + if (!atomic_read(&pctrl->wakeup_source)) {
>> + ret = clk_prepare_enable(pctrl->clk);
>> + if (ret)
>> + return ret;
>> + }
>
> Is there any need to restore the registers when pinctrl is part of
> the wake-up path, and its module clock was not disabled?
>
>> +
>> + writel(cache->qspi, pctrl->base + QSPI);
>> + writel(cache->eth_mode, pctrl->base + ETH_MODE);
>> + for (u8 i = 0; i < 2; i++) {
>> + writel(cache->sd_ch[i], pctrl->base + SD_CH(regs->sd_ch, i));
>> + writel(cache->eth_poc[i], pctrl->base + ETH_POC(regs->eth_poc, i));
>> + }
>> +
>> + rzg2l_pinctrl_pm_setup_pfc(pctrl);
>> + rzg2l_pinctrl_pm_setup_regs(pctrl, false);
>> + rzg2l_pinctrl_pm_setup_dedicated_regs(pctrl, false);
>> + rzg2l_gpio_irq_restore(pctrl);
>> +
>> + return 0;
>> +}
>
> Gr{oetje,eeting}s,
>
> Geert
>
Hi, Geert,
On 12.02.2024 17:35, claudiu beznea wrote:
>>> static const u16 available_ps[] = { 1800, 2500, 3300 };
>>> @@ -1880,6 +1938,19 @@ static void rzg2l_gpio_irq_print_chip(struct irq_data *data, struct seq_file *p)
>>> seq_printf(p, dev_name(gc->parent));
>>> }
>>>
>>> +static int rzg2l_gpio_irq_set_wake(struct irq_data *data, unsigned int on)
>>> +{
>>> + struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
>>> + struct rzg2l_pinctrl *pctrl = container_of(gc, struct rzg2l_pinctrl, gpio_chip);
>>> +
>> I think you also have to call irq_set_irq_wake(pctrl->hwirq[...]) here.
>> Cfr. drivers/gpio/gpio-rcar.c (which is simpler, as it has a single interrupt
>> parent, instead of a parent irq_domain with multiple interrupts).
> I had it in my initial implementation (done long time ago) but I don't
> remember why I removed it. I'll re-add it anyway.
I did some investigation on this. It seems adding irq_set_irq_wake() is not
necessary as the pinctrl has no virq requested on behalf of itself.
With this irqchip hierarchy (pinctrl-rzg2l -> irq-renesas-rzg2l -> gic) if
an IRQ consumer, e.g., the gpio-keys, request an interrupt then it may call
irq_set_irq_wake(virq) (gpio-keys does that).
irq_set_irq_wake(virq) is forwarded to pinctrl as follows:
irq_set_irq_wake(virq, on) ->
set_irq_wake_real(virq, ono) ->
rzg2l_gpio_irq_set_wake(irq, on)
As the irq_set_irq_wake() gets a virq as argument and as we have no virq
requested by pinctrl driver there is no need to call irq_set_irq_wake(), as
of my investigation. Calling it with hwirq will return with -22 and calling
it with virq received as argument leads to deadlock (as it's the same virq
that consumer already is configuring with irq_set_irq_wake()) due the
following line from irq_set_irq_wake():
struct irq_desc *desc = irq_get_desc_buslock(irq, &flags,
IRQ_GET_DESC_CHECK_GLOBAL);
What we can do is to forward irq_set_wake() to the parent IRQ chip
(irq-renesas-rzg2l) with irq_chip_set_wake_parent() to let him set its
wakeup_path, if any. But, at the moment the irq-renesas-rzg2l has
IRQCHIP_SKIP_SET_WAKE thus the irq_chip_set_wake_parent() does nothing (but
it can be updated for that). Now I remember that irq_chip_set_wake_parent()
is what I've called in my initial implementation and removed it due to
IRQCHIP_SKIP_SET_WAKE.
Please let me know if you are OK to add irq_chip_set_wake_parent() and
update the irq-renesas-rzg2l driver.
Thank you,
Claudiu Beznea
Hi Claudiu,
On Thu, Feb 15, 2024 at 10:15 AM claudiu beznea
<[email protected]> wrote:
> On 12.02.2024 17:35, claudiu beznea wrote:
> >>> static const u16 available_ps[] = { 1800, 2500, 3300 };
> >>> @@ -1880,6 +1938,19 @@ static void rzg2l_gpio_irq_print_chip(struct irq_data *data, struct seq_file *p)
> >>> seq_printf(p, dev_name(gc->parent));
> >>> }
> >>>
> >>> +static int rzg2l_gpio_irq_set_wake(struct irq_data *data, unsigned int on)
> >>> +{
> >>> + struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
> >>> + struct rzg2l_pinctrl *pctrl = container_of(gc, struct rzg2l_pinctrl, gpio_chip);
> >>> +
> >> I think you also have to call irq_set_irq_wake(pctrl->hwirq[...]) here.
> >> Cfr. drivers/gpio/gpio-rcar.c (which is simpler, as it has a single interrupt
> >> parent, instead of a parent irq_domain with multiple interrupts).
> > I had it in my initial implementation (done long time ago) but I don't
> > remember why I removed it. I'll re-add it anyway.
>
> I did some investigation on this. It seems adding irq_set_irq_wake() is not
> necessary as the pinctrl has no virq requested on behalf of itself.
>
> With this irqchip hierarchy (pinctrl-rzg2l -> irq-renesas-rzg2l -> gic) if
> an IRQ consumer, e.g., the gpio-keys, request an interrupt then it may call
> irq_set_irq_wake(virq) (gpio-keys does that).
>
> irq_set_irq_wake(virq) is forwarded to pinctrl as follows:
>
> irq_set_irq_wake(virq, on) ->
> set_irq_wake_real(virq, ono) ->
> rzg2l_gpio_irq_set_wake(irq, on)
>
> As the irq_set_irq_wake() gets a virq as argument and as we have no virq
> requested by pinctrl driver there is no need to call irq_set_irq_wake(), as
> of my investigation. Calling it with hwirq will return with -22 and calling
> it with virq received as argument leads to deadlock (as it's the same virq
> that consumer already is configuring with irq_set_irq_wake()) due the
> following line from irq_set_irq_wake():
>
> struct irq_desc *desc = irq_get_desc_buslock(irq, &flags,
> IRQ_GET_DESC_CHECK_GLOBAL);
>
> What we can do is to forward irq_set_wake() to the parent IRQ chip
> (irq-renesas-rzg2l) with irq_chip_set_wake_parent() to let him set its
> wakeup_path, if any. But, at the moment the irq-renesas-rzg2l has
> IRQCHIP_SKIP_SET_WAKE thus the irq_chip_set_wake_parent() does nothing (but
> it can be updated for that). Now I remember that irq_chip_set_wake_parent()
> is what I've called in my initial implementation and removed it due to
> IRQCHIP_SKIP_SET_WAKE.
>
> Please let me know if you are OK to add irq_chip_set_wake_parent() and
> update the irq-renesas-rzg2l driver.
I think calling irq_chip_set_wake_parent() regardless is a good thing
to do. Whether the irq-renesas-rzg2l needs an update for wake-up
handling, I don't know (and that is orthogonal to the above).
If you haven't already done so, you may want to browse the wake-related
git history of e.g. drivers/gpio/gpio-rcar.c.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg
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