[U-Boot] [PATCH v3 1/6] powerpc/ppc4xx: Use generic accessor functions for gdsys FPGA

[Wolfgang Denk]

Dear dirk.eibach at gdsys.cc,

In message <1370959364-28943-2-git-send-email-dirk.eibach at gdsys.cc> you wrote:
> From: Dirk Eibach <eibach at gdsys.de>
> 
> A set of accessor functions was added to be able to access not only
> memory mapped FPGA in a generic way.
> 
> Thanks to Wolfgang Denk for getting this sorted properly.

Sorry, I still have comments...


> +#ifdef CONFIG_SYS_FPGA_NO_RFL_HI
> +			FPGA_GET_REG(k, reflection_low, &val);
> +#else
> +			FPGA_GET_REG(k, reflection_high, &val);
> +#endif

Can this #ifdef here (and similar below) not be avoided by defining a
proper value to some macro and using this as function argument instead?

> diff --git a/board/gdsys/405ep/dlvision-10g.c b/board/gdsys/405ep/dlvision-10g.c
> index 644493b..332d82f 100644
> --- a/board/gdsys/405ep/dlvision-10g.c
> +++ b/board/gdsys/405ep/dlvision-10g.c
...
> +struct ihs_fpga *fpga_ptr[] = CONFIG_SYS_FPGA_PTR;
> +
> +int fpga_set_reg(u32 fpga, u16 *reg, off_t regoff, u16 data)
> +{
> +	out_le16(reg, data);
> +
> +	return 0;
> +}
> +
> +int fpga_get_reg(u32 fpga, u16 *reg, off_t regoff, u16 *data)
> +{
> +	*data = in_le16(reg);
> +
> +	return 0;
> +}
...
> diff --git a/board/gdsys/405ep/io.c b/board/gdsys/405ep/io.c
> index 070dcbb..05707c4 100644
> --- a/board/gdsys/405ep/io.c
> +++ b/board/gdsys/405ep/io.c
> @@ -53,6 +53,22 @@ enum {
>  	HWVER_122 = 3,
>  };
>  
> +struct ihs_fpga *fpga_ptr[] = CONFIG_SYS_FPGA_PTR;
> +
> +int fpga_set_reg(u32 fpga, u16 *reg, off_t regoff, u16 data)
> +{
> +	out_le16(reg, data);
> +
> +	return 0;
> +}
> +
> +int fpga_get_reg(u32 fpga, u16 *reg, off_t regoff, u16 *data)
> +{
> +	*data = in_le16(reg);
> +
> +	return 0;
> +}
...
> diff --git a/board/gdsys/405ep/iocon.c b/board/gdsys/405ep/iocon.c
> index 0fc7db2..a531e5d 100644
> --- a/board/gdsys/405ep/iocon.c
> +++ b/board/gdsys/405ep/iocon.c
> @@ -69,6 +69,22 @@ enum {
>  	RAM_DDR2_32 = 0,
>  };
>  
> +struct ihs_fpga *fpga_ptr[] = CONFIG_SYS_FPGA_PTR;
> +
> +int fpga_set_reg(u32 fpga, u16 *reg, off_t regoff, u16 data)
> +{
> +	out_le16(reg, data);
> +
> +	return 0;
> +}
> +
> +int fpga_get_reg(u32 fpga, u16 *reg, off_t regoff, u16 *data)
> +{
> +	*data = in_le16(reg);
> +
> +	return 0;
> +}

We have the very same code repeated 3 times aready here, and two more
times follow further down.  Please factor out this common code so we
have a single copy only.

Also, this does not handle the non-memory-mapped case (for fpga == 0) ?

> @@ -230,17 +249,21 @@ int last_stage_init(void)
>   */
>  void fpga_gpio_set(int pin)
>  {
> -	out_le16((void *)(CONFIG_SYS_FPGA0_BASE + 0x18), pin);
> +	FPGA_SET_REG(0, gpio.set, pin);
>  }
>  
>  void fpga_gpio_clear(int pin)
>  {
> -	out_le16((void *)(CONFIG_SYS_FPGA0_BASE + 0x16), pin);
> +	FPGA_SET_REG(0, gpio.clear, pin);
>  }
>  
>  int fpga_gpio_get(int pin)
>  {
> -	return in_le16((void *)(CONFIG_SYS_FPGA0_BASE + 0x14)) & pin;
> +	u16 val;
> +
> +	FPGA_GET_REG(0, gpio.read, &val);
> +
> +	return val & pin;
>  }

Do you really have to keep these fpga_gpio_set() / fpga_gpio_clear() /
fpga_gpio_get() functions?  Can you not fix the related code to use
FPGA_GET_REG() directly?


> @@ -282,7 +288,7 @@ static int osd_print(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  {
>  	unsigned screen;
>  
> -	for (screen = 0; screen < CONFIG_SYS_OSD_SCREENS; ++screen) {
> +	for (screen = 0; screen <= max_osd_screen; ++screen) {

This looks like an unrelated change.  Is it intentionally included
here?

...
> -	out_le16(&osd->xy_size, ((32 - 1) << 8) | (16 - 1));
> -	out_le16(&osd->x_pos, 0x007f);
> -	out_le16(&osd->y_pos, 0x005f);
> +	if (screen > max_osd_screen)
> +		max_osd_screen = screen;

Ditto here?

> @@ -386,7 +396,7 @@ int osd_write(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  {
>  	unsigned screen;
>  
> -	for (screen = 0; screen < CONFIG_SYS_OSD_SCREENS; ++screen) {
> +	for (screen = 0; screen <= max_osd_screen; ++screen) {

And here?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Do not simplify the design of a program if a way can be found to make
it complex and wonderful.