2019-01-17 06:49:50

by Jerome Brunet

[permalink] [raw]
Subject: [PATCH] arm64: dts: meson: fix g12a buses

periph and hiu bus addresses/size are wrong.
cbus, aobus and apb just don't exist in the memory map so remove them.

Fixes: 9c8c52f7cb4f ("arm64: dts: meson-g12a: add initial g12a s905d2 SoC DT support")
Signed-off-by: Jerome Brunet <[email protected]>
---
arch/arm64/boot/dts/amlogic/meson-g12a.dtsi | 70 +++++++--------------
1 file changed, 24 insertions(+), 46 deletions(-)

diff --git a/arch/arm64/boot/dts/amlogic/meson-g12a.dtsi b/arch/arm64/boot/dts/amlogic/meson-g12a.dtsi
index 3b82a975c663..2aea5ba62fee 100644
--- a/arch/arm64/boot/dts/amlogic/meson-g12a.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-g12a.dtsi
@@ -78,46 +78,40 @@
#size-cells = <2>;
ranges;

- periphs: periphs@ff634000 {
+ periphs: bus@ff634400 {
compatible = "simple-bus";
- reg = <0x0 0xff634000 0x0 0x2000>;
+ reg = <0x0 0xff634400 0x0 0x400>;
#address-cells = <2>;
#size-cells = <2>;
- ranges = <0x0 0x0 0x0 0xff634000 0x0 0x2000>;
+ ranges = <0x0 0x0 0x0 0xff634400 0x0 0x400>;
};

- hiubus: bus@ff63c000 {
+ hiu: bus@ff63c000 {
compatible = "simple-bus";
- reg = <0x0 0xff63c000 0x0 0x1c00>;
+ reg = <0x0 0xff63c000 0x0 0x1400>;
#address-cells = <2>;
#size-cells = <2>;
- ranges = <0x0 0x0 0x0 0xff63c000 0x0 0x1c00>;
+ ranges = <0x0 0x0 0x0 0xff63c000 0x0 0x1400>;
};

- aobus: bus@ff800000 {
- compatible = "simple-bus";
- reg = <0x0 0xff800000 0x0 0x100000>;
- #address-cells = <2>;
- #size-cells = <2>;
- ranges = <0x0 0x0 0x0 0xff800000 0x0 0x100000>;
-
- uart_AO: serial@3000 {
- compatible = "amlogic,meson-gx-uart", "amlogic,meson-ao-uart";
- reg = <0x0 0x3000 0x0 0x18>;
- interrupts = <GIC_SPI 193 IRQ_TYPE_EDGE_RISING>;
- clocks = <&xtal>, <&xtal>, <&xtal>;
- clock-names = "xtal", "pclk", "baud";
- status = "disabled";
- };
-
- uart_AO_B: serial@4000 {
- compatible = "amlogic,meson-gx-uart", "amlogic,meson-ao-uart";
- reg = <0x0 0x4000 0x0 0x18>;
- interrupts = <GIC_SPI 197 IRQ_TYPE_EDGE_RISING>;
- clocks = <&xtal>, <&xtal>, <&xtal>;
- clock-names = "xtal", "pclk", "baud";
- status = "disabled";
- };
+ uart_AO: serial@ff803000 {
+ compatible = "amlogic,meson-gx-uart",
+ "amlogic,meson-ao-uart";
+ reg = <0x0 0xff803000 0x0 0x18>;
+ interrupts = <GIC_SPI 193 IRQ_TYPE_EDGE_RISING>;
+ clocks = <&xtal>, <&xtal>, <&xtal>;
+ clock-names = "xtal", "pclk", "baud";
+ status = "disabled";
+ };
+
+ uart_AO_B: serial@ff804000 {
+ compatible = "amlogic,meson-gx-uart",
+ "amlogic,meson-ao-uart";
+ reg = <0x0 0xff804000 0x0 0x18>;
+ interrupts = <GIC_SPI 197 IRQ_TYPE_EDGE_RISING>;
+ clocks = <&xtal>, <&xtal>, <&xtal>;
+ clock-names = "xtal", "pclk", "baud";
+ status = "disabled";
};

gic: interrupt-controller@ffc01000 {
@@ -132,22 +126,6 @@
#interrupt-cells = <3>;
#address-cells = <0>;
};
-
- cbus: bus@ffd00000 {
- compatible = "simple-bus";
- reg = <0x0 0xffd00000 0x0 0x25000>;
- #address-cells = <2>;
- #size-cells = <2>;
- ranges = <0x0 0x0 0x0 0xffd00000 0x0 0x25000>;
- };
-
- apb: apb@ffe00000 {
- compatible = "simple-bus";
- reg = <0x0 0xffe00000 0x0 0x200000>;
- #address-cells = <2>;
- #size-cells = <2>;
- ranges = <0x0 0x0 0x0 0xffe00000 0x0 0x200000>;
- };
};

timer {
--
2.20.1



2019-01-17 19:57:18

by Kevin Hilman

[permalink] [raw]
Subject: Re: [PATCH] arm64: dts: meson: fix g12a buses

Jerome Brunet <[email protected]> writes:

> periph and hiu bus addresses/size are wrong.
> cbus, aobus and apb just don't exist in the memory map so remove them.
>
> Fixes: 9c8c52f7cb4f ("arm64: dts: meson-g12a: add initial g12a s905d2 SoC DT support")
> Signed-off-by: Jerome Brunet <[email protected]>

Queuing as a fix for v5.0-rc,

Thanks,

Kevin

2019-01-17 20:04:28

by Kevin Hilman

[permalink] [raw]
Subject: Re: [PATCH] arm64: dts: meson: fix g12a buses

Jerome Brunet <[email protected]> writes:

> periph and hiu bus addresses/size are wrong.
> cbus, aobus and apb just don't exist in the memory map so remove them.
>
> Fixes: 9c8c52f7cb4f ("arm64: dts: meson-g12a: add initial g12a s905d2 SoC DT support")
> Signed-off-by: Jerome Brunet <[email protected]>

Queued as a fix for v5.0-rc (branch: v5.0/fixes)

Kevin

2019-01-17 20:06:07

by Martin Blumenstingl

[permalink] [raw]
Subject: Re: [PATCH] arm64: dts: meson: fix g12a buses

Hi Jerome,

On Wed, Jan 16, 2019 at 5:52 PM Jerome Brunet <[email protected]> wrote:
[...]
> - aobus: bus@ff800000 {
> - compatible = "simple-bus";
> - reg = <0x0 0xff800000 0x0 0x100000>;
are you sure about removing aobus?
in your patch "arm64: dts: meson: g12a: add pinctrl support
controllers" from [0] you're adding back an "rti" bus at the same
memory address, except that it's size is 0x1000 instead of 0x100000

I don't have any G12A datasheet so it's hard for me to ACK / NACK this
patch, here are some clues why I'm asking:
- GXM's public datasheet lists the "RTI" region at 0xC8100000 -
0xC81FFFFF, we call it "aobus" in our .dtsi
- buildroot_openlinux_kernel_4.9_fbdev_20180706/kernel/aml-4.9/arch/arm64/boot/dts/amlogic/mesong12a.dtsi
also uses an aobus node as well as an io_aobus_base sub-node for the
codec_io node (the latter is obviously not upstream)


Regards
Martin


[0] https://lore.kernel.org/patchwork/patch/1032981/

2019-01-17 20:30:34

by Martin Blumenstingl

[permalink] [raw]
Subject: Re: [PATCH] arm64: dts: meson: fix g12a buses

Hi Jerome,

On Thu, Jan 17, 2019 at 9:12 PM Jerome Brunet <[email protected]> wrote:
>
>
> On Thu, 2019-01-17 at 21:03 +0100, Martin Blumenstingl wrote:
> > Hi Jerome,
> >
> > On Wed, Jan 16, 2019 at 5:52 PM Jerome Brunet <[email protected]> wrote:
> > [...]
> > > - aobus: bus@ff800000 {
> > > - compatible = "simple-bus";
> > > - reg = <0x0 0xff800000 0x0 0x100000>;
> > are you sure about removing aobus?
> > in your patch "arm64: dts: meson: g12a: add pinctrl support
> > controllers" from [0] you're adding back an "rti" bus at the same
> > memory address, except that it's size is 0x1000 instead of 0x100000
>
> Yes, I'm sure. The fact that the aobus region size is completly made up, for
> all I know, is the reason
>
> >
> > I don't have any G12A datasheet so it's hard for me to ACK / NACK this
> > patch, here are some clues why I'm asking:
> > - GXM's public datasheet lists the "RTI" region at 0xC8100000 -
> > 0xC81FFFFF, we call it "aobus" in our .dtsi
>
> ... RTI region which will appear once i submit the clock controller
I assume you're speaking of G12A and not about GXM:
you already need it for your G12A pinctrl patches, which is why you
add it in: [0]

> Yes GXM (and GXBB, GXL or AXG) has the same made up region with nothing
> matching in the datasheet, but it is bit late fix that now.
looking at the datasheet: it seems that there's only a name mismatch
(aobus in our .dtsi, "rti" in the datasheet)
offset and size correspond with the datasheet

> > - buildroot_openlinux_kernel_4.9_fbdev_20180706/kernel/aml-
> > 4.9/arch/arm64/boot/dts/amlogic/mesong12a.dtsi
> > also uses an aobus node as well as an io_aobus_base sub-node for the
> > codec_io node (the latter is obviously not upstream)
>
> I know, but again, it does not map to anything in the doc, and it has been the
> case for several SoC.
OK, but we had incorrect documentation in the past. did you check this
with someone from Amlogic?

I'm curious because there seem to be two different approaches here:
1) hiubus name and offsets are being fixed within this patch
2) aobus is being dropped here and re-introduced with a different name later on

approach 1) can also be used for the "rti" region (at least in my
opinion, the patch doesn't explain why it can't be done):
rename "aobus" to "rti" and change the size to either 0x1000 or 0xb000
(both values can be found in mesong12a.dtsi from
buildroot_openlinux_kernel_4.9_fbdev_20180706)


Regards
Martin


[0] https://lore.kernel.org/patchwork/patch/1032981/

2019-01-17 21:22:26

by Martin Blumenstingl

[permalink] [raw]
Subject: Re: [PATCH] arm64: dts: meson: fix g12a buses

On Thu, Jan 17, 2019 at 9:39 PM Jerome Brunet <[email protected]> wrote:
>
> On Thu, 2019-01-17 at 21:27 +0100, Martin Blumenstingl wrote:
> > OK, but we had incorrect documentation in the past. did you check this
> > with someone from Amlogic?
> >
> > I'm curious because there seem to be two different approaches here:
> > 1) hiubus name and offsets are being fixed within this patch
> > 2) aobus is being dropped here and re-introduced with a different name later
> > on
> >
>
> because hiu exist and aobus does not, for which both the name and size was
> wrong
>
> > approach 1) can also be used for the "rti" region (at least in my
> > opinion, the patch doesn't explain why it can't be done):
>
> THe patch remove aobus (instead of fixing name and size) because, of the
> multiple region documented covered by this 'made region', I did not anticipate
> which one will be required and I did not want to add them all.
>
> Better to add them as needed, which is want I done for pinctrl as you pointed
> out
>
> > rename "aobus" to "rti" and change the size to either 0x1000 or 0xb000
> > (both values can be found in mesong12a.dtsi from
> > buildroot_openlinux_kernel_4.9_fbdev_20180706)
>
> RTI is added here:
> https://lkml.kernel.org/r/[email protected]
>
> I don't really understand the problem ? result is the same
the actual problem is "me" as I have conflicting information:
- Amlogic's buildroot kernel (for G12A) uses similar bus definitions
as the GX SoCs (for which there are public datasheets) - this is how
Jianxin added it to meson-g12a.dtsi originally
- this patch does it different - but cannot check if this is correct
(no public datasheet is available for G12A or AXG) nor do I have a
"big picture" of upcoming changes

