[U-Boot] [PATCH] arm: Add support for Trenz TE0820 (zynqmp)

Wed May 2 10:53:32 UTC 2018

Hi,

On 2.5.2018 11:06, Vladimir Svoboda wrote:
> Thanks for the review,
> 
> On 02/05/18 10:00, Michal Simek wrote:
>> Hi, + Trenz support.
>>
>> On 30.4.2018 16:10, Vladimir Svoboda wrote:
>>> Add support for Trenz TE0820 revision 2 MPSoC module.
>>>
>>> Signed-off-by: Vladimir Svoboda <ze.vlad at gmail.com>
>> I can't see your name in git log that's why I expect this is maybe your
>> the first submission. How can I trust that you will test this board
>> regularly that make sense to have it in the tree?
>>
>> I don't have this board that's why it can be broken quickly.
> This is indeed my first patch submission to U-boot.
> I work at HIPPEROS S.A. and we develop a RTOS.
> We are part of a european-funded project, called Tulipp, where partners use
> that board.
> That's the guarantee I can give you.

It means when this project ends the most likely none will take care
about this configuration anymore.

>>
>>> ---
>>>
>>>   arch/arm/dts/Makefile                         |   1 +
>>>   arch/arm/dts/zynqmp-te0820-rev2.dts           | 669 ++++++++++++++++++
>>>   .../zynqmp/zynqmp-te0820-rev2/psu_init_gpl.c  | 624 ++++++++++++++++
>> There are a lot of versions with different chips on it. It will be good
>> to exactly write what version this is going to support.
>> Also there is different amount of flash on 1EL version based on
>> https://shop.trenz-electronic.de/en/Products/Trenz-Electronic/TE08XX-Zynq-UltraScale/TE0820-Zynq-UltraScale/
>>
>>
>> Also this is SOM and it requires Carrier board to know wiring. What
>> carrier board is this configuration for?
> 
> Indeed, there are several versions.
> I can only test with the module TE0820-02-03EG-1EA as this is the only
> one we
> have.

That's not a problem but it needs to be said in commit message.

> The carrier board we use is the Sundance EMC2, they will sell it with the
> TE0820 module under the name EMC2-ZU3EG.

Ok. Then dts needs to be structure properly.

