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

[Soeren Moch]

Hi Stefano,

Thanks for your review.

>> +static int mx6_rgmii_rework(struct phy_device *phydev)
>> +{
>> +	unsigned short val;
>> +
>> +	/* To enable AR8035 ouput a 125MHz clk from CLK_25M */
>> +	phy_write(phydev, MDIO_DEVAD_NONE, 0xd, 0x7);
>> +	phy_write(phydev, MDIO_DEVAD_NONE, 0xe, 0x8016);
>> +	phy_write(phydev, MDIO_DEVAD_NONE, 0xd, 0x4007);
>> +
>> +	val = phy_read(phydev, MDIO_DEVAD_NONE, 0xe);
>> +	val &= 0xffe3;
>> +	val |= 0x18;
>> +	phy_write(phydev, MDIO_DEVAD_NONE, 0xe, val);
>> +
>> +	/* introduce tx clock delay */
>> +	phy_write(phydev, MDIO_DEVAD_NONE, 0x1d, 0x5);
>> +	val = phy_read(phydev, MDIO_DEVAD_NONE, 0x1e);
>> +	val |= 0x0100;
>> +	phy_write(phydev, MDIO_DEVAD_NONE, 0x1e, val);
>> +
>> +	return 0;
>
> I miss the point - which is the difference with ar8035_config() in
> drivers/net/phy/atheros.c and why cnnot you reuse it ? Can we avoid to
> duplicate the code ?

OK, I will remove this code.

>> +static void setup_display(void)
>> +{
>> +	struct mxc_ccm_reg *mxc_ccm = (struct mxc_ccm_reg *)CCM_BASE_ADDR;
>> +	int reg;
>> +
>> +	enable_ipu_clock();
>> +	imx_setup_hdmi();
>> +
>> +	/* Turn on LDB0, LDB1, IPU,IPU DI0 clocks */
>> +	reg = readl(&mxc_ccm->CCGR3);
>> +	reg |=  MXC_CCM_CCGR3_LDB_DI0_MASK | MXC_CCM_CCGR3_LDB_DI1_MASK;
>> +	writel(reg, &mxc_ccm->CCGR3);
>> +
>> +	/* set LDB0, LDB1 clk select to 011/011 */
>> +	reg = readl(&mxc_ccm->cs2cdr);
>> +	reg &= ~(MXC_CCM_CS2CDR_LDB_DI0_CLK_SEL_MASK
>> +		 | MXC_CCM_CS2CDR_LDB_DI1_CLK_SEL_MASK);
>> +	reg |= (3 << MXC_CCM_CS2CDR_LDB_DI0_CLK_SEL_OFFSET)
>> +	      | (3 << MXC_CCM_CS2CDR_LDB_DI1_CLK_SEL_OFFSET);
>> +	writel(reg, &mxc_ccm->cs2cdr);
>> +
>> +	reg = readl(&mxc_ccm->cscmr2);
>> +	reg |= MXC_CCM_CSCMR2_LDB_DI0_IPU_DIV | MXC_CCM_CSCMR2_LDB_DI1_IPU_DIV;
>> +	writel(reg, &mxc_ccm->cscmr2);
>> +
>> +	reg = readl(&mxc_ccm->chsccdr);
>> +	reg |= (CHSCCDR_CLK_SEL_LDB_DI0
>> +		<< MXC_CCM_CHSCCDR_IPU1_DI0_CLK_SEL_OFFSET);
>> +	reg |= (CHSCCDR_CLK_SEL_LDB_DI0
>> +		<< MXC_CCM_CHSCCDR_IPU1_DI1_CLK_SEL_OFFSET);
>> +	writel(reg, &mxc_ccm->chsccdr);
>> +}
>
> 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,...).

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).

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?
Or should we aim for a fixed video clock in the first place?

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

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

Best regards,
Soeren Moch