| Message ID | 20190116165236.8330-1-jbrunet@baylibre.com |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: dts: meson: fix g12a buses | expand |
Jerome Brunet <jbrunet@baylibre.com> 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 <jbrunet@baylibre.com> Queuing as a fix for v5.0-rc, Thanks, Kevin
Jerome Brunet <jbrunet@baylibre.com> 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 <jbrunet@baylibre.com> Queued as a fix for v5.0-rc (branch: v5.0/fixes) Kevin
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 <jbrunet@baylibre.com> 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/
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/20190117103151.3349-1-jbrunet@baylibre.com I don't really understand the problem ? result is the same
On Thu, Jan 17, 2019 at 9:39 PM Jerome Brunet <jbrunet@baylibre.com> 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/20190117103151.3349-1-jbrunet@baylibre.com > > 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
On Thu, Jan 17, 2019 at 10:45 PM Jerome Brunet <jbrunet@baylibre.com> wrote: > > On Thu, 2019-01-17 at 22:20 +0100, Martin Blumenstingl wrote: > > On Thu, Jan 17, 2019 at 9:39 PM Jerome Brunet <jbrunet@baylibre.com> 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/20190117103151.3349-1-jbrunet@baylibre.com > > > > > > 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
Hi Martin and Jerome, On 2019/1/18 5:20, Martin Blumenstingl wrote: > On Thu, Jan 17, 2019 at 9:39 PM Jerome Brunet <jbrunet@baylibre.com> 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/20190117103151.3349-1-jbrunet@baylibre.com >> >> 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 > > . >
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 <jbrunet@baylibre.com> > > 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/20190117103151.3349-1-jbrunet@baylibre.com > > > > > > 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 > > > > . > >
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 {
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 <jbrunet@baylibre.com> --- arch/arm64/boot/dts/amlogic/meson-g12a.dtsi | 70 +++++++-------------- 1 file changed, 24 insertions(+), 46 deletions(-) -- 2.20.1