On Mon, Jul 10, 2017 at 01:23:51PM +0200, Olliver Schinagl wrote:
> Hi Pleas,
>
> On 04-07-17 22:05, Priit Laes wrote:
> > Convert sun7i-a20.dtsi to new CCU driver.
> >
> > Tested on Cubietruck.
> >
> > Signed-off-by: Priit Laes <[email protected]>
> > ---
> > arch/arm/boot/dts/sun7i-a20.dtsi | 719 +++-----------------------------
> > 1 file changed, 84 insertions(+), 635 deletions(-)
> >
> > diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
> > index 96bee77..a5ca5a8 100644
> > --- a/arch/arm/boot/dts/sun7i-a20.dtsi
> > +++ b/arch/arm/boot/dts/sun7i-a20.dtsi
> > @@ -46,8 +46,6 @@
> >
> > #include <dt-bindings/interrupt-controller/arm-gic.h>
> > #include <dt-bindings/thermal/thermal.h>
> > -
> > -#include <dt-bindings/clock/sun4i-a10-pll2.h>
> > #include <dt-bindings/dma/sun4i-a10.h>
> I had to add these includes here to make this all work:

That's unlikely, none of the defines are used in this patch.

Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Attachments:

(No filename) (1.04 kB)
signature.asc (801.00 B)
Download all attachments

2017-07-10 12:24:23

by Olliver Schinagl

[permalink] [raw]

Subject: Re: [linux-sunxi] [PATCH v5 5/6] ARM: sun7i: Convert to CCU

Hi Maxime,

On 10-07-17 13:55, Maxime Ripard wrote:
> On Mon, Jul 10, 2017 at 01:23:51PM +0200, Olliver Schinagl wrote:
>> Hi Pleas,
>>
>> On 04-07-17 22:05, Priit Laes wrote:
>>> Convert sun7i-a20.dtsi to new CCU driver.
>>>
>>> Tested on Cubietruck.
>>>
>>> Signed-off-by: Priit Laes <[email protected]>
>>> ---
>>> arch/arm/boot/dts/sun7i-a20.dtsi | 719 +++-----------------------------
>>> 1 file changed, 84 insertions(+), 635 deletions(-)
>>>
>>> diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
>>> index 96bee77..a5ca5a8 100644
>>> --- a/arch/arm/boot/dts/sun7i-a20.dtsi
>>> +++ b/arch/arm/boot/dts/sun7i-a20.dtsi
>>> @@ -46,8 +46,6 @@
>>>
>>> #include <dt-bindings/interrupt-controller/arm-gic.h>
>>> #include <dt-bindings/thermal/thermal.h>
>>> -
>>> -#include <dt-bindings/clock/sun4i-a10-pll2.h>
>>> #include <dt-bindings/dma/sun4i-a10.h>
>> I had to add these includes here to make this all work:
>
> That's unlikely, none of the defines are used in this patch.
>

My appologies you are right. I needed to add them to get this ontop of
your DRM stack working for the A20. I looked at my local diff and
responded a bit overeagerly probably. Nice catch!

Olliver

> Maxime
>

2017-07-13 19:12:24

by Priit Laes

[permalink] [raw]

Subject: Re: [PATCH v5 2/6] clk: sunxi-ng: Add sun4i/sun7i CCU driver

On Sun, Jul 09, 2017 at 10:25:23PM +1000, Jonathan Liu wrote:
> Hi Priit,
>
> On 5 July 2017 at 06:04, Priit Laes <[email protected]> wrote:
> > Introduce a clock controller driver for sun4i A10 and sun7i A20
> > series SoCs.
> >
> > Signed-off-by: Priit Laes <[email protected]>
> > ---
> > drivers/clk/sunxi-ng/Kconfig | 14 +-
> > drivers/clk/sunxi-ng/Makefile | 1 +-
> > drivers/clk/sunxi-ng/ccu-sun4i-a10.c | 1448 ++++++++++++++++++++++-
> > drivers/clk/sunxi-ng/ccu-sun4i-a10.h | 61 +-
> > include/dt-bindings/clock/sun4i-a10-ccu.h | 200 +++-
> > include/dt-bindings/clock/sun7i-a20-ccu.h | 53 +-
> > include/dt-bindings/reset/sun4i-a10-ccu.h | 67 +-
> > 7 files changed, 1844 insertions(+)
> > create mode 100644 drivers/clk/sunxi-ng/ccu-sun4i-a10.c
> > create mode 100644 drivers/clk/sunxi-ng/ccu-sun4i-a10.h
> > create mode 100644 include/dt-bindings/clock/sun4i-a10-ccu.h
> > create mode 100644 include/dt-bindings/clock/sun7i-a20-ccu.h
> > create mode 100644 include/dt-bindings/reset/sun4i-a10-ccu.h
> >
> [snip]
> > diff --git a/drivers/clk/sunxi-ng/ccu-sun4i-a10.c b/drivers/clk/sunxi-ng/ccu-sun4i-a10.c
> > new file mode 100644
> > index 0000000..49052b7
> > --- /dev/null
> > +++ b/drivers/clk/sunxi-ng/ccu-sun4i-a10.c
> [snip]
>
> > +static const char *const hdmi_parents[] = { "pll-video0", "pll-video0-2x",
> > + "pll-video1", "pll-video1-2x" };
> > +static SUNXI_CCU_M_WITH_MUX_GATE(hdmi_clk, "hdmi", hdmi_parents,
> > + 0x150, 0, 4, 24, 2, BIT(31), 0);
>
> hdmi_parents is in the wrong order. The correct order is "pll-video0",
> "pll-video1", "pll-video0-2x", "pll-video1-2x".

Ugh.. I'm really sorry.

P?ikest,
Priit

2017-07-13 19:24:02

by Priit Laes

[permalink] [raw]

Subject: Re: [linux-sunxi] [PATCH v5 2/6] clk: sunxi-ng: Add sun4i/sun7i CCU driver

On Mon, Jul 10, 2017 at 11:45:32AM +0200, Olliver Schinagl wrote:
> Hi Pleas,
>
> again, but this time with content :)
>
> On 04-07-17 22:04, Priit Laes wrote:
> >Introduce a clock controller driver for sun4i A10 and sun7i A20
> >series SoCs.

[ ... ]

