[U-Boot] [RFC 06/10] MIPS: qemu-malta: add PCI support

[Wolfgang Denk]

Dear Gabor Juhos,

In message <1358608777-7270-7-git-send-email-juhosg at openwrt.org> you wrote:
> Qemu emulates the Galileo GT64120 System Controller
> which provides a CPU bus to PCI bus bridge.
> 
> The patch adds driver for this bridge and enables
> PCI support for the emulated Malta board.
> 
> Signed-off-by: Gabor Juhos <juhosg at openwrt.org>
> Cc: Daniel Schwierzeck <daniel.schwierzeck at googlemail.com>
> ---
>  board/qemu-malta/Makefile    |    5 +-
>  board/qemu-malta/pci.c       |  173 ++++++++++++++++++++++++++++++++++++++++++
>  include/configs/qemu-malta.h |    6 ++
>  3 files changed, 183 insertions(+), 1 deletion(-)
>  create mode 100644 board/qemu-malta/pci.c
> 
> diff --git a/board/qemu-malta/Makefile b/board/qemu-malta/Makefile
> index 6251bb8..59c1b1d 100644
> --- a/board/qemu-malta/Makefile
> +++ b/board/qemu-malta/Makefile
> @@ -25,7 +25,10 @@ include $(TOPDIR)/config.mk
>  
>  LIB	= $(obj)lib$(BOARD).o
>  
> -COBJS	= $(BOARD).o
> +COBJS-y			+= $(BOARD).o
> +COBJS-$(CONFIG_PCI)	+= pci.o
> +
> +COBJS   := $(COBJS-y)
>  SOBJS	= lowlevel_init.o
>  
>  SRCS	:= $(SOBJS:.o=.S) $(COBJS:.o=.c)
> diff --git a/board/qemu-malta/pci.c b/board/qemu-malta/pci.c
> new file mode 100644
> index 0000000..823ae9b
> --- /dev/null
> +++ b/board/qemu-malta/pci.c
> @@ -0,0 +1,173 @@
> +/*
> + * Copyright (C) 2013 Gabor Juhos <juhosg at openwrt.org>
> + *
> + * Based on the Linux implementation.
> + *   Copyright (C) 1999, 2000, 2004  MIPS Technologies, Inc.
> + *   Authors: Carsten Langgaard <carstenl at mips.com>
> + *            Maciej W. Rozycki <macro at mips.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + */
> +
> +#include <common.h>
> +#include <asm/addrspace.h>
> +#include <asm/gt64120.h>
> +#include <asm/malta.h>
> +#include <asm/io.h>
> +#include <pci.h>
> +
> +#define PCI_ACCESS_READ  0
> +#define PCI_ACCESS_WRITE 1
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +/*
> + * PCI controller "hose" value
> + */
> +
> +static struct pci_controller hose;
> +static void __iomem *gt_base = (void __iomem *)CKSEG1ADDR(MALTA_GT_BASE);
> +
> +static inline u32 gt_read_host_reg(unsigned reg)
> +{
> +	return __raw_readl(gt_base + reg);
> +}
> +
> +static inline void gt_write_host_reg(unsigned reg, u32 val)
> +{
> +	__raw_writel(val, gt_base + reg);
> +}
> +
> +static inline u32 gt_read_reg(unsigned reg)
> +{
> +	return le32_to_cpu(gt_read_host_reg(reg));
> +}
> +
> +static inline gt_write_reg(unsigned reg, u32 val)
> +{
> +	gt_write_host_reg(reg, cpu_to_le32(val));
> +}

I dislike that you introduce new I/O accessors here, and additionally
in a way which is explicitly discouraged in U-Boot.

We don't allow to access device registers through a base address plus
offset notation; instead, we use C structs to describe the register
layout.

Also, on real hardware your accessors areprobably lacking sufficient
memory barriers etc.

Is there any specific reason for not using the usual standard
accessors as provided by <asm/io.h> ?

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
You can observe a lot just by watching.                  - Yogi Berra