On 01/08/16 03:20, Simon Glass wrote:

Hi Paul,

On 27 July 2016 at 08:26, Paul Burton paul.burton@imgtec.com wrote:

...

Provide a trivial syscon driver matching the generic "syscon" compatible string, allowing for simple system controllers to be used without a custom driver just as in Linux.

Signed-off-by: Paul Burton paul.burton@imgtec.com

---

Changes in v2:

• New patch

drivers/core/syscon-uclass.c | 11 +++++++++++ 1 file changed, 11 insertions(+)

diff --git a/drivers/core/syscon-uclass.c b/drivers/core/syscon-uclass.c index 01bd968..2148469 100644 --- a/drivers/core/syscon-uclass.c +++ b/drivers/core/syscon-uclass.c @@ -95,3 +95,14 @@ UCLASS_DRIVER(syscon) = { .per_device_auto_alloc_size = sizeof(struct syscon_uc_info), .pre_probe = syscon_pre_probe, };

+static const struct udevice_id generic_syscon_ids[] = {

•   { .compatible = "syscon" },
  

•   { }
  

+};

+U_BOOT_DRIVER(generic_syscon) = {

•   .name   = "syscon",
  

•   .id     = UCLASS_SYSCON,
  

•   .of_match = generic_syscon_ids,
  

+};

2.9.0

This would work if driver model checked compatible strings in priority order. But it does not.

lists_bind_fdt() checks every driver to see if it is compatible. So your driver may be picked in preference to a more specific one.

I've been aware of this limitation but it hasn't come up until now.

I suppose the solution is to flip things around so that we:

1. Pick a node
2. Look at each compatible string in turn, starting from the first

(most specific) 3. Search for a driver for that string

Do you want to take a look?

Regards, Simon

Hi Simon,

Ah - I wasn't aware of that discrepancy vs the device tree spec. I've just submitted v4 of my series which introduces a new patch 8 "dm: core: Match compatible strings in order of priority" which should fix that.

Thanks, Paul

Hi Paul,

On 27 July 2016 at 08:26, Paul Burton paul.burton@imgtec.com wrote:

Add a simple driver for the clocks provided by the MIPS Boston development board. The system provides information about 2 clocks whose rates are fixed by the bitfile flashed in the boards FPGA, and this driver simply reads the rates of these 2 clocks.

Signed-off-by: Paul Burton paul.burton@imgtec.com

---

Changes in v2:

• New patch

drivers/clk/Kconfig | 8 +++ drivers/clk/Makefile | 1 + drivers/clk/clk_boston.c | 96 ++++++++++++++++++++++++++++++++ include/dt-bindings/clock/boston-clock.h | 13 +++++ 4 files changed, 118 insertions(+) create mode 100644 drivers/clk/clk_boston.c create mode 100644 include/dt-bindings/clock/boston-clock.h

Reviewed-by: Simon Glass sjg@chromium.org

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index 6eee8eb..85eea0d 100644 --- a/drivers/clk/Kconfig +++ b/drivers/clk/Kconfig @@ -20,6 +20,14 @@ config SPL_CLK setting up clocks within SPL, and allows the same drivers to be used as U-Boot proper.

+config CLK_BOSTON

•   def_bool y if TARGET_BOSTON
  

•   depends on CLK
  

•   select REGMAP
  

•   select SYSCON
  

•   help
  

•     Enable this to support the clocks
  

source "drivers/clk/uniphier/Kconfig" source "drivers/clk/exynos/Kconfig"

diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index f7a8891..9d14122 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -13,3 +13,4 @@ obj-$(CONFIG_SANDBOX) += clk_sandbox_test.o obj-$(CONFIG_MACH_PIC32) += clk_pic32.o obj-$(CONFIG_CLK_UNIPHIER) += uniphier/ obj-$(CONFIG_CLK_EXYNOS) += exynos/ +obj-$(CONFIG_CLK_BOSTON) += clk_boston.o diff --git a/drivers/clk/clk_boston.c b/drivers/clk/clk_boston.c new file mode 100644 index 0000000..dbbb05b --- /dev/null +++ b/drivers/clk/clk_boston.c @@ -0,0 +1,96 @@ +/*

• • Copyright (C) 2016 Imagination Technologies
• • SPDX-License-Identifier: GPL-2.0
• */

+#include <common.h> +#include <clk-uclass.h> +#include <dm.h> +#include <dt-bindings/clock/boston-clock.h> +#include <regmap.h> +#include <syscon.h>

+struct clk_boston {

•   struct regmap *regmap;
  

+};

+#define BOSTON_PLAT_MMCMDIV 0x30 +# define BOSTON_PLAT_MMCMDIV_CLK0DIV (0xff << 0) +# define BOSTON_PLAT_MMCMDIV_INPUT (0xff << 8) +# define BOSTON_PLAT_MMCMDIV_MUL (0xff << 16) +# define BOSTON_PLAT_MMCMDIV_CLK1DIV (0xff << 24)

+static ulong clk_boston_get_rate(struct clk *clk) +{

•   struct clk_boston *state = dev_get_platdata(clk->dev);
  

•   uint32_t in_rate, mul, div;
  

•   uint mmcmdiv;
  

•   int err;
  

•   err = regmap_read(state->regmap, BOSTON_PLAT_MMCMDIV, &mmcmdiv);
  

•   if (err)
  

•           return 0;
  

+#define EXT(field) ((mmcmdiv & field) >> (ffs(field) - 1))

Why not use a function for this?

•   in_rate = EXT(BOSTON_PLAT_MMCMDIV_INPUT);
  

•   mul = EXT(BOSTON_PLAT_MMCMDIV_MUL);
  

•   switch (clk->id) {
  

•   case BOSTON_CLK_SYS:
  

•           div = EXT(BOSTON_PLAT_MMCMDIV_CLK0DIV);
  

•           break;
  

•   case BOSTON_CLK_CPU:
  

•           div = EXT(BOSTON_PLAT_MMCMDIV_CLK1DIV);
  

•           break;
  

•   default:
  

•           return 0;
  

•   }
  

+#undef EXT

•   return (in_rate * mul * 1000000) / div;
  

+}

+const struct clk_ops clk_boston_ops = {

•   .get_rate = clk_boston_get_rate,
  

+};

+static int clk_boston_ofdata_to_platdata(struct udevice *dev) +{

•   struct clk_boston *state = dev_get_platdata(dev);
  

•   struct udevice *syscon;
  

•   int err;
  

•   err = uclass_get_device_by_phandle(UCLASS_SYSCON, dev,
  

•                                      "regmap", &syscon);
  

Note that this is slow. But perhaps it doesn't matter.

•   if (err) {
  

•           error("unable to find syscon device\n");
  

•           return err;
  

•   }
  

•   state->regmap = syscon_get_regmap(syscon);
  

•   if (!state->regmap) {
  

•           error("unable to find regmap\n");
  

•           return -ENODEV;
  

•   }
  

•   return 0;
  

+}

+static const struct udevice_id clk_boston_match[] = {

•   {
  

•           .compatible = "img,boston-clock",
  

•   },
  

•   { /* sentinel */ }
  

+};

+U_BOOT_DRIVER(clk_boston) = {

•   .name = "boston_clock",
  

•   .id = UCLASS_CLK,
  

•   .of_match = clk_boston_match,
  

•   .ofdata_to_platdata = clk_boston_ofdata_to_platdata,
  

•   .platdata_auto_alloc_size = sizeof(struct clk_boston),
  

•   .ops = &clk_boston_ops,
  

+}; diff --git a/include/dt-bindings/clock/boston-clock.h b/include/dt-bindings/clock/boston-clock.h new file mode 100644 index 0000000..25f9cd2 --- /dev/null +++ b/include/dt-bindings/clock/boston-clock.h @@ -0,0 +1,13 @@ +/*

• • Copyright (C) 2016 Imagination Technologies
• • SPDX-License-Identifier: GPL-2.0
• */

