[U-Boot] [PATCH v2] zynqmp: Add generic target

Wed Apr 25 07:40:25 UTC 2018

On 25.04.18 08:25, Michal Simek wrote:
> Hi Alex,
> 
> On 24.4.2018 21:50, Alexander Graf wrote:
>> I would like to create a generic U-Boot build that adapts itself completely
>> based on the DT passed in. That way we can potentially support running
>> random board configurations with a single U-Boot binary built as part of
>> the distribution.
>>
>> Currently a few things are still missing to make it a full reality. The
>> most obvious one I think is the EEPROM location. This would need to also
>> move into something described by DT.
> 
> yes eeprom is one part. We have usb in generic way in Xilinx tree and my
> colleague will be rebasing that patches hopefully soon to get rid of
> CONFIG_ZYNQMP_XHCI_LIST.

That's great to hear :).

> Also storing variables is problematic category.

Yes, we need to probably create a u-boot specific device node on top of
an entity in DT to indicate where variables are stored and then use that
instead of the hard coded locations.

> i2c DM driver is already in the tree but not all i2c device drivers have
> been updated yet.
> 
> I have sent one patch to fix ident string.
> "arm64: zynqmp: Show model information instead of custom IDENT_STRING"

Thanks, I integrated that in v3 - which apparently collided with your
email :).

>> Apart from that, we're almost there. This patch adds a defconfig that simply
>> contains all drivers we could make use of. We can then enable individual
>> boards along the way and slowly adapt everything to be fully DT described
>> while we identify each missing bit.
> 
> I have this in my mind for pretty long time and microblaze-generic
> configuration is proof of that. The first step is get rid of all
> include/configs/xilinx_zynqmp_* files.
> This is done for example for zc1751 dc4.
> 
> With usb patches in we can remove dc2 directly and reduce board configs.
> 
> dc1/dc3/dc5/zc1275 can be remove when we found how to handle mmcs boot
> target better. (You have the same issue below). This is maybe a topic we
> should look together because it is directly related to distro default
> and distro boot.

More than happy to :)

> 
>>
>> Signed-off-by: Alexander Graf <agraf at suse.de>
>>
>> ---
>>
>> v1 -> v2:
>>
>>   - Remove debug uart
>> ---
>>  configs/xilinx_zynqmp_generic_defconfig | 96 +++++++++++++++++++++++++++++++++
>>  include/configs/xilinx_zynqmp_generic.h | 23 ++++++++
>>  2 files changed, 119 insertions(+)
>>  create mode 100644 configs/xilinx_zynqmp_generic_defconfig
>>  create mode 100644 include/configs/xilinx_zynqmp_generic.h
>>
>> diff --git a/configs/xilinx_zynqmp_generic_defconfig b/configs/xilinx_zynqmp_generic_defconfig
>> new file mode 100644
>> index 0000000000..d534866d23
>> --- /dev/null
>> +++ b/configs/xilinx_zynqmp_generic_defconfig
>> @@ -0,0 +1,96 @@
>> +CONFIG_ARM=y
>> +CONFIG_SYS_CONFIG_NAME="xilinx_zynqmp_generic"
> 
> it shouldn't be needed at all.

We need it until at least usb3 is converted, no?

> 
> 
>> +CONFIG_ARCH_ZYNQMP=y
>> +CONFIG_SYS_TEXT_BASE=0x8000000
>> +CONFIG_SYS_MALLOC_F_LEN=0x8000
>> +CONFIG_IDENT_STRING=" Xilinx ZynqMP based platform"
> 
> It should be fine to keep this here to say that this is generic platform.

Good point.

> 
>> +CONFIG_DEFAULT_DEVICE_TREE="zynqmp-mini-emmc"
> 
> you don't need to fill it

It broke for me earlier without the option, but you're right - it
compiles now. I'll omit it.

> 
>> +CONFIG_ZYNQMP_USB=y
>> +CONFIG_AHCI=y
>> +CONFIG_DISTRO_DEFAULTS=y
>> +CONFIG_FIT=y
>> +CONFIG_FIT_VERBOSE=y
>> +CONFIG_SPL_LOAD_FIT=y
>> +# CONFIG_DISPLAY_CPUINFO is not set
>> +# CONFIG_DISPLAY_BOARDINFO is not set
> 
> This should be enabled to see model from DT.

