Dear Jean-Christophe PLAGNIOL-VILLARD,
In message <[email protected]> you wrote:
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <[email protected]>
...
> #ifdef CONFIG_SMC_USE_32_BIT
> -#define USE_32_BIT 1
> +#define is_use_32bit(x) (x->use_32bit)
Does switching from a compile time check to a run time check not cause
an avoidable growth of the memory footprint?
How much bigger is the new code?
> -#undef USE_32_BIT
> +#define is_use_32bit(x) (0)
> #endif
...
> -static inline word SMC_inw(dword offset)
> +static inline word SMC_io_inw(struct smc91111_device *smc, dword offset)
> {
> word v;
> - v = *((volatile word*)(SMC_BASE_ADDRESS+offset));
> + v = *((volatile word*)(smc->regs+offset));
> barrier(); *(volatile u32*)(0xc0000000);
> return v;
> }
>
> -static inline void SMC_outw(word value, dword offset)
> +static inline void SMC_io_outw(struct smc91111_device *smc, word value,
> + dword offset)
> {
> - *((volatile word*)(SMC_BASE_ADDRESS+offset)) = value;
> + *((volatile word*)(smc->regs+offset)) = value;
> barrier(); *(volatile u32*)(0xc0000000);
> }
Please use proper I/O accessor functions here.
> -static inline byte SMC_inb(dword offset)
> +static inline byte SMC_io_inb(struct smc91111_device *smc, dword offset)
> {
> word _w;
...and here.
> @@ -264,18 +234,22 @@ static inline byte SMC_inb(dword offset)
> return (offset & 1) ? (byte)(_w >> 8) : (byte)(_w);
> }
>
> -static inline void SMC_outb(byte value, dword offset)
> +static inline void SMC_io_outb(struct smc91111_device *smc, byte value,
> + dword offset)
> {
> word _w;
>
> _w = SMC_inw(offset & ~((dword)1));
> if (offset & 1)
> - *((volatile word*)(SMC_BASE_ADDRESS+(offset &
> ~((dword)1)))) = (value<<8) | (_w & 0x00ff);
> + *((volatile word*)(smc->regs+(offset & ~((dword)1)))) =
> + (value<<8) | (_w & 0x00ff);
> else
> - *((volatile word*)(SMC_BASE_ADDRESS+offset)) = value |
> (_w & 0xff00);
> + *((volatile word*)(smc->regs+offset)) =
> + value | (_w & 0xff00);
> }
...and here.
> -static inline void SMC_insw(dword offset, volatile uchar* buf, dword len)
> +static inline void SMC_io_insw(struct smc91111_device *smc, dword offset,
> + volatile uchar* buf, dword len)
> {
> volatile word *p = (volatile word *)buf;
...and here.
> @@ -286,7 +260,8 @@ static inline void SMC_insw(dword offset, volatile uchar*
> buf, dword len)
> }
> }
>
> -static inline void SMC_outsw(dword offset, uchar* buf, dword len)
> +static inline void SMC_io_outsw(struct smc91111_device *smc, dword offset,
> + uchar* buf, dword len)
> {
> volatile word *p = (volatile word *)buf;
...and here.
> -static int smc_close()
> +static void smc_halt(struct eth_device *netdev)
> {
> + struct smc91111_device *smc = to_smc91111(netdev);
> PRINTK2("%s: smc_close\n", SMC_DEV_NAME);
You should also adapt the debug messages to the changed function
names.
> +#ifdef CONFIG_SMC_USE_32_BIT
> +#define USE_32BIT 1
> +#else
> +#define USE_32BIT 0
> +#endif
Above you get rid of the USE_32BIT stuff; here you re-introduce it.
Why?
> diff --git a/drivers/net/smc91111.h b/drivers/net/smc91111.h
> index 967addd..b2ed6a5 100644
> --- a/drivers/net/smc91111.h
> +++ b/drivers/net/smc91111.h
> @@ -72,6 +72,10 @@ typedef unsigned long int dword;
>
> /* Because of bank switching, the LAN91xxx uses only 16 I/O ports */
>
> +#ifndef SMC_BASE_ADDRESS
> +#define SMC_BASE_ADDRESS smc->regs
> +#endif
> +
> #define SMC_IO_EXTENT 16
>
> #ifdef CONFIG_PXA250
> @@ -301,8 +305,6 @@ typedef unsigned long int dword;
>
> #endif /* CONFIG_SMC_USE_IOFUNCS */
>
> -#if defined(CONFIG_SMC_USE_32_BIT)
> -
> #ifdef CONFIG_XSENGINE
> #define SMC_inl(r) (*((volatile dword *)(SMC_BASE_ADDRESS+(r<<1))))
This should be fixed to use I/O accessors, too.
> include/configs/ADNPESC1.h | 1 +
> include/configs/DK1C20.h | 1 +
> include/configs/DK1S10.h | 1 +
> include/configs/EP1C20.h | 1 +
> include/configs/EP1S10.h | 1 +
> include/configs/EP1S40.h | 1 +
> include/configs/MigoR.h | 1 +
> include/configs/PK1C20.h | 1 +
> include/configs/bf533-ezkit.h | 1 +
> include/configs/bf533-stamp.h | 1 +
> include/configs/bf538f-ezkit.h | 1 +
> include/configs/bf561-ezkit.h | 1 +
> include/configs/blackstamp.h | 1 +
> include/configs/cerf250.h | 1 +
> include/configs/cm-bf533.h | 1 +
> include/configs/cm-bf561.h | 1 +
> include/configs/cradle.h | 1 +
> include/configs/delta.h | 1 +
> include/configs/dnp1110.h | 1 +
> include/configs/gr_cpci_ax2000.h | 1 +
> include/configs/gr_ep2s60.h | 1 +
> include/configs/innokom.h | 1 +
> include/configs/integratorcp.h | 1 +
> include/configs/logodl.h | 1 +
> include/configs/lpd7a400-10.h | 1 +
> include/configs/lpd7a404-10.h | 1 +
> include/configs/ms7722se.h | 1 +
> include/configs/netstar.h | 1 +
> include/configs/nhk8815.h | 1 +
> include/configs/pxa255_idp.h | 1 +
> include/configs/versatile.h | 1 +
> include/configs/voiceblue.h | 1 +
> include/configs/xaeniax.h | 1 +
> include/configs/xm250.h | 1 +
> include/configs/xsengine.h | 1 +
> include/configs/zylonite.h | 1 +
This is a pretty long list of boards which is affected. How many of
these have actuaaly been tested with this patch, and which ones, and
to what extent?
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: [email protected]
Lack of skill dictates economy of style. - Joey Ramone
_______________________________________________
U-Boot mailing list
[email protected]
http://lists.denx.de/mailman/listinfo/u-boot