Cc'ing Jianxin: can you please review Jerome's patch and give some
more details on the memory map on G12A so further contributions can be
reviewed easier?


Regards
Martin

2019-01-17 21:34:41

by Jerome Brunet

[permalink] [raw]
Subject: Re: [PATCH] arm64: dts: meson: fix g12a buses


On Thu, 2019-01-17 at 21:03 +0100, Martin Blumenstingl wrote:
> Hi Jerome,
>
> On Wed, Jan 16, 2019 at 5:52 PM Jerome Brunet <[email protected]> wrote:
> [...]
> > - aobus: bus@ff800000 {
> > - compatible = "simple-bus";
> > - reg = <0x0 0xff800000 0x0 0x100000>;
> are you sure about removing aobus?
> in your patch "arm64: dts: meson: g12a: add pinctrl support
> controllers" from [0] you're adding back an "rti" bus at the same
> memory address, except that it's size is 0x1000 instead of 0x100000

Yes, I'm sure. The fact that the aobus region size is completly made up, for
all I know, is the reason

>
> I don't have any G12A datasheet so it's hard for me to ACK / NACK this
> patch, here are some clues why I'm asking:
> - GXM's public datasheet lists the "RTI" region at 0xC8100000 -
> 0xC81FFFFF, we call it "aobus" in our .dtsi

... RTI region which will appear once i submit the clock controller

Yes GXM (and GXBB, GXL or AXG) has the same made up region with nothing
matching in the datasheet, but it is bit late fix that now.

> - buildroot_openlinux_kernel_4.9_fbdev_20180706/kernel/aml-
> 4.9/arch/arm64/boot/dts/amlogic/mesong12a.dtsi
> also uses an aobus node as well as an io_aobus_base sub-node for the
> codec_io node (the latter is obviously not upstream)

I know, but again, it does not map to anything in the doc, and it has been the
case for several SoC.

>
> Regards
> Martin
>
>
> [0] https://lore.kernel.org/patchwork/patch/1032981/



2019-01-17 21:35:52

by Jerome Brunet

[permalink] [raw]
Subject: Re: [PATCH] arm64: dts: meson: fix g12a buses

On Thu, 2019-01-17 at 21:27 +0100, Martin Blumenstingl wrote:
> OK, but we had incorrect documentation in the past. did you check this
> with someone from Amlogic?
>
> I'm curious because there seem to be two different approaches here:
> 1) hiubus name and offsets are being fixed within this patch
> 2) aobus is being dropped here and re-introduced with a different name later
> on
>

because hiu exist and aobus does not, for which both the name and size was
wrong

> approach 1) can also be used for the "rti" region (at least in my
> opinion, the patch doesn't explain why it can't be done):

THe patch remove aobus (instead of fixing name and size) because, of the
multiple region documented covered by this 'made region', I did not anticipate
which one will be required and I did not want to add them all.

Better to add them as needed, which is want I done for pinctrl as you pointed
out

> rename "aobus" to "rti" and change the size to either 0x1000 or 0xb000
> (both values can be found in mesong12a.dtsi from
> buildroot_openlinux_kernel_4.9_fbdev_20180706)

RTI is added here:
https://lkml.kernel.org/r/[email protected]

I don't really understand the problem ? result is the same


2019-01-17 21:50:31

by Jerome Brunet

[permalink] [raw]
Subject: Re: [PATCH] arm64: dts: meson: fix g12a buses

On Thu, 2019-01-17 at 22:20 +0100, Martin Blumenstingl wrote:
> On Thu, Jan 17, 2019 at 9:39 PM Jerome Brunet <[email protected]> wrote:
> > On Thu, 2019-01-17 at 21:27 +0100, Martin Blumenstingl wrote:
> > > OK, but we had incorrect documentation in the past. did you check this
> > > with someone from Amlogic?
> > >
> > > I'm curious because there seem to be two different approaches here:
> > > 1) hiubus name and offsets are being fixed within this patch
> > > 2) aobus is being dropped here and re-introduced with a different name
> > > later
> > > on
> > >
> >
> > because hiu exist and aobus does not, for which both the name and size was
> > wrong
> >
> > > approach 1) can also be used for the "rti" region (at least in my
> > > opinion, the patch doesn't explain why it can't be done):
> >
> > THe patch remove aobus (instead of fixing name and size) because, of the
> > multiple region documented covered by this 'made region', I did not
> > anticipate
> > which one will be required and I did not want to add them all.
> >
> > Better to add them as needed, which is want I done for pinctrl as you
> > pointed
> > out
> >
> > > rename "aobus" to "rti" and change the size to either 0x1000 or 0xb000
> > > (both values can be found in mesong12a.dtsi from
> > > buildroot_openlinux_kernel_4.9_fbdev_20180706)
> >
> > RTI is added here:
> > https://lkml.kernel.org/r/[email protected]
> >
> > I don't really understand the problem ? result is the same
> the actual problem is "me" as I have conflicting information:
> - Amlogic's buildroot kernel (for G12A) uses similar bus definitions
> as the GX SoCs (for which there are public datasheets) - this is how
> Jianxin added it to meson-g12a.dtsi originally

