[PATCH 0/2] ARM: sunxi: Enable syscon for the system controller

[PATCH 0/2] ARM: sunxi: Enable syscon for the system controller

[Carlo Caione]

The so called "system controller" in Allwinner A20 and A31 SoCs is
multi-purpose controller that tries to add misc functionality to one
memory region.
In these SoCs it controls the internal SRAM partitioning but it also
includes registers for chip versioning and NMI control.
This patch adds the proper nodes in the DTS files and enable the syscon
in the defconfig files.

Even though the system controller includes also register for managing the
NMI controller, these register are not mapped in the syscon since they
are directly used and mapped by the NMI controller itself.

Carlo Caione (2):
  ARM: sunxi: dts: Enable syscon in DTSI
  ARM: sunxi: Add syscon support in defconfig

 arch/arm/boot/dts/sun6i-a31.dtsi    | 5 +++++
 arch/arm/boot/dts/sun7i-a20.dtsi    | 5 +++++
 arch/arm/configs/multi_v7_defconfig | 1 +
 arch/arm/configs/sunxi_defconfig    | 1 +
 4 files changed, 12 insertions(+)

-- 
1.8.3.2

[PATCH 1/2] ARM: sunxi: dts: Enable syscon in DTSI

[Carlo Caione]

This patch adds the proper nodes in the DTSI files for Allwinner
A20 and A31.

Signed-off-by: Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>
---
 arch/arm/boot/dts/sun6i-a31.dtsi | 5 +++++
 arch/arm/boot/dts/sun7i-a20.dtsi | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
index bc46814..775e64a 100644
--- a/arch/arm/boot/dts/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/sun6i-a31.dtsi
@@ -237,6 +237,11 @@
 		#size-cells = <1>;
 		ranges;
 
+		syscon: syscon@01c00000 {
+			compatible = "syscon";
+			reg = <0x01c00000 0x27>;
+		};
+
 		nmi_intc: interrupt-controller@01f00c0c {
 			compatible = "allwinner,sun6i-a31-sc-nmi";
 			interrupt-controller;
diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
index 88171f4..14f8933 100644
--- a/arch/arm/boot/dts/sun7i-a20.dtsi
+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
@@ -401,6 +401,11 @@
 		#size-cells = <1>;
 		ranges;
 
+		syscon: syscon@01c00000 {
+			compatible = "syscon";
+			reg = <0x01c00000 0x27>;
+		};
+
 		nmi_intc: interrupt-controller@01c00030 {
 			compatible = "allwinner,sun7i-a20-sc-nmi";
 			interrupt-controller;
-- 
1.8.3.2

[PATCH 2/2] ARM: sunxi: Add syscon support in defconfig

[Carlo Caione]

Signed-off-by: Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org>
---
 arch/arm/configs/multi_v7_defconfig | 1 +
 arch/arm/configs/sunxi_defconfig    | 1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
index d4e8a47..d04fef5 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -343,3 +343,4 @@ CONFIG_DEBUG_FS=y
 CONFIG_MAGIC_SYSRQ=y
 CONFIG_LOCKUP_DETECTOR=y
 CONFIG_CRYPTO_DEV_TEGRA_AES=y
+CONFIG_MFD_SYSCON=y
diff --git a/arch/arm/configs/sunxi_defconfig b/arch/arm/configs/sunxi_defconfig
index b5df4a5..2ee8838 100644
--- a/arch/arm/configs/sunxi_defconfig
+++ b/arch/arm/configs/sunxi_defconfig
@@ -69,3 +69,4 @@ CONFIG_NFS_FS=y
 CONFIG_ROOT_NFS=y
 CONFIG_NLS=y
 CONFIG_PRINTK_TIME=y
+CONFIG_MFD_SYSCON=y
-- 
1.8.3.2

Re: [PATCH 0/2] ARM: sunxi: Enable syscon for the system controller

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1207 bytes --]

On Sun, May 04, 2014 at 04:02:38PM +0200, Carlo Caione wrote:
> The so called "system controller" in Allwinner A20 and A31 SoCs is
> multi-purpose controller that tries to add misc functionality to one
> memory region.
> In these SoCs it controls the internal SRAM partitioning but it also
> includes registers for chip versioning and NMI control.
> This patch adds the proper nodes in the DTS files and enable the syscon
> in the defconfig files.
> 
> Even though the system controller includes also register for managing the
> NMI controller, these register are not mapped in the syscon since they
> are directly used and mapped by the NMI controller itself.

Hmmm, what exactly do you want to achieve with this?

The NMI controller won't be able to use it, since it's initialized
much earlier than syscon and regmap.

Moreover, the A31 doesn't seem to have this system controller, or at
least this overlap.

And since on the A20, registers seem to have one usage only, so I
guess we can just split this IP into several nodes, just like we did
with the NMI.

Maxime

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [linux-sunxi] Re: [PATCH 0/2] ARM: sunxi: Enable syscon for the system controller

[Chen-Yu Tsai]

Hi,

On Tue, May 6, 2014 at 6:55 AM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> On Sun, May 04, 2014 at 04:02:38PM +0200, Carlo Caione wrote:
>> The so called "system controller" in Allwinner A20 and A31 SoCs is
>> multi-purpose controller that tries to add misc functionality to one
>> memory region.
>> In these SoCs it controls the internal SRAM partitioning but it also
>> includes registers for chip versioning and NMI control.
>> This patch adds the proper nodes in the DTS files and enable the syscon
>> in the defconfig files.
>>
>> Even though the system controller includes also register for managing the
>> NMI controller, these register are not mapped in the syscon since they
>> are directly used and mapped by the NMI controller itself.
>
> Hmmm, what exactly do you want to achieve with this?
>
> The NMI controller won't be able to use it, since it's initialized
> much earlier than syscon and regmap.

I believe this will be used for toggling the SRAM mappings. (Am I right?)
The second register toggles mappings for MUSB FIFO, EMAC, and a few of
the other IP blocks we currently don't support.

> Moreover, the A31 doesn't seem to have this system controller, or at
> least this overlap.

There should be something similar, as does the A23. There is no overlap AFAIK.

> And since on the A20, registers seem to have one usage only, so I
> guess we can just split this IP into several nodes, just like we did
> with the NMI.

As stated above, the second register toggles SRAM mappings for at most
4 SRAM blocks (for EMAC, MUSB, ACE, ISP).

syscon would be a good way to share this register among the various drivers.
We do not toggle it in the current EMAC driver. The driver seems to assume
it is setup by the bootloader, and on the A20, it seems to be mapped to
EMAC by default.

The MUSB glue layer driver must toggle this.

I think this approach is better than all the individual drivers mapping
the registers and toggling a single bit. In fact I did something similar
when working on preliminary musb support.


Cheers
ChenYu
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Re: [PATCH 0/2] ARM: sunxi: Enable syscon for the system controller

[Carlo Caione]

On Tue, May 6, 2014 at 8:36 AM, Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org> wrote:
> Hi,

Hi,

> On Tue, May 6, 2014 at 6:55 AM, Maxime Ripard
> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>> On Sun, May 04, 2014 at 04:02:38PM +0200, Carlo Caione wrote:
>>> The so called "system controller" in Allwinner A20 and A31 SoCs is
>>> multi-purpose controller that tries to add misc functionality to one
>>> memory region.
>>> In these SoCs it controls the internal SRAM partitioning but it also
>>> includes registers for chip versioning and NMI control.
>>> This patch adds the proper nodes in the DTS files and enable the syscon
>>> in the defconfig files.
>>>
>>> Even though the system controller includes also register for managing the
>>> NMI controller, these register are not mapped in the syscon since they
>>> are directly used and mapped by the NMI controller itself.
>>
>> Hmmm, what exactly do you want to achieve with this?
>>
>> The NMI controller won't be able to use it, since it's initialized
>> much earlier than syscon and regmap.

This is what I meant with that phrase. NMI controller doesn't use the
syscon but we can use it for several other drivers. In fact the
registers for NMI controller are excluded from the range of syscon
registers.

> I believe this will be used for toggling the SRAM mappings. (Am I right?)

Definitely right.

> The second register toggles mappings for MUSB FIFO, EMAC, and a few of
> the other IP blocks we currently don't support.

Not yet :)

>> Moreover, the A31 doesn't seem to have this system controller, or at
>> least this overlap.

I admit that I didn't check the A31 manual but I trusted the wiki page
at http://linux-sunxi.org/SRAM_Controller and
http://linux-sunxi.org/A31/Memory_map

> There should be something similar, as does the A23. There is no overlap AFAIK.

I agree and will check also A23.

>> And since on the A20, registers seem to have one usage only, so I
>> guess we can just split this IP into several nodes, just like we did
>> with the NMI.
>
> As stated above, the second register toggles SRAM mappings for at most
> 4 SRAM blocks (for EMAC, MUSB, ACE, ISP).
>
> syscon would be a good way to share this register among the various drivers.
> We do not toggle it in the current EMAC driver. The driver seems to assume
> it is setup by the bootloader, and on the A20, it seems to be mapped to
> EMAC by default.
>
> The MUSB glue layer driver must toggle this.

This is exactly why I wrote these patches. I started hacking /
studying your MUSB driver and I think that using syscon is a better
way to manage these registers instead of mapping them in several
drivers also because most of the time a single register has to be used
by multiple drivers (i.e. SRAM_CTL1_CFG is used for USB,  EMAC,
etc...)

> I think this approach is better than all the individual drivers mapping
> the registers and toggling a single bit. In fact I did something similar
> when working on preliminary musb support.

Thanks,

-- 
Carlo Caione

Re: Re: [PATCH 0/2] ARM: sunxi: Enable syscon for the system controller

[Chen-Yu Tsai]

On Tue, May 6, 2014 at 4:03 PM, Carlo Caione <carlo-KA+7E9HrN00dnm+yROfE0A@public.gmane.org> wrote:
> On Tue, May 6, 2014 at 8:36 AM, Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org> wrote:
>> Hi,
>
> Hi,
>
>> On Tue, May 6, 2014 at 6:55 AM, Maxime Ripard
>> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>>> On Sun, May 04, 2014 at 04:02:38PM +0200, Carlo Caione wrote:
>>>> The so called "system controller" in Allwinner A20 and A31 SoCs is
>>>> multi-purpose controller that tries to add misc functionality to one
>>>> memory region.
>>>> In these SoCs it controls the internal SRAM partitioning but it also
>>>> includes registers for chip versioning and NMI control.
>>>> This patch adds the proper nodes in the DTS files and enable the syscon
>>>> in the defconfig files.
>>>>
>>>> Even though the system controller includes also register for managing the
>>>> NMI controller, these register are not mapped in the syscon since they
>>>> are directly used and mapped by the NMI controller itself.
>>>
>>> Hmmm, what exactly do you want to achieve with this?
>>>
>>> The NMI controller won't be able to use it, since it's initialized
>>> much earlier than syscon and regmap.
>
> This is what I meant with that phrase. NMI controller doesn't use the
> syscon but we can use it for several other drivers. In fact the
> registers for NMI controller are excluded from the range of syscon
> registers.
>
>> I believe this will be used for toggling the SRAM mappings. (Am I right?)
>
> Definitely right.
>
>> The second register toggles mappings for MUSB FIFO, EMAC, and a few of
>> the other IP blocks we currently don't support.
>
> Not yet :)
>
>>> Moreover, the A31 doesn't seem to have this system controller, or at
>>> least this overlap.
>
> I admit that I didn't check the A31 manual but I trusted the wiki page
> at http://linux-sunxi.org/SRAM_Controller and
> http://linux-sunxi.org/A31/Memory_map

I think the wiki hasn't been updated with much A31/A23 specifics yet.

>> There should be something similar, as does the A23. There is no overlap AFAIK.
>
> I agree and will check also A23.

I will try to find time to update the wiki tonight.

>>> And since on the A20, registers seem to have one usage only, so I
>>> guess we can just split this IP into several nodes, just like we did
>>> with the NMI.
>>
>> As stated above, the second register toggles SRAM mappings for at most
>> 4 SRAM blocks (for EMAC, MUSB, ACE, ISP).
>>
>> syscon would be a good way to share this register among the various drivers.
>> We do not toggle it in the current EMAC driver. The driver seems to assume
>> it is setup by the bootloader, and on the A20, it seems to be mapped to
>> EMAC by default.
>>
>> The MUSB glue layer driver must toggle this.
>
> This is exactly why I wrote these patches. I started hacking /
> studying your MUSB driver and I think that using syscon is a better
> way to manage these registers instead of mapping them in several
> drivers also because most of the time a single register has to be used
> by multiple drivers (i.e. SRAM_CTL1_CFG is used for USB,  EMAC,
> etc...)

I came to the same conclusion, though I somehow forgot to update my tree
on github. Here it is:

  https://github.com/wens/linux/tree/wip/sunxi-musb

This also has the bit definitions:

  https://github.com/wens/linux/commit/3db9c184df5e5b3e1ddef59926e88fc2d03b4de9

Beware musb in my tree doesn't work. I was focused on getting the phy driver
and port controller correct.

>> I think this approach is better than all the individual drivers mapping
>> the registers and toggling a single bit. In fact I did something similar
>> when working on preliminary musb support.

Cheers

ChenYu

Re: Re: [PATCH 0/2] ARM: sunxi: Enable syscon for the system controller

[Carlo Caione]

On Tue, May 6, 2014 at 10:27 AM, Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org> wrote:

<snip>

> I came to the same conclusion, though I somehow forgot to update my tree
> on github. Here it is:
>
>   https://github.com/wens/linux/tree/wip/sunxi-musb
>
> This also has the bit definitions:
>
>   https://github.com/wens/linux/commit/3db9c184df5e5b3e1ddef59926e88fc2d03b4de9

Oh, I missed that wip branch. I'll add your SoB also and I'll add the
bit definitions in v2 (if Maxime ACKs it)

> Beware musb in my tree doesn't work. I was focused on getting the phy driver
> and port controller correct.

No prob, still scouting around :)

Thanks,

-- 
Carlo Caione

Re: Re: [PATCH 0/2] ARM: sunxi: Enable syscon for the system controller

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3696 bytes --]

On Tue, May 06, 2014 at 10:03:19AM +0200, Carlo Caione wrote:
> On Tue, May 6, 2014 at 8:36 AM, Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org> wrote:
> > Hi,
> 
> Hi,
> 
> > On Tue, May 6, 2014 at 6:55 AM, Maxime Ripard
> > <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> >> On Sun, May 04, 2014 at 04:02:38PM +0200, Carlo Caione wrote:
> >>> The so called "system controller" in Allwinner A20 and A31 SoCs is
> >>> multi-purpose controller that tries to add misc functionality to one
> >>> memory region.
> >>> In these SoCs it controls the internal SRAM partitioning but it also
> >>> includes registers for chip versioning and NMI control.
> >>> This patch adds the proper nodes in the DTS files and enable the syscon
> >>> in the defconfig files.
> >>>
> >>> Even though the system controller includes also register for managing the
> >>> NMI controller, these register are not mapped in the syscon since they
> >>> are directly used and mapped by the NMI controller itself.
> >>
> >> Hmmm, what exactly do you want to achieve with this?
> >>
> >> The NMI controller won't be able to use it, since it's initialized
> >> much earlier than syscon and regmap.
> 
> This is what I meant with that phrase. NMI controller doesn't use the
> syscon but we can use it for several other drivers.

I'm sorry, but I believe this should be more handled by the soon-to-be
drivers/soc "framework".

> In fact the registers for NMI controller are excluded from the range
> of syscon registers.

Then you are lying in the DT :)

> > I believe this will be used for toggling the SRAM mappings. (Am I right?)
> 
> Definitely right.
> 
> > The second register toggles mappings for MUSB FIFO, EMAC, and a few of
> > the other IP blocks we currently don't support.
> 
> Not yet :)

I wonder how other SoCs are actually handling this mapping between CPU
& DMA vs device of some SRAMs. Did you look at this?

> >> Moreover, the A31 doesn't seem to have this system controller, or at
> >> least this overlap.
> 
> I admit that I didn't check the A31 manual but I trusted the wiki page
> at http://linux-sunxi.org/SRAM_Controller and
> http://linux-sunxi.org/A31/Memory_map
> 
> > There should be something similar, as does the A23. There is no overlap AFAIK.
> 
> I agree and will check also A23.
> 
> >> And since on the A20, registers seem to have one usage only, so I
> >> guess we can just split this IP into several nodes, just like we did
> >> with the NMI.
> >
> > As stated above, the second register toggles SRAM mappings for at most
> > 4 SRAM blocks (for EMAC, MUSB, ACE, ISP).
> >
> > syscon would be a good way to share this register among the various drivers.
> > We do not toggle it in the current EMAC driver. The driver seems to assume
> > it is setup by the bootloader, and on the A20, it seems to be mapped to
> > EMAC by default.
> >
> > The MUSB glue layer driver must toggle this.
> 
> This is exactly why I wrote these patches. I started hacking /
> studying your MUSB driver and I think that using syscon is a better
> way to manage these registers instead of mapping them in several
> drivers also because most of the time a single register has to be used
> by multiple drivers (i.e. SRAM_CTL1_CFG is used for USB,  EMAC,
> etc...)
> 
> > I think this approach is better than all the individual drivers mapping
> > the registers and toggling a single bit. In fact I did something similar
> > when working on preliminary musb support.