+#ifndef __DT_BINDINGS_CLOCK_BOSTON_CLOCK_H__ +#define __DT_BINDINGS_CLOCK_BOSTON_CLOCK_H__

+#define BOSTON_CLK_SYS 0 +#define BOSTON_CLK_CPU 1

+#endif /* __DT_BINDINGS_CLOCK_BOSTON_CLOCK_H__ */

2.9.0

Regards, Simon

Am 27.07.2016 um 16:26 schrieb Paul Burton:

This patch introduces support for building U-Boot to run on the MIPS Boston development board. This is a board built around an FPGA & an Intel EG20T Platform Controller Hub, used largely as part of the development of new CPUs and their software support. It is essentially the successor to the older MIPS Malta board.

Signed-off-by: Paul Burton paul.burton@imgtec.com

---

Changes in v2:

• Use AT instead of $1
• Use a clock driver instead of patching the DT

arch/mips/Kconfig | 16 +++ arch/mips/dts/Makefile | 1 + arch/mips/dts/img,boston.dts | 216 ++++++++++++++++++++++++++++++++++++ board/imgtec/boston/Kconfig | 16 +++ board/imgtec/boston/MAINTAINERS | 6 + board/imgtec/boston/Makefile | 9 ++ board/imgtec/boston/boston-lcd.h | 21 ++++ board/imgtec/boston/boston-regs.h | 47 ++++++++ board/imgtec/boston/checkboard.c | 29 +++++ board/imgtec/boston/ddr.c | 30 +++++ board/imgtec/boston/lowlevel_init.S | 56 ++++++++++ configs/boston_defconfig | 41 +++++++ include/configs/boston.h | 68 ++++++++++++ 13 files changed, 556 insertions(+) create mode 100644 arch/mips/dts/img,boston.dts create mode 100644 board/imgtec/boston/Kconfig create mode 100644 board/imgtec/boston/MAINTAINERS create mode 100644 board/imgtec/boston/Makefile create mode 100644 board/imgtec/boston/boston-lcd.h create mode 100644 board/imgtec/boston/boston-regs.h create mode 100644 board/imgtec/boston/checkboard.c create mode 100644 board/imgtec/boston/ddr.c create mode 100644 board/imgtec/boston/lowlevel_init.S create mode 100644 configs/boston_defconfig create mode 100644 include/configs/boston.h

diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig index 21066f0..7ba0ef2 100644 --- a/arch/mips/Kconfig +++ b/arch/mips/Kconfig @@ -73,9 +73,25 @@ config MACH_PIC32 select OF_CONTROL select DM

+config TARGET_BOSTON

• bool "Support Boston"
• select DM
• select DM_SERIAL
• select OF_CONTROL
• select MIPS_L1_CACHE_SHIFT_6
• select SUPPORTS_BIG_ENDIAN
• select SUPPORTS_LITTLE_ENDIAN
• select SUPPORTS_CPU_MIPS32_R1
• select SUPPORTS_CPU_MIPS32_R2
• select SUPPORTS_CPU_MIPS32_R6
• select SUPPORTS_CPU_MIPS64_R1
• select SUPPORTS_CPU_MIPS64_R2
• select SUPPORTS_CPU_MIPS64_R6

endchoice

source "board/dbau1x00/Kconfig" +source "board/imgtec/boston/Kconfig" source "board/imgtec/malta/Kconfig" source "board/micronas/vct/Kconfig" source "board/pb1x00/Kconfig" diff --git a/arch/mips/dts/Makefile b/arch/mips/dts/Makefile index 2f04d73..6a5e43e 100644 --- a/arch/mips/dts/Makefile +++ b/arch/mips/dts/Makefile @@ -4,6 +4,7 @@

dtb-$(CONFIG_TARGET_AP121) += ap121.dtb dtb-$(CONFIG_TARGET_AP143) += ap143.dtb +dtb-$(CONFIG_TARGET_BOSTON) += img,boston.dtb dtb-$(CONFIG_TARGET_MALTA) += mti,malta.dtb dtb-$(CONFIG_TARGET_PIC32MZDASK) += pic32mzda_sk.dtb dtb-$(CONFIG_BOARD_TPLINK_WDR4300) += tplink_wdr4300.dtb diff --git a/arch/mips/dts/img,boston.dts b/arch/mips/dts/img,boston.dts new file mode 100644 index 0000000..2fbcb93 --- /dev/null +++ b/arch/mips/dts/img,boston.dts @@ -0,0 +1,216 @@ +/dts-v1/;

+#include <dt-bindings/clock/boston-clock.h> +#include <dt-bindings/gpio/gpio.h> +#include <dt-bindings/interrupt-controller/irq.h> +#include <dt-bindings/interrupt-controller/mips-gic.h>

+/ {

• #address-cells = <1>;
• #size-cells = <1>;
• compatible = "img,boston";
• chosen {

• stdout-path = &uart0;
  

• };
• cpus {

• #address-cells = <1>;
  

• #size-cells = <0>;
  

• cpu@0 {
  

• 	device_type = "cpu";
  

• 	compatible = "img,mips";
  

• 	reg = <0>;
  

• 	clocks = <&clk_boston BOSTON_CLK_CPU>;
  

• };
  

• };
• memory@0 {

• device_type = "memory";
  

• reg = <0x00000000 0x10000000>;
  

• };
• gic: interrupt-controller {

• compatible = "mti,gic";
  

• interrupt-controller;
  

• #interrupt-cells = <3>;
  

• timer {
  

• 	compatible = "mti,gic-timer";
  

• 	interrupts = <GIC_LOCAL 1 IRQ_TYPE_NONE>;
  

• 	clocks = <&clk_boston BOSTON_CLK_CPU>;
  

• };
  

• };
• pci0: pci@10000000 {

• status = "disabled";
  

• compatible = "xlnx,axi-pcie-host-1.00.a";
  

• device_type = "pci";
  

• reg = <0x10000000 0x2000000>;
  

• #address-cells = <3>;
  

• #size-cells = <2>;
  

• #interrupt-cells = <1>;
  

• interrupt-parent = <&gic>;
  

• interrupts = <GIC_SHARED 2 IRQ_TYPE_LEVEL_HIGH>;
  

• ranges = <0x02000000 0 0x40000000
  

• 	  0x40000000 0 0x40000000>;
  

• interrupt-map-mask = <0 0 0 7>;
  

• interrupt-map = <0 0 0 1 &pci0_intc 0>,
  

• 		<0 0 0 2 &pci0_intc 1>,
  

• 		<0 0 0 3 &pci0_intc 2>,
  

• 		<0 0 0 4 &pci0_intc 3>;
  

• pci0_intc: interrupt-controller {
  

• 	interrupt-controller;
  

• 	#address-cells = <0>;
  

• 	#interrupt-cells = <1>;
  

• };
  

• };
• pci1: pci@12000000 {

• status = "disabled";
  

• compatible = "xlnx,axi-pcie-host-1.00.a";
  

• device_type = "pci";
  

• reg = <0x12000000 0x2000000>;
  

• #address-cells = <3>;
  

• #size-cells = <2>;
  

• #interrupt-cells = <1>;
  

• interrupt-parent = <&gic>;
  

• interrupts = <GIC_SHARED 1 IRQ_TYPE_LEVEL_HIGH>;
  

• ranges = <0x02000000 0 0x20000000
  

• 	  0x20000000 0 0x20000000>;
  

• interrupt-map-mask = <0 0 0 7>;
  

• interrupt-map = <0 0 0 1 &pci1_intc 0>,
  

• 		<0 0 0 2 &pci1_intc 1>,
  

• 		<0 0 0 3 &pci1_intc 2>,
  

• 		<0 0 0 4 &pci1_intc 3>;
  

• pci1_intc: interrupt-controller {
  

• 	interrupt-controller;
  

• 	#address-cells = <0>;
  

• 	#interrupt-cells = <1>;
  

• };
  

• };
• pci2: pci@14000000 {

• compatible = "xlnx,axi-pcie-host-1.00.a";
  

• device_type = "pci";
  

• reg = <0x14000000 0x2000000>;
  

• #address-cells = <3>;
  

• #size-cells = <2>;
  

• #interrupt-cells = <1>;
  

• interrupt-parent = <&gic>;
  

• interrupts = <GIC_SHARED 0 IRQ_TYPE_LEVEL_HIGH>;
  

• ranges = <0x02000000 0 0x16000000
  

• 	  0x16000000 0 0x100000>;
  

• interrupt-map-mask = <0 0 0 7>;
  

• interrupt-map = <0 0 0 1 &pci2_intc 0>,
  

• 		<0 0 0 2 &pci2_intc 1>,
  

• 		<0 0 0 3 &pci2_intc 2>,
  

• 		<0 0 0 4 &pci2_intc 3>;
  

• pci2_intc: interrupt-controller {
  

• 	interrupt-controller;
  

• 	#address-cells = <0>;
  

• 	#interrupt-cells = <1>;
  

• };
  

• pci2_root@0,0,0 {
  

• 	compatible = "pci10ee,7021";
  

• 	reg = <0x00000000 0 0 0 0>;
  

• 	#address-cells = <3>;
  

• 	#size-cells = <2>;
  

• 	#interrupt-cells = <1>;
  

• 	eg20t_bridge@1,0,0 {
  

• 		compatible = "pci8086,8800";
  

• 		reg = <0x00010000 0 0 0 0>;
  

• 		#address-cells = <3>;
  

• 		#size-cells = <2>;
  

• 		#interrupt-cells = <1>;
  

• 		eg20t_mac@2,0,1 {
  

• 			compatible = "pci8086,8802";
  

• 			reg = <0x00020100 0 0 0 0>;
  

• 			phy-reset-gpios = <&eg20t_gpio 6 GPIO_ACTIVE_LOW>;
  

• 		};
  

• 		eg20t_gpio: eg20t_gpio@2,0,2 {
  

• 			compatible = "pci8086,8803";
  

• 			reg = <0x00020200 0 0 0 0>;
  

• 			gpio-controller;
  

• 			#gpio-cells = <2>;
  

• 		};
  

• 		eg20t_i2c@2,12,2 {
  

• 			compatible = "pci8086,8817";
  

• 			reg = <0x00026200 0 0 0 0>;
  

• 			#address-cells = <1>;
  

• 			#size-cells = <0>;
  

• 			rtc@0x68 {
  

• 				compatible = "st,m41t81s";
  

• 				reg = <0x68>;
  

• 			};
  

• 		};
  

• 	};
  

• };
  

• };
• plat_regs: system-controller@17ffd000 {

• compatible = "img,boston-platform-regs", "syscon";
  

• reg = <0x17ffd000 0x1000>;
  

• u-boot,dm-pre-reloc;
  

• };
• clk_boston: clock {

• compatible = "img,boston-clock";
  

• #clock-cells = <1>;
  

• regmap = <&plat_regs>;
  

• u-boot,dm-pre-reloc;
  

• };
• reboot: syscon-reboot {

• compatible = "syscon-reboot";
  

• regmap = <&plat_regs>;
  

• offset = <0x10>;
  

• mask = <0x10>;
  

• };
• uart0: uart@17ffe000 {

• compatible = "ns16550a";
  

• reg = <0x17ffe000 0x1000>;
  

• reg-shift = <2>;
  

• reg-io-width = <4>;
  

• interrupt-parent = <&gic>;
  

• interrupts = <GIC_SHARED 3 IRQ_TYPE_LEVEL_HIGH>;
  

• clocks = <&clk_boston BOSTON_CLK_SYS>;
  

• u-boot,dm-pre-reloc;
  

• };
• lcd: lcd@17fff000 {

• compatible = "img,boston-lcd";
  

• reg = <0x17fff000 0x8>;
  

• };

+}; diff --git a/board/imgtec/boston/Kconfig b/board/imgtec/boston/Kconfig new file mode 100644 index 0000000..ab76a3c --- /dev/null +++ b/board/imgtec/boston/Kconfig @@ -0,0 +1,16 @@ +if TARGET_BOSTON

+config SYS_BOARD

• default "boston"

+config SYS_VENDOR

• default "imgtec"

+config SYS_CONFIG_NAME

• default "boston"

+config SYS_TEXT_BASE

• default 0x9fc00000 if 32BIT
• default 0xffffffff9fc00000 if 64BIT

+endif diff --git a/board/imgtec/boston/MAINTAINERS b/board/imgtec/boston/MAINTAINERS new file mode 100644 index 0000000..30dd481 --- /dev/null +++ b/board/imgtec/boston/MAINTAINERS @@ -0,0 +1,6 @@ +BOSTON BOARD +M: Paul Burton paul.burton@imgtec.com +S: Maintained +F: board/imgtec/boston/ +F: include/configs/boston.h +F: configs/boston_defconfig diff --git a/board/imgtec/boston/Makefile b/board/imgtec/boston/Makefile new file mode 100644 index 0000000..deda457 --- /dev/null +++ b/board/imgtec/boston/Makefile @@ -0,0 +1,9 @@ +# +# Copyright (C) 2016 Imagination Technologies +# +# SPDX-License-Identifier: GPL-2.0 +#

+obj-y += checkboard.o +obj-y += ddr.o +obj-y += lowlevel_init.o diff --git a/board/imgtec/boston/boston-lcd.h b/board/imgtec/boston/boston-lcd.h new file mode 100644 index 0000000..9f5c1b9 --- /dev/null +++ b/board/imgtec/boston/boston-lcd.h @@ -0,0 +1,21 @@ +/*

• • Copyright (C) 2016 Imagination Technologies
• • SPDX-License-Identifier: GPL-2.0
• */

+#ifndef __BOARD_BOSTON_LCD_H__ +#define __BOARD_BOSTON_LCD_H__

+/**

• • lowlevel_display() - Display a message on Boston's LCD
• • @msg: The string to display
• • Display the string @msg on the 7 character LCD display of the Boston board.
• • This is typically used for debug or to present some form of status
• • indication to the user, allowing faults to be identified when things go
• • wrong early enough that the UART isn't up.
• */

+void lowlevel_display(const char msg[static 8]);

+#endif /* __BOARD_BOSTON_LCD_H__ */ diff --git a/board/imgtec/boston/boston-regs.h b/board/imgtec/boston/boston-regs.h new file mode 100644 index 0000000..c33535e --- /dev/null +++ b/board/imgtec/boston/boston-regs.h @@ -0,0 +1,47 @@ +/*

• • Copyright (C) 2016 Imagination Technologies
• • SPDX-License-Identifier: GPL-2.0
• */

+#ifndef __BOARD_BOSTON_REGS_H__ +#define __BOARD_BOSTON_REGS_H__

+#define BOSTON_PLAT_BASE 0x17ffd000 +#define BOSTON_LCD_BASE 0x17fff000

+/*

• • Platform Register Definitions
• */

+#define BOSTON_PLAT_CORE_CL 0x04

+#define BOSTON_PLAT_DDR3STAT 0x14 +# define BOSTON_PLAT_DDR3STAT_CALIB (1 << 2)

+#define BOSTON_PLAT_MMCMDIV 0x30 +# define BOSTON_PLAT_MMCMDIV_CLK0DIV (0xff << 0) +# define BOSTON_PLAT_MMCMDIV_INPUT (0xff << 8) +# define BOSTON_PLAT_MMCMDIV_MUL (0xff << 16) +# define BOSTON_PLAT_MMCMDIV_CLK1DIV (0xff << 24)

+#define BOSTON_PLAT_DDRCONF0 0x38 +# define BOSTON_PLAT_DDRCONF0_SIZE (0xf << 0)

+#ifndef __ASSEMBLY__

