Re: [PATCH] Consolidate XILINX_VIRTEX board support

[Arnd Bergmann <arnd@arndb.de>]

On Tuesday 07 August 2007, Wolfgang Reissnegger wrote:
>=20
> Make support for Xilinx boards more generic, making it easier
> to add new boards. =A0Add initial support for xupv2p and ml410 boards.

I really think we shouldn't add stuff to arch/ppc at this point,
it has been deprecated for over two years now.

> --- a/arch/ppc/platforms/4xx/virtex.h
> +++ b/arch/ppc/platforms/4xx/virtex.h
> @@ -31,5 +31,14 @@ extern const char* virtex_machine_name;
> =A0#define PPC4xx_ONB_IO_VADDR=A0=A0=A0=A00u
> =A0#define PPC4xx_ONB_IO_SIZE=A0=A0=A0=A0=A00u
> =A0
> +
> +#if defined(CONFIG_XILINX_VIRTEX_II_PRO)
> +#define XILINX_ARCH "Virtex-II Pro"
> +#elif defined(CONFIG_XILINX_VIRTEX_4_FX)
> +#define XILINX_ARCH "Virtex-4 FX"
> +#else
> +#error "No Xilinx Architecture recognized."
> +#endif
> +
> =A0#endif=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0/* __ASM_VIRTEX_H__ */
> =A0#endif=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0/* __KERNEL__ */

When you move over to arch/powerpc, you can't do it this way, because
that prevents you from building a multiplatform kernel.

Every macro and global function that gets defined must be able to coexist
with others that are enabled by nonexclusive configuration options.

> +#if defined(CONFIG_XILINX_ML300)
> +const char *virtex_machine_name =3D "Xilinx ML300";
> +#elif defined(CONFIG_XILINX_XUPV2P)
> +const char *virtex_machine_name =3D "Xilinx XUPV2P";
> +#elif defined(CONFIG_XILINX_ML40x)
> +const char *virtex_machine_name =3D "Xilinx ML40x";
> +#elif defined(CONFIG_XILINX_ML41x)
> +const char *virtex_machine_name =3D "Xilinx ML41x";
> +#else
> +const char *virtex_machine_name =3D "Unknown Xilinx with PowerPC";
> +#endif

same here.

> +
> +
> +#if defined(XPAR_POWER_0_POWERDOWN_BASEADDR)
> +static volatile unsigned *powerdown_base =3D
> + =A0 =A0(volatile unsigned *) XPAR_POWER_0_POWERDOWN_BASEADDR;

volatile is a bug. This needs to be __iomem, and the address probably
needs to come from of_iomap() or a similar function.

> +static void
> +xilinx_power_off(void)
> +{
> +=A0=A0=A0=A0=A0=A0=A0local_irq_disable();
> +=A0=A0=A0=A0=A0=A0=A0out_be32(powerdown_base, XPAR_POWER_0_POWERDOWN_VAL=
UE);
> +=A0=A0=A0=A0=A0=A0=A0while (1) ;
> +}
> +#endif

You should run 'sparse' on your code before submitting patches. That
would have caught this bug in the out_be32() instruction that expects
an __iomem pointer.

> +
> +void __init
> +xilinx_generic_ppc_map_io(void)
> +{
> +=A0=A0=A0=A0=A0=A0=A0ppc4xx_map_io();
> +
> +#if defined(XPAR_POWER_0_POWERDOWN_BASEADDR)
> +=A0=A0=A0=A0=A0=A0=A0powerdown_base =3D ioremap((unsigned long) powerdow=
n_base,
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0=A0=A0=A0 XPAR_POWER_0_POWERDOWN_HIGHADDR -
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=
=A0=A0=A0=A0=A0=A0=A0 XPAR_POWER_0_POWERDOWN_BASEADDR + 1);
> +#endif
> +}

It's a rather unconventional way of working with the addresses.
You should never use a single variable for pointers going to
two different address spaces (physical and kernel virtual in this
case).