Yes.

> 
>> +CONFIG_BOARD_EARLY_INIT_R=y
>> +CONFIG_SPL_OS_BOOT=y
>> +CONFIG_SPL_RAM_SUPPORT=y
>> +CONFIG_SPL_RAM_DEVICE=y
>> +CONFIG_SPL_ATF=y
> 
> you have probably c&p this from different zynqmp target but
> these SPL will go away. make xilinx_zynqmp_generic_defconfig; make
> savedefconfig; cp defconfig configs/xilinx_zynqmp_generic_defconfig; git
> diff.

Thanks.

> 
>> +CONFIG_SYS_PROMPT="ZynqMP> "
>> +CONFIG_FASTBOOT=y
>> +CONFIG_FASTBOOT_FLASH=y
>> +CONFIG_FASTBOOT_FLASH_MMC_DEV=0
>> +CONFIG_CMD_THOR_DOWNLOAD=y
>> +CONFIG_CMD_MEMTEST=y
>> +CONFIG_SYS_ALT_MEMTEST=y
>> +CONFIG_CMD_CLK=y
>> +CONFIG_CMD_DFU=y
>> +# CONFIG_CMD_FLASH is not set
>> +CONFIG_CMD_FPGA_LOADBP=y
>> +CONFIG_CMD_FPGA_LOADP=y
>> +CONFIG_CMD_GPIO=y
>> +CONFIG_CMD_GPT=y
>> +CONFIG_CMD_I2C=y
>> +CONFIG_CMD_MMC=y
>> +CONFIG_CMD_USB=y
>> +CONFIG_CMD_TFTPPUT=y
>> +CONFIG_CMD_TIME=y
>> +CONFIG_CMD_TIMER=y
>> +CONFIG_CMD_EXT4_WRITE=y
>> +CONFIG_SPL_OF_CONTROL=y
>> +CONFIG_OF_BOARD=y
>> +CONFIG_ENV_IS_IN_FAT=y
> 
> this is probably wrong because this is expecting variables in fat which
> won't work for systems without SD.

As mentioned above this should probably be solved by exposing a special
environment device that describes where the environment is stored.

For the time being, I think SD is the main target, so storing env there
doesn't sound too bad.

> 
> 
>> +CONFIG_NET_RANDOM_ETHADDR=y
>> +CONFIG_SPL_DM=y
>> +CONFIG_SPL_DM_SEQ_ALIAS=y
>> +CONFIG_SCSI_AHCI=y
>> +CONFIG_SATA_CEVA=y
>> +CONFIG_CLK_ZYNQMP=y
>> +CONFIG_DFU_RAM=y
>> +CONFIG_FPGA_XILINX=y
>> +CONFIG_FPGA_ZYNQMPPL=y
>> +CONFIG_DM_GPIO=y
>> +CONFIG_CMD_PCA953X=y
>> +CONFIG_SYS_I2C_ZYNQ=y
>> +CONFIG_ZYNQ_I2C0=y
>> +CONFIG_ZYNQ_I2C1=y
> 
> These 3 should be replaced by CONFIG_SYS_I2C_CADENCE

Thanks :)