It means dts file for Sundance EMC2 which describe board components (at
least that one you care.
dtsi for SOM which includes zynqmp.dtsi and really describes only things
available on the SOM.

>>>   configs/xilinx_zynqmp_te0820_rev2_defconfig   | 105 +++
>>>   include/configs/xilinx_zynqmp_te0820.h        |  49 ++
>>>   5 files changed, 1448 insertions(+)
>>>   create mode 100644 arch/arm/dts/zynqmp-te0820-rev2.dts
>>>   create mode 100644
>>> board/xilinx/zynqmp/zynqmp-te0820-rev2/psu_init_gpl.c
>>>   create mode 100644 configs/xilinx_zynqmp_te0820_rev2_defconfig
>>>   create mode 100644 include/configs/xilinx_zynqmp_te0820.h
>>>
>>> diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile
>>> index ac7667b1e8..ec4e7cc206 100644
>>> --- a/arch/arm/dts/Makefile
>>> +++ b/arch/arm/dts/Makefile
>>> @@ -148,6 +148,7 @@ dtb-$(CONFIG_ARCH_ZYNQ) += \
>>>   dtb-$(CONFIG_ARCH_ZYNQMP) += \
>>>       zynqmp-mini-emmc.dtb            \
>>>       zynqmp-mini-nand.dtb            \
>>> +    zynqmp-te0820-rev2.dtb            \
>> This is Trenz that's why it you should use trenz instead of zynqmp.
> Just for my information, what does zynqmp refers to then?
> I thought it was the technical name for UltraScale+ MPSoC.

ZynqMP is shortcut for UltraScale+ MPSoC and in u-boot this is a little
bit mess but in the Linux kernel it is inside xilinx folder.

Based on information above it will be the best to call it
sundance-emc2-revX-te0820-revY.dts
where revX is carrier board revision and revY is SoM revision.

>>
>>>       zynqmp-zcu100-revC.dtb            \
>>>       zynqmp-zcu102-revA.dtb            \
>>>       zynqmp-zcu102-revB.dtb            \
>>> diff --git a/arch/arm/dts/zynqmp-te0820-rev2.dts
>>> b/arch/arm/dts/zynqmp-te0820-rev2.dts
>>> new file mode 100644
>>> index 0000000000..db23dfe45c
>>> --- /dev/null
>>> +++ b/arch/arm/dts/zynqmp-te0820-rev2.dts
>>> @@ -0,0 +1,669 @@
>>> +/*
>>> + * dts file for Xilinx ZynqMP TE0820 Rev2
>> Trenz again.
> So you expect "Xilinx Trenz TE0820 Rev2" or "Trenz TE0820 Rev2"?
>>
>>> + *
>>> + * (C) Copyright 2018, HIPPEROS.
>>> + *
>>> + * Vladimir Svoboda <vladimir.svoboda at hipperos.com>
>>> + *
>>> + * SPDX-License-Identifier:    GPL-2.0+
>> Please use kernel format where this line comes first.
>>
>>> + */
>>> +
>>> +/dts-v1/;
>>> +
>>> +#include "zynqmp.dtsi"
>>> +#include "zynqmp-clk-ccf.dtsi"
>>> +#include <dt-bindings/input/input.h>
>>> +#include <dt-bindings/gpio/gpio.h>
>>> +#include <dt-bindings/phy/phy.h>
>>> +
>>> +/ {
>>> +    model = "ZynqMP TE0820 Rev2";
>> Trenz,
>>
>>> +    compatible = "xlnx,zynqmp-te0820-rev2", "xlnx,zynqmp-te0820",
>>> "xlnx,zynqmp";
>> Add trenz to vendor-prefixes.txt in the Linux kernel and use it here.
>>
>>> +
>>> +    aliases {
>>> +        ethernet0 = &gem3;
>>> +        gpio0 = &gpio;
>> Based on Rob Herring this shouldn't be here.
>>
>>
>>> +        i2c0 = &i2c0;
>>> +        i2c1 = &i2c1;
>>> +        mmc0 = &sdhci1;
>>> +        rtc0 = &rtc;
>>> +        serial0 = &uart0;
>>> +        serial1 = &uart1;
>>> +        serial2 = &dcc;
>>> +        spi0 = &qspi;
>>> +        usb0 = &usb0;
>> The same for gpio.
>>
>>> +    };
>>> +
>>> +    chosen {
>>> +        bootargs = "earlycon";
>>> +        stdout-path = "serial0:115200n8";
>>> +    };
>>> +
>>> +    memory at 0 {
>>> +        device_type = "memory";
>>> +        reg = <0x0 0x0 0x0 0x40000000>;
>>> +    };
>>> +
>>> +    gpio-keys {
>>> +        compatible = "gpio-keys";
>>> +        #address-cells = <1>;
>>> +        #size-cells = <0>;
>>> +        autorepeat;
>>> +        sw19 {
>>> +            label = "sw19";
>>> +            gpios = <&gpio 22 GPIO_ACTIVE_HIGH>;
>>> +            linux,code = <KEY_DOWN>;
>>> +            gpio-key,wakeup;
>>> +            autorepeat;
>>> +        };
>>> +    };
>>> +
>>> +    leds {
>>> +        compatible = "gpio-leds";
>>> +        heartbeat_led {
>>> +            label = "heartbeat";
>>> +            gpios = <&gpio 23 GPIO_ACTIVE_HIGH>;
>>> +            linux,default-trigger = "heartbeat";
>>> +        };
>>> +    };
>> This and stuff below looks like c&p from zcu102 version which is
>> incorrect.
> This is indeed a c&p from the ZCU 102 where I modified a couple of things
> (memory size, SDIO...).
> I did not check the validity of all the devices information and there are
> probably errors regarding the routing of GPIO.

Keep there only things you have validated and ignore the rest.

>>
>> I am going to skip the rest of this patch because questions above needs
>> to be answered first.
> 
> I hope that this answer will help to continue the review.

Will see. The best will be to see Trenz to support their HW instead of
3rd party.

Thanks,
Michal