[U-Boot] [PATCH 1/2] mx6: Factor out common HDMI setup code

[Eric Nelson <eric.nelson@boundarydevices.com>]

Hi Pardeep,

On 07/12/2013 03:21 PM, Pardeep Kumar Singla wrote:
> Instead of duplicating HDMI setup code for every mx6 board, factor out the common code
>
> Signed-off-by: Pardeep Kumar Singla <b45784@freescale.com>
> ---
>   arch/arm/cpu/armv7/mx6/soc.c                  |   51 ++++++++++++++
>   arch/arm/include/asm/arch-mx6/mxc_hdmi.h      |    6 ++
>   arch/arm/include/asm/arch-mx6/sys_proto.h     |   13 ++++
>   board/boundary/nitrogen6x/nitrogen6x.c        |   57 +---------------
>   board/freescale/mx6qsabrelite/mx6qsabrelite.c |   60 ++--------------
>   board/wandboard/wandboard.c                   |   91 ++++++-------------------
>   include/configs/mx6qsabrelite.h               |    8 ++-
>   include/configs/nitrogen6x.h                  |    2 +-
>   include/configs/wandboard.h                   |    1 +
>   9 files changed, 106 insertions(+), 183 deletions(-)
>
> diff --git a/arch/arm/cpu/armv7/mx6/soc.c b/arch/arm/cpu/armv7/mx6/soc.c
> index fc436fb..6e79310 100644
> --- a/arch/arm/cpu/armv7/mx6/soc.c
> +++ b/arch/arm/cpu/armv7/mx6/soc.c
> @@ -32,6 +32,8 @@
>   #include <asm/imx-common/boot_mode.h>
>   #include <asm/imx-common/dma.h>
>   #include <stdbool.h>
> +#include <asm/arch/mxc_hdmi.h>
> +#include <asm/arch/crm_regs.h>
>
>   struct scu_regs {
>   	u32	ctrl;
> @@ -228,3 +230,52 @@ const struct boot_mode soc_boot_modes[] = {
>   void s_init(void)
>   {
>   }
> +
> +#ifdef CONFIG_IMX_HDMI
> +void imx_enable_hdmi_phy(struct display_info_t const *dev)
> +{

The only reason this has a dev parameter is so it can be used by
auto-detect code.

It should be kept out of this code to make the interface clearer.

Boards that support auto-detect (mx6qsabrelite/nitrogen6x)
can wrap it.

> +	struct hdmi_regs *hdmi  = (struct hdmi_regs *)HDMI_ARB_BASE_ADDR;
> +	u8 reg;
> +	reg = readb(&hdmi->phy_conf0);
> +	reg |= HDMI_PHY_CONF0_PDZ_MASK;
> +	writeb(reg, &hdmi->phy_conf0);
> +	udelay(3000);
> +	reg |= HDMI_PHY_CONF0_ENTMDS_MASK;
> +	writeb(reg, &hdmi->phy_conf0);
> +	udelay(3000);
> +	reg |= HDMI_PHY_CONF0_GEN2_TXPWRON_MASK;
> +	writeb(reg, &hdmi->phy_conf0);
> +	writeb(HDMI_MC_PHYRSTZ_ASSERT, &hdmi->mc_phyrstz);
> +}
> +
> +void imx_setup_hdmi(void)
> +{
> +	struct mxc_ccm_reg *mxc_ccm = (struct mxc_ccm_reg *)CCM_BASE_ADDR;
> +	struct hdmi_regs *hdmi  = (struct hdmi_regs *)HDMI_ARB_BASE_ADDR;
> +	int reg;
> +
> +	/* Turn on IPU clock */
> +	reg = readl(&mxc_ccm->CCGR3);
> +	reg |= MXC_CCM_CCGR3_IPU1_IPU_DI0_OFFSET;
> +	writel(reg, &mxc_ccm->CCGR3);
> +
> +	/* Turn on HDMI PHY clock */
> +	reg = readl(&mxc_ccm->CCGR2);
> +	reg |=  MXC_CCM_CCGR2_HDMI_TX_IAHBCLK_MASK|
> +		 MXC_CCM_CCGR2_HDMI_TX_ISFRCLK_MASK;
> +	writel(reg, &mxc_ccm->CCGR2);
> +	 /* clear HDMI PHY reset */
> +	 writeb(HDMI_MC_PHYRSTZ_DEASSERT, &hdmi->mc_phyrstz);


There's a lot of policy in this register write and some of it isn't
really HDMI-specific (LDB clock selects). I'd recommend keeping
that in board-specific code:

> +	reg = readl(&mxc_ccm->chsccdr);
> +	reg &= ~(MXC_CCM_CHSCCDR_IPU1_DI0_PRE_CLK_SEL_MASK|
> +		 MXC_CCM_CHSCCDR_IPU1_DI0_PODF_MASK|
> +		 MXC_CCM_CHSCCDR_IPU1_DI0_CLK_SEL_MASK);
> +	reg |= (CHSCCDR_CLK_SEL_LDB_DI0
> +		 << MXC_CCM_CHSCCDR_IPU1_DI0_CLK_SEL_OFFSET)|
> +		 (CHSCCDR_PODF_DIVIDE_BY_3
> +		 << MXC_CCM_CHSCCDR_IPU1_DI0_PODF_OFFSET)
> +		 |(CHSCCDR_IPU_PRE_CLK_540M_PFD
> +		 << MXC_CCM_CHSCCDR_IPU1_DI0_PRE_CLK_SEL_OFFSET);
> +	writel(reg, &mxc_ccm->chsccdr);
> +}
> +#endif
> diff --git a/arch/arm/include/asm/arch-mx6/mxc_hdmi.h b/arch/arm/include/asm/arch-mx6/mxc_hdmi.h
> index 9dccb3f..9e2074b 100644
> --- a/arch/arm/include/asm/arch-mx6/mxc_hdmi.h
> +++ b/arch/arm/include/asm/arch-mx6/mxc_hdmi.h
> @@ -21,6 +21,12 @@
>   #ifndef __MXC_HDMI_H__
>   #define __MXC_HDMI_H__
>
> +#ifdef CONFIG_IMX_HDMI
> +#include<asm/arch/sys_proto.h>
> +void imx_enable_hdmi_phy(struct display_info_t const *dev);
> +void imx_setup_hdmi(void);
> +#endif
> +
>   /*
>    * Hdmi controller registers
>    */
> diff --git a/arch/arm/include/asm/arch-mx6/sys_proto.h b/arch/arm/include/asm/arch-mx6/sys_proto.h
> index 38e4e51..9fb539b 100644
> --- a/arch/arm/include/asm/arch-mx6/sys_proto.h
> +++ b/arch/arm/include/asm/arch-mx6/sys_proto.h
> @@ -25,6 +25,9 @@
>   #define _SYS_PROTO_H_
>
>   #include <asm/imx-common/regs-common.h>
> +#include<linux/list.h>
> +#include <linux/fb.h>
> +#include <i2c.h>
>
>   #define MXC_CPU_MX51		0x51
>   #define MXC_CPU_MX53		0x53
> @@ -34,6 +37,16 @@
>   #define MXC_CPU_MX6Q		0x63
>
>   #define is_soc_rev(rev)	((get_cpu_rev() & 0xFF) - rev)

This is specific to a couple of boards, and not mx6-specific:

> +
> +struct display_info_t {
> +	int        bus;
> +	int        addr;
> +	int        pixfmt;
> +	int        (*detect)(struct display_info_t const *dev);
> +	void        (*enable)(struct display_info_t const *dev);
> +	struct        fb_videomode mode;
> +};
> +

>   u32 get_cpu_rev(void);
>   const char *get_imx_type(u32 imxtype);
>   unsigned imx_ddr_size(void);
> diff --git a/board/boundary/nitrogen6x/nitrogen6x.c b/board/boundary/nitrogen6x/nitrogen6x.c
> index 8f0f9b8..d97d47a 100644
> --- a/board/boundary/nitrogen6x/nitrogen6x.c
> +++ b/board/boundary/nitrogen6x/nitrogen6x.c
> @@ -464,40 +464,12 @@ static iomux_v3_cfg_t const rgb_pads[] = {
>   	MX6_PAD_DISP0_DAT23__IPU1_DISP0_DAT_23,
>   };
>
>  <snip>
>
> -static void enable_hdmi(struct display_info_t const *dev)
> -{

static void do_enable_hdmi(struct display_info_t const *dev)
{
	return enable_hdmi();
}

Regards,


Eric