+#include <asm/io.h>

+#define BUILD_PLAT_ACCESSORS(offset, name) \ +static inline uint32_t read_boston_##name(void) \ +{ \

• uint32_t *reg = (void *)CKSEG1ADDR(BOSTON_PLAT_BASE) + (offset);\
• return __raw_readl(reg); \

+}

+BUILD_PLAT_ACCESSORS(BOSTON_PLAT_CORE_CL, core_cl) +BUILD_PLAT_ACCESSORS(BOSTON_PLAT_MMCMDIV, mmcmdiv) +BUILD_PLAT_ACCESSORS(BOSTON_PLAT_DDRCONF0, ddrconf0)

+#endif /* !__ASSEMBLY__ */

+#endif /* __BOARD_BOSTON_REGS_H__ */ diff --git a/board/imgtec/boston/checkboard.c b/board/imgtec/boston/checkboard.c new file mode 100644 index 0000000..417ac4e --- /dev/null +++ b/board/imgtec/boston/checkboard.c @@ -0,0 +1,29 @@ +/*

• • Copyright (C) 2016 Imagination Technologies
• • SPDX-License-Identifier: GPL-2.0
• */

+#include <common.h>

+#include <asm/mipsregs.h>

+#include "boston-lcd.h" +#include "boston-regs.h"

+int checkboard(void) +{

• u32 changelist;
• lowlevel_display("U-boot ");
• printf("Board: MIPS Boston\n");
• printf("CPU: 0x%08x", read_c0_prid());
• changelist = read_boston_core_cl();
• if (changelist > 1)

• printf(" cl%x", changelist);
  

• putc('\n');
• return 0;

+} diff --git a/board/imgtec/boston/ddr.c b/board/imgtec/boston/ddr.c new file mode 100644 index 0000000..7caed4b --- /dev/null +++ b/board/imgtec/boston/ddr.c @@ -0,0 +1,30 @@ +/*

• • Copyright (C) 2016 Imagination Technologies
• • SPDX-License-Identifier: GPL-2.0
• */

+#include <common.h>

+#include <asm/addrspace.h>

+#include "boston-regs.h"

+phys_size_t initdram(int board_type) +{

• u32 ddrconf0 = read_boston_ddrconf0();
• return (phys_size_t)(ddrconf0 & BOSTON_PLAT_DDRCONF0_SIZE) << 30;

+}

+ulong board_get_usable_ram_top(ulong total_size) +{

• DECLARE_GLOBAL_DATA_PTR;
• if (gd->ram_top < CONFIG_SYS_SDRAM_BASE) {

• /* 2GB wrapped around to 0 */
  

• return CKSEG0ADDR(256 << 20);
  

• }
• return min_t(unsigned long, gd->ram_top, CKSEG0ADDR(256 << 20));

+} diff --git a/board/imgtec/boston/lowlevel_init.S b/board/imgtec/boston/lowlevel_init.S new file mode 100644 index 0000000..8928172 --- /dev/null +++ b/board/imgtec/boston/lowlevel_init.S @@ -0,0 +1,56 @@ +/*

• • Copyright (C) 2016 Imagination Technologies
• • SPDX-License-Identifier: GPL-2.0
• */

+#include <config.h>

+#include <asm/addrspace.h> +#include <asm/asm.h> +#include <asm/mipsregs.h> +#include <asm/regdef.h>

+#include "boston-regs.h"

+.data

+msg_ddr_cal: .ascii "DDR Cal " +msg_ddr_ok: .ascii "DDR OK "

+.text

+LEAF(lowlevel_init)

• move s0, ra
• PTR_LA a0, msg_ddr_cal
• bal lowlevel_display
• PTR_LI t0, CKSEG1ADDR(BOSTON_PLAT_BASE)

+1: lw t1, BOSTON_PLAT_DDR3STAT(t0)

• andi t1, t1, BOSTON_PLAT_DDR3STAT_CALIB
• beqz t1, 1b
• PTR_LA a0, msg_ddr_ok
• bal lowlevel_display
• move v0, zero
• jr s0
• END(lowlevel_init)

+LEAF(lowlevel_display)

• .set push
• .set noat
• PTR_LI AT, CKSEG1ADDR(BOSTON_LCD_BASE)

+#ifdef CONFIG_64BIT

• ld k1, 0(a0)
• sd k1, 0(AT)

+#else

• lw k1, 0(a0)
• sw k1, 0(AT)
• lw k1, 4(a0)
• sw k1, 4(AT)

+#endif

• .set pop

+1: jr ra

• END(lowlevel_display)

diff --git a/configs/boston_defconfig b/configs/boston_defconfig new file mode 100644 index 0000000..381203a --- /dev/null +++ b/configs/boston_defconfig @@ -0,0 +1,41 @@ +CONFIG_MIPS=y +CONFIG_TARGET_BOSTON=y +CONFIG_SYS_LITTLE_ENDIAN=y +CONFIG_CPU_MIPS64_R6=y

I've noticed that this defconfig with MIPS64r6 breaks buildman and TravisCI for me and likely a lot of other people because most available toolchains (ELDK, kernel.org, etc.) don't have gcc-5.x yet. And Imgtec provides two different toolchains for R6 and R1..R5 so this needs to be handled too in buildman.

Does it make sense to add different defconfigs for Boston similar to Malta to have build coverage for MIPS32r2 and MIPS64r2 as well as BigEndian and LittleEndian? We can add the MIPS32r6 and MIPS64r6 variants later when more toolchains with a recent gcc are available. What do you think?

+# CONFIG_MIPS_BOOT_CMDLINE_LEGACY is not set +# CONFIG_MIPS_BOOT_ENV_LEGACY is not set +CONFIG_MIPS_BOOT_FDT=y +CONFIG_DEFAULT_DEVICE_TREE="img,boston" +CONFIG_FIT=y +CONFIG_FIT_VERBOSE=y +CONFIG_FIT_BEST_MATCH=y +CONFIG_OF_STDOUT_VIA_ALIAS=y +CONFIG_SYS_NO_FLASH=y +CONFIG_HUSH_PARSER=y +CONFIG_SYS_PROMPT="boston # " +# CONFIG_CMD_ELF is not set +# CONFIG_CMD_IMLS is not set +CONFIG_CMD_GREPENV=y +CONFIG_CMD_MEMTEST=y +# CONFIG_CMD_LOADB is not set +# CONFIG_CMD_LOADS is not set +# CONFIG_CMD_FPGA is not set +CONFIG_CMD_DHCP=y +CONFIG_CMD_PING=y +CONFIG_CMD_SNTP=y +CONFIG_CMD_DNS=y +CONFIG_CMD_LINK_LOCAL=y +CONFIG_CMD_TIME=y +CONFIG_CMD_EXT4=y +CONFIG_CMD_EXT4_WRITE=y +CONFIG_CMD_FAT=y +CONFIG_CMD_FS_GENERIC=y +CONFIG_OF_EMBED=y +CONFIG_NET_RANDOM_ETHADDR=y +CONFIG_CLK=y +CONFIG_DM_ETH=y +CONFIG_PCH_GBE=y +CONFIG_DM_PCI=y +CONFIG_PCI_XILINX=y +CONFIG_SYS_NS16550=y +CONFIG_LZ4=y diff --git a/include/configs/boston.h b/include/configs/boston.h new file mode 100644 index 0000000..e25c8d5 --- /dev/null +++ b/include/configs/boston.h @@ -0,0 +1,68 @@ +/*

• • Copyright (C) 2016 Imagination Technologies
• • SPDX-License-Identifier: GPL-2.0
• */

+#ifndef __CONFIGS_BOSTON_H__ +#define __CONFIGS_BOSTON_H__

+/*

• • General board configuration
• */

+#define CONFIG_DISPLAY_BOARDINFO

+/*

• • CPU
• */

+#define CONFIG_SYS_MIPS_TIMER_FREQ 30000000

+/*

• • PCI
• */

+#define CONFIG_PCI +#define CONFIG_PCI_PNP +#define CONFIG_CMD_PCI

+/*

• • Environment
• */

+#define CONFIG_ENV_IS_NOWHERE +#define CONFIG_ENV_SIZE 1024

+/*

• • Memory map
• */

+#ifdef CONFIG_64BIT +# define CONFIG_SYS_SDRAM_BASE 0xffffffff80000000 +#else +# define CONFIG_SYS_SDRAM_BASE 0x80000000 +#endif

+#define CONFIG_SYS_INIT_SP_OFFSET 0x400000

+#define CONFIG_SYS_MONITOR_BASE CONFIG_SYS_TEXT_BASE

+#define CONFIG_SYS_LOAD_ADDR (CONFIG_SYS_SDRAM_BASE + 0x100000)

+#define CONFIG_SYS_MEMTEST_START (CONFIG_SYS_SDRAM_BASE + 0) +#define CONFIG_SYS_MEMTEST_END (CONFIG_SYS_SDRAM_BASE + 0x10000000)

+#define CONFIG_SYS_MALLOC_LEN (256 * 1024)

+/*

• • Console
• */

+#define CONFIG_SYS_MAXARGS 16 +#define CONFIG_SYS_CBSIZE 256 +#define CONFIG_SYS_PBSIZE (CONFIG_SYS_CBSIZE + \

• 			 sizeof(CONFIG_SYS_PROMPT) + 16)
  

+#define CONFIG_SYS_LONGHELP +#define CONFIG_BAUDRATE 115200

+/*

• • Flash
• */

+#define CONFIG_SYS_MAX_FLASH_BANKS 1

+#endif /* __CONFIGS_BOSTON_H__ */

Am 31.07.2016 um 12:04 schrieb Paul Burton:

On 27/07/16 20:21, Daniel Schwierzeck wrote:

...

...

diff --git a/configs/boston_defconfig b/configs/boston_defconfig new file mode 100644 index 0000000..381203a --- /dev/null +++ b/configs/boston_defconfig @@ -0,0 +1,41 @@ +CONFIG_MIPS=y +CONFIG_TARGET_BOSTON=y +CONFIG_SYS_LITTLE_ENDIAN=y +CONFIG_CPU_MIPS64_R6=y

I've noticed that this defconfig with MIPS64r6 breaks buildman and TravisCI for me and likely a lot of other people because most available toolchains (ELDK, kernel.org, etc.) don't have gcc-5.x yet. And Imgtec provides two different toolchains for R6 and R1..R5 so this needs to be handled too in buildman.

Does it make sense to add different defconfigs for Boston similar to Malta to have build coverage for MIPS32r2 and MIPS64r2 as well as BigEndian and LittleEndian? We can add the MIPS32r6 and MIPS64r6 variants later when more toolchains with a recent gcc are available. What do you think?

Hi Daniel,

I don't mind so much - I'm happy to change the defconfig to R2 & have separate 32 & 64 bit ones if you prefer. I chose MIPS64r6 for it since that's what's being used primarily on real Boston boards. Would you like me to submit a v3?

yes please, but only for patch 10/10. The other patches are already good for merging.

FYI whilst the codescape toolchains on imgtec.com have a split between pre-R6 (MTI toolchain) & R6 (IMG toolchain) they only differ in the prebuilt libraries included with them, so since U-Boot (or Linux) don't link with those libraries you can build with either toolchain for any arch revision. For internal U-Boot releases I compile for all combinations of r2 & r6, 32 & 64 bit, big & little endian with the same toolchain (then bolt them all together into 1 binary with a bit of magic trampoline code at the start, which it'd be neat to get tidied & upstream some time!).

thanks for the info. I'll have a look into extending buildman to fetch the Codescape IMG toolchains for MIPS.

On 07/27/2016 04:26 PM, Paul Burton wrote:

This patch introduces support for building U-Boot to run on the MIPS Boston development board. This is a board built around an FPGA & an Intel EG20T Platform Controller Hub, used largely as part of the development of new CPUs and their software support. It is essentially the successor to the older MIPS Malta board.

Signed-off-by: Paul Burton paul.burton@imgtec.com

[...]

+++ b/board/imgtec/boston/boston-regs.h @@ -0,0 +1,47 @@ +/*

• • Copyright (C) 2016 Imagination Technologies
• • SPDX-License-Identifier: GPL-2.0
• */

+#ifndef __BOARD_BOSTON_REGS_H__ +#define __BOARD_BOSTON_REGS_H__

+#define BOSTON_PLAT_BASE 0x17ffd000 +#define BOSTON_LCD_BASE 0x17fff000

+/*

• • Platform Register Definitions
• */

+#define BOSTON_PLAT_CORE_CL 0x04

+#define BOSTON_PLAT_DDR3STAT 0x14 +# define BOSTON_PLAT_DDR3STAT_CALIB (1 << 2)

+#define BOSTON_PLAT_MMCMDIV 0x30 +# define BOSTON_PLAT_MMCMDIV_CLK0DIV (0xff << 0) +# define BOSTON_PLAT_MMCMDIV_INPUT (0xff << 8) +# define BOSTON_PLAT_MMCMDIV_MUL (0xff << 16) +# define BOSTON_PLAT_MMCMDIV_CLK1DIV (0xff << 24)

+#define BOSTON_PLAT_DDRCONF0 0x38 +# define BOSTON_PLAT_DDRCONF0_SIZE (0xf << 0)

+#ifndef __ASSEMBLY__

+#include <asm/io.h>

+#define BUILD_PLAT_ACCESSORS(offset, name) \ +static inline uint32_t read_boston_##name(void) \ +{ \

• uint32_t *reg = (void *)CKSEG1ADDR(BOSTON_PLAT_BASE) + (offset);\
• return __raw_readl(reg); \

+}

Don't we have enough standard accessors to confuse people ? Why do you add another custom ones ? Remove this and just use standard accessors throughout the code.

+BUILD_PLAT_ACCESSORS(BOSTON_PLAT_CORE_CL, core_cl) +BUILD_PLAT_ACCESSORS(BOSTON_PLAT_MMCMDIV, mmcmdiv) +BUILD_PLAT_ACCESSORS(BOSTON_PLAT_DDRCONF0, ddrconf0)

+#endif /* !__ASSEMBLY__ */

+#endif /* __BOARD_BOSTON_REGS_H__ */ diff --git a/board/imgtec/boston/checkboard.c b/board/imgtec/boston/checkboard.c new file mode 100644 index 0000000..417ac4e --- /dev/null +++ b/board/imgtec/boston/checkboard.c @@ -0,0 +1,29 @@ +/*

• • Copyright (C) 2016 Imagination Technologies
• • SPDX-License-Identifier: GPL-2.0
• */

+#include <common.h>

+#include <asm/mipsregs.h>

+#include "boston-lcd.h" +#include "boston-regs.h"

+int checkboard(void) +{

• u32 changelist;
• lowlevel_display("U-boot ");
• printf("Board: MIPS Boston\n");
• printf("CPU: 0x%08x", read_c0_prid());

This should be in print_cpuinfo()

• changelist = read_boston_core_cl();
• if (changelist > 1)

• printf(" cl%x", changelist);
  

• putc('\n');
• return 0;

+} diff --git a/board/imgtec/boston/ddr.c b/board/imgtec/boston/ddr.c new file mode 100644 index 0000000..7caed4b --- /dev/null +++ b/board/imgtec/boston/ddr.c @@ -0,0 +1,30 @@ +/*

• • Copyright (C) 2016 Imagination Technologies
• • SPDX-License-Identifier: GPL-2.0
• */

+#include <common.h>

+#include <asm/addrspace.h>

+#include "boston-regs.h"

+phys_size_t initdram(int board_type) +{

• u32 ddrconf0 = read_boston_ddrconf0();
• return (phys_size_t)(ddrconf0 & BOSTON_PLAT_DDRCONF0_SIZE) << 30;

+}

