Re: [PATCH 4/4] clk: renesas: r8a77980-cpg-mssr: add RPC clocks
Hi Sergei,
On Tue, Nov 27, 2018 at 6:45 PM Sergei Shtylyov
wrote:
> On 11/23/2018 03:59 PM, Geert Uytterhoeven wrote:
> >> Add the RPCSRC internal clock on R-Car V3H (R8A77980) -- it's controlled
> >> by the RPCCKCR.DIV[4:3] on all the R-Car gen3 SoCs except V3M (R8A77970)
> >> but the encoding of this field is different between SoCs.
> >> --- renesas-drivers.orig/drivers/clk/renesas/r8a77980-cpg-mssr.c
> >> +++ renesas-drivers/drivers/clk/renesas/r8a77980-cpg-mssr.c
> [...]
> >> + const struct clk *parent = clks[core->parent];
> >> +
> >> + if (IS_ERR(parent))
> >> + return ERR_CAST(parent);
> >> +
> >> + return clk_register_divider_table(NULL, core->name,
> >> + __clk_get_name(parent),
> >> 0,
> >> + base + CPG_RPCCKCR, 3,
> >> 2, 0,
> >> + cpg_rpcsrc_div_table,
> >> NULL);
> >
> > Don't you need a spinlock (last parameter, currently NULL)?
> > This needs to be synchronized with controlling the RPCD2 and RPC clocks,
> > as they operate on the same register.
>
>Indeed. How about the RMW accesses to the other register? I'd like to place
> the lock/unlock right in cpg_reg_modify()...
Yes, RMW, too.
> > However, that would deadlock, as enabling e.g. RPC-IF will enable
> > all parent clocks?
>
> Could toy please elaborate?
Sorry, I incorrectly assumed it would hold the lock while calling into the
parent, which would deadlock if using the same lock.
But as it only holds the lock for register access, it's fine.
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/4] clk: renesas: r8a77980-cpg-mssr: add RPC clocks
On 11/23/2018 03:59 PM, Geert Uytterhoeven wrote:
>> Add the RPCSRC internal clock on R-Car V3H (R8A77980) -- it's controlled
>> by the RPCCKCR.DIV[4:3] on all the R-Car gen3 SoCs except V3M (R8A77970)
>> but the encoding of this field is different between SoCs.
>
> Given the tables and encoding are the same on H3, M3-W, M3-N, and V3H,
> I think it makes sense to move the common support to rcar-gen3-cpg.c.
Done.
> Heck, you could even just select a different table on D3/E3 using
> soc_device_match(), if only one encoding would not select a different parent
> clock :-(
Indeed...
>> Add the RPC[D2] clocks (derived from this internal clock) and the RPC-IF
>> module clock as well...
>>
>> Signed-off-by: Sergei Shtylyov
>
>> --- renesas-drivers.orig/drivers/clk/renesas/r8a77980-cpg-mssr.c
>> +++ renesas-drivers/drivers/clk/renesas/r8a77980-cpg-mssr.c
[...]
>> + const struct clk *parent = clks[core->parent];
>> +
>> + if (IS_ERR(parent))
>> + return ERR_CAST(parent);
>> +
>> + return clk_register_divider_table(NULL, core->name,
>> + __clk_get_name(parent), 0,
>> + base + CPG_RPCCKCR, 3, 2,
>> 0,
>> + cpg_rpcsrc_div_table,
>> NULL);
>
> Don't you need a spinlock (last parameter, currently NULL)?
> This needs to be synchronized with controlling the RPCD2 and RPC clocks,
> as they operate on the same register.
Indeed. How about the RMW accesses to the other register? I'd like to place
the lock/unlock right in cpg_reg_modify()...
> However, that would deadlock, as enabling e.g. RPC-IF will enable
> all parent clocks?
Could toy please elaborate?
>> + } else {
>> + return rcar_gen3_cpg_clk_register(dev, core, info, clks,
>> base,
>> + notifiers);
>> + }
>
> Gr{oetje,eeting}s,
>
> Geert
MBR, Sergei
Re: [PATCH 4/4] clk: renesas: r8a77980-cpg-mssr: add RPC clocks
Hi Sergei,
Thanks for your patch!
On Thu, Nov 22, 2018 at 7:45 PM Sergei Shtylyov
wrote:
> Add the RPCSRC internal clock on R-Car V3H (R8A77980) -- it's controlled
> by the RPCCKCR.DIV[4:3] on all the R-Car gen3 SoCs except V3M (R8A77970)
> but the encoding of this field is different between SoCs.
Given the tables and encoding are the same on H3, M3-W, M3-N, and V3H,
I think it makes sense to move the common support to rcar-gen3-cpg.c.
Heck, you could even just select a different table on D3/E3 using
soc_device_match(), if only one encoding would not select a different parent
clock :-(
> Add the RPC[D2] clocks (derived from this internal clock) and the RPC-IF
> module clock as well...
>
> Signed-off-by: Sergei Shtylyov
> --- renesas-drivers.orig/drivers/clk/renesas/r8a77980-cpg-mssr.c
> +++ renesas-drivers/drivers/clk/renesas/r8a77980-cpg-mssr.c
> @@ -21,6 +22,10 @@
> #include "renesas-cpg-mssr.h"
> #include "rcar-gen3-cpg.h"
>
> +enum r8a77980_clk_types {
> + CLK_TYPE_R8A77980_RPCSRC = CLK_TYPE_GEN3_SOC_BASE,
Rename and move to rcar_gen3_clk_types?
> @@ -215,6 +232,27 @@ static int __init r8a77980_cpg_mssr_init
> return rcar_gen3_cpg_init(cpg_pll_config, CLK_EXTALR, cpg_mode);
> }
>
> +static struct clk * __init r8a77980_cpg_clk_register(struct device *dev,
> + const struct cpg_core_clk *core, const struct cpg_mssr_info *info,
> + struct clk **clks, void __iomem *base,
> + struct raw_notifier_head *notifiers)
> +{
> + if (core->type == CLK_TYPE_R8A77980_RPCSRC) {
I'd use a switch() statement here, for consistency with other drivers.
> + const struct clk *parent = clks[core->parent];
> +
> + if (IS_ERR(parent))
> + return ERR_CAST(parent);
> +
> + return clk_register_divider_table(NULL, core->name,
> + __clk_get_name(parent), 0,
> + base + CPG_RPCCKCR, 3, 2, 0,
> + cpg_rpcsrc_div_table, NULL);
Don't you need a spinlock (last parameter, currently NULL)?
This needs to be synchronized with controlling the RPCD2 and RPC clocks,
as they operate on the same register.
However, that would deadlock, as enabling e.g. RPC-IF will enable
all parent clocks?
> + } else {
> + return rcar_gen3_cpg_clk_register(dev, core, info, clks, base,
> + notifiers);
> + }
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
