[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