+ulong board_get_usable_ram_top(ulong total_size) +{

• DECLARE_GLOBAL_DATA_PTR;
• if (gd->ram_top < CONFIG_SYS_SDRAM_BASE) {

• /* 2GB wrapped around to 0 */
  

• return CKSEG0ADDR(256 << 20);
  

• }
• return min_t(unsigned long, gd->ram_top, CKSEG0ADDR(256 << 20));

+} diff --git a/board/imgtec/boston/lowlevel_init.S b/board/imgtec/boston/lowlevel_init.S new file mode 100644 index 0000000..8928172 --- /dev/null +++ b/board/imgtec/boston/lowlevel_init.S @@ -0,0 +1,56 @@ +/*

• • Copyright (C) 2016 Imagination Technologies
• • SPDX-License-Identifier: GPL-2.0
• */

+#include <config.h>

+#include <asm/addrspace.h> +#include <asm/asm.h> +#include <asm/mipsregs.h> +#include <asm/regdef.h>

+#include "boston-regs.h"

+.data

+msg_ddr_cal: .ascii "DDR Cal " +msg_ddr_ok: .ascii "DDR OK "

+.text

+LEAF(lowlevel_init)

• move s0, ra
• PTR_LA a0, msg_ddr_cal
• bal lowlevel_display

Don't you need nop after branch on mips ?

• PTR_LI t0, CKSEG1ADDR(BOSTON_PLAT_BASE)

+1: lw t1, BOSTON_PLAT_DDR3STAT(t0)

• andi t1, t1, BOSTON_PLAT_DDR3STAT_CALIB
• beqz t1, 1b
• PTR_LA a0, msg_ddr_ok
• bal lowlevel_display
• move v0, zero
• jr s0
• END(lowlevel_init)

+LEAF(lowlevel_display)

• .set push
• .set noat
• PTR_LI AT, CKSEG1ADDR(BOSTON_LCD_BASE)

+#ifdef CONFIG_64BIT

• ld k1, 0(a0)
• sd k1, 0(AT)

+#else

• lw k1, 0(a0)
• sw k1, 0(AT)
• lw k1, 4(a0)
• sw k1, 4(AT)

+#endif

• .set pop

+1: jr ra

• END(lowlevel_display)

diff --git a/configs/boston_defconfig b/configs/boston_defconfig new file mode 100644 index 0000000..381203a --- /dev/null +++ b/configs/boston_defconfig @@ -0,0 +1,41 @@ +CONFIG_MIPS=y +CONFIG_TARGET_BOSTON=y +CONFIG_SYS_LITTLE_ENDIAN=y +CONFIG_CPU_MIPS64_R6=y +# CONFIG_MIPS_BOOT_CMDLINE_LEGACY is not set +# CONFIG_MIPS_BOOT_ENV_LEGACY is not set +CONFIG_MIPS_BOOT_FDT=y +CONFIG_DEFAULT_DEVICE_TREE="img,boston" +CONFIG_FIT=y +CONFIG_FIT_VERBOSE=y +CONFIG_FIT_BEST_MATCH=y +CONFIG_OF_STDOUT_VIA_ALIAS=y +CONFIG_SYS_NO_FLASH=y +CONFIG_HUSH_PARSER=y +CONFIG_SYS_PROMPT="boston # " +# CONFIG_CMD_ELF is not set +# CONFIG_CMD_IMLS is not set +CONFIG_CMD_GREPENV=y +CONFIG_CMD_MEMTEST=y +# CONFIG_CMD_LOADB is not set +# CONFIG_CMD_LOADS is not set +# CONFIG_CMD_FPGA is not set +CONFIG_CMD_DHCP=y +CONFIG_CMD_PING=y +CONFIG_CMD_SNTP=y +CONFIG_CMD_DNS=y +CONFIG_CMD_LINK_LOCAL=y +CONFIG_CMD_TIME=y +CONFIG_CMD_EXT4=y +CONFIG_CMD_EXT4_WRITE=y +CONFIG_CMD_FAT=y +CONFIG_CMD_FS_GENERIC=y +CONFIG_OF_EMBED=y +CONFIG_NET_RANDOM_ETHADDR=y +CONFIG_CLK=y +CONFIG_DM_ETH=y +CONFIG_PCH_GBE=y +CONFIG_DM_PCI=y +CONFIG_PCI_XILINX=y +CONFIG_SYS_NS16550=y +CONFIG_LZ4=y diff --git a/include/configs/boston.h b/include/configs/boston.h new file mode 100644 index 0000000..e25c8d5 --- /dev/null +++ b/include/configs/boston.h @@ -0,0 +1,68 @@ +/*

• • Copyright (C) 2016 Imagination Technologies
• • SPDX-License-Identifier: GPL-2.0
• */

+#ifndef __CONFIGS_BOSTON_H__ +#define __CONFIGS_BOSTON_H__

+/*

• • General board configuration
• */

+#define CONFIG_DISPLAY_BOARDINFO

+/*

• • CPU
• */

+#define CONFIG_SYS_MIPS_TIMER_FREQ 30000000

+/*

• • PCI
• */

+#define CONFIG_PCI +#define CONFIG_PCI_PNP +#define CONFIG_CMD_PCI

+/*

• • Environment
• */

+#define CONFIG_ENV_IS_NOWHERE +#define CONFIG_ENV_SIZE 1024

+/*

• • Memory map
• */

+#ifdef CONFIG_64BIT +# define CONFIG_SYS_SDRAM_BASE 0xffffffff80000000 +#else +# define CONFIG_SYS_SDRAM_BASE 0x80000000 +#endif

+#define CONFIG_SYS_INIT_SP_OFFSET 0x400000

+#define CONFIG_SYS_MONITOR_BASE CONFIG_SYS_TEXT_BASE

+#define CONFIG_SYS_LOAD_ADDR (CONFIG_SYS_SDRAM_BASE + 0x100000)

+#define CONFIG_SYS_MEMTEST_START (CONFIG_SYS_SDRAM_BASE + 0) +#define CONFIG_SYS_MEMTEST_END (CONFIG_SYS_SDRAM_BASE + 0x10000000)

+#define CONFIG_SYS_MALLOC_LEN (256 * 1024)

+/*

• • Console
• */

+#define CONFIG_SYS_MAXARGS 16 +#define CONFIG_SYS_CBSIZE 256 +#define CONFIG_SYS_PBSIZE (CONFIG_SYS_CBSIZE + \

• 			 sizeof(CONFIG_SYS_PROMPT) + 16)
  

+#define CONFIG_SYS_LONGHELP +#define CONFIG_BAUDRATE 115200

+/*

• • Flash
• */

+#define CONFIG_SYS_MAX_FLASH_BANKS 1

+#endif /* __CONFIGS_BOSTON_H__ */

On 07/29/2016 10:36 AM, Paul Burton wrote: [...]

...

...

+#ifndef __ASSEMBLY__

+#include <asm/io.h>

+#define BUILD_PLAT_ACCESSORS(offset, name) \ +static inline uint32_t read_boston_##name(void) \ +{ \

• uint32_t *reg = (void *)CKSEG1ADDR(BOSTON_PLAT_BASE) + (offset);\
• return __raw_readl(reg); \

+}

Don't we have enough standard accessors to confuse people ? Why do you add another custom ones ? Remove this and just use standard accessors throughout the code.

Hi Marek,

These accessors are simple wrappers around __raw_readl, I'd hardly say they can be considered confusing. The alternative is lots of:

val = __raw_readl((void *)CKSEG1ADDR(BOSTON_PLAT_BASE) + OFFSET);

...and that is just plain ugly.

This should be map_physmem() + readl(), see ie. the ag7xxx.c driver or whatever other stuff from the atheros ath79 port. Does this work ?

Invoking readl on a field of a struct representing these registers would be nice, but some of them need to be accessed from assembly so that would involve duplication which isn't nice.

