Hi Mario, On 26 July 2017 at 02:24, Mario Six <mario....@gdsys.cc> wrote: > Hi Simon, > > On Wed, Jul 19, 2017 at 11:07 AM, Simon Glass <s...@chromium.org> wrote: >> Hi Mario, >> >> On 14 July 2017 at 05:55, Mario Six <mario....@gdsys.cc> wrote: >>> From: Dirk Eibach <dirk.eib...@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.eib...@gdsys.cc> >>> Signed-off-by: Mario Six <mario....@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot