[U-Boot] [PATCH V2 01/14] mxs: add parameter base_addr for mxs_set_lcdclk

[Peng Fan]

Hi Stefano,

Sorry for this late reply.

On Tue, Oct 20, 2015 at 03:05:05PM +0200, Stefano Babic wrote:
>Hi Peng,
>
>
>On 20/10/2015 13:39, Peng Fan wrote:
>> Change mxs_set_lcdclk prototype to add a new parameter
>> base_addr. There are two LCD interfaces for i.MX6SX,
>> we may support LCDIF1 or LCDIF2.
>> 
>
>ok - from your commit message I am waiting that both LCDIF1 and LCDIF2
>can be supported, and a user (board maintainer) can switch between them.
>And in later patch mxs_set_lcdclk() is avalilable for mx6sx and mx6ul,
>that makes this inconsistent.

I can not get you about 'inconsistent'.

The reason to add LCDIF1 and LCDIF2
support is that the board design may use LCDIF1 or LCDIF2. The mxs_set_lcdlck
should support setting clock for LCDIF1 and LCDIF2.

There two LCDIFs for i.MX6SX and i.MX7D.

>
>But I do not understand how the support for the second LCDIF is
>implemented. I should see that the register set points to LCDIF1 or LCDIF2.
>
>More as extending to both LCDIF1 and LCDIF2, it looks to me that this is
>consistent with mxs_set_lcdclk() in mx7.

The current mxs_set_lcdclk for mx7 is same with freescale vendor uboot, but
its prototype is not consistent with the current upstream uboot.
Since its prototype is same with my idea in this patchset, I do not need to
change it.
>
>
>> Signed-off-by: Peng Fan <Peng.Fan at freescale.com>
>> Cc: Stefano Babic <sbabic at denx.de>
>> Cc: Anatolij Gustschin <agust at denx.de>
>> ---
>> 
>> V2:
>>  none
>> 
>>  arch/arm/cpu/arm926ejs/mxs/clock.c    | 2 +-
>>  arch/arm/include/asm/arch-mxs/clock.h | 2 +-
>>  drivers/video/mxsfb.c                 | 2 +-
>>  3 files changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/arch/arm/cpu/arm926ejs/mxs/clock.c b/arch/arm/cpu/arm926ejs/mxs/clock.c
>> index e9d8800..dac8dc6 100644
>> --- a/arch/arm/cpu/arm926ejs/mxs/clock.c
>> +++ b/arch/arm/cpu/arm926ejs/mxs/clock.c
>> @@ -309,7 +309,7 @@ void mxs_set_ssp_busclock(unsigned int bus, uint32_t freq)
>>  		bus, tgtclk, freq);
>>  }
>>  
>> -void mxs_set_lcdclk(uint32_t freq)
>> +void mxs_set_lcdclk(uint32_t base_addr, uint32_t freq)
>>  {
>>  	struct mxs_clkctrl_regs *clkctrl_regs =
>>  		(struct mxs_clkctrl_regs *)MXS_CLKCTRL_BASE;
>> diff --git a/arch/arm/include/asm/arch-mxs/clock.h b/arch/arm/include/asm/arch-mxs/clock.h
>
>This patch should generate warnings, because base_addr is not used. You
>should at least add an  __maybe_unused attribute.

Ok. will add this attribute.

>
>Anyway, I am asking myself if it makes sense to pass an address. Really,
>you want to pass an index if you want to support LCDIF1 or LCDIF2. This
>makes more sense as passing an address. In you code later (patch 7/14),
>you check the address like an index:
>
>
>	if (base_addr == LCDIF1_BASE_ADDR) {
>
>This makes inconsistent. If you need it as address, this means you will
>use it as address later, casting to a register structure. If you use as
>I understand as index, that please use an enumeration for it.

I'll switch to use index.
mxs_set_clk(u32 index, u32 freq), and in mxs_set_clk, choose coresponding
base address according to index.

Regards,
Peng.

>
>> index fc9d75b..8840335 100644
>> --- a/arch/arm/include/asm/arch-mxs/clock.h
>> +++ b/arch/arm/include/asm/arch-mxs/clock.h
>> @@ -46,7 +46,7 @@ uint32_t mxc_get_clock(enum mxc_clock clk);
>>  void mxs_set_ioclk(enum mxs_ioclock io, uint32_t freq);
>>  void mxs_set_sspclk(enum mxs_sspclock ssp, uint32_t freq, int xtal);
>>  void mxs_set_ssp_busclock(unsigned int bus, uint32_t freq);
>> -void mxs_set_lcdclk(uint32_t freq);
>> +void mxs_set_lcdclk(uint32_t base_addr, uint32_t freq);
>>  
>>  /* Compatibility with the FEC Ethernet driver */
>>  #define	imx_get_fecclk()	mxc_get_clock(MXC_AHB_CLK)
>> diff --git a/drivers/video/mxsfb.c b/drivers/video/mxsfb.c
>> index 03b0f88..eed57d5 100644
>> --- a/drivers/video/mxsfb.c
>> +++ b/drivers/video/mxsfb.c
>> @@ -55,7 +55,7 @@ static void mxs_lcd_init(GraphicDevice *panel,
>>  	uint8_t valid_data = 0;
>>  
>>  	/* Kick in the LCDIF clock */
>> -	mxs_set_lcdclk(PS2KHZ(mode->pixclock));
>> +	mxs_set_lcdclk(MXS_LCDIF_BASE, PS2KHZ(mode->pixclock));
>>  
>>  	/* Restart the LCDIF block */
>>  	mxs_reset_block(&regs->hw_lcdif_ctrl_reg);
>> 
>
>Best regards,
>Stefano Babic
>
>-- 
>=====================================================================
>DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>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
>=====================================================================

--