And it was the same for the GX family. AOBUS in the DT while there nothing
about this in the memory map. Keep wrong patterns does not make them right.

I'm merely reading the memory map here

> - this patch does it different - but cannot check if this is correct
> (no public datasheet is available for G12A or AXG) nor do I have a
> "big picture" of upcoming changes

Yes it does it differently. We should have picked up on this a while ago,
since gxbb at least, and we did not. There reason to create bus that don't
exist in the memory map of any recent SoC.

Creating bus should at least require a start and end offset explained
somewhere. Copying vendor DT is not enough

>
> Cc'ing Jianxin: can you please review Jerome's patch and give some
> more details on the memory map on G12A so further contributions can be
> reviewed easier?
>
>
> Regards
> Martin



2019-01-17 22:44:30

by Martin Blumenstingl

[permalink] [raw]
Subject: Re: [PATCH] arm64: dts: meson: fix g12a buses

On Thu, Jan 17, 2019 at 10:45 PM Jerome Brunet <[email protected]> wrote:
>
> On Thu, 2019-01-17 at 22:20 +0100, Martin Blumenstingl wrote:
> > On Thu, Jan 17, 2019 at 9:39 PM Jerome Brunet <[email protected]> wrote:
> > > On Thu, 2019-01-17 at 21:27 +0100, Martin Blumenstingl wrote:
> > > > OK, but we had incorrect documentation in the past. did you check this
> > > > with someone from Amlogic?
> > > >
> > > > I'm curious because there seem to be two different approaches here:
> > > > 1) hiubus name and offsets are being fixed within this patch
> > > > 2) aobus is being dropped here and re-introduced with a different name
> > > > later
> > > > on
> > > >
> > >
> > > because hiu exist and aobus does not, for which both the name and size was
> > > wrong
> > >
> > > > approach 1) can also be used for the "rti" region (at least in my
> > > > opinion, the patch doesn't explain why it can't be done):
> > >
> > > THe patch remove aobus (instead of fixing name and size) because, of the
> > > multiple region documented covered by this 'made region', I did not
> > > anticipate
> > > which one will be required and I did not want to add them all.
> > >
> > > Better to add them as needed, which is want I done for pinctrl as you
> > > pointed
> > > out
> > >
> > > > rename "aobus" to "rti" and change the size to either 0x1000 or 0xb000
> > > > (both values can be found in mesong12a.dtsi from
> > > > buildroot_openlinux_kernel_4.9_fbdev_20180706)
> > >
> > > RTI is added here:
> > > https://lkml.kernel.org/r/[email protected]
> > >
> > > I don't really understand the problem ? result is the same
> > the actual problem is "me" as I have conflicting information:
> > - Amlogic's buildroot kernel (for G12A) uses similar bus definitions
> > as the GX SoCs (for which there are public datasheets) - this is how
> > Jianxin added it to meson-g12a.dtsi originally
>
> And it was the same for the GX family. AOBUS in the DT while there nothing
> about this in the memory map. Keep wrong patterns does not make them right.
>
> I'm merely reading the memory map here
can you explain which "memory map" you are reading exactly?