> +void __init
> +xilinx_generic_ppc_setup_arch(void)
> +{
> +=A0=A0=A0=A0=A0=A0=A0virtex_early_serial_map();

Use legacy_serial/of_serial to do this, with proper device tree entries.

> +
> +int virtex_device_fixup(struct platform_device *dev)
> +{
> + int i;
> +#ifdef XPAR_ONEWIRE_0_BASEADDR
> + /* Use the Silicon Serial ID attached on the onewire bus to */
> + /* generate sensible MAC addresses. */
> + unsigned char *pOnewire =3D ioremap(XPAR_ONEWIRE_0_BASEADDR, 6);
> + struct xemac_platform_data *pdata =3D dev->dev.platform_data;
> + if (strcmp(dev->name, "xilinx_emac") =3D=3D 0) {

This is heavily whitespace damaged, use tabs for indentation instead
of spaces.

> diff --git a/arch/ppc/platforms/4xx/xparameters/xparameters.h b/arch/ppc/=
platforms/4xx/xparameters/xparameters.h
> index 01aa043..34d9844 100644
> --- a/arch/ppc/platforms/4xx/xparameters/xparameters.h
> +++ b/arch/ppc/platforms/4xx/xparameters/xparameters.h
> @@ -15,8 +15,12 @@
> =A0
> =A0#if defined(CONFIG_XILINX_ML300)
> =A0 =A0#include "xparameters_ml300.h"
> +#elif defined(CONFIG_XILINX_XUPV2P)
> + =A0#include "xparameters_xupv2p.h"
> =A0#elif defined(CONFIG_XILINX_ML403)
> =A0 =A0#include "xparameters_ml403.h"
> +#elif defined(CONFIG_XILINX_ML41x)
> + =A0#include "xparameters_ml41x.h"
> =A0#else
> =A0 =A0/* Add other board xparameter includes here before the #else */
> =A0 =A0#error No xparameters_*.h file included

see comment above.

> +/* Definitions for driver PLBARB */
> +#define XPAR_XPLBARB_NUM_INSTANCES 1
> +
> +/* Definitions for peripheral PLB */
> +#define XPAR_PLB_BASEADDR 0x00000000
> +#define XPAR_PLB_HIGHADDR 0x00000000
> +#define XPAR_PLB_DEVICE_ID 0
> +#define XPAR_PLB_PLB_NUM_MASTERS 3
> +
> +
> +/******************************************************************/
> +
> +/* Definitions for driver OPBARB */
> +#define XPAR_XOPBARB_NUM_INSTANCES 1
> +
> +/* Definitions for peripheral OPB */
> +#define XPAR_OPB_BASEADDR 0xFFFFFFFF
> +#define XPAR_OPB_HIGHADDR 0x00000000
> +#define XPAR_OPB_DEVICE_ID 0
> +#define XPAR_OPB_NUM_MASTERS 1
> +
> +/******************************************************************/
> +
> +
> +/* Definitions for peripheral OPB_SOCKET_0 */
> +#define XPAR_OPB_SOCKET_0_BASEADDR 0x40000000
> +#define XPAR_OPB_SOCKET_0_HIGHADDR 0x7FFFFFFF
> +#define XPAR_OPB_SOCKET_0_DCR_BASEADDR 0x40700300
> +#define XPAR_OPB_SOCKET_0_DCR_HIGHADDR 0x40700307
> +
> +/******************************************************************/
> +
> +/* Definitions for driver UARTNS550 */
> +#define XPAR_XUARTNS550_NUM_INSTANCES 2
> +#define XPAR_XUARTNS550_CLOCK_HZ 100000000
> +
> +/* Definitions for peripheral RS232_UART_1 */
> +#define XPAR_RS232_UART_1_BASEADDR 0x40400000
> +#define XPAR_RS232_UART_1_HIGHADDR 0x4040FFFF
> +#define XPAR_RS232_UART_1_DEVICE_ID 0
> +
> +
> +/* Definitions for peripheral RS232_UART_2 */
> +#define XPAR_RS232_UART_2_BASEADDR 0x40420000
> +#define XPAR_RS232_UART_2_HIGHADDR 0x4042FFFF
> +#define XPAR_RS232_UART_2_DEVICE_ID 1
> +

none of theses addresses, nor those following below should be hardcoded
in the kernel source, but should come from the device tree.

	Arnd <><