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

[Soeren Moch]

Hi Stefano!

On 10/22/14 10:36, Stefano Babic wrote:
> 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.

U-Boot without video support is useless for me. So I will try to
setup the video clock correctly in a new version of this patch.

>>
>>>> +#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.
>

You are right here, there is no problem with setup_display() in board_init().

So I wonder why all imx6q boards have setup_display() in board_early_init(),
you even committed such code for mx6qsabreauto yesterday.

Best regards,
Soeren Moch