my source is the "memory map" section in the public S905X and S912 datasheets:
- "S912_Datasheet_V0.220170314publicversion-Wesion.pdf" page 48
- "S905X_Datasheet V0.3 20170314publicversion-Wesion.pdf" page 43

> > - this patch does it different - but cannot check if this is correct
> > (no public datasheet is available for G12A or AXG) nor do I have a
> > "big picture" of upcoming changes
>
> Yes it does it differently. We should have picked up on this a while ago,
> since gxbb at least, and we did not. There reason to create bus that don't
> exist in the memory map of any recent SoC.
in the GXL and GXM datasheets from above there is:
range: 0xC8100000 - 0xC81FFFFF
name: RTI

thus in my opinion, based on the two datasheets from above:
- the bus definition (start, size) in meson-gx.dtsi is fine
- probably have the wrong name for this bus in meson-gx.dtsi
- (assuming that GXBB, GXL and GXM are all the same - I only compared
the last two)

> Creating bus should at least require a start and end offset explained
> somewhere. Copying vendor DT is not enough
I fully agree on this one!


Regards
Martin

2019-01-18 00:14:09

by Jerome Brunet

[permalink] [raw]
Subject: Re: [PATCH] arm64: dts: meson: fix g12a buses

On Thu, 2019-01-17 at 23:42 +0100, Martin Blumenstingl wrote:
> On Thu, Jan 17, 2019 at 10:45 PM Jerome Brunet <[email protected]> wrote:
> > On Thu, 2019-01-17 at 22:20 +0100, Martin Blumenstingl wrote:
> > > On Thu, Jan 17, 2019 at 9:39 PM Jerome Brunet <[email protected]>
> > > wrote:
> > > > On Thu, 2019-01-17 at 21:27 +0100, Martin Blumenstingl wrote:
> > > > > OK, but we had incorrect documentation in the past. did you check
> > > > > this
> > > > > with someone from Amlogic?
> > > > >
> > > > > I'm curious because there seem to be two different approaches here:
> > > > > 1) hiubus name and offsets are being fixed within this patch
> > > > > 2) aobus is being dropped here and re-introduced with a different
> > > > > name
> > > > > later
> > > > > on
> > > > >
> > > >
> > > > because hiu exist and aobus does not, for which both the name and size
> > > > was
> > > > wrong
> > > >
> > > > > approach 1) can also be used for the "rti" region (at least in my
> > > > > opinion, the patch doesn't explain why it can't be done):
> > > >
> > > > THe patch remove aobus (instead of fixing name and size) because, of
> > > > the
> > > > multiple region documented covered by this 'made region', I did not
> > > > anticipate
> > > > which one will be required and I did not want to add them all.
> > > >
> > > > Better to add them as needed, which is want I done for pinctrl as you
> > > > pointed
> > > > out
> > > >
> > > > > rename "aobus" to "rti" and change the size to either 0x1000 or
> > > > > 0xb000
> > > > > (both values can be found in mesong12a.dtsi from
> > > > > buildroot_openlinux_kernel_4.9_fbdev_20180706)
> > > >
> > > > RTI is added here:
> > > > https://lkml.kernel.org/r/[email protected]
> > > >
> > > > I don't really understand the problem ? result is the same
> > > the actual problem is "me" as I have conflicting information:
> > > - Amlogic's buildroot kernel (for G12A) uses similar bus definitions
> > > as the GX SoCs (for which there are public datasheets) - this is how
> > > Jianxin added it to meson-g12a.dtsi originally
> >
> > And it was the same for the GX family. AOBUS in the DT while there nothing
> > about this in the memory map. Keep wrong patterns does not make them
> > right.
> >
> > I'm merely reading the memory map here
> can you explain which "memory map" you are reading exactly?
>
> my source is the "memory map" section in the public S905X and S912
> datasheets:
> - "S912_Datasheet_V0.220170314publicversion-Wesion.pdf" page 48
> - "S905X_Datasheet V0.3 20170314publicversion-Wesion.pdf" page 43