I agree with that.

Maxime

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Re: [PATCH 0/2] ARM: sunxi: Enable syscon for the system controller

[Carlo Caione]

On Wed, May 7, 2014 at 5:25 AM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> On Tue, May 06, 2014 at 10:03:19AM +0200, Carlo Caione wrote:
>> On Tue, May 6, 2014 at 8:36 AM, Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org> wrote:
>> > Hi,
>>
>> Hi,
>>
>> > On Tue, May 6, 2014 at 6:55 AM, Maxime Ripard
>> > <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
>> >> On Sun, May 04, 2014 at 04:02:38PM +0200, Carlo Caione wrote:
>> >>> The so called "system controller" in Allwinner A20 and A31 SoCs is
>> >>> multi-purpose controller that tries to add misc functionality to one
>> >>> memory region.
>> >>> In these SoCs it controls the internal SRAM partitioning but it also
>> >>> includes registers for chip versioning and NMI control.
>> >>> This patch adds the proper nodes in the DTS files and enable the syscon
>> >>> in the defconfig files.
>> >>>
>> >>> Even though the system controller includes also register for managing the
>> >>> NMI controller, these register are not mapped in the syscon since they
>> >>> are directly used and mapped by the NMI controller itself.
>> >>
>> >> Hmmm, what exactly do you want to achieve with this?
>> >>
>> >> The NMI controller won't be able to use it, since it's initialized
>> >> much earlier than syscon and regmap.
>>
>> This is what I meant with that phrase. NMI controller doesn't use the
>> syscon but we can use it for several other drivers.
>
> I'm sorry, but I believe this should be more handled by the soon-to-be
> drivers/soc "framework".

Oh, didn't know of this framework. Is it supposed to replace syscon?

>> In fact the registers for NMI controller are excluded from the range
>> of syscon registers.
>
> Then you are lying in the DT :)

Uhm, in DT I have:

reg = <0x01c00000 0x27>;

that is I'm mapping the first three registers: SRAM_CTRL_REG0,
SRAM_CTRL_REG1, VER_REG, leaving out the three register (from offset
0x30) of the NMI controller.
I'm not lying :)

>> > I believe this will be used for toggling the SRAM mappings. (Am I right?)
>>
>> Definitely right.
>>
>> > The second register toggles mappings for MUSB FIFO, EMAC, and a few of
>> > the other IP blocks we currently don't support.
>>
>> Not yet :)
>
> I wonder how other SoCs are actually handling this mapping between CPU
> & DMA vs device of some SRAMs. Did you look at this?

It seems quite a few grepping for syscon

>> >> Moreover, the A31 doesn't seem to have this system controller, or at
>> >> least this overlap.
>>
>> I admit that I didn't check the A31 manual but I trusted the wiki page
>> at http://linux-sunxi.org/SRAM_Controller and
>> http://linux-sunxi.org/A31/Memory_map
>>
>> > There should be something similar, as does the A23. There is no overlap AFAIK.
>>
>> I agree and will check also A23.
>>
>> >> And since on the A20, registers seem to have one usage only, so I
>> >> guess we can just split this IP into several nodes, just like we did
>> >> with the NMI.
>> >
>> > As stated above, the second register toggles SRAM mappings for at most
>> > 4 SRAM blocks (for EMAC, MUSB, ACE, ISP).
>> >
>> > syscon would be a good way to share this register among the various drivers.
>> > We do not toggle it in the current EMAC driver. The driver seems to assume
>> > it is setup by the bootloader, and on the A20, it seems to be mapped to
>> > EMAC by default.
>> >
>> > The MUSB glue layer driver must toggle this.
>>
>> This is exactly why I wrote these patches. I started hacking /
>> studying your MUSB driver and I think that using syscon is a better
>> way to manage these registers instead of mapping them in several
>> drivers also because most of the time a single register has to be used
>> by multiple drivers (i.e. SRAM_CTL1_CFG is used for USB,  EMAC,
>> etc...)
>>
>> > I think this approach is better than all the individual drivers mapping
>> > the registers and toggling a single bit. In fact I did something similar
>> > when working on preliminary musb support.
>
> I agree with that.

So do you suggest to drop the syscon idea waiting for the new soc framework?

Thanks,

-- 
Carlo Caione

Re: Re: [PATCH 0/2] ARM: sunxi: Enable syscon for the system controller

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 5493 bytes --]

On Wed, May 07, 2014 at 10:19:19AM +0200, Carlo Caione wrote:
> On Wed, May 7, 2014 at 5:25 AM, Maxime Ripard
> <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> > On Tue, May 06, 2014 at 10:03:19AM +0200, Carlo Caione wrote:
> >> On Tue, May 6, 2014 at 8:36 AM, Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org> wrote:
> >> > Hi,
> >>
> >> Hi,
> >>
> >> > On Tue, May 6, 2014 at 6:55 AM, Maxime Ripard
> >> > <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> >> >> On Sun, May 04, 2014 at 04:02:38PM +0200, Carlo Caione wrote:
> >> >>> The so called "system controller" in Allwinner A20 and A31 SoCs is
> >> >>> multi-purpose controller that tries to add misc functionality to one
> >> >>> memory region.
> >> >>> In these SoCs it controls the internal SRAM partitioning but it also
> >> >>> includes registers for chip versioning and NMI control.
> >> >>> This patch adds the proper nodes in the DTS files and enable the syscon
> >> >>> in the defconfig files.
> >> >>>
> >> >>> Even though the system controller includes also register for managing the
> >> >>> NMI controller, these register are not mapped in the syscon since they
> >> >>> are directly used and mapped by the NMI controller itself.
> >> >>
> >> >> Hmmm, what exactly do you want to achieve with this?
> >> >>
> >> >> The NMI controller won't be able to use it, since it's initialized
> >> >> much earlier than syscon and regmap.
> >>
> >> This is what I meant with that phrase. NMI controller doesn't use the
> >> syscon but we can use it for several other drivers.
> >
> > I'm sorry, but I believe this should be more handled by the soon-to-be
> > drivers/soc "framework".
> 
> Oh, didn't know of this framework. Is it supposed to replace syscon?

It's supposed to be a framework to handle all the weird things the
SoCs have, that have to be enabled soon, and don't really fit into any
of the existing frameworks, or multiple of them. Which looks pretty
much like what we are experiencing here.

> >> In fact the registers for NMI controller are excluded from the range
> >> of syscon registers.
> >
> > Then you are lying in the DT :)
> 
> Uhm, in DT I have:
> 
> reg = <0x01c00000 0x27>;
> 
> that is I'm mapping the first three registers: SRAM_CTRL_REG0,
> SRAM_CTRL_REG1, VER_REG, leaving out the three register (from offset
> 0x30) of the NMI controller.
> I'm not lying :)

Then you want to set 0x30 as a base. And ok, you're splitting this in
two.


> >> > I believe this will be used for toggling the SRAM mappings. (Am I right?)
> >>
> >> Definitely right.
> >>
> >> > The second register toggles mappings for MUSB FIFO, EMAC, and a few of
> >> > the other IP blocks we currently don't support.
> >>
> >> Not yet :)
> >
> > I wonder how other SoCs are actually handling this mapping between CPU
> > & DMA vs device of some SRAMs. Did you look at this?
> 
> It seems quite a few grepping for syscon

??

What do you mean?

I wasn't really talking about syscon itself, just how other SoCs
usually deals with this kind of remapping usually.

> >> >> Moreover, the A31 doesn't seem to have this system controller, or at
> >> >> least this overlap.
> >>
> >> I admit that I didn't check the A31 manual but I trusted the wiki page
> >> at http://linux-sunxi.org/SRAM_Controller and
> >> http://linux-sunxi.org/A31/Memory_map
> >>
> >> > There should be something similar, as does the A23. There is no overlap AFAIK.
> >>
> >> I agree and will check also A23.
> >>
> >> >> And since on the A20, registers seem to have one usage only, so I
> >> >> guess we can just split this IP into several nodes, just like we did
> >> >> with the NMI.
> >> >
> >> > As stated above, the second register toggles SRAM mappings for at most
> >> > 4 SRAM blocks (for EMAC, MUSB, ACE, ISP).
> >> >
> >> > syscon would be a good way to share this register among the various drivers.
> >> > We do not toggle it in the current EMAC driver. The driver seems to assume
> >> > it is setup by the bootloader, and on the A20, it seems to be mapped to
> >> > EMAC by default.
> >> >
> >> > The MUSB glue layer driver must toggle this.
> >>
> >> This is exactly why I wrote these patches. I started hacking /
> >> studying your MUSB driver and I think that using syscon is a better
> >> way to manage these registers instead of mapping them in several
> >> drivers also because most of the time a single register has to be used
> >> by multiple drivers (i.e. SRAM_CTL1_CFG is used for USB,  EMAC,
> >> etc...)
> >>
> >> > I think this approach is better than all the individual drivers mapping
> >> > the registers and toggling a single bit. In fact I did something similar
> >> > when working on preliminary musb support.
> >
> > I agree with that.
> 
> So do you suggest to drop the syscon idea waiting for the new soc
> framework?

To be honest, I don't really know what to think of it. I'd need to see
some code that uses this. Maybe we can just postpone the decision to
whenever we will actually have some code submitted that make any use
of this?

If it's easier for you to keep the syscon at the moment for whateever
driver you're working on before submitting it, I'm fine with it, but
I'm not going to merge it right now either.

Thanks,
Maxime

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Re: [PATCH 0/2] ARM: sunxi: Enable syscon for the system controller

[Carlo Caione]

On Thu, May 8, 2014 at 5:04 AM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:

<snip>

>> >> > I believe this will be used for toggling the SRAM mappings. (Am I right?)
>> >>
>> >> Definitely right.
>> >>
>> >> > The second register toggles mappings for MUSB FIFO, EMAC, and a few of
>> >> > the other IP blocks we currently don't support.
>> >>
>> >> Not yet :)
>> >
>> > I wonder how other SoCs are actually handling this mapping between CPU
>> > & DMA vs device of some SRAMs. Did you look at this?
>>
>> It seems quite a few grepping for syscon
>
> ??
>
> What do you mean?
>
> I wasn't really talking about syscon itself, just how other SoCs
> usually deals with this kind of remapping usually.

Ho, sorry. In general for what I have seen the drivers use syscon or
just map directly the register they need to use.
Probably there is something smarter that I'm not aware of.

>> >> >> Moreover, the A31 doesn't seem to have this system controller, or at
>> >> >> least this overlap.
>> >>
>> >> I admit that I didn't check the A31 manual but I trusted the wiki page
>> >> at http://linux-sunxi.org/SRAM_Controller and
>> >> http://linux-sunxi.org/A31/Memory_map
>> >>
>> >> > There should be something similar, as does the A23. There is no overlap AFAIK.
>> >>
>> >> I agree and will check also A23.
>> >>
>> >> >> And since on the A20, registers seem to have one usage only, so I
>> >> >> guess we can just split this IP into several nodes, just like we did
>> >> >> with the NMI.
>> >> >
>> >> > As stated above, the second register toggles SRAM mappings for at most
>> >> > 4 SRAM blocks (for EMAC, MUSB, ACE, ISP).
>> >> >
>> >> > syscon would be a good way to share this register among the various drivers.
>> >> > We do not toggle it in the current EMAC driver. The driver seems to assume
>> >> > it is setup by the bootloader, and on the A20, it seems to be mapped to
>> >> > EMAC by default.
>> >> >
>> >> > The MUSB glue layer driver must toggle this.
>> >>
>> >> This is exactly why I wrote these patches. I started hacking /
>> >> studying your MUSB driver and I think that using syscon is a better
>> >> way to manage these registers instead of mapping them in several
>> >> drivers also because most of the time a single register has to be used
>> >> by multiple drivers (i.e. SRAM_CTL1_CFG is used for USB,  EMAC,
>> >> etc...)
>> >>
>> >> > I think this approach is better than all the individual drivers mapping
>> >> > the registers and toggling a single bit. In fact I did something similar
>> >> > when working on preliminary musb support.
>> >
>> > I agree with that.
>>
>> So do you suggest to drop the syscon idea waiting for the new soc
>> framework?
>
> To be honest, I don't really know what to think of it. I'd need to see
> some code that uses this. Maybe we can just postpone the decision to
> whenever we will actually have some code submitted that make any use
> of this?
>
> If it's easier for you to keep the syscon at the moment for whateever
> driver you're working on before submitting it, I'm fine with it, but
> I'm not going to merge it right now either.

It's fine with me.
Thank you for your review,

-- 
Carlo Caione