Re: [PATCH 1/1] powerpc: mpc85xx: Add board support for ucp1020

[Scott Wood <scottwood@freescale.com>]

On Tue, 2015-05-05 at 11:52 -0400, Oleksandr G Zhadan wrote:
> New QorIQ p1020 based board support from Arcturus Networks Inc.
> http://www.arcturusnetworks.com/products/ucp1020/
> 
> Signed-off-by: Michael Durrant <mdurrant@arcturusnetworks.com>
> Signed-off-by: Oleksandr G Zhadan <oleks@arcturusnetworks.com>
> ---
>  Documentation/devicetree/bindings/pci/fsl,pci.txt  |    2 +-
>  .../devicetree/bindings/powerpc/arcturus/board.txt |  149 ++
>  .../devicetree/bindings/powerpc/arcturus/ecm.txt   |   64 +
>  Documentation/devicetree/bindings/usb/fsl-usb.txt  |    2 +-
>  .../devicetree/bindings/vendor-prefixes.txt        |    1 +
>  arch/powerpc/boot/dts/fsl/ucp1020som-post.dtsi     |  179 ++
>  arch/powerpc/boot/dts/fsl/ucp1020som-pre.dtsi      |   70 +
>  arch/powerpc/boot/dts/ucp1020_32b.dts              |   88 +
>  arch/powerpc/boot/dts/ucp1020_32b.dtsi             |  174 ++
>  arch/powerpc/configs/ucp1020_defconfig             | 2731 ++++++++++++++++++++
>  arch/powerpc/platforms/85xx/Kconfig                |    7 +
>  arch/powerpc/platforms/85xx/Makefile               |    1 +
>  arch/powerpc/platforms/85xx/ucp1020_som.c          |  100 +
>  13 files changed, 3566 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/powerpc/arcturus/board.txt
>  create mode 100644 Documentation/devicetree/bindings/powerpc/arcturus/ecm.txt
>  create mode 100644 arch/powerpc/boot/dts/fsl/ucp1020som-post.dtsi
>  create mode 100644 arch/powerpc/boot/dts/fsl/ucp1020som-pre.dtsi
>  create mode 100644 arch/powerpc/boot/dts/ucp1020_32b.dts
>  create mode 100644 arch/powerpc/boot/dts/ucp1020_32b.dtsi
>  create mode 100644 arch/powerpc/configs/ucp1020_defconfig
>  create mode 100644 arch/powerpc/platforms/85xx/ucp1020_som.c
> 
> diff --git a/Documentation/devicetree/bindings/pci/fsl,pci.txt b/Documentation/devicetree/bindings/pci/fsl,pci.txt
> index d8ac4a7..298a5e6 100644
> --- a/Documentation/devicetree/bindings/pci/fsl,pci.txt
> +++ b/Documentation/devicetree/bindings/pci/fsl,pci.txt
> @@ -20,7 +20,7 @@ Example:
>  		#interrupt-cells = <1>;
>  		#size-cells = <2>;
>  		#address-cells = <3>;
> -		compatible = "fsl,mpc8540-pcix", "fsl,mpc8540-pci";
> +		compatible = "fsl,mpc8540-pcix", "fsl,mpc8540-pci", "fsl,mpc8548-pcie";
>  		device_type = "pci";
>  		...
>  		...
> diff --git a/Documentation/devicetree/bindings/powerpc/arcturus/board.txt b/Documentation/devicetree/bindings/powerpc/arcturus/board.txt
> new file mode 100644
> index 0000000..54e9765
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/powerpc/arcturus/board.txt
> @@ -0,0 +1,149 @@
> +UCP1020 module Tree Bindings
> +----------------------------
> +
> +Copyright 2013-2015 Arcturus Networks, Inc.
> +
> +QorIQ p1020 based board
> +http://www.arcturusnetworks.com/products/ucp1020/
> +-------------------------------------------------
> +
> +Root Module
> +
> +Properties:
> +- model:		"arcturus,uCP1020"
> +- compatible:		"arcturus,uCP1020"
> +- SN:			"1234567890-1234"
> +
> +/ {
> +	model = "arcturus,uCP1020";
> +	compatible = "arcturus,uCP1020", "fsl,P1020";
> +	SN = "1234567890-1234";
> +	...
> +  }