Those section yes

>
> > > - this patch does it different - but cannot check if this is correct
> > > (no public datasheet is available for G12A or AXG) nor do I have a
> > > "big picture" of upcoming changes
> >
> > Yes it does it differently. We should have picked up on this a while ago,
> > since gxbb at least, and we did not. There reason to create bus that don't
> > exist in the memory map of any recent SoC.
> in the GXL and GXM datasheets from above there is:
> range: 0xC8100000 - 0xC81FFFFF
> name: RTI

So we apparently agree that aobus does not exist

RTI simply goes fron 0xff800000 - 0xff800fff on the G12a doc, it is as simple
as that - as done in the pinctrl.

>
> thus in my opinion, based on the two datasheets from above:
> - the bus definition (start, size) in meson-gx.dtsi is fine
> - probably have the wrong name for this bus in meson-gx.dtsi
> - (assuming that GXBB, GXL and GXM are all the same - I only compared
> the last two)

I still dont understand this focus on aobus which simply does not exist in the
map, whether it is by name of size or offset. Same goes cbus and apb.

>
> > Creating bus should at least require a start and end offset explained
> > somewhere. Copying vendor DT is not enough
> I fully agree on this one!
>
>
> Regards
> Martin



2019-01-18 18:11:27

by Jianxin Pan

[permalink] [raw]
Subject: Re: [PATCH] arm64: dts: meson: fix g12a buses


Hi Martin and Jerome,

On 2019/1/18 5:20, Martin Blumenstingl wrote:
> On Thu, Jan 17, 2019 at 9:39 PM Jerome Brunet <[email protected]> wrote:
>>
>> On Thu, 2019-01-17 at 21:27 +0100, Martin Blumenstingl wrote:
>>> OK, but we had incorrect documentation in the past. did you check this
>>> with someone from Amlogic?
>>>
>>> I'm curious because there seem to be two different approaches here:
>>> 1) hiubus name and offsets are being fixed within this patch
>>> 2) aobus is being dropped here and re-introduced with a different name later
>>> on
>>>
>>
>> because hiu exist and aobus does not, for which both the name and size was
>> wrong
>>
>>> approach 1) can also be used for the "rti" region (at least in my
>>> opinion, the patch doesn't explain why it can't be done):
>>
>> THe patch remove aobus (instead of fixing name and size) because, of the
>> multiple region documented covered by this 'made region', I did not anticipate
>> which one will be required and I did not want to add them all.
>>
>> Better to add them as needed, which is want I done for pinctrl as you pointed
>> out
>>
>>> rename "aobus" to "rti" and change the size to either 0x1000 or 0xb000
>>> (both values can be found in mesong12a.dtsi from
>>> buildroot_openlinux_kernel_4.9_fbdev_20180706)
>>
>> RTI is added here:
>> https://lkml.kernel.org/r/[email protected]
>>
>> I don't really understand the problem ? result is the same
> the actual problem is "me" as I have conflicting information:
> - Amlogic's buildroot kernel (for G12A) uses similar bus definitions
> as the GX SoCs (for which there are public datasheets) - this is how
> Jianxin added it to meson-g12a.dtsi originally
> - this patch does it different - but cannot check if this is correct
> (no public datasheet is available for G12A or AXG) nor do I have a
> "big picture" of upcoming changes
>
> Cc'ing Jianxin: can you please review Jerome's patch and give some
> more details on the memory map on G12A so further contributions can be
> reviewed easier?
>
1. "aobus: bus@ff800000" describes the following registers:
ahb ao_reg reserved 980 FF80B000
ahb ao_reg ao_mailbox 4 FF80A000
ahb ao_reg sar_adc 4 FF809000
ahb ao_reg ir_dec 4 FF808000
ahb ao_reg pwm_ab 4 FF807000
ahb ao_reg i2c_s 4 FF806000
ahb ao_reg i2c_m 4 FF805000
ahb ao_reg uart2 4 FF804000
ahb ao_reg uart 4 FF803000
ahb ao_reg pwm_cd 4 FF802000
ahb ao_reg reserved 4 FF801000
ahb ao_reg rti 4 FF800000


