Re: [PATCH V2 2/2] ARM: CCMX51: Switch to multiimage support

[Oleksij Rempel <o.rempel@pengutronix.de>]

Hi,

On 17.12.18 11:10, Sascha Hauer wrote:
> On Mon, Dec 17, 2018 at 08:00:16AM +0100, Oleksij Rempel wrote:
>> Hi Alexander,
>>
>> please, use pr_info, or better dev_info() if you wont to print some
>> thing from the driver.
>>
>>>   }
>>
>> Hm, I assume, PMIC code should be moved to drivers/regulator/..
> 
> We do not have any code to configure PMICs to sane defaults or to
> defaults from the devicetree. I would surely appreciate if we would
> get that code, but I am perfectly fine to put it into board code until
> we are there.
> 
>>
>>> +static int ccxmx51_board_fixup(struct device_node *root, void *unused)
>>> +{
>>> +	char *serial;
>>> +
>>> +	if (!ccxmx_id->accel)
>>> +		ccxmx51_disable_device(root, "mma7455l@1d");
>>> +
>>> +	if (!ccxmx_id->eth0)
>>> +		ccxmx51_disable_device(root, "ethernet@83fec000");
>>> +
>>> +	if (!ccxmx_id->eth1)
>>> +		ccxmx51_disable_device(root, "lan9221@5,0");
>>> +
>>> +	if (!ccxmx_id->wless)
>>> +		ccxmx51_disable_device(root, "esdhc@70008000");
>>> +
>>> +	serial = basprintf("%08x%08x", 0, boardserial);
>>> +	of_set_property(root, "serial-number", serial, strlen(serial) + 1, 1);
>>> +	free(serial);
>>
>> should it be done by devicetree?
> 
> How should runtime specific patching of the devicetree be done by the
> devicetree?

Do you mean, this board specific information is not described by devicetree?

> 
>>> +	imx51_bbu_internal_mmc_register_handler("mmc", "/dev/mmc0",
>>> +						BBU_HANDLER_FLAG_DEFAULT);
>>
>> this can be done in separate patch.
> 
> Could be done, I am fine with leaving it in this patch.
> 
>>
>>> -	imx51_add_uart0();
>>> +	if (IS_ENABLED(CONFIG_DEFAULT_ENVIRONMENT))
>>> +		defaultenv_append_directory(defaultenv_ccxmx51);
>>
>> do we really need extra defaultenv_append_directory() for this board?
> 
>>> -late_initcall(ccxmx51js_init);
>>> diff --git a/arch/arm/boards/ccxmx51/env/boot/nand b/arch/arm/boards/ccxmx51/defaultenv-ccxmx51/boot/nand
>>> similarity index 100%
>>> rename from arch/arm/boards/ccxmx51/env/boot/nand
>>> rename to arch/arm/boards/ccxmx51/defaultenv-ccxmx51/boot/nand
>>> diff --git a/arch/arm/boards/ccxmx51/env/nv/autoboot_timeout b/arch/arm/boards/ccxmx51/defaultenv-ccxmx51/nv/autoboot_timeout
>>> similarity index 100%
>>> rename from arch/arm/boards/ccxmx51/env/nv/autoboot_timeout
>>> rename to arch/arm/boards/ccxmx51/defaultenv-ccxmx51/nv/autoboot_timeout
>>> diff --git a/arch/arm/boards/ccxmx51/env/nv/boot.default b/arch/arm/boards/ccxmx51/defaultenv-ccxmx51/nv/boot.default
>>> similarity index 100%
>>> rename from arch/arm/boards/ccxmx51/env/nv/boot.default
>>> rename to arch/arm/boards/ccxmx51/defaultenv-ccxmx51/nv/boot.default
>>> diff --git a/arch/arm/boards/ccxmx51/defaultenv-ccxmx51/nv/linux.bootargs.base b/arch/arm/boards/ccxmx51/defaultenv-ccxmx51/nv/linux.bootargs.base
>>
>> I would really prefer to remove board specific env if possible, it is
>> hard to keep it in sync with real default env.
> 
> For 'autoboot_timeout' I agree since there is no real point in putting
> personal preference in there. 2s vs. 3s doesn't really justify putting
> board specifics in. 'linux.bootargs.base' contains 'earlyprintk' which
> should really be in a default environment as this can make a kernel non
> bootable.
> 
> For 'nand' and 'boot.default' I would say that this is exactly the way
> how a different default boot should be done, so I am fine with leaving
> it in.
> 
>>> @@ -41,7 +42,6 @@ CONFIG_MACH_PHYTEC_PHYCORE_IMX7=y
>>>   CONFIG_MACH_FREESCALE_MX7_SABRESD=y
>>>   CONFIG_MACH_NXP_IMX6ULL_EVK=y
>>>   CONFIG_MACH_GRINN_LITEBOARD=y
>>> -CONFIG_IMX_IIM=y
>>
>> Why this was silently changed by this patch?
> 
> It's not silently changed, this goes down to a make savedefconfig which
> removes all options which are set to default (or are selected by other
> options).

Well, for me it looks cryptic. The patch is "CCMX51: Switch to 
multiimage support", at same time it modifies generic imx config in not 
obvious way.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox