[U-Boot] [PATCH 51/51] mpc83xx: Add gazerbeam board
Simon Glass
sjg at chromium.org
Fri Aug 4 09:34:06 UTC 2017
Hi Mario,
On 26 July 2017 at 02:24, Mario Six <mario.six at gdsys.cc> wrote:
> Hi Simon,
>
> On Wed, Jul 19, 2017 at 11:07 AM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Mario,
>>
>> On 14 July 2017 at 05:55, Mario Six <mario.six at gdsys.cc> wrote:
>>> From: Dirk Eibach <dirk.eibach at gdsys.cc>
>>>
>>> The gdsys gazerbeam board is based on a Freescale MPC8308 SOC.
>>> It boots from NOR-Flash, kernel and rootfs are stored on
>>> SD-Card.
>>>
>>> On board peripherals include:
>>> - 2x 10/100 Mbit/s Ethernet (optional)
>>>
>>> Signed-off-by: Dirk Eibach <dirk.eibach at gdsys.cc>
>>> Signed-off-by: Mario Six <mario.six at gdsys.cc>
>>> ---
>>>
>>> arch/powerpc/cpu/mpc83xx/Kconfig | 3 +
>>> arch/powerpc/dts/.gitignore | 1 +
>>> arch/powerpc/dts/Makefile | 15 +
>>> board/gdsys/common/Makefile | 1 +
>>> board/gdsys/common/ioep-fpga.c | 617 ++++++++++++++++++++++++++++++---------
>>> board/gdsys/mpc8308/Kconfig | 19 ++
>>> board/gdsys/mpc8308/MAINTAINERS | 2 +
>>> board/gdsys/mpc8308/Makefile | 1 +
>>> board/gdsys/mpc8308/gazerbeam.c | 332 +++++++++++++++++++++
>>> configs/gazerbeam_defconfig | 76 +++++
>>> include/configs/gazerbeam.h | 484 ++++++++++++++++++++++++++++++
>>> include/fdt_fixup.h | 5 +
>>> 12 files changed, 1416 insertions(+), 140 deletions(-)
>>> create mode 100644 arch/powerpc/dts/.gitignore
>>> create mode 100644 arch/powerpc/dts/Makefile
>>> create mode 100644 board/gdsys/mpc8308/gazerbeam.c
>>> create mode 100644 configs/gazerbeam_defconfig
>>> create mode 100644 include/configs/gazerbeam.h
[..]
>>> +
>>> + if (!data->mc4) {
>>> + if (!request_gpio_by_dev(&gpio, gpio_dev, 0, "var-mc_sc"))
>>
>> Can you have this GPIO in the device tree so you can use
>> gpio_request_by_name() from a driver? What is this GPIO for?
>>
>
> That's a bit of a problem. That GPIO determines whether the board is a
> single-channel variant or a multi-channel variant. So it doesn't really
> "belong" to any device at all; it's a property of the board itself.
>
> Introducing some kind of "variant driver" with a matching node in the DT would
> probably work, but I don't know if the concept is generic enough to warrant
> something like that (it would need another new uclass, probably).
>
> The nicest thing would probably be a "board driver" that describes properties
> of the board itself (this GPIO could then be a property of the board). This
> would also be a possible way to do away with board files completely: Every
> board that needs board-specific initialization implements a board driver with
> the current callback functions board_early_init_{r,f}, last_stage_init etc. as
> driver functions as needed. Those are called from board_{f,r}.c via
> board-uclass.c, and we don't need board files anymore. But, that's probably a
> bit too far fetched.
That makes a lot of sense to me.I did actually send a series with
something like this:
https://lists.denx.de/pipermail/u-boot/2017-March/284087.html
But it was more focussed on init, and I may revisit that another time.
For what you want I think a new uclass would be nice. The question is
what operations it should support. I suppose one would be something
like detect(struct udevice *) to figure out the board type. Perhaps
then it could provide a set of properties which can be queried? Each
could have an index and a name and the uclass could provide a way to
obtain the value of each property (perhaps support bool, int, string?)
Then for drivers which need to obtain a property, they can make an
appropriate uclass call.
Another option would be to mangle the device tree just before
relocation, changing compatible strings or adding/changing properties
for drivers, based on the board detectoin stuff. I suppose these two
things are not mutually exclusive.
[..]
>>> +
>>> + if (tpm_init() || tpm_startup(TPM_ST_CLEAR) ||
>>> + tpm_continue_self_test()) {
>>> + printf("TPM init failed\n");
>>> + }
>>> +
>>> + if (fpga_hw_rev >= 4) {
>>> + for (uclass_find_first_device(UCLASS_RXAUI_CTRL, &rxaui);
>>> + rxaui;
>>> + uclass_find_next_device(&rxaui)) {
>>> + rxaui_disable_polarity_inversion(rxaui);
>>> + }
>>> + }
>>> +
>>> + for (uclass_find_first_device(UCLASS_IHS_FPGA, &fpga);
>>> + fpga;
>>> + uclass_find_next_device(&fpga)) {
>>
>> Can you use uclass_first_next_device() since it does the probing for you.
>>
>
> OK, will fix in v2.
>
>>> + struct mii_dev *bus;
>>> +
>>> + device_probe(fpga);
>>> +
>>> + if (fpga->seq < 0)
>>> + continue;
>>
>> What is this checking for?
>>
>
> The phy initialization below should only run for the first FPGA, so we skip it
> for all others.
>
So perhaps just do uclass_get_device_by_seq(UCLASS_FPGA, 0)?
[..]
>>> +/*
>>> + * Hardware Reset Configuration Word
>>> + * if CLKIN is 66.66MHz, then
>>> + * CSB = 133MHz, DDRC = 266MHz, LBC = 133MHz
>>> + * We choose the A type silicon as default, so the core is 400Mhz.
>>> + */
>>> +#define CONFIG_SYS_HRCW_LOW (\
>>> + HRCWL_LCL_BUS_TO_SCB_CLK_1X1 |\
>>> + HRCWL_DDR_TO_SCB_CLK_2X1 |\
>>> + HRCWL_SVCOD_DIV_2 |\
>>> + HRCWL_CSB_TO_CLKIN_4X1 |\
>>> + HRCWL_CORE_TO_CSB_3X1)
>>
>> Seems like this should be in the DT instead of a CONFIG. TBD for later?
>>
>
> The HRCW (Hard Reset Configuration Word) has to be hard-coded in the image at a
> specific position, since the SoC reads it when it starts up, and configures
> itself according to it. So it could be moved to Kconfig, but not to DT, I'm
> afraid.
OK I see.
[..]
>>> +#define CONFIG_SYS_DDR_TIMING_1 ((2 << TIMING_CFG1_PRETOACT_SHIFT) \
>>> + | (6 << TIMING_CFG1_ACTTOPRE_SHIFT) \
>>> + | (2 << TIMING_CFG1_ACTTORW_SHIFT) \
>>> + | (7 << TIMING_CFG1_CASLAT_SHIFT) \
>>> + | (9 << TIMING_CFG1_REFREC_SHIFT) \
>>> + | (2 << TIMING_CFG1_WRREC_SHIFT) \
>>> + | (2 << TIMING_CFG1_ACTTOACT_SHIFT) \
>>> + | (2 << TIMING_CFG1_WRTORD_SHIFT))
>>> + /* 0x26279222 */
>>> +#define CONFIG_SYS_DDR_TIMING_2 ((0 << TIMING_CFG2_ADD_LAT_SHIFT) \
>>> + | (4 << TIMING_CFG2_CPO_SHIFT) \
>>> + | (3 << TIMING_CFG2_WR_LAT_DELAY_SHIFT) \
>>> + | (2 << TIMING_CFG2_RD_TO_PRE_SHIFT) \
>>> + | (2 << TIMING_CFG2_WR_DATA_DELAY_SHIFT) \
>>> + | (3 << TIMING_CFG2_CKE_PLS_SHIFT) \
>>> + | (5 << TIMING_CFG2_FOUR_ACT_SHIFT))
>>> + /* 0x021848c5 */
>>> +#define CONFIG_SYS_DDR_INTERVAL ((0x0824 << SDRAM_INTERVAL_REFINT_SHIFT) \
>>> + | (0x0100 << SDRAM_INTERVAL_BSTOPRE_SHIFT))
>>> + /* 0x08240100 */
>>> +#define CONFIG_SYS_DDR_SDRAM_CFG (SDRAM_CFG_SREN \
>>> + | SDRAM_CFG_SDRAM_TYPE_DDR2 \
>>> + | SDRAM_CFG_DBW_16)
>>> + /* 0x43100000 */
>>> +
>>> +#define CONFIG_SYS_DDR_SDRAM_CFG2 0x00401000 /* 1 posted refresh */
>>> +#define CONFIG_SYS_DDR_MODE ((0x0440 << SDRAM_MODE_ESD_SHIFT) \
>>> + | (0x0242 << SDRAM_MODE_SD_SHIFT))
>>> + /* ODT 150ohm CL=4, AL=0 on SDRAM */
>>> +#define CONFIG_SYS_DDR_MODE2 0x00000000
>>
>> I think this should all be a UCLASS_RAM driver with DT configuration.
>> Too many CONFIGs :-)
>>
>
> Yeah, it's not really nice, that's true. I'll look into it. I was planning on
> taking on MPC83xx DM conversion anyway, so I might as well combine it with the
> Gazerbeam series while I'm at it.
>
Sounds good
[..[
>>> +
>>> +#define CONFIG_SYS_FLASH_ERASE_TOUT 60000 /* Flash Erase Timeout (ms) */
>>> +#define CONFIG_SYS_FLASH_WRITE_TOUT 500 /* Flash Write Timeout (ms) */
>>> +
>>> +/* Window base at FPGA base */
>>> +#define CONFIG_SYS_LBLAWBAR1_PRELIM CONFIG_SYS_FPGA0_BASE
>>> +#define CONFIG_SYS_LBLAWAR1_PRELIM (LBLAWAR_EN | LBLAWAR_1MB)
>>> +#define CONFIG_SYS_LBLAWBAR2_PRELIM CONFIG_SYS_FPGA1_BASE
>>> +#define CONFIG_SYS_LBLAWAR2_PRELIM (LBLAWAR_EN | LBLAWAR_1MB)
>>> +
>>> +#define CONFIG_SYS_BR1_PRELIM (CONFIG_SYS_FPGA0_BASE \
>>> + | BR_PS_16 /* 16 bit port */ \
>>> + | BR_MS_GPCM /* MSEL = GPCM */ \
>>> + | BR_V) /* valid */
>>> +#define CONFIG_SYS_OR1_PRELIM (MEG_TO_AM(CONFIG_SYS_FPGA0_SIZE) \
>>> + | OR_UPM_XAM \
>>> + | OR_GPCM_CSNT \
>>> + | OR_GPCM_SCY_5 \
>>> + | OR_GPCM_TRLX_CLEAR \
>>> + | OR_GPCM_EHTR_CLEAR)
>>> +
>>> +#define CONFIG_SYS_BR2_PRELIM (CONFIG_SYS_FPGA1_BASE \
>>> + | BR_PS_16 /* 16 bit port */ \
>>> + | BR_MS_GPCM /* MSEL = GPCM */ \
>>> + | BR_V) /* valid */
>>> +#define CONFIG_SYS_OR2_PRELIM (MEG_TO_AM(CONFIG_SYS_FPGA1_SIZE) \
>>> + | OR_UPM_XAM \
>>> + | OR_GPCM_CSNT \
>>> + | OR_GPCM_SCY_5 \
>>> + | OR_GPCM_TRLX_CLEAR \
>>> + | OR_GPCM_EHTR_CLEAR)
>>
>> Put this config in the DT.
>>
>
> The recently resurrected MPC8xx has converted this to Kconfig; see 53193a4
> ("powerpc, 8xx: Add support for MCR3000 board from CSSI"). It's set in
> arch/powerpc/cpu/mpc8xxx/fsl_lbc.c, so I'm pretty sure that it could be moved
> to the DT as well. So, where should I put it? Rather in the DT, or in Kconfig
> option(s)? I would prefer DT myself, since we can retain at least some of the
> semantics that are embedded in the macro defines, whereas in Kconfig it's just
> the final value of the option, which has to be decoded manually to be
> understood.
>
I think CONFIG should be used for enabling features / options (mostly
at compile time) and DT should be used for configuring those features.
We have too many CONFIG options.
>>> +
>>> +#define CONFIG_CMDLINE_EDITING 1 /* add command line history */
>>
>> Drop '1' on the end of these.
>>
>
> OK. Another ancient copied/pasted thing, since it's in almost every other
> MPC83xx config as well.
Yes I see that. The convention changed at some point.
>
>>> @@ -1,4 +1,9 @@
>>> struct of_board_fixup_data {
>>> +#ifdef CONFIG_TARGET_GAZERBEAM
>>> + bool var_con;
>>> + bool mc4;
>>> + bool mc2;
>>> +#endif
>>> #ifdef CONFIG_TARGET_CONTROLCENTERDC
>>> bool chip_exists[6];
>>> #endif
>>> --
>>> 2.11.0
>>>
>>
>> Regards,
>> Simon
>
> Thank you very much for reviewing the series! It'll probably take a bit until
> v2 is ready (there's quite a bit to do).
Yes, a lot to do, but very important.
Regards,
Simon
More information about the U-Boot
mailing list