2. "cbus: bus@ffd00000" describes the following registers:
capb3 cbus reserved 872 FFD26000 FFDFFFFF
capb3 cbus sc 4 FFD25000 FFD25FFF
capb3 cbus uart0 4 FFD24000 FFD24FFF
capb3 cbus uart1 4 FFD23000 FFD23FFF
capb3 cbus uart2 4 FFD22000 FFD22FFF
capb3 cbus reserved 4 FFD21000 FFD21FFF
capb3 cbus reserved 4 FFD20000 FFD20FFF
capb3 cbus i2c_m0 4 FFD1F000 FFD1FFFF
capb3 cbus i2c_m1 4 FFD1E000 FFD1EFFF
capb3 cbus i2c_m2 4 FFD1D000 FFD1DFFF
capb3 cbus i2c_m3 4 FFD1C000 FFD1CFFF
capb3 cbus pwm_ab 4 FFD1B000 FFD1BFFF
capb3 cbus pwm_cd 4 FFD1A000 FFD1AFFF
capb3 cbus pwm_ef 4 FFD19000 FFD19FFF
capb3 cbus msr_clk 4 FFD18000 FFD18FFF
capb3 cbus reserved 4 FFD17000 FFD17FFF
capb3 cbus reserved 4 FFD16000 FFD16FFF
capb3 cbus spicc_1 4 FFD15000 FFD15FFF
capb3 cbus spifc 4 FFD14000 FFD14FFF
capb3 cbus spicc_0 4 FFD13000 FFD13FFF
capb3 cbus reserved 4 FFD12000 FFD12FFF
capb3 cbus reserved 4 FFD11000 FFD11FFF
capb3 cbus reserved 4 FFD10000 FFD10FFF
capb3 cbus isa 4 FFD0F000 FFD0FFFF
capb3 cbus parser 4 FFD0E000 FFD0EFFF
capb3 cbus reserved 4 FFD0D000 FFD0DFFF
capb3 cbus sana 4 FFD0C000 FFD0CFFF
capb3 cbus stream 4 FFD0B000 FFD0BFFF
capb3 cbus async_fifo 4 FFD0A000 FFD0AFFF
capb3 cbus async_fifo2 4 FFD09000 FFD09FFF
capb3 cbus assist 4 FFD08000 FFD08FFF
capb3 cbus mipi_dsi_host 4 FFD07000 FFD07FFF
capb3 cbus stb 4 FFD06000 FFD06FFF
capb3 cbus aififo 4 FFD05000 FFD05FFF
capb3 cbus reserved 4 FFD04000 FFD04FFF
capb3 cbus reserved 4 FFD03000 FFD03FFF
capb3 cbus reserved 4 FFD02000 FFD02FFF
capb3 cbus reset 4 FFD01000 FFD01FFF
capb3 cbus reserved 4 FFD00000 FFD00FFF

3. In public data sheet, I can only found memory map about sub-regions of each bus.
And no information about the bus itself.

>
> Regards
> Martin
>
> .
>


2019-01-18 21:09:00

by Jerome Brunet

[permalink] [raw]
Subject: Re: [PATCH] arm64: dts: meson: fix g12a buses