The struct based access is deprecated, don't bother with it.

I think this way is the best option, where if you want to read the Boston core_cl register you call read_boston_core_cl() - it's hardly confusing what that does.

Now imagine what would happen if everyone introduced his own my_platform_read_random_register() accessor(s) . This would be utter chaos.

...

...

+BUILD_PLAT_ACCESSORS(BOSTON_PLAT_CORE_CL, core_cl) +BUILD_PLAT_ACCESSORS(BOSTON_PLAT_MMCMDIV, mmcmdiv) +BUILD_PLAT_ACCESSORS(BOSTON_PLAT_DDRCONF0, ddrconf0)

+#endif /* !__ASSEMBLY__ */

+#endif /* __BOARD_BOSTON_REGS_H__ */ diff --git a/board/imgtec/boston/checkboard.c b/board/imgtec/boston/checkboard.c new file mode 100644 index 0000000..417ac4e --- /dev/null +++ b/board/imgtec/boston/checkboard.c @@ -0,0 +1,29 @@ +/*

• • Copyright (C) 2016 Imagination Technologies
• • SPDX-License-Identifier: GPL-2.0
• */

+#include <common.h>

+#include <asm/mipsregs.h>

+#include "boston-lcd.h" +#include "boston-regs.h"

+int checkboard(void) +{

• u32 changelist;
• lowlevel_display("U-boot ");
• printf("Board: MIPS Boston\n");
• printf("CPU: 0x%08x", read_c0_prid());

This should be in print_cpuinfo()

I don't agree. This goes on to read a board-specific register to determine information about the CPU (the revision of its RTL) and that should not be done in arch-level code, which is what every other implementation of print_cpuinfo is.

Ah, so the register used to determine CPU info is board-specific ? That is utterly braindead design in my mind. The read_c0_prid() looked like it is reading some standard register, maybe that's not true ...

Perhaps it would be better if we had a nice DM-using CPU driver which Boston could have an extension of, but we don't have that for MIPS right now & this series is not the place to add it.

...

...

• changelist = read_boston_core_cl();
• if (changelist > 1)

•    printf(" cl%x", changelist);
  

• putc('\n');
• return 0;

+} diff --git a/board/imgtec/boston/ddr.c b/board/imgtec/boston/ddr.c new file mode 100644 index 0000000..7caed4b --- /dev/null +++ b/board/imgtec/boston/ddr.c @@ -0,0 +1,30 @@ +/*

• • Copyright (C) 2016 Imagination Technologies
• • SPDX-License-Identifier: GPL-2.0
• */

+#include <common.h>

+#include <asm/addrspace.h>

+#include "boston-regs.h"

+phys_size_t initdram(int board_type) +{

• u32 ddrconf0 = read_boston_ddrconf0();
• return (phys_size_t)(ddrconf0 & BOSTON_PLAT_DDRCONF0_SIZE) << 30;

+}

+ulong board_get_usable_ram_top(ulong total_size) +{

• DECLARE_GLOBAL_DATA_PTR;
• if (gd->ram_top < CONFIG_SYS_SDRAM_BASE) {

•    /* 2GB wrapped around to 0 */
  

•    return CKSEG0ADDR(256 << 20);
  

• }
• return min_t(unsigned long, gd->ram_top, CKSEG0ADDR(256 << 20));

+} diff --git a/board/imgtec/boston/lowlevel_init.S b/board/imgtec/boston/lowlevel_init.S new file mode 100644 index 0000000..8928172 --- /dev/null +++ b/board/imgtec/boston/lowlevel_init.S @@ -0,0 +1,56 @@ +/*

• • Copyright (C) 2016 Imagination Technologies
• • SPDX-License-Identifier: GPL-2.0
• */

+#include <config.h>

+#include <asm/addrspace.h> +#include <asm/asm.h> +#include <asm/mipsregs.h> +#include <asm/regdef.h>

+#include "boston-regs.h"

+.data

+msg_ddr_cal: .ascii "DDR Cal " +msg_ddr_ok: .ascii "DDR OK "

+.text

+LEAF(lowlevel_init)

• move s0, ra
• PTR_LA a0, msg_ddr_cal
• bal lowlevel_display

Don't you need nop after branch on mips ?

No. Branch delay slots are filled automatically by the assembler unless you explicitly tell it not to with a ".set noreorder" directive, and they're not just filled with NOPs - they can contain essentially any non-control-flow-affecting instruction that it's safe to execute regardless of the outcome of the branch. They're also somewhat optional in MIPSr6 where "compact" branches don't have delay slots, and gone entirely in microMIPSr6.

Oh ok.

...

...

• PTR_LI t0, CKSEG1ADDR(BOSTON_PLAT_BASE)

+1: lw t1, BOSTON_PLAT_DDR3STAT(t0)

• andi t1, t1, BOSTON_PLAT_DDR3STAT_CALIB
• beqz t1, 1b

On 07/31/2016 07:32 PM, Paul Burton wrote:

On 31/07/16 16:56, Marek Vasut wrote:

...

On 07/29/2016 10:36 AM, Paul Burton wrote: [...]

...

...

...

+#ifndef __ASSEMBLY__

+#include <asm/io.h>

+#define BUILD_PLAT_ACCESSORS(offset, name) \ +static inline uint32_t read_boston_##name(void) \ +{ \

• uint32_t *reg = (void *)CKSEG1ADDR(BOSTON_PLAT_BASE) + (offset);\
• return __raw_readl(reg); \

+}

Don't we have enough standard accessors to confuse people ? Why do you add another custom ones ? Remove this and just use standard accessors throughout the code.

Hi Marek,

These accessors are simple wrappers around __raw_readl, I'd hardly say they can be considered confusing. The alternative is lots of:

val = __raw_readl((void *)CKSEG1ADDR(BOSTON_PLAT_BASE) + OFFSET);

...and that is just plain ugly.

This should be map_physmem() + readl(), see ie. the ag7xxx.c driver or whatever other stuff from the atheros ath79 port. Does this work ?

Yes this works. I suggest you read about the MIPS memory map if you wish to critique this code.

What am I missing ?

...

...

Invoking readl on a field of a struct representing these registers would be nice, but some of them need to be accessed from assembly so that would involve duplication which isn't nice.

The struct based access is deprecated, don't bother with it.

...

I think this way is the best option, where if you want to read the Boston core_cl register you call read_boston_core_cl() - it's hardly confusing what that does.

Now imagine what would happen if everyone introduced his own my_platform_read_random_register() accessor(s) . This would be utter chaos.

You speak as though this patch introduces new general purpose accessor functions that perform some arbitrary memory read. It does not.

Yes it does, the accessor is globally available.

It introduces functions each of which reads a single register in the only sane way to read that register, via the standard __raw_readl. It does so in a pretty well namespaced manner & with names that match the register names of the platform. If everyone were to do that I fail to see what the problem would be.

Say you want to find all register accesses -- with random functions with ad-hoc names, you cannot do simple git grep, you need to grep for these ad-hoc functions as well ... but they won't show up, since there is also preprocessor string concatenation, which further obfuscates things and makes it unpleasant to work with.

In my opinion, this macro has no value.

...

...

...

...

+BUILD_PLAT_ACCESSORS(BOSTON_PLAT_CORE_CL, core_cl) +BUILD_PLAT_ACCESSORS(BOSTON_PLAT_MMCMDIV, mmcmdiv) +BUILD_PLAT_ACCESSORS(BOSTON_PLAT_DDRCONF0, ddrconf0)

+#endif /* !__ASSEMBLY__ */

+#endif /* __BOARD_BOSTON_REGS_H__ */ diff --git a/board/imgtec/boston/checkboard.c b/board/imgtec/boston/checkboard.c new file mode 100644 index 0000000..417ac4e --- /dev/null +++ b/board/imgtec/boston/checkboard.c @@ -0,0 +1,29 @@ +/*

