diff mbox series

pinctrl: stm32: improve bank clocks management

Message ID 20220422143608.226580-1-fabien.dessenne@foss.st.com
State New
Headers show
Series pinctrl: stm32: improve bank clocks management | expand

Commit Message

Fabien Dessenne April 22, 2022, 2:36 p.m. UTC 
Instead of enabling/disabling the clock at each IO configuration update,
just keep the clock enabled from the probe.
This makes things simpler and more efficient (e.g. the time required to
toggle an output IO is drastically decreased) without significantly
increasing the power consumption.

Signed-off-by: Fabien Dessenne <fabien.dessenne@foss.st.com>
---
 drivers/pinctrl/stm32/pinctrl-stm32.c      | 82 ++++++++--------------
 drivers/pinctrl/stm32/pinctrl-stm32.h      |  1 +
 drivers/pinctrl/stm32/pinctrl-stm32mp135.c |  2 +-
 drivers/pinctrl/stm32/pinctrl-stm32mp157.c |  2 +-
 4 files changed, 34 insertions(+), 53 deletions(-)

Comments

Marek Vasut April 22, 2022, 4:26 p.m. UTC | #1 
On 4/22/22 16:36, Fabien Dessenne wrote:
> Instead of enabling/disabling the clock at each IO configuration update,
> just keep the clock enabled from the probe.
> This makes things simpler and more efficient (e.g. the time required to
> toggle an output IO is drastically decreased) without significantly
> increasing the power consumption.

[...]

