[U-Boot] [PATCH 1/2] arm: mx6: add support for TBS2910 Matrix ARM miniPC

[Stefano Babic]

Hi Soeren,

On 21/10/2014 19:54, Soeren Moch wrote:
>>
>> This is also copied from sabresd. Can we factorize in some way ?
> 
> I can, and probably should, simplify this code. But in fact this code
> is wrong, and in the same way for many imx6q boards (e.g. sabresd,
> wandboard, nitrogen6x,...).

That makes things only worse. Duplicating the code, we have an
additional board with bad / wrong code.

> 
> This code configures the external video clock (LDB_DIx clock). This
> clock is hardcoded to have 65MHz in drivers/video/ipu_common.c:ldb_clk.
> But in fact the clock rate is configured to 75.42MHz (528MHz/7) on all
> boards. So the display does not show 1024x768 at 60Hz as configured, but
> something similar to 1024x768 at 70Hz (not VESA compliant), which much
> monitors can handle, but on other monitors there are problems.
> (one out of tree monitors works for me).

Ok, understood.

> 
> My intention was to get this initial support for tbs2910 merged with the
> (wrong) code from sabresd used as template, and later to discuss how to
> cleanup this code.
> 
> Do you prefer a simplified version of this code for the initial patch?

Maybe this is the best approach: you can at the beginning drop support
for video, an let your board be merged without wrong code. Then we can
discuss about fixing the wrong clock as shared code, letting that all
boards take advantage for that.

> 
>>> +#endif /* CONFIG_VIDEO_IPUV3 */
>>> +
>>> +int board_eth_init(bd_t *bis)
>>> +{
>>> +    setup_iomux_enet();
>>> +    setup_pcie();
>>> +
>>> +    return cpu_eth_init(bis);
>>> +}
>>> +
>>> +int board_early_init_f(void)
>>> +{
>>> +    setup_iomux_uart();
>>> +#ifdef CONFIG_VIDEO_IPUV3
>>> +    setup_display();
>>
>> I do not understand why setup_display() should be called at this point.
>> Generally, board_early_init_f() is called to setup iomux for peripherals
>> needed before relocation, as uart, letting the rest of the setup in
>> board_init(). Why do you need here ?
> 
> In fact this was also copied from sabresd/wandboard/nitrogen6x.
> My assumption was, that the clocks must be configured before the
> ipu is initialized.

That is correct, but ipu is initialized by video_init() after
board_init() is called. Generally, board_early_init() is responsible for
setup some initial peripherals, for example the iomux for uart or for
RAM controller. The common initialization is then put into board_init().
I am expecting that you have no issues by moving setup_display() in the
board_init() function.

> 
>>> diff --git a/configs/tbs2910_defconfig b/configs/tbs2910_defconfig
>>> new file mode 100644
>>> index 0000000..602d691
>>> --- /dev/null
>>> +++ b/configs/tbs2910_defconfig
>>> @@ -0,0 +1,3 @@
>>> +CONFIG_SYS_EXTRA_OPTIONS="IMX_CONFIG=board/boundary/nitrogen6x/nitrogen6q2g.cfg,MX6Q"
>>>
>>
>> Be aware that changes in the ddr-setup in that file will have a
>> consequence on your board.
> 
> Yes, I'm aware of that. But nitrogen6x is very well maintained. So I see
> no need to
> duplicate this config and reuse it in the same way as wandboard does.

ok, that is enough, then it is fine with me.

Best regards,
Stefano Babic


-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================