Re: [PATCH 4/8] clk: renesas: cpg-mssr: Add support for reset control
Hi Niklas, On Fri, Jan 20, 2017 at 11:03 PM, Niklas Söderlundwrote: > 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 Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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
Re: [PATCH 4/8] clk: renesas: cpg-mssr: Add support for reset control
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> --- Acked-by: Stephen Boyd -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH 4/8] clk: renesas: cpg-mssr: Add support for reset control
Hi Philipp, On Fri, Jan 20, 2017 at 4:57 PM, Philipp Zabelwrote: > 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 > > Looks good to me, > > Acked-by: Philipp Zabel 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(>rmw_lock, flags); >> + value = readl(priv->base + SRCR(reg)); >> + value |= bitmask; > > Here you use it. > >> + writel(value, priv->base + SRCR(reg)); >> + spin_unlock_irqrestore(>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(>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(>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 -- ge...@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
Re: [PATCH 4/8] clk: renesas: cpg-mssr: Add support for reset control
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 UytterhoevenLooks good to me, Acked-by: Philipp Zabel 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(>rmw_lock, flags); > + value = readl(priv->base + SRCR(reg)); > + value |= bitmask; Here you use it. > + writel(value, priv->base + SRCR(reg)); > + spin_unlock_irqrestore(>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(>rmw_lock, flags); > + value = readl(priv->base + SRCR(reg)); > + writel(value | BIT(bit), priv->base + SRCR(reg)); Here you don't. > + spin_unlock_irqrestore(>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
[PATCH 4/8] clk: renesas: cpg-mssr: Add support for reset control
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--- 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 #include #include +#include #include #include #include @@ -25,6 +26,7 @@ #include #include #include +#include #include #include @@ -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(>rmw_lock, flags); + value = readl(priv->base + SRCR(reg)); + value |= bitmask; + writel(value, priv->base + SRCR(reg)); + spin_unlock_irqrestore(>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(>rmw_lock, flags); + value = readl(priv->base + SRCR(reg)); + writel(value | BIT(bit), priv->base + SRCR(reg)); + spin_unlock_irqrestore(>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 = _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, >rcdev); +} + +#else /* !CONFIG_RESET_CONTROLLER */ +static inline int cpg_mssr_reset_controller_register(struct cpg_mssr_priv *priv) +{ + return 0; +} +#endif /* !CONFIG_RESET_CONTROLLER */ + +