> >+++ b/drivers/clk/sunxi-ng/Kconfig
> >@@ -11,6 +11,19 @@ config SUN50I_A64_CCU
> > default ARM64 && ARCH_SUNXI
> > depends on (ARM64 && ARCH_SUNXI) || COMPILE_TEST
> >
> >+config SUNXI_A10_CCU
> I understand why you say sunXi here (it's support for both sun4i and sun7i)
> but then why A10, as it also supports the A20.
>
> I guess the CCU is identical on the A20 and the A10, right? Thus would it
> not be sensible to just call it sun4i_ccu (like we do for sun5i_ccu below?

No, it's not identical.

> >+ bool "Support for the Allwinner A10/A20 CCU"
> >+ select SUNXI_CCU_DIV
> >+ select SUNXI_CCU_MULT
> >+ select SUNXI_CCU_NK
> >+ select SUNXI_CCU_NKM
> >+ select SUNXI_CCU_NM
> >+ select SUNXI_CCU_MP
> >+ select SUNXI_CCU_PHASE
> >+ default MACH_SUN4I
> >+ default MACH_SUN7I
> >+ depends on MACH_SUN4I || MACH_SUN7I || COMPILE_TEST
> >+
> > config SUN5I_CCU
> > bool "Support for the Allwinner sun5i family CCM"
> > default MACH_SUN5I
> >@@ -57,4 +70,5 @@ config SUN8I_R_CCU
> > bool "Support for Allwinner SoCs' PRCM CCUs"
> > default MACH_SUN8I || (ARCH_SUNXI && ARM64)
> >
> >+
> oops?

OK

>
> >new file mode 100644
> >index 0000000..49052b7
> >--- /dev/null
> >+++ b/drivers/clk/sunxi-ng/ccu-sun4i-a10.c
>
> <snip>
>
> >+static const char *const apb1_parents[] = { "hosc", "pll-periph", "osc32k" };
> >+static SUNXI_CCU_MP_WITH_MUX(apb1_clk, "apb1", apb1_parents, 0x058,
> >+ 0, 5, /* M */
> >+ 16, 2, /* P */
> >+ 24, 2, /* mux */
> >+ 0);
> >+
> >+/* Not present on A20 */
> >+static SUNXI_CCU_GATE(axi_dram_clk, "axi-dram", "ahb",
> >+ 0x05c, BIT(31), 0);
>
> Same here I guess, two defines make this a bit more readable.

You mean SUN4I_CCU_GATE? and SUN7I_CCU_GATE defines?
I don't think it makes things more readable...

> >+
> >+static SUNXI_CCU_GATE(ahb_otg_clk, "ahb-otg", "ahb",
...
> >+ 0x060, BIT(14), CLK_IS_CRITICAL);
>
> <snip>
>
> >+static struct ccu_reset_map sun7i_a20_ccu_resets[] = {
> >+ [RST_USB_PHY0] = { 0x0cc, BIT(0) },
> >+ [RST_USB_PHY1] = { 0x0cc, BIT(1) },
> >+ [RST_USB_PHY2] = { 0x0cc, BIT(2) },
> >+ [RST_GPS] = { 0x0d0, BIT(0) },
> >+ [RST_DE_BE0] = { 0x104, BIT(30) },
> >+ [RST_DE_BE1] = { 0x108, BIT(30) },
> >+ [RST_DE_FE0] = { 0x10c, BIT(30) },
> >+ [RST_DE_FE1] = { 0x110, BIT(30) },
> >+ [RST_DE_MP] = { 0x114, BIT(30) },
> >+ [RST_TCON0] = { 0x118, BIT(30) },
> >+ [RST_TCON1] = { 0x11c, BIT(30) },
> You are missing the TV encoder reset:
> + [RST_TVE0] = { 0x118, BIT(29) },
> + [RST_TVE1] = { 0x11c, BIT(29) },
>
> (to match your table i did not use defines :p)

Where did you get this information?
This is not present in any datasheets I have:
* A10 - 1.50
* A20 - 1.4

[...]

P?ikest,
Priit Laes

2017-07-13 19:47:09

by Olliver Schinagl

[permalink] [raw]

Subject: Re: [linux-sunxi] [PATCH v5 2/6] clk: sunxi-ng: Add sun4i/sun7i CCU driver

Hey Priit,

On 07/13/17 21:23, Priit Laes wrote:
> On Mon, Jul 10, 2017 at 11:45:32AM +0200, Olliver Schinagl wrote:
>> Hi Pleas,
>>
>> again, but this time with content :)
>>
>> On 04-07-17 22:04, Priit Laes wrote:
>>> Introduce a clock controller driver for sun4i A10 and sun7i A20
>>> series SoCs.
> [ ... ]
>
>>> +++ b/drivers/clk/sunxi-ng/Kconfig
>>> @@ -11,6 +11,19 @@ config SUN50I_A64_CCU
>>> default ARM64 && ARCH_SUNXI
>>> depends on (ARM64 && ARCH_SUNXI) || COMPILE_TEST
>>>
>>> +config SUNXI_A10_CCU
>> I understand why you say sunXi here (it's support for both sun4i and sun7i)
>> but then why A10, as it also supports the A20.
>>
>> I guess the CCU is identical on the A20 and the A10, right? Thus would it
>> not be sensible to just call it sun4i_ccu (like we do for sun5i_ccu below?
> No, it's not identical.
But then saying SUNXI_A10_CCU is not correct? Since it is not identical
on the A20? So what does the A10 stand for?
>
>>> + bool "Support for the Allwinner A10/A20 CCU"
>>> + select SUNXI_CCU_DIV
>>> + select SUNXI_CCU_MULT
>>> + select SUNXI_CCU_NK
>>> + select SUNXI_CCU_NKM
>>> + select SUNXI_CCU_NM
>>> + select SUNXI_CCU_MP
>>> + select SUNXI_CCU_PHASE
>>> + default MACH_SUN4I
>>> + default MACH_SUN7I
>>> + depends on MACH_SUN4I || MACH_SUN7I || COMPILE_TEST
>>> +
>>> config SUN5I_CCU
>>> bool "Support for the Allwinner sun5i family CCM"
>>> default MACH_SUN5I
>>> @@ -57,4 +70,5 @@ config SUN8I_R_CCU
>>> bool "Support for Allwinner SoCs' PRCM CCUs"
>>> default MACH_SUN8I || (ARCH_SUNXI && ARM64)
>>>
>>> +
>> oops?
> OK
>
>>> new file mode 100644
>>> index 0000000..49052b7
>>> --- /dev/null
>>> +++ b/drivers/clk/sunxi-ng/ccu-sun4i-a10.c
>> <snip>
>>
>>> +static const char *const apb1_parents[] = { "hosc", "pll-periph", "osc32k" };
>>> +static SUNXI_CCU_MP_WITH_MUX(apb1_clk, "apb1", apb1_parents, 0x058,
>>> + 0, 5, /* M */
>>> + 16, 2, /* P */
>>> + 24, 2, /* mux */
>>> + 0);
>>> +
>>> +/* Not present on A20 */
>>> +static SUNXI_CCU_GATE(axi_dram_clk, "axi-dram", "ahb",
>>> + 0x05c, BIT(31), 0);
>> Same here I guess, two defines make this a bit more readable.
> You mean SUN4I_CCU_GATE? and SUN7I_CCU_GATE defines?
> I don't think it makes things more readable...
you think 0x05c and BIT(31) are easier to read? I'll do a pop quiz in 6
months from now and see if you remember :p
>
>>> +
>>> +static SUNXI_CCU_GATE(ahb_otg_clk, "ahb-otg", "ahb",
> ...
>>> + 0x060, BIT(14), CLK_IS_CRITICAL);
>> <snip>
>>
>>> +static struct ccu_reset_map sun7i_a20_ccu_resets[] = {
>>> + [RST_USB_PHY0] = { 0x0cc, BIT(0) },
>>> + [RST_USB_PHY1] = { 0x0cc, BIT(1) },
>>> + [RST_USB_PHY2] = { 0x0cc, BIT(2) },
>>> + [RST_GPS] = { 0x0d0, BIT(0) },
>>> + [RST_DE_BE0] = { 0x104, BIT(30) },
>>> + [RST_DE_BE1] = { 0x108, BIT(30) },
>>> + [RST_DE_FE0] = { 0x10c, BIT(30) },
>>> + [RST_DE_FE1] = { 0x110, BIT(30) },
>>> + [RST_DE_MP] = { 0x114, BIT(30) },
>>> + [RST_TCON0] = { 0x118, BIT(30) },
>>> + [RST_TCON1] = { 0x11c, BIT(30) },
>> You are missing the TV encoder reset:
>> + [RST_TVE0] = { 0x118, BIT(29) },
>> + [RST_TVE1] = { 0x11c, BIT(29) },
>>
>> (to match your table i did not use defines :p)
> Where did you get this information?
> This is not present in any datasheets I have:
> * A10 - 1.50
> * A20 - 1.4
It is actually from the A13. In the A13 all the other bits match up. We
know from both that TCON0 is at 0x118 with its reset at BIT(30) and
TCON1 has its reset 0x11c. From the A13 datasheet we gather that TCON(0)
and TV(0) are at 0x118 with RST_TV on BIT(31) and thus it is only
logical that that for the TVE1 we have the rest at 0x11c.

But this is writing from the top of my head, I think we can also find it
in the 3.4 sources if I recall correctly.
>
> [...]
>
> P?ikest,
> Priit Laes

2017-07-14 13:48:58

by Priit Laes

[permalink] [raw]

Subject: Re: [linux-sunxi] [PATCH v5 2/6] clk: sunxi-ng: Add sun4i/sun7i CCU driver

On Thu, Jul 13, 2017 at 09:46:57PM +0200, Olliver Schinagl wrote:
> Hey Priit,
>
> On 07/13/17 21:23, Priit Laes wrote:
> > On Mon, Jul 10, 2017 at 11:45:32AM +0200, Olliver Schinagl wrote:
> >> Hi Pleas,
> >>
> >> again, but this time with content :)
> >>
> >> On 04-07-17 22:04, Priit Laes wrote:
> >>> Introduce a clock controller driver for sun4i A10 and sun7i A20
> >>> series SoCs.
> > [ ... ]
> >
> >>> +++ b/drivers/clk/sunxi-ng/Kconfig
> >>> @@ -11,6 +11,19 @@ config SUN50I_A64_CCU
> >>> default ARM64 && ARCH_SUNXI
> >>> depends on (ARM64 && ARCH_SUNXI) || COMPILE_TEST
> >>>
> >>> +config SUNXI_A10_CCU
> >> I understand why you say sunXi here (it's support for both sun4i and sun7i)
> >> but then why A10, as it also supports the A20.
> >>
> >> I guess the CCU is identical on the A20 and the A10, right? Thus would it
> >> not be sensible to just call it sun4i_ccu (like we do for sun5i_ccu below?
> > No, it's not identical.
> But then saying SUNXI_A10_CCU is not correct? Since it is not identical
> on the A20? So what does the A10 stand for?

There's no easy way it supports both SUN4I_A10 and SUN7I_A20, therefore
I used SUNXI_A10 where SUNXI may indicate it's not only for SUN4I and
I'm currently keeping it as is...

[ ... ]
> >>> +/* Not present on A20 */
> >>> +static SUNXI_CCU_GATE(axi_dram_clk, "axi-dram", "ahb",
> >>> + 0x05c, BIT(31), 0);
> >> Same here I guess, two defines make this a bit more readable.
> > You mean SUN4I_CCU_GATE? and SUN7I_CCU_GATE defines?
> > I don't think it makes things more readable...
> you think 0x05c and BIT(31) are easier to read? I'll do a pop quiz in 6
> months from now and see if you remember :p

Can you give an example on how it should be written?

> >
> >>> +
> >>> +static SUNXI_CCU_GATE(ahb_otg_clk, "ahb-otg", "ahb",
> > ...
> >>> + 0x060, BIT(14), CLK_IS_CRITICAL);
> >> <snip>
> >>
> >>> +static struct ccu_reset_map sun7i_a20_ccu_resets[] = {
> >>> + [RST_USB_PHY0] = { 0x0cc, BIT(0) },
> >>> + [RST_USB_PHY1] = { 0x0cc, BIT(1) },
> >>> + [RST_USB_PHY2] = { 0x0cc, BIT(2) },
> >>> + [RST_GPS] = { 0x0d0, BIT(0) },
> >>> + [RST_DE_BE0] = { 0x104, BIT(30) },
> >>> + [RST_DE_BE1] = { 0x108, BIT(30) },
> >>> + [RST_DE_FE0] = { 0x10c, BIT(30) },
> >>> + [RST_DE_FE1] = { 0x110, BIT(30) },
> >>> + [RST_DE_MP] = { 0x114, BIT(30) },
> >>> + [RST_TCON0] = { 0x118, BIT(30) },
> >>> + [RST_TCON1] = { 0x11c, BIT(30) },
> >> You are missing the TV encoder reset:
> >> + [RST_TVE0] = { 0x118, BIT(29) },
> >> + [RST_TVE1] = { 0x11c, BIT(29) },
> >>
> >> (to match your table i did not use defines :p)
> > Where did you get this information?
> > This is not present in any datasheets I have:
> > * A10 - 1.50
> > * A20 - 1.4
> It is actually from the A13. In the A13 all the other bits match up. We
> know from both that TCON0 is at 0x118 with its reset at BIT(30) and
> TCON1 has its reset 0x11c. From the A13 datasheet we gather that TCON(0)
> and TV(0) are at 0x118 with RST_TV on BIT(31) and thus it is only
> logical that that for the TVE1 we have the rest at 0x11c.
>
> But this is writing from the top of my head, I think we can also find it
> in the 3.4 sources if I recall correctly.

Thanks, added the reset bits for TVE0/1.

P?ikest,
Priit :)