Re: [PATCH v2] clk/axs10x: introduce AXS10X pll driver
On 02/21, Vlad Zakharov wrote:
> diff --git a/Documentation/devicetree/bindings/clock/snps,pll-clock.txt
> b/Documentation/devicetree/bindings/clock/snps,pll-clock.txt
> new file mode 100644
> index 000..5706246
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/snps,pll-clock.txt
> @@ -0,0 +1,28 @@
> +Binding for the AXS10X Generic PLL clock
> +
> +This binding uses the common clock binding[1].
> +
> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
> +
> +Required properties:
> +- compatible: should be "snps,axs10x--pll-clock"
> + "snps,axs10x-arc-pll-clock"
> + "snps,axs10x-pgu-pll-clock"
> +- reg: should always contain 2 pairs address - length: first for PLL config
> +registers and second for corresponding LOCK CGU register.
> +- clocks: shall be the input parent clock phandle for the PLL.
> +- #clock-cells: from common clock binding; Should always be set to 0.
> +
> +Example:
> + input-clk: input-clk {
> + clock-frequency = <>;
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> + };
> +
> + core-clk: core-clk@80 {
> + compatible = "snps,axs10x-arc-pll-clock";
> + reg = <0x80 0x10 0x100 0x10>;
Can you add the braces around register pairs in the example?
Makes it clearer with a quick glance.
> + #clock-cells = <0>;
> + clocks = <&input-clk>;
> + };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3960e7f..5805833 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11910,6 +11910,12 @@ F: arch/arc/plat-axs10x
> F: arch/arc/boot/dts/ax*
> F: Documentation/devicetree/bindings/arc/axs10*
>
> +SYNOPSYS ARC SDP clock driver
This also includes the existing driver there. Jose can ack this?
> +M: Vlad Zakharov
> +S: Supported
> +F: drivers/clk/axs10x/*
> +F: Documentation/devicetree/bindings/clock/snps,pll-clock.txt
> +
> SYSTEM CONFIGURATION (SYSCON)
> M: Lee Jones
> M: Arnd Bergmann
> diff --git a/drivers/clk/axs10x/Makefile b/drivers/clk/axs10x/Makefile
> index 01996b8..d747dea 100644
> --- a/drivers/clk/axs10x/Makefile
> +++ b/drivers/clk/axs10x/Makefile
> @@ -1 +1,2 @@
> obj-y += i2s_pll_clock.o
> +obj-y += pll_clock.o
> diff --git a/drivers/clk/axs10x/pll_clock.c b/drivers/clk/axs10x/pll_clock.c
> new file mode 100644
> index 000..784a0a2
> --- /dev/null
> +++ b/drivers/clk/axs10x/pll_clock.c
> @@ -0,0 +1,384 @@
> +/*
> + * Synopsys AXS10X SDP Generic PLL clock driver
> + *
> + * Copyright (C) 2017 Synopsys
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +/* PLL registers addresses */
> +#define PLL_REG_IDIV 0x0
> +#define PLL_REG_FBDIV0x4
> +#define PLL_REG_ODIV 0x8
> +
> +/*
> + * Bit fields of the PLL IDIV/FBDIV/ODIV registers:
> + *
> + * |3115|14| 13 | 12 |11 6|5 0|
> + * |---RESRVED--|-NOUPDATE-|-BYPASS-|-EDGE-|--HIGHTIME--|--LOWTIME--|
> + * ||__||__||___|
> + *
> + * Following macros detirmine the way of access to these registers
s/detirmine/determine/
> + * They should be set up only using the macros.
> + * reg should be and uint32_t variable.
s/and/an/?
> + */
> +
> +#define PLL_REG_GET_LOW(reg) \
> + (((reg) & (0x3F << 0)) >> 0)
> +#define PLL_REG_GET_HIGH(reg)\
> + (((reg) & (0x3F << 6)) >> 6)
> +#define PLL_REG_GET_EDGE(reg)\
> + (((reg) & (BIT(12))) ? 1 : 0)
> +#define PLL_REG_GET_BYPASS(reg) \
> + (((reg) & (BIT(13))) ? 1 : 0)
> +#define PLL_REG_GET_NOUPD(reg) \
> + (((reg) & (BIT(14))) ? 1 : 0)
> +#define PLL_REG_GET_PAD(reg) \
> + (((reg) & (0x1 << 15)) >> 15)
> +
> +#define PLL_REG_SET_LOW(reg, value) \
> + { reg |= (((value) & 0x3F) << 0); }
> +#define PLL_REG_SET_HIGH(reg, value) \
> + { reg |= (((value) & 0x3F) << 6); }
> +#define PLL_REG_SET_EDGE(reg, value) \
> + { reg |= (((value) & 0x01) << 12); }
> +#define PLL_REG_SET_BYPASS(reg, value) \
> + { reg |= (((value) & 0x01) << 13); }
> +#define PLL_REG_SET_NOUPD(reg, value)\
> + { reg |= (((value) & 0x01) << 14); }
> +#define PLL_REG_SET_PAD(reg, value) \
> + { reg |= (((value) & 0x1) << 15); }
> +
> +#define PLL_LOCK 0x1
> +#define PLL_MAX_LOCK_TIME 100 /* 100 us */
> +
> +struct pll_cfg {
> + u32 rate;
> + u32 idiv;
> + u32 fbdiv;
> + u32 odiv;
> +};
> +
> +struct pll_of_table {
> + unsigned long prate;
> + struct pll_cfg *
Re: [PATCH v2] clk/axs10x: introduce AXS10X pll driver
Hi Stephen,
On Tue, 2017-04-04 at 18:35 -0700, Stephen Boyd wrote:
> > + .pll_table = (struct pll_of_table []){
> > + {
> > + .prate = 2700,
>
> Can this be another clk in the framework instead of hardcoding
> the parent rate?
In fact there is another clk in the framework that represents this parent
clock. But this field is needed to get
appropriate pll_cfg_table as it depends on parent clock frequency. Below in
pll_cfg_get function we are searching for
the correct table comparing .parent_node field with real hardware parent clock
frequency:
-->8
for (i = 0; pll_table[i].prate != 0; i++)
if (pll_table[i].prate == prate)
return pll_table[i].pll_cfg_table;
-->8
>
> > + .pll_cfg_table = (struct pll_cfg []){
> > + { 2520, 1, 84, 90 },
> > + { 5000, 1, 100, 54 },
> > + { 7425, 1, 44, 16 },
> > + { },
> > + },
> > + },
> > + /* Used as list limiter */
> > + { },
>
> There's only ever one, so I'm confused why we're making a list.
By this patch we only add support of core arc pll and pgu pll and today they
are clocked by the only parent clocks
introduced here. But other plls on axs10x may be driven by different or
configurable clocks, so in such cases we will
have more than one entry in this list. And we are going to add more supported
plls to this driver in the nearest future.
> > +
> > + clk = clk_register(NULL, &pll_clk->hw);
> > + if (IS_ERR(clk)) {
> > + pr_err("failed to register %s clock (%ld)\n",
> > + node->name, PTR_ERR(clk));
> > + kfree(pll_clk);
> > + return;
> > + }
> > +
> > + of_clk_add_provider(node, of_clk_src_simple_get, clk);
>
> Can you please use the clk_hw based provider and clk registration
> functions?
Sure. Could you be so kind to explain what is the difference between hw and
non-hw based provider and clk registration
functions please? In which cases they are preferred?
>
> > +}
> > +
> > +CLK_OF_DECLARE(axs10x_pll_clock, "snps,axs10x-arc-pll-clock",
> > of_pll_clk_setup);
>
> Does this need to be CLK_OF_DECLARE_DRIVER? I mean does the
> driver need to probe and also have this of declare happen? Is the
> PLL special and needs to be used for the timers?
It is special and is used for the timers, so we have to CLK_OF_DECLARE it. On
the other hand similar pll is used to
drive PGU clock frequency and other subsystems and so we add usual probe func.
--
Best regards,
Vlad Zakharov
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
[PATCH] ldso/arc: fix debug prints
Signed-off-by: Vineet Gupta --- ldso/ldso/arc/elfinterp.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ldso/ldso/arc/elfinterp.c b/ldso/ldso/arc/elfinterp.c index 2f0cf7f6635b..c164d5dae6bf 100644 --- a/ldso/ldso/arc/elfinterp.c +++ b/ldso/ldso/arc/elfinterp.c @@ -179,8 +179,8 @@ _dl_do_reloc(struct elf_resolve *tpnt, struct r_scope_elem *scope, log_entry: #if defined __SUPPORT_LD_DEBUG__ if (_dl_debug_detail) - _dl_dprintf(_dl_debug_file,"\tpatched: %lx ==> %lx @ %pl: addend %x ", - old_val, *reloc_addr, reloc_addr, rpnt->r_addend); + _dl_dprintf(_dl_debug_file,"\tpatched: %x ==> %x @ %x", + old_val, *reloc_addr, reloc_addr); #endif return 0; @@ -215,7 +215,7 @@ _dl_do_lazy_reloc(struct elf_resolve *tpnt, struct r_scope_elem *scope, #if defined __SUPPORT_LD_DEBUG__ if (_dl_debug_reloc && _dl_debug_detail) - _dl_dprintf(_dl_debug_file, "\tpatched: %lx ==> %lx @ %pl\n", + _dl_dprintf(_dl_debug_file, "\tpatched: %x ==> %x @ %x\n", old_val, *reloc_addr, reloc_addr); #endif -- 2.7.4 ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
[PATCH] ldso: exit if zalloc can't alloc memory
_dl_zalloc callers don't check for allocaton failure. It kind of makes sense since such early allocations are unlikely to fail, and even if they do, ldso would segv anyways thus bringing the issue to attention. However there's a gcc nuance which led to this patch. It seems gcc at -O2 (for DODEBUG build), does additional coge gen analysis for pointer dereference from erroneous paths and it "isolates" such code paths with a intrinsic abort/trap etc. The ldso code fragment which was triggering this: | add_ldso(struct dyn_elf *rpnt) |if (rpnt) |... |else | rpnt = _dl_zalloc() | |rpnt->dyn = tpnt < potential NULL pointer deref ARC gcc currently generates an abort() call which doesn't exist in ldso, causing link errors (with a newer vrsion of binutils). ARM gcc 6.2.1 behaves similarly, altough instead of abort, it generates a trap inducing UDF instruction |367c: ebfffb79 bl 2468 <_dl_malloc> |3680: e51b2068 ldr r2, [fp, #-104] ; 0xff98 |3684: e350 cmp r0, #0 |3688: 0a06 beq 36a8 <_dl_add_elf_hash_table+0x280> | ... |36a8: e5862000 str r2, [r6] |36ac: e7f000f0 udf # So add an explict dl_exit() in dl_zalloc error case to beat the compiler. Note that this error propagagtion analysis stops if the function in consideration (_dl_zalloc) is NOT inlined. Hence the reason it only shows up for DODEBUG builds which builds ldso at -O2 which is more aggressive about inlining. If this patch is not considered worth applying then the workaround suggested by Claudiu is to to build ldso with -fno-isolate-erroneous-paths-dereference Signed-off-by: Vineet Gupta --- ldso/ldso/ldso.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ldso/ldso/ldso.c b/ldso/ldso/ldso.c index 4e8a49ef5f91..539975bcfb0c 100644 --- a/ldso/ldso/ldso.c +++ b/ldso/ldso/ldso.c @@ -259,6 +259,8 @@ static void *_dl_zalloc(size_t size) void *p = _dl_malloc(size); if (p) _dl_memset(p, 0, size); + else + _dl_exit(1); return p; } -- 2.7.4 ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