>   static struct irq_domain *stm32_pctrl_get_irq_domain(struct device_node *np)
> @@ -1575,6 +1537,10 @@ int stm32_pctl_probe(struct platform_device *pdev)
>   			ret = stm32_gpiolib_register_bank(pctl, child);
>   			if (ret) {
>   				of_node_put(child);
> +
> +				for (i = 0; i < pctl->nbanks; i++)
> +					clk_disable_unprepare(pctl->banks[i].clk);
> +

There are clk_bulk_*() functions, maybe you can use those to get rid of 
these loops ?

The rest looks good to me.
Fabien Dessenne April 25, 2022, 9:27 a.m. UTC | #2 
Hi Marek


On 22/04/2022 18:26, Marek Vasut wrote:
> On 4/22/22 16:36, Fabien Dessenne wrote:
>> Instead of enabling/disabling the clock at each IO configuration update,
>> just keep the clock enabled from the probe.
>> This makes things simpler and more efficient (e.g. the time required to
>> toggle an output IO is drastically decreased) without significantly
>> increasing the power consumption.
> 
> [...]
> 
>>   static struct irq_domain *stm32_pctrl_get_irq_domain(struct 
>> device_node *np)
>> @@ -1575,6 +1537,10 @@ int stm32_pctl_probe(struct platform_device *pdev)
>>               ret = stm32_gpiolib_register_bank(pctl, child);
>>               if (ret) {
>>                   of_node_put(child);
>> +
>> +                for (i = 0; i < pctl->nbanks; i++)
>> +                    clk_disable_unprepare(pctl->banks[i].clk);
>> +
> 
> There are clk_bulk_*() functions, maybe you can use those to get rid of 
> these loops ?

This sounds goods, but checking more in details I see that moving to the 
'bulk' implementation would require to move the clk information from the 
"struct stm32_gpio_bank *banks" member to its parent "struct stm32_pinctrl".

This would make the clk device information stored in a different 
structure from the other device-related information (base address, reset 
control, ...). It's better to keep all those information together in the 
same struct.

As another drawback we would loose access to 'clk' from any function 
that have 'bank' (or 'struct gpio_chip *chip') as input parameter (e.g. 
stm32_gpio_get() called from gpiolib).

So I really prefer to keep the current implementation.

BR
Fabien

> 
> The rest looks good to me.
Marek Vasut April 25, 2022, 10:07 p.m. UTC | #3 
On 4/25/22 11:27, Fabien DESSENNE wrote:
> Hi Marek
> 
> 
> On 22/04/2022 18:26, Marek Vasut wrote:
>> On 4/22/22 16:36, Fabien Dessenne wrote:
>>> Instead of enabling/disabling the clock at each IO configuration update,
>>> just keep the clock enabled from the probe.
>>> This makes things simpler and more efficient (e.g. the time required to
>>> toggle an output IO is drastically decreased) without significantly
>>> increasing the power consumption.
>>
>> [...]
>>
>>>   static struct irq_domain *stm32_pctrl_get_irq_domain(struct 
>>> device_node *np)
>>> @@ -1575,6 +1537,10 @@ int stm32_pctl_probe(struct platform_device 
>>> *pdev)
>>>               ret = stm32_gpiolib_register_bank(pctl, child);
>>>               if (ret) {
>>>                   of_node_put(child);
>>> +
>>> +                for (i = 0; i < pctl->nbanks; i++)
>>> +                    clk_disable_unprepare(pctl->banks[i].clk);
>>> +
>>
>> There are clk_bulk_*() functions, maybe you can use those to get rid 
>> of these loops ?
> 
> This sounds goods, but checking more in details I see that moving to the 
> 'bulk' implementation would require to move the clk information from the 
> "struct stm32_gpio_bank *banks" member to its parent "struct 
> stm32_pinctrl".
> 
> This would make the clk device information stored in a different 
> structure from the other device-related information (base address, reset 
> control, ...). It's better to keep all those information together in the 
> same struct.
> 
> As another drawback we would loose access to 'clk' from any function 
> that have 'bank' (or 'struct gpio_chip *chip') as input parameter (e.g. 
> stm32_gpio_get() called from gpiolib).
> 
> So I really prefer to keep the current implementation.

All right, I agree.

>> The rest looks good to me.

Reviewed-by: Marek Vasut <marex@denx.de>
Linus Walleij May 1, 2022, 9:26 p.m. UTC | #4 
On Fri, Apr 22, 2022 at 4:36 PM Fabien Dessenne
<fabien.dessenne@foss.st.com> wrote:

> Instead of enabling/disabling the clock at each IO configuration update,
> just keep the clock enabled from the probe.
> This makes things simpler and more efficient (e.g. the time required to
> toggle an output IO is drastically decreased) without significantly
> increasing the power consumption.
>
> Signed-off-by: Fabien Dessenne <fabien.dessenne@foss.st.com>

Patch applied after merging in v5.18-rc5 as a prerequisite.

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
index f7c9459f6628..b308e7bb7487 100644
--- a/drivers/pinctrl/stm32/pinctrl-stm32.c
+++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
@@ -197,11 +197,7 @@  static inline void __stm32_gpio_set(struct stm32_gpio_bank *bank,
 	if (!value)
 		offset += STM32_GPIO_PINS_PER_BANK;
 
-	clk_enable(bank->clk);
-
 	writel_relaxed(BIT(offset), bank->base + STM32_GPIO_BSRR);
-
-	clk_disable(bank->clk);
 }
 
 static int stm32_gpio_request(struct gpio_chip *chip, unsigned offset)
@@ -225,25 +221,11 @@  static void stm32_gpio_free(struct gpio_chip *chip, unsigned offset)
 	pinctrl_gpio_free(chip->base + offset);
 }
 
-static int stm32_gpio_get_noclk(struct gpio_chip *chip, unsigned int offset)
-{
-	struct stm32_gpio_bank *bank = gpiochip_get_data(chip);
-
-	return !!(readl_relaxed(bank->base + STM32_GPIO_IDR) & BIT(offset));
-}
-
 static int stm32_gpio_get(struct gpio_chip *chip, unsigned offset)
 {
 	struct stm32_gpio_bank *bank = gpiochip_get_data(chip);
-	int ret;
 
-	clk_enable(bank->clk);
-
-	ret = stm32_gpio_get_noclk(chip, offset);
-
-	clk_disable(bank->clk);
-
-	return ret;
+	return !!(readl_relaxed(bank->base + STM32_GPIO_IDR) & BIT(offset));
 }
 
 static void stm32_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
@@ -323,7 +305,7 @@  static void stm32_gpio_irq_trigger(struct irq_data *d)
 		return;
 
 	/* If level interrupt type then retrig */
-	level = stm32_gpio_get_noclk(&bank->gpio_chip, d->hwirq);
+	level = stm32_gpio_get(&bank->gpio_chip, d->hwirq);
 	if ((level == 0 && bank->irq_type[d->hwirq] == IRQ_TYPE_LEVEL_LOW) ||
 	    (level == 1 && bank->irq_type[d->hwirq] == IRQ_TYPE_LEVEL_HIGH))
 		irq_chip_retrigger_hierarchy(d);
@@ -365,7 +347,6 @@  static int stm32_gpio_irq_request_resources(struct irq_data *irq_data)
 {
 	struct stm32_gpio_bank *bank = irq_data->domain->host_data;
 	struct stm32_pinctrl *pctl = dev_get_drvdata(bank->gpio_chip.parent);
-	unsigned long flags;
 	int ret;
 
 	ret = stm32_gpio_direction_input(&bank->gpio_chip, irq_data->hwirq);
@@ -379,10 +360,6 @@  static int stm32_gpio_irq_request_resources(struct irq_data *irq_data)
 		return ret;
 	}
 
-	flags = irqd_get_trigger_type(irq_data);
-	if (flags & IRQ_TYPE_LEVEL_MASK)
-		clk_enable(bank->clk);
-
 	return 0;
 }
 
@@ -390,9 +367,6 @@  static void stm32_gpio_irq_release_resources(struct irq_data *irq_data)
 {
 	struct stm32_gpio_bank *bank = irq_data->domain->host_data;
 
-	if (bank->irq_type[irq_data->hwirq] & IRQ_TYPE_LEVEL_MASK)
-		clk_disable(bank->clk);
-
 	gpiochip_unlock_as_irq(&bank->gpio_chip, irq_data->hwirq);
 }
 
@@ -769,7 +743,6 @@  static int stm32_pmx_set_mode(struct stm32_gpio_bank *bank,
 	unsigned long flags;
 	int err = 0;
 
-	clk_enable(bank->clk);
 	spin_lock_irqsave(&bank->lock, flags);
 
 	if (pctl->hwlock) {
@@ -798,7 +771,6 @@  static int stm32_pmx_set_mode(struct stm32_gpio_bank *bank,
 
 unlock:
 	spin_unlock_irqrestore(&bank->lock, flags);
-	clk_disable(bank->clk);
 
 	return err;
 }
@@ -811,7 +783,6 @@  void stm32_pmx_get_mode(struct stm32_gpio_bank *bank, int pin, u32 *mode,
 	int alt_offset = STM32_GPIO_AFRL + (pin / 8) * 4;
 	unsigned long flags;
 
-	clk_enable(bank->clk);
 	spin_lock_irqsave(&bank->lock, flags);
 
 	val = readl_relaxed(bank->base + alt_offset);
@@ -823,7 +794,6 @@  void stm32_pmx_get_mode(struct stm32_gpio_bank *bank, int pin, u32 *mode,
 	*mode = val >> (pin * 2);
 
 	spin_unlock_irqrestore(&bank->lock, flags);
-	clk_disable(bank->clk);
 }
 
 static int stm32_pmx_set_mux(struct pinctrl_dev *pctldev,
@@ -886,7 +856,6 @@  static int stm32_pconf_set_driving(struct stm32_gpio_bank *bank,
 	u32 val;
 	int err = 0;
 
-	clk_enable(bank->clk);
 	spin_lock_irqsave(&bank->lock, flags);
 
 	if (pctl->hwlock) {
@@ -910,7 +879,6 @@  static int stm32_pconf_set_driving(struct stm32_gpio_bank *bank,
 
 unlock:
 	spin_unlock_irqrestore(&bank->lock, flags);
-	clk_disable(bank->clk);
 
 	return err;
 }
@@ -921,14 +889,12 @@  static u32 stm32_pconf_get_driving(struct stm32_gpio_bank *bank,
 	unsigned long flags;
 	u32 val;
 
-	clk_enable(bank->clk);
 	spin_lock_irqsave(&bank->lock, flags);
 
 	val = readl_relaxed(bank->base + STM32_GPIO_TYPER);
 	val &= BIT(offset);
 
 	spin_unlock_irqrestore(&bank->lock, flags);
-	clk_disable(bank->clk);
 
 	return (val >> offset);
 }
@@ -941,7 +907,6 @@  static int stm32_pconf_set_speed(struct stm32_gpio_bank *bank,
 	u32 val;
 	int err = 0;
 
-	clk_enable(bank->clk);
 	spin_lock_irqsave(&bank->lock, flags);
 
 	if (pctl->hwlock) {
@@ -965,7 +930,6 @@  static int stm32_pconf_set_speed(struct stm32_gpio_bank *bank,
 
 unlock:
 	spin_unlock_irqrestore(&bank->lock, flags);
-	clk_disable(bank->clk);
 
 	return err;
 }
@@ -976,14 +940,12 @@  static u32 stm32_pconf_get_speed(struct stm32_gpio_bank *bank,
 	unsigned long flags;
 	u32 val;
 
-	clk_enable(bank->clk);
 	spin_lock_irqsave(&bank->lock, flags);
 
 	val = readl_relaxed(bank->base + STM32_GPIO_SPEEDR);
 	val &= GENMASK(offset * 2 + 1, offset * 2);
 
 	spin_unlock_irqrestore(&bank->lock, flags);
-	clk_disable(bank->clk);
 
 	return (val >> (offset * 2));
 }
@@ -996,7 +958,6 @@  static int stm32_pconf_set_bias(struct stm32_gpio_bank *bank,
 	u32 val;
 	int err = 0;
 
-	clk_enable(bank->clk);
 	spin_lock_irqsave(&bank->lock, flags);
 
 	if (pctl->hwlock) {
@@ -1020,7 +981,6 @@  static int stm32_pconf_set_bias(struct stm32_gpio_bank *bank,
 
 unlock:
 	spin_unlock_irqrestore(&bank->lock, flags);
-	clk_disable(bank->clk);
 
 	return err;
 }
@@ -1031,14 +991,12 @@  static u32 stm32_pconf_get_bias(struct stm32_gpio_bank *bank,
 	unsigned long flags;
 	u32 val;
 
-	clk_enable(bank->clk);
 	spin_lock_irqsave(&bank->lock, flags);
 
 	val = readl_relaxed(bank->base + STM32_GPIO_PUPDR);
 	val &= GENMASK(offset * 2 + 1, offset * 2);
 
 	spin_unlock_irqrestore(&bank->lock, flags);
-	clk_disable(bank->clk);
 
 	return (val >> (offset * 2));
 }
@@ -1049,7 +1007,6 @@  static bool stm32_pconf_get(struct stm32_gpio_bank *bank,
 	unsigned long flags;
 	u32 val;
 
-	clk_enable(bank->clk);
 	spin_lock_irqsave(&bank->lock, flags);
 
 	if (dir)
@@ -1060,7 +1017,6 @@  static bool stm32_pconf_get(struct stm32_gpio_bank *bank,
 			 BIT(offset));
 
 	spin_unlock_irqrestore(&bank->lock, flags);
-	clk_disable(bank->clk);
 
 	return val;
 }
@@ -1256,9 +1212,9 @@  static int stm32_gpiolib_register_bank(struct stm32_pinctrl *pctl,
 	if (IS_ERR(bank->base))
 		return PTR_ERR(bank->base);
 
-	err = clk_prepare(bank->clk);
+	err = clk_prepare_enable(bank->clk);
 	if (err) {
-		dev_err(dev, "failed to prepare clk (%d)\n", err);
+		dev_err(dev, "failed to prepare_enable clk (%d)\n", err);
 		return err;
 	}
 
@@ -1306,17 +1262,23 @@  static int stm32_gpiolib_register_bank(struct stm32_pinctrl *pctl,
 					STM32_GPIO_IRQ_LINE, bank->fwnode,
 					&stm32_gpio_domain_ops, bank);
 
-	if (!bank->domain)
-		return -ENODEV;
+	if (!bank->domain) {
+		err = -ENODEV;
+		goto err_clk;
+	}
 
 	err = gpiochip_add_data(&bank->gpio_chip, bank);
 	if (err) {
 		dev_err(dev, "Failed to add gpiochip(%d)!\n", bank_nr);
-		return err;
+		goto err_clk;
 	}
 
 	dev_info(dev, "%s bank added\n", bank->gpio_chip.label);
 	return 0;
+
+err_clk:
+	clk_disable_unprepare(bank->clk);
+	return err;
 }
 
 static struct irq_domain *stm32_pctrl_get_irq_domain(struct device_node *np)
@@ -1575,6 +1537,10 @@  int stm32_pctl_probe(struct platform_device *pdev)
 			ret = stm32_gpiolib_register_bank(pctl, child);
 			if (ret) {
 				of_node_put(child);
+
+				for (i = 0; i < pctl->nbanks; i++)
+					clk_disable_unprepare(pctl->banks[i].clk);
+
 				return ret;
 			}
 
@@ -1647,12 +1613,26 @@  static int __maybe_unused stm32_pinctrl_restore_gpio_regs(
 	return 0;
 }
 
+int __maybe_unused stm32_pinctrl_suspend(struct device *dev)
+{
+	struct stm32_pinctrl *pctl = dev_get_drvdata(dev);
+	int i;
+
+	for (i = 0; i < pctl->nbanks; i++)
+		clk_disable(pctl->banks[i].clk);
+
+	return 0;
+}
+
 int __maybe_unused stm32_pinctrl_resume(struct device *dev)
 {
 	struct stm32_pinctrl *pctl = dev_get_drvdata(dev);
 	struct stm32_pinctrl_group *g = pctl->groups;
 	int i;
 
+	for (i = 0; i < pctl->nbanks; i++)
+		clk_enable(pctl->banks[i].clk);
+
 	for (i = 0; i < pctl->ngroups; i++, g++)
 		stm32_pinctrl_restore_gpio_regs(pctl, g->pin);
 
diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.h b/drivers/pinctrl/stm32/pinctrl-stm32.h
index b0882d120765..b9584039cdf5 100644
--- a/drivers/pinctrl/stm32/pinctrl-stm32.h
+++ b/drivers/pinctrl/stm32/pinctrl-stm32.h
@@ -65,6 +65,7 @@  struct stm32_gpio_bank;
 int stm32_pctl_probe(struct platform_device *pdev);
 void stm32_pmx_get_mode(struct stm32_gpio_bank *bank,
 			int pin, u32 *mode, u32 *alt);
+int stm32_pinctrl_suspend(struct device *dev);
 int stm32_pinctrl_resume(struct device *dev);
 
 #endif /* __PINCTRL_STM32_H */
diff --git a/drivers/pinctrl/stm32/pinctrl-stm32mp135.c b/drivers/pinctrl/stm32/pinctrl-stm32mp135.c
index 4ab03520c407..f98717fe23ed 100644
--- a/drivers/pinctrl/stm32/pinctrl-stm32mp135.c
+++ b/drivers/pinctrl/stm32/pinctrl-stm32mp135.c
@@ -1660,7 +1660,7 @@  static const struct of_device_id stm32mp135_pctrl_match[] = {
 };
 
 static const struct dev_pm_ops stm32_pinctrl_dev_pm_ops = {
-	 SET_LATE_SYSTEM_SLEEP_PM_OPS(NULL, stm32_pinctrl_resume)
+	 SET_LATE_SYSTEM_SLEEP_PM_OPS(stm32_pinctrl_suspend, stm32_pinctrl_resume)
 };
 
 static struct platform_driver stm32mp135_pinctrl_driver = {
diff --git a/drivers/pinctrl/stm32/pinctrl-stm32mp157.c b/drivers/pinctrl/stm32/pinctrl-stm32mp157.c
index 2ccb99d64df8..91b2fc8ddbdb 100644
--- a/drivers/pinctrl/stm32/pinctrl-stm32mp157.c
+++ b/drivers/pinctrl/stm32/pinctrl-stm32mp157.c
@@ -2343,7 +2343,7 @@  static const struct of_device_id stm32mp157_pctrl_match[] = {
 };
 
 static const struct dev_pm_ops stm32_pinctrl_dev_pm_ops = {
-	 SET_LATE_SYSTEM_SLEEP_PM_OPS(NULL, stm32_pinctrl_resume)
+	 SET_LATE_SYSTEM_SLEEP_PM_OPS(stm32_pinctrl_suspend, stm32_pinctrl_resume)
 };
 
 static struct platform_driver stm32mp157_pinctrl_driver = {