[PATCH 3/3] PowerPC: keymile: Add support for kmcent2 board
Niel Fourie
lusus at denx.de
Thu Jan 21 13:14:44 CET 2021
Hi Stefan
On 19/01/2021 16:31, Stefan Roese wrote:
<snip>
>> diff --git a/board/keymile/common/ivm.c b/board/keymile/common/ivm.c
>> index 60b89fe348..608406dacd 100644
>> --- a/board/keymile/common/ivm.c
>> +++ b/board/keymile/common/ivm.c
>> @@ -321,6 +321,11 @@ static int ivm_populate_env(unsigned char *buf,
>> int len, int mac_address_offset)
>> process_mac(valbuf, page2, mac_address_offset, true);
>> env_set((char *)"eth1addr", (char *)valbuf);
>> #endif
>> +#if defined(CONFIG_TARGET_KMCENT2)
>
> Can't you switch to using if (IS_ENABLED(CONFIG_TARGET_KMCENT2)) instead
> adding more #ifdef's to this code?
Makes sense, changed that.
>
>> +/* 3rd ethernet interface */
>> + process_mac(valbuf, page2, 2, true);
>> + env_set((char *)"eth4addr", (char *)valbuf);
>> +#endif
>
> eth_env_set_enetaddr() ?
This one is tricky, as process_mac() already formatted the mac address
as a string (using the "%pM" format string) already, while
eth_env_set_enetaddr() again applies the "%pM" format string to its input.
There are three other instances of the process_mac()/env_set() as above
in this file, which would need to be changed along with process_mac()
itself (to not apply "%pM"), so that eth_env_set_enetaddr() can be used.
Should I do this for all the cases?
<snip>
>> diff --git a/board/keymile/kmcent2/ddr.c b/board/keymile/kmcent2/ddr.c
>> new file mode 100644
>> index 0000000000..2b4e58795a
>> --- /dev/null
>> +++ b/board/keymile/kmcent2/ddr.c
>> @@ -0,0 +1,88 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * (C) Copyright 2016 Keymile AG
>> + * Rainer Boschung <rainer.boschung at keymile.com>
>> + *
>> + * Copyright 2013 Freescale Semiconductor, Inc.
>> + */
>> +
>> +#include <common.h>
>
> Please don't include common.h any more. Simon has worked hard to move
> things out of common.h so that it should not be used any more.
Thanks, I did not know of this, will apply everywhere.
>> +#include <i2c.h>
>> +#include <hwconfig.h>
>> +#include <asm/mmu.h>
>> +#include <fsl_ddr_sdram.h>
>> +#include <fsl_ddr_dimm_params.h>
>> +#include <asm/fsl_law.h>
>> +#include <asm/mpc85xx_gpio.h>
>> +#include <init.h>
>
> Sort header includes?
Thanks, will apply everywhere. I hit a couple of snags, though. Above,
fsl_ddr_dimm_params.h depends on generic_spd_eeprom_t defined in
fsl_ddr_sdram.h, so I kept those two in that order.
Also in law.c, asm/fsl_law.h need config.h before it to compile.
>> +
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>> +#define DQSn_POS(n) (3 - (((n) - 1) % 4)) * 8
>> +#define DQSn_START(n, start) ((start) << DQSn_POS(n))
>
> Empty line please.
Fixed
<snip>
>> diff --git a/board/keymile/kmcent2/kmcent2.c
>> b/board/keymile/kmcent2/kmcent2.c
>> new file mode 100644
>> index 0000000000..3d6bd8328e
>> --- /dev/null
>> +++ b/board/keymile/kmcent2/kmcent2.c
<snip>
> Nitpicking: The official coding style for multi line comment is this:
>
> /*
> * CFE_RST (front phy):
> * reset at power-up, unit and core reset, deasset reset w/o WD
> */
>
> Please consistant in this file.
Thanks, fixed everywhere.
<snip>
>> +#if defined(CONFIG_HUSH_INIT_VAR)
>> +int hush_init_var(void)
>> +{
>> + ivm_analyze_eeprom(ivm_content, CONFIG_SYS_IVM_EEPROM_MAX_LEN);
>> + return 0;
>> +}
>> +#endif
>
> Is hush disabled in any case? If not, remove the #ifdef please.
>
>> +
>> +#if defined(CONFIG_LAST_STAGE_INIT)
>
> Again, is CONFIG_LAST_STAGE_INIT ever disabled?
Good point, thanks. I removed the #ifdefs for CONFIG_LAST_STAGE_INIT,
CONFIG_LAST_STAGE_INIT, CONFIG_SYS_DPAA_FMAN and CONFIG_POST.
<snip>
>> diff --git a/board/keymile/kmcent2/tlb.c b/board/keymile/kmcent2/tlb.c
>> new file mode 100644
>> index 0000000000..8a726fbd17
>> --- /dev/null
>> +++ b/board/keymile/kmcent2/tlb.c
>> @@ -0,0 +1,120 @@
<snip>
>> +#if defined(CONFIG_SYS_MRAM_BASE)
>> + SET_TLB_ENTRY(1, CONFIG_SYS_MRAM_BASE, SYS_MRAM_BASE_PHYS,
>> + MAS3_SW | MAS3_SR, MAS2_I | MAS2_G,
>> + 0, 12, BOOKE_PAGESZ_128M, 1),
>> +#endif
>> + /* BFTIC */
>> +#if defined(SYS_BFTIC_BASE)
>
> Really? "SYS_BFTIC_BASE" or "CONFIG_SYS_BFTIC_BASE" ?
Well spotted! It turns out SYS_BFTIC_BASE, as CONFIG_SYS_BFTIC_BASE is
not in config_whitelist.txt, and renaming it was the compromise. The
code originally predates the disallowing of ad-hoc CONFIGs... I hope
this is okay?
> And here as well, if some of these defines are always set or never,
> please remove the #ifdef's or the complete block if never set.
Thanks. Applied this to tlb.c and law.c.
>> + SET_TLB_ENTRY(1, SYS_BFTIC_BASE, SYS_BFTIC_BASE_PHYS,
>> + MAS3_SW | MAS3_SR, MAS2_I | MAS2_G,
>> + 0, 13, BOOKE_PAGESZ_128M, 1),
>> +#endif
<snip>
>> diff --git a/include/configs/kmcent2.h b/include/configs/kmcent2.h
>> new file mode 100644
>> index 0000000000..a0f3a09057
>> --- /dev/null
>> +++ b/include/configs/kmcent2.h
>> @@ -0,0 +1,537 @@
>> +/* SPDX-License-Identifier: GPL-2.0+ */
>> +/*
>> + * (C) Copyright 2016 Keymile AG
>> + * Rainer Boschung <rainer.boschung at keymile.com>
>> + *
>> + */
>> +
>> +#ifndef __KMCENT2_H
>> +#define __KMCENT2_H
>> +
>> +#define CONFIG_HOSTNAME "kmcent2"
>> +#define KM_BOARD_NAME CONFIG_HOSTNAME
>> +
>> +#define CONFIG_KM_UBI_PARTITION_NAME_BOOT "ubi0"
>> +#define CONFIG_KM_UBI_PARTITION_NAME_APP "ubi1"
>> +#define MTDIDS_DEFAULT "nor0=fe8000000.nor,nand0=ffa000000.flash"
>> +
>> +#define MTDPARTS_DEFAULT "mtdparts=" \
>> + "fe8000000.nor:" \
>> + "128k(RCW)," \
>> + "128k(fman)," \
>> + "128k(QE)," \
>> + "128k(zarlink)," \
>> + "512k(res)," \
>> + "62m(" CONFIG_KM_UBI_PARTITION_NAME_BOOT ")," \
>> + "128k(envred)," \
>> + "128k(env)," \
>> + "768k(u-boot);" \
>> + "ffa000000.flash:" \
>> + "-(" CONFIG_KM_UBI_PARTITION_NAME_APP ");"
>
> These MTD defines have been moved to Kconfig. Please don't add them to
> the board config header. And please also check for other macros in the
> config header, if they are defined in Kconfig and should be configured
> via "make menuconfig" instead.
>
Okay, I will add it there. The only reason it was kept here was its
length and the two defines embedded inside it; MTDIDS_DEFAULT was kept
because the defines go together.
<snip>
>> +/*
>> + * Serial Port - controlled on board with jumper J8
>> + * open - index 2
>> + * shorted - index 1
>> + * Retain non-DM serial port for debug purposes.
>> + */
>> +#if !defined(CONFIG_DM_SERIAL)
>> +#define CONFIG_CONS_INDEX 1
>> +#define CONFIG_SYS_NS16550_SERIAL
>> +#define CONFIG_SYS_NS16550_REG_SIZE 1
>> +#define CONFIG_SYS_NS16550_CLK (get_bus_freq(0) / 2)
>> +#define CONFIG_SYS_NS16550_COM1 (CONFIG_SYS_CCSRBAR + 0x11C500)
>> +#endif
>
> Is CONFIG_DM_SERIAL set? Always? If yes, remove the #ifdef please.
I have to admit that I found being able to disable CONFIG_DM_SERIAL
incredibly useful while debugging the DT and DM (especially with
CONFIG_DM_DEBUG enabled), as DM_SERIAL only starts up during that
process. I would appreciate if we could keep this one...
<snip>
> As mentioned above, please double-check if some of the macros are
> available in Kconfig and move them over to the config/foo.h file
> instead if available in Kconfig.
With the exceptions of MTDPARTS_DEFAULT and MTDIDS_DEFAULT, I originally
did go through each of these defines, and moved everything that could
into the defconfig already. Everything else would require editing the
Kconfigs themselves. Some macros above do exist in the Kconfig, but are
tied to different platforms, etc. This is unfortunately one of the snags
of this platform.
Thank you for the review!
Best regards,
Niel Fourie
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-21 Fax: +49-8142-66989-80 Email: lusus at denx.de
More information about the U-Boot
mailing list