[linux-sunxi] Re: [PATCH v5 15/26] clk: sunxi: Add ccu clock tree support
On Thu, Jan 10, 2019 at 6:21 AM André Przywara wrote: > > On 08/01/2019 19:12, Jagan Teki wrote: > > On Tue, Jan 8, 2019 at 5:09 PM Andre Przywara > > wrote: > >> > >> On Tue, 8 Jan 2019 16:27:14 +0530 > >> Jagan Teki wrote: > >> > >> Hi, > >> > >>> On Mon, Jan 7, 2019 at 6:35 AM André Przywara > >>> wrote: > > On 31/12/2018 16:59, Jagan Teki wrote: > > Clock control unit comprises of parent clocks, gates, > > multiplexers, dividers, multipliers, pre/post dividers and flags > > etc. > > > > So, the U-Boot implementation of ccu has divided into gates and > > tree. gates are generic clock configuration of enable/disable bit > > management which can be handle via ccu_clock_gate. > > So if I understand this correctly, you implement the gate > functionality separately from the complex clock code, even if they > are the same clock from the DT point of view? So if one wants to > enable the MMC0 clock, which is a mux/divider clock, one needs to > specify an extra entry in the gate array to describe the enable bit > in this special clock register? Sounds a bit surprising, but is > probably a neat trick to keep things simple. There should be a > comment in the code to this regard then. > >>> > >>> Exactly. Idea is to keep the macro's as simple as possible. > >>> > >>> Adding gates clocks separately make easy and reasonable way to > >>> enable/disable clock operations. We even operate with single macro > >>> with all clock attributes along with gate(either another member or > >>> common structure like Linux does), but that seems not simple as per as > >>> my experince since there are many IP's like USB's just need > >>> enable/disable. > >>> > > > Tree clocks are parent clock type, fixed clocks, mp, nk, nkm, > > nkmp, pre/post div, flags etc. which were managed via > > ccu_clock_tree. > > For a start, can we use more descriptive names than those very > specific MP/NK names? DIV_MUX and PLL sound more descriptive to me. > I understand that Linux uses those terms, but it would be great if > uninitiated people have a chance to understand this as well. > > > This patch add support for MP, NK, MISC, FIXED clock types as > > part of ccu clock tree with get_rate functionality this > > eventually used by uart driver. and rest of the infrastructure > > will try to add while CLK is being used on respective peripherals. > > > > Note that few of the tree type clock would require to enable > > gates on their specific clock, in that case we need to add the > > gate details via ccu_clock_gate, example: MP with gate so the > > gate offset, bit value should add as part of ccu_clock_gate. > > > > Signed-off-by: Jagan Teki > > --- > > arch/arm/include/asm/arch-sunxi/ccu.h | 192 > > +- drivers/clk/sunxi/clk_a64.c > > | 40 ++ drivers/clk/sunxi/clk_sunxi.c | 182 > > 3 files changed, 413 insertions(+), 1 > > deletion(-) > > > > diff --git a/arch/arm/include/asm/arch-sunxi/ccu.h > > b/arch/arm/include/asm/arch-sunxi/ccu.h index > > 3fdc26978d..61b8c36b3b 100644 --- > > a/arch/arm/include/asm/arch-sunxi/ccu.h +++ > > b/arch/arm/include/asm/arch-sunxi/ccu.h @@ -7,15 +7,204 @@ > > #ifndef _ASM_ARCH_CCU_H > > #define _ASM_ARCH_CCU_H > > > > +#define OSC_32K_ULL 32000ULL > > 32768 > > And why ULL? The whole Allwinner clock system works with 32-bit > values, so just U would be totally sufficient. This avoid blowing > this up to 64 bit unnecessarily, which sounds painful for those > poor ARMv7 parts. > > +#define OSC_24M_ULL 2400ULL > > + > > +/** > > + * enum ccu_clk_type - ccu clock types > > + * > > + * @CCU_CLK_TYPE_MISC: misc clock type > > What is MISC, exactly? Seems like an artefact clock to me, some > placeholder you need because gate clocks are handled separately in > the gates struct. Should this be called something with SIMPLE > instead, or GATE? > >>> > >>> Unlike MP type in MMC, UART doesn't need any clock attributes like > >>> dividers, mux, etc, just have attached parent. I don't think UART > >>> clock is simple one It has parent, that indeed have another parents > >>> and so...on, ie reason I named as MISC. > >> > >> Not really, as far as I can see the UART clock is a just a gate clock as > >> many others, with one parent (APB2). > >> The fact that APB2 in turn can have multiple parents doesn't affect the > >> UART clock itself, as you model this via the clock tree. > >> > >> In fact we could have similar clocks in the tree structure for the > >> other gate clocks (USB, for instance), it's just that the UART is the > >> only user so far which actually queries the clock rate. > >> > >> So MISC is way
[linux-sunxi] Re: [PATCH v5 15/26] clk: sunxi: Add ccu clock tree support
On 08/01/2019 19:12, Jagan Teki wrote: > On Tue, Jan 8, 2019 at 5:09 PM Andre Przywara wrote: >> >> On Tue, 8 Jan 2019 16:27:14 +0530 >> Jagan Teki wrote: >> >> Hi, >> >>> On Mon, Jan 7, 2019 at 6:35 AM André Przywara >>> wrote: On 31/12/2018 16:59, Jagan Teki wrote: > Clock control unit comprises of parent clocks, gates, > multiplexers, dividers, multipliers, pre/post dividers and flags > etc. > > So, the U-Boot implementation of ccu has divided into gates and > tree. gates are generic clock configuration of enable/disable bit > management which can be handle via ccu_clock_gate. So if I understand this correctly, you implement the gate functionality separately from the complex clock code, even if they are the same clock from the DT point of view? So if one wants to enable the MMC0 clock, which is a mux/divider clock, one needs to specify an extra entry in the gate array to describe the enable bit in this special clock register? Sounds a bit surprising, but is probably a neat trick to keep things simple. There should be a comment in the code to this regard then. >>> >>> Exactly. Idea is to keep the macro's as simple as possible. >>> >>> Adding gates clocks separately make easy and reasonable way to >>> enable/disable clock operations. We even operate with single macro >>> with all clock attributes along with gate(either another member or >>> common structure like Linux does), but that seems not simple as per as >>> my experince since there are many IP's like USB's just need >>> enable/disable. >>> > Tree clocks are parent clock type, fixed clocks, mp, nk, nkm, > nkmp, pre/post div, flags etc. which were managed via > ccu_clock_tree. For a start, can we use more descriptive names than those very specific MP/NK names? DIV_MUX and PLL sound more descriptive to me. I understand that Linux uses those terms, but it would be great if uninitiated people have a chance to understand this as well. > This patch add support for MP, NK, MISC, FIXED clock types as > part of ccu clock tree with get_rate functionality this > eventually used by uart driver. and rest of the infrastructure > will try to add while CLK is being used on respective peripherals. > > Note that few of the tree type clock would require to enable > gates on their specific clock, in that case we need to add the > gate details via ccu_clock_gate, example: MP with gate so the > gate offset, bit value should add as part of ccu_clock_gate. > > Signed-off-by: Jagan Teki > --- > arch/arm/include/asm/arch-sunxi/ccu.h | 192 > +- drivers/clk/sunxi/clk_a64.c > | 40 ++ drivers/clk/sunxi/clk_sunxi.c | 182 > 3 files changed, 413 insertions(+), 1 > deletion(-) > > diff --git a/arch/arm/include/asm/arch-sunxi/ccu.h > b/arch/arm/include/asm/arch-sunxi/ccu.h index > 3fdc26978d..61b8c36b3b 100644 --- > a/arch/arm/include/asm/arch-sunxi/ccu.h +++ > b/arch/arm/include/asm/arch-sunxi/ccu.h @@ -7,15 +7,204 @@ > #ifndef _ASM_ARCH_CCU_H > #define _ASM_ARCH_CCU_H > > +#define OSC_32K_ULL 32000ULL 32768 And why ULL? The whole Allwinner clock system works with 32-bit values, so just U would be totally sufficient. This avoid blowing this up to 64 bit unnecessarily, which sounds painful for those poor ARMv7 parts. > +#define OSC_24M_ULL 2400ULL > + > +/** > + * enum ccu_clk_type - ccu clock types > + * > + * @CCU_CLK_TYPE_MISC: misc clock type What is MISC, exactly? Seems like an artefact clock to me, some placeholder you need because gate clocks are handled separately in the gates struct. Should this be called something with SIMPLE instead, or GATE? >>> >>> Unlike MP type in MMC, UART doesn't need any clock attributes like >>> dividers, mux, etc, just have attached parent. I don't think UART >>> clock is simple one It has parent, that indeed have another parents >>> and so...on, ie reason I named as MISC. >> >> Not really, as far as I can see the UART clock is a just a gate clock as >> many others, with one parent (APB2). >> The fact that APB2 in turn can have multiple parents doesn't affect the >> UART clock itself, as you model this via the clock tree. >> >> In fact we could have similar clocks in the tree structure for the >> other gate clocks (USB, for instance), it's just that the UART is the >> only user so far which actually queries the clock rate. >> >> So MISC is way too generic, I would still prefer CCU_CLK_TYPE_GATE. > > TYPE_GATE is more sense. fine for me. > >> > + * @CCU_CLK_TYPE_FIXED: fixed clock type > + * @CCU_CLK_TYPE_MP: mp clock type > + * @CCU_CLK_TYPE_NK: nk clock
[linux-sunxi] Re: [PATCH v5 15/26] clk: sunxi: Add ccu clock tree support
On Tue, Jan 8, 2019 at 5:09 PM Andre Przywara wrote: > > On Tue, 8 Jan 2019 16:27:14 +0530 > Jagan Teki wrote: > > Hi, > > > On Mon, Jan 7, 2019 at 6:35 AM André Przywara > > wrote: > > > > > > On 31/12/2018 16:59, Jagan Teki wrote: > > > > Clock control unit comprises of parent clocks, gates, > > > > multiplexers, dividers, multipliers, pre/post dividers and flags > > > > etc. > > > > > > > > So, the U-Boot implementation of ccu has divided into gates and > > > > tree. gates are generic clock configuration of enable/disable bit > > > > management which can be handle via ccu_clock_gate. > > > > > > So if I understand this correctly, you implement the gate > > > functionality separately from the complex clock code, even if they > > > are the same clock from the DT point of view? So if one wants to > > > enable the MMC0 clock, which is a mux/divider clock, one needs to > > > specify an extra entry in the gate array to describe the enable bit > > > in this special clock register? Sounds a bit surprising, but is > > > probably a neat trick to keep things simple. There should be a > > > comment in the code to this regard then. > > > > Exactly. Idea is to keep the macro's as simple as possible. > > > > Adding gates clocks separately make easy and reasonable way to > > enable/disable clock operations. We even operate with single macro > > with all clock attributes along with gate(either another member or > > common structure like Linux does), but that seems not simple as per as > > my experince since there are many IP's like USB's just need > > enable/disable. > > > > > > > > > Tree clocks are parent clock type, fixed clocks, mp, nk, nkm, > > > > nkmp, pre/post div, flags etc. which were managed via > > > > ccu_clock_tree. > > > > > > For a start, can we use more descriptive names than those very > > > specific MP/NK names? DIV_MUX and PLL sound more descriptive to me. > > > I understand that Linux uses those terms, but it would be great if > > > uninitiated people have a chance to understand this as well. > > > > > > > This patch add support for MP, NK, MISC, FIXED clock types as > > > > part of ccu clock tree with get_rate functionality this > > > > eventually used by uart driver. and rest of the infrastructure > > > > will try to add while CLK is being used on respective peripherals. > > > > > > > > Note that few of the tree type clock would require to enable > > > > gates on their specific clock, in that case we need to add the > > > > gate details via ccu_clock_gate, example: MP with gate so the > > > > gate offset, bit value should add as part of ccu_clock_gate. > > > > > > > > Signed-off-by: Jagan Teki > > > > --- > > > > arch/arm/include/asm/arch-sunxi/ccu.h | 192 > > > > +- drivers/clk/sunxi/clk_a64.c > > > > | 40 ++ drivers/clk/sunxi/clk_sunxi.c | 182 > > > > 3 files changed, 413 insertions(+), 1 > > > > deletion(-) > > > > > > > > diff --git a/arch/arm/include/asm/arch-sunxi/ccu.h > > > > b/arch/arm/include/asm/arch-sunxi/ccu.h index > > > > 3fdc26978d..61b8c36b3b 100644 --- > > > > a/arch/arm/include/asm/arch-sunxi/ccu.h +++ > > > > b/arch/arm/include/asm/arch-sunxi/ccu.h @@ -7,15 +7,204 @@ > > > > #ifndef _ASM_ARCH_CCU_H > > > > #define _ASM_ARCH_CCU_H > > > > > > > > +#define OSC_32K_ULL 32000ULL > > > > > > 32768 > > > > > > And why ULL? The whole Allwinner clock system works with 32-bit > > > values, so just U would be totally sufficient. This avoid blowing > > > this up to 64 bit unnecessarily, which sounds painful for those > > > poor ARMv7 parts. > > > > +#define OSC_24M_ULL 2400ULL > > > > + > > > > +/** > > > > + * enum ccu_clk_type - ccu clock types > > > > + * > > > > + * @CCU_CLK_TYPE_MISC: misc clock type > > > > > > What is MISC, exactly? Seems like an artefact clock to me, some > > > placeholder you need because gate clocks are handled separately in > > > the gates struct. Should this be called something with SIMPLE > > > instead, or GATE? > > > > Unlike MP type in MMC, UART doesn't need any clock attributes like > > dividers, mux, etc, just have attached parent. I don't think UART > > clock is simple one It has parent, that indeed have another parents > > and so...on, ie reason I named as MISC. > > Not really, as far as I can see the UART clock is a just a gate clock as > many others, with one parent (APB2). > The fact that APB2 in turn can have multiple parents doesn't affect the > UART clock itself, as you model this via the clock tree. > > In fact we could have similar clocks in the tree structure for the > other gate clocks (USB, for instance), it's just that the UART is the > only user so far which actually queries the clock rate. > > So MISC is way too generic, I would still prefer CCU_CLK_TYPE_GATE. TYPE_GATE is more sense. fine for me. > > > > > + * @CCU_CLK_TYPE_FIXED: fixed clock type > > > > + * @CCU_CLK_TYPE_MP: mp clock
[linux-sunxi] Re: [PATCH v5 15/26] clk: sunxi: Add ccu clock tree support
On Tue, 8 Jan 2019 16:27:14 +0530 Jagan Teki wrote: Hi, > On Mon, Jan 7, 2019 at 6:35 AM André Przywara > wrote: > > > > On 31/12/2018 16:59, Jagan Teki wrote: > > > Clock control unit comprises of parent clocks, gates, > > > multiplexers, dividers, multipliers, pre/post dividers and flags > > > etc. > > > > > > So, the U-Boot implementation of ccu has divided into gates and > > > tree. gates are generic clock configuration of enable/disable bit > > > management which can be handle via ccu_clock_gate. > > > > So if I understand this correctly, you implement the gate > > functionality separately from the complex clock code, even if they > > are the same clock from the DT point of view? So if one wants to > > enable the MMC0 clock, which is a mux/divider clock, one needs to > > specify an extra entry in the gate array to describe the enable bit > > in this special clock register? Sounds a bit surprising, but is > > probably a neat trick to keep things simple. There should be a > > comment in the code to this regard then. > > Exactly. Idea is to keep the macro's as simple as possible. > > Adding gates clocks separately make easy and reasonable way to > enable/disable clock operations. We even operate with single macro > with all clock attributes along with gate(either another member or > common structure like Linux does), but that seems not simple as per as > my experince since there are many IP's like USB's just need > enable/disable. > > > > > > Tree clocks are parent clock type, fixed clocks, mp, nk, nkm, > > > nkmp, pre/post div, flags etc. which were managed via > > > ccu_clock_tree. > > > > For a start, can we use more descriptive names than those very > > specific MP/NK names? DIV_MUX and PLL sound more descriptive to me. > > I understand that Linux uses those terms, but it would be great if > > uninitiated people have a chance to understand this as well. > > > > > This patch add support for MP, NK, MISC, FIXED clock types as > > > part of ccu clock tree with get_rate functionality this > > > eventually used by uart driver. and rest of the infrastructure > > > will try to add while CLK is being used on respective peripherals. > > > > > > Note that few of the tree type clock would require to enable > > > gates on their specific clock, in that case we need to add the > > > gate details via ccu_clock_gate, example: MP with gate so the > > > gate offset, bit value should add as part of ccu_clock_gate. > > > > > > Signed-off-by: Jagan Teki > > > --- > > > arch/arm/include/asm/arch-sunxi/ccu.h | 192 > > > +- drivers/clk/sunxi/clk_a64.c > > > | 40 ++ drivers/clk/sunxi/clk_sunxi.c | 182 > > > 3 files changed, 413 insertions(+), 1 > > > deletion(-) > > > > > > diff --git a/arch/arm/include/asm/arch-sunxi/ccu.h > > > b/arch/arm/include/asm/arch-sunxi/ccu.h index > > > 3fdc26978d..61b8c36b3b 100644 --- > > > a/arch/arm/include/asm/arch-sunxi/ccu.h +++ > > > b/arch/arm/include/asm/arch-sunxi/ccu.h @@ -7,15 +7,204 @@ > > > #ifndef _ASM_ARCH_CCU_H > > > #define _ASM_ARCH_CCU_H > > > > > > +#define OSC_32K_ULL 32000ULL > > > > 32768 > > > > And why ULL? The whole Allwinner clock system works with 32-bit > > values, so just U would be totally sufficient. This avoid blowing > > this up to 64 bit unnecessarily, which sounds painful for those > > poor ARMv7 parts. > > > +#define OSC_24M_ULL 2400ULL > > > + > > > +/** > > > + * enum ccu_clk_type - ccu clock types > > > + * > > > + * @CCU_CLK_TYPE_MISC: misc clock type > > > > What is MISC, exactly? Seems like an artefact clock to me, some > > placeholder you need because gate clocks are handled separately in > > the gates struct. Should this be called something with SIMPLE > > instead, or GATE? > > Unlike MP type in MMC, UART doesn't need any clock attributes like > dividers, mux, etc, just have attached parent. I don't think UART > clock is simple one It has parent, that indeed have another parents > and so...on, ie reason I named as MISC. Not really, as far as I can see the UART clock is a just a gate clock as many others, with one parent (APB2). The fact that APB2 in turn can have multiple parents doesn't affect the UART clock itself, as you model this via the clock tree. In fact we could have similar clocks in the tree structure for the other gate clocks (USB, for instance), it's just that the UART is the only user so far which actually queries the clock rate. So MISC is way too generic, I would still prefer CCU_CLK_TYPE_GATE. > > > + * @CCU_CLK_TYPE_FIXED: fixed clock type > > > + * @CCU_CLK_TYPE_MP: mp clock type > > > + * @CCU_CLK_TYPE_NK: nk clock type > > > > What is the point of those comments, as you are basically repeating > > the enum name? What about: > > * @CCU_CLK_TYPE_PLL: PLL clock with two multiplier fields > > * @CCU_CLK_TYPE_MUX_DIV:
[linux-sunxi] Re: [PATCH v5 15/26] clk: sunxi: Add ccu clock tree support
On Mon, Jan 7, 2019 at 6:35 AM André Przywara wrote: > > On 31/12/2018 16:59, Jagan Teki wrote: > > Clock control unit comprises of parent clocks, gates, multiplexers, > > dividers, multipliers, pre/post dividers and flags etc. > > > > So, the U-Boot implementation of ccu has divided into gates and tree. > > gates are generic clock configuration of enable/disable bit management > > which can be handle via ccu_clock_gate. > > So if I understand this correctly, you implement the gate functionality > separately from the complex clock code, even if they are the same clock > from the DT point of view? So if one wants to enable the MMC0 clock, > which is a mux/divider clock, one needs to specify an extra entry in the > gate array to describe the enable bit in this special clock register? > Sounds a bit surprising, but is probably a neat trick to keep things > simple. There should be a comment in the code to this regard then. > > > Tree clocks are parent clock type, fixed clocks, mp, nk, nkm, nkmp, > > pre/post div, flags etc. which were managed via ccu_clock_tree. > > For a start, can we use more descriptive names than those very specific > MP/NK names? DIV_MUX and PLL sound more descriptive to me. I understand > that Linux uses those terms, but it would be great if uninitiated people > have a chance to understand this as well. > > > This patch add support for MP, NK, MISC, FIXED clock types as part of > > ccu clock tree with get_rate functionality this eventually used by > > uart driver. and rest of the infrastructure will try to add while CLK > > is being used on respective peripherals. > > > > Note that few of the tree type clock would require to enable gates on > > their specific clock, in that case we need to add the gate details via > > ccu_clock_gate, example: MP with gate so the gate offset, bit value > > should add as part of ccu_clock_gate. > > > > Signed-off-by: Jagan Teki > > --- > > arch/arm/include/asm/arch-sunxi/ccu.h | 192 +- > > drivers/clk/sunxi/clk_a64.c | 40 ++ > > drivers/clk/sunxi/clk_sunxi.c | 182 > > 3 files changed, 413 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm/include/asm/arch-sunxi/ccu.h > > b/arch/arm/include/asm/arch-sunxi/ccu.h > > index 3fdc26978d..61b8c36b3b 100644 > > --- a/arch/arm/include/asm/arch-sunxi/ccu.h > > +++ b/arch/arm/include/asm/arch-sunxi/ccu.h > > @@ -7,15 +7,204 @@ > > #ifndef _ASM_ARCH_CCU_H > > #define _ASM_ARCH_CCU_H > > > > +#define OSC_32K_ULL 32000ULL > > 32768 > > And why ULL? The whole Allwinner clock system works with 32-bit values, > so just U would be totally sufficient. This avoid blowing this up to 64 > bit unnecessarily, which sounds painful for those poor ARMv7 parts. > > > +#define OSC_24M_ULL 2400ULL > > + > > +/** > > + * enum ccu_clk_type - ccu clock types > > + * > > + * @CCU_CLK_TYPE_MISC: misc clock type > > What is MISC, exactly? Seems like an artefact clock to me, some > placeholder you need because gate clocks are handled separately in the > gates struct. Should this be called something with SIMPLE instead, or GATE? > > > + * @CCU_CLK_TYPE_FIXED: fixed clock type > > + * @CCU_CLK_TYPE_MP: mp clock type > > + * @CCU_CLK_TYPE_NK: nk clock type > > What is the point of those comments, as you are basically repeating the > enum name? What about: > * @CCU_CLK_TYPE_PLL: PLL clock with two multiplier fields > * @CCU_CLK_TYPE_MUX_DIV: multiple parents, two divider fields > > This gives more speaking names, plus some explanation. My idea is to give generic name for a given clock type for example MP clock has different varients like MP clock, MP with DIV, MP with Postdiv, MP can be MMC etc. same like NK clock type as NK with postdiv, NK can be PLL. With this we can expose the base name to outside and keep fill the require variants during macro initialization. This can avoid to many names on the same clock based on the different variants. Yes we can add proper full detailed comments on the given type. -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
[linux-sunxi] Re: [PATCH v5 15/26] clk: sunxi: Add ccu clock tree support
On Mon, Jan 7, 2019 at 6:35 AM André Przywara wrote:
>
> On 31/12/2018 16:59, Jagan Teki wrote:
> > Clock control unit comprises of parent clocks, gates, multiplexers,
> > dividers, multipliers, pre/post dividers and flags etc.
> >
> > So, the U-Boot implementation of ccu has divided into gates and tree.
> > gates are generic clock configuration of enable/disable bit management
> > which can be handle via ccu_clock_gate.
>
> So if I understand this correctly, you implement the gate functionality
> separately from the complex clock code, even if they are the same clock
> from the DT point of view? So if one wants to enable the MMC0 clock,
> which is a mux/divider clock, one needs to specify an extra entry in the
> gate array to describe the enable bit in this special clock register?
> Sounds a bit surprising, but is probably a neat trick to keep things
> simple. There should be a comment in the code to this regard then.
Exactly. Idea is to keep the macro's as simple as possible.
Adding gates clocks separately make easy and reasonable way to
enable/disable clock operations. We even operate with single macro
with all clock attributes along with gate(either another member or
common structure like Linux does), but that seems not simple as per as
my experince since there are many IP's like USB's just need
enable/disable.
>
> > Tree clocks are parent clock type, fixed clocks, mp, nk, nkm, nkmp,
> > pre/post div, flags etc. which were managed via ccu_clock_tree.
>
> For a start, can we use more descriptive names than those very specific
> MP/NK names? DIV_MUX and PLL sound more descriptive to me. I understand
> that Linux uses those terms, but it would be great if uninitiated people
> have a chance to understand this as well.
>
> > This patch add support for MP, NK, MISC, FIXED clock types as part of
> > ccu clock tree with get_rate functionality this eventually used by
> > uart driver. and rest of the infrastructure will try to add while CLK
> > is being used on respective peripherals.
> >
> > Note that few of the tree type clock would require to enable gates on
> > their specific clock, in that case we need to add the gate details via
> > ccu_clock_gate, example: MP with gate so the gate offset, bit value
> > should add as part of ccu_clock_gate.
> >
> > Signed-off-by: Jagan Teki
> > ---
> > arch/arm/include/asm/arch-sunxi/ccu.h | 192 +-
> > drivers/clk/sunxi/clk_a64.c | 40 ++
> > drivers/clk/sunxi/clk_sunxi.c | 182
> > 3 files changed, 413 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/include/asm/arch-sunxi/ccu.h
> > b/arch/arm/include/asm/arch-sunxi/ccu.h
> > index 3fdc26978d..61b8c36b3b 100644
> > --- a/arch/arm/include/asm/arch-sunxi/ccu.h
> > +++ b/arch/arm/include/asm/arch-sunxi/ccu.h
> > @@ -7,15 +7,204 @@
> > #ifndef _ASM_ARCH_CCU_H
> > #define _ASM_ARCH_CCU_H
> >
> > +#define OSC_32K_ULL 32000ULL
>
> 32768
>
> And why ULL? The whole Allwinner clock system works with 32-bit values,
> so just U would be totally sufficient. This avoid blowing this up to 64
> bit unnecessarily, which sounds painful for those poor ARMv7 parts.
>
> > +#define OSC_24M_ULL 2400ULL
> > +
> > +/**
> > + * enum ccu_clk_type - ccu clock types
> > + *
> > + * @CCU_CLK_TYPE_MISC: misc clock type
>
> What is MISC, exactly? Seems like an artefact clock to me, some
> placeholder you need because gate clocks are handled separately in the
> gates struct. Should this be called something with SIMPLE instead, or GATE?
Unlike MP type in MMC, UART doesn't need any clock attributes like
dividers, mux, etc, just have attached parent. I don't think UART
clock is simple one It has parent, that indeed have another parents
and so...on, ie reason I named as MISC.
>
> > + * @CCU_CLK_TYPE_FIXED: fixed clock type
> > + * @CCU_CLK_TYPE_MP: mp clock type
> > + * @CCU_CLK_TYPE_NK: nk clock type
>
> What is the point of those comments, as you are basically repeating the
> enum name? What about:
> * @CCU_CLK_TYPE_PLL: PLL clock with two multiplier fields
> * @CCU_CLK_TYPE_MUX_DIV: multiple parents, two divider fields
>
> This gives more speaking names, plus some explanation.
>
> > + */
> > +enum ccu_clk_type {
> > + CCU_CLK_TYPE_MISC = 0,
> > + CCU_CLK_TYPE_FIXED = 1,
> > + CCU_CLK_TYPE_MP = 2,
> > + CCU_CLK_TYPE_NK = 3,
> > +};
> > +
> > /**
> > * enum ccu_clk_flags - ccu clock flags
> > *
> > - * @CCU_CLK_F_INIT_DONE: clock gate init done check
> > + * @CCU_CLK_F_INIT_DONE: clock tree/gate init done check
>
> Is this flag to tell implemented clocks apart from unimplemented ones,
> which would be reset to all zeroes by the compiler? Then it should be
> called something with VALID in it.
Since the flags attached with real numeric initialized by
[linux-sunxi] Re: [PATCH v5 15/26] clk: sunxi: Add ccu clock tree support
On Mon, Jan 07, 2019 at 02:09:12PM +, Andre Przywara wrote: > > > What is MISC, exactly? Seems like an artefact clock to me, some > > > placeholder you need because gate clocks are handled separately in > > > the gates struct. Should this be called something with SIMPLE > > > instead, or GATE? > > > > + * @CCU_CLK_TYPE_FIXED:fixed clock type > > > > + * @CCU_CLK_TYPE_MP: mp clock type > > > > + * @CCU_CLK_TYPE_NK: nk clock type > > > > > > What is the point of those comments, as you are basically repeating > > > the enum name? What about: > > > * @CCU_CLK_TYPE_PLL: PLL clock with two multiplier > > > fields > > > > We have PLL with 2 multipliers, but also others with other factors > > sets, so that will end up being confusing. If the MP, NK and so on > > stuff is confusing, maybe we should just add a comment on top of that > > structure to explain what those factors are and what it actually > > means? > > Fair enough, or we name it CCU_CLK_TYPE_PLL_NK, because this is what > this type deals with. Point is that by chance I happened to know about > those naming of factors in the manual, but other might be lost by just > seeing "mp" or "nk", without any explanation - and the comment doesn't > help here at all. Either way, we should really document this properly. > The other part is that the "TYPE_MP" is twice as confusing, as it can > perfectly describe the MMC clocks, which use "N" and "M" in the A64 > manual, for instance. That's why my suggestion for calling a spade a > spade and saying it's a divider clock with a multiplexer. Happy to have > the Linux naming in the comments. NM and MP aren't really the same though. NM is one multiplier and one divider, while MP is one divider and one right shift. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout. signature.asc Description: PGP signature
[linux-sunxi] Re: [PATCH v5 15/26] clk: sunxi: Add ccu clock tree support
On Mon, 7 Jan 2019 14:01:01 +0100 Maxime Ripard wrote: Hi, > On Mon, Jan 07, 2019 at 01:03:33AM +, André Przywara wrote: > > On 31/12/2018 16:59, Jagan Teki wrote: > > > Clock control unit comprises of parent clocks, gates, > > > multiplexers, dividers, multipliers, pre/post dividers and flags > > > etc. > > > > > > So, the U-Boot implementation of ccu has divided into gates and > > > tree. gates are generic clock configuration of enable/disable bit > > > management which can be handle via ccu_clock_gate. > > > > So if I understand this correctly, you implement the gate > > functionality separately from the complex clock code, even if they > > are the same clock from the DT point of view? So if one wants to > > enable the MMC0 clock, which is a mux/divider clock, one needs to > > specify an extra entry in the gate array to describe the enable bit > > in this special clock register? Sounds a bit surprising, but is > > probably a neat trick to keep things simple. There should be a > > comment in the code to this regard then. > > > Tree clocks are parent clock type, fixed clocks, mp, nk, nkm, > > > nkmp, pre/post div, flags etc. which were managed via > > > ccu_clock_tree. > > > > For a start, can we use more descriptive names than those very > > specific MP/NK names? DIV_MUX and PLL sound more descriptive to me. > > I understand that Linux uses those terms, but it would be great if > > uninitiated people have a chance to understand this as well. > > > > > This patch add support for MP, NK, MISC, FIXED clock types as > > > part of ccu clock tree with get_rate functionality this > > > eventually used by uart driver. and rest of the infrastructure > > > will try to add while CLK is being used on respective peripherals. > > > > > > Note that few of the tree type clock would require to enable > > > gates on their specific clock, in that case we need to add the > > > gate details via ccu_clock_gate, example: MP with gate so the > > > gate offset, bit value should add as part of ccu_clock_gate. > > > > > > Signed-off-by: Jagan Teki > > > --- > > > arch/arm/include/asm/arch-sunxi/ccu.h | 192 > > > +- drivers/clk/sunxi/clk_a64.c > > > | 40 ++ drivers/clk/sunxi/clk_sunxi.c | 182 > > > 3 files changed, 413 insertions(+), 1 > > > deletion(-) > > > > > > diff --git a/arch/arm/include/asm/arch-sunxi/ccu.h > > > b/arch/arm/include/asm/arch-sunxi/ccu.h index > > > 3fdc26978d..61b8c36b3b 100644 --- > > > a/arch/arm/include/asm/arch-sunxi/ccu.h +++ > > > b/arch/arm/include/asm/arch-sunxi/ccu.h @@ -7,15 +7,204 @@ > > > #ifndef _ASM_ARCH_CCU_H > > > #define _ASM_ARCH_CCU_H > > > > > > +#define OSC_32K_ULL 32000ULL > > > > 32768 > > > > And why ULL? The whole Allwinner clock system works with 32-bit > > values, so just U would be totally sufficient. This avoid blowing > > this up to 64 bit unnecessarily, which sounds painful for those > > poor ARMv7 parts. > > > +#define OSC_24M_ULL 2400ULL > > > + > > > +/** > > > + * enum ccu_clk_type - ccu clock types > > > + * > > > + * @CCU_CLK_TYPE_MISC: misc clock type > > > > What is MISC, exactly? Seems like an artefact clock to me, some > > placeholder you need because gate clocks are handled separately in > > the gates struct. Should this be called something with SIMPLE > > instead, or GATE? > > > + * @CCU_CLK_TYPE_FIXED: fixed clock type > > > + * @CCU_CLK_TYPE_MP: mp clock type > > > + * @CCU_CLK_TYPE_NK: nk clock type > > > > What is the point of those comments, as you are basically repeating > > the enum name? What about: > > * @CCU_CLK_TYPE_PLL: PLL clock with two multiplier > > fields > > We have PLL with 2 multipliers, but also others with other factors > sets, so that will end up being confusing. If the MP, NK and so on > stuff is confusing, maybe we should just add a comment on top of that > structure to explain what those factors are and what it actually > means? Fair enough, or we name it CCU_CLK_TYPE_PLL_NK, because this is what this type deals with. Point is that by chance I happened to know about those naming of factors in the manual, but other might be lost by just seeing "mp" or "nk", without any explanation - and the comment doesn't help here at all. The other part is that the "TYPE_MP" is twice as confusing, as it can perfectly describe the MMC clocks, which use "N" and "M" in the A64 manual, for instance. That's why my suggestion for calling a spade a spade and saying it's a divider clock with a multiplexer. Happy to have the Linux naming in the comments. Thanks, Andre. -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. For more options, visit
[linux-sunxi] Re: [PATCH v5 15/26] clk: sunxi: Add ccu clock tree support
On Mon, Jan 07, 2019 at 01:03:33AM +, André Przywara wrote: > On 31/12/2018 16:59, Jagan Teki wrote: > > Clock control unit comprises of parent clocks, gates, multiplexers, > > dividers, multipliers, pre/post dividers and flags etc. > > > > So, the U-Boot implementation of ccu has divided into gates and tree. > > gates are generic clock configuration of enable/disable bit management > > which can be handle via ccu_clock_gate. > > So if I understand this correctly, you implement the gate functionality > separately from the complex clock code, even if they are the same clock > from the DT point of view? So if one wants to enable the MMC0 clock, > which is a mux/divider clock, one needs to specify an extra entry in the > gate array to describe the enable bit in this special clock register? > Sounds a bit surprising, but is probably a neat trick to keep things > simple. There should be a comment in the code to this regard then. > > > Tree clocks are parent clock type, fixed clocks, mp, nk, nkm, nkmp, > > pre/post div, flags etc. which were managed via ccu_clock_tree. > > For a start, can we use more descriptive names than those very specific > MP/NK names? DIV_MUX and PLL sound more descriptive to me. I understand > that Linux uses those terms, but it would be great if uninitiated people > have a chance to understand this as well. > > > This patch add support for MP, NK, MISC, FIXED clock types as part of > > ccu clock tree with get_rate functionality this eventually used by > > uart driver. and rest of the infrastructure will try to add while CLK > > is being used on respective peripherals. > > > > Note that few of the tree type clock would require to enable gates on > > their specific clock, in that case we need to add the gate details via > > ccu_clock_gate, example: MP with gate so the gate offset, bit value > > should add as part of ccu_clock_gate. > > > > Signed-off-by: Jagan Teki > > --- > > arch/arm/include/asm/arch-sunxi/ccu.h | 192 +- > > drivers/clk/sunxi/clk_a64.c | 40 ++ > > drivers/clk/sunxi/clk_sunxi.c | 182 > > 3 files changed, 413 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm/include/asm/arch-sunxi/ccu.h > > b/arch/arm/include/asm/arch-sunxi/ccu.h > > index 3fdc26978d..61b8c36b3b 100644 > > --- a/arch/arm/include/asm/arch-sunxi/ccu.h > > +++ b/arch/arm/include/asm/arch-sunxi/ccu.h > > @@ -7,15 +7,204 @@ > > #ifndef _ASM_ARCH_CCU_H > > #define _ASM_ARCH_CCU_H > > > > +#define OSC_32K_ULL32000ULL > > 32768 > > And why ULL? The whole Allwinner clock system works with 32-bit values, > so just U would be totally sufficient. This avoid blowing this up to 64 > bit unnecessarily, which sounds painful for those poor ARMv7 parts. > > > +#define OSC_24M_ULL2400ULL > > + > > +/** > > + * enum ccu_clk_type - ccu clock types > > + * > > + * @CCU_CLK_TYPE_MISC: misc clock type > > What is MISC, exactly? Seems like an artefact clock to me, some > placeholder you need because gate clocks are handled separately in the > gates struct. Should this be called something with SIMPLE instead, or GATE? > > > + * @CCU_CLK_TYPE_FIXED:fixed clock type > > + * @CCU_CLK_TYPE_MP: mp clock type > > + * @CCU_CLK_TYPE_NK: nk clock type > > What is the point of those comments, as you are basically repeating the > enum name? What about: > * @CCU_CLK_TYPE_PLL: PLL clock with two multiplier fields We have PLL with 2 multipliers, but also others with other factors sets, so that will end up being confusing. If the MP, NK and so on stuff is confusing, maybe we should just add a comment on top of that structure to explain what those factors are and what it actually means? Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout. signature.asc Description: PGP signature
[linux-sunxi] Re: [PATCH v5 15/26] clk: sunxi: Add ccu clock tree support
On 31/12/2018 16:59, Jagan Teki wrote:
> Clock control unit comprises of parent clocks, gates, multiplexers,
> dividers, multipliers, pre/post dividers and flags etc.
>
> So, the U-Boot implementation of ccu has divided into gates and tree.
> gates are generic clock configuration of enable/disable bit management
> which can be handle via ccu_clock_gate.
So if I understand this correctly, you implement the gate functionality
separately from the complex clock code, even if they are the same clock
from the DT point of view? So if one wants to enable the MMC0 clock,
which is a mux/divider clock, one needs to specify an extra entry in the
gate array to describe the enable bit in this special clock register?
Sounds a bit surprising, but is probably a neat trick to keep things
simple. There should be a comment in the code to this regard then.
> Tree clocks are parent clock type, fixed clocks, mp, nk, nkm, nkmp,
> pre/post div, flags etc. which were managed via ccu_clock_tree.
For a start, can we use more descriptive names than those very specific
MP/NK names? DIV_MUX and PLL sound more descriptive to me. I understand
that Linux uses those terms, but it would be great if uninitiated people
have a chance to understand this as well.
> This patch add support for MP, NK, MISC, FIXED clock types as part of
> ccu clock tree with get_rate functionality this eventually used by
> uart driver. and rest of the infrastructure will try to add while CLK
> is being used on respective peripherals.
>
> Note that few of the tree type clock would require to enable gates on
> their specific clock, in that case we need to add the gate details via
> ccu_clock_gate, example: MP with gate so the gate offset, bit value
> should add as part of ccu_clock_gate.
>
> Signed-off-by: Jagan Teki
> ---
> arch/arm/include/asm/arch-sunxi/ccu.h | 192 +-
> drivers/clk/sunxi/clk_a64.c | 40 ++
> drivers/clk/sunxi/clk_sunxi.c | 182
> 3 files changed, 413 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/include/asm/arch-sunxi/ccu.h
> b/arch/arm/include/asm/arch-sunxi/ccu.h
> index 3fdc26978d..61b8c36b3b 100644
> --- a/arch/arm/include/asm/arch-sunxi/ccu.h
> +++ b/arch/arm/include/asm/arch-sunxi/ccu.h
> @@ -7,15 +7,204 @@
> #ifndef _ASM_ARCH_CCU_H
> #define _ASM_ARCH_CCU_H
>
> +#define OSC_32K_ULL 32000ULL
32768
And why ULL? The whole Allwinner clock system works with 32-bit values,
so just U would be totally sufficient. This avoid blowing this up to 64
bit unnecessarily, which sounds painful for those poor ARMv7 parts.
> +#define OSC_24M_ULL 2400ULL
> +
> +/**
> + * enum ccu_clk_type - ccu clock types
> + *
> + * @CCU_CLK_TYPE_MISC: misc clock type
What is MISC, exactly? Seems like an artefact clock to me, some
placeholder you need because gate clocks are handled separately in the
gates struct. Should this be called something with SIMPLE instead, or GATE?
> + * @CCU_CLK_TYPE_FIXED: fixed clock type
> + * @CCU_CLK_TYPE_MP: mp clock type
> + * @CCU_CLK_TYPE_NK: nk clock type
What is the point of those comments, as you are basically repeating the
enum name? What about:
* @CCU_CLK_TYPE_PLL: PLL clock with two multiplier fields
* @CCU_CLK_TYPE_MUX_DIV: multiple parents, two divider fields
This gives more speaking names, plus some explanation.
> + */
> +enum ccu_clk_type {
> + CCU_CLK_TYPE_MISC = 0,
> + CCU_CLK_TYPE_FIXED = 1,
> + CCU_CLK_TYPE_MP = 2,
> + CCU_CLK_TYPE_NK = 3,
> +};
> +
> /**
> * enum ccu_clk_flags - ccu clock flags
> *
> - * @CCU_CLK_F_INIT_DONE: clock gate init done check
> + * @CCU_CLK_F_INIT_DONE: clock tree/gate init done check
Is this flag to tell implemented clocks apart from unimplemented ones,
which would be reset to all zeroes by the compiler? Then it should be
called something with VALID in it.
> + * @CCU_CLK_F_POSTDIV: clock post divider
> */
> enum ccu_clk_flags {
> CCU_CLK_F_INIT_DONE = BIT(0),
> + CCU_CLK_F_POSTDIV = BIT(1),
> };
>
> +/**
> + * struct ccu_mult - ccu clock multiplier
> + *
> + * @shift: multiplier shift value
> + * @width: multiplier width value
> + * @offset: multiplier offset
> + * @min: minimum multiplier
> + * @max: maximum multiplier
> + */
> +struct ccu_mult {
> + u8 shift;
> + u8 width;
> + u8 offset;
> + u8 min;
> + u8 max;
> +};
> +
> +#define _CCU_MULT_OFF_MIN_MAX(_shift, _width, _offset, \
> + _min, _max) { \
> + .shift = _shift,\
> + .width = _width,\
> + .offset = _offset,