On Sat, 2019-01-19 at 02:09 +0800, Jianxin Pan wrote:
> Hi Martin and Jerome,
>
> On 2019/1/18 5:20, Martin Blumenstingl wrote:
> > On Thu, Jan 17, 2019 at 9:39 PM Jerome Brunet <[email protected]>
> > wrote:
> > > On Thu, 2019-01-17 at 21:27 +0100, Martin Blumenstingl wrote:
> > > > OK, but we had incorrect documentation in the past. did you check this
> > > > with someone from Amlogic?
> > > >
> > > > I'm curious because there seem to be two different approaches here:
> > > > 1) hiubus name and offsets are being fixed within this patch
> > > > 2) aobus is being dropped here and re-introduced with a different name
> > > > later
> > > > on
> > > >
> > >
> > > because hiu exist and aobus does not, for which both the name and size
> > > was
> > > wrong
> > >
> > > > approach 1) can also be used for the "rti" region (at least in my
> > > > opinion, the patch doesn't explain why it can't be done):
> > >
> > > THe patch remove aobus (instead of fixing name and size) because, of the
> > > multiple region documented covered by this 'made region', I did not
> > > anticipate
> > > which one will be required and I did not want to add them all.
> > >
> > > Better to add them as needed, which is want I done for pinctrl as you
> > > pointed
> > > out
> > >
> > > > rename "aobus" to "rti" and change the size to either 0x1000 or 0xb000
> > > > (both values can be found in mesong12a.dtsi from
> > > > buildroot_openlinux_kernel_4.9_fbdev_20180706)
> > >
> > > RTI is added here:
> > > https://lkml.kernel.org/r/[email protected]
> > >
> > > I don't really understand the problem ? result is the same
> > the actual problem is "me" as I have conflicting information:
> > - Amlogic's buildroot kernel (for G12A) uses similar bus definitions
> > as the GX SoCs (for which there are public datasheets) - this is how
> > Jianxin added it to meson-g12a.dtsi originally
> > - this patch does it different - but cannot check if this is correct
> > (no public datasheet is available for G12A or AXG) nor do I have a
> > "big picture" of upcoming changes
> >
> > Cc'ing Jianxin: can you please review Jerome's patch and give some
> > more details on the memory map on G12A so further contributions can be
> > reviewed easier?
> >
> 1. "aobus: bus@ff800000" describes the following registers:
> ahb ao_reg reserved 980 FF80B000
> ahb ao_reg ao_mailbox 4 FF80A000
> ahb ao_reg sar_adc 4 FF809000
> ahb ao_reg ir_dec 4 FF808000
> ahb ao_reg pwm_ab 4 FF807000
> ahb ao_reg i2c_s 4 FF806000
> ahb ao_reg i2c_m 4 FF805000
> ahb ao_reg uart2 4 FF804000
> ahb ao_reg uart 4 FF803000
> ahb ao_reg pwm_cd 4 FF802000
> ahb ao_reg reserved 4 FF801000
> ahb ao_reg rti 4 FF800000
>
>
> 2. "cbus: bus@ffd00000" describes the following registers:
> capb3 cbus reserved 872 FFD26000 FFDFFFFF
> capb3 cbus sc 4 FFD25000 FFD25FFF
> capb3 cbus uart0 4 FFD24000 FFD24FFF
> capb3 cbus uart1 4 FFD23000 FFD23FFF
> capb3 cbus uart2 4 FFD22000 FFD22FFF
> capb3 cbus reserved 4 FFD21000 FFD21FFF
> capb3 cbus reserved 4 FFD20000 FFD20FFF
> capb3 cbus i2c_m0 4 FFD1F000 FFD1FFFF
> capb3 cbus i2c_m1 4 FFD1E000 FFD1EFFF
> capb3 cbus i2c_m2 4 FFD1D000 FFD1DFFF
> capb3 cbus i2c_m3 4 FFD1C000 FFD1CFFF
> capb3 cbus pwm_ab 4 FFD1B000 FFD1BFFF
> capb3 cbus pwm_cd 4 FFD1A000 FFD1AFFF
> capb3 cbus pwm_ef 4 FFD19000 FFD19FFF
> capb3 cbus msr_clk 4 FFD18000 FFD18FFF
> capb3 cbus reserved 4 FFD17000 FFD17FFF
> capb3 cbus reserved 4 FFD16000 FFD16FFF
> capb3 cbus spicc_1 4 FFD15000 FFD15FFF
> capb3 cbus spifc 4 FFD14000 FFD14FFF
> capb3 cbus spicc_0 4 FFD13000 FFD13FFF
> capb3 cbus reserved 4 FFD12000 FFD12FFF
> capb3 cbus reserved 4 FFD11000 FFD11FFF
> capb3 cbus reserved 4 FFD10000 FFD10FFF
> capb3 cbus isa 4 FFD0F000 FFD0FFFF
> capb3 cbus parser 4 FFD0E000 FFD0EFFF
> capb3 cbus reserved 4 FFD0D000 FFD0DFFF
> capb3 cbus sana 4 FFD0C000 FFD0CFFF
> capb3 cbus stream 4 FFD0B000 FFD0BFFF
> capb3 cbus async_fifo 4 FFD0A000 FFD0AFFF
> capb3 cbus async_fifo2 4 FFD09000 FFD09FFF
> capb3 cbus assist 4 FFD08000 FFD08FFF
> capb3 cbus mipi_dsi_host 4 FFD07000 FFD07FFF
> capb3 cbus stb 4 FFD06000 FFD06FFF
> capb3 cbus aififo 4 FFD05000 FFD05FFF
> capb3 cbus reserved 4 FFD04000 FFD04FFF
> capb3 cbus reserved 4 FFD03000 FFD03FFF
> capb3 cbus reserved 4 FFD02000 FFD02FFF
> capb3 cbus reset 4 FFD01000 FFD01FFF
> capb3 cbus reserved 4 FFD00000 FFD00FFF

Hu Jianxin,

Thanks for sharing this. Any other region we should know about ? what about
the apb region that isn't documented either ?

>
> 3. In public data sheet, I can only found memory map about sub-regions of
> each bus.
> And no information about the bus itself.
>
> > Regards
> > Martin
> >
> > .
> >