Drop the "fsl,P1020" compatible.  Top-level compatible strings describe
the whole board.

SN is a bad property name.  Call it something like "arcturus,serial#",
and define what it actually means rather than just giving an example.

> +-------------------------------------------------
> +
> +P1020 SPI controller
> +
> +Properties:
> +- compatible:		"spansion,s25fl008k", "winbond,w25q80bl"
> +
> +Example:
> +	spi@7000 {
> +		flash@0 {
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			compatible = "spansion,s25fl008k", "winbond,w25q80bl";
> +			reg = <0>;
> +			spi-max-frequency = <40000000>; /* input clock */
> +			...
> +		};

This isn't describing the controller, but rather a SPI chip attached to
the controller.  This also doesn't seem like the right place for random
SPI chips.

If all you're specifying is the compatible, maybe create a
spi/trivial-devices.txt similar to i2c/trivial-devices.txt?  Or
something specific to SPI flash chips to describe the partition
specification, though I generally recommend against describing
partitions in the device tree -- especially if this is a developer board
rather than something fixed-purpose where the partitioning is not going
to change based on user requirements.


> +-------------------------------------------------
> +
> +Chipselect/Local Bus
> +
> +Properties:
> +- #address-cells:	<2>.
> +- #size-cells:		<1>.
> +- compatible:		"fsl,p1020-elbc", "fsl,elbc", "simple-bus","fsl,p1020-immr"
> +- interrupts:		interrupts to report localbus events.
> +
> +Example:
> +
> +&lbc {
> +	#address-cells = <2>;
> +	#size-cells = <1>;
> +	compatible = "fsl,p1020-elbc", "fsl,elbc", "simple-bus";
> +	interrupts = <19 2 0 0>;
> +};

There's already a binding for elbc -- and the elbc node certainly should
not claim compatibility with "fsl,p1020-immr".


> +-------------------------------------------------
> +
> +Freescale L2 Cache Controller
> +
> +L2 cache is present in Freescale's QorIQ and QorIQ Qonverge platforms.
> +The cache bindings explained below are ePAPR compliant
> +
> +Properties:
> +
> +- compatible	: Should include "fsl,chip-l2-cache-controller" and "cache"
> +		  where chip is the processor (bsc9132, npc8572 etc.)
> +- reg		: Address and size of L2 cache controller registers
> +- cache-size	: Size of the entire L2 cache
> +- interrupts	: Error interrupt of L2 controller
> +- cache-line-size : Size of L2 cache lines
> +
> +Example:
> +
> +	L2: l2-cache-controller@20000 {
> +		compatible = "fsl,p1020-l2-cache-controller", "cache";
> +		reg = <0x20000 0x1000>;
> +		cache-line-size = <32>; // 32 bytes
> +		cache-size = <0x40000>; // L2,256K
> +		interrupts = <16 2 1 0>;
> +	};

This is copied and pasted from the existing binding.  Ignore
checkpatch's inability to understand that "fsl,chip-l2-cache-controller"
includes "fsl,p1020-l2-cache-controller".

Likewise for the other bindings.  If you're going to have a binding
document for your board, it should only talk about things that are
specific to your board.

> diff --git a/Documentation/devicetree/bindings/usb/fsl-usb.txt b/Documentation/devicetree/bindings/usb/fsl-usb.txt
> index 4779c02..a2663ad 100644
> --- a/Documentation/devicetree/bindings/usb/fsl-usb.txt
> +++ b/Documentation/devicetree/bindings/usb/fsl-usb.txt
> @@ -57,7 +57,7 @@ Example multi port host USB controller device node :
>  
>  Example dual role USB controller device node :
>  	usb@23000 {
> -		compatible = "fsl-usb2-dr";
> +		compatible = "fsl-usb2-dr", "fsl-usb2-dr-v1.6";
>  		reg = <23000 1000>;
>  		#address-cells = <1>;
>  		#size-cells = <0>;

More specific compatibles come first.

> diff --git a/arch/powerpc/boot/dts/fsl/ucp1020som-post.dtsi b/arch/powerpc/boot/dts/fsl/ucp1020som-post.dtsi
> new file mode 100644
> index 0000000..930a6e3
> --- /dev/null
> +++ b/arch/powerpc/boot/dts/fsl/ucp1020som-post.dtsi

Why can't you use p1020si-post.dtsi?  The "si" means "silicon" -- it's
meant to be included by all p1020 boards.

> diff --git a/arch/powerpc/boot/dts/ucp1020_32b.dts b/arch/powerpc/boot/dts/ucp1020_32b.dts
> new file mode 100644
> index 0000000..4a8358c
> --- /dev/null
> +++ b/arch/powerpc/boot/dts/ucp1020_32b.dts
> @@ -0,0 +1,88 @@
> +/*
> + * uCP1020 Tree Source (32-bit address map)

Do you plan on supporting both 32 and 36 bits?  If not, leave off the
"_32b".

> diff --git a/arch/powerpc/configs/ucp1020_defconfig b/arch/powerpc/configs/ucp1020_defconfig
> new file mode 100644
> index 0000000..62f99aa
> --- /dev/null
> +++ b/arch/powerpc/configs/ucp1020_defconfig

Please explain why your board needs its own defconfig.

Plus, all defconfigs need to be run through savedefconfig to trim
everything that doesn't change a default.

> diff --git a/arch/powerpc/platforms/85xx/ucp1020_som.c b/arch/powerpc/platforms/85xx/ucp1020_som.c
> new file mode 100644
> index 0000000..aa98d31
> --- /dev/null
> +++ b/arch/powerpc/platforms/85xx/ucp1020_som.c
> @@ -0,0 +1,100 @@
> +/*
> + * Arcturus Networks Inc. uCP1020 system on module Setup
> + *
> + * Copyright 2014-2015 Arcturus Networks Inc.
> + *
> + * by Oleksandr G Zhadan & Michael Durrant (www.ArcturusNetworks.com)
> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + */
> +
> +#include <linux/stddef.h>
> +#include <linux/kernel.h>
> +#include <linux/pci.h>
> +#include <linux/kdev_t.h>
> +#include <linux/delay.h>
> +#include <linux/seq_file.h>
> +#include <linux/interrupt.h>
> +#include <linux/of_platform.h>
> +
> +#include <asm/time.h>
> +#include <asm/machdep.h>
> +#include <asm/pci-bridge.h>
> +#include <mm/mmu_decl.h>
> +#include <asm/prom.h>
> +#include <asm/udbg.h>
> +#include <asm/mpic.h>
> +#include <asm/fsl_guts.h>
> +
> +#include <sysdev/fsl_soc.h>
> +#include <sysdev/fsl_pci.h>
> +#include "smp.h"
> +
> +#include "mpc85xx.h"

You don't need all of these headers.

> +
> +void __init ucp1020_som_pic_init(void)
> +{
> +	struct mpic *mpic;
> +	unsigned long root = of_get_flat_dt_root();
> +
> +	if (of_flat_dt_is_compatible(root, "fsl,MPC85XXRDB-CAMP")) {
> +		mpic = mpic_alloc(NULL, 0, MPIC_NO_RESET |
> +				  MPIC_BIG_ENDIAN |
> +				  MPIC_SINGLE_DEST_CPU, 0, 256, " OpenPIC  ");
> +	} else {
> +		mpic = mpic_alloc(NULL, 0,
> +				  MPIC_BIG_ENDIAN |
> +				  MPIC_SINGLE_DEST_CPU, 0, 256, " OpenPIC  ");
> +	}

Why do you have fsl,MPC85XXRDB-CAMP in here?

-Scott