* Re: [PATCH 0/3] ARM: dts: aspeed: Deprecate g[45]-style compatibles
From: Joel Stanley @ 2019-08-02 6:15 UTC (permalink / raw)
To: Andrew Jeffery
Cc: Linus Walleij, linux-aspeed, Lee Jones, Rob Herring, Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, linux-kernel@vger.kernel.org, open list:GPIO SUBSYSTEM
In-Reply-To: <3691f6cb-2451-43f7-9f00-d5693071ba59@www.fastmail.com>
On Thu, 1 Aug 2019 at 05:45, Andrew Jeffery <andrew@aj.id.au> wrote:
>
>
>
> On Tue, 30 Jul 2019, at 10:27, Andrew Jeffery wrote:
> >
> >
> > On Tue, 30 Jul 2019, at 07:23, Linus Walleij wrote:
> > > On Wed, Jul 24, 2019 at 10:13 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> > >
> > > > It's probably best if we push the three patches all through one tree rather
> > > > than fragmenting. Is everyone happy if Joel applies them to the aspeed tree?
> > >
> > > If you are sure it will not collide with parallell work in the
> > > pinctrl tree, yes.
> > > Acked-by: Linus Walleij <linus.walleij@linaro.org>
> > >
> > > (If it does collide I'd prefer to take the pinctrl patches and fix the
> > > conflicts in my tree.)
> >
> > Fair enough, I don't know the answer so I'll poke around. I don't
> > really mind
> > where the series goes in, I just want to avoid landing only part of it
> > if I split it up.
>
> Okay, it currently conflicts with my cleanup-devicetree-warnings series.
>
> Joel, do you mind if Linus takes this series through the pinctrl tree, given
> the fix to the devicetrees is patch 1/3?
It depends if you plan more changes to that part of the device tree
this merge window :)
Linus, perhaps the safer option is for me to take 1/3 through my tree
and you can take the rest through yours?
Cheers,
Joel
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: pinctrl: qcom: Add SC7180 pinctrl binding
From: Vinod Koul @ 2019-08-02 6:33 UTC (permalink / raw)
To: Rajendra Nayak
Cc: linus.walleij, bjorn.andersson, linux-arm-msm, agross, robh+dt,
linux-gpio, devicetree, linux-kernel, Jitendra Sharma,
Vivek Gautam
In-Reply-To: <20190802041507.12365-1-rnayak@codeaurora.org>
On 02-08-19, 09:45, Rajendra Nayak wrote:
> From: Jitendra Sharma <shajit@codeaurora.org>
>
> Add the binding for the TLMM pinctrl block found in the SC7180 platform
>
> Signed-off-by: Jitendra Sharma <shajit@codeaurora.org>
> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
> [rnayak: Fix some copy-paste issues, sort and fix functions]
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
changes since v1: ..?
> +- reg-names:
> + Usage: required
> + Value type: <prop-encoded-array>
> + Defintiion: names for the cells of reg, must contain "north", "south"
s/Defintiion/Definition
> +Example:
> +
> + tlmm: pinctrl@3000000 {
this should be: pinctrl@3500000
with these two nitpicks fixed:
Reviewed-by: Vinod Koul <vkoul@kernel.org>
--
~Vinod
^ permalink raw reply
* Re: [PATCH v2 2/2] pinctrl: qcom: Add SC7180 pinctrl driver
From: Vinod Koul @ 2019-08-02 6:34 UTC (permalink / raw)
To: Rajendra Nayak
Cc: linus.walleij, bjorn.andersson, linux-arm-msm, agross, robh+dt,
linux-gpio, devicetree, linux-kernel, Jitendra Sharma,
Vivek Gautam
In-Reply-To: <20190802041507.12365-2-rnayak@codeaurora.org>
On 02-08-19, 09:45, Rajendra Nayak wrote:
> From: Jitendra Sharma <shajit@codeaurora.org>
>
> Add initial pinctrl driver to support pin configuration with
> pinctrl framework for SC7180
Reviewed-by: Vinod Koul <vkoul@kernel.org>
--
~Vinod
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: pinctrl: qcom: Add SC7180 pinctrl binding
From: Rajendra Nayak @ 2019-08-02 6:53 UTC (permalink / raw)
To: Vinod Koul
Cc: linus.walleij, bjorn.andersson, linux-arm-msm, agross, robh+dt,
linux-gpio, devicetree, linux-kernel, Jitendra Sharma,
Vivek Gautam
In-Reply-To: <20190802063317.GB12733@vkoul-mobl.Dlink>
On 8/2/2019 12:03 PM, Vinod Koul wrote:
> On 02-08-19, 09:45, Rajendra Nayak wrote:
>> From: Jitendra Sharma <shajit@codeaurora.org>
>>
>> Add the binding for the TLMM pinctrl block found in the SC7180 platform
>>
>> Signed-off-by: Jitendra Sharma <shajit@codeaurora.org>
>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
>> [rnayak: Fix some copy-paste issues, sort and fix functions]
>> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
>> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>> ---
>
> changes since v1: ..?
>
>> +- reg-names:
>> + Usage: required
>> + Value type: <prop-encoded-array>
>> + Defintiion: names for the cells of reg, must contain "north", "south"
>
> s/Defintiion/Definition
>
>> +Example:
>> +
>> + tlmm: pinctrl@3000000 {
>
> this should be: pinctrl@3500000
>
> with these two nitpicks fixed:
Thanks Vinod for the review. I will fix these and respin, after I wait
a while to see if there is any more feedback :)
>
> Reviewed-by: Vinod Koul <vkoul@kernel.org>
>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply
* Re: [PATCH] gpio: mxc: Use devm_clk_get_optional instead of devm_clk_get
From: Bartosz Golaszewski @ 2019-08-02 7:06 UTC (permalink / raw)
To: Anson Huang; +Cc: Linus Walleij, linux-gpio, LKML, dl-linux-imx
In-Reply-To: <20190801084439.36487-1-Anson.Huang@nxp.com>
czw., 1 sie 2019 o 10:54 <Anson.Huang@nxp.com> napisał(a):
>
> From: Anson Huang <Anson.Huang@nxp.com>
>
> i.MX SoC's GPIO clock is optional, so it is better to use
> devm_clk_get_optional instead of devm_clk_get.
>
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
> drivers/gpio/gpio-mxc.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
> index b281358..7907a87 100644
> --- a/drivers/gpio/gpio-mxc.c
> +++ b/drivers/gpio/gpio-mxc.c
> @@ -435,12 +435,9 @@ static int mxc_gpio_probe(struct platform_device *pdev)
> return port->irq;
>
> /* the controller clock is optional */
> - port->clk = devm_clk_get(&pdev->dev, NULL);
> - if (IS_ERR(port->clk)) {
> - if (PTR_ERR(port->clk) == -EPROBE_DEFER)
> - return -EPROBE_DEFER;
> - port->clk = NULL;
> - }
> + port->clk = devm_clk_get_optional(&pdev->dev, NULL);
> + if (IS_ERR(port->clk))
> + return PTR_ERR(port->clk);
>
> err = clk_prepare_enable(port->clk);
> if (err) {
> --
> 2.7.4
>
Patch applied, thanks!
Bart
^ permalink raw reply
* Re: [PATCH 05/14] gpio: lpc32xx: allow building on non-lpc32xx targets
From: Bartosz Golaszewski @ 2019-08-02 7:10 UTC (permalink / raw)
To: Arnd Bergmann
Cc: soc, arm-soc, Vladimir Zapolskiy, Sylvain Lemieux, Russell King,
Gregory Clement, Linus Walleij, Jason Cooper, Andrew Lunn,
Sebastian Hesselbarth, David S. Miller, Greg Kroah-Hartman,
Alan Stern, Guenter Roeck, linux-gpio, netdev, linux-serial,
linux-usb, LINUXWATCHDOG, Lee Jones, LKML
In-Reply-To: <20190731195713.3150463-6-arnd@arndb.de>
śr., 31 lip 2019 o 22:06 Arnd Bergmann <arnd@arndb.de> napisał(a):
>
> The driver uses hardwire MMIO addresses instead of the data
> that is passed in device tree. Change it over to only
> hardcode the register offset values and allow compile-testing.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Hi Arnd,
thanks for working on this.
> ---
> drivers/gpio/Kconfig | 8 +++++
> drivers/gpio/Makefile | 2 +-
> drivers/gpio/gpio-lpc32xx.c | 63 ++++++++++++++++++++++++-------------
> 3 files changed, 50 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index bb13c266c329..ae86ee963eae 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -311,6 +311,14 @@ config GPIO_LPC18XX
> Select this option to enable GPIO driver for
> NXP LPC18XX/43XX devices.
>
> +config GPIO_LPC32XX
> + tristate "NXP LPC32XX GPIO support"
> + default ARCH_LPC32XX
> + depends on OF_GPIO && (ARCH_LPC32XX || COMPILE_TEST)
> + help
> + Select this option to enable GPIO driver for
> + NXP LPC32XX devices.
> +
> config GPIO_LYNXPOINT
> tristate "Intel Lynxpoint GPIO support"
> depends on ACPI && X86
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index a4e91175c708..87d659ae95eb 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -74,7 +74,7 @@ obj-$(CONFIG_GPIO_LP3943) += gpio-lp3943.o
> obj-$(CONFIG_GPIO_LP873X) += gpio-lp873x.o
> obj-$(CONFIG_GPIO_LP87565) += gpio-lp87565.o
> obj-$(CONFIG_GPIO_LPC18XX) += gpio-lpc18xx.o
> -obj-$(CONFIG_ARCH_LPC32XX) += gpio-lpc32xx.o
> +obj-$(CONFIG_GPIO_LPC32XX) += gpio-lpc32xx.o
> obj-$(CONFIG_GPIO_LYNXPOINT) += gpio-lynxpoint.o
> obj-$(CONFIG_GPIO_MADERA) += gpio-madera.o
> obj-$(CONFIG_GPIO_MAX3191X) += gpio-max3191x.o
> diff --git a/drivers/gpio/gpio-lpc32xx.c b/drivers/gpio/gpio-lpc32xx.c
> index 24885b3db3d5..548f7cb69386 100644
> --- a/drivers/gpio/gpio-lpc32xx.c
> +++ b/drivers/gpio/gpio-lpc32xx.c
> @@ -16,8 +16,7 @@
> #include <linux/platform_device.h>
> #include <linux/module.h>
>
> -#include <mach/hardware.h>
> -#include <mach/platform.h>
> +#define _GPREG(x) (x)
What purpose does this macro serve?
>
> #define LPC32XX_GPIO_P3_INP_STATE _GPREG(0x000)
> #define LPC32XX_GPIO_P3_OUTP_SET _GPREG(0x004)
> @@ -72,12 +71,12 @@
> #define LPC32XX_GPO_P3_GRP (LPC32XX_GPI_P3_GRP + LPC32XX_GPI_P3_MAX)
>
> struct gpio_regs {
> - void __iomem *inp_state;
> - void __iomem *outp_state;
> - void __iomem *outp_set;
> - void __iomem *outp_clr;
> - void __iomem *dir_set;
> - void __iomem *dir_clr;
> + unsigned long inp_state;
> + unsigned long outp_state;
> + unsigned long outp_set;
> + unsigned long outp_clr;
> + unsigned long dir_set;
> + unsigned long dir_clr;
> };
>
> /*
> @@ -167,14 +166,26 @@ struct lpc32xx_gpio_chip {
> struct gpio_regs *gpio_grp;
> };
>
> +void __iomem *gpio_reg_base;
Any reason why this can't be made part of struct lpc32xx_gpio_chip?
> +
> +static inline u32 gpreg_read(unsigned long offset)
Here and elsewhere: could you please keep the lpc32xx_gpio prefix for
all symbols?
> +{
> + return __raw_readl(gpio_reg_base + offset);
> +}
> +
> +static inline void gpreg_write(u32 val, unsigned long offset)
> +{
> + __raw_writel(val, gpio_reg_base + offset);
> +}
> +
> static void __set_gpio_dir_p012(struct lpc32xx_gpio_chip *group,
> unsigned pin, int input)
> {
> if (input)
> - __raw_writel(GPIO012_PIN_TO_BIT(pin),
> + gpreg_write(GPIO012_PIN_TO_BIT(pin),
> group->gpio_grp->dir_clr);
> else
> - __raw_writel(GPIO012_PIN_TO_BIT(pin),
> + gpreg_write(GPIO012_PIN_TO_BIT(pin),
> group->gpio_grp->dir_set);
> }
>
> @@ -184,19 +195,19 @@ static void __set_gpio_dir_p3(struct lpc32xx_gpio_chip *group,
> u32 u = GPIO3_PIN_TO_BIT(pin);
>
> if (input)
> - __raw_writel(u, group->gpio_grp->dir_clr);
> + gpreg_write(u, group->gpio_grp->dir_clr);
> else
> - __raw_writel(u, group->gpio_grp->dir_set);
> + gpreg_write(u, group->gpio_grp->dir_set);
> }
>
> static void __set_gpio_level_p012(struct lpc32xx_gpio_chip *group,
> unsigned pin, int high)
> {
> if (high)
> - __raw_writel(GPIO012_PIN_TO_BIT(pin),
> + gpreg_write(GPIO012_PIN_TO_BIT(pin),
> group->gpio_grp->outp_set);
> else
> - __raw_writel(GPIO012_PIN_TO_BIT(pin),
> + gpreg_write(GPIO012_PIN_TO_BIT(pin),
> group->gpio_grp->outp_clr);
> }
>
> @@ -206,31 +217,31 @@ static void __set_gpio_level_p3(struct lpc32xx_gpio_chip *group,
> u32 u = GPIO3_PIN_TO_BIT(pin);
>
> if (high)
> - __raw_writel(u, group->gpio_grp->outp_set);
> + gpreg_write(u, group->gpio_grp->outp_set);
> else
> - __raw_writel(u, group->gpio_grp->outp_clr);
> + gpreg_write(u, group->gpio_grp->outp_clr);
> }
>
> static void __set_gpo_level_p3(struct lpc32xx_gpio_chip *group,
> unsigned pin, int high)
> {
> if (high)
> - __raw_writel(GPO3_PIN_TO_BIT(pin), group->gpio_grp->outp_set);
> + gpreg_write(GPO3_PIN_TO_BIT(pin), group->gpio_grp->outp_set);
> else
> - __raw_writel(GPO3_PIN_TO_BIT(pin), group->gpio_grp->outp_clr);
> + gpreg_write(GPO3_PIN_TO_BIT(pin), group->gpio_grp->outp_clr);
> }
>
> static int __get_gpio_state_p012(struct lpc32xx_gpio_chip *group,
> unsigned pin)
> {
> - return GPIO012_PIN_IN_SEL(__raw_readl(group->gpio_grp->inp_state),
> + return GPIO012_PIN_IN_SEL(gpreg_read(group->gpio_grp->inp_state),
> pin);
> }
>
> static int __get_gpio_state_p3(struct lpc32xx_gpio_chip *group,
> unsigned pin)
> {
> - int state = __raw_readl(group->gpio_grp->inp_state);
> + int state = gpreg_read(group->gpio_grp->inp_state);
>
> /*
> * P3 GPIO pin input mapping is not contiguous, GPIOP3-0..4 is mapped
> @@ -242,13 +253,13 @@ static int __get_gpio_state_p3(struct lpc32xx_gpio_chip *group,
> static int __get_gpi_state_p3(struct lpc32xx_gpio_chip *group,
> unsigned pin)
> {
> - return GPI3_PIN_IN_SEL(__raw_readl(group->gpio_grp->inp_state), pin);
> + return GPI3_PIN_IN_SEL(gpreg_read(group->gpio_grp->inp_state), pin);
> }
>
> static int __get_gpo_state_p3(struct lpc32xx_gpio_chip *group,
> unsigned pin)
> {
> - return GPO3_PIN_IN_SEL(__raw_readl(group->gpio_grp->outp_state), pin);
> + return GPO3_PIN_IN_SEL(gpreg_read(group->gpio_grp->outp_state), pin);
> }
>
> /*
> @@ -498,6 +509,10 @@ static int lpc32xx_gpio_probe(struct platform_device *pdev)
> {
> int i;
>
> + gpio_reg_base = devm_platform_ioremap_resource(pdev, 0);
> + if (gpio_reg_base)
> + return -ENXIO;
> +
> for (i = 0; i < ARRAY_SIZE(lpc32xx_gpiochip); i++) {
> if (pdev->dev.of_node) {
> lpc32xx_gpiochip[i].chip.of_xlate = lpc32xx_of_xlate;
> @@ -527,3 +542,7 @@ static struct platform_driver lpc32xx_gpio_driver = {
> };
>
> module_platform_driver(lpc32xx_gpio_driver);
> +
> +MODULE_AUTHOR("Kevin Wells <kevin.wells@nxp.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("GPIO driver for LPC32xx SoC");
> --
> 2.20.0
>
Bart
^ permalink raw reply
* Re: [PATCH v1 1/4] gpio: pca953x: Switch to use device_get_match_data()
From: Bartosz Golaszewski @ 2019-08-02 7:51 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Linus Walleij, linux-gpio, Marek Vasut
In-Reply-To: <20190801173938.36676-1-andriy.shevchenko@linux.intel.com>
czw., 1 sie 2019 o 19:39 Andy Shevchenko
<andriy.shevchenko@linux.intel.com> napisał(a):
>
> Instead of open coded variants, switch to direct use of
> device_get_match_data().
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> drivers/gpio/gpio-pca953x.c | 20 ++++++++------------
> 1 file changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c
> index 378b206d2dc9..54cf01901320 100644
> --- a/drivers/gpio/gpio-pca953x.c
> +++ b/drivers/gpio/gpio-pca953x.c
> @@ -949,19 +949,15 @@ static int pca953x_probe(struct i2c_client *client,
> if (i2c_id) {
> chip->driver_data = i2c_id->driver_data;
> } else {
> - const struct acpi_device_id *acpi_id;
> - struct device *dev = &client->dev;
> -
> - chip->driver_data = (uintptr_t)of_device_get_match_data(dev);
> - if (!chip->driver_data) {
> - acpi_id = acpi_match_device(pca953x_acpi_ids, dev);
> - if (!acpi_id) {
> - ret = -ENODEV;
> - goto err_exit;
> - }
> -
> - chip->driver_data = acpi_id->driver_data;
> + const void *match;
> +
> + match = device_get_match_data(&client->dev);
> + if (!match) {
> + ret = -ENODEV;
> + goto err_exit;
> }
> +
> + chip->driver_data = (uintptr_t)match;
> }
>
> i2c_set_clientdata(client, chip);
> --
> 2.20.1
>
Excellent work, all four applied.
Bart
^ permalink raw reply
* [PATCH 4.4 109/158] pinctrl: rockchip: fix leaked of_node references
From: Greg Kroah-Hartman @ 2019-08-02 9:28 UTC (permalink / raw)
To: linux-kernel
Cc: Greg Kroah-Hartman, stable, Wen Yang, Linus Walleij,
Heiko Stuebner, linux-gpio, linux-rockchip, Sasha Levin
In-Reply-To: <20190802092203.671944552@linuxfoundation.org>
[ Upstream commit 3c89c70634bb0b6f48512de873e7a45c7e1fbaa5 ]
The call to of_parse_phandle returns a node pointer with refcount
incremented thus it must be explicitly decremented after the last
usage.
Detected by coccinelle with the following warnings:
./drivers/pinctrl/pinctrl-rockchip.c:3221:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 3196, but without a corresponding object release within this function.
./drivers/pinctrl/pinctrl-rockchip.c:3223:1-7: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 3196, but without a corresponding object release within this function.
Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: linux-gpio@vger.kernel.org
Cc: linux-rockchip@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/pinctrl/pinctrl-rockchip.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index a0651128e23a..616055b5e996 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -1837,6 +1837,7 @@ static int rockchip_get_bank_data(struct rockchip_pin_bank *bank,
base,
&rockchip_regmap_config);
}
+ of_node_put(node);
}
bank->irq = irq_of_parse_and_map(bank->of_node, 0);
--
2.20.1
^ permalink raw reply related
* [PATCH 4.9 162/223] pinctrl: rockchip: fix leaked of_node references
From: Greg Kroah-Hartman @ 2019-08-02 9:36 UTC (permalink / raw)
To: linux-kernel
Cc: Greg Kroah-Hartman, stable, Wen Yang, Linus Walleij,
Heiko Stuebner, linux-gpio, linux-rockchip, Sasha Levin
In-Reply-To: <20190802092238.692035242@linuxfoundation.org>
[ Upstream commit 3c89c70634bb0b6f48512de873e7a45c7e1fbaa5 ]
The call to of_parse_phandle returns a node pointer with refcount
incremented thus it must be explicitly decremented after the last
usage.
Detected by coccinelle with the following warnings:
./drivers/pinctrl/pinctrl-rockchip.c:3221:2-8: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 3196, but without a corresponding object release within this function.
./drivers/pinctrl/pinctrl-rockchip.c:3223:1-7: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 3196, but without a corresponding object release within this function.
Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: linux-gpio@vger.kernel.org
Cc: linux-rockchip@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/pinctrl/pinctrl-rockchip.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
index f826793e972c..417cd3bd7e0c 100644
--- a/drivers/pinctrl/pinctrl-rockchip.c
+++ b/drivers/pinctrl/pinctrl-rockchip.c
@@ -2208,6 +2208,7 @@ static int rockchip_get_bank_data(struct rockchip_pin_bank *bank,
base,
&rockchip_regmap_config);
}
+ of_node_put(node);
}
bank->irq = irq_of_parse_and_map(bank->of_node, 0);
--
2.20.1
^ permalink raw reply related
* Re: [PATCH 05/14] gpio: lpc32xx: allow building on non-lpc32xx targets
From: Arnd Bergmann @ 2019-08-02 11:19 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: soc, arm-soc, Vladimir Zapolskiy, Sylvain Lemieux, Russell King,
Gregory Clement, Linus Walleij, Jason Cooper, Andrew Lunn,
Sebastian Hesselbarth, David S. Miller, Greg Kroah-Hartman,
Alan Stern, Guenter Roeck, linux-gpio, netdev, linux-serial,
USB list, LINUXWATCHDOG, Lee Jones, LKML
In-Reply-To: <CAMpxmJWFfT_vrDas2fzW5tnxskk9kmgHQpGnGQ-_C20UaS_jhA@mail.gmail.com>
On Fri, Aug 2, 2019 at 9:10 AM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
> > -#include <mach/hardware.h>
> > -#include <mach/platform.h>
> > +#define _GPREG(x) (x)
>
> What purpose does this macro serve?
>
> >
> > #define LPC32XX_GPIO_P3_INP_STATE _GPREG(0x000)
> > #define LPC32XX_GPIO_P3_OUTP_SET _GPREG(0x004)
In the existing code base, this macro converts a register offset to
an __iomem pointer for a gpio register. I changed the definition of the
macro here to keep the number of changes down, but I it's just
as easy to remove it if you prefer.
> > @@ -167,14 +166,26 @@ struct lpc32xx_gpio_chip {
> > struct gpio_regs *gpio_grp;
> > };
> >
> > +void __iomem *gpio_reg_base;
>
> Any reason why this can't be made part of struct lpc32xx_gpio_chip?
It could be, but it's the same for each instance, and not known until
probe() time, so the same pointer would need to be copied into each
instance that is otherwise read-only.
Let me know if you'd prefer me to rework these two things or leave
them as they are.
> > +static inline u32 gpreg_read(unsigned long offset)
>
> Here and elsewhere: could you please keep the lpc32xx_gpio prefix for
> all symbols?
Sure.
Arnd
^ permalink raw reply
* Re: [PATCH v7 07/20] clk: tegra: clk-periph: Add save and restore support
From: Dmitry Osipenko @ 2019-08-02 12:32 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree
In-Reply-To: <1564532424-10449-8-git-send-email-skomatineni@nvidia.com>
31.07.2019 3:20, Sowjanya Komatineni пишет:
> This patch implements save and restore context for peripheral fixed
> clock ops, peripheral gate clock ops, sdmmc mux clock ops, and
> peripheral clock ops.
>
> During system suspend, core power goes off and looses the settings
> of the Tegra CAR controller registers.
>
> So during suspend entry clock and reset state of peripherals is saved
> and on resume they are restored to have clocks back to same rate and
> state as before suspend.
>
> Acked-by: Thierry Reding <treding@nvidia.com>
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
> drivers/clk/tegra/clk-periph-fixed.c | 33 ++++++++++++++++++++++++++++++++
> drivers/clk/tegra/clk-periph-gate.c | 34 +++++++++++++++++++++++++++++++++
> drivers/clk/tegra/clk-periph.c | 37 ++++++++++++++++++++++++++++++++++++
> drivers/clk/tegra/clk-sdmmc-mux.c | 28 +++++++++++++++++++++++++++
> drivers/clk/tegra/clk.h | 6 ++++++
> 5 files changed, 138 insertions(+)
>
> diff --git a/drivers/clk/tegra/clk-periph-fixed.c b/drivers/clk/tegra/clk-periph-fixed.c
> index c088e7a280df..21b24530fa00 100644
> --- a/drivers/clk/tegra/clk-periph-fixed.c
> +++ b/drivers/clk/tegra/clk-periph-fixed.c
> @@ -60,11 +60,44 @@ tegra_clk_periph_fixed_recalc_rate(struct clk_hw *hw,
> return (unsigned long)rate;
> }
>
> +static int tegra_clk_periph_fixed_save_context(struct clk_hw *hw)
> +{
> + struct tegra_clk_periph_fixed *fixed = to_tegra_clk_periph_fixed(hw);
> + u32 mask = 1 << (fixed->num % 32);
This could be BIT(fixed->num % 32).
> + fixed->enb_ctx = readl_relaxed(fixed->base + fixed->regs->enb_reg) &
> + mask;
> + fixed->rst_ctx = readl_relaxed(fixed->base + fixed->regs->rst_reg) &
> + mask;
The enb_ctx/rst_ctx are booleans, while you assigning an integer value
here. You're getting away here because bool is an 32bit unsigned int,
but you shouldn't rely on it and always explicitly convert to a bool.
> + return 0;
> +}
> +
> +static void tegra_clk_periph_fixed_restore_context(struct clk_hw *hw)
> +{
> + struct tegra_clk_periph_fixed *fixed = to_tegra_clk_periph_fixed(hw);
> + u32 mask = 1 << (fixed->num % 32);
> +
> + if (fixed->enb_ctx)
> + writel_relaxed(mask, fixed->base + fixed->regs->enb_set_reg);
> + else
> + writel_relaxed(mask, fixed->base + fixed->regs->enb_clr_reg);
> +
> + udelay(2);
Will be better to read out and compare the hardware's state with the
restored one, then bail out if the state is unchanged.
Shouldn't it be fence_udelay()?
> + if (!fixed->rst_ctx) {
> + udelay(5); /* reset propogation delay */
Why delaying is done before the writing to the reset register?
> + writel_relaxed(mask, fixed->base + fixed->regs->rst_reg);
I'm not quite sure what's going on here, this looks wrong.
1. rst_reg points to RST_DEVICES_x
2. Each bit of RST_DEVICES_x represents the reset-assertion state of
each individual device
3. By writing to rst_reg, all (!) devices are deasserted, except the one
device which corresponds to the mask
4. The reset is asserted for a single device, while !fixed->rst_ctx
means that it actually should be deasserted (?)
Apparently you should use rst_set_reg / rst_clr_reg.
> + }
What about the case where rst_ctx=true?
> +}
> @@ -517,6 +517,8 @@ struct tegra_clk_periph_gate {
> int clk_num;
> int *enable_refcnt;
> const struct tegra_clk_periph_regs *regs;
> + bool clk_state_ctx;
> + bool rst_state_ctx;
> };
>
> #define to_clk_periph_gate(_hw) \
> @@ -543,6 +545,8 @@ struct tegra_clk_periph_fixed {
> unsigned int mul;
> unsigned int div;
> unsigned int num;
> + bool enb_ctx;
> + bool rst_ctx;
> };
I'd expect these to be bool:1.
^ permalink raw reply
* Re: [PATCH v7 07/20] clk: tegra: clk-periph: Add save and restore support
From: Dmitry Osipenko @ 2019-08-02 12:38 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree
In-Reply-To: <9f6fc791-5c76-76d5-98fb-fd8facfd75d7@nvidia.com>
02.08.2019 2:49, Sowjanya Komatineni пишет:
>
> On 8/1/19 4:19 PM, Sowjanya Komatineni wrote:
>>
>> On 8/1/19 2:30 PM, Sowjanya Komatineni wrote:
>>>
>>> On 8/1/19 1:54 PM, Dmitry Osipenko wrote:
>>>> 01.08.2019 23:31, Sowjanya Komatineni пишет:
>>>>> On 8/1/19 1:17 PM, Dmitry Osipenko wrote:
>>>>>> 01.08.2019 22:42, Sowjanya Komatineni пишет:
>>>>>>> On 8/1/19 12:00 PM, Dmitry Osipenko wrote:
>>>>>>>> 01.08.2019 20:58, Sowjanya Komatineni пишет:
>>>>>>>>> On 7/31/19 4:09 PM, Sowjanya Komatineni wrote:
>>>>>>>>>> On 7/31/19 3:44 AM, Dmitry Osipenko wrote:
>>>>>>>>>>> 31.07.2019 12:50, Dmitry Osipenko пишет:
>>>>>>>>>>>> 31.07.2019 3:20, Sowjanya Komatineni пишет:
>>>>>>>>>>>>> This patch implements save and restore context for peripheral
>>>>>>>>>>>>> fixed
>>>>>>>>>>>>> clock ops, peripheral gate clock ops, sdmmc mux clock ops, and
>>>>>>>>>>>>> peripheral clock ops.
>>>>>>>>>>>>>
>>>>>>>>>>>>> During system suspend, core power goes off and looses the
>>>>>>>>>>>>> settings
>>>>>>>>>>>>> of the Tegra CAR controller registers.
>>>>>>>>>>>>>
>>>>>>>>>>>>> So during suspend entry clock and reset state of
>>>>>>>>>>>>> peripherals is
>>>>>>>>>>>>> saved
>>>>>>>>>>>>> and on resume they are restored to have clocks back to same
>>>>>>>>>>>>> rate and
>>>>>>>>>>>>> state as before suspend.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>>>>>>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>> drivers/clk/tegra/clk-periph-fixed.c | 33
>>>>>>>>>>>>> ++++++++++++++++++++++++++++++++
>>>>>>>>>>>>> drivers/clk/tegra/clk-periph-gate.c | 34
>>>>>>>>>>>>> +++++++++++++++++++++++++++++++++
>>>>>>>>>>>>> drivers/clk/tegra/clk-periph.c | 37
>>>>>>>>>>>>> ++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>>> drivers/clk/tegra/clk-sdmmc-mux.c | 28
>>>>>>>>>>>>> +++++++++++++++++++++++++++
>>>>>>>>>>>>> drivers/clk/tegra/clk.h | 6 ++++++
>>>>>>>>>>>>> 5 files changed, 138 insertions(+)
>>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>> index c088e7a280df..21b24530fa00 100644
>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>> @@ -60,11 +60,44 @@ tegra_clk_periph_fixed_recalc_rate(struct
>>>>>>>>>>>>> clk_hw *hw,
>>>>>>>>>>>>> return (unsigned long)rate;
>>>>>>>>>>>>> }
>>>>>>>>>>>>> +static int tegra_clk_periph_fixed_save_context(struct
>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>> +{
>>>>>>>>>>>>> + struct tegra_clk_periph_fixed *fixed =
>>>>>>>>>>>>> to_tegra_clk_periph_fixed(hw);
>>>>>>>>>>>>> + u32 mask = 1 << (fixed->num % 32);
>>>>>>>>>>>>> +
>>>>>>>>>>>>> + fixed->enb_ctx = readl_relaxed(fixed->base +
>>>>>>>>>>>>> fixed->regs->enb_reg) &
>>>>>>>>>>>>> + mask;
>>>>>>>>>>>>> + fixed->rst_ctx = readl_relaxed(fixed->base +
>>>>>>>>>>>>> fixed->regs->rst_reg) &
>>>>>>>>>>>>> + mask;
>>>>>>>>>>>>> +
>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>> +}
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +static void tegra_clk_periph_fixed_restore_context(struct
>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>> +{
>>>>>>>>>>>>> + struct tegra_clk_periph_fixed *fixed =
>>>>>>>>>>>>> to_tegra_clk_periph_fixed(hw);
>>>>>>>>>>>>> + u32 mask = 1 << (fixed->num % 32);
>>>>>>>>>>>>> +
>>>>>>>>>>>>> + if (fixed->enb_ctx)
>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>> fixed->regs->enb_set_reg);
>>>>>>>>>>>>> + else
>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>> fixed->regs->enb_clr_reg);
>>>>>>>>>>>>> +
>>>>>>>>>>>>> + udelay(2);
>>>>>>>>>>>>> +
>>>>>>>>>>>>> + if (!fixed->rst_ctx) {
>>>>>>>>>>>>> + udelay(5); /* reset propogation delay */
>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>> fixed->regs->rst_reg);
>>>>>>>>>>>>> + }
>>>>>>>>>>>>> +}
>>>>>>>>>>>>> +
>>>>>>>>>>>>> static const struct clk_ops tegra_clk_periph_fixed_ops
>>>>>>>>>>>>> = {
>>>>>>>>>>>>> .is_enabled = tegra_clk_periph_fixed_is_enabled,
>>>>>>>>>>>>> .enable = tegra_clk_periph_fixed_enable,
>>>>>>>>>>>>> .disable = tegra_clk_periph_fixed_disable,
>>>>>>>>>>>>> .recalc_rate = tegra_clk_periph_fixed_recalc_rate,
>>>>>>>>>>>>> + .save_context = tegra_clk_periph_fixed_save_context,
>>>>>>>>>>>>> + .restore_context =
>>>>>>>>>>>>> tegra_clk_periph_fixed_restore_context,
>>>>>>>>>>>>> };
>>>>>>>>>>>>> struct clk *tegra_clk_register_periph_fixed(const char
>>>>>>>>>>>>> *name,
>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>> index 4b31beefc9fc..6ba5b08e0787 100644
>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>> @@ -25,6 +25,8 @@ static DEFINE_SPINLOCK(periph_ref_lock);
>>>>>>>>>>>>> #define read_rst(gate) \
>>>>>>>>>>>>> readl_relaxed(gate->clk_base + (gate->regs->rst_reg))
>>>>>>>>>>>>> +#define write_rst_set(val, gate) \
>>>>>>>>>>>>> + writel_relaxed(val, gate->clk_base +
>>>>>>>>>>>>> (gate->regs->rst_set_reg))
>>>>>>>>>>>>> #define write_rst_clr(val, gate) \
>>>>>>>>>>>>> writel_relaxed(val, gate->clk_base +
>>>>>>>>>>>>> (gate->regs->rst_clr_reg))
>>>>>>>>>>>>> @@ -110,10 +112,42 @@ static void
>>>>>>>>>>>>> clk_periph_disable(struct
>>>>>>>>>>>>> clk_hw *hw)
>>>>>>>>>>>>> spin_unlock_irqrestore(&periph_ref_lock, flags);
>>>>>>>>>>>>> }
>>>>>>>>>>>>> +static int clk_periph_gate_save_context(struct clk_hw
>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>> +{
>>>>>>>>>>>>> + struct tegra_clk_periph_gate *gate =
>>>>>>>>>>>>> to_clk_periph_gate(hw);
>>>>>>>>>>>>> +
>>>>>>>>>>>>> + gate->clk_state_ctx = read_enb(gate) &
>>>>>>>>>>>>> periph_clk_to_bit(gate);
>>>>>>>>>>>>> + gate->rst_state_ctx = read_rst(gate) &
>>>>>>>>>>>>> periph_clk_to_bit(gate);
>>>>>>>>>>>>> +
>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>> +}
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +static void clk_periph_gate_restore_context(struct clk_hw
>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>> +{
>>>>>>>>>>>>> + struct tegra_clk_periph_gate *gate =
>>>>>>>>>>>>> to_clk_periph_gate(hw);
>>>>>>>>>>>>> +
>>>>>>>>>>>>> + if (gate->clk_state_ctx)
>>>>>>>>>>>>> + write_enb_set(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>> + else
>>>>>>>>>>>>> + write_enb_clr(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>> +
>>>>>>>>>>>>> + udelay(5);
>>>>>>>>>>>>> +
>>>>>>>>>>>>> + if (!(gate->flags & TEGRA_PERIPH_NO_RESET) &&
>>>>>>>>>>>>> + !(gate->flags & TEGRA_PERIPH_MANUAL_RESET)) {
>>>>>>>>>>>>> + if (gate->rst_state_ctx)
>>>>>>>>>>>>> + write_rst_set(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>> + else
>>>>>>>>>>>>> + write_rst_clr(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>> + }
>>>>>>>>>>>>> +}
>>>>>>>>>>>>> +
>>>>>>>>>>>>> const struct clk_ops tegra_clk_periph_gate_ops = {
>>>>>>>>>>>>> .is_enabled = clk_periph_is_enabled,
>>>>>>>>>>>>> .enable = clk_periph_enable,
>>>>>>>>>>>>> .disable = clk_periph_disable,
>>>>>>>>>>>>> + .save_context = clk_periph_gate_save_context,
>>>>>>>>>>>>> + .restore_context = clk_periph_gate_restore_context,
>>>>>>>>>>>>> };
>>>>>>>>>>>>> struct clk *tegra_clk_register_periph_gate(const
>>>>>>>>>>>>> char *name,
>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>> index 58437da25156..06fb62955768 100644
>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>> @@ -99,6 +99,37 @@ static void clk_periph_disable(struct
>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>> gate_ops->disable(gate_hw);
>>>>>>>>>>>>> }
>>>>>>>>>>>>> +static int clk_periph_save_context(struct clk_hw *hw)
>>>>>>>>>>>>> +{
>>>>>>>>>>>>> + struct tegra_clk_periph *periph = to_clk_periph(hw);
>>>>>>>>>>>>> + const struct clk_ops *gate_ops = periph->gate_ops;
>>>>>>>>>>>>> + struct clk_hw *gate_hw = &periph->gate.hw;
>>>>>>>>>>>>> +
>>>>>>>>>>>>> + if (!(periph->gate.flags & TEGRA_PERIPH_NO_GATE))
>>>>>>>>>>>>> + gate_ops->save_context(gate_hw);
>>>>>>>>>>>>> +
>>>>>>>>>>>>> + periph->parent_ctx = clk_periph_get_parent(hw);
>>>>>>>>>>>>> +
>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>> +}
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +static void clk_periph_restore_context(struct clk_hw *hw)
>>>>>>>>>>>>> +{
>>>>>>>>>>>>> + struct tegra_clk_periph *periph = to_clk_periph(hw);
>>>>>>>>>>>>> + const struct clk_ops *gate_ops = periph->gate_ops;
>>>>>>>>>>>>> + struct clk_hw *gate_hw = &periph->gate.hw;
>>>>>>>>>>>>> + const struct clk_ops *div_ops = periph->div_ops;
>>>>>>>>>>>>> + struct clk_hw *div_hw = &periph->divider.hw;
>>>>>>>>>>>>> +
>>>>>>>>>>>>> + clk_periph_set_parent(hw, periph->parent_ctx);
>>>>>>>>>>>>> +
>>>>>>>>>>>>> + if (!(periph->gate.flags & TEGRA_PERIPH_NO_DIV))
>>>>>>>>>>>>> + div_ops->restore_context(div_hw);
>>>>>>>>>>>> Could you please point to where the divider's save_context()
>>>>>>>>>>>> happens?
>>>>>>>>>>>> Because I can't see it.
>>>>>>>>>>> Ah, I now see that there is no need to save the dividers context
>>>>>>>>>>> because
>>>>>>>>>>> clk itself has enough info that is needed for the context's
>>>>>>>>>>> restoring
>>>>>>>>>>> (like I pointed in the review to v6).
>>>>>>>>>>>
>>>>>>>>>>> Looks like you could also implement a new
>>>>>>>>>>> clk_hw_get_parent_index()
>>>>>>>>>>> generic helper to get the index instead of storing it manually.
>>>>>>>>>> clk_periph_get_parent basically invokes existing clk_mux_ops
>>>>>>>>>> get_parent() which is then saved in tegra_clk_periph.
>>>>>>>>>>
>>>>>>>>>> All existing drivers are using directly get_parent() from clk_mux
>>>>>>>>>> which actually gets index from the register read.
>>>>>>>>>>
>>>>>>>>>> To have this more generic w.r.t save/restore context point of
>>>>>>>>>> view,
>>>>>>>>>> probably instead of implementing new get_parent_index helper,
>>>>>>>>>> I think
>>>>>>>>>> its better to implement save_context and restore_context to
>>>>>>>>>> clk_mux_ops along with creating parent_index field into
>>>>>>>>>> clk_mux to
>>>>>>>>>> cache index during set_parent.
>>>>>>>>>>
>>>>>>>>>> So we just need to invoke mux_ops save_context and
>>>>>>>>>> restore_context.
>>>>>>>>>>
>>>>>>>>> I hope its ok to add save/restore context to clk_mux_ops to be
>>>>>>>>> more
>>>>>>>>> generic w.r.t save/restore context rather than get_parent_index
>>>>>>>>> API.
>>>>>>>>> Please confirm if you agree.
>>>>>>>> Sounds like a good idea. I see that there is a 'restoring'
>>>>>>>> helper for
>>>>>>>> the generic clk_gate, seems something similar could be done for the
>>>>>>>> clk_mux. And looks like anyway you'll need to associate the parent
>>>>>>>> clock
>>>>>>>> with the hw index in order to restore the muxing.
>>>>>>> by 'restoring' helper for generic clk_gate, are you referring to
>>>>>>> clk_gate_restore_context API?
>>>>>> Yes.
>>>>>>
>>>>>>> clk_gate_restore_context is API that's any clk drivers can use for
>>>>>>> clk_gate operation restore for custom gate clk_ops.
>>>>>>>
>>>>>>> But clk-periph is directly using generic clk_mux ops from clk_mux
>>>>>>> so I
>>>>>>> think we should add .restore_context to clk_mux_ops and then during
>>>>>>> clk-periph restore need to invoke mux_ops->restore_context.
>>>>>> I'm not sure whether it will be good for every driver that uses
>>>>>> generic
>>>>>> clk_mux ops. Should be more flexible to have a generic helper
>>>>>> function
>>>>>> that any driver could use in order to restore the clock's parent.
>>>>>>
>>>>>> The clk-periph restoring also includes case of combining divider and
>>>>>> parent restoring, so generic helper could be useful in that case
>>>>>> as well.
>>>>>>
>>>>>> It also looks like you could actually use the
>>>>>> clk_gate_restore_context()
>>>>>> instead of manually saving the clock's enable-state, couldn't you?
>>>>> ok for clk_mux, can add generic clk_mux_restore_context API rather
>>>>> than
>>>>> using restore_context in clk_ops and will invoke that during
>>>>> clk_periph
>>>>> restore.
>>>>>
> digging thru looks like for clk_periph source restore instead of
> clk_mux_restore_context, i can directly do clk_hw_get_parent and
> clk_set_parent with mux_hw as they invoke mux_ops get/set parent anyway.
> Will do this for periph clk mux
>>>>>
>>>>> Reg clk_gate, looks like we cant use generic clk_gate_restore_context
>>>>> for clk-periph as it calls enable/disable callbacks and
>>>>> clk_periph_enable/disable in clk-periph-gate also updated refcnt and
>>>>> depending on that actual enable/disable is set.
>>>>>
>>>>> During suspend, peripherals that are already enabled have their
>>>>> refcnt >
>>>>> 1, so they dont go thru enable/disable on restore if we use same
>>>>> enable/disable callback.
>>>> Looks like you could just decrement the gate's enable_refcnt on
>>>> save_context, wouldn't that work?
>>>>
>> gate->enable_refcnt is within clk-periph-gate which gets updated when
>> enable/disable callbacks get execute thru clk_core_enable/disable.
>> But actual enable_count used in clk_gate_restore_context is the one
>> which gets updated with in the clk core enable/disable functions which
>> invokes these callbacks. Depending on this enable_count in clk core it
>> invokes enable/disable.
>>
>> So, this will cause mismatch if we handle refcnt during save/restore
>> of tegra_clk_periph_gate_ops and also enable/disable thru this
>> clk_gate_restore_context is based on enable_count from clk core.
>>
>>>>> Also to align exact reset state along with CLK (like for case where
>>>>> CLK
>>>>> is enabled but peripheral might be in reset state), implemented
>>>>> save/restore in tegra specific tegra_clk_periph_gate_ops
>>>> I'm wondering whether instead of saving/restoring reset-state of every
>>>> clock, you could simply save/restore the whole RST_DEV_x_SET register.
>>>> Couldn't you?
>>> Thats what I was doing in first version of patch. But later as we
>>> moved to use clk_save_context and clk_restore_context, peripheral
>>> clk_hw RST & CLK enables happen thru its corresponding save/restore
>>> after source restore
>>
>>
>> Also, to align both CLK & RST to the exact state of register, doing
>> save/restore in tegra_clk_periph_gate_ops and invoking this after
>> source restore for peripheral clock, seems cleaner to avoid any
>> misconfiguration b/w rst & clk settings.
>>
It looks to me that it is very wasteful to store/restore each individual
gate and reset state, also given that some of them are shared. I think
that the gates and resets should be restored separately for the
peripherals by a custom tegra_clk_save/restore_periph_gates/resets().
^ permalink raw reply
* Re: [PATCH V6 01/21] irqchip: tegra: Do not disable COP IRQ during suspend
From: Peter De Schrijver @ 2019-08-02 13:05 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Sowjanya Komatineni, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland, pgaikwad,
sboyd, linux-clk, linux-gpio, jckuo, josephl, talho, linux-tegra,
linux-kernel, mperttunen, spatra, robh+dt, devicetree
In-Reply-To: <de1723df-8580-32fb-eb9d-e4c02f2b4306@gmail.com>
On Thu, Jul 25, 2019 at 01:59:09PM +0300, Dmitry Osipenko wrote:
> 25.07.2019 13:38, Peter De Schrijver пишет:
> > On Thu, Jul 25, 2019 at 01:33:48PM +0300, Peter De Schrijver wrote:
> >> On Thu, Jul 25, 2019 at 01:05:13PM +0300, Dmitry Osipenko wrote:
> >>> 25.07.2019 12:55, Peter De Schrijver пишет:
> >>>> On Mon, Jul 22, 2019 at 12:54:51PM +0300, Dmitry Osipenko wrote:
> >>>>>
> >>>>> All Tegra SoCs support SC7, hence the 'supports_sc7' and the comment
> >>>>> doesn't sound correct to me. Something like 'firmware_sc7' should suit
> >>>>> better here.
> >>>>>
> >>>>>> + writel_relaxed(~0ul, ictlr + ICTLR_COP_IER_CLR);
> >>>>>
> >>>>> Secondly, I'm also not sure why COP interrupts need to be disabled for
> >>>>> pre-T210 at all, since COP is unused. This looks to me like it was
> >>>>> cut-n-pasted from downstream kernel without a good reason and could be
> >>>>> simply removed.
> >>>>
> >>>> I don't think we can rely on the fact that COP is unused. People can
> >>>> write their own code to run on COP.
> >>>
> >>> 1. Not upstream - doesn't matter.
> >>>
> >>
> >> The code is not part of the kernel, so obviously it's not upstream?
> >>
> >>> 2. That's not very good if something unknown is running on COP and then
> >>> kernel suddenly intervenes, don't you think so?
> >>
> >> Unless the code was written with this in mind.
> >>
>
> In that case, please see 1. ;)
>
In general the kernel should not touch the COP interrupts I think.
> >
> > Looking at this again, I don't think we need to enable the IRQ at all.
>
> Could you please clarify? The code only saves/restores COP's interrupts
> context across suspend-resume.
The sc7 entry firmware doesn't use interrupts.
Peter.
^ permalink raw reply
* Re: [PATCH V6 01/21] irqchip: tegra: Do not disable COP IRQ during suspend
From: Dmitry Osipenko @ 2019-08-02 17:35 UTC (permalink / raw)
To: Peter De Schrijver
Cc: Sowjanya Komatineni, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland, pgaikwad,
sboyd, linux-clk, linux-gpio, jckuo, josephl, talho, linux-tegra,
linux-kernel, mperttunen, spatra, robh+dt, devicetree
In-Reply-To: <20190802130537.GB3883@pdeschrijver-desktop.Nvidia.com>
02.08.2019 16:05, Peter De Schrijver пишет:
> On Thu, Jul 25, 2019 at 01:59:09PM +0300, Dmitry Osipenko wrote:
>> 25.07.2019 13:38, Peter De Schrijver пишет:
>>> On Thu, Jul 25, 2019 at 01:33:48PM +0300, Peter De Schrijver wrote:
>>>> On Thu, Jul 25, 2019 at 01:05:13PM +0300, Dmitry Osipenko wrote:
>>>>> 25.07.2019 12:55, Peter De Schrijver пишет:
>>>>>> On Mon, Jul 22, 2019 at 12:54:51PM +0300, Dmitry Osipenko wrote:
>>>>>>>
>>>>>>> All Tegra SoCs support SC7, hence the 'supports_sc7' and the comment
>>>>>>> doesn't sound correct to me. Something like 'firmware_sc7' should suit
>>>>>>> better here.
>>>>>>>
>>>>>>>> + writel_relaxed(~0ul, ictlr + ICTLR_COP_IER_CLR);
>>>>>>>
>>>>>>> Secondly, I'm also not sure why COP interrupts need to be disabled for
>>>>>>> pre-T210 at all, since COP is unused. This looks to me like it was
>>>>>>> cut-n-pasted from downstream kernel without a good reason and could be
>>>>>>> simply removed.
>>>>>>
>>>>>> I don't think we can rely on the fact that COP is unused. People can
>>>>>> write their own code to run on COP.
>>>>>
>>>>> 1. Not upstream - doesn't matter.
>>>>>
>>>>
>>>> The code is not part of the kernel, so obviously it's not upstream?
>>>>
>>>>> 2. That's not very good if something unknown is running on COP and then
>>>>> kernel suddenly intervenes, don't you think so?
>>>>
>>>> Unless the code was written with this in mind.
>>>>
>>
>> In that case, please see 1. ;)
>>
>
> In general the kernel should not touch the COP interrupts I think.
>
>>>
>>> Looking at this again, I don't think we need to enable the IRQ at all.
>>
>> Could you please clarify? The code only saves/restores COP's interrupts
>> context across suspend-resume.
>
> The sc7 entry firmware doesn't use interrupts.
Okay, it shouldn't hurt to clean up the LIC's code a tad by removing the
COP's bits, will make a patch.
^ permalink raw reply
* Re: [PATCH V6 14/21] clk: tegra210: Add suspend and resume support
From: Stephen Boyd @ 2019-08-02 17:51 UTC (permalink / raw)
To: Dmitry Osipenko, Sowjanya Komatineni, jason, jonathanh,
linus.walleij, marc.zyngier, mark.rutland, stefan, tglx,
thierry.reding
Cc: pdeschrijver, pgaikwad, linux-clk, linux-gpio, jckuo, josephl,
talho, linux-tegra, linux-kernel, mperttunen, spatra, robh+dt,
devicetree
In-Reply-To: <8c259511-d8ea-51b2-0b1d-c85b964bc44c@gmail.com>
Quoting Dmitry Osipenko (2019-07-22 00:12:17)
> 22.07.2019 10:09, Dmitry Osipenko пишет:
> > 22.07.2019 9:52, Sowjanya Komatineni пишет:
> >>
> >> On 7/21/19 11:10 PM, Dmitry Osipenko wrote:
> >>> 22.07.2019 1:45, Sowjanya Komatineni пишет:
> >>>> On 7/21/19 2:38 PM, Dmitry Osipenko wrote:
> >>>>> 21.07.2019 22:40, Sowjanya Komatineni пишет:
> >>>>>> @@ -2853,9 +2859,8 @@ static int tegra210_enable_pllu(void)
> >>>>>> reg |= PLL_ENABLE;
> >>>>>> writel(reg, clk_base + PLLU_BASE);
> >>>>>> - readl_relaxed_poll_timeout_atomic(clk_base + PLLU_BASE, reg,
> >>>>>> - reg & PLL_BASE_LOCK, 2, 1000);
> >>>>>> - if (!(reg & PLL_BASE_LOCK)) {
> >>>>>> + ret = tegra210_wait_for_mask(&pllu, PLLU_BASE, PLL_BASE_LOCK);
> >>>>>> + if (ret) {
> >>>>> Why this is needed? Was there a bug?
> >>>>>
> >>>> during resume pllu init is needed and to use same terga210_init_pllu,
> >>>> poll_timeout_atomic can't be used as its ony for atomic context.
> >>>>
> >>>> So changed to use wait_for_mask which should work in both cases.
> >>> Atomic variant could be used from any context, not sure what do you
> >>> mean. The 'atomic' part only means that function won't cause scheduling
> >>> and that's it.
> >>
> >> Sorry, replied incorrect. readx_poll_timeout_atomic uses ktime_get() and
> >> during resume timekeeping suspend/resume happens later than clock
> >> suspend/resume. So using tegra210_wait_for_mask.
> >>
> >> both timekeeping and clk-tegra210 drivers are registered as syscore but
> >> not ordered.
> >
> > Okay, thank you for the clarification.
> >
> > [snip]
> >
>
> You should remove the 'iopoll.h' then, since it's not used anymore.
And also add a comment to this location in the code because it's
non-obvious that we can't use iopoll here.
^ permalink raw reply
* Re: [PATCH v7 07/20] clk: tegra: clk-periph: Add save and restore support
From: Sowjanya Komatineni @ 2019-08-02 18:33 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree
In-Reply-To: <8bca50b2-a78c-c6b1-6547-4cec98a3e9cb@gmail.com>
On 8/2/19 5:38 AM, Dmitry Osipenko wrote:
> 02.08.2019 2:49, Sowjanya Komatineni пишет:
>> On 8/1/19 4:19 PM, Sowjanya Komatineni wrote:
>>> On 8/1/19 2:30 PM, Sowjanya Komatineni wrote:
>>>> On 8/1/19 1:54 PM, Dmitry Osipenko wrote:
>>>>> 01.08.2019 23:31, Sowjanya Komatineni пишет:
>>>>>> On 8/1/19 1:17 PM, Dmitry Osipenko wrote:
>>>>>>> 01.08.2019 22:42, Sowjanya Komatineni пишет:
>>>>>>>> On 8/1/19 12:00 PM, Dmitry Osipenko wrote:
>>>>>>>>> 01.08.2019 20:58, Sowjanya Komatineni пишет:
>>>>>>>>>> On 7/31/19 4:09 PM, Sowjanya Komatineni wrote:
>>>>>>>>>>> On 7/31/19 3:44 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>> 31.07.2019 12:50, Dmitry Osipenko пишет:
>>>>>>>>>>>>> 31.07.2019 3:20, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>> This patch implements save and restore context for peripheral
>>>>>>>>>>>>>> fixed
>>>>>>>>>>>>>> clock ops, peripheral gate clock ops, sdmmc mux clock ops, and
>>>>>>>>>>>>>> peripheral clock ops.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> During system suspend, core power goes off and looses the
>>>>>>>>>>>>>> settings
>>>>>>>>>>>>>> of the Tegra CAR controller registers.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> So during suspend entry clock and reset state of
>>>>>>>>>>>>>> peripherals is
>>>>>>>>>>>>>> saved
>>>>>>>>>>>>>> and on resume they are restored to have clocks back to same
>>>>>>>>>>>>>> rate and
>>>>>>>>>>>>>> state as before suspend.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>>>>>>>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph-fixed.c | 33
>>>>>>>>>>>>>> ++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph-gate.c | 34
>>>>>>>>>>>>>> +++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph.c | 37
>>>>>>>>>>>>>> ++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>> drivers/clk/tegra/clk-sdmmc-mux.c | 28
>>>>>>>>>>>>>> +++++++++++++++++++++++++++
>>>>>>>>>>>>>> drivers/clk/tegra/clk.h | 6 ++++++
>>>>>>>>>>>>>> 5 files changed, 138 insertions(+)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>> index c088e7a280df..21b24530fa00 100644
>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>> @@ -60,11 +60,44 @@ tegra_clk_periph_fixed_recalc_rate(struct
>>>>>>>>>>>>>> clk_hw *hw,
>>>>>>>>>>>>>> return (unsigned long)rate;
>>>>>>>>>>>>>> }
>>>>>>>>>>>>>> +static int tegra_clk_periph_fixed_save_context(struct
>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>> + struct tegra_clk_periph_fixed *fixed =
>>>>>>>>>>>>>> to_tegra_clk_periph_fixed(hw);
>>>>>>>>>>>>>> + u32 mask = 1 << (fixed->num % 32);
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + fixed->enb_ctx = readl_relaxed(fixed->base +
>>>>>>>>>>>>>> fixed->regs->enb_reg) &
>>>>>>>>>>>>>> + mask;
>>>>>>>>>>>>>> + fixed->rst_ctx = readl_relaxed(fixed->base +
>>>>>>>>>>>>>> fixed->regs->rst_reg) &
>>>>>>>>>>>>>> + mask;
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +static void tegra_clk_periph_fixed_restore_context(struct
>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>> + struct tegra_clk_periph_fixed *fixed =
>>>>>>>>>>>>>> to_tegra_clk_periph_fixed(hw);
>>>>>>>>>>>>>> + u32 mask = 1 << (fixed->num % 32);
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + if (fixed->enb_ctx)
>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>> fixed->regs->enb_set_reg);
>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>> fixed->regs->enb_clr_reg);
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + udelay(2);
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + if (!fixed->rst_ctx) {
>>>>>>>>>>>>>> + udelay(5); /* reset propogation delay */
>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>> fixed->regs->rst_reg);
>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> static const struct clk_ops tegra_clk_periph_fixed_ops
>>>>>>>>>>>>>> = {
>>>>>>>>>>>>>> .is_enabled = tegra_clk_periph_fixed_is_enabled,
>>>>>>>>>>>>>> .enable = tegra_clk_periph_fixed_enable,
>>>>>>>>>>>>>> .disable = tegra_clk_periph_fixed_disable,
>>>>>>>>>>>>>> .recalc_rate = tegra_clk_periph_fixed_recalc_rate,
>>>>>>>>>>>>>> + .save_context = tegra_clk_periph_fixed_save_context,
>>>>>>>>>>>>>> + .restore_context =
>>>>>>>>>>>>>> tegra_clk_periph_fixed_restore_context,
>>>>>>>>>>>>>> };
>>>>>>>>>>>>>> struct clk *tegra_clk_register_periph_fixed(const char
>>>>>>>>>>>>>> *name,
>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>> index 4b31beefc9fc..6ba5b08e0787 100644
>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>> @@ -25,6 +25,8 @@ static DEFINE_SPINLOCK(periph_ref_lock);
>>>>>>>>>>>>>> #define read_rst(gate) \
>>>>>>>>>>>>>> readl_relaxed(gate->clk_base + (gate->regs->rst_reg))
>>>>>>>>>>>>>> +#define write_rst_set(val, gate) \
>>>>>>>>>>>>>> + writel_relaxed(val, gate->clk_base +
>>>>>>>>>>>>>> (gate->regs->rst_set_reg))
>>>>>>>>>>>>>> #define write_rst_clr(val, gate) \
>>>>>>>>>>>>>> writel_relaxed(val, gate->clk_base +
>>>>>>>>>>>>>> (gate->regs->rst_clr_reg))
>>>>>>>>>>>>>> @@ -110,10 +112,42 @@ static void
>>>>>>>>>>>>>> clk_periph_disable(struct
>>>>>>>>>>>>>> clk_hw *hw)
>>>>>>>>>>>>>> spin_unlock_irqrestore(&periph_ref_lock, flags);
>>>>>>>>>>>>>> }
>>>>>>>>>>>>>> +static int clk_periph_gate_save_context(struct clk_hw
>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>> + struct tegra_clk_periph_gate *gate =
>>>>>>>>>>>>>> to_clk_periph_gate(hw);
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + gate->clk_state_ctx = read_enb(gate) &
>>>>>>>>>>>>>> periph_clk_to_bit(gate);
>>>>>>>>>>>>>> + gate->rst_state_ctx = read_rst(gate) &
>>>>>>>>>>>>>> periph_clk_to_bit(gate);
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +static void clk_periph_gate_restore_context(struct clk_hw
>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>> + struct tegra_clk_periph_gate *gate =
>>>>>>>>>>>>>> to_clk_periph_gate(hw);
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + if (gate->clk_state_ctx)
>>>>>>>>>>>>>> + write_enb_set(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>> + write_enb_clr(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + udelay(5);
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + if (!(gate->flags & TEGRA_PERIPH_NO_RESET) &&
>>>>>>>>>>>>>> + !(gate->flags & TEGRA_PERIPH_MANUAL_RESET)) {
>>>>>>>>>>>>>> + if (gate->rst_state_ctx)
>>>>>>>>>>>>>> + write_rst_set(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>> + write_rst_clr(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> const struct clk_ops tegra_clk_periph_gate_ops = {
>>>>>>>>>>>>>> .is_enabled = clk_periph_is_enabled,
>>>>>>>>>>>>>> .enable = clk_periph_enable,
>>>>>>>>>>>>>> .disable = clk_periph_disable,
>>>>>>>>>>>>>> + .save_context = clk_periph_gate_save_context,
>>>>>>>>>>>>>> + .restore_context = clk_periph_gate_restore_context,
>>>>>>>>>>>>>> };
>>>>>>>>>>>>>> struct clk *tegra_clk_register_periph_gate(const
>>>>>>>>>>>>>> char *name,
>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>> index 58437da25156..06fb62955768 100644
>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>> @@ -99,6 +99,37 @@ static void clk_periph_disable(struct
>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>> gate_ops->disable(gate_hw);
>>>>>>>>>>>>>> }
>>>>>>>>>>>>>> +static int clk_periph_save_context(struct clk_hw *hw)
>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>> + struct tegra_clk_periph *periph = to_clk_periph(hw);
>>>>>>>>>>>>>> + const struct clk_ops *gate_ops = periph->gate_ops;
>>>>>>>>>>>>>> + struct clk_hw *gate_hw = &periph->gate.hw;
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + if (!(periph->gate.flags & TEGRA_PERIPH_NO_GATE))
>>>>>>>>>>>>>> + gate_ops->save_context(gate_hw);
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + periph->parent_ctx = clk_periph_get_parent(hw);
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> +static void clk_periph_restore_context(struct clk_hw *hw)
>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>> + struct tegra_clk_periph *periph = to_clk_periph(hw);
>>>>>>>>>>>>>> + const struct clk_ops *gate_ops = periph->gate_ops;
>>>>>>>>>>>>>> + struct clk_hw *gate_hw = &periph->gate.hw;
>>>>>>>>>>>>>> + const struct clk_ops *div_ops = periph->div_ops;
>>>>>>>>>>>>>> + struct clk_hw *div_hw = &periph->divider.hw;
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + clk_periph_set_parent(hw, periph->parent_ctx);
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> + if (!(periph->gate.flags & TEGRA_PERIPH_NO_DIV))
>>>>>>>>>>>>>> + div_ops->restore_context(div_hw);
>>>>>>>>>>>>> Could you please point to where the divider's save_context()
>>>>>>>>>>>>> happens?
>>>>>>>>>>>>> Because I can't see it.
>>>>>>>>>>>> Ah, I now see that there is no need to save the dividers context
>>>>>>>>>>>> because
>>>>>>>>>>>> clk itself has enough info that is needed for the context's
>>>>>>>>>>>> restoring
>>>>>>>>>>>> (like I pointed in the review to v6).
>>>>>>>>>>>>
>>>>>>>>>>>> Looks like you could also implement a new
>>>>>>>>>>>> clk_hw_get_parent_index()
>>>>>>>>>>>> generic helper to get the index instead of storing it manually.
>>>>>>>>>>> clk_periph_get_parent basically invokes existing clk_mux_ops
>>>>>>>>>>> get_parent() which is then saved in tegra_clk_periph.
>>>>>>>>>>>
>>>>>>>>>>> All existing drivers are using directly get_parent() from clk_mux
>>>>>>>>>>> which actually gets index from the register read.
>>>>>>>>>>>
>>>>>>>>>>> To have this more generic w.r.t save/restore context point of
>>>>>>>>>>> view,
>>>>>>>>>>> probably instead of implementing new get_parent_index helper,
>>>>>>>>>>> I think
>>>>>>>>>>> its better to implement save_context and restore_context to
>>>>>>>>>>> clk_mux_ops along with creating parent_index field into
>>>>>>>>>>> clk_mux to
>>>>>>>>>>> cache index during set_parent.
>>>>>>>>>>>
>>>>>>>>>>> So we just need to invoke mux_ops save_context and
>>>>>>>>>>> restore_context.
>>>>>>>>>>>
>>>>>>>>>> I hope its ok to add save/restore context to clk_mux_ops to be
>>>>>>>>>> more
>>>>>>>>>> generic w.r.t save/restore context rather than get_parent_index
>>>>>>>>>> API.
>>>>>>>>>> Please confirm if you agree.
>>>>>>>>> Sounds like a good idea. I see that there is a 'restoring'
>>>>>>>>> helper for
>>>>>>>>> the generic clk_gate, seems something similar could be done for the
>>>>>>>>> clk_mux. And looks like anyway you'll need to associate the parent
>>>>>>>>> clock
>>>>>>>>> with the hw index in order to restore the muxing.
>>>>>>>> by 'restoring' helper for generic clk_gate, are you referring to
>>>>>>>> clk_gate_restore_context API?
>>>>>>> Yes.
>>>>>>>
>>>>>>>> clk_gate_restore_context is API that's any clk drivers can use for
>>>>>>>> clk_gate operation restore for custom gate clk_ops.
>>>>>>>>
>>>>>>>> But clk-periph is directly using generic clk_mux ops from clk_mux
>>>>>>>> so I
>>>>>>>> think we should add .restore_context to clk_mux_ops and then during
>>>>>>>> clk-periph restore need to invoke mux_ops->restore_context.
>>>>>>> I'm not sure whether it will be good for every driver that uses
>>>>>>> generic
>>>>>>> clk_mux ops. Should be more flexible to have a generic helper
>>>>>>> function
>>>>>>> that any driver could use in order to restore the clock's parent.
>>>>>>>
>>>>>>> The clk-periph restoring also includes case of combining divider and
>>>>>>> parent restoring, so generic helper could be useful in that case
>>>>>>> as well.
>>>>>>>
>>>>>>> It also looks like you could actually use the
>>>>>>> clk_gate_restore_context()
>>>>>>> instead of manually saving the clock's enable-state, couldn't you?
>>>>>> ok for clk_mux, can add generic clk_mux_restore_context API rather
>>>>>> than
>>>>>> using restore_context in clk_ops and will invoke that during
>>>>>> clk_periph
>>>>>> restore.
>>>>>>
>> digging thru looks like for clk_periph source restore instead of
>> clk_mux_restore_context, i can directly do clk_hw_get_parent and
>> clk_set_parent with mux_hw as they invoke mux_ops get/set parent anyway.
>> Will do this for periph clk mux
>>>>>> Reg clk_gate, looks like we cant use generic clk_gate_restore_context
>>>>>> for clk-periph as it calls enable/disable callbacks and
>>>>>> clk_periph_enable/disable in clk-periph-gate also updated refcnt and
>>>>>> depending on that actual enable/disable is set.
>>>>>>
>>>>>> During suspend, peripherals that are already enabled have their
>>>>>> refcnt >
>>>>>> 1, so they dont go thru enable/disable on restore if we use same
>>>>>> enable/disable callback.
>>>>> Looks like you could just decrement the gate's enable_refcnt on
>>>>> save_context, wouldn't that work?
>>>>>
>>> gate->enable_refcnt is within clk-periph-gate which gets updated when
>>> enable/disable callbacks get execute thru clk_core_enable/disable.
>>> But actual enable_count used in clk_gate_restore_context is the one
>>> which gets updated with in the clk core enable/disable functions which
>>> invokes these callbacks. Depending on this enable_count in clk core it
>>> invokes enable/disable.
>>>
>>> So, this will cause mismatch if we handle refcnt during save/restore
>>> of tegra_clk_periph_gate_ops and also enable/disable thru this
>>> clk_gate_restore_context is based on enable_count from clk core.
>>>
>>>>>> Also to align exact reset state along with CLK (like for case where
>>>>>> CLK
>>>>>> is enabled but peripheral might be in reset state), implemented
>>>>>> save/restore in tegra specific tegra_clk_periph_gate_ops
>>>>> I'm wondering whether instead of saving/restoring reset-state of every
>>>>> clock, you could simply save/restore the whole RST_DEV_x_SET register.
>>>>> Couldn't you?
>>>> Thats what I was doing in first version of patch. But later as we
>>>> moved to use clk_save_context and clk_restore_context, peripheral
>>>> clk_hw RST & CLK enables happen thru its corresponding save/restore
>>>> after source restore
>>>
>>> Also, to align both CLK & RST to the exact state of register, doing
>>> save/restore in tegra_clk_periph_gate_ops and invoking this after
>>> source restore for peripheral clock, seems cleaner to avoid any
>>> misconfiguration b/w rst & clk settings.
>>>
> It looks to me that it is very wasteful to store/restore each individual
> gate and reset state, also given that some of them are shared. I think
> that the gates and resets should be restored separately for the
> peripherals by a custom tegra_clk_save/restore_periph_gates/resets().
clk_periph_fixed_disable just disables clock only without deasserting
the corresponding peripheral.
corresponding peripheral drivers can also issue reset assert/deassert
thru reset_control_assert/deassert.
So, we will not get the actual state of clk and rst unless we read and
save state of reset and clock separately during save_context.
Currently patch is already using custom
tegra_clk_periph_fixed_save/restore_context for corresponding clk_ops.
Are you suggesting to do save and restore of complete CLK_ENB/RST_DEV
register settings instead of individual peripheral bits?
^ permalink raw reply
* Re: [PATCH v7 07/20] clk: tegra: clk-periph: Add save and restore support
From: Sowjanya Komatineni @ 2019-08-02 18:43 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree
In-Reply-To: <c703b4fc-9ebb-0fd4-11de-80974b5c3842@gmail.com>
On 8/2/19 5:32 AM, Dmitry Osipenko wrote:
> 31.07.2019 3:20, Sowjanya Komatineni пишет:
>> This patch implements save and restore context for peripheral fixed
>> clock ops, peripheral gate clock ops, sdmmc mux clock ops, and
>> peripheral clock ops.
>>
>> During system suspend, core power goes off and looses the settings
>> of the Tegra CAR controller registers.
>>
>> So during suspend entry clock and reset state of peripherals is saved
>> and on resume they are restored to have clocks back to same rate and
>> state as before suspend.
>>
>> Acked-by: Thierry Reding <treding@nvidia.com>
>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>> ---
>> drivers/clk/tegra/clk-periph-fixed.c | 33 ++++++++++++++++++++++++++++++++
>> drivers/clk/tegra/clk-periph-gate.c | 34 +++++++++++++++++++++++++++++++++
>> drivers/clk/tegra/clk-periph.c | 37 ++++++++++++++++++++++++++++++++++++
>> drivers/clk/tegra/clk-sdmmc-mux.c | 28 +++++++++++++++++++++++++++
>> drivers/clk/tegra/clk.h | 6 ++++++
>> 5 files changed, 138 insertions(+)
>>
>> diff --git a/drivers/clk/tegra/clk-periph-fixed.c b/drivers/clk/tegra/clk-periph-fixed.c
>> index c088e7a280df..21b24530fa00 100644
>> --- a/drivers/clk/tegra/clk-periph-fixed.c
>> +++ b/drivers/clk/tegra/clk-periph-fixed.c
>> @@ -60,11 +60,44 @@ tegra_clk_periph_fixed_recalc_rate(struct clk_hw *hw,
>> return (unsigned long)rate;
>> }
>>
>> +static int tegra_clk_periph_fixed_save_context(struct clk_hw *hw)
>> +{
>> + struct tegra_clk_periph_fixed *fixed = to_tegra_clk_periph_fixed(hw);
>> + u32 mask = 1 << (fixed->num % 32);
> This could be BIT(fixed->num % 32).
>
>> + fixed->enb_ctx = readl_relaxed(fixed->base + fixed->regs->enb_reg) &
>> + mask;
>> + fixed->rst_ctx = readl_relaxed(fixed->base + fixed->regs->rst_reg) &
>> + mask;
> The enb_ctx/rst_ctx are booleans, while you assigning an integer value
> here. You're getting away here because bool is an 32bit unsigned int,
> but you shouldn't rely on it and always explicitly convert to a bool.
>
>> + return 0;
>> +}
>> +
>> +static void tegra_clk_periph_fixed_restore_context(struct clk_hw *hw)
>> +{
>> + struct tegra_clk_periph_fixed *fixed = to_tegra_clk_periph_fixed(hw);
>> + u32 mask = 1 << (fixed->num % 32);
>> +
>> + if (fixed->enb_ctx)
>> + writel_relaxed(mask, fixed->base + fixed->regs->enb_set_reg);
>> + else
>> + writel_relaxed(mask, fixed->base + fixed->regs->enb_clr_reg);
>> +
>> + udelay(2);
> Will be better to read out and compare the hardware's state with the
> restored one, then bail out if the state is unchanged.
>
> Shouldn't it be fence_udelay()?
>
>> + if (!fixed->rst_ctx) {
>> + udelay(5); /* reset propogation delay */
> Why delaying is done before the writing to the reset register?
During SC7 exit, peripheral reset state is set to POR state. So some
peripherals will already be in reset state and making sure of
propagation delay before releasing from reset.
It should be rst_clr_reg. will fix in next rev
>
>> + writel_relaxed(mask, fixed->base + fixed->regs->rst_reg);
> I'm not quite sure what's going on here, this looks wrong.
>
> 1. rst_reg points to RST_DEVICES_x
> 2. Each bit of RST_DEVICES_x represents the reset-assertion state of
> each individual device
> 3. By writing to rst_reg, all (!) devices are deasserted, except the one
> device which corresponds to the mask
> 4. The reset is asserted for a single device, while !fixed->rst_ctx
> means that it actually should be deasserted (?)
>
> Apparently you should use rst_set_reg / rst_clr_reg.
Yes, It should be rst_clr_reg. will fix in next rev
>> + }
> What about the case where rst_ctx=true?
ON SC7 exit, state of RST_DEV will be POR state where most peripherals
will already be in reset state.
Few of them which are not in reset state in POR values are those that
need to stay de-asserted across the boot states anyway.
>
>> +}
>> @@ -517,6 +517,8 @@ struct tegra_clk_periph_gate {
>> int clk_num;
>> int *enable_refcnt;
>> const struct tegra_clk_periph_regs *regs;
>> + bool clk_state_ctx;
>> + bool rst_state_ctx;
>> };
>>
>> #define to_clk_periph_gate(_hw) \
>> @@ -543,6 +545,8 @@ struct tegra_clk_periph_fixed {
>> unsigned int mul;
>> unsigned int div;
>> unsigned int num;
>> + bool enb_ctx;
>> + bool rst_ctx;
>> };
> I'd expect these to be bool:1.
^ permalink raw reply
* Re: [PATCH v7 07/20] clk: tegra: clk-periph: Add save and restore support
From: Dmitry Osipenko @ 2019-08-02 20:13 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree
In-Reply-To: <314b5572-4113-d5c5-5956-1a55555a573c@nvidia.com>
02.08.2019 21:33, Sowjanya Komatineni пишет:
>
> On 8/2/19 5:38 AM, Dmitry Osipenko wrote:
>> 02.08.2019 2:49, Sowjanya Komatineni пишет:
>>> On 8/1/19 4:19 PM, Sowjanya Komatineni wrote:
>>>> On 8/1/19 2:30 PM, Sowjanya Komatineni wrote:
>>>>> On 8/1/19 1:54 PM, Dmitry Osipenko wrote:
>>>>>> 01.08.2019 23:31, Sowjanya Komatineni пишет:
>>>>>>> On 8/1/19 1:17 PM, Dmitry Osipenko wrote:
>>>>>>>> 01.08.2019 22:42, Sowjanya Komatineni пишет:
>>>>>>>>> On 8/1/19 12:00 PM, Dmitry Osipenko wrote:
>>>>>>>>>> 01.08.2019 20:58, Sowjanya Komatineni пишет:
>>>>>>>>>>> On 7/31/19 4:09 PM, Sowjanya Komatineni wrote:
>>>>>>>>>>>> On 7/31/19 3:44 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>> 31.07.2019 12:50, Dmitry Osipenko пишет:
>>>>>>>>>>>>>> 31.07.2019 3:20, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>> This patch implements save and restore context for
>>>>>>>>>>>>>>> peripheral
>>>>>>>>>>>>>>> fixed
>>>>>>>>>>>>>>> clock ops, peripheral gate clock ops, sdmmc mux clock
>>>>>>>>>>>>>>> ops, and
>>>>>>>>>>>>>>> peripheral clock ops.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> During system suspend, core power goes off and looses the
>>>>>>>>>>>>>>> settings
>>>>>>>>>>>>>>> of the Tegra CAR controller registers.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> So during suspend entry clock and reset state of
>>>>>>>>>>>>>>> peripherals is
>>>>>>>>>>>>>>> saved
>>>>>>>>>>>>>>> and on resume they are restored to have clocks back to same
>>>>>>>>>>>>>>> rate and
>>>>>>>>>>>>>>> state as before suspend.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>>>>>>>>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph-fixed.c | 33
>>>>>>>>>>>>>>> ++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph-gate.c | 34
>>>>>>>>>>>>>>> +++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph.c | 37
>>>>>>>>>>>>>>> ++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>> drivers/clk/tegra/clk-sdmmc-mux.c | 28
>>>>>>>>>>>>>>> +++++++++++++++++++++++++++
>>>>>>>>>>>>>>> drivers/clk/tegra/clk.h | 6 ++++++
>>>>>>>>>>>>>>> 5 files changed, 138 insertions(+)
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>> index c088e7a280df..21b24530fa00 100644
>>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>> @@ -60,11 +60,44 @@
>>>>>>>>>>>>>>> tegra_clk_periph_fixed_recalc_rate(struct
>>>>>>>>>>>>>>> clk_hw *hw,
>>>>>>>>>>>>>>> return (unsigned long)rate;
>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>> +static int tegra_clk_periph_fixed_save_context(struct
>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>> + struct tegra_clk_periph_fixed *fixed =
>>>>>>>>>>>>>>> to_tegra_clk_periph_fixed(hw);
>>>>>>>>>>>>>>> + u32 mask = 1 << (fixed->num % 32);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + fixed->enb_ctx = readl_relaxed(fixed->base +
>>>>>>>>>>>>>>> fixed->regs->enb_reg) &
>>>>>>>>>>>>>>> + mask;
>>>>>>>>>>>>>>> + fixed->rst_ctx = readl_relaxed(fixed->base +
>>>>>>>>>>>>>>> fixed->regs->rst_reg) &
>>>>>>>>>>>>>>> + mask;
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +static void tegra_clk_periph_fixed_restore_context(struct
>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>> + struct tegra_clk_periph_fixed *fixed =
>>>>>>>>>>>>>>> to_tegra_clk_periph_fixed(hw);
>>>>>>>>>>>>>>> + u32 mask = 1 << (fixed->num % 32);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + if (fixed->enb_ctx)
>>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>>> fixed->regs->enb_set_reg);
>>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>>> fixed->regs->enb_clr_reg);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + udelay(2);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + if (!fixed->rst_ctx) {
>>>>>>>>>>>>>>> + udelay(5); /* reset propogation delay */
>>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>>> fixed->regs->rst_reg);
>>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> static const struct clk_ops tegra_clk_periph_fixed_ops
>>>>>>>>>>>>>>> = {
>>>>>>>>>>>>>>> .is_enabled = tegra_clk_periph_fixed_is_enabled,
>>>>>>>>>>>>>>> .enable = tegra_clk_periph_fixed_enable,
>>>>>>>>>>>>>>> .disable = tegra_clk_periph_fixed_disable,
>>>>>>>>>>>>>>> .recalc_rate = tegra_clk_periph_fixed_recalc_rate,
>>>>>>>>>>>>>>> + .save_context = tegra_clk_periph_fixed_save_context,
>>>>>>>>>>>>>>> + .restore_context =
>>>>>>>>>>>>>>> tegra_clk_periph_fixed_restore_context,
>>>>>>>>>>>>>>> };
>>>>>>>>>>>>>>> struct clk *tegra_clk_register_periph_fixed(const
>>>>>>>>>>>>>>> char
>>>>>>>>>>>>>>> *name,
>>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>> index 4b31beefc9fc..6ba5b08e0787 100644
>>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>> @@ -25,6 +25,8 @@ static DEFINE_SPINLOCK(periph_ref_lock);
>>>>>>>>>>>>>>> #define read_rst(gate) \
>>>>>>>>>>>>>>> readl_relaxed(gate->clk_base +
>>>>>>>>>>>>>>> (gate->regs->rst_reg))
>>>>>>>>>>>>>>> +#define write_rst_set(val, gate) \
>>>>>>>>>>>>>>> + writel_relaxed(val, gate->clk_base +
>>>>>>>>>>>>>>> (gate->regs->rst_set_reg))
>>>>>>>>>>>>>>> #define write_rst_clr(val, gate) \
>>>>>>>>>>>>>>> writel_relaxed(val, gate->clk_base +
>>>>>>>>>>>>>>> (gate->regs->rst_clr_reg))
>>>>>>>>>>>>>>> @@ -110,10 +112,42 @@ static void
>>>>>>>>>>>>>>> clk_periph_disable(struct
>>>>>>>>>>>>>>> clk_hw *hw)
>>>>>>>>>>>>>>> spin_unlock_irqrestore(&periph_ref_lock, flags);
>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>> +static int clk_periph_gate_save_context(struct clk_hw
>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>> + struct tegra_clk_periph_gate *gate =
>>>>>>>>>>>>>>> to_clk_periph_gate(hw);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + gate->clk_state_ctx = read_enb(gate) &
>>>>>>>>>>>>>>> periph_clk_to_bit(gate);
>>>>>>>>>>>>>>> + gate->rst_state_ctx = read_rst(gate) &
>>>>>>>>>>>>>>> periph_clk_to_bit(gate);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +static void clk_periph_gate_restore_context(struct clk_hw
>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>> + struct tegra_clk_periph_gate *gate =
>>>>>>>>>>>>>>> to_clk_periph_gate(hw);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + if (gate->clk_state_ctx)
>>>>>>>>>>>>>>> + write_enb_set(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>>> + write_enb_clr(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + udelay(5);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + if (!(gate->flags & TEGRA_PERIPH_NO_RESET) &&
>>>>>>>>>>>>>>> + !(gate->flags & TEGRA_PERIPH_MANUAL_RESET)) {
>>>>>>>>>>>>>>> + if (gate->rst_state_ctx)
>>>>>>>>>>>>>>> + write_rst_set(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>>> + write_rst_clr(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> const struct clk_ops tegra_clk_periph_gate_ops = {
>>>>>>>>>>>>>>> .is_enabled = clk_periph_is_enabled,
>>>>>>>>>>>>>>> .enable = clk_periph_enable,
>>>>>>>>>>>>>>> .disable = clk_periph_disable,
>>>>>>>>>>>>>>> + .save_context = clk_periph_gate_save_context,
>>>>>>>>>>>>>>> + .restore_context = clk_periph_gate_restore_context,
>>>>>>>>>>>>>>> };
>>>>>>>>>>>>>>> struct clk *tegra_clk_register_periph_gate(const
>>>>>>>>>>>>>>> char *name,
>>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>> index 58437da25156..06fb62955768 100644
>>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>> @@ -99,6 +99,37 @@ static void clk_periph_disable(struct
>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>> gate_ops->disable(gate_hw);
>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>> +static int clk_periph_save_context(struct clk_hw *hw)
>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>> + struct tegra_clk_periph *periph = to_clk_periph(hw);
>>>>>>>>>>>>>>> + const struct clk_ops *gate_ops = periph->gate_ops;
>>>>>>>>>>>>>>> + struct clk_hw *gate_hw = &periph->gate.hw;
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + if (!(periph->gate.flags & TEGRA_PERIPH_NO_GATE))
>>>>>>>>>>>>>>> + gate_ops->save_context(gate_hw);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + periph->parent_ctx = clk_periph_get_parent(hw);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> +static void clk_periph_restore_context(struct clk_hw *hw)
>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>> + struct tegra_clk_periph *periph = to_clk_periph(hw);
>>>>>>>>>>>>>>> + const struct clk_ops *gate_ops = periph->gate_ops;
>>>>>>>>>>>>>>> + struct clk_hw *gate_hw = &periph->gate.hw;
>>>>>>>>>>>>>>> + const struct clk_ops *div_ops = periph->div_ops;
>>>>>>>>>>>>>>> + struct clk_hw *div_hw = &periph->divider.hw;
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + clk_periph_set_parent(hw, periph->parent_ctx);
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> + if (!(periph->gate.flags & TEGRA_PERIPH_NO_DIV))
>>>>>>>>>>>>>>> + div_ops->restore_context(div_hw);
>>>>>>>>>>>>>> Could you please point to where the divider's save_context()
>>>>>>>>>>>>>> happens?
>>>>>>>>>>>>>> Because I can't see it.
>>>>>>>>>>>>> Ah, I now see that there is no need to save the dividers
>>>>>>>>>>>>> context
>>>>>>>>>>>>> because
>>>>>>>>>>>>> clk itself has enough info that is needed for the context's
>>>>>>>>>>>>> restoring
>>>>>>>>>>>>> (like I pointed in the review to v6).
>>>>>>>>>>>>>
>>>>>>>>>>>>> Looks like you could also implement a new
>>>>>>>>>>>>> clk_hw_get_parent_index()
>>>>>>>>>>>>> generic helper to get the index instead of storing it
>>>>>>>>>>>>> manually.
>>>>>>>>>>>> clk_periph_get_parent basically invokes existing clk_mux_ops
>>>>>>>>>>>> get_parent() which is then saved in tegra_clk_periph.
>>>>>>>>>>>>
>>>>>>>>>>>> All existing drivers are using directly get_parent() from
>>>>>>>>>>>> clk_mux
>>>>>>>>>>>> which actually gets index from the register read.
>>>>>>>>>>>>
>>>>>>>>>>>> To have this more generic w.r.t save/restore context point of
>>>>>>>>>>>> view,
>>>>>>>>>>>> probably instead of implementing new get_parent_index helper,
>>>>>>>>>>>> I think
>>>>>>>>>>>> its better to implement save_context and restore_context to
>>>>>>>>>>>> clk_mux_ops along with creating parent_index field into
>>>>>>>>>>>> clk_mux to
>>>>>>>>>>>> cache index during set_parent.
>>>>>>>>>>>>
>>>>>>>>>>>> So we just need to invoke mux_ops save_context and
>>>>>>>>>>>> restore_context.
>>>>>>>>>>>>
>>>>>>>>>>> I hope its ok to add save/restore context to clk_mux_ops to be
>>>>>>>>>>> more
>>>>>>>>>>> generic w.r.t save/restore context rather than get_parent_index
>>>>>>>>>>> API.
>>>>>>>>>>> Please confirm if you agree.
>>>>>>>>>> Sounds like a good idea. I see that there is a 'restoring'
>>>>>>>>>> helper for
>>>>>>>>>> the generic clk_gate, seems something similar could be done
>>>>>>>>>> for the
>>>>>>>>>> clk_mux. And looks like anyway you'll need to associate the
>>>>>>>>>> parent
>>>>>>>>>> clock
>>>>>>>>>> with the hw index in order to restore the muxing.
>>>>>>>>> by 'restoring' helper for generic clk_gate, are you referring to
>>>>>>>>> clk_gate_restore_context API?
>>>>>>>> Yes.
>>>>>>>>
>>>>>>>>> clk_gate_restore_context is API that's any clk drivers can use for
>>>>>>>>> clk_gate operation restore for custom gate clk_ops.
>>>>>>>>>
>>>>>>>>> But clk-periph is directly using generic clk_mux ops from clk_mux
>>>>>>>>> so I
>>>>>>>>> think we should add .restore_context to clk_mux_ops and then
>>>>>>>>> during
>>>>>>>>> clk-periph restore need to invoke mux_ops->restore_context.
>>>>>>>> I'm not sure whether it will be good for every driver that uses
>>>>>>>> generic
>>>>>>>> clk_mux ops. Should be more flexible to have a generic helper
>>>>>>>> function
>>>>>>>> that any driver could use in order to restore the clock's parent.
>>>>>>>>
>>>>>>>> The clk-periph restoring also includes case of combining divider
>>>>>>>> and
>>>>>>>> parent restoring, so generic helper could be useful in that case
>>>>>>>> as well.
>>>>>>>>
>>>>>>>> It also looks like you could actually use the
>>>>>>>> clk_gate_restore_context()
>>>>>>>> instead of manually saving the clock's enable-state, couldn't you?
>>>>>>> ok for clk_mux, can add generic clk_mux_restore_context API rather
>>>>>>> than
>>>>>>> using restore_context in clk_ops and will invoke that during
>>>>>>> clk_periph
>>>>>>> restore.
>>>>>>>
>>> digging thru looks like for clk_periph source restore instead of
>>> clk_mux_restore_context, i can directly do clk_hw_get_parent and
>>> clk_set_parent with mux_hw as they invoke mux_ops get/set parent anyway.
>>> Will do this for periph clk mux
>>>>>>> Reg clk_gate, looks like we cant use generic
>>>>>>> clk_gate_restore_context
>>>>>>> for clk-periph as it calls enable/disable callbacks and
>>>>>>> clk_periph_enable/disable in clk-periph-gate also updated refcnt and
>>>>>>> depending on that actual enable/disable is set.
>>>>>>>
>>>>>>> During suspend, peripherals that are already enabled have their
>>>>>>> refcnt >
>>>>>>> 1, so they dont go thru enable/disable on restore if we use same
>>>>>>> enable/disable callback.
>>>>>> Looks like you could just decrement the gate's enable_refcnt on
>>>>>> save_context, wouldn't that work?
>>>>>>
>>>> gate->enable_refcnt is within clk-periph-gate which gets updated when
>>>> enable/disable callbacks get execute thru clk_core_enable/disable.
>>>> But actual enable_count used in clk_gate_restore_context is the one
>>>> which gets updated with in the clk core enable/disable functions which
>>>> invokes these callbacks. Depending on this enable_count in clk core it
>>>> invokes enable/disable.
>>>>
>>>> So, this will cause mismatch if we handle refcnt during save/restore
>>>> of tegra_clk_periph_gate_ops and also enable/disable thru this
>>>> clk_gate_restore_context is based on enable_count from clk core.
>>>>
>>>>>>> Also to align exact reset state along with CLK (like for case where
>>>>>>> CLK
>>>>>>> is enabled but peripheral might be in reset state), implemented
>>>>>>> save/restore in tegra specific tegra_clk_periph_gate_ops
>>>>>> I'm wondering whether instead of saving/restoring reset-state of
>>>>>> every
>>>>>> clock, you could simply save/restore the whole RST_DEV_x_SET
>>>>>> register.
>>>>>> Couldn't you?
>>>>> Thats what I was doing in first version of patch. But later as we
>>>>> moved to use clk_save_context and clk_restore_context, peripheral
>>>>> clk_hw RST & CLK enables happen thru its corresponding save/restore
>>>>> after source restore
>>>>
>>>> Also, to align both CLK & RST to the exact state of register, doing
>>>> save/restore in tegra_clk_periph_gate_ops and invoking this after
>>>> source restore for peripheral clock, seems cleaner to avoid any
>>>> misconfiguration b/w rst & clk settings.
>>>>
>> It looks to me that it is very wasteful to store/restore each individual
>> gate and reset state, also given that some of them are shared. I think
>> that the gates and resets should be restored separately for the
>> peripherals by a custom tegra_clk_save/restore_periph_gates/resets().
> clk_periph_fixed_disable just disables clock only without deasserting
> the corresponding peripheral.
>
> corresponding peripheral drivers can also issue reset assert/deassert
> thru reset_control_assert/deassert.
>
> So, we will not get the actual state of clk and rst unless we read and
> save state of reset and clock separately during save_context.
>
> Currently patch is already using custom
> tegra_clk_periph_fixed_save/restore_context for corresponding clk_ops.
> Are you suggesting to do save and restore of complete CLK_ENB/RST_DEV
> register settings instead of individual peripheral bits?
Yes, I'm suggesting to do a complete ungate/reset handling of the
devices in a separate function. All enabling/deassertion will be done in
a single hop, hence using 7us delay and four u32 words, which is much
nicer IMHO.
^ permalink raw reply
* Re: [PATCH v7 07/20] clk: tegra: clk-periph: Add save and restore support
From: Dmitry Osipenko @ 2019-08-02 20:17 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree
In-Reply-To: <a64472fd-46b7-5ff9-3140-11f71d5f88ff@gmail.com>
02.08.2019 23:13, Dmitry Osipenko пишет:
> 02.08.2019 21:33, Sowjanya Komatineni пишет:
>>
>> On 8/2/19 5:38 AM, Dmitry Osipenko wrote:
>>> 02.08.2019 2:49, Sowjanya Komatineni пишет:
>>>> On 8/1/19 4:19 PM, Sowjanya Komatineni wrote:
>>>>> On 8/1/19 2:30 PM, Sowjanya Komatineni wrote:
>>>>>> On 8/1/19 1:54 PM, Dmitry Osipenko wrote:
>>>>>>> 01.08.2019 23:31, Sowjanya Komatineni пишет:
>>>>>>>> On 8/1/19 1:17 PM, Dmitry Osipenko wrote:
>>>>>>>>> 01.08.2019 22:42, Sowjanya Komatineni пишет:
>>>>>>>>>> On 8/1/19 12:00 PM, Dmitry Osipenko wrote:
>>>>>>>>>>> 01.08.2019 20:58, Sowjanya Komatineni пишет:
>>>>>>>>>>>> On 7/31/19 4:09 PM, Sowjanya Komatineni wrote:
>>>>>>>>>>>>> On 7/31/19 3:44 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>> 31.07.2019 12:50, Dmitry Osipenko пишет:
>>>>>>>>>>>>>>> 31.07.2019 3:20, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>> This patch implements save and restore context for
>>>>>>>>>>>>>>>> peripheral
>>>>>>>>>>>>>>>> fixed
>>>>>>>>>>>>>>>> clock ops, peripheral gate clock ops, sdmmc mux clock
>>>>>>>>>>>>>>>> ops, and
>>>>>>>>>>>>>>>> peripheral clock ops.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> During system suspend, core power goes off and looses the
>>>>>>>>>>>>>>>> settings
>>>>>>>>>>>>>>>> of the Tegra CAR controller registers.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> So during suspend entry clock and reset state of
>>>>>>>>>>>>>>>> peripherals is
>>>>>>>>>>>>>>>> saved
>>>>>>>>>>>>>>>> and on resume they are restored to have clocks back to same
>>>>>>>>>>>>>>>> rate and
>>>>>>>>>>>>>>>> state as before suspend.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>>>>>>>>>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph-fixed.c | 33
>>>>>>>>>>>>>>>> ++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph-gate.c | 34
>>>>>>>>>>>>>>>> +++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph.c | 37
>>>>>>>>>>>>>>>> ++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-sdmmc-mux.c | 28
>>>>>>>>>>>>>>>> +++++++++++++++++++++++++++
>>>>>>>>>>>>>>>> drivers/clk/tegra/clk.h | 6 ++++++
>>>>>>>>>>>>>>>> 5 files changed, 138 insertions(+)
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>> index c088e7a280df..21b24530fa00 100644
>>>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>> @@ -60,11 +60,44 @@
>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_recalc_rate(struct
>>>>>>>>>>>>>>>> clk_hw *hw,
>>>>>>>>>>>>>>>> return (unsigned long)rate;
>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>> +static int tegra_clk_periph_fixed_save_context(struct
>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>> + struct tegra_clk_periph_fixed *fixed =
>>>>>>>>>>>>>>>> to_tegra_clk_periph_fixed(hw);
>>>>>>>>>>>>>>>> + u32 mask = 1 << (fixed->num % 32);
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + fixed->enb_ctx = readl_relaxed(fixed->base +
>>>>>>>>>>>>>>>> fixed->regs->enb_reg) &
>>>>>>>>>>>>>>>> + mask;
>>>>>>>>>>>>>>>> + fixed->rst_ctx = readl_relaxed(fixed->base +
>>>>>>>>>>>>>>>> fixed->regs->rst_reg) &
>>>>>>>>>>>>>>>> + mask;
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> +static void tegra_clk_periph_fixed_restore_context(struct
>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>> + struct tegra_clk_periph_fixed *fixed =
>>>>>>>>>>>>>>>> to_tegra_clk_periph_fixed(hw);
>>>>>>>>>>>>>>>> + u32 mask = 1 << (fixed->num % 32);
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + if (fixed->enb_ctx)
>>>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>>>> fixed->regs->enb_set_reg);
>>>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>>>> fixed->regs->enb_clr_reg);
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + udelay(2);
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + if (!fixed->rst_ctx) {
>>>>>>>>>>>>>>>> + udelay(5); /* reset propogation delay */
>>>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>>>> fixed->regs->rst_reg);
>>>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> static const struct clk_ops tegra_clk_periph_fixed_ops
>>>>>>>>>>>>>>>> = {
>>>>>>>>>>>>>>>> .is_enabled = tegra_clk_periph_fixed_is_enabled,
>>>>>>>>>>>>>>>> .enable = tegra_clk_periph_fixed_enable,
>>>>>>>>>>>>>>>> .disable = tegra_clk_periph_fixed_disable,
>>>>>>>>>>>>>>>> .recalc_rate = tegra_clk_periph_fixed_recalc_rate,
>>>>>>>>>>>>>>>> + .save_context = tegra_clk_periph_fixed_save_context,
>>>>>>>>>>>>>>>> + .restore_context =
>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_restore_context,
>>>>>>>>>>>>>>>> };
>>>>>>>>>>>>>>>> struct clk *tegra_clk_register_periph_fixed(const
>>>>>>>>>>>>>>>> char
>>>>>>>>>>>>>>>> *name,
>>>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>> index 4b31beefc9fc..6ba5b08e0787 100644
>>>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>> @@ -25,6 +25,8 @@ static DEFINE_SPINLOCK(periph_ref_lock);
>>>>>>>>>>>>>>>> #define read_rst(gate) \
>>>>>>>>>>>>>>>> readl_relaxed(gate->clk_base +
>>>>>>>>>>>>>>>> (gate->regs->rst_reg))
>>>>>>>>>>>>>>>> +#define write_rst_set(val, gate) \
>>>>>>>>>>>>>>>> + writel_relaxed(val, gate->clk_base +
>>>>>>>>>>>>>>>> (gate->regs->rst_set_reg))
>>>>>>>>>>>>>>>> #define write_rst_clr(val, gate) \
>>>>>>>>>>>>>>>> writel_relaxed(val, gate->clk_base +
>>>>>>>>>>>>>>>> (gate->regs->rst_clr_reg))
>>>>>>>>>>>>>>>> @@ -110,10 +112,42 @@ static void
>>>>>>>>>>>>>>>> clk_periph_disable(struct
>>>>>>>>>>>>>>>> clk_hw *hw)
>>>>>>>>>>>>>>>> spin_unlock_irqrestore(&periph_ref_lock, flags);
>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>> +static int clk_periph_gate_save_context(struct clk_hw
>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>> + struct tegra_clk_periph_gate *gate =
>>>>>>>>>>>>>>>> to_clk_periph_gate(hw);
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + gate->clk_state_ctx = read_enb(gate) &
>>>>>>>>>>>>>>>> periph_clk_to_bit(gate);
>>>>>>>>>>>>>>>> + gate->rst_state_ctx = read_rst(gate) &
>>>>>>>>>>>>>>>> periph_clk_to_bit(gate);
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> +static void clk_periph_gate_restore_context(struct clk_hw
>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>> + struct tegra_clk_periph_gate *gate =
>>>>>>>>>>>>>>>> to_clk_periph_gate(hw);
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + if (gate->clk_state_ctx)
>>>>>>>>>>>>>>>> + write_enb_set(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>>>> + write_enb_clr(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + udelay(5);
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + if (!(gate->flags & TEGRA_PERIPH_NO_RESET) &&
>>>>>>>>>>>>>>>> + !(gate->flags & TEGRA_PERIPH_MANUAL_RESET)) {
>>>>>>>>>>>>>>>> + if (gate->rst_state_ctx)
>>>>>>>>>>>>>>>> + write_rst_set(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>>>> + write_rst_clr(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> const struct clk_ops tegra_clk_periph_gate_ops = {
>>>>>>>>>>>>>>>> .is_enabled = clk_periph_is_enabled,
>>>>>>>>>>>>>>>> .enable = clk_periph_enable,
>>>>>>>>>>>>>>>> .disable = clk_periph_disable,
>>>>>>>>>>>>>>>> + .save_context = clk_periph_gate_save_context,
>>>>>>>>>>>>>>>> + .restore_context = clk_periph_gate_restore_context,
>>>>>>>>>>>>>>>> };
>>>>>>>>>>>>>>>> struct clk *tegra_clk_register_periph_gate(const
>>>>>>>>>>>>>>>> char *name,
>>>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>> index 58437da25156..06fb62955768 100644
>>>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>> @@ -99,6 +99,37 @@ static void clk_periph_disable(struct
>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>> gate_ops->disable(gate_hw);
>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>> +static int clk_periph_save_context(struct clk_hw *hw)
>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>> + struct tegra_clk_periph *periph = to_clk_periph(hw);
>>>>>>>>>>>>>>>> + const struct clk_ops *gate_ops = periph->gate_ops;
>>>>>>>>>>>>>>>> + struct clk_hw *gate_hw = &periph->gate.hw;
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + if (!(periph->gate.flags & TEGRA_PERIPH_NO_GATE))
>>>>>>>>>>>>>>>> + gate_ops->save_context(gate_hw);
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + periph->parent_ctx = clk_periph_get_parent(hw);
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> +static void clk_periph_restore_context(struct clk_hw *hw)
>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>> + struct tegra_clk_periph *periph = to_clk_periph(hw);
>>>>>>>>>>>>>>>> + const struct clk_ops *gate_ops = periph->gate_ops;
>>>>>>>>>>>>>>>> + struct clk_hw *gate_hw = &periph->gate.hw;
>>>>>>>>>>>>>>>> + const struct clk_ops *div_ops = periph->div_ops;
>>>>>>>>>>>>>>>> + struct clk_hw *div_hw = &periph->divider.hw;
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + clk_periph_set_parent(hw, periph->parent_ctx);
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> + if (!(periph->gate.flags & TEGRA_PERIPH_NO_DIV))
>>>>>>>>>>>>>>>> + div_ops->restore_context(div_hw);
>>>>>>>>>>>>>>> Could you please point to where the divider's save_context()
>>>>>>>>>>>>>>> happens?
>>>>>>>>>>>>>>> Because I can't see it.
>>>>>>>>>>>>>> Ah, I now see that there is no need to save the dividers
>>>>>>>>>>>>>> context
>>>>>>>>>>>>>> because
>>>>>>>>>>>>>> clk itself has enough info that is needed for the context's
>>>>>>>>>>>>>> restoring
>>>>>>>>>>>>>> (like I pointed in the review to v6).
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Looks like you could also implement a new
>>>>>>>>>>>>>> clk_hw_get_parent_index()
>>>>>>>>>>>>>> generic helper to get the index instead of storing it
>>>>>>>>>>>>>> manually.
>>>>>>>>>>>>> clk_periph_get_parent basically invokes existing clk_mux_ops
>>>>>>>>>>>>> get_parent() which is then saved in tegra_clk_periph.
>>>>>>>>>>>>>
>>>>>>>>>>>>> All existing drivers are using directly get_parent() from
>>>>>>>>>>>>> clk_mux
>>>>>>>>>>>>> which actually gets index from the register read.
>>>>>>>>>>>>>
>>>>>>>>>>>>> To have this more generic w.r.t save/restore context point of
>>>>>>>>>>>>> view,
>>>>>>>>>>>>> probably instead of implementing new get_parent_index helper,
>>>>>>>>>>>>> I think
>>>>>>>>>>>>> its better to implement save_context and restore_context to
>>>>>>>>>>>>> clk_mux_ops along with creating parent_index field into
>>>>>>>>>>>>> clk_mux to
>>>>>>>>>>>>> cache index during set_parent.
>>>>>>>>>>>>>
>>>>>>>>>>>>> So we just need to invoke mux_ops save_context and
>>>>>>>>>>>>> restore_context.
>>>>>>>>>>>>>
>>>>>>>>>>>> I hope its ok to add save/restore context to clk_mux_ops to be
>>>>>>>>>>>> more
>>>>>>>>>>>> generic w.r.t save/restore context rather than get_parent_index
>>>>>>>>>>>> API.
>>>>>>>>>>>> Please confirm if you agree.
>>>>>>>>>>> Sounds like a good idea. I see that there is a 'restoring'
>>>>>>>>>>> helper for
>>>>>>>>>>> the generic clk_gate, seems something similar could be done
>>>>>>>>>>> for the
>>>>>>>>>>> clk_mux. And looks like anyway you'll need to associate the
>>>>>>>>>>> parent
>>>>>>>>>>> clock
>>>>>>>>>>> with the hw index in order to restore the muxing.
>>>>>>>>>> by 'restoring' helper for generic clk_gate, are you referring to
>>>>>>>>>> clk_gate_restore_context API?
>>>>>>>>> Yes.
>>>>>>>>>
>>>>>>>>>> clk_gate_restore_context is API that's any clk drivers can use for
>>>>>>>>>> clk_gate operation restore for custom gate clk_ops.
>>>>>>>>>>
>>>>>>>>>> But clk-periph is directly using generic clk_mux ops from clk_mux
>>>>>>>>>> so I
>>>>>>>>>> think we should add .restore_context to clk_mux_ops and then
>>>>>>>>>> during
>>>>>>>>>> clk-periph restore need to invoke mux_ops->restore_context.
>>>>>>>>> I'm not sure whether it will be good for every driver that uses
>>>>>>>>> generic
>>>>>>>>> clk_mux ops. Should be more flexible to have a generic helper
>>>>>>>>> function
>>>>>>>>> that any driver could use in order to restore the clock's parent.
>>>>>>>>>
>>>>>>>>> The clk-periph restoring also includes case of combining divider
>>>>>>>>> and
>>>>>>>>> parent restoring, so generic helper could be useful in that case
>>>>>>>>> as well.
>>>>>>>>>
>>>>>>>>> It also looks like you could actually use the
>>>>>>>>> clk_gate_restore_context()
>>>>>>>>> instead of manually saving the clock's enable-state, couldn't you?
>>>>>>>> ok for clk_mux, can add generic clk_mux_restore_context API rather
>>>>>>>> than
>>>>>>>> using restore_context in clk_ops and will invoke that during
>>>>>>>> clk_periph
>>>>>>>> restore.
>>>>>>>>
>>>> digging thru looks like for clk_periph source restore instead of
>>>> clk_mux_restore_context, i can directly do clk_hw_get_parent and
>>>> clk_set_parent with mux_hw as they invoke mux_ops get/set parent anyway.
>>>> Will do this for periph clk mux
>>>>>>>> Reg clk_gate, looks like we cant use generic
>>>>>>>> clk_gate_restore_context
>>>>>>>> for clk-periph as it calls enable/disable callbacks and
>>>>>>>> clk_periph_enable/disable in clk-periph-gate also updated refcnt and
>>>>>>>> depending on that actual enable/disable is set.
>>>>>>>>
>>>>>>>> During suspend, peripherals that are already enabled have their
>>>>>>>> refcnt >
>>>>>>>> 1, so they dont go thru enable/disable on restore if we use same
>>>>>>>> enable/disable callback.
>>>>>>> Looks like you could just decrement the gate's enable_refcnt on
>>>>>>> save_context, wouldn't that work?
>>>>>>>
>>>>> gate->enable_refcnt is within clk-periph-gate which gets updated when
>>>>> enable/disable callbacks get execute thru clk_core_enable/disable.
>>>>> But actual enable_count used in clk_gate_restore_context is the one
>>>>> which gets updated with in the clk core enable/disable functions which
>>>>> invokes these callbacks. Depending on this enable_count in clk core it
>>>>> invokes enable/disable.
>>>>>
>>>>> So, this will cause mismatch if we handle refcnt during save/restore
>>>>> of tegra_clk_periph_gate_ops and also enable/disable thru this
>>>>> clk_gate_restore_context is based on enable_count from clk core.
>>>>>
>>>>>>>> Also to align exact reset state along with CLK (like for case where
>>>>>>>> CLK
>>>>>>>> is enabled but peripheral might be in reset state), implemented
>>>>>>>> save/restore in tegra specific tegra_clk_periph_gate_ops
>>>>>>> I'm wondering whether instead of saving/restoring reset-state of
>>>>>>> every
>>>>>>> clock, you could simply save/restore the whole RST_DEV_x_SET
>>>>>>> register.
>>>>>>> Couldn't you?
>>>>>> Thats what I was doing in first version of patch. But later as we
>>>>>> moved to use clk_save_context and clk_restore_context, peripheral
>>>>>> clk_hw RST & CLK enables happen thru its corresponding save/restore
>>>>>> after source restore
>>>>>
>>>>> Also, to align both CLK & RST to the exact state of register, doing
>>>>> save/restore in tegra_clk_periph_gate_ops and invoking this after
>>>>> source restore for peripheral clock, seems cleaner to avoid any
>>>>> misconfiguration b/w rst & clk settings.
>>>>>
>>> It looks to me that it is very wasteful to store/restore each individual
>>> gate and reset state, also given that some of them are shared. I think
>>> that the gates and resets should be restored separately for the
>>> peripherals by a custom tegra_clk_save/restore_periph_gates/resets().
>> clk_periph_fixed_disable just disables clock only without deasserting
>> the corresponding peripheral.
>>
>> corresponding peripheral drivers can also issue reset assert/deassert
>> thru reset_control_assert/deassert.
>>
>> So, we will not get the actual state of clk and rst unless we read and
>> save state of reset and clock separately during save_context.
>>
>> Currently patch is already using custom
>> tegra_clk_periph_fixed_save/restore_context for corresponding clk_ops.
>> Are you suggesting to do save and restore of complete CLK_ENB/RST_DEV
>> register settings instead of individual peripheral bits?
>
> Yes, I'm suggesting to do a complete ungate/reset handling of the
> devices in a separate function. All enabling/deassertion will be done in
> a single hop, hence using 7us delay and four u32 words, which is much
> nicer IMHO.
Actually six words, three for CLKs and three for RSTs.
^ permalink raw reply
* Re: [PATCH v7 07/20] clk: tegra: clk-periph: Add save and restore support
From: Dmitry Osipenko @ 2019-08-02 20:20 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree
In-Reply-To: <614e3fec-cfa2-9e49-6130-d6de253acf03@nvidia.com>
02.08.2019 21:43, Sowjanya Komatineni пишет:
>
> On 8/2/19 5:32 AM, Dmitry Osipenko wrote:
>> 31.07.2019 3:20, Sowjanya Komatineni пишет:
>>> This patch implements save and restore context for peripheral fixed
>>> clock ops, peripheral gate clock ops, sdmmc mux clock ops, and
>>> peripheral clock ops.
>>>
>>> During system suspend, core power goes off and looses the settings
>>> of the Tegra CAR controller registers.
>>>
>>> So during suspend entry clock and reset state of peripherals is saved
>>> and on resume they are restored to have clocks back to same rate and
>>> state as before suspend.
>>>
>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>> ---
>>> drivers/clk/tegra/clk-periph-fixed.c | 33
>>> ++++++++++++++++++++++++++++++++
>>> drivers/clk/tegra/clk-periph-gate.c | 34
>>> +++++++++++++++++++++++++++++++++
>>> drivers/clk/tegra/clk-periph.c | 37
>>> ++++++++++++++++++++++++++++++++++++
>>> drivers/clk/tegra/clk-sdmmc-mux.c | 28 +++++++++++++++++++++++++++
>>> drivers/clk/tegra/clk.h | 6 ++++++
>>> 5 files changed, 138 insertions(+)
>>>
>>> diff --git a/drivers/clk/tegra/clk-periph-fixed.c
>>> b/drivers/clk/tegra/clk-periph-fixed.c
>>> index c088e7a280df..21b24530fa00 100644
>>> --- a/drivers/clk/tegra/clk-periph-fixed.c
>>> +++ b/drivers/clk/tegra/clk-periph-fixed.c
>>> @@ -60,11 +60,44 @@ tegra_clk_periph_fixed_recalc_rate(struct clk_hw
>>> *hw,
>>> return (unsigned long)rate;
>>> }
>>> +static int tegra_clk_periph_fixed_save_context(struct clk_hw *hw)
>>> +{
>>> + struct tegra_clk_periph_fixed *fixed =
>>> to_tegra_clk_periph_fixed(hw);
>>> + u32 mask = 1 << (fixed->num % 32);
>> This could be BIT(fixed->num % 32).
>>
>>> + fixed->enb_ctx = readl_relaxed(fixed->base +
>>> fixed->regs->enb_reg) &
>>> + mask;
>>> + fixed->rst_ctx = readl_relaxed(fixed->base +
>>> fixed->regs->rst_reg) &
>>> + mask;
>> The enb_ctx/rst_ctx are booleans, while you assigning an integer value
>> here. You're getting away here because bool is an 32bit unsigned int,
>> but you shouldn't rely on it and always explicitly convert to a bool.
>>
>>> + return 0;
>>> +}
>>> +
>>> +static void tegra_clk_periph_fixed_restore_context(struct clk_hw *hw)
>>> +{
>>> + struct tegra_clk_periph_fixed *fixed =
>>> to_tegra_clk_periph_fixed(hw);
>>> + u32 mask = 1 << (fixed->num % 32);
>>> +
>>> + if (fixed->enb_ctx)
>>> + writel_relaxed(mask, fixed->base + fixed->regs->enb_set_reg);
>>> + else
>>> + writel_relaxed(mask, fixed->base + fixed->regs->enb_clr_reg);
>>> +
>>> + udelay(2);
>> Will be better to read out and compare the hardware's state with the
>> restored one, then bail out if the state is unchanged.
>>
>> Shouldn't it be fence_udelay()?
>>
>>> + if (!fixed->rst_ctx) {
>>> + udelay(5); /* reset propogation delay */
>> Why delaying is done before the writing to the reset register?
>
> During SC7 exit, peripheral reset state is set to POR state. So some
> peripherals will already be in reset state and making sure of
> propagation delay before releasing from reset.
>
> It should be rst_clr_reg. will fix in next rev
>
>>
>>> + writel_relaxed(mask, fixed->base + fixed->regs->rst_reg);
>> I'm not quite sure what's going on here, this looks wrong.
>>
>> 1. rst_reg points to RST_DEVICES_x
>> 2. Each bit of RST_DEVICES_x represents the reset-assertion state of
>> each individual device
>> 3. By writing to rst_reg, all (!) devices are deasserted, except the one
>> device which corresponds to the mask
>> 4. The reset is asserted for a single device, while !fixed->rst_ctx
>> means that it actually should be deasserted (?)
>>
>> Apparently you should use rst_set_reg / rst_clr_reg.
> Yes, It should be rst_clr_reg. will fix in next rev
>>> + }
>> What about the case where rst_ctx=true?
>
> ON SC7 exit, state of RST_DEV will be POR state where most peripherals
> will already be in reset state.
>
> Few of them which are not in reset state in POR values are those that
> need to stay de-asserted across the boot states anyway.
Okay, sounds reasonable.
BTW, it would be nice if you could add a brief clarifying comment to the
code for each of the questions asked during of the review.
^ permalink raw reply
* Re: [PATCH v7 07/20] clk: tegra: clk-periph: Add save and restore support
From: Sowjanya Komatineni @ 2019-08-02 20:32 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree
In-Reply-To: <90268663-e5a7-4715-bd1a-31644c2fe9ab@gmail.com>
On 8/2/19 1:17 PM, Dmitry Osipenko wrote:
> 02.08.2019 23:13, Dmitry Osipenko пишет:
>> 02.08.2019 21:33, Sowjanya Komatineni пишет:
>>> On 8/2/19 5:38 AM, Dmitry Osipenko wrote:
>>>> 02.08.2019 2:49, Sowjanya Komatineni пишет:
>>>>> On 8/1/19 4:19 PM, Sowjanya Komatineni wrote:
>>>>>> On 8/1/19 2:30 PM, Sowjanya Komatineni wrote:
>>>>>>> On 8/1/19 1:54 PM, Dmitry Osipenko wrote:
>>>>>>>> 01.08.2019 23:31, Sowjanya Komatineni пишет:
>>>>>>>>> On 8/1/19 1:17 PM, Dmitry Osipenko wrote:
>>>>>>>>>> 01.08.2019 22:42, Sowjanya Komatineni пишет:
>>>>>>>>>>> On 8/1/19 12:00 PM, Dmitry Osipenko wrote:
>>>>>>>>>>>> 01.08.2019 20:58, Sowjanya Komatineni пишет:
>>>>>>>>>>>>> On 7/31/19 4:09 PM, Sowjanya Komatineni wrote:
>>>>>>>>>>>>>> On 7/31/19 3:44 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>> 31.07.2019 12:50, Dmitry Osipenko пишет:
>>>>>>>>>>>>>>>> 31.07.2019 3:20, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>>> This patch implements save and restore context for
>>>>>>>>>>>>>>>>> peripheral
>>>>>>>>>>>>>>>>> fixed
>>>>>>>>>>>>>>>>> clock ops, peripheral gate clock ops, sdmmc mux clock
>>>>>>>>>>>>>>>>> ops, and
>>>>>>>>>>>>>>>>> peripheral clock ops.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> During system suspend, core power goes off and looses the
>>>>>>>>>>>>>>>>> settings
>>>>>>>>>>>>>>>>> of the Tegra CAR controller registers.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> So during suspend entry clock and reset state of
>>>>>>>>>>>>>>>>> peripherals is
>>>>>>>>>>>>>>>>> saved
>>>>>>>>>>>>>>>>> and on resume they are restored to have clocks back to same
>>>>>>>>>>>>>>>>> rate and
>>>>>>>>>>>>>>>>> state as before suspend.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>>>>>>>>>>>>>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph-fixed.c | 33
>>>>>>>>>>>>>>>>> ++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph-gate.c | 34
>>>>>>>>>>>>>>>>> +++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph.c | 37
>>>>>>>>>>>>>>>>> ++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-sdmmc-mux.c | 28
>>>>>>>>>>>>>>>>> +++++++++++++++++++++++++++
>>>>>>>>>>>>>>>>> drivers/clk/tegra/clk.h | 6 ++++++
>>>>>>>>>>>>>>>>> 5 files changed, 138 insertions(+)
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>>> index c088e7a280df..21b24530fa00 100644
>>>>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>>> @@ -60,11 +60,44 @@
>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_recalc_rate(struct
>>>>>>>>>>>>>>>>> clk_hw *hw,
>>>>>>>>>>>>>>>>> return (unsigned long)rate;
>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>> +static int tegra_clk_periph_fixed_save_context(struct
>>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>> + struct tegra_clk_periph_fixed *fixed =
>>>>>>>>>>>>>>>>> to_tegra_clk_periph_fixed(hw);
>>>>>>>>>>>>>>>>> + u32 mask = 1 << (fixed->num % 32);
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> + fixed->enb_ctx = readl_relaxed(fixed->base +
>>>>>>>>>>>>>>>>> fixed->regs->enb_reg) &
>>>>>>>>>>>>>>>>> + mask;
>>>>>>>>>>>>>>>>> + fixed->rst_ctx = readl_relaxed(fixed->base +
>>>>>>>>>>>>>>>>> fixed->regs->rst_reg) &
>>>>>>>>>>>>>>>>> + mask;
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> +static void tegra_clk_periph_fixed_restore_context(struct
>>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>> + struct tegra_clk_periph_fixed *fixed =
>>>>>>>>>>>>>>>>> to_tegra_clk_periph_fixed(hw);
>>>>>>>>>>>>>>>>> + u32 mask = 1 << (fixed->num % 32);
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> + if (fixed->enb_ctx)
>>>>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>>>>> fixed->regs->enb_set_reg);
>>>>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>>>>> fixed->regs->enb_clr_reg);
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> + udelay(2);
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> + if (!fixed->rst_ctx) {
>>>>>>>>>>>>>>>>> + udelay(5); /* reset propogation delay */
>>>>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>>>>> fixed->regs->rst_reg);
>>>>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> static const struct clk_ops tegra_clk_periph_fixed_ops
>>>>>>>>>>>>>>>>> = {
>>>>>>>>>>>>>>>>> .is_enabled = tegra_clk_periph_fixed_is_enabled,
>>>>>>>>>>>>>>>>> .enable = tegra_clk_periph_fixed_enable,
>>>>>>>>>>>>>>>>> .disable = tegra_clk_periph_fixed_disable,
>>>>>>>>>>>>>>>>> .recalc_rate = tegra_clk_periph_fixed_recalc_rate,
>>>>>>>>>>>>>>>>> + .save_context = tegra_clk_periph_fixed_save_context,
>>>>>>>>>>>>>>>>> + .restore_context =
>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_restore_context,
>>>>>>>>>>>>>>>>> };
>>>>>>>>>>>>>>>>> struct clk *tegra_clk_register_periph_fixed(const
>>>>>>>>>>>>>>>>> char
>>>>>>>>>>>>>>>>> *name,
>>>>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>>> index 4b31beefc9fc..6ba5b08e0787 100644
>>>>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>>> @@ -25,6 +25,8 @@ static DEFINE_SPINLOCK(periph_ref_lock);
>>>>>>>>>>>>>>>>> #define read_rst(gate) \
>>>>>>>>>>>>>>>>> readl_relaxed(gate->clk_base +
>>>>>>>>>>>>>>>>> (gate->regs->rst_reg))
>>>>>>>>>>>>>>>>> +#define write_rst_set(val, gate) \
>>>>>>>>>>>>>>>>> + writel_relaxed(val, gate->clk_base +
>>>>>>>>>>>>>>>>> (gate->regs->rst_set_reg))
>>>>>>>>>>>>>>>>> #define write_rst_clr(val, gate) \
>>>>>>>>>>>>>>>>> writel_relaxed(val, gate->clk_base +
>>>>>>>>>>>>>>>>> (gate->regs->rst_clr_reg))
>>>>>>>>>>>>>>>>> @@ -110,10 +112,42 @@ static void
>>>>>>>>>>>>>>>>> clk_periph_disable(struct
>>>>>>>>>>>>>>>>> clk_hw *hw)
>>>>>>>>>>>>>>>>> spin_unlock_irqrestore(&periph_ref_lock, flags);
>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>> +static int clk_periph_gate_save_context(struct clk_hw
>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>> + struct tegra_clk_periph_gate *gate =
>>>>>>>>>>>>>>>>> to_clk_periph_gate(hw);
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> + gate->clk_state_ctx = read_enb(gate) &
>>>>>>>>>>>>>>>>> periph_clk_to_bit(gate);
>>>>>>>>>>>>>>>>> + gate->rst_state_ctx = read_rst(gate) &
>>>>>>>>>>>>>>>>> periph_clk_to_bit(gate);
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> +static void clk_periph_gate_restore_context(struct clk_hw
>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>> + struct tegra_clk_periph_gate *gate =
>>>>>>>>>>>>>>>>> to_clk_periph_gate(hw);
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> + if (gate->clk_state_ctx)
>>>>>>>>>>>>>>>>> + write_enb_set(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>>>>> + write_enb_clr(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> + udelay(5);
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> + if (!(gate->flags & TEGRA_PERIPH_NO_RESET) &&
>>>>>>>>>>>>>>>>> + !(gate->flags & TEGRA_PERIPH_MANUAL_RESET)) {
>>>>>>>>>>>>>>>>> + if (gate->rst_state_ctx)
>>>>>>>>>>>>>>>>> + write_rst_set(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>>>>> + write_rst_clr(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> const struct clk_ops tegra_clk_periph_gate_ops = {
>>>>>>>>>>>>>>>>> .is_enabled = clk_periph_is_enabled,
>>>>>>>>>>>>>>>>> .enable = clk_periph_enable,
>>>>>>>>>>>>>>>>> .disable = clk_periph_disable,
>>>>>>>>>>>>>>>>> + .save_context = clk_periph_gate_save_context,
>>>>>>>>>>>>>>>>> + .restore_context = clk_periph_gate_restore_context,
>>>>>>>>>>>>>>>>> };
>>>>>>>>>>>>>>>>> struct clk *tegra_clk_register_periph_gate(const
>>>>>>>>>>>>>>>>> char *name,
>>>>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>>> index 58437da25156..06fb62955768 100644
>>>>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>>> @@ -99,6 +99,37 @@ static void clk_periph_disable(struct
>>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>> gate_ops->disable(gate_hw);
>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>> +static int clk_periph_save_context(struct clk_hw *hw)
>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>> + struct tegra_clk_periph *periph = to_clk_periph(hw);
>>>>>>>>>>>>>>>>> + const struct clk_ops *gate_ops = periph->gate_ops;
>>>>>>>>>>>>>>>>> + struct clk_hw *gate_hw = &periph->gate.hw;
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> + if (!(periph->gate.flags & TEGRA_PERIPH_NO_GATE))
>>>>>>>>>>>>>>>>> + gate_ops->save_context(gate_hw);
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> + periph->parent_ctx = clk_periph_get_parent(hw);
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> +static void clk_periph_restore_context(struct clk_hw *hw)
>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>> + struct tegra_clk_periph *periph = to_clk_periph(hw);
>>>>>>>>>>>>>>>>> + const struct clk_ops *gate_ops = periph->gate_ops;
>>>>>>>>>>>>>>>>> + struct clk_hw *gate_hw = &periph->gate.hw;
>>>>>>>>>>>>>>>>> + const struct clk_ops *div_ops = periph->div_ops;
>>>>>>>>>>>>>>>>> + struct clk_hw *div_hw = &periph->divider.hw;
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> + clk_periph_set_parent(hw, periph->parent_ctx);
>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>> + if (!(periph->gate.flags & TEGRA_PERIPH_NO_DIV))
>>>>>>>>>>>>>>>>> + div_ops->restore_context(div_hw);
>>>>>>>>>>>>>>>> Could you please point to where the divider's save_context()
>>>>>>>>>>>>>>>> happens?
>>>>>>>>>>>>>>>> Because I can't see it.
>>>>>>>>>>>>>>> Ah, I now see that there is no need to save the dividers
>>>>>>>>>>>>>>> context
>>>>>>>>>>>>>>> because
>>>>>>>>>>>>>>> clk itself has enough info that is needed for the context's
>>>>>>>>>>>>>>> restoring
>>>>>>>>>>>>>>> (like I pointed in the review to v6).
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Looks like you could also implement a new
>>>>>>>>>>>>>>> clk_hw_get_parent_index()
>>>>>>>>>>>>>>> generic helper to get the index instead of storing it
>>>>>>>>>>>>>>> manually.
>>>>>>>>>>>>>> clk_periph_get_parent basically invokes existing clk_mux_ops
>>>>>>>>>>>>>> get_parent() which is then saved in tegra_clk_periph.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> All existing drivers are using directly get_parent() from
>>>>>>>>>>>>>> clk_mux
>>>>>>>>>>>>>> which actually gets index from the register read.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> To have this more generic w.r.t save/restore context point of
>>>>>>>>>>>>>> view,
>>>>>>>>>>>>>> probably instead of implementing new get_parent_index helper,
>>>>>>>>>>>>>> I think
>>>>>>>>>>>>>> its better to implement save_context and restore_context to
>>>>>>>>>>>>>> clk_mux_ops along with creating parent_index field into
>>>>>>>>>>>>>> clk_mux to
>>>>>>>>>>>>>> cache index during set_parent.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> So we just need to invoke mux_ops save_context and
>>>>>>>>>>>>>> restore_context.
>>>>>>>>>>>>>>
>>>>>>>>>>>>> I hope its ok to add save/restore context to clk_mux_ops to be
>>>>>>>>>>>>> more
>>>>>>>>>>>>> generic w.r.t save/restore context rather than get_parent_index
>>>>>>>>>>>>> API.
>>>>>>>>>>>>> Please confirm if you agree.
>>>>>>>>>>>> Sounds like a good idea. I see that there is a 'restoring'
>>>>>>>>>>>> helper for
>>>>>>>>>>>> the generic clk_gate, seems something similar could be done
>>>>>>>>>>>> for the
>>>>>>>>>>>> clk_mux. And looks like anyway you'll need to associate the
>>>>>>>>>>>> parent
>>>>>>>>>>>> clock
>>>>>>>>>>>> with the hw index in order to restore the muxing.
>>>>>>>>>>> by 'restoring' helper for generic clk_gate, are you referring to
>>>>>>>>>>> clk_gate_restore_context API?
>>>>>>>>>> Yes.
>>>>>>>>>>
>>>>>>>>>>> clk_gate_restore_context is API that's any clk drivers can use for
>>>>>>>>>>> clk_gate operation restore for custom gate clk_ops.
>>>>>>>>>>>
>>>>>>>>>>> But clk-periph is directly using generic clk_mux ops from clk_mux
>>>>>>>>>>> so I
>>>>>>>>>>> think we should add .restore_context to clk_mux_ops and then
>>>>>>>>>>> during
>>>>>>>>>>> clk-periph restore need to invoke mux_ops->restore_context.
>>>>>>>>>> I'm not sure whether it will be good for every driver that uses
>>>>>>>>>> generic
>>>>>>>>>> clk_mux ops. Should be more flexible to have a generic helper
>>>>>>>>>> function
>>>>>>>>>> that any driver could use in order to restore the clock's parent.
>>>>>>>>>>
>>>>>>>>>> The clk-periph restoring also includes case of combining divider
>>>>>>>>>> and
>>>>>>>>>> parent restoring, so generic helper could be useful in that case
>>>>>>>>>> as well.
>>>>>>>>>>
>>>>>>>>>> It also looks like you could actually use the
>>>>>>>>>> clk_gate_restore_context()
>>>>>>>>>> instead of manually saving the clock's enable-state, couldn't you?
>>>>>>>>> ok for clk_mux, can add generic clk_mux_restore_context API rather
>>>>>>>>> than
>>>>>>>>> using restore_context in clk_ops and will invoke that during
>>>>>>>>> clk_periph
>>>>>>>>> restore.
>>>>>>>>>
>>>>> digging thru looks like for clk_periph source restore instead of
>>>>> clk_mux_restore_context, i can directly do clk_hw_get_parent and
>>>>> clk_set_parent with mux_hw as they invoke mux_ops get/set parent anyway.
>>>>> Will do this for periph clk mux
>>>>>>>>> Reg clk_gate, looks like we cant use generic
>>>>>>>>> clk_gate_restore_context
>>>>>>>>> for clk-periph as it calls enable/disable callbacks and
>>>>>>>>> clk_periph_enable/disable in clk-periph-gate also updated refcnt and
>>>>>>>>> depending on that actual enable/disable is set.
>>>>>>>>>
>>>>>>>>> During suspend, peripherals that are already enabled have their
>>>>>>>>> refcnt >
>>>>>>>>> 1, so they dont go thru enable/disable on restore if we use same
>>>>>>>>> enable/disable callback.
>>>>>>>> Looks like you could just decrement the gate's enable_refcnt on
>>>>>>>> save_context, wouldn't that work?
>>>>>>>>
>>>>>> gate->enable_refcnt is within clk-periph-gate which gets updated when
>>>>>> enable/disable callbacks get execute thru clk_core_enable/disable.
>>>>>> But actual enable_count used in clk_gate_restore_context is the one
>>>>>> which gets updated with in the clk core enable/disable functions which
>>>>>> invokes these callbacks. Depending on this enable_count in clk core it
>>>>>> invokes enable/disable.
>>>>>>
>>>>>> So, this will cause mismatch if we handle refcnt during save/restore
>>>>>> of tegra_clk_periph_gate_ops and also enable/disable thru this
>>>>>> clk_gate_restore_context is based on enable_count from clk core.
>>>>>>
>>>>>>>>> Also to align exact reset state along with CLK (like for case where
>>>>>>>>> CLK
>>>>>>>>> is enabled but peripheral might be in reset state), implemented
>>>>>>>>> save/restore in tegra specific tegra_clk_periph_gate_ops
>>>>>>>> I'm wondering whether instead of saving/restoring reset-state of
>>>>>>>> every
>>>>>>>> clock, you could simply save/restore the whole RST_DEV_x_SET
>>>>>>>> register.
>>>>>>>> Couldn't you?
>>>>>>> Thats what I was doing in first version of patch. But later as we
>>>>>>> moved to use clk_save_context and clk_restore_context, peripheral
>>>>>>> clk_hw RST & CLK enables happen thru its corresponding save/restore
>>>>>>> after source restore
>>>>>> Also, to align both CLK & RST to the exact state of register, doing
>>>>>> save/restore in tegra_clk_periph_gate_ops and invoking this after
>>>>>> source restore for peripheral clock, seems cleaner to avoid any
>>>>>> misconfiguration b/w rst & clk settings.
>>>>>>
>>>> It looks to me that it is very wasteful to store/restore each individual
>>>> gate and reset state, also given that some of them are shared. I think
>>>> that the gates and resets should be restored separately for the
>>>> peripherals by a custom tegra_clk_save/restore_periph_gates/resets().
>>> clk_periph_fixed_disable just disables clock only without deasserting
>>> the corresponding peripheral.
>>>
>>> corresponding peripheral drivers can also issue reset assert/deassert
>>> thru reset_control_assert/deassert.
>>>
>>> So, we will not get the actual state of clk and rst unless we read and
>>> save state of reset and clock separately during save_context.
>>>
>>> Currently patch is already using custom
>>> tegra_clk_periph_fixed_save/restore_context for corresponding clk_ops.
>>> Are you suggesting to do save and restore of complete CLK_ENB/RST_DEV
>>> register settings instead of individual peripheral bits?
>> Yes, I'm suggesting to do a complete ungate/reset handling of the
>> devices in a separate function. All enabling/deassertion will be done in
>> a single hop, hence using 7us delay and four u32 words, which is much
>> nicer IMHO.
> Actually six words, three for CLKs and three for RSTs.
OK, So with separate function doing complete register save/restore for
clk & rst, we can't do this thru clk_ops save/restore as clk_ops
save_restore happens per peripheral wise. So if we decide to do this,
then this should be invoked in clk-tegra210 driver suspend/resume.
^ permalink raw reply
* Re: [PATCH v7 07/20] clk: tegra: clk-periph: Add save and restore support
From: Sowjanya Komatineni @ 2019-08-02 20:37 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree
In-Reply-To: <92e95688-1984-9967-d47c-57380466a0f2@gmail.com>
On 8/2/19 1:20 PM, Dmitry Osipenko wrote:
> 02.08.2019 21:43, Sowjanya Komatineni пишет:
>> On 8/2/19 5:32 AM, Dmitry Osipenko wrote:
>>> 31.07.2019 3:20, Sowjanya Komatineni пишет:
>>>> This patch implements save and restore context for peripheral fixed
>>>> clock ops, peripheral gate clock ops, sdmmc mux clock ops, and
>>>> peripheral clock ops.
>>>>
>>>> During system suspend, core power goes off and looses the settings
>>>> of the Tegra CAR controller registers.
>>>>
>>>> So during suspend entry clock and reset state of peripherals is saved
>>>> and on resume they are restored to have clocks back to same rate and
>>>> state as before suspend.
>>>>
>>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>>>> ---
>>>> drivers/clk/tegra/clk-periph-fixed.c | 33
>>>> ++++++++++++++++++++++++++++++++
>>>> drivers/clk/tegra/clk-periph-gate.c | 34
>>>> +++++++++++++++++++++++++++++++++
>>>> drivers/clk/tegra/clk-periph.c | 37
>>>> ++++++++++++++++++++++++++++++++++++
>>>> drivers/clk/tegra/clk-sdmmc-mux.c | 28 +++++++++++++++++++++++++++
>>>> drivers/clk/tegra/clk.h | 6 ++++++
>>>> 5 files changed, 138 insertions(+)
>>>>
>>>> diff --git a/drivers/clk/tegra/clk-periph-fixed.c
>>>> b/drivers/clk/tegra/clk-periph-fixed.c
>>>> index c088e7a280df..21b24530fa00 100644
>>>> --- a/drivers/clk/tegra/clk-periph-fixed.c
>>>> +++ b/drivers/clk/tegra/clk-periph-fixed.c
>>>> @@ -60,11 +60,44 @@ tegra_clk_periph_fixed_recalc_rate(struct clk_hw
>>>> *hw,
>>>> return (unsigned long)rate;
>>>> }
>>>> +static int tegra_clk_periph_fixed_save_context(struct clk_hw *hw)
>>>> +{
>>>> + struct tegra_clk_periph_fixed *fixed =
>>>> to_tegra_clk_periph_fixed(hw);
>>>> + u32 mask = 1 << (fixed->num % 32);
>>> This could be BIT(fixed->num % 32).
>>>
>>>> + fixed->enb_ctx = readl_relaxed(fixed->base +
>>>> fixed->regs->enb_reg) &
>>>> + mask;
>>>> + fixed->rst_ctx = readl_relaxed(fixed->base +
>>>> fixed->regs->rst_reg) &
>>>> + mask;
>>> The enb_ctx/rst_ctx are booleans, while you assigning an integer value
>>> here. You're getting away here because bool is an 32bit unsigned int,
>>> but you shouldn't rely on it and always explicitly convert to a bool.
>>>
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static void tegra_clk_periph_fixed_restore_context(struct clk_hw *hw)
>>>> +{
>>>> + struct tegra_clk_periph_fixed *fixed =
>>>> to_tegra_clk_periph_fixed(hw);
>>>> + u32 mask = 1 << (fixed->num % 32);
>>>> +
>>>> + if (fixed->enb_ctx)
>>>> + writel_relaxed(mask, fixed->base + fixed->regs->enb_set_reg);
>>>> + else
>>>> + writel_relaxed(mask, fixed->base + fixed->regs->enb_clr_reg);
>>>> +
>>>> + udelay(2);
>>> Will be better to read out and compare the hardware's state with the
>>> restored one, then bail out if the state is unchanged.
>>>
>>> Shouldn't it be fence_udelay()?
>>>
>>>> + if (!fixed->rst_ctx) {
>>>> + udelay(5); /* reset propogation delay */
>>> Why delaying is done before the writing to the reset register?
>> During SC7 exit, peripheral reset state is set to POR state. So some
>> peripherals will already be in reset state and making sure of
>> propagation delay before releasing from reset.
>>
>> It should be rst_clr_reg. will fix in next rev
>>
>>>> + writel_relaxed(mask, fixed->base + fixed->regs->rst_reg);
>>> I'm not quite sure what's going on here, this looks wrong.
>>>
>>> 1. rst_reg points to RST_DEVICES_x
>>> 2. Each bit of RST_DEVICES_x represents the reset-assertion state of
>>> each individual device
>>> 3. By writing to rst_reg, all (!) devices are deasserted, except the one
>>> device which corresponds to the mask
>>> 4. The reset is asserted for a single device, while !fixed->rst_ctx
>>> means that it actually should be deasserted (?)
>>>
>>> Apparently you should use rst_set_reg / rst_clr_reg.
>> Yes, It should be rst_clr_reg. will fix in next rev
>>>> + }
>>> What about the case where rst_ctx=true?
>> ON SC7 exit, state of RST_DEV will be POR state where most peripherals
>> will already be in reset state.
>>
>> Few of them which are not in reset state in POR values are those that
>> need to stay de-asserted across the boot states anyway.
> Okay, sounds reasonable.
>
> BTW, it would be nice if you could add a brief clarifying comment to the
> code for each of the questions asked during of the review.
OK, Will add comments in code ...
^ permalink raw reply
* Re: [PATCH V6 14/21] clk: tegra210: Add suspend and resume support
From: Sowjanya Komatineni @ 2019-08-02 20:39 UTC (permalink / raw)
To: Stephen Boyd, Dmitry Osipenko, jason, jonathanh, linus.walleij,
marc.zyngier, mark.rutland, stefan, tglx, thierry.reding
Cc: pdeschrijver, pgaikwad, linux-clk, linux-gpio, jckuo, josephl,
talho, linux-tegra, linux-kernel, mperttunen, spatra, robh+dt,
devicetree
In-Reply-To: <20190802175119.1E401217F5@mail.kernel.org>
On 8/2/19 10:51 AM, Stephen Boyd wrote:
> Quoting Dmitry Osipenko (2019-07-22 00:12:17)
>> 22.07.2019 10:09, Dmitry Osipenko пишет:
>>> 22.07.2019 9:52, Sowjanya Komatineni пишет:
>>>> On 7/21/19 11:10 PM, Dmitry Osipenko wrote:
>>>>> 22.07.2019 1:45, Sowjanya Komatineni пишет:
>>>>>> On 7/21/19 2:38 PM, Dmitry Osipenko wrote:
>>>>>>> 21.07.2019 22:40, Sowjanya Komatineni пишет:
>>>>>>>> @@ -2853,9 +2859,8 @@ static int tegra210_enable_pllu(void)
>>>>>>>> reg |= PLL_ENABLE;
>>>>>>>> writel(reg, clk_base + PLLU_BASE);
>>>>>>>> - readl_relaxed_poll_timeout_atomic(clk_base + PLLU_BASE, reg,
>>>>>>>> - reg & PLL_BASE_LOCK, 2, 1000);
>>>>>>>> - if (!(reg & PLL_BASE_LOCK)) {
>>>>>>>> + ret = tegra210_wait_for_mask(&pllu, PLLU_BASE, PLL_BASE_LOCK);
>>>>>>>> + if (ret) {
>>>>>>> Why this is needed? Was there a bug?
>>>>>>>
>>>>>> during resume pllu init is needed and to use same terga210_init_pllu,
>>>>>> poll_timeout_atomic can't be used as its ony for atomic context.
>>>>>>
>>>>>> So changed to use wait_for_mask which should work in both cases.
>>>>> Atomic variant could be used from any context, not sure what do you
>>>>> mean. The 'atomic' part only means that function won't cause scheduling
>>>>> and that's it.
>>>> Sorry, replied incorrect. readx_poll_timeout_atomic uses ktime_get() and
>>>> during resume timekeeping suspend/resume happens later than clock
>>>> suspend/resume. So using tegra210_wait_for_mask.
>>>>
>>>> both timekeeping and clk-tegra210 drivers are registered as syscore but
>>>> not ordered.
>>> Okay, thank you for the clarification.
>>>
>>> [snip]
>>>
>> You should remove the 'iopoll.h' then, since it's not used anymore.
> And also add a comment to this location in the code because it's
> non-obvious that we can't use iopoll here.
>
Actually added comment during function usage instead of during include
as iopoll.h is removed.
Will add additional comment in include section as well highlighting
reason for removal of iopoll.h
^ permalink raw reply
* Re: [PATCH v7 07/20] clk: tegra: clk-periph: Add save and restore support
From: Dmitry Osipenko @ 2019-08-02 21:15 UTC (permalink / raw)
To: Sowjanya Komatineni, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree
In-Reply-To: <c6e1d744-3a7a-fe1b-2c86-a3d49f022232@nvidia.com>
02.08.2019 23:32, Sowjanya Komatineni пишет:
>
> On 8/2/19 1:17 PM, Dmitry Osipenko wrote:
>> 02.08.2019 23:13, Dmitry Osipenko пишет:
>>> 02.08.2019 21:33, Sowjanya Komatineni пишет:
>>>> On 8/2/19 5:38 AM, Dmitry Osipenko wrote:
>>>>> 02.08.2019 2:49, Sowjanya Komatineni пишет:
>>>>>> On 8/1/19 4:19 PM, Sowjanya Komatineni wrote:
>>>>>>> On 8/1/19 2:30 PM, Sowjanya Komatineni wrote:
>>>>>>>> On 8/1/19 1:54 PM, Dmitry Osipenko wrote:
>>>>>>>>> 01.08.2019 23:31, Sowjanya Komatineni пишет:
>>>>>>>>>> On 8/1/19 1:17 PM, Dmitry Osipenko wrote:
>>>>>>>>>>> 01.08.2019 22:42, Sowjanya Komatineni пишет:
>>>>>>>>>>>> On 8/1/19 12:00 PM, Dmitry Osipenko wrote:
>>>>>>>>>>>>> 01.08.2019 20:58, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>> On 7/31/19 4:09 PM, Sowjanya Komatineni wrote:
>>>>>>>>>>>>>>> On 7/31/19 3:44 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>> 31.07.2019 12:50, Dmitry Osipenko пишет:
>>>>>>>>>>>>>>>>> 31.07.2019 3:20, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>>>> This patch implements save and restore context for
>>>>>>>>>>>>>>>>>> peripheral
>>>>>>>>>>>>>>>>>> fixed
>>>>>>>>>>>>>>>>>> clock ops, peripheral gate clock ops, sdmmc mux clock
>>>>>>>>>>>>>>>>>> ops, and
>>>>>>>>>>>>>>>>>> peripheral clock ops.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> During system suspend, core power goes off and looses the
>>>>>>>>>>>>>>>>>> settings
>>>>>>>>>>>>>>>>>> of the Tegra CAR controller registers.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> So during suspend entry clock and reset state of
>>>>>>>>>>>>>>>>>> peripherals is
>>>>>>>>>>>>>>>>>> saved
>>>>>>>>>>>>>>>>>> and on resume they are restored to have clocks back to
>>>>>>>>>>>>>>>>>> same
>>>>>>>>>>>>>>>>>> rate and
>>>>>>>>>>>>>>>>>> state as before suspend.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>>>>>>>>>>>>>>>>> Signed-off-by: Sowjanya Komatineni
>>>>>>>>>>>>>>>>>> <skomatineni@nvidia.com>
>>>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph-fixed.c | 33
>>>>>>>>>>>>>>>>>> ++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph-gate.c | 34
>>>>>>>>>>>>>>>>>> +++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph.c | 37
>>>>>>>>>>>>>>>>>> ++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-sdmmc-mux.c | 28
>>>>>>>>>>>>>>>>>> +++++++++++++++++++++++++++
>>>>>>>>>>>>>>>>>> drivers/clk/tegra/clk.h | 6 ++++++
>>>>>>>>>>>>>>>>>> 5 files changed, 138 insertions(+)
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>>>> index c088e7a280df..21b24530fa00 100644
>>>>>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>>>> @@ -60,11 +60,44 @@
>>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_recalc_rate(struct
>>>>>>>>>>>>>>>>>> clk_hw *hw,
>>>>>>>>>>>>>>>>>> return (unsigned long)rate;
>>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>>> +static int
>>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_save_context(struct
>>>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>> + struct tegra_clk_periph_fixed *fixed =
>>>>>>>>>>>>>>>>>> to_tegra_clk_periph_fixed(hw);
>>>>>>>>>>>>>>>>>> + u32 mask = 1 << (fixed->num % 32);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + fixed->enb_ctx = readl_relaxed(fixed->base +
>>>>>>>>>>>>>>>>>> fixed->regs->enb_reg) &
>>>>>>>>>>>>>>>>>> + mask;
>>>>>>>>>>>>>>>>>> + fixed->rst_ctx = readl_relaxed(fixed->base +
>>>>>>>>>>>>>>>>>> fixed->regs->rst_reg) &
>>>>>>>>>>>>>>>>>> + mask;
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> +static void
>>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_restore_context(struct
>>>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>> + struct tegra_clk_periph_fixed *fixed =
>>>>>>>>>>>>>>>>>> to_tegra_clk_periph_fixed(hw);
>>>>>>>>>>>>>>>>>> + u32 mask = 1 << (fixed->num % 32);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + if (fixed->enb_ctx)
>>>>>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>>>>>> fixed->regs->enb_set_reg);
>>>>>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>>>>>> fixed->regs->enb_clr_reg);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + udelay(2);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + if (!fixed->rst_ctx) {
>>>>>>>>>>>>>>>>>> + udelay(5); /* reset propogation delay */
>>>>>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>>>>>> fixed->regs->rst_reg);
>>>>>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> static const struct clk_ops
>>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_ops
>>>>>>>>>>>>>>>>>> = {
>>>>>>>>>>>>>>>>>> .is_enabled =
>>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_is_enabled,
>>>>>>>>>>>>>>>>>> .enable = tegra_clk_periph_fixed_enable,
>>>>>>>>>>>>>>>>>> .disable = tegra_clk_periph_fixed_disable,
>>>>>>>>>>>>>>>>>> .recalc_rate =
>>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_recalc_rate,
>>>>>>>>>>>>>>>>>> + .save_context = tegra_clk_periph_fixed_save_context,
>>>>>>>>>>>>>>>>>> + .restore_context =
>>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_restore_context,
>>>>>>>>>>>>>>>>>> };
>>>>>>>>>>>>>>>>>> struct clk
>>>>>>>>>>>>>>>>>> *tegra_clk_register_periph_fixed(const
>>>>>>>>>>>>>>>>>> char
>>>>>>>>>>>>>>>>>> *name,
>>>>>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>>>> index 4b31beefc9fc..6ba5b08e0787 100644
>>>>>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>>>> @@ -25,6 +25,8 @@ static
>>>>>>>>>>>>>>>>>> DEFINE_SPINLOCK(periph_ref_lock);
>>>>>>>>>>>>>>>>>> #define read_rst(gate) \
>>>>>>>>>>>>>>>>>> readl_relaxed(gate->clk_base +
>>>>>>>>>>>>>>>>>> (gate->regs->rst_reg))
>>>>>>>>>>>>>>>>>> +#define write_rst_set(val, gate) \
>>>>>>>>>>>>>>>>>> + writel_relaxed(val, gate->clk_base +
>>>>>>>>>>>>>>>>>> (gate->regs->rst_set_reg))
>>>>>>>>>>>>>>>>>> #define write_rst_clr(val, gate) \
>>>>>>>>>>>>>>>>>> writel_relaxed(val, gate->clk_base +
>>>>>>>>>>>>>>>>>> (gate->regs->rst_clr_reg))
>>>>>>>>>>>>>>>>>> @@ -110,10 +112,42 @@ static void
>>>>>>>>>>>>>>>>>> clk_periph_disable(struct
>>>>>>>>>>>>>>>>>> clk_hw *hw)
>>>>>>>>>>>>>>>>>> spin_unlock_irqrestore(&periph_ref_lock, flags);
>>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>>> +static int clk_periph_gate_save_context(struct
>>>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>> + struct tegra_clk_periph_gate *gate =
>>>>>>>>>>>>>>>>>> to_clk_periph_gate(hw);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + gate->clk_state_ctx = read_enb(gate) &
>>>>>>>>>>>>>>>>>> periph_clk_to_bit(gate);
>>>>>>>>>>>>>>>>>> + gate->rst_state_ctx = read_rst(gate) &
>>>>>>>>>>>>>>>>>> periph_clk_to_bit(gate);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> +static void clk_periph_gate_restore_context(struct
>>>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>> + struct tegra_clk_periph_gate *gate =
>>>>>>>>>>>>>>>>>> to_clk_periph_gate(hw);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + if (gate->clk_state_ctx)
>>>>>>>>>>>>>>>>>> + write_enb_set(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>>>>>> + write_enb_clr(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + udelay(5);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + if (!(gate->flags & TEGRA_PERIPH_NO_RESET) &&
>>>>>>>>>>>>>>>>>> + !(gate->flags & TEGRA_PERIPH_MANUAL_RESET)) {
>>>>>>>>>>>>>>>>>> + if (gate->rst_state_ctx)
>>>>>>>>>>>>>>>>>> + write_rst_set(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>>>>>> + write_rst_clr(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> const struct clk_ops tegra_clk_periph_gate_ops = {
>>>>>>>>>>>>>>>>>> .is_enabled = clk_periph_is_enabled,
>>>>>>>>>>>>>>>>>> .enable = clk_periph_enable,
>>>>>>>>>>>>>>>>>> .disable = clk_periph_disable,
>>>>>>>>>>>>>>>>>> + .save_context = clk_periph_gate_save_context,
>>>>>>>>>>>>>>>>>> + .restore_context = clk_periph_gate_restore_context,
>>>>>>>>>>>>>>>>>> };
>>>>>>>>>>>>>>>>>> struct clk *tegra_clk_register_periph_gate(const
>>>>>>>>>>>>>>>>>> char *name,
>>>>>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>>>> index 58437da25156..06fb62955768 100644
>>>>>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>>>> @@ -99,6 +99,37 @@ static void clk_periph_disable(struct
>>>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>>> gate_ops->disable(gate_hw);
>>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>>> +static int clk_periph_save_context(struct
>>>>>>>>>>>>>>>>>> clk_hw *hw)
>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>> + struct tegra_clk_periph *periph = to_clk_periph(hw);
>>>>>>>>>>>>>>>>>> + const struct clk_ops *gate_ops = periph->gate_ops;
>>>>>>>>>>>>>>>>>> + struct clk_hw *gate_hw = &periph->gate.hw;
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + if (!(periph->gate.flags & TEGRA_PERIPH_NO_GATE))
>>>>>>>>>>>>>>>>>> + gate_ops->save_context(gate_hw);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + periph->parent_ctx = clk_periph_get_parent(hw);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> +static void clk_periph_restore_context(struct clk_hw
>>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>> + struct tegra_clk_periph *periph = to_clk_periph(hw);
>>>>>>>>>>>>>>>>>> + const struct clk_ops *gate_ops = periph->gate_ops;
>>>>>>>>>>>>>>>>>> + struct clk_hw *gate_hw = &periph->gate.hw;
>>>>>>>>>>>>>>>>>> + const struct clk_ops *div_ops = periph->div_ops;
>>>>>>>>>>>>>>>>>> + struct clk_hw *div_hw = &periph->divider.hw;
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + clk_periph_set_parent(hw, periph->parent_ctx);
>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>> + if (!(periph->gate.flags & TEGRA_PERIPH_NO_DIV))
>>>>>>>>>>>>>>>>>> + div_ops->restore_context(div_hw);
>>>>>>>>>>>>>>>>> Could you please point to where the divider's
>>>>>>>>>>>>>>>>> save_context()
>>>>>>>>>>>>>>>>> happens?
>>>>>>>>>>>>>>>>> Because I can't see it.
>>>>>>>>>>>>>>>> Ah, I now see that there is no need to save the dividers
>>>>>>>>>>>>>>>> context
>>>>>>>>>>>>>>>> because
>>>>>>>>>>>>>>>> clk itself has enough info that is needed for the context's
>>>>>>>>>>>>>>>> restoring
>>>>>>>>>>>>>>>> (like I pointed in the review to v6).
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Looks like you could also implement a new
>>>>>>>>>>>>>>>> clk_hw_get_parent_index()
>>>>>>>>>>>>>>>> generic helper to get the index instead of storing it
>>>>>>>>>>>>>>>> manually.
>>>>>>>>>>>>>>> clk_periph_get_parent basically invokes existing clk_mux_ops
>>>>>>>>>>>>>>> get_parent() which is then saved in tegra_clk_periph.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> All existing drivers are using directly get_parent() from
>>>>>>>>>>>>>>> clk_mux
>>>>>>>>>>>>>>> which actually gets index from the register read.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> To have this more generic w.r.t save/restore context
>>>>>>>>>>>>>>> point of
>>>>>>>>>>>>>>> view,
>>>>>>>>>>>>>>> probably instead of implementing new get_parent_index
>>>>>>>>>>>>>>> helper,
>>>>>>>>>>>>>>> I think
>>>>>>>>>>>>>>> its better to implement save_context and restore_context to
>>>>>>>>>>>>>>> clk_mux_ops along with creating parent_index field into
>>>>>>>>>>>>>>> clk_mux to
>>>>>>>>>>>>>>> cache index during set_parent.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> So we just need to invoke mux_ops save_context and
>>>>>>>>>>>>>>> restore_context.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I hope its ok to add save/restore context to clk_mux_ops
>>>>>>>>>>>>>> to be
>>>>>>>>>>>>>> more
>>>>>>>>>>>>>> generic w.r.t save/restore context rather than
>>>>>>>>>>>>>> get_parent_index
>>>>>>>>>>>>>> API.
>>>>>>>>>>>>>> Please confirm if you agree.
>>>>>>>>>>>>> Sounds like a good idea. I see that there is a 'restoring'
>>>>>>>>>>>>> helper for
>>>>>>>>>>>>> the generic clk_gate, seems something similar could be done
>>>>>>>>>>>>> for the
>>>>>>>>>>>>> clk_mux. And looks like anyway you'll need to associate the
>>>>>>>>>>>>> parent
>>>>>>>>>>>>> clock
>>>>>>>>>>>>> with the hw index in order to restore the muxing.
>>>>>>>>>>>> by 'restoring' helper for generic clk_gate, are you
>>>>>>>>>>>> referring to
>>>>>>>>>>>> clk_gate_restore_context API?
>>>>>>>>>>> Yes.
>>>>>>>>>>>
>>>>>>>>>>>> clk_gate_restore_context is API that's any clk drivers can
>>>>>>>>>>>> use for
>>>>>>>>>>>> clk_gate operation restore for custom gate clk_ops.
>>>>>>>>>>>>
>>>>>>>>>>>> But clk-periph is directly using generic clk_mux ops from
>>>>>>>>>>>> clk_mux
>>>>>>>>>>>> so I
>>>>>>>>>>>> think we should add .restore_context to clk_mux_ops and then
>>>>>>>>>>>> during
>>>>>>>>>>>> clk-periph restore need to invoke mux_ops->restore_context.
>>>>>>>>>>> I'm not sure whether it will be good for every driver that uses
>>>>>>>>>>> generic
>>>>>>>>>>> clk_mux ops. Should be more flexible to have a generic helper
>>>>>>>>>>> function
>>>>>>>>>>> that any driver could use in order to restore the clock's
>>>>>>>>>>> parent.
>>>>>>>>>>>
>>>>>>>>>>> The clk-periph restoring also includes case of combining divider
>>>>>>>>>>> and
>>>>>>>>>>> parent restoring, so generic helper could be useful in that case
>>>>>>>>>>> as well.
>>>>>>>>>>>
>>>>>>>>>>> It also looks like you could actually use the
>>>>>>>>>>> clk_gate_restore_context()
>>>>>>>>>>> instead of manually saving the clock's enable-state, couldn't
>>>>>>>>>>> you?
>>>>>>>>>> ok for clk_mux, can add generic clk_mux_restore_context API
>>>>>>>>>> rather
>>>>>>>>>> than
>>>>>>>>>> using restore_context in clk_ops and will invoke that during
>>>>>>>>>> clk_periph
>>>>>>>>>> restore.
>>>>>>>>>>
>>>>>> digging thru looks like for clk_periph source restore instead of
>>>>>> clk_mux_restore_context, i can directly do clk_hw_get_parent and
>>>>>> clk_set_parent with mux_hw as they invoke mux_ops get/set parent
>>>>>> anyway.
>>>>>> Will do this for periph clk mux
>>>>>>>>>> Reg clk_gate, looks like we cant use generic
>>>>>>>>>> clk_gate_restore_context
>>>>>>>>>> for clk-periph as it calls enable/disable callbacks and
>>>>>>>>>> clk_periph_enable/disable in clk-periph-gate also updated
>>>>>>>>>> refcnt and
>>>>>>>>>> depending on that actual enable/disable is set.
>>>>>>>>>>
>>>>>>>>>> During suspend, peripherals that are already enabled have their
>>>>>>>>>> refcnt >
>>>>>>>>>> 1, so they dont go thru enable/disable on restore if we use same
>>>>>>>>>> enable/disable callback.
>>>>>>>>> Looks like you could just decrement the gate's enable_refcnt on
>>>>>>>>> save_context, wouldn't that work?
>>>>>>>>>
>>>>>>> gate->enable_refcnt is within clk-periph-gate which gets updated
>>>>>>> when
>>>>>>> enable/disable callbacks get execute thru clk_core_enable/disable.
>>>>>>> But actual enable_count used in clk_gate_restore_context is the one
>>>>>>> which gets updated with in the clk core enable/disable functions
>>>>>>> which
>>>>>>> invokes these callbacks. Depending on this enable_count in clk
>>>>>>> core it
>>>>>>> invokes enable/disable.
>>>>>>>
>>>>>>> So, this will cause mismatch if we handle refcnt during save/restore
>>>>>>> of tegra_clk_periph_gate_ops and also enable/disable thru this
>>>>>>> clk_gate_restore_context is based on enable_count from clk core.
>>>>>>>
>>>>>>>>>> Also to align exact reset state along with CLK (like for case
>>>>>>>>>> where
>>>>>>>>>> CLK
>>>>>>>>>> is enabled but peripheral might be in reset state), implemented
>>>>>>>>>> save/restore in tegra specific tegra_clk_periph_gate_ops
>>>>>>>>> I'm wondering whether instead of saving/restoring reset-state of
>>>>>>>>> every
>>>>>>>>> clock, you could simply save/restore the whole RST_DEV_x_SET
>>>>>>>>> register.
>>>>>>>>> Couldn't you?
>>>>>>>> Thats what I was doing in first version of patch. But later as we
>>>>>>>> moved to use clk_save_context and clk_restore_context, peripheral
>>>>>>>> clk_hw RST & CLK enables happen thru its corresponding save/restore
>>>>>>>> after source restore
>>>>>>> Also, to align both CLK & RST to the exact state of register, doing
>>>>>>> save/restore in tegra_clk_periph_gate_ops and invoking this after
>>>>>>> source restore for peripheral clock, seems cleaner to avoid any
>>>>>>> misconfiguration b/w rst & clk settings.
>>>>>>>
>>>>> It looks to me that it is very wasteful to store/restore each
>>>>> individual
>>>>> gate and reset state, also given that some of them are shared. I think
>>>>> that the gates and resets should be restored separately for the
>>>>> peripherals by a custom tegra_clk_save/restore_periph_gates/resets().
>>>> clk_periph_fixed_disable just disables clock only without deasserting
>>>> the corresponding peripheral.
>>>>
>>>> corresponding peripheral drivers can also issue reset assert/deassert
>>>> thru reset_control_assert/deassert.
>>>>
>>>> So, we will not get the actual state of clk and rst unless we read and
>>>> save state of reset and clock separately during save_context.
>>>>
>>>> Currently patch is already using custom
>>>> tegra_clk_periph_fixed_save/restore_context for corresponding clk_ops.
>>>> Are you suggesting to do save and restore of complete CLK_ENB/RST_DEV
>>>> register settings instead of individual peripheral bits?
>>> Yes, I'm suggesting to do a complete ungate/reset handling of the
>>> devices in a separate function. All enabling/deassertion will be done in
>>> a single hop, hence using 7us delay and four u32 words, which is much
>>> nicer IMHO.
>> Actually six words, three for CLKs and three for RSTs.
>
> OK, So with separate function doing complete register save/restore for
> clk & rst, we can't do this thru clk_ops save/restore as clk_ops
> save_restore happens per peripheral wise. So if we decide to do this,
> then this should be invoked in clk-tegra210 driver suspend/resume.
Yes, per-clock save/restore should be used for setting rate and parent.
The ungating and resetting could be done separately to keep things cleaner.
^ permalink raw reply
* Re: [PATCH v7 07/20] clk: tegra: clk-periph: Add save and restore support
From: Sowjanya Komatineni @ 2019-08-02 21:18 UTC (permalink / raw)
To: Dmitry Osipenko, thierry.reding, jonathanh, tglx, jason,
marc.zyngier, linus.walleij, stefan, mark.rutland
Cc: pdeschrijver, pgaikwad, sboyd, linux-clk, linux-gpio, jckuo,
josephl, talho, linux-tegra, linux-kernel, mperttunen, spatra,
robh+dt, devicetree
In-Reply-To: <73cd521b-782c-7fb2-d904-ae8b07927d47@gmail.com>
On 8/2/19 2:15 PM, Dmitry Osipenko wrote:
> 02.08.2019 23:32, Sowjanya Komatineni пишет:
>> On 8/2/19 1:17 PM, Dmitry Osipenko wrote:
>>> 02.08.2019 23:13, Dmitry Osipenko пишет:
>>>> 02.08.2019 21:33, Sowjanya Komatineni пишет:
>>>>> On 8/2/19 5:38 AM, Dmitry Osipenko wrote:
>>>>>> 02.08.2019 2:49, Sowjanya Komatineni пишет:
>>>>>>> On 8/1/19 4:19 PM, Sowjanya Komatineni wrote:
>>>>>>>> On 8/1/19 2:30 PM, Sowjanya Komatineni wrote:
>>>>>>>>> On 8/1/19 1:54 PM, Dmitry Osipenko wrote:
>>>>>>>>>> 01.08.2019 23:31, Sowjanya Komatineni пишет:
>>>>>>>>>>> On 8/1/19 1:17 PM, Dmitry Osipenko wrote:
>>>>>>>>>>>> 01.08.2019 22:42, Sowjanya Komatineni пишет:
>>>>>>>>>>>>> On 8/1/19 12:00 PM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>> 01.08.2019 20:58, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>> On 7/31/19 4:09 PM, Sowjanya Komatineni wrote:
>>>>>>>>>>>>>>>> On 7/31/19 3:44 AM, Dmitry Osipenko wrote:
>>>>>>>>>>>>>>>>> 31.07.2019 12:50, Dmitry Osipenko пишет:
>>>>>>>>>>>>>>>>>> 31.07.2019 3:20, Sowjanya Komatineni пишет:
>>>>>>>>>>>>>>>>>>> This patch implements save and restore context for
>>>>>>>>>>>>>>>>>>> peripheral
>>>>>>>>>>>>>>>>>>> fixed
>>>>>>>>>>>>>>>>>>> clock ops, peripheral gate clock ops, sdmmc mux clock
>>>>>>>>>>>>>>>>>>> ops, and
>>>>>>>>>>>>>>>>>>> peripheral clock ops.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> During system suspend, core power goes off and looses the
>>>>>>>>>>>>>>>>>>> settings
>>>>>>>>>>>>>>>>>>> of the Tegra CAR controller registers.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> So during suspend entry clock and reset state of
>>>>>>>>>>>>>>>>>>> peripherals is
>>>>>>>>>>>>>>>>>>> saved
>>>>>>>>>>>>>>>>>>> and on resume they are restored to have clocks back to
>>>>>>>>>>>>>>>>>>> same
>>>>>>>>>>>>>>>>>>> rate and
>>>>>>>>>>>>>>>>>>> state as before suspend.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Acked-by: Thierry Reding <treding@nvidia.com>
>>>>>>>>>>>>>>>>>>> Signed-off-by: Sowjanya Komatineni
>>>>>>>>>>>>>>>>>>> <skomatineni@nvidia.com>
>>>>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph-fixed.c | 33
>>>>>>>>>>>>>>>>>>> ++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph-gate.c | 34
>>>>>>>>>>>>>>>>>>> +++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-periph.c | 37
>>>>>>>>>>>>>>>>>>> ++++++++++++++++++++++++++++++++++++
>>>>>>>>>>>>>>>>>>> drivers/clk/tegra/clk-sdmmc-mux.c | 28
>>>>>>>>>>>>>>>>>>> +++++++++++++++++++++++++++
>>>>>>>>>>>>>>>>>>> drivers/clk/tegra/clk.h | 6 ++++++
>>>>>>>>>>>>>>>>>>> 5 files changed, 138 insertions(+)
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>>>>> index c088e7a280df..21b24530fa00 100644
>>>>>>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph-fixed.c
>>>>>>>>>>>>>>>>>>> @@ -60,11 +60,44 @@
>>>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_recalc_rate(struct
>>>>>>>>>>>>>>>>>>> clk_hw *hw,
>>>>>>>>>>>>>>>>>>> return (unsigned long)rate;
>>>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>>>> +static int
>>>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_save_context(struct
>>>>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>>> + struct tegra_clk_periph_fixed *fixed =
>>>>>>>>>>>>>>>>>>> to_tegra_clk_periph_fixed(hw);
>>>>>>>>>>>>>>>>>>> + u32 mask = 1 << (fixed->num % 32);
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> + fixed->enb_ctx = readl_relaxed(fixed->base +
>>>>>>>>>>>>>>>>>>> fixed->regs->enb_reg) &
>>>>>>>>>>>>>>>>>>> + mask;
>>>>>>>>>>>>>>>>>>> + fixed->rst_ctx = readl_relaxed(fixed->base +
>>>>>>>>>>>>>>>>>>> fixed->regs->rst_reg) &
>>>>>>>>>>>>>>>>>>> + mask;
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> +static void
>>>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_restore_context(struct
>>>>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>>> + struct tegra_clk_periph_fixed *fixed =
>>>>>>>>>>>>>>>>>>> to_tegra_clk_periph_fixed(hw);
>>>>>>>>>>>>>>>>>>> + u32 mask = 1 << (fixed->num % 32);
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> + if (fixed->enb_ctx)
>>>>>>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>>>>>>> fixed->regs->enb_set_reg);
>>>>>>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>>>>>>> fixed->regs->enb_clr_reg);
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> + udelay(2);
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> + if (!fixed->rst_ctx) {
>>>>>>>>>>>>>>>>>>> + udelay(5); /* reset propogation delay */
>>>>>>>>>>>>>>>>>>> + writel_relaxed(mask, fixed->base +
>>>>>>>>>>>>>>>>>>> fixed->regs->rst_reg);
>>>>>>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> static const struct clk_ops
>>>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_ops
>>>>>>>>>>>>>>>>>>> = {
>>>>>>>>>>>>>>>>>>> .is_enabled =
>>>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_is_enabled,
>>>>>>>>>>>>>>>>>>> .enable = tegra_clk_periph_fixed_enable,
>>>>>>>>>>>>>>>>>>> .disable = tegra_clk_periph_fixed_disable,
>>>>>>>>>>>>>>>>>>> .recalc_rate =
>>>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_recalc_rate,
>>>>>>>>>>>>>>>>>>> + .save_context = tegra_clk_periph_fixed_save_context,
>>>>>>>>>>>>>>>>>>> + .restore_context =
>>>>>>>>>>>>>>>>>>> tegra_clk_periph_fixed_restore_context,
>>>>>>>>>>>>>>>>>>> };
>>>>>>>>>>>>>>>>>>> struct clk
>>>>>>>>>>>>>>>>>>> *tegra_clk_register_periph_fixed(const
>>>>>>>>>>>>>>>>>>> char
>>>>>>>>>>>>>>>>>>> *name,
>>>>>>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>>>>> index 4b31beefc9fc..6ba5b08e0787 100644
>>>>>>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph-gate.c
>>>>>>>>>>>>>>>>>>> @@ -25,6 +25,8 @@ static
>>>>>>>>>>>>>>>>>>> DEFINE_SPINLOCK(periph_ref_lock);
>>>>>>>>>>>>>>>>>>> #define read_rst(gate) \
>>>>>>>>>>>>>>>>>>> readl_relaxed(gate->clk_base +
>>>>>>>>>>>>>>>>>>> (gate->regs->rst_reg))
>>>>>>>>>>>>>>>>>>> +#define write_rst_set(val, gate) \
>>>>>>>>>>>>>>>>>>> + writel_relaxed(val, gate->clk_base +
>>>>>>>>>>>>>>>>>>> (gate->regs->rst_set_reg))
>>>>>>>>>>>>>>>>>>> #define write_rst_clr(val, gate) \
>>>>>>>>>>>>>>>>>>> writel_relaxed(val, gate->clk_base +
>>>>>>>>>>>>>>>>>>> (gate->regs->rst_clr_reg))
>>>>>>>>>>>>>>>>>>> @@ -110,10 +112,42 @@ static void
>>>>>>>>>>>>>>>>>>> clk_periph_disable(struct
>>>>>>>>>>>>>>>>>>> clk_hw *hw)
>>>>>>>>>>>>>>>>>>> spin_unlock_irqrestore(&periph_ref_lock, flags);
>>>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>>>> +static int clk_periph_gate_save_context(struct
>>>>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>>> + struct tegra_clk_periph_gate *gate =
>>>>>>>>>>>>>>>>>>> to_clk_periph_gate(hw);
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> + gate->clk_state_ctx = read_enb(gate) &
>>>>>>>>>>>>>>>>>>> periph_clk_to_bit(gate);
>>>>>>>>>>>>>>>>>>> + gate->rst_state_ctx = read_rst(gate) &
>>>>>>>>>>>>>>>>>>> periph_clk_to_bit(gate);
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> +static void clk_periph_gate_restore_context(struct
>>>>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>>> + struct tegra_clk_periph_gate *gate =
>>>>>>>>>>>>>>>>>>> to_clk_periph_gate(hw);
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> + if (gate->clk_state_ctx)
>>>>>>>>>>>>>>>>>>> + write_enb_set(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>>>>>>> + write_enb_clr(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> + udelay(5);
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> + if (!(gate->flags & TEGRA_PERIPH_NO_RESET) &&
>>>>>>>>>>>>>>>>>>> + !(gate->flags & TEGRA_PERIPH_MANUAL_RESET)) {
>>>>>>>>>>>>>>>>>>> + if (gate->rst_state_ctx)
>>>>>>>>>>>>>>>>>>> + write_rst_set(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>>>>> + else
>>>>>>>>>>>>>>>>>>> + write_rst_clr(periph_clk_to_bit(gate), gate);
>>>>>>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> const struct clk_ops tegra_clk_periph_gate_ops = {
>>>>>>>>>>>>>>>>>>> .is_enabled = clk_periph_is_enabled,
>>>>>>>>>>>>>>>>>>> .enable = clk_periph_enable,
>>>>>>>>>>>>>>>>>>> .disable = clk_periph_disable,
>>>>>>>>>>>>>>>>>>> + .save_context = clk_periph_gate_save_context,
>>>>>>>>>>>>>>>>>>> + .restore_context = clk_periph_gate_restore_context,
>>>>>>>>>>>>>>>>>>> };
>>>>>>>>>>>>>>>>>>> struct clk *tegra_clk_register_periph_gate(const
>>>>>>>>>>>>>>>>>>> char *name,
>>>>>>>>>>>>>>>>>>> diff --git a/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>>>>> b/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>>>>> index 58437da25156..06fb62955768 100644
>>>>>>>>>>>>>>>>>>> --- a/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>>>>> +++ b/drivers/clk/tegra/clk-periph.c
>>>>>>>>>>>>>>>>>>> @@ -99,6 +99,37 @@ static void clk_periph_disable(struct
>>>>>>>>>>>>>>>>>>> clk_hw
>>>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>>>> gate_ops->disable(gate_hw);
>>>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>>>> +static int clk_periph_save_context(struct
>>>>>>>>>>>>>>>>>>> clk_hw *hw)
>>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>>> + struct tegra_clk_periph *periph = to_clk_periph(hw);
>>>>>>>>>>>>>>>>>>> + const struct clk_ops *gate_ops = periph->gate_ops;
>>>>>>>>>>>>>>>>>>> + struct clk_hw *gate_hw = &periph->gate.hw;
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> + if (!(periph->gate.flags & TEGRA_PERIPH_NO_GATE))
>>>>>>>>>>>>>>>>>>> + gate_ops->save_context(gate_hw);
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> + periph->parent_ctx = clk_periph_get_parent(hw);
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> + return 0;
>>>>>>>>>>>>>>>>>>> +}
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> +static void clk_periph_restore_context(struct clk_hw
>>>>>>>>>>>>>>>>>>> *hw)
>>>>>>>>>>>>>>>>>>> +{
>>>>>>>>>>>>>>>>>>> + struct tegra_clk_periph *periph = to_clk_periph(hw);
>>>>>>>>>>>>>>>>>>> + const struct clk_ops *gate_ops = periph->gate_ops;
>>>>>>>>>>>>>>>>>>> + struct clk_hw *gate_hw = &periph->gate.hw;
>>>>>>>>>>>>>>>>>>> + const struct clk_ops *div_ops = periph->div_ops;
>>>>>>>>>>>>>>>>>>> + struct clk_hw *div_hw = &periph->divider.hw;
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> + clk_periph_set_parent(hw, periph->parent_ctx);
>>>>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>>>>> + if (!(periph->gate.flags & TEGRA_PERIPH_NO_DIV))
>>>>>>>>>>>>>>>>>>> + div_ops->restore_context(div_hw);
>>>>>>>>>>>>>>>>>> Could you please point to where the divider's
>>>>>>>>>>>>>>>>>> save_context()
>>>>>>>>>>>>>>>>>> happens?
>>>>>>>>>>>>>>>>>> Because I can't see it.
>>>>>>>>>>>>>>>>> Ah, I now see that there is no need to save the dividers
>>>>>>>>>>>>>>>>> context
>>>>>>>>>>>>>>>>> because
>>>>>>>>>>>>>>>>> clk itself has enough info that is needed for the context's
>>>>>>>>>>>>>>>>> restoring
>>>>>>>>>>>>>>>>> (like I pointed in the review to v6).
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Looks like you could also implement a new
>>>>>>>>>>>>>>>>> clk_hw_get_parent_index()
>>>>>>>>>>>>>>>>> generic helper to get the index instead of storing it
>>>>>>>>>>>>>>>>> manually.
>>>>>>>>>>>>>>>> clk_periph_get_parent basically invokes existing clk_mux_ops
>>>>>>>>>>>>>>>> get_parent() which is then saved in tegra_clk_periph.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> All existing drivers are using directly get_parent() from
>>>>>>>>>>>>>>>> clk_mux
>>>>>>>>>>>>>>>> which actually gets index from the register read.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> To have this more generic w.r.t save/restore context
>>>>>>>>>>>>>>>> point of
>>>>>>>>>>>>>>>> view,
>>>>>>>>>>>>>>>> probably instead of implementing new get_parent_index
>>>>>>>>>>>>>>>> helper,
>>>>>>>>>>>>>>>> I think
>>>>>>>>>>>>>>>> its better to implement save_context and restore_context to
>>>>>>>>>>>>>>>> clk_mux_ops along with creating parent_index field into
>>>>>>>>>>>>>>>> clk_mux to
>>>>>>>>>>>>>>>> cache index during set_parent.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> So we just need to invoke mux_ops save_context and
>>>>>>>>>>>>>>>> restore_context.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I hope its ok to add save/restore context to clk_mux_ops
>>>>>>>>>>>>>>> to be
>>>>>>>>>>>>>>> more
>>>>>>>>>>>>>>> generic w.r.t save/restore context rather than
>>>>>>>>>>>>>>> get_parent_index
>>>>>>>>>>>>>>> API.
>>>>>>>>>>>>>>> Please confirm if you agree.
>>>>>>>>>>>>>> Sounds like a good idea. I see that there is a 'restoring'
>>>>>>>>>>>>>> helper for
>>>>>>>>>>>>>> the generic clk_gate, seems something similar could be done
>>>>>>>>>>>>>> for the
>>>>>>>>>>>>>> clk_mux. And looks like anyway you'll need to associate the
>>>>>>>>>>>>>> parent
>>>>>>>>>>>>>> clock
>>>>>>>>>>>>>> with the hw index in order to restore the muxing.
>>>>>>>>>>>>> by 'restoring' helper for generic clk_gate, are you
>>>>>>>>>>>>> referring to
>>>>>>>>>>>>> clk_gate_restore_context API?
>>>>>>>>>>>> Yes.
>>>>>>>>>>>>
>>>>>>>>>>>>> clk_gate_restore_context is API that's any clk drivers can
>>>>>>>>>>>>> use for
>>>>>>>>>>>>> clk_gate operation restore for custom gate clk_ops.
>>>>>>>>>>>>>
>>>>>>>>>>>>> But clk-periph is directly using generic clk_mux ops from
>>>>>>>>>>>>> clk_mux
>>>>>>>>>>>>> so I
>>>>>>>>>>>>> think we should add .restore_context to clk_mux_ops and then
>>>>>>>>>>>>> during
>>>>>>>>>>>>> clk-periph restore need to invoke mux_ops->restore_context.
>>>>>>>>>>>> I'm not sure whether it will be good for every driver that uses
>>>>>>>>>>>> generic
>>>>>>>>>>>> clk_mux ops. Should be more flexible to have a generic helper
>>>>>>>>>>>> function
>>>>>>>>>>>> that any driver could use in order to restore the clock's
>>>>>>>>>>>> parent.
>>>>>>>>>>>>
>>>>>>>>>>>> The clk-periph restoring also includes case of combining divider
>>>>>>>>>>>> and
>>>>>>>>>>>> parent restoring, so generic helper could be useful in that case
>>>>>>>>>>>> as well.
>>>>>>>>>>>>
>>>>>>>>>>>> It also looks like you could actually use the
>>>>>>>>>>>> clk_gate_restore_context()
>>>>>>>>>>>> instead of manually saving the clock's enable-state, couldn't
>>>>>>>>>>>> you?
>>>>>>>>>>> ok for clk_mux, can add generic clk_mux_restore_context API
>>>>>>>>>>> rather
>>>>>>>>>>> than
>>>>>>>>>>> using restore_context in clk_ops and will invoke that during
>>>>>>>>>>> clk_periph
>>>>>>>>>>> restore.
>>>>>>>>>>>
>>>>>>> digging thru looks like for clk_periph source restore instead of
>>>>>>> clk_mux_restore_context, i can directly do clk_hw_get_parent and
>>>>>>> clk_set_parent with mux_hw as they invoke mux_ops get/set parent
>>>>>>> anyway.
>>>>>>> Will do this for periph clk mux
>>>>>>>>>>> Reg clk_gate, looks like we cant use generic
>>>>>>>>>>> clk_gate_restore_context
>>>>>>>>>>> for clk-periph as it calls enable/disable callbacks and
>>>>>>>>>>> clk_periph_enable/disable in clk-periph-gate also updated
>>>>>>>>>>> refcnt and
>>>>>>>>>>> depending on that actual enable/disable is set.
>>>>>>>>>>>
>>>>>>>>>>> During suspend, peripherals that are already enabled have their
>>>>>>>>>>> refcnt >
>>>>>>>>>>> 1, so they dont go thru enable/disable on restore if we use same
>>>>>>>>>>> enable/disable callback.
>>>>>>>>>> Looks like you could just decrement the gate's enable_refcnt on
>>>>>>>>>> save_context, wouldn't that work?
>>>>>>>>>>
>>>>>>>> gate->enable_refcnt is within clk-periph-gate which gets updated
>>>>>>>> when
>>>>>>>> enable/disable callbacks get execute thru clk_core_enable/disable.
>>>>>>>> But actual enable_count used in clk_gate_restore_context is the one
>>>>>>>> which gets updated with in the clk core enable/disable functions
>>>>>>>> which
>>>>>>>> invokes these callbacks. Depending on this enable_count in clk
>>>>>>>> core it
>>>>>>>> invokes enable/disable.
>>>>>>>>
>>>>>>>> So, this will cause mismatch if we handle refcnt during save/restore
>>>>>>>> of tegra_clk_periph_gate_ops and also enable/disable thru this
>>>>>>>> clk_gate_restore_context is based on enable_count from clk core.
>>>>>>>>
>>>>>>>>>>> Also to align exact reset state along with CLK (like for case
>>>>>>>>>>> where
>>>>>>>>>>> CLK
>>>>>>>>>>> is enabled but peripheral might be in reset state), implemented
>>>>>>>>>>> save/restore in tegra specific tegra_clk_periph_gate_ops
>>>>>>>>>> I'm wondering whether instead of saving/restoring reset-state of
>>>>>>>>>> every
>>>>>>>>>> clock, you could simply save/restore the whole RST_DEV_x_SET
>>>>>>>>>> register.
>>>>>>>>>> Couldn't you?
>>>>>>>>> Thats what I was doing in first version of patch. But later as we
>>>>>>>>> moved to use clk_save_context and clk_restore_context, peripheral
>>>>>>>>> clk_hw RST & CLK enables happen thru its corresponding save/restore
>>>>>>>>> after source restore
>>>>>>>> Also, to align both CLK & RST to the exact state of register, doing
>>>>>>>> save/restore in tegra_clk_periph_gate_ops and invoking this after
>>>>>>>> source restore for peripheral clock, seems cleaner to avoid any
>>>>>>>> misconfiguration b/w rst & clk settings.
>>>>>>>>
>>>>>> It looks to me that it is very wasteful to store/restore each
>>>>>> individual
>>>>>> gate and reset state, also given that some of them are shared. I think
>>>>>> that the gates and resets should be restored separately for the
>>>>>> peripherals by a custom tegra_clk_save/restore_periph_gates/resets().
>>>>> clk_periph_fixed_disable just disables clock only without deasserting
>>>>> the corresponding peripheral.
>>>>>
>>>>> corresponding peripheral drivers can also issue reset assert/deassert
>>>>> thru reset_control_assert/deassert.
>>>>>
>>>>> So, we will not get the actual state of clk and rst unless we read and
>>>>> save state of reset and clock separately during save_context.
>>>>>
>>>>> Currently patch is already using custom
>>>>> tegra_clk_periph_fixed_save/restore_context for corresponding clk_ops.
>>>>> Are you suggesting to do save and restore of complete CLK_ENB/RST_DEV
>>>>> register settings instead of individual peripheral bits?
>>>> Yes, I'm suggesting to do a complete ungate/reset handling of the
>>>> devices in a separate function. All enabling/deassertion will be done in
>>>> a single hop, hence using 7us delay and four u32 words, which is much
>>>> nicer IMHO.
>>> Actually six words, three for CLKs and three for RSTs.
>> OK, So with separate function doing complete register save/restore for
>> clk & rst, we can't do this thru clk_ops save/restore as clk_ops
>> save_restore happens per peripheral wise. So if we decide to do this,
>> then this should be invoked in clk-tegra210 driver suspend/resume.
> Yes, per-clock save/restore should be used for setting rate and parent.
> The ungating and resetting could be done separately to keep things cleaner.
>
OK, Will move back to register wise save/restore for clk_enb/rst_dev in
next version.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox