Message ID | 1484921306-9967-5-git-send-email-geert+renesas@glider.be (mailing list archive) |
---|---|
State | Accepted |
Headers | show |
Hi Geert, On Fri, 2017-01-20 at 15:08 +0100, Geert Uytterhoeven wrote: > Add optional support for the Reset Control feature of the Renesas Clock > Pulse Generator / Module Standby and Software Reset module on R-Car > Gen2, R-Car Gen3, and RZ/G1 SoCs. Is there a reason to make this optional? > This allows to reset SoC devices using the Reset Controller API. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> Looks good to me, Acked-by: Philipp Zabel <p.zabel@pengutronix.de> Just a small issue below, > --- > drivers/clk/renesas/renesas-cpg-mssr.c | 122 +++++++++++++++++++++++++++++++++ > 1 file changed, 122 insertions(+) > > diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c > index f1161a585c57e433..ea4af714ac14603a 100644 > --- a/drivers/clk/renesas/renesas-cpg-mssr.c > +++ b/drivers/clk/renesas/renesas-cpg-mssr.c [...] > +static int cpg_mssr_reset(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev); > + unsigned int reg = id / 32; > + unsigned int bit = id % 32; > + u32 bitmask = BIT(bit); Here you have a bitmask = BIT(bit) variable. > + unsigned long flags; > + u32 value; > + > + dev_dbg(priv->dev, "reset %u%02u\n", reg, bit); > + > + /* Reset module */ > + spin_lock_irqsave(&priv->rmw_lock, flags); > + value = readl(priv->base + SRCR(reg)); > + value |= bitmask; Here you use it. > + writel(value, priv->base + SRCR(reg)); > + spin_unlock_irqrestore(&priv->rmw_lock, flags); > + > + /* Wait for at least one cycle of the RCLK clock (@ ca. 32 kHz) */ > + udelay(35); > + > + /* Release module from reset state */ > + writel(bitmask, priv->base + SRSTCLR(reg)); > + > + return 0; > +} > + > +static int cpg_mssr_assert(struct reset_controller_dev *rcdev, unsigned long id) > +{ > + struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev); > + unsigned int reg = id / 32; > + unsigned int bit = id % 32; Here you haven't. > + unsigned long flags; > + u32 value; > + > + dev_dbg(priv->dev, "assert %u%02u\n", reg, bit); > + > + spin_lock_irqsave(&priv->rmw_lock, flags); > + value = readl(priv->base + SRCR(reg)); > + writel(value | BIT(bit), priv->base + SRCR(reg)); Here you don't. > + spin_unlock_irqrestore(&priv->rmw_lock, flags); > + return 0; > +} > + > +static int cpg_mssr_deassert(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev); > + unsigned int reg = id / 32; > + unsigned int bit = id % 32; > + > + dev_dbg(priv->dev, "deassert %u%02u\n", reg, bit); > + > + writel(BIT(bit), priv->base + SRSTCLR(reg)); And here ... > + return 0; > +} > + > +static int cpg_mssr_status(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev); > + unsigned int reg = id / 32; > + unsigned int bit = id % 32; > + > + return !!(readl(priv->base + SRCR(reg)) & BIT(bit)); And here neither. I'd choose one variant over the other for consistency. regards Philipp -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Philipp, On Fri, Jan 20, 2017 at 4:57 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: > On Fri, 2017-01-20 at 15:08 +0100, Geert Uytterhoeven wrote: >> Add optional support for the Reset Control feature of the Renesas Clock >> Pulse Generator / Module Standby and Software Reset module on R-Car >> Gen2, R-Car Gen3, and RZ/G1 SoCs. > > Is there a reason to make this optional? With "optional", I mean that I don't select CONFIG_RESET_CONTROLLER, and make the reset controller code depend on CONFIG_RESET_CONTROLLER. So far we don't have any mandatory users. >> This allows to reset SoC devices using the Reset Controller API. >> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Looks good to me, > > Acked-by: Philipp Zabel <p.zabel@pengutronix.de> Thanks! > Just a small issue below, > >> --- >> drivers/clk/renesas/renesas-cpg-mssr.c | 122 +++++++++++++++++++++++++++++++++ >> 1 file changed, 122 insertions(+) >> >> diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c >> index f1161a585c57e433..ea4af714ac14603a 100644 >> --- a/drivers/clk/renesas/renesas-cpg-mssr.c >> +++ b/drivers/clk/renesas/renesas-cpg-mssr.c > [...] >> +static int cpg_mssr_reset(struct reset_controller_dev *rcdev, >> + unsigned long id) >> +{ >> + struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev); >> + unsigned int reg = id / 32; >> + unsigned int bit = id % 32; >> + u32 bitmask = BIT(bit); > > Here you have a bitmask = BIT(bit) variable. Because there are two users in the function. >> + unsigned long flags; >> + u32 value; >> + >> + dev_dbg(priv->dev, "reset %u%02u\n", reg, bit); >> + >> + /* Reset module */ >> + spin_lock_irqsave(&priv->rmw_lock, flags); >> + value = readl(priv->base + SRCR(reg)); >> + value |= bitmask; > > Here you use it. > >> + writel(value, priv->base + SRCR(reg)); >> + spin_unlock_irqrestore(&priv->rmw_lock, flags); >> + >> + /* Wait for at least one cycle of the RCLK clock (@ ca. 32 kHz) */ >> + udelay(35); >> + >> + /* Release module from reset state */ >> + writel(bitmask, priv->base + SRSTCLR(reg)); >> + >> + return 0; >> +} >> + >> +static int cpg_mssr_assert(struct reset_controller_dev *rcdev, unsigned long id) >> +{ >> + struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev); >> + unsigned int reg = id / 32; >> + unsigned int bit = id % 32; > > Here you haven't. > >> + unsigned long flags; >> + u32 value; >> + >> + dev_dbg(priv->dev, "assert %u%02u\n", reg, bit); >> + >> + spin_lock_irqsave(&priv->rmw_lock, flags); >> + value = readl(priv->base + SRCR(reg)); >> + writel(value | BIT(bit), priv->base + SRCR(reg)); > > Here you don't. Because there's a single user in the function. >> + spin_unlock_irqrestore(&priv->rmw_lock, flags); >> + return 0; >> +} >> + >> +static int cpg_mssr_deassert(struct reset_controller_dev *rcdev, >> + unsigned long id) >> +{ >> + struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev); >> + unsigned int reg = id / 32; >> + unsigned int bit = id % 32; >> + >> + dev_dbg(priv->dev, "deassert %u%02u\n", reg, bit); >> + >> + writel(BIT(bit), priv->base + SRSTCLR(reg)); > > And here ... > >> + return 0; >> +} >> + >> +static int cpg_mssr_status(struct reset_controller_dev *rcdev, >> + unsigned long id) >> +{ >> + struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev); >> + unsigned int reg = id / 32; >> + unsigned int bit = id % 32; >> + >> + return !!(readl(priv->base + SRCR(reg)) & BIT(bit)); > > And here neither. > > I'd choose one variant over the other for consistency. OK, I'll use the "bitmask" variable in all functions. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, Nice patch! It took me a while to understand why you didn't need to read the register before writing to it in cpg_mssr_deassert() :-) Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> On 2017-01-20 15:08:22 +0100, Geert Uytterhoeven wrote: > Add optional support for the Reset Control feature of the Renesas Clock > Pulse Generator / Module Standby and Software Reset module on R-Car > Gen2, R-Car Gen3, and RZ/G1 SoCs. > > This allows to reset SoC devices using the Reset Controller API. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > drivers/clk/renesas/renesas-cpg-mssr.c | 122 +++++++++++++++++++++++++++++++++ > 1 file changed, 122 insertions(+) > > diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c > index f1161a585c57e433..ea4af714ac14603a 100644 > --- a/drivers/clk/renesas/renesas-cpg-mssr.c > +++ b/drivers/clk/renesas/renesas-cpg-mssr.c > @@ -16,6 +16,7 @@ > #include <linux/clk.h> > #include <linux/clk-provider.h> > #include <linux/clk/renesas.h> > +#include <linux/delay.h> > #include <linux/device.h> > #include <linux/init.h> > #include <linux/mod_devicetable.h> > @@ -25,6 +26,7 @@ > #include <linux/platform_device.h> > #include <linux/pm_clock.h> > #include <linux/pm_domain.h> > +#include <linux/reset-controller.h> > #include <linux/slab.h> > > #include <dt-bindings/clock/renesas-cpg-mssr.h> > @@ -96,6 +98,7 @@ > /** > * Clock Pulse Generator / Module Standby and Software Reset Private Data > * > + * @rcdev: Optional reset controller entity > * @dev: CPG/MSSR device > * @base: CPG/MSSR register block base address > * @rmw_lock: protects RMW register accesses > @@ -105,6 +108,9 @@ > * @last_dt_core_clk: ID of the last Core Clock exported to DT > */ > struct cpg_mssr_priv { > +#ifdef CONFIG_RESET_CONTROLLER > + struct reset_controller_dev rcdev; > +#endif > struct device *dev; > void __iomem *base; > spinlock_t rmw_lock; > @@ -494,6 +500,118 @@ static int __init cpg_mssr_add_clk_domain(struct device *dev, > return 0; > } > > +#ifdef CONFIG_RESET_CONTROLLER > + > +#define rcdev_to_priv(x) container_of(x, struct cpg_mssr_priv, rcdev) > + > +static int cpg_mssr_reset(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev); > + unsigned int reg = id / 32; > + unsigned int bit = id % 32; > + u32 bitmask = BIT(bit); > + unsigned long flags; > + u32 value; > + > + dev_dbg(priv->dev, "reset %u%02u\n", reg, bit); > + > + /* Reset module */ > + spin_lock_irqsave(&priv->rmw_lock, flags); > + value = readl(priv->base + SRCR(reg)); > + value |= bitmask; > + writel(value, priv->base + SRCR(reg)); > + spin_unlock_irqrestore(&priv->rmw_lock, flags); > + > + /* Wait for at least one cycle of the RCLK clock (@ ca. 32 kHz) */ > + udelay(35); > + > + /* Release module from reset state */ > + writel(bitmask, priv->base + SRSTCLR(reg)); > + > + return 0; > +} > + > +static int cpg_mssr_assert(struct reset_controller_dev *rcdev, unsigned long id) > +{ > + struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev); > + unsigned int reg = id / 32; > + unsigned int bit = id % 32; > + unsigned long flags; > + u32 value; > + > + dev_dbg(priv->dev, "assert %u%02u\n", reg, bit); > + > + spin_lock_irqsave(&priv->rmw_lock, flags); > + value = readl(priv->base + SRCR(reg)); > + writel(value | BIT(bit), priv->base + SRCR(reg)); > + spin_unlock_irqrestore(&priv->rmw_lock, flags); > + return 0; > +} > + > +static int cpg_mssr_deassert(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev); > + unsigned int reg = id / 32; > + unsigned int bit = id % 32; > + > + dev_dbg(priv->dev, "deassert %u%02u\n", reg, bit); > + > + writel(BIT(bit), priv->base + SRSTCLR(reg)); > + return 0; > +} > + > +static int cpg_mssr_status(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev); > + unsigned int reg = id / 32; > + unsigned int bit = id % 32; > + > + return !!(readl(priv->base + SRCR(reg)) & BIT(bit)); > +} > + > +static const struct reset_control_ops cpg_mssr_reset_ops = { > + .reset = cpg_mssr_reset, > + .assert = cpg_mssr_assert, > + .deassert = cpg_mssr_deassert, > + .status = cpg_mssr_status, > +}; > + > +static int cpg_mssr_reset_xlate(struct reset_controller_dev *rcdev, > + const struct of_phandle_args *reset_spec) > +{ > + struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev); > + unsigned int unpacked = reset_spec->args[0]; > + unsigned int idx = MOD_CLK_PACK(unpacked); > + > + if (unpacked % 100 > 31 || idx >= rcdev->nr_resets) { > + dev_err(priv->dev, "Invalid reset index %u\n", unpacked); > + return -EINVAL; > + } > + > + return idx; > +} > + > +static int cpg_mssr_reset_controller_register(struct cpg_mssr_priv *priv) > +{ > + priv->rcdev.ops = &cpg_mssr_reset_ops; > + priv->rcdev.of_node = priv->dev->of_node; > + priv->rcdev.of_reset_n_cells = 1; > + priv->rcdev.of_xlate = cpg_mssr_reset_xlate; > + priv->rcdev.nr_resets = priv->num_mod_clks; > + return devm_reset_controller_register(priv->dev, &priv->rcdev); > +} > + > +#else /* !CONFIG_RESET_CONTROLLER */ > +static inline int cpg_mssr_reset_controller_register(struct cpg_mssr_priv *priv) > +{ > + return 0; > +} > +#endif /* !CONFIG_RESET_CONTROLLER */ > + > + > static const struct of_device_id cpg_mssr_match[] = { > #ifdef CONFIG_ARCH_R8A7743 > { > @@ -591,6 +709,10 @@ static int __init cpg_mssr_probe(struct platform_device *pdev) > if (error) > return error; > > + error = cpg_mssr_reset_controller_register(priv); > + if (error) > + return error; > + > return 0; > } > > -- > 1.9.1 >
On 01/20, Geert Uytterhoeven wrote: > Add optional support for the Reset Control feature of the Renesas Clock > Pulse Generator / Module Standby and Software Reset module on R-Car > Gen2, R-Car Gen3, and RZ/G1 SoCs. > > This allows to reset SoC devices using the Reset Controller API. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- Acked-by: Stephen Boyd <sboyd@codeaurora.org>
Hi Niklas, On Fri, Jan 20, 2017 at 11:03 PM, Niklas Söderlund <niklas.soderlund@ragnatech.se> wrote: > Nice patch! It took me a while to understand why you didn't need to read > the register before writing to it in cpg_mssr_deassert() :-) Yeah, deassertion and assertion are asymmetrical. Note that on older (not yet supported) SH/R-Mobile parts, there are no reset clear registers, and deassertion is symmetrical. > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/clk/renesas/renesas-cpg-mssr.c b/drivers/clk/renesas/renesas-cpg-mssr.c index f1161a585c57e433..ea4af714ac14603a 100644 --- a/drivers/clk/renesas/renesas-cpg-mssr.c +++ b/drivers/clk/renesas/renesas-cpg-mssr.c @@ -16,6 +16,7 @@ #include <linux/clk.h> #include <linux/clk-provider.h> #include <linux/clk/renesas.h> +#include <linux/delay.h> #include <linux/device.h> #include <linux/init.h> #include <linux/mod_devicetable.h> @@ -25,6 +26,7 @@ #include <linux/platform_device.h> #include <linux/pm_clock.h> #include <linux/pm_domain.h> +#include <linux/reset-controller.h> #include <linux/slab.h> #include <dt-bindings/clock/renesas-cpg-mssr.h> @@ -96,6 +98,7 @@ /** * Clock Pulse Generator / Module Standby and Software Reset Private Data * + * @rcdev: Optional reset controller entity * @dev: CPG/MSSR device * @base: CPG/MSSR register block base address * @rmw_lock: protects RMW register accesses @@ -105,6 +108,9 @@ * @last_dt_core_clk: ID of the last Core Clock exported to DT */ struct cpg_mssr_priv { +#ifdef CONFIG_RESET_CONTROLLER + struct reset_controller_dev rcdev; +#endif struct device *dev; void __iomem *base; spinlock_t rmw_lock; @@ -494,6 +500,118 @@ static int __init cpg_mssr_add_clk_domain(struct device *dev, return 0; } +#ifdef CONFIG_RESET_CONTROLLER + +#define rcdev_to_priv(x) container_of(x, struct cpg_mssr_priv, rcdev) + +static int cpg_mssr_reset(struct reset_controller_dev *rcdev, + unsigned long id) +{ + struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev); + unsigned int reg = id / 32; + unsigned int bit = id % 32; + u32 bitmask = BIT(bit); + unsigned long flags; + u32 value; + + dev_dbg(priv->dev, "reset %u%02u\n", reg, bit); + + /* Reset module */ + spin_lock_irqsave(&priv->rmw_lock, flags); + value = readl(priv->base + SRCR(reg)); + value |= bitmask; + writel(value, priv->base + SRCR(reg)); + spin_unlock_irqrestore(&priv->rmw_lock, flags); + + /* Wait for at least one cycle of the RCLK clock (@ ca. 32 kHz) */ + udelay(35); + + /* Release module from reset state */ + writel(bitmask, priv->base + SRSTCLR(reg)); + + return 0; +} + +static int cpg_mssr_assert(struct reset_controller_dev *rcdev, unsigned long id) +{ + struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev); + unsigned int reg = id / 32; + unsigned int bit = id % 32; + unsigned long flags; + u32 value; + + dev_dbg(priv->dev, "assert %u%02u\n", reg, bit); + + spin_lock_irqsave(&priv->rmw_lock, flags); + value = readl(priv->base + SRCR(reg)); + writel(value | BIT(bit), priv->base + SRCR(reg)); + spin_unlock_irqrestore(&priv->rmw_lock, flags); + return 0; +} + +static int cpg_mssr_deassert(struct reset_controller_dev *rcdev, + unsigned long id) +{ + struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev); + unsigned int reg = id / 32; + unsigned int bit = id % 32; + + dev_dbg(priv->dev, "deassert %u%02u\n", reg, bit); + + writel(BIT(bit), priv->base + SRSTCLR(reg)); + return 0; +} + +static int cpg_mssr_status(struct reset_controller_dev *rcdev, + unsigned long id) +{ + struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev); + unsigned int reg = id / 32; + unsigned int bit = id % 32; + + return !!(readl(priv->base + SRCR(reg)) & BIT(bit)); +} + +static const struct reset_control_ops cpg_mssr_reset_ops = { + .reset = cpg_mssr_reset, + .assert = cpg_mssr_assert, + .deassert = cpg_mssr_deassert, + .status = cpg_mssr_status, +}; + +static int cpg_mssr_reset_xlate(struct reset_controller_dev *rcdev, + const struct of_phandle_args *reset_spec) +{ + struct cpg_mssr_priv *priv = rcdev_to_priv(rcdev); + unsigned int unpacked = reset_spec->args[0]; + unsigned int idx = MOD_CLK_PACK(unpacked); + + if (unpacked % 100 > 31 || idx >= rcdev->nr_resets) { + dev_err(priv->dev, "Invalid reset index %u\n", unpacked); + return -EINVAL; + } + + return idx; +} + +static int cpg_mssr_reset_controller_register(struct cpg_mssr_priv *priv) +{ + priv->rcdev.ops = &cpg_mssr_reset_ops; + priv->rcdev.of_node = priv->dev->of_node; + priv->rcdev.of_reset_n_cells = 1; + priv->rcdev.of_xlate = cpg_mssr_reset_xlate; + priv->rcdev.nr_resets = priv->num_mod_clks; + return devm_reset_controller_register(priv->dev, &priv->rcdev); +} + +#else /* !CONFIG_RESET_CONTROLLER */ +static inline int cpg_mssr_reset_controller_register(struct cpg_mssr_priv *priv) +{ + return 0; +} +#endif /* !CONFIG_RESET_CONTROLLER */ + + static const struct of_device_id cpg_mssr_match[] = { #ifdef CONFIG_ARCH_R8A7743 { @@ -591,6 +709,10 @@ static int __init cpg_mssr_probe(struct platform_device *pdev) if (error) return error; + error = cpg_mssr_reset_controller_register(priv); + if (error) + return error; + return 0; }
Add optional support for the Reset Control feature of the Renesas Clock Pulse Generator / Module Standby and Software Reset module on R-Car Gen2, R-Car Gen3, and RZ/G1 SoCs. This allows to reset SoC devices using the Reset Controller API. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/clk/renesas/renesas-cpg-mssr.c | 122 +++++++++++++++++++++++++++++++++ 1 file changed, 122 insertions(+)