• • Copyright (C) 2016 Imagination Technologies
• • SPDX-License-Identifier: GPL-2.0
• */

+#include <common.h>

+#include <asm/mipsregs.h>

+#include "boston-lcd.h" +#include "boston-regs.h"

+int checkboard(void) +{

• u32 changelist;
• lowlevel_display("U-boot ");
• printf("Board: MIPS Boston\n");
• printf("CPU: 0x%08x", read_c0_prid());

This should be in print_cpuinfo()

I don't agree. This goes on to read a board-specific register to determine information about the CPU (the revision of its RTL) and that should not be done in arch-level code, which is what every other implementation of print_cpuinfo is.

Ah, so the register used to determine CPU info is board-specific ? That is utterly braindead design in my mind. The read_c0_prid() looked like it is reading some standard register, maybe that's not true ...

read_c0_prid() is generic, it's the read_boston_core_cl() that is board-specific & used to print the CPU's RTL revision, as I described with "goes on to...".

So this stuff should be in print_cpuinfo() if it's generic.

I disagree that this is a bad design. It's pretty logical that an FPGA based development platform might wish to expose more information about the CPU loaded on it, such as its RTL revision, than that CPU would expose in general use.

I am fine with this, you can print an ascii-art pikachu there if you want. But board info should go into show_board_info() and CPU info should be in print_cpuinfo() .

You can insult the design of the system all you like if it makes you feel better. However, if you expect me to pay any attention to your opinions then I suggest that you'd do better to make an effort to understand the system rather than than spewing insulting words & false assertions about memory accesses being broken or branches being incorrectly written.

I am trying to wrap my mind around the design, sorry if that sounded like I'm trying to step on your toys.

Thanks, Paul

On 08/02/2016 03:12 PM, Paul Burton wrote:

On 01/08/16 19:36, Marek Vasut wrote:

...

On 07/31/2016 07:32 PM, Paul Burton wrote:

...

On 31/07/16 16:56, Marek Vasut wrote:

...

On 07/29/2016 10:36 AM, Paul Burton wrote: [...]

...

...

> +#ifndef __ASSEMBLY__ > + > +#include <asm/io.h> > + > +#define BUILD_PLAT_ACCESSORS(offset, name) \ > +static inline uint32_t read_boston_##name(void) \ > +{ \ > + uint32_t *reg = (void *)CKSEG1ADDR(BOSTON_PLAT_BASE) + > (offset);\ > + return __raw_readl(reg); \ > +}

Don't we have enough standard accessors to confuse people ? Why do you add another custom ones ? Remove this and just use standard accessors throughout the code.

Hi Marek,

These accessors are simple wrappers around __raw_readl, I'd hardly say they can be considered confusing. The alternative is lots of:

val = __raw_readl((void *)CKSEG1ADDR(BOSTON_PLAT_BASE) + OFFSET);

...and that is just plain ugly.

This should be map_physmem() + readl(), see ie. the ag7xxx.c driver or whatever other stuff from the atheros ath79 port. Does this work ?

Yes this works. I suggest you read about the MIPS memory map if you wish to critique this code.

What am I missing ?

Hi Marek,

You're missing that in MIPS the virtual address space includes the unmapped regions kseg0 & kseg1. To perform uncached access to a physical address beneath 512MB one can simply use it as an offset into kseg1, with no need to perform any mapping.

The map_physmem() does this translation, no ?

...

...

...

...

Invoking readl on a field of a struct representing these registers would be nice, but some of them need to be accessed from assembly so that would involve duplication which isn't nice.

The struct based access is deprecated, don't bother with it.

...

I think this way is the best option, where if you want to read the Boston core_cl register you call read_boston_core_cl() - it's hardly confusing what that does.

Now imagine what would happen if everyone introduced his own my_platform_read_random_register() accessor(s) . This would be utter chaos.

You speak as though this patch introduces new general purpose accessor functions that perform some arbitrary memory read. It does not.

Yes it does, the accessor is globally available.

They're only available if you include boston-regs.h which lives inside board/imgtec/boston/, and regardless their availability does not make them general purpose. Each accesses only a single register in a single way. That is not a general purpose accessor like readl, __raw_readl, inl or whatever else - indeed it's built using the standard __raw_readl.

OK, I see we have two stubborn people bashing heads against one another. I'll leave this decision about this accessor thing to Dan if you don't mind.

...

...

It introduces functions each of which reads a single register in the only sane way to read that register, via the standard __raw_readl. It does so in a pretty well namespaced manner & with names that match the register names of the platform. If everyone were to do that I fail to see what the problem would be.

Say you want to find all register accesses -- with random functions with ad-hoc names, you cannot do simple git grep, you need to grep for these ad-hoc functions as well ... but they won't show up, since there is also preprocessor string concatenation, which further obfuscates things and makes it unpleasant to work with.

In my opinion, this macro has no value.

I disagree & find it rather pleasant to use with minimal costs, but given that there are only 2 such register accesses left since the clock changes in v2 I've removed it.

...

...

...

...

...

> +BUILD_PLAT_ACCESSORS(BOSTON_PLAT_CORE_CL, core_cl) > +BUILD_PLAT_ACCESSORS(BOSTON_PLAT_MMCMDIV, mmcmdiv) > +BUILD_PLAT_ACCESSORS(BOSTON_PLAT_DDRCONF0, ddrconf0) > + > +#endif /* !__ASSEMBLY__ */ > + > +#endif /* __BOARD_BOSTON_REGS_H__ */ > diff --git a/board/imgtec/boston/checkboard.c > b/board/imgtec/boston/checkboard.c > new file mode 100644 > index 0000000..417ac4e > --- /dev/null > +++ b/board/imgtec/boston/checkboard.c > @@ -0,0 +1,29 @@ > +/* > + * Copyright (C) 2016 Imagination Technologies > + * > + * SPDX-License-Identifier: GPL-2.0 > + */ > + > +#include <common.h> > + > +#include <asm/mipsregs.h> > + > +#include "boston-lcd.h" > +#include "boston-regs.h" > > +int checkboard(void) > +{ > + u32 changelist; > + > + lowlevel_display("U-boot "); > + > + printf("Board: MIPS Boston\n"); > + > + printf("CPU: 0x%08x", read_c0_prid());

This should be in print_cpuinfo()

I don't agree. This goes on to read a board-specific register to determine information about the CPU (the revision of its RTL) and that should not be done in arch-level code, which is what every other implementation of print_cpuinfo is.

Ah, so the register used to determine CPU info is board-specific ? That is utterly braindead design in my mind. The read_c0_prid() looked like it is reading some standard register, maybe that's not true ...

read_c0_prid() is generic, it's the read_boston_core_cl() that is board-specific & used to print the CPU's RTL revision, as I described with "goes on to...".

So this stuff should be in print_cpuinfo() if it's generic.

...

I disagree that this is a bad design. It's pretty logical that an FPGA based development platform might wish to expose more information about the CPU loaded on it, such as its RTL revision, than that CPU would expose in general use.

I am fine with this, you can print an ascii-art pikachu there if you want. But board info should go into show_board_info() and CPU info should be in print_cpuinfo() .

You don't seem to understand what I'm saying: this is information about the CPU but provided by the board.

Well this is kinda weird, yes.

This could be tidied up at some point if we had some way for the arch code to print basic CPU info & the board to extend it, but that isn't in place. I think potentially the neatest way to handle this would be via a CPU driver, which we don't yet have.

No point in overcomplicating things.

I don't think this series, which has already grown to include various generic changes, is the place to do that.

ok

Thanks, Paul

...

...

You can insult the design of the system all you like if it makes you feel better. However, if you expect me to pay any attention to your opinions then I suggest that you'd do better to make an effort to understand the system rather than than spewing insulting words & false assertions about memory accesses being broken or branches being incorrectly written.

I am trying to wrap my mind around the design, sorry if that sounded like I'm trying to step on your toys.

...

Thanks, Paul