> 
>> +CONFIG_MISC=y
>> +CONFIG_DM_MMC=y
>> +CONFIG_MMC_SDHCI=y
>> +CONFIG_MMC_SDHCI_ZYNQ=y
>> +CONFIG_SPI_FLASH=y
>> +CONFIG_SPI_FLASH_BAR=y
>> +CONFIG_SPI_FLASH_MACRONIX=y
>> +CONFIG_SPI_FLASH_SPANSION=y
>> +CONFIG_SPI_FLASH_STMICRO=y
>> +CONFIG_SPI_FLASH_WINBOND=y
>> +# CONFIG_SPI_FLASH_USE_4K_SECTORS is not set
>> +CONFIG_PHY_MARVELL=y
>> +CONFIG_PHY_NATSEMI=y
>> +CONFIG_PHY_REALTEK=y
>> +CONFIG_PHY_TI=y
>> +CONFIG_PHY_VITESSE=y
>> +CONFIG_PHY_FIXED=y
>> +CONFIG_DM_ETH=y
>> +CONFIG_PHY_GIGE=y
>> +CONFIG_ZYNQ_GEM=y
>> +CONFIG_SCSI=y
>> +CONFIG_DM_SCSI=y
>> +CONFIG_ZYNQ_SERIAL=y
>> +CONFIG_USB=y
>> +CONFIG_USB_XHCI_HCD=y
>> +CONFIG_USB_XHCI_DWC3=y
>> +CONFIG_USB_XHCI_ZYNQMP=y
>> +CONFIG_USB_DWC3=y
>> +CONFIG_USB_DWC3_GADGET=y
>> +CONFIG_USB_ULPI_VIEWPORT=y
>> +CONFIG_USB_ULPI=y
>> +CONFIG_USB_STORAGE=y
>> +CONFIG_USB_GADGET=y
>> +CONFIG_USB_GADGET_MANUFACTURER="Xilinx"
>> +CONFIG_USB_GADGET_VENDOR_NUM=0x03FD
>> +CONFIG_USB_GADGET_PRODUCT_NUM=0x0300
>> +CONFIG_USB_FUNCTION_THOR=y
>> +CONFIG_EFI_LOADER_BOUNCE_BUFFER=y
>> diff --git a/include/configs/xilinx_zynqmp_generic.h b/include/configs/xilinx_zynqmp_generic.h
>> new file mode 100644
>> index 0000000000..56bd5bd6f1
>> --- /dev/null
>> +++ b/include/configs/xilinx_zynqmp_generic.h
>> @@ -0,0 +1,23 @@
>> +/*
>> + * Configuration for the Xilinx ZynqMP generic platform
>> + *
>> + * (C) Copyright 2018 Alexander Graf <agraf at suse.de>
>> + * (C) Copyright 2015 Xilinx, Inc.
>> + * Michal Simek <michal.simek at xilinx.com>
>> + *
>> + * SPDX-License-Identifier:	GPL-2.0+
>> + */
>> +
>> +#ifndef __CONFIG_ZYNQMP_GENERIC_H
>> +#define __CONFIG_ZYNQMP_GENERIC_H
>> +
>> +/* All of these should be enumerated using DT instead ... */
>> +
>> +#define CONFIG_ZYNQ_SDHCI0
>> +#define CONFIG_ZYNQ_SDHCI1
> 
> This is wrong. It has to be solved differently.

I see what you mean.

> 
> 
>> +
>> +#define CONFIG_ZYNQMP_XHCI_LIST {ZYNQMP_USB0_XHCI_BASEADDR}
> 
> In generic case here should be both controllers but as I said we know
> how go get rid of this.

Ok, let me make it both controllers then.

> 
>> +
>> +#include <configs/xilinx_zynqmp.h>
>> +
>> +#endif /* __CONFIG_ZYNQMP_GENERIC_H */
>>
> 
> 
> We are not that far from using generic defconfig even now but there are
> some issues on that path and unfortunately your patch is not solving any
> of that issue. It almost just enables OF_BOARD instead of OF_EMBED which
> I use as default because OF_BOARD didn't work in past and I didn't have
> a time to look at it why.

Yes, the rationale is a different one from what you might think though :).

I want to build tools on top of the generic support. If the generic
support isn't as generic as it can be today, that's perfectly fine. Then
we just simply don't support a few features. But at least we know that
the path is properly sanctioned upstream and simply dropping in a newer
U-Boot version will get us more features.

> I am happy to work with you to resolve issues described above but we are
> not there yet.
> Also at this stage I don't think it is worth to create another target
> which is suggesting generic configuration but it is not now.

I would like to first introduce the target so that tools on top can
build on it. Then resolve each issue one by one. And as you mentioned
some parts are already in the works by people in your team.

If we block the generic target until everything is truly generic, we'd
end up in a situation where one thing waits for another waits for
another etc etc and we'll never finish (like your firmware driver
situation in Linux).

> At the end of this path will be interesting to see generic defconfig and
> then just board defconfig fragments and use
> scripts/kconfig/merge_config.sh to create final .config file.

I agree, but that's a different goal :). I want to be able to build a
fully generic tool stack on top that you can just pass an hdf file and
it gives you a working openSUSE distribution. And I'd prefer if I didn't
have to do that based on downstream code.

Alex