diff mbox

[v1,1/5] ARM: imx28: add basic dt support

Message ID 1331628428-24017-2-git-send-email-b29396@freescale.com
State New
Headers show

Commit Message

Dong Aisheng March 13, 2012, 8:47 a.m. UTC
From: Dong Aisheng <dong.aisheng@linaro.org>

This patch includes basic dt support which can boot via nfs rootfs.

Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
---
 Documentation/devicetree/bindings/arm/fsl.txt |    4 +
 arch/arm/boot/dts/imx28-evk.dts               |   31 +++++++++
 arch/arm/boot/dts/imx28.dtsi                  |   88 +++++++++++++++++++++++++
 arch/arm/mach-mxs/Kconfig                     |    9 +++
 arch/arm/mach-mxs/Makefile                    |    1 +
 arch/arm/mach-mxs/imx28-dt.c                  |   67 +++++++++++++++++++
 6 files changed, 200 insertions(+), 0 deletions(-)

Comments

Rob Herring March 13, 2012, 2:35 p.m. UTC | #1
On 03/13/2012 03:47 AM, Dong Aisheng wrote:
> From: Dong Aisheng <dong.aisheng@linaro.org>
> 
> This patch includes basic dt support which can boot via nfs rootfs.
> 
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> ---
>  Documentation/devicetree/bindings/arm/fsl.txt |    4 +
>  arch/arm/boot/dts/imx28-evk.dts               |   31 +++++++++
>  arch/arm/boot/dts/imx28.dtsi                  |   88 +++++++++++++++++++++++++
>  arch/arm/mach-mxs/Kconfig                     |    9 +++
>  arch/arm/mach-mxs/Makefile                    |    1 +
>  arch/arm/mach-mxs/imx28-dt.c                  |   67 +++++++++++++++++++
>  6 files changed, 200 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/fsl.txt b/Documentation/devicetree/bindings/arm/fsl.txt
> index 54bddda..9f21faf 100644
> --- a/Documentation/devicetree/bindings/arm/fsl.txt
> +++ b/Documentation/devicetree/bindings/arm/fsl.txt
> @@ -1,6 +1,10 @@
>  Freescale i.MX Platforms Device Tree Bindings
>  -----------------------------------------------
>  
> +i.MX28 Evaluation Kit
> +Required root node properties:
> +    - compatible = "fsl,imx28-evk", "fsl,imx28";
> +
>  i.MX51 Babbage Board
>  Required root node properties:
>      - compatible = "fsl,imx51-babbage", "fsl,imx51";
> diff --git a/arch/arm/boot/dts/imx28-evk.dts b/arch/arm/boot/dts/imx28-evk.dts
> new file mode 100644
> index 0000000..9758dc4
> --- /dev/null
> +++ b/arch/arm/boot/dts/imx28-evk.dts
> @@ -0,0 +1,31 @@
> +/*
> + * Copyright 2012 Freescale Semiconductor, Inc.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +/dts-v1/;
> +/include/ "imx28.dtsi"
> +
> +/ {
> +	model = "Freescale i.MX28 Evaluation Kit";
> +	compatible = "fsl,imx28-evk", "fsl,imx28";
> +
> +	memory {
> +		device_type = "memory";
> +		reg = <0x40000000 0x08000000>;
> +	};
> +
> +	ahb@80080000 {
> +		fec@800f0000 {

Use generic names: ethernet@800f0000

> +			phy-mode = "rmii";
> +			local-mac-address = [00 04 9F 01 7D 5B];
> +			status = "okay";
> +		};
> +	};
> +};
> diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
> new file mode 100644
> index 0000000..acf0dab
> --- /dev/null
> +++ b/arch/arm/boot/dts/imx28.dtsi
> @@ -0,0 +1,88 @@
> +/*
> + * Copyright 2012 Freescale Semiconductor, Inc.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +/include/ "skeleton.dtsi"
> +
> +/ {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	interrupt-parent = <&icoll>;
> +
> +	aliases {
> +		serial0 = &uart1;
> +	};
> +
> +	cpus {
> +		cpu@0 {
> +			compatible = "arm,arm926ejs";
> +		};
> +	};
> +
> +	apb@80000000 {
> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		reg = <0x80000000 0x80000>;
> +		ranges;
> +
> +		apbh@80000000 {
> +			compatible = "simple-bus";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			reg = <0x80000000 0x3c900>;
> +			ranges;
> +
> +			icoll: interrupt-controller@80000000 {
> +				compatible = "fsl,imx28-icoll";
> +				interrupt-controller;
> +				#interrupt-cells = <1>;
> +				reg = <0x80000000 0x2000>;
> +			};
> +		};
> +
> +		apbx@80040000 {
> +			compatible = "simple-bus";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			reg = <0x80040000 0x40000>;
> +			ranges;
> +
> +			uart1: uart@80074000 {

Use generic names: uart1: serial@...

> +				compatible = "arm,pl011", "arm,primecell";
> +				reg = <0x80074000 0x2000>;

This is really only 0x1000 in length.

> +				interrupts = <47>;
> +			};
> +		};
> +	};
> +
> +	ahb@80080000 {
> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		reg = <0x80080000 0x80000>;
> +		ranges;
> +
> +		fec@800f0000 {
> +			compatible = "fsl,imx28-fec";
> +			reg = <0x800f0000 0x4000>;

This too IIRC.

> +			interrupts = <101>;
> +			status = "disabled";
> +		};
> +
> +		fec@800f4000 {
> +			compatible = "fsl,imx28-fec";
> +			reg = <0x800f4000 0x4000>;
> +			interrupts = <102>;
> +			status = "disabled";
> +		};
> +
> +	};
> +};
> diff --git a/arch/arm/mach-mxs/Kconfig b/arch/arm/mach-mxs/Kconfig
> index c57f996..6ab66af 100644
> --- a/arch/arm/mach-mxs/Kconfig
> +++ b/arch/arm/mach-mxs/Kconfig
> @@ -17,6 +17,15 @@ config SOC_IMX28
>  
>  comment "MXS platforms:"
>  
> +config MACH_IMX28_DT

Perhaps this should be more generic like MACH_MXS_DT to eventually
support MX23 as well?

> +	bool "Support i.MX28 platforms from device tree"
> +	select SOC_IMX28
> +	select USE_OF
> +	select MACH_MX28EVK

This should not be selected here.

> +	help
> +	  Include support for Freescale i.MX28 based platforms
> +	  using the device tree for discovery
> +
>  config MACH_STMP378X_DEVB
>  	bool "Support STMP378x_devb Platform"
>  	select SOC_IMX23
> diff --git a/arch/arm/mach-mxs/Makefile b/arch/arm/mach-mxs/Makefile
> index 908bf9a..25a4122 100644
> --- a/arch/arm/mach-mxs/Makefile
> +++ b/arch/arm/mach-mxs/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_PM) += pm.o
>  obj-$(CONFIG_SOC_IMX23) += clock-mx23.o
>  obj-$(CONFIG_SOC_IMX28) += clock-mx28.o
>  
> +obj-$(CONFIG_MACH_IMX28_DT) += imx28-dt.o
>  obj-$(CONFIG_MACH_STMP378X_DEVB) += mach-stmp378x_devb.o
>  obj-$(CONFIG_MACH_MX23EVK) += mach-mx23evk.o
>  obj-$(CONFIG_MACH_MX28EVK) += mach-mx28evk.o
> diff --git a/arch/arm/mach-mxs/imx28-dt.c b/arch/arm/mach-mxs/imx28-dt.c
> new file mode 100644
> index 0000000..78d1129
> --- /dev/null
> +++ b/arch/arm/mach-mxs/imx28-dt.c
> @@ -0,0 +1,67 @@
> +/*
> + * Copyright 2011 Freescale Semiconductor, Inc.
> + * Copyright 2011 Linaro Ltd.

It's 2012 now.

> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +#include <linux/init.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <asm/mach/arch.h>
> +#include <asm/mach/time.h>
> +#include <mach/common.h>
> +#include <mach/mx28.h>
> +
> +static const struct of_dev_auxdata imx28_auxdata_lookup[] __initconst = {
> +	OF_DEV_AUXDATA("arm,pl011", MX28_DUART_BASE_ADDR, "duart", NULL),
> +	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC0_BASE_ADDR, "imx28-fec.0", NULL),
> +	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC1_BASE_ADDR, "imx28-fec.1", NULL),

Why do you need the old names?

> +	{ /* sentinel */ }
> +};
> +
> +static void __init imx28_init(void)
> +{
> +	of_platform_populate(NULL, of_default_bus_match_table,
> +			     imx28_auxdata_lookup, NULL);
> +}
> +
> +static const struct of_device_id icoll_of_match[] __initconst = {
> +	{ .compatible = "fsl,imx28-icoll", },
> +	{},
> +};
> +
> +static void __init imx28_dt_init_irq(void)
> +{
> +	irq_domain_generate_simple(icoll_of_match, MX28_ICOLL_BASE_ADDR, 0);

Please do "proper" domain support for the interrupt controller and use
of_irq_init.

> +	mx28_init_irq();
> +}
> +
> +static void __init imx28_timer_init(void)
> +{
> +	mx28_clocks_init();
> +}
> +
> +static struct sys_timer imx28_timer = {
> +	.init = imx28_timer_init,
> +};
> +
> +static const char *imx28_dt_compat[] __initdata = {
> +	"fsl,imx28-evk",
> +	NULL,
> +};
> +
> +DT_MACHINE_START(IMX28, "Freescale i.MX28 (Device Tree)")
> +	.map_io		= mx28_map_io,
> +	.init_irq	= imx28_dt_init_irq,
> +	.timer		= &imx28_timer,
> +	.init_machine	= imx28_init,
> +	.dt_compat	= imx28_dt_compat,
> +	.restart	= mxs_restart,
> +MACHINE_END
Zach Sadecki March 13, 2012, 2:59 p.m. UTC | #2
On 03/13/2012 09:35 AM, Rob Herring wrote:
> +	ahb@80080000 {
> +		fec@800f0000 {
> Use generic names: ethernet@800f0000
Generic is good, but consistency is better, IMHO.  grepping existing dts 
files in 3.2.9 finds 6 instances of "fec@" and 0 instances of "ethernet@"
>> +			uart1: uart@80074000 {
> Use generic names: uart1: serial@...
Same comment here, but unfortunately there is already inconsistency in 
existing files...  25 instances of "serial@" and 35 instances of "uart@"
Grant Likely March 13, 2012, 5:23 p.m. UTC | #3
On Tue, 13 Mar 2012 16:47:04 +0800, Dong Aisheng <b29396@freescale.com> wrote:
> From: Dong Aisheng <dong.aisheng@linaro.org>
> 
> This patch includes basic dt support which can boot via nfs rootfs.
> 
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> ---
>  Documentation/devicetree/bindings/arm/fsl.txt |    4 +
>  arch/arm/boot/dts/imx28-evk.dts               |   31 +++++++++
>  arch/arm/boot/dts/imx28.dtsi                  |   88 +++++++++++++++++++++++++
>  arch/arm/mach-mxs/Kconfig                     |    9 +++
>  arch/arm/mach-mxs/Makefile                    |    1 +
>  arch/arm/mach-mxs/imx28-dt.c                  |   67 +++++++++++++++++++
>  6 files changed, 200 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/fsl.txt b/Documentation/devicetree/bindings/arm/fsl.txt
> index 54bddda..9f21faf 100644
> --- a/Documentation/devicetree/bindings/arm/fsl.txt
> +++ b/Documentation/devicetree/bindings/arm/fsl.txt
> @@ -1,6 +1,10 @@
>  Freescale i.MX Platforms Device Tree Bindings
>  -----------------------------------------------
>  
> +i.MX28 Evaluation Kit
> +Required root node properties:
> +    - compatible = "fsl,imx28-evk", "fsl,imx28";
> +
>  i.MX51 Babbage Board
>  Required root node properties:
>      - compatible = "fsl,imx51-babbage", "fsl,imx51";
> diff --git a/arch/arm/boot/dts/imx28-evk.dts b/arch/arm/boot/dts/imx28-evk.dts
> new file mode 100644
> index 0000000..9758dc4
> --- /dev/null
> +++ b/arch/arm/boot/dts/imx28-evk.dts
> @@ -0,0 +1,31 @@
> +/*
> + * Copyright 2012 Freescale Semiconductor, Inc.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +/dts-v1/;
> +/include/ "imx28.dtsi"
> +
> +/ {
> +	model = "Freescale i.MX28 Evaluation Kit";
> +	compatible = "fsl,imx28-evk", "fsl,imx28";
> +
> +	memory {
> +		device_type = "memory";
> +		reg = <0x40000000 0x08000000>;
> +	};
> +
> +	ahb@80080000 {
> +		fec@800f0000 {
> +			phy-mode = "rmii";
> +			local-mac-address = [00 04 9F 01 7D 5B];

Generally a bad idea to put a specific mac address into the device tree.
Better to fill it with zeros.  Otherwise all the dev boards will end up using
the same value.

> +			status = "okay";
> +		};
> +	};
> +};
> diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
> new file mode 100644
> index 0000000..acf0dab
> --- /dev/null
> +++ b/arch/arm/boot/dts/imx28.dtsi
> @@ -0,0 +1,88 @@
> +/*
> + * Copyright 2012 Freescale Semiconductor, Inc.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +/include/ "skeleton.dtsi"
> +
> +/ {
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +	interrupt-parent = <&icoll>;
> +
> +	aliases {
> +		serial0 = &uart1;
> +	};
> +
> +	cpus {
> +		cpu@0 {
> +			compatible = "arm,arm926ejs";
> +		};
> +	};
> +
> +	apb@80000000 {
> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		reg = <0x80000000 0x80000>;
> +		ranges;
> +
> +		apbh@80000000 {
> +			compatible = "simple-bus";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			reg = <0x80000000 0x3c900>;
> +			ranges;
> +
> +			icoll: interrupt-controller@80000000 {
> +				compatible = "fsl,imx28-icoll";
> +				interrupt-controller;
> +				#interrupt-cells = <1>;
> +				reg = <0x80000000 0x2000>;
> +			};
> +		};
> +
> +		apbx@80040000 {
> +			compatible = "simple-bus";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			reg = <0x80040000 0x40000>;
> +			ranges;
> +
> +			uart1: uart@80074000 {
> +				compatible = "arm,pl011", "arm,primecell";
> +				reg = <0x80074000 0x2000>;
> +				interrupts = <47>;
> +			};
> +		};

What is the purpose of the apbh and apbx busses?  Will more device nodes
get added to them later, or does each only contain a single device?

g.
Grant Likely March 13, 2012, 5:28 p.m. UTC | #4
On Tue, 13 Mar 2012 09:59:39 -0500, Zach Sadecki <zsadecki@itwatchdogs.com> wrote:
> On 03/13/2012 09:35 AM, Rob Herring wrote:
> > +	ahb@80080000 {
> > +		fec@800f0000 {
> > Use generic names: ethernet@800f0000
> Generic is good, but consistency is better, IMHO.  grepping existing dts 
> files in 3.2.9 finds 6 instances of "fec@" and 0 instances of "ethernet@"
> >> +			uart1: uart@80074000 {
> > Use generic names: uart1: serial@...
> Same comment here, but unfortunately there is already inconsistency in 
> existing files...  25 instances of "serial@" and 35 instances of "uart@"

No, Rob is correct.  The generic names recommended practice is well
established and documented.  Expand your grep search to include
arch/powerpc/bot/dts/*.

See section 2.2.2 of ePAPR[1]

[1]https://www.power.org/resources/downloads/Power_ePAPR_APPROVED_v1.1.pdf
Shawn Guo March 14, 2012, 5:38 a.m. UTC | #5
On Tue, Mar 13, 2012 at 11:28:07AM -0600, Grant Likely wrote:
> On Tue, 13 Mar 2012 09:59:39 -0500, Zach Sadecki <zsadecki@itwatchdogs.com> wrote:
> > On 03/13/2012 09:35 AM, Rob Herring wrote:
> > > +	ahb@80080000 {
> > > +		fec@800f0000 {
> > > Use generic names: ethernet@800f0000
> > Generic is good, but consistency is better, IMHO.  grepping existing dts 
> > files in 3.2.9 finds 6 instances of "fec@" and 0 instances of "ethernet@"
> > >> +			uart1: uart@80074000 {
> > > Use generic names: uart1: serial@...
> > Same comment here, but unfortunately there is already inconsistency in 
> > existing files...  25 instances of "serial@" and 35 instances of "uart@"
> 
> No, Rob is correct.  The generic names recommended practice is well
> established and documented.  Expand your grep search to include
> arch/powerpc/bot/dts/*.
> 
> See section 2.2.2 of ePAPR[1]
> 
> [1]https://www.power.org/resources/downloads/Power_ePAPR_APPROVED_v1.1.pdf
> 
I will probably need to patch imx5 and imx6 dts files with the
inconsistency fixed.
Shawn Guo March 14, 2012, 5:41 a.m. UTC | #6
On Tue, Mar 13, 2012 at 11:23:51AM -0600, Grant Likely wrote:
...
> > +	apb@80000000 {
> > +		compatible = "simple-bus";
> > +		#address-cells = <1>;
> > +		#size-cells = <1>;
> > +		reg = <0x80000000 0x80000>;
> > +		ranges;
> > +
> > +		apbh@80000000 {
> > +			compatible = "simple-bus";
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +			reg = <0x80000000 0x3c900>;
> > +			ranges;
> > +
> > +			icoll: interrupt-controller@80000000 {
> > +				compatible = "fsl,imx28-icoll";
> > +				interrupt-controller;
> > +				#interrupt-cells = <1>;
> > +				reg = <0x80000000 0x2000>;
> > +			};
> > +		};
> > +
> > +		apbx@80040000 {
> > +			compatible = "simple-bus";
> > +			#address-cells = <1>;
> > +			#size-cells = <1>;
> > +			reg = <0x80040000 0x40000>;
> > +			ranges;
> > +
> > +			uart1: uart@80074000 {
> > +				compatible = "arm,pl011", "arm,primecell";
> > +				reg = <0x80074000 0x2000>;
> > +				interrupts = <47>;
> > +			};
> > +		};
> 
> What is the purpose of the apbh and apbx busses?  Will more device nodes
> get added to them later, or does each only contain a single device?
> 
Since our ultimate goal is to convert mach-mxs over to device tree,
I would also suggest we have all the hardware blocks defined in dts
from the beginning.
Marek Vasut March 14, 2012, 5:56 a.m. UTC | #7
Dear Shawn Guo,

> On Tue, Mar 13, 2012 at 11:23:51AM -0600, Grant Likely wrote:
> ...
> 
> > > +	apb@80000000 {
> > > +		compatible = "simple-bus";
> > > +		#address-cells = <1>;
> > > +		#size-cells = <1>;
> > > +		reg = <0x80000000 0x80000>;
> > > +		ranges;
> > > +
> > > +		apbh@80000000 {
> > > +			compatible = "simple-bus";
> > > +			#address-cells = <1>;
> > > +			#size-cells = <1>;
> > > +			reg = <0x80000000 0x3c900>;
> > > +			ranges;
> > > +
> > > +			icoll: interrupt-controller@80000000 {
> > > +				compatible = "fsl,imx28-icoll";
> > > +				interrupt-controller;
> > > +				#interrupt-cells = <1>;
> > > +				reg = <0x80000000 0x2000>;
> > > +			};
> > > +		};
> > > +
> > > +		apbx@80040000 {
> > > +			compatible = "simple-bus";
> > > +			#address-cells = <1>;
> > > +			#size-cells = <1>;
> > > +			reg = <0x80040000 0x40000>;
> > > +			ranges;
> > > +
> > > +			uart1: uart@80074000 {
> > > +				compatible = "arm,pl011", "arm,primecell";
> > > +				reg = <0x80074000 0x2000>;
> > > +				interrupts = <47>;
> > > +			};
> > > +		};
> > 
> > What is the purpose of the apbh and apbx busses?  Will more device nodes
> > get added to them later, or does each only contain a single device?
> 
> Since our ultimate goal is to convert mach-mxs over to device tree,
> I would also suggest we have all the hardware blocks defined in dts
> from the beginning.

Agreed.

Best regards,
Marek Vasut
Dong Aisheng March 14, 2012, 6:23 a.m. UTC | #8
On Tue, Mar 13, 2012 at 09:35:43AM -0500, Rob Herring wrote:
> On 03/13/2012 03:47 AM, Dong Aisheng wrote:
> > From: Dong Aisheng <dong.aisheng@linaro.org>
> > 
> > This patch includes basic dt support which can boot via nfs rootfs.
> > 
> > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/arm/fsl.txt |    4 +
> >  arch/arm/boot/dts/imx28-evk.dts               |   31 +++++++++
> >  arch/arm/boot/dts/imx28.dtsi                  |   88 +++++++++++++++++++++++++
> >  arch/arm/mach-mxs/Kconfig                     |    9 +++
> >  arch/arm/mach-mxs/Makefile                    |    1 +
> >  arch/arm/mach-mxs/imx28-dt.c                  |   67 +++++++++++++++++++
> >  6 files changed, 200 insertions(+), 0 deletions(-)
....
> > +				interrupts = <47>;
> > +			};
> > +		};
> > +	};
> > +
> > +	ahb@80080000 {
> > +		compatible = "simple-bus";
> > +		#address-cells = <1>;
> > +		#size-cells = <1>;
> > +		reg = <0x80080000 0x80000>;
> > +		ranges;
> > +
> > +		fec@800f0000 {
> > +			compatible = "fsl,imx28-fec";
> > +			reg = <0x800f0000 0x4000>;
> 
> This too IIRC.
> 
The size is 16KB/0x4000 in iMX28 spec. :)

> > +			interrupts = <101>;
> > +			status = "disabled";
> > +		};
> > +
> > +		fec@800f4000 {
> > +			compatible = "fsl,imx28-fec";
> > +			reg = <0x800f4000 0x4000>;
> > +			interrupts = <102>;
> > +			status = "disabled";
> > +		};
> > +
> > +	};
> > +};
> > diff --git a/arch/arm/mach-mxs/Kconfig b/arch/arm/mach-mxs/Kconfig
> > index c57f996..6ab66af 100644
> > --- a/arch/arm/mach-mxs/Kconfig
> > +++ b/arch/arm/mach-mxs/Kconfig
> > @@ -17,6 +17,15 @@ config SOC_IMX28
> >  
> >  comment "MXS platforms:"
> >  
> > +config MACH_IMX28_DT
> 
> Perhaps this should be more generic like MACH_MXS_DT to eventually
> support MX23 as well?
> 
I just did like the imx ways.
But yes if we see the need when do imx23 dt support.

> > +	bool "Support i.MX28 platforms from device tree"
> > +	select SOC_IMX28
> > +	select USE_OF
> > +	select MACH_MX28EVK
> 
> This should not be selected here.
> 
Like other imx soc dt board files, the imx28-dt.c may need to use some bits
like pinmux in mx28evk.c board file.

Yes, currently i can remove it since it is using uboot pinmux setting.
But when add other devices support which is not covered in u-boot like flexcan,
i may need to use non-dt board's pinmux setting.
So maybe we can keep it first and remove it when totally not dependent
on non-dt files.

> > + * The code contained herein is licensed under the GNU General Public
> > + * License. You may obtain a copy of the GNU General Public License
> > + * Version 2 or later at the following locations:
> > + *
> > + * http://www.opensource.org/licenses/gpl-license.html
> > + * http://www.gnu.org/copyleft/gpl.html
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/of_platform.h>
> > +#include <asm/mach/arch.h>
> > +#include <asm/mach/time.h>
> > +#include <mach/common.h>
> > +#include <mach/mx28.h>
> > +
> > +static const struct of_dev_auxdata imx28_auxdata_lookup[] __initconst = {
> > +	OF_DEV_AUXDATA("arm,pl011", MX28_DUART_BASE_ADDR, "duart", NULL),
> > +	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC0_BASE_ADDR, "imx28-fec.0", NULL),
> > +	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC1_BASE_ADDR, "imx28-fec.1", NULL),
> 
> Why do you need the old names?
> 
To keep align with the old clocks.
See arch/arm/mach-mxs/clock-mx28.c:
_REGISTER_CLOCK("imx28-fec.0", NULL, fec_clk)
_REGISTER_CLOCK("imx28-fec.1", NULL, fec_clk)
Is there any better way to avoid this?

> > +	{ /* sentinel */ }
> > +};
> > +
> > +static void __init imx28_init(void)
> > +{
> > +	of_platform_populate(NULL, of_default_bus_match_table,
> > +			     imx28_auxdata_lookup, NULL);
> > +}
> > +
> > +static const struct of_device_id icoll_of_match[] __initconst = {
> > +	{ .compatible = "fsl,imx28-icoll", },
> > +	{},
> > +};
> > +
> > +static void __init imx28_dt_init_irq(void)
> > +{
> > +	irq_domain_generate_simple(icoll_of_match, MX28_ICOLL_BASE_ADDR, 0);
> 
> Please do "proper" domain support for the interrupt controller and use
> of_irq_init.
> 
Ok, i will see it.

For others, will fix them all.
Thanks for the review.

Regards
Dong Aisheng
Dong Aisheng March 14, 2012, 6:30 a.m. UTC | #9
On Wed, Mar 14, 2012 at 01:41:20PM +0800, Shawn Guo wrote:
> On Tue, Mar 13, 2012 at 11:23:51AM -0600, Grant Likely wrote:
> ...
> > > +	apb@80000000 {
> > > +		compatible = "simple-bus";
> > > +		#address-cells = <1>;
> > > +		#size-cells = <1>;
> > > +		reg = <0x80000000 0x80000>;
> > > +		ranges;
> > > +
> > > +		apbh@80000000 {
> > > +			compatible = "simple-bus";
> > > +			#address-cells = <1>;
> > > +			#size-cells = <1>;
> > > +			reg = <0x80000000 0x3c900>;
> > > +			ranges;
> > > +
> > > +			icoll: interrupt-controller@80000000 {
> > > +				compatible = "fsl,imx28-icoll";
> > > +				interrupt-controller;
> > > +				#interrupt-cells = <1>;
> > > +				reg = <0x80000000 0x2000>;
> > > +			};
> > > +		};
> > > +
> > > +		apbx@80040000 {
> > > +			compatible = "simple-bus";
> > > +			#address-cells = <1>;
> > > +			#size-cells = <1>;
> > > +			reg = <0x80040000 0x40000>;
> > > +			ranges;
> > > +
> > > +			uart1: uart@80074000 {
> > > +				compatible = "arm,pl011", "arm,primecell";
> > > +				reg = <0x80074000 0x2000>;
> > > +				interrupts = <47>;
> > > +			};
> > > +		};
> > 
> > What is the purpose of the apbh and apbx busses?  Will more device nodes
> > get added to them later, or does each only contain a single device?
> > 
> Since our ultimate goal is to convert mach-mxs over to device tree,
> I would also suggest we have all the hardware blocks defined in dts
> from the beginning.
> 
Also agree.
Will add them all in later patches.

Regards
Dong Aisheng
Marek Vasut March 14, 2012, 6:51 a.m. UTC | #10
Dear Dong Aisheng,

> On Tue, Mar 13, 2012 at 09:35:43AM -0500, Rob Herring wrote:
> > On 03/13/2012 03:47 AM, Dong Aisheng wrote:
> > > From: Dong Aisheng <dong.aisheng@linaro.org>
> > > 
> > > This patch includes basic dt support which can boot via nfs rootfs.
> > > 
> > > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > > ---
> > > 
> > >  Documentation/devicetree/bindings/arm/fsl.txt |    4 +
> > >  arch/arm/boot/dts/imx28-evk.dts               |   31 +++++++++
> > >  arch/arm/boot/dts/imx28.dtsi                  |   88
> > >  +++++++++++++++++++++++++ arch/arm/mach-mxs/Kconfig                  
> > >    |    9 +++
> > >  arch/arm/mach-mxs/Makefile                    |    1 +
> > >  arch/arm/mach-mxs/imx28-dt.c                  |   67
> > >  +++++++++++++++++++ 6 files changed, 200 insertions(+), 0
> > >  deletions(-)
> 
> ....
> 
> > > +				interrupts = <47>;
> > > +			};
> > > +		};
> > > +	};
> > > +
> > > +	ahb@80080000 {
> > > +		compatible = "simple-bus";
> > > +		#address-cells = <1>;
> > > +		#size-cells = <1>;
> > > +		reg = <0x80080000 0x80000>;
> > > +		ranges;
> > > +
> > > +		fec@800f0000 {
> > > +			compatible = "fsl,imx28-fec";
> > > +			reg = <0x800f0000 0x4000>;
> > 
> > This too IIRC.
> 
> The size is 16KB/0x4000 in iMX28 spec. :)

Agreed, the memory map is slightly non-standard.

> 
> > > +			interrupts = <101>;
> > > +			status = "disabled";
> > > +		};
> > > +
> > > +		fec@800f4000 {
> > > +			compatible = "fsl,imx28-fec";
> > > +			reg = <0x800f4000 0x4000>;
> > > +			interrupts = <102>;
> > > +			status = "disabled";
> > > +		};
> > > +
> > > +	};
> > > +};
> > > diff --git a/arch/arm/mach-mxs/Kconfig b/arch/arm/mach-mxs/Kconfig
> > > index c57f996..6ab66af 100644
> > > --- a/arch/arm/mach-mxs/Kconfig
> > > +++ b/arch/arm/mach-mxs/Kconfig
> > > @@ -17,6 +17,15 @@ config SOC_IMX28
> > > 
> > >  comment "MXS platforms:"
> > > 
> > > +config MACH_IMX28_DT
> > 
> > Perhaps this should be more generic like MACH_MXS_DT to eventually
> > support MX23 as well?
> 
> I just did like the imx ways.
> But yes if we see the need when do imx23 dt support.

Let's do it all in one swipe, there's not so much difference between these two 
chips.
> 
> > > +	bool "Support i.MX28 platforms from device tree"
> > > +	select SOC_IMX28
> > > +	select USE_OF
> > > +	select MACH_MX28EVK
> > 
> > This should not be selected here.
> 
> Like other imx soc dt board files, the imx28-dt.c may need to use some bits
> like pinmux in mx28evk.c board file.
> 
> Yes, currently i can remove it since it is using uboot pinmux setting.
> But when add other devices support which is not covered in u-boot like
> flexcan, i may need to use non-dt board's pinmux setting.
> So maybe we can keep it first and remove it when totally not dependent
> on non-dt files.

I'd be all in for the "decremental approach" -- keep in whatever is needed and 
remove is as it becomes redundant.

> 
> > > + * The code contained herein is licensed under the GNU General Public
> > > + * License. You may obtain a copy of the GNU General Public License
> > > + * Version 2 or later at the following locations:
> > > + *
> > > + * http://www.opensource.org/licenses/gpl-license.html
> > > + * http://www.gnu.org/copyleft/gpl.html
> > > + */
> > > +
> > > +#include <linux/init.h>
> > > +#include <linux/irqdomain.h>
> > > +#include <linux/of_irq.h>
> > > +#include <linux/of_platform.h>
> > > +#include <asm/mach/arch.h>
> > > +#include <asm/mach/time.h>
> > > +#include <mach/common.h>
> > > +#include <mach/mx28.h>
> > > +
> > > +static const struct of_dev_auxdata imx28_auxdata_lookup[] __initconst
> > > = { +	OF_DEV_AUXDATA("arm,pl011", MX28_DUART_BASE_ADDR, "duart",
> > > NULL), +	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC0_BASE_ADDR,
> > > "imx28-fec.0", NULL), +	OF_DEV_AUXDATA("fsl,imx28-fec",
> > > MX28_ENET_MAC1_BASE_ADDR, "imx28-fec.1", NULL),
> > 
> > Why do you need the old names?
> 
> To keep align with the old clocks.
> See arch/arm/mach-mxs/clock-mx28.c:
> _REGISTER_CLOCK("imx28-fec.0", NULL, fec_clk)
> _REGISTER_CLOCK("imx28-fec.1", NULL, fec_clk)
> Is there any better way to avoid this?
> 
> > > +	{ /* sentinel */ }
> > > +};
> > > +
> > > +static void __init imx28_init(void)
> > > +{
> > > +	of_platform_populate(NULL, of_default_bus_match_table,
> > > +			     imx28_auxdata_lookup, NULL);
> > > +}
> > > +
> > > +static const struct of_device_id icoll_of_match[] __initconst = {
> > > +	{ .compatible = "fsl,imx28-icoll", },
> > > +	{},
> > > +};
> > > +
> > > +static void __init imx28_dt_init_irq(void)
> > > +{
> > > +	irq_domain_generate_simple(icoll_of_match, MX28_ICOLL_BASE_ADDR, 0);
> > 
> > Please do "proper" domain support for the interrupt controller and use
> > of_irq_init.
> 
> Ok, i will see it.
> 
> For others, will fix them all.
> Thanks for the review.
> 
> Regards
> Dong Aisheng


Best regards,
Marek Vasut
Dong Aisheng March 14, 2012, 12:45 p.m. UTC | #11
On Wed, Mar 14, 2012 at 01:23:51AM +0800, Grant Likely wrote:
> On Tue, 13 Mar 2012 16:47:04 +0800, Dong Aisheng <b29396@freescale.com> wrote:
> > From: Dong Aisheng <dong.aisheng@linaro.org>
> > 
> > This patch includes basic dt support which can boot via nfs rootfs.
> > 
> > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/arm/fsl.txt |    4 +
> >  arch/arm/boot/dts/imx28-evk.dts               |   31 +++++++++
> >  arch/arm/boot/dts/imx28.dtsi                  |   88 +++++++++++++++++++++++++
> >  arch/arm/mach-mxs/Kconfig                     |    9 +++
> >  arch/arm/mach-mxs/Makefile                    |    1 +
> >  arch/arm/mach-mxs/imx28-dt.c                  |   67 +++++++++++++++++++
> >  6 files changed, 200 insertions(+), 0 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/fsl.txt b/Documentation/devicetree/bindings/arm/fsl.txt
> > index 54bddda..9f21faf 100644
> > --- a/Documentation/devicetree/bindings/arm/fsl.txt
> > +++ b/Documentation/devicetree/bindings/arm/fsl.txt
> > @@ -1,6 +1,10 @@
> >  Freescale i.MX Platforms Device Tree Bindings
> >  -----------------------------------------------
> >  
> > +i.MX28 Evaluation Kit
> > +Required root node properties:
> > +    - compatible = "fsl,imx28-evk", "fsl,imx28";
> > +
> >  i.MX51 Babbage Board
> >  Required root node properties:
> >      - compatible = "fsl,imx51-babbage", "fsl,imx51";
> > diff --git a/arch/arm/boot/dts/imx28-evk.dts b/arch/arm/boot/dts/imx28-evk.dts
> > new file mode 100644
> > index 0000000..9758dc4
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/imx28-evk.dts
> > @@ -0,0 +1,31 @@
> > +/*
> > + * Copyright 2012 Freescale Semiconductor, Inc.
> > + *
> > + * The code contained herein is licensed under the GNU General Public
> > + * License. You may obtain a copy of the GNU General Public License
> > + * Version 2 or later at the following locations:
> > + *
> > + * http://www.opensource.org/licenses/gpl-license.html
> > + * http://www.gnu.org/copyleft/gpl.html
> > + */
> > +
> > +/dts-v1/;
> > +/include/ "imx28.dtsi"
> > +
> > +/ {
> > +	model = "Freescale i.MX28 Evaluation Kit";
> > +	compatible = "fsl,imx28-evk", "fsl,imx28";
> > +
> > +	memory {
> > +		device_type = "memory";
> > +		reg = <0x40000000 0x08000000>;
> > +	};
> > +
> > +	ahb@80080000 {
> > +		fec@800f0000 {
> > +			phy-mode = "rmii";
> > +			local-mac-address = [00 04 9F 01 7D 5B];
> 
> Generally a bad idea to put a specific mac address into the device tree.
> Better to fill it with zeros.  Otherwise all the dev boards will end up using
> the same value.
> 
Yes, this issue also exists on other platfroms like mx6q.
One way is to dynamically get mac address by reading otp register as non-dt does
like:
static int __init mx28evk_fec_get_mac(void)
{
        int i;
        u32 val;
        const u32 *ocotp = mxs_get_ocotp();

        if (!ocotp)
                return -ETIMEDOUT;

        /*
         * OCOTP only stores the last 4 octets for each mac address,
         * so hard-code Freescale OUI (00:04:9f) here.
         */
        for (i = 0; i < 2; i++) {
                val = ocotp[i];
                mx28_fec_pdata[i].mac[0] = 0x00;
                mx28_fec_pdata[i].mac[1] = 0x04;
                mx28_fec_pdata[i].mac[2] = 0x9f;
                mx28_fec_pdata[i].mac[3] = (val >> 16) & 0xff;
                mx28_fec_pdata[i].mac[4] = (val >> 8) & 0xff;
                mx28_fec_pdata[i].mac[5] = (val >> 0) & 0xff;
        }

        return 0;
}

But it seems this needs pass mac address to fec driver via platforom data which is
not friendly to dt.

Another way may be changing fec driver to get the fixed part of mac address(first
two bytes) from device tree and read the left dynamical part from otp which i'm not
sure is better enough.

BTW, filling with zeros seems not work since it's invalid mac address.

Regards
Dong Aisheng
Rob Herring March 14, 2012, 1:05 p.m. UTC | #12
On 03/14/2012 01:23 AM, Dong Aisheng wrote:
> On Tue, Mar 13, 2012 at 09:35:43AM -0500, Rob Herring wrote:
>> On 03/13/2012 03:47 AM, Dong Aisheng wrote:
>>> From: Dong Aisheng <dong.aisheng@linaro.org>
>>>
>>> This patch includes basic dt support which can boot via nfs rootfs.
>>>
>>> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
>>> ---
>>>  Documentation/devicetree/bindings/arm/fsl.txt |    4 +
>>>  arch/arm/boot/dts/imx28-evk.dts               |   31 +++++++++
>>>  arch/arm/boot/dts/imx28.dtsi                  |   88 +++++++++++++++++++++++++
>>>  arch/arm/mach-mxs/Kconfig                     |    9 +++
>>>  arch/arm/mach-mxs/Makefile                    |    1 +
>>>  arch/arm/mach-mxs/imx28-dt.c                  |   67 +++++++++++++++++++
>>>  6 files changed, 200 insertions(+), 0 deletions(-)
> ....
>>> +				interrupts = <47>;
>>> +			};
>>> +		};
>>> +	};
>>> +
>>> +	ahb@80080000 {
>>> +		compatible = "simple-bus";
>>> +		#address-cells = <1>;
>>> +		#size-cells = <1>;
>>> +		reg = <0x80080000 0x80000>;
>>> +		ranges;
>>> +
>>> +		fec@800f0000 {
>>> +			compatible = "fsl,imx28-fec";
>>> +			reg = <0x800f0000 0x4000>;
>>
>> This too IIRC.
>>
> The size is 16KB/0x4000 in iMX28 spec. :)

Yes, but the module only uses <4KB. You waste virtual memory space by
mapping all the unused area. It's not really an issue when you have
<512MB of RAM, but does become an issue.


>>> +			interrupts = <101>;
>>> +			status = "disabled";
>>> +		};
>>> +
>>> +		fec@800f4000 {
>>> +			compatible = "fsl,imx28-fec";
>>> +			reg = <0x800f4000 0x4000>;
>>> +			interrupts = <102>;
>>> +			status = "disabled";
>>> +		};
>>> +
>>> +	};
>>> +};
>>> diff --git a/arch/arm/mach-mxs/Kconfig b/arch/arm/mach-mxs/Kconfig
>>> index c57f996..6ab66af 100644
>>> --- a/arch/arm/mach-mxs/Kconfig
>>> +++ b/arch/arm/mach-mxs/Kconfig
>>> @@ -17,6 +17,15 @@ config SOC_IMX28
>>>  
>>>  comment "MXS platforms:"
>>>  
>>> +config MACH_IMX28_DT
>>
>> Perhaps this should be more generic like MACH_MXS_DT to eventually
>> support MX23 as well?
>>
> I just did like the imx ways.
> But yes if we see the need when do imx23 dt support.
> 
>>> +	bool "Support i.MX28 platforms from device tree"
>>> +	select SOC_IMX28
>>> +	select USE_OF
>>> +	select MACH_MX28EVK
>>
>> This should not be selected here.
>>
> Like other imx soc dt board files, the imx28-dt.c may need to use some bits
> like pinmux in mx28evk.c board file.
> 
> Yes, currently i can remove it since it is using uboot pinmux setting.
> But when add other devices support which is not covered in u-boot like flexcan,
> i may need to use non-dt board's pinmux setting.
> So maybe we can keep it first and remove it when totally not dependent
> on non-dt files.

Okay either way, but my vote would be add it as needed.

>>> + * The code contained herein is licensed under the GNU General Public
>>> + * License. You may obtain a copy of the GNU General Public License
>>> + * Version 2 or later at the following locations:
>>> + *
>>> + * http://www.opensource.org/licenses/gpl-license.html
>>> + * http://www.gnu.org/copyleft/gpl.html
>>> + */
>>> +
>>> +#include <linux/init.h>
>>> +#include <linux/irqdomain.h>
>>> +#include <linux/of_irq.h>
>>> +#include <linux/of_platform.h>
>>> +#include <asm/mach/arch.h>
>>> +#include <asm/mach/time.h>
>>> +#include <mach/common.h>
>>> +#include <mach/mx28.h>
>>> +
>>> +static const struct of_dev_auxdata imx28_auxdata_lookup[] __initconst = {
>>> +	OF_DEV_AUXDATA("arm,pl011", MX28_DUART_BASE_ADDR, "duart", NULL),
>>> +	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC0_BASE_ADDR, "imx28-fec.0", NULL),
>>> +	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC1_BASE_ADDR, "imx28-fec.1", NULL),
>>
>> Why do you need the old names?
>>
> To keep align with the old clocks.
> See arch/arm/mach-mxs/clock-mx28.c:
> _REGISTER_CLOCK("imx28-fec.0", NULL, fec_clk)
> _REGISTER_CLOCK("imx28-fec.1", NULL, fec_clk)
> Is there any better way to avoid this?

Yes, you can add more clock look-ups with the DT name.

Rob

> 
>>> +	{ /* sentinel */ }
>>> +};
>>> +
>>> +static void __init imx28_init(void)
>>> +{
>>> +	of_platform_populate(NULL, of_default_bus_match_table,
>>> +			     imx28_auxdata_lookup, NULL);
>>> +}
>>> +
>>> +static const struct of_device_id icoll_of_match[] __initconst = {
>>> +	{ .compatible = "fsl,imx28-icoll", },
>>> +	{},
>>> +};
>>> +
>>> +static void __init imx28_dt_init_irq(void)
>>> +{
>>> +	irq_domain_generate_simple(icoll_of_match, MX28_ICOLL_BASE_ADDR, 0);
>>
>> Please do "proper" domain support for the interrupt controller and use
>> of_irq_init.
>>
> Ok, i will see it.
> 
> For others, will fix them all.
> Thanks for the review.
> 
> Regards
> Dong Aisheng
>
Sascha Hauer March 14, 2012, 2:16 p.m. UTC | #13
On Wed, Mar 14, 2012 at 08:45:23PM +0800, Dong Aisheng wrote:
> On Wed, Mar 14, 2012 at 01:23:51AM +0800, Grant Likely wrote:
> > On Tue, 13 Mar 2012 16:47:04 +0800, Dong Aisheng <b29396@freescale.com> wrote:
> > > From: Dong Aisheng <dong.aisheng@linaro.org>
> > > 
> > > This patch includes basic dt support which can boot via nfs rootfs.
> > > 
> > > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > > ---
> > >  Documentation/devicetree/bindings/arm/fsl.txt |    4 +
> > >  arch/arm/boot/dts/imx28-evk.dts               |   31 +++++++++
> > >  arch/arm/boot/dts/imx28.dtsi                  |   88 +++++++++++++++++++++++++
> > >  arch/arm/mach-mxs/Kconfig                     |    9 +++
> > >  arch/arm/mach-mxs/Makefile                    |    1 +
> > >  arch/arm/mach-mxs/imx28-dt.c                  |   67 +++++++++++++++++++
> > >  6 files changed, 200 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/arm/fsl.txt b/Documentation/devicetree/bindings/arm/fsl.txt
> > > index 54bddda..9f21faf 100644
> > > --- a/Documentation/devicetree/bindings/arm/fsl.txt
> > > +++ b/Documentation/devicetree/bindings/arm/fsl.txt
> > > @@ -1,6 +1,10 @@
> > >  Freescale i.MX Platforms Device Tree Bindings
> > >  -----------------------------------------------
> > >  
> > > +i.MX28 Evaluation Kit
> > > +Required root node properties:
> > > +    - compatible = "fsl,imx28-evk", "fsl,imx28";
> > > +
> > >  i.MX51 Babbage Board
> > >  Required root node properties:
> > >      - compatible = "fsl,imx51-babbage", "fsl,imx51";
> > > diff --git a/arch/arm/boot/dts/imx28-evk.dts b/arch/arm/boot/dts/imx28-evk.dts
> > > new file mode 100644
> > > index 0000000..9758dc4
> > > --- /dev/null
> > > +++ b/arch/arm/boot/dts/imx28-evk.dts
> > > @@ -0,0 +1,31 @@
> > > +/*
> > > + * Copyright 2012 Freescale Semiconductor, Inc.
> > > + *
> > > + * The code contained herein is licensed under the GNU General Public
> > > + * License. You may obtain a copy of the GNU General Public License
> > > + * Version 2 or later at the following locations:
> > > + *
> > > + * http://www.opensource.org/licenses/gpl-license.html
> > > + * http://www.gnu.org/copyleft/gpl.html
> > > + */
> > > +
> > > +/dts-v1/;
> > > +/include/ "imx28.dtsi"
> > > +
> > > +/ {
> > > +	model = "Freescale i.MX28 Evaluation Kit";
> > > +	compatible = "fsl,imx28-evk", "fsl,imx28";
> > > +
> > > +	memory {
> > > +		device_type = "memory";
> > > +		reg = <0x40000000 0x08000000>;
> > > +	};
> > > +
> > > +	ahb@80080000 {
> > > +		fec@800f0000 {
> > > +			phy-mode = "rmii";
> > > +			local-mac-address = [00 04 9F 01 7D 5B];
> > 
> > Generally a bad idea to put a specific mac address into the device tree.
> > Better to fill it with zeros.  Otherwise all the dev boards will end up using
> > the same value.
> > 
> Yes, this issue also exists on other platfroms like mx6q.
> One way is to dynamically get mac address by reading otp register as non-dt does
> like:
> static int __init mx28evk_fec_get_mac(void)
> {
>         int i;
>         u32 val;
>         const u32 *ocotp = mxs_get_ocotp();
> 
>         if (!ocotp)
>                 return -ETIMEDOUT;
> 
>         /*
>          * OCOTP only stores the last 4 octets for each mac address,
>          * so hard-code Freescale OUI (00:04:9f) here.
>          */
>         for (i = 0; i < 2; i++) {
>                 val = ocotp[i];
>                 mx28_fec_pdata[i].mac[0] = 0x00;
>                 mx28_fec_pdata[i].mac[1] = 0x04;
>                 mx28_fec_pdata[i].mac[2] = 0x9f;
>                 mx28_fec_pdata[i].mac[3] = (val >> 16) & 0xff;
>                 mx28_fec_pdata[i].mac[4] = (val >> 8) & 0xff;
>                 mx28_fec_pdata[i].mac[5] = (val >> 0) & 0xff;
>         }
> 
>         return 0;
> }
> 
> But it seems this needs pass mac address to fec driver via platforom data which is
> not friendly to dt.
> 
> Another way may be changing fec driver to get the fixed part of mac address(first
> two bytes) from device tree and read the left dynamical part from otp which i'm not
> sure is better enough.
> 
> BTW, filling with zeros seems not work since it's invalid mac address.

Yes, that's the idea of using this value...

Sascha
Sascha Hauer March 14, 2012, 7:41 p.m. UTC | #14
On Tue, Mar 13, 2012 at 04:47:04PM +0800, Dong Aisheng wrote:
> From: Dong Aisheng <dong.aisheng@linaro.org>
> 
> This patch includes basic dt support which can boot via nfs rootfs.
> 
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> ---
>  Documentation/devicetree/bindings/arm/fsl.txt |    4 +
>  arch/arm/boot/dts/imx28-evk.dts               |   31 +++++++++
>  arch/arm/boot/dts/imx28.dtsi                  |   88 +++++++++++++++++++++++++
>  arch/arm/mach-mxs/Kconfig                     |    9 +++
>  arch/arm/mach-mxs/Makefile                    |    1 +
>  arch/arm/mach-mxs/imx28-dt.c                  |   67 +++++++++++++++++++
>  6 files changed, 200 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/fsl.txt b/Documentation/devicetree/bindings/arm/fsl.txt
> index 54bddda..9f21faf 100644
> --- a/Documentation/devicetree/bindings/arm/fsl.txt
> +++ b/Documentation/devicetree/bindings/arm/fsl.txt
> @@ -1,6 +1,10 @@

> +
> +static const char *imx28_dt_compat[] __initdata = {
> +	"fsl,imx28-evk",
> +	NULL,
> +};
> +

I suggest to add a generic "fsl,imx28" entry here. It helps people to
start an unpatched kernel on their boards.

Sascha

> +DT_MACHINE_START(IMX28, "Freescale i.MX28 (Device Tree)")
> +	.map_io		= mx28_map_io,
> +	.init_irq	= imx28_dt_init_irq,
> +	.timer		= &imx28_timer,
> +	.init_machine	= imx28_init,
> +	.dt_compat	= imx28_dt_compat,
> +	.restart	= mxs_restart,
> +MACHINE_END
> -- 
> 1.7.0.4
> 
> 
>
Dong Aisheng March 15, 2012, 2:57 a.m. UTC | #15
On Wed, Mar 14, 2012 at 09:05:34PM +0800, Rob Herring wrote:
> On 03/14/2012 01:23 AM, Dong Aisheng wrote:
> > On Tue, Mar 13, 2012 at 09:35:43AM -0500, Rob Herring wrote:
> >> On 03/13/2012 03:47 AM, Dong Aisheng wrote:
> >>> From: Dong Aisheng <dong.aisheng@linaro.org>
> >>>
> >>> This patch includes basic dt support which can boot via nfs rootfs.
> >>>
> >>> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> >>> ---
> >>>  Documentation/devicetree/bindings/arm/fsl.txt |    4 +
> >>>  arch/arm/boot/dts/imx28-evk.dts               |   31 +++++++++
> >>>  arch/arm/boot/dts/imx28.dtsi                  |   88 +++++++++++++++++++++++++
> >>>  arch/arm/mach-mxs/Kconfig                     |    9 +++
> >>>  arch/arm/mach-mxs/Makefile                    |    1 +
> >>>  arch/arm/mach-mxs/imx28-dt.c                  |   67 +++++++++++++++++++
> >>>  6 files changed, 200 insertions(+), 0 deletions(-)
> > ....
> >>> +				interrupts = <47>;
> >>> +			};
> >>> +		};
> >>> +	};
> >>> +
> >>> +	ahb@80080000 {
> >>> +		compatible = "simple-bus";
> >>> +		#address-cells = <1>;
> >>> +		#size-cells = <1>;
> >>> +		reg = <0x80080000 0x80000>;
> >>> +		ranges;
> >>> +
> >>> +		fec@800f0000 {
> >>> +			compatible = "fsl,imx28-fec";
> >>> +			reg = <0x800f0000 0x4000>;
> >>
> >> This too IIRC.
> >>
> > The size is 16KB/0x4000 in iMX28 spec. :)
> 
> Yes, but the module only uses <4KB. You waste virtual memory space by
> mapping all the unused area. It's not really an issue when you have
> <512MB of RAM, but does become an issue.
> 
Yes, it seems an issue of spec.
And all other places are using the same size.
The simplest way now may be just keep align with spec first.

> >>> +			interrupts = <101>;
> >>> +			status = "disabled";
> >>> +		};
> >>> +
> >>> +		fec@800f4000 {
> >>> +			compatible = "fsl,imx28-fec";
> >>> +			reg = <0x800f4000 0x4000>;
> >>> +			interrupts = <102>;
> >>> +			status = "disabled";
> >>> +		};
> >>> +
> >>> +	};
> >>> +};
> >>> diff --git a/arch/arm/mach-mxs/Kconfig b/arch/arm/mach-mxs/Kconfig
> >>> index c57f996..6ab66af 100644
> >>> --- a/arch/arm/mach-mxs/Kconfig
> >>> +++ b/arch/arm/mach-mxs/Kconfig
> >>> @@ -17,6 +17,15 @@ config SOC_IMX28
> >>>  
> >>>  comment "MXS platforms:"
> >>>  
> >>> +config MACH_IMX28_DT
> >>
> >> Perhaps this should be more generic like MACH_MXS_DT to eventually
> >> support MX23 as well?
> >>
> > I just did like the imx ways.
> > But yes if we see the need when do imx23 dt support.
> > 
> >>> +	bool "Support i.MX28 platforms from device tree"
> >>> +	select SOC_IMX28
> >>> +	select USE_OF
> >>> +	select MACH_MX28EVK
> >>
> >> This should not be selected here.
> >>
> > Like other imx soc dt board files, the imx28-dt.c may need to use some bits
> > like pinmux in mx28evk.c board file.
> > 
> > Yes, currently i can remove it since it is using uboot pinmux setting.
> > But when add other devices support which is not covered in u-boot like flexcan,
> > i may need to use non-dt board's pinmux setting.
> > So maybe we can keep it first and remove it when totally not dependent
> > on non-dt files.
> 
> Okay either way, but my vote would be add it as needed.
> 
Your vote is ok to me.

> >>> + * The code contained herein is licensed under the GNU General Public
> >>> + * License. You may obtain a copy of the GNU General Public License
> >>> + * Version 2 or later at the following locations:
> >>> + *
> >>> + * http://www.opensource.org/licenses/gpl-license.html
> >>> + * http://www.gnu.org/copyleft/gpl.html
> >>> + */
> >>> +
> >>> +#include <linux/init.h>
> >>> +#include <linux/irqdomain.h>
> >>> +#include <linux/of_irq.h>
> >>> +#include <linux/of_platform.h>
> >>> +#include <asm/mach/arch.h>
> >>> +#include <asm/mach/time.h>
> >>> +#include <mach/common.h>
> >>> +#include <mach/mx28.h>
> >>> +
> >>> +static const struct of_dev_auxdata imx28_auxdata_lookup[] __initconst = {
> >>> +	OF_DEV_AUXDATA("arm,pl011", MX28_DUART_BASE_ADDR, "duart", NULL),
> >>> +	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC0_BASE_ADDR, "imx28-fec.0", NULL),
> >>> +	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC1_BASE_ADDR, "imx28-fec.1", NULL),
> >>
> >> Why do you need the old names?
> >>
> > To keep align with the old clocks.
> > See arch/arm/mach-mxs/clock-mx28.c:
> > _REGISTER_CLOCK("imx28-fec.0", NULL, fec_clk)
> > _REGISTER_CLOCK("imx28-fec.1", NULL, fec_clk)
> > Is there any better way to avoid this?
> 
> Yes, you can add more clock look-ups with the DT name.
> 
Got it.

Regards
Dong Aisheng
Dong Aisheng March 15, 2012, 3:02 a.m. UTC | #16
On Wed, Mar 14, 2012 at 10:16:43PM +0800, s.hauer@pengutronix.de wrote:
> On Wed, Mar 14, 2012 at 08:45:23PM +0800, Dong Aisheng wrote:
> > On Wed, Mar 14, 2012 at 01:23:51AM +0800, Grant Likely wrote:
> > > On Tue, 13 Mar 2012 16:47:04 +0800, Dong Aisheng <b29396@freescale.com> wrote:
> > > > From: Dong Aisheng <dong.aisheng@linaro.org>
> > > > 
> > > > This patch includes basic dt support which can boot via nfs rootfs.
> > > > 
> > > > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > > > ---
> > > >  Documentation/devicetree/bindings/arm/fsl.txt |    4 +
> > > >  arch/arm/boot/dts/imx28-evk.dts               |   31 +++++++++
> > > >  arch/arm/boot/dts/imx28.dtsi                  |   88 +++++++++++++++++++++++++
> > > >  arch/arm/mach-mxs/Kconfig                     |    9 +++
> > > >  arch/arm/mach-mxs/Makefile                    |    1 +
> > > >  arch/arm/mach-mxs/imx28-dt.c                  |   67 +++++++++++++++++++
> > > >  6 files changed, 200 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/arm/fsl.txt b/Documentation/devicetree/bindings/arm/fsl.txt
> > > > index 54bddda..9f21faf 100644
> > > > --- a/Documentation/devicetree/bindings/arm/fsl.txt
> > > > +++ b/Documentation/devicetree/bindings/arm/fsl.txt
> > > > @@ -1,6 +1,10 @@
> > > >  Freescale i.MX Platforms Device Tree Bindings
> > > >  -----------------------------------------------
> > > >  
> > > > +i.MX28 Evaluation Kit
> > > > +Required root node properties:
> > > > +    - compatible = "fsl,imx28-evk", "fsl,imx28";
> > > > +
> > > >  i.MX51 Babbage Board
> > > >  Required root node properties:
> > > >      - compatible = "fsl,imx51-babbage", "fsl,imx51";
> > > > diff --git a/arch/arm/boot/dts/imx28-evk.dts b/arch/arm/boot/dts/imx28-evk.dts
> > > > new file mode 100644
> > > > index 0000000..9758dc4
> > > > --- /dev/null
> > > > +++ b/arch/arm/boot/dts/imx28-evk.dts
> > > > @@ -0,0 +1,31 @@
> > > > +/*
> > > > + * Copyright 2012 Freescale Semiconductor, Inc.
> > > > + *
> > > > + * The code contained herein is licensed under the GNU General Public
> > > > + * License. You may obtain a copy of the GNU General Public License
> > > > + * Version 2 or later at the following locations:
> > > > + *
> > > > + * http://www.opensource.org/licenses/gpl-license.html
> > > > + * http://www.gnu.org/copyleft/gpl.html
> > > > + */
> > > > +
> > > > +/dts-v1/;
> > > > +/include/ "imx28.dtsi"
> > > > +
> > > > +/ {
> > > > +	model = "Freescale i.MX28 Evaluation Kit";
> > > > +	compatible = "fsl,imx28-evk", "fsl,imx28";
> > > > +
> > > > +	memory {
> > > > +		device_type = "memory";
> > > > +		reg = <0x40000000 0x08000000>;
> > > > +	};
> > > > +
> > > > +	ahb@80080000 {
> > > > +		fec@800f0000 {
> > > > +			phy-mode = "rmii";
> > > > +			local-mac-address = [00 04 9F 01 7D 5B];
> > > 
> > > Generally a bad idea to put a specific mac address into the device tree.
> > > Better to fill it with zeros.  Otherwise all the dev boards will end up using
> > > the same value.
> > > 
> > Yes, this issue also exists on other platfroms like mx6q.
> > One way is to dynamically get mac address by reading otp register as non-dt does
> > like:
> > static int __init mx28evk_fec_get_mac(void)
> > {
> >         int i;
> >         u32 val;
> >         const u32 *ocotp = mxs_get_ocotp();
> > 
> >         if (!ocotp)
> >                 return -ETIMEDOUT;
> > 
> >         /*
> >          * OCOTP only stores the last 4 octets for each mac address,
> >          * so hard-code Freescale OUI (00:04:9f) here.
> >          */
> >         for (i = 0; i < 2; i++) {
> >                 val = ocotp[i];
> >                 mx28_fec_pdata[i].mac[0] = 0x00;
> >                 mx28_fec_pdata[i].mac[1] = 0x04;
> >                 mx28_fec_pdata[i].mac[2] = 0x9f;
> >                 mx28_fec_pdata[i].mac[3] = (val >> 16) & 0xff;
> >                 mx28_fec_pdata[i].mac[4] = (val >> 8) & 0xff;
> >                 mx28_fec_pdata[i].mac[5] = (val >> 0) & 0xff;
> >         }
> > 
> >         return 0;
> > }
> > 
> > But it seems this needs pass mac address to fec driver via platforom data which is
> > not friendly to dt.
> > 
> > Another way may be changing fec driver to get the fixed part of mac address(first
> > two bytes) from device tree and read the left dynamical part from otp which i'm not
> > sure is better enough.
> > 
> > BTW, filling with zeros seems not work since it's invalid mac address.
> 
> Yes, that's the idea of using this value...
> 
But comparing to use an invalid value, why not just do not define that
local-mac-address property?

Regards
Dong Aisheng
Dong Aisheng March 15, 2012, 3:05 a.m. UTC | #17
On Thu, Mar 15, 2012 at 03:41:23AM +0800, Sascha Hauer wrote:
> On Tue, Mar 13, 2012 at 04:47:04PM +0800, Dong Aisheng wrote:
> > From: Dong Aisheng <dong.aisheng@linaro.org>
> > 
> > This patch includes basic dt support which can boot via nfs rootfs.
> > 
> > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > ---
> >  Documentation/devicetree/bindings/arm/fsl.txt |    4 +
> >  arch/arm/boot/dts/imx28-evk.dts               |   31 +++++++++
> >  arch/arm/boot/dts/imx28.dtsi                  |   88 +++++++++++++++++++++++++
> >  arch/arm/mach-mxs/Kconfig                     |    9 +++
> >  arch/arm/mach-mxs/Makefile                    |    1 +
> >  arch/arm/mach-mxs/imx28-dt.c                  |   67 +++++++++++++++++++
> >  6 files changed, 200 insertions(+), 0 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/fsl.txt b/Documentation/devicetree/bindings/arm/fsl.txt
> > index 54bddda..9f21faf 100644
> > --- a/Documentation/devicetree/bindings/arm/fsl.txt
> > +++ b/Documentation/devicetree/bindings/arm/fsl.txt
> > @@ -1,6 +1,10 @@
> 
> > +
> > +static const char *imx28_dt_compat[] __initdata = {
> > +	"fsl,imx28-evk",
> > +	NULL,
> > +};
> > +
> 
> I suggest to add a generic "fsl,imx28" entry here. It helps people to
> start an unpatched kernel on their boards.
> 
Good point.
Thanks for reminder.

Regards
Dong Aisheng
Lothar Waßmann March 15, 2012, 6:53 a.m. UTC | #18
Hi,

Dong Aisheng writes:
> On Wed, Mar 14, 2012 at 10:16:43PM +0800, s.hauer@pengutronix.de wrote:
> > On Wed, Mar 14, 2012 at 08:45:23PM +0800, Dong Aisheng wrote:
> > > On Wed, Mar 14, 2012 at 01:23:51AM +0800, Grant Likely wrote:
[...]
> > > But it seems this needs pass mac address to fec driver via platforom data which is
> > > not friendly to dt.
> > > 
> > > Another way may be changing fec driver to get the fixed part of mac address(first
> > > two bytes) from device tree and read the left dynamical part from otp which i'm not
> > > sure is better enough.
> > > 
> > > BTW, filling with zeros seems not work since it's invalid mac address.
> > 
> > Yes, that's the idea of using this value...
> > 
> But comparing to use an invalid value, why not just do not define that
> local-mac-address property?
> 
Because a MAC address is by definition a GLOBALLY UNIQUE IDENTIFIER
which is contrary to coding a "valid" default value for it somewhere!
The owner of a certain MAC address range (OUI) is responsible for
ensuring the uniqueness. Thus only they may assign a specific MAC
address to a specific device.


Lothar Waßmann
Dong Aisheng March 15, 2012, 10:59 a.m. UTC | #19
On Thu, Mar 15, 2012 at 02:53:29PM +0800, Lothar Waßmann wrote:
> Hi,
> 
> Dong Aisheng writes:
> > On Wed, Mar 14, 2012 at 10:16:43PM +0800, s.hauer@pengutronix.de wrote:
> > > On Wed, Mar 14, 2012 at 08:45:23PM +0800, Dong Aisheng wrote:
> > > > On Wed, Mar 14, 2012 at 01:23:51AM +0800, Grant Likely wrote:
> [...]
> > > > But it seems this needs pass mac address to fec driver via platforom data which is
> > > > not friendly to dt.
> > > > 
> > > > Another way may be changing fec driver to get the fixed part of mac address(first
> > > > two bytes) from device tree and read the left dynamical part from otp which i'm not
> > > > sure is better enough.
> > > > 
> > > > BTW, filling with zeros seems not work since it's invalid mac address.
> > > 
> > > Yes, that's the idea of using this value...
> > > 
> > But comparing to use an invalid value, why not just do not define that
> > local-mac-address property?
> > 
> Because a MAC address is by definition a GLOBALLY UNIQUE IDENTIFIER
> which is contrary to coding a "valid" default value for it somewhere!
> The owner of a certain MAC address range (OUI) is responsible for
> ensuring the uniqueness. Thus only they may assign a specific MAC
> address to a specific device.
> 
Yes, you're correct.
So i propose to read the MAC address from MX28 OTP in fec driver instead of
define it in device tree in my last mail.
http://www.spinics.net/lists/arm-kernel/msg165040.html
Do you have any comment on that way?

Regards
Dong Aisheng
Lothar Waßmann March 15, 2012, 11:22 a.m. UTC | #20
Hi,

Dong Aisheng writes:
> On Thu, Mar 15, 2012 at 02:53:29PM +0800, Lothar Waßmann wrote:
> > Hi,
> > 
> > Dong Aisheng writes:
> > > On Wed, Mar 14, 2012 at 10:16:43PM +0800, s.hauer@pengutronix.de wrote:
> > > > On Wed, Mar 14, 2012 at 08:45:23PM +0800, Dong Aisheng wrote:
> > > > > On Wed, Mar 14, 2012 at 01:23:51AM +0800, Grant Likely wrote:
> > [...]
> > > > > But it seems this needs pass mac address to fec driver via platforom data which is
> > > > > not friendly to dt.
> > > > > 
> > > > > Another way may be changing fec driver to get the fixed part of mac address(first
> > > > > two bytes) from device tree and read the left dynamical part from otp which i'm not
> > > > > sure is better enough.
> > > > > 
> > > > > BTW, filling with zeros seems not work since it's invalid mac address.
> > > > 
> > > > Yes, that's the idea of using this value...
> > > > 
> > > But comparing to use an invalid value, why not just do not define that
> > > local-mac-address property?
> > > 
> > Because a MAC address is by definition a GLOBALLY UNIQUE IDENTIFIER
> > which is contrary to coding a "valid" default value for it somewhere!
> > The owner of a certain MAC address range (OUI) is responsible for
> > ensuring the uniqueness. Thus only they may assign a specific MAC
> > address to a specific device.
> > 
> Yes, you're correct.
> So i propose to read the MAC address from MX28 OTP in fec driver instead of
> define it in device tree in my last mail.
> http://www.spinics.net/lists/arm-kernel/msg165040.html
> Do you have any comment on that way?
> 
That patch sets the OUI to the Freescale owned MAC range. Thus only
Freescale products may be able to use the driver.

Anyway there is no definite spec how the MAC address(es) are stored
in the fuse map. Thus reading the MAC from there is more or less
platform specific.

Currently I'm setting up the MAC address for our TX28 from the fuses
in the platform code passed via platform_data, but that will obviously
not work with DT.

The correct way would probably be to pass the MAC from the bootloader
via a DT blob. But that would require all bootloaders to be updated
first to support DTS. :(

An intermediate solution could be using OF_DEV_AUXDATA to pass a
platform_data struct that may contain a MAC address set up by the
platform code.


Lothar Waßmann
Sascha Hauer March 15, 2012, 11:24 a.m. UTC | #21
On Thu, Mar 15, 2012 at 06:59:28PM +0800, Dong Aisheng wrote:
> On Thu, Mar 15, 2012 at 02:53:29PM +0800, Lothar Waßmann wrote:
> > Hi,
> > 
> > Dong Aisheng writes:
> > > On Wed, Mar 14, 2012 at 10:16:43PM +0800, s.hauer@pengutronix.de wrote:
> > > > On Wed, Mar 14, 2012 at 08:45:23PM +0800, Dong Aisheng wrote:
> > > > > On Wed, Mar 14, 2012 at 01:23:51AM +0800, Grant Likely wrote:
> > [...]
> > > > > But it seems this needs pass mac address to fec driver via platforom data which is
> > > > > not friendly to dt.
> > > > > 
> > > > > Another way may be changing fec driver to get the fixed part of mac address(first
> > > > > two bytes) from device tree and read the left dynamical part from otp which i'm not
> > > > > sure is better enough.
> > > > > 
> > > > > BTW, filling with zeros seems not work since it's invalid mac address.
> > > > 
> > > > Yes, that's the idea of using this value...
> > > > 
> > > But comparing to use an invalid value, why not just do not define that
> > > local-mac-address property?
> > > 
> > Because a MAC address is by definition a GLOBALLY UNIQUE IDENTIFIER
> > which is contrary to coding a "valid" default value for it somewhere!
> > The owner of a certain MAC address range (OUI) is responsible for
> > ensuring the uniqueness. Thus only they may assign a specific MAC
> > address to a specific device.
> > 
> Yes, you're correct.
> So i propose to read the MAC address from MX28 OTP in fec driver instead of
> define it in device tree in my last mail.
> http://www.spinics.net/lists/arm-kernel/msg165040.html
> Do you have any comment on that way?

Yes, you could do that. Otherwise it's perfectly fine to pass the MAC
address in the device tree, it's just not ok to put specific values
into the kernel source. You could leave the values blank and let the
bootloader fill them in. Or you could fill in valid values before you
compile the devicetree, but be prepared for surprises once you have
two boards in your network and use the devicetree for both.

Sascha
Dong Aisheng March 16, 2012, 3:01 a.m. UTC | #22
On Thu, Mar 15, 2012 at 07:22:04PM +0800, Lothar Waßmann wrote:
> Hi,
> 
> Dong Aisheng writes:
> > On Thu, Mar 15, 2012 at 02:53:29PM +0800, Lothar Waßmann wrote:
> > > Hi,
> > > 
> > > Dong Aisheng writes:
> > > > On Wed, Mar 14, 2012 at 10:16:43PM +0800, s.hauer@pengutronix.de wrote:
> > > > > On Wed, Mar 14, 2012 at 08:45:23PM +0800, Dong Aisheng wrote:
> > > > > > On Wed, Mar 14, 2012 at 01:23:51AM +0800, Grant Likely wrote:
> > > [...]
> > > > > > But it seems this needs pass mac address to fec driver via platforom data which is
> > > > > > not friendly to dt.
> > > > > > 
> > > > > > Another way may be changing fec driver to get the fixed part of mac address(first
> > > > > > two bytes) from device tree and read the left dynamical part from otp which i'm not
> > > > > > sure is better enough.
> > > > > > 
> > > > > > BTW, filling with zeros seems not work since it's invalid mac address.
> > > > > 
> > > > > Yes, that's the idea of using this value...
> > > > > 
> > > > But comparing to use an invalid value, why not just do not define that
> > > > local-mac-address property?
> > > > 
> > > Because a MAC address is by definition a GLOBALLY UNIQUE IDENTIFIER
> > > which is contrary to coding a "valid" default value for it somewhere!
> > > The owner of a certain MAC address range (OUI) is responsible for
> > > ensuring the uniqueness. Thus only they may assign a specific MAC
> > > address to a specific device.
> > > 
> > Yes, you're correct.
> > So i propose to read the MAC address from MX28 OTP in fec driver instead of
> > define it in device tree in my last mail.
> > http://www.spinics.net/lists/arm-kernel/msg165040.html
> > Do you have any comment on that way?
> > 
> That patch sets the OUI to the Freescale owned MAC range. Thus only
> Freescale products may be able to use the driver.
> 
No.
My proposal is only set the fixed part(first two octets) in board dts file,
each board knows it's value, and read the left 4 octets from OCOTP dynamically.
Obviously the freeescale board dts file is only for FSL platforms.
Other platforms can define their fix part of MAC address in their dts file.

> Anyway there is no definite spec how the MAC address(es) are stored
> in the fuse map. Thus reading the MAC from there is more or less
> platform specific.
> 
It's just provide one more option since there are customers storing the MAC
in the fuse map.

> Currently I'm setting up the MAC address for our TX28 from the fuses
> in the platform code passed via platform_data, but that will obviously
> not work with DT.
> 
Non-dt can also use my proposal, then you only need to pass the fixed part of
MAC via platfrom data, the left will be read by fec driver.

> The correct way would probably be to pass the MAC from the bootloader
> via a DT blob. But that would require all bootloaders to be updated
> first to support DTS. :(
> 
But uboot will face the same problem that can not define a valid MAC
in dts, did i undertand correctly?

> An intermediate solution could be using OF_DEV_AUXDATA to pass a
> platform_data struct that may contain a MAC address set up by the
> platform code.
> 
Yes, it's a way but i'm afraid will not get accepted by dt people.

Regards
Dong Aisheng
Dong Aisheng March 16, 2012, 3:05 a.m. UTC | #23
On Thu, Mar 15, 2012 at 07:24:19PM +0800, s.hauer@pengutronix.de wrote:
> On Thu, Mar 15, 2012 at 06:59:28PM +0800, Dong Aisheng wrote:
> > On Thu, Mar 15, 2012 at 02:53:29PM +0800, Lothar Waßmann wrote:
> > > Hi,
> > > 
> > > Dong Aisheng writes:
> > > > On Wed, Mar 14, 2012 at 10:16:43PM +0800, s.hauer@pengutronix.de wrote:
> > > > > On Wed, Mar 14, 2012 at 08:45:23PM +0800, Dong Aisheng wrote:
> > > > > > On Wed, Mar 14, 2012 at 01:23:51AM +0800, Grant Likely wrote:
> > > [...]
> > > > > > But it seems this needs pass mac address to fec driver via platforom data which is
> > > > > > not friendly to dt.
> > > > > > 
> > > > > > Another way may be changing fec driver to get the fixed part of mac address(first
> > > > > > two bytes) from device tree and read the left dynamical part from otp which i'm not
> > > > > > sure is better enough.
> > > > > > 
> > > > > > BTW, filling with zeros seems not work since it's invalid mac address.
> > > > > 
> > > > > Yes, that's the idea of using this value...
> > > > > 
> > > > But comparing to use an invalid value, why not just do not define that
> > > > local-mac-address property?
> > > > 
> > > Because a MAC address is by definition a GLOBALLY UNIQUE IDENTIFIER
> > > which is contrary to coding a "valid" default value for it somewhere!
> > > The owner of a certain MAC address range (OUI) is responsible for
> > > ensuring the uniqueness. Thus only they may assign a specific MAC
> > > address to a specific device.
> > > 
> > Yes, you're correct.
> > So i propose to read the MAC address from MX28 OTP in fec driver instead of
> > define it in device tree in my last mail.
> > http://www.spinics.net/lists/arm-kernel/msg165040.html
> > Do you have any comment on that way?
> 
> Yes, you could do that. Otherwise it's perfectly fine to pass the MAC
> address in the device tree, it's just not ok to put specific values
> into the kernel source. You could leave the values blank and let the
Yes, correct.

> bootloader fill them in. Or you could fill in valid values before you
> compile the devicetree, but be prepared for surprises once you have
> two boards in your network and use the devicetree for both.
> 

Thanks for the info.
I will submit that patch for discussion.

Regards
Dong Aisheng
Lothar Waßmann March 16, 2012, 7:48 a.m. UTC | #24
Hi,

Dong Aisheng writes:
> On Thu, Mar 15, 2012 at 07:22:04PM +0800, Lothar Waßmann wrote:
[...]
> My proposal is only set the fixed part(first two octets) in board dts file,
> each board knows it's value, and read the left 4 octets from OCOTP dynamically.
>
The OUI part is three bytes, not two! But anyway, since there is no
definition on how the MAC has to be stored in OCOTP there is no way
for the driver to know how to interpret the data it may read from
there.

> > Anyway there is no definite spec how the MAC address(es) are stored
> > in the fuse map. Thus reading the MAC from there is more or less
> > platform specific.
> > 
> It's just provide one more option since there are customers storing the MAC
> in the fuse map.
> 
How would the driver know, whether and how the customer has stored the
MAC address in OCOTP? E.g. on our TX28 modules the FULL MAC is stored
in the CUSTOM registers in OCOTP.

> > Currently I'm setting up the MAC address for our TX28 from the fuses
> > in the platform code passed via platform_data, but that will obviously
> > not work with DT.
> > 
> Non-dt can also use my proposal, then you only need to pass the fixed part of
> MAC via platfrom data, the left will be read by fec driver.
> 
Reading the MAC from fuses is platform sepcific. The driver cannot
know how the MAC is stored there and needs to rely on platform
specific code to do this.

> > The correct way would probably be to pass the MAC from the bootloader
> > via a DT blob. But that would require all bootloaders to be updated
> > first to support DTS. :(
> > 
> But uboot will face the same problem that can not define a valid MAC
> in dts, did i undertand correctly?
> 
That's why I said "would require all bootloaders to be updated first
to support DTS".


Lothar Waßmann
Dong Aisheng March 16, 2012, 8:22 a.m. UTC | #25
On Fri, Mar 16, 2012 at 03:48:13PM +0800, Lothar Waßmann wrote:
> Hi,
> 
> Dong Aisheng writes:
> > On Thu, Mar 15, 2012 at 07:22:04PM +0800, Lothar Waßmann wrote:
> [...]
> > My proposal is only set the fixed part(first two octets) in board dts file,
> > each board knows it's value, and read the left 4 octets from OCOTP dynamically.
> >
> The OUI part is three bytes, not two! But anyway, since there is no
> definition on how the MAC has to be stored in OCOTP there is no way
> for the driver to know how to interpret the data it may read from
> there.
> 
I may miss this, i will double check it.
Thanks for the info. :-)

> > > Anyway there is no definite spec how the MAC address(es) are stored
> > > in the fuse map. Thus reading the MAC from there is more or less
> > > platform specific.
> > > 
> > It's just provide one more option since there are customers storing the MAC
> > in the fuse map.
> > 
> How would the driver know, whether and how the customer has stored the
> MAC address in OCOTP? E.g. on our TX28 modules the FULL MAC is stored
> in the CUSTOM registers in OCOTP.
> 
> > > Currently I'm setting up the MAC address for our TX28 from the fuses
> > > in the platform code passed via platform_data, but that will obviously
> > > not work with DT.
> > > 
> > Non-dt can also use my proposal, then you only need to pass the fixed part of
> > MAC via platfrom data, the left will be read by fec driver.
> > 
> Reading the MAC from fuses is platform sepcific. The driver cannot
> know how the MAC is stored there and needs to rely on platform
> specific code to do this.
> 
> > > The correct way would probably be to pass the MAC from the bootloader
> > > via a DT blob. But that would require all bootloaders to be updated
> > > first to support DTS. :(
> > > 
> > But uboot will face the same problem that can not define a valid MAC
> > in dts, did i undertand correctly?
> > 
> That's why I said "would require all bootloaders to be updated first
> to support DTS".

Regards
Dong Aisheng
Shawn Guo March 16, 2012, 8:49 a.m. UTC | #26
On Thu, Mar 15, 2012 at 12:22:04PM +0100, Lothar Waßmann wrote:
...
> An intermediate solution could be using OF_DEV_AUXDATA to pass a
> platform_data struct that may contain a MAC address set up by the
> platform code.
> 
+1
Lothar Waßmann March 19, 2012, 6:54 a.m. UTC | #27
Hi,

Grant Likely writes:
> On Fri, 16 Mar 2012 11:01:35 +0800, Dong Aisheng <aisheng.dong@freescale.com> wrote:
> > On Thu, Mar 15, 2012 at 07:22:04PM +0800, Lothar Waßmann wrote:
> > > Hi,
> > > 
> > > Dong Aisheng writes:
> > > > On Thu, Mar 15, 2012 at 02:53:29PM +0800, Lothar Waßmann wrote:
> > > > > Hi,
> > > > > 
> > > > > Dong Aisheng writes:
> > > > > > On Wed, Mar 14, 2012 at 10:16:43PM +0800, s.hauer@pengutronix.de wrote:
> > > > > > > On Wed, Mar 14, 2012 at 08:45:23PM +0800, Dong Aisheng wrote:
> > > > > > > > On Wed, Mar 14, 2012 at 01:23:51AM +0800, Grant Likely wrote:
> > > > > [...]
> > > > > > > > But it seems this needs pass mac address to fec driver via platforom data which is
> > > > > > > > not friendly to dt.
> > > > > > > > 
> > > > > > > > Another way may be changing fec driver to get the fixed part of mac address(first
> > > > > > > > two bytes) from device tree and read the left dynamical part from otp which i'm not
> > > > > > > > sure is better enough.
> > > > > > > > 
> > > > > > > > BTW, filling with zeros seems not work since it's invalid mac address.
> > > > > > > 
> > > > > > > Yes, that's the idea of using this value...
> > > > > > > 
> > > > > > But comparing to use an invalid value, why not just do not define that
> > > > > > local-mac-address property?
> > > > > > 
> > > > > Because a MAC address is by definition a GLOBALLY UNIQUE IDENTIFIER
> > > > > which is contrary to coding a "valid" default value for it somewhere!
> > > > > The owner of a certain MAC address range (OUI) is responsible for
> > > > > ensuring the uniqueness. Thus only they may assign a specific MAC
> > > > > address to a specific device.
> > > > > 
> > > > Yes, you're correct.
> > > > So i propose to read the MAC address from MX28 OTP in fec driver instead of
> > > > define it in device tree in my last mail.
> > > > http://www.spinics.net/lists/arm-kernel/msg165040.html
> > > > Do you have any comment on that way?
> > > > 
> > > That patch sets the OUI to the Freescale owned MAC range. Thus only
> > > Freescale products may be able to use the driver.
> > > 
> > No.
> > My proposal is only set the fixed part(first two octets) in board dts file,
> > each board knows it's value, and read the left 4 octets from OCOTP dynamically.
> > Obviously the freeescale board dts file is only for FSL platforms.
> > Other platforms can define their fix part of MAC address in their dts file.
> > 
> > > Anyway there is no definite spec how the MAC address(es) are stored
> > > in the fuse map. Thus reading the MAC from there is more or less
> > > platform specific.
> > > 
> > It's just provide one more option since there are customers storing the MAC
> > in the fuse map.
> 
> That should be straight forward to support; have a property that
> specifies the method used for fetching/calculating the MAC.
> 
Executable code stored inside a DT blob? ;)

> > > Currently I'm setting up the MAC address for our TX28 from the fuses
> > > in the platform code passed via platform_data, but that will obviously
> > > not work with DT.
> > > 
> > Non-dt can also use my proposal, then you only need to pass the fixed part of
> > MAC via platfrom data, the left will be read by fec driver.
> > 
> > > The correct way would probably be to pass the MAC from the bootloader
> > > via a DT blob. But that would require all bootloaders to be updated
> > > first to support DTS. :(
> > > 
> > But uboot will face the same problem that can not define a valid MAC
> > in dts, did i undertand correctly?
> > 
> > > An intermediate solution could be using OF_DEV_AUXDATA to pass a
> > > platform_data struct that may contain a MAC address set up by the
> > > platform code.
> > > 
> > Yes, it's a way but i'm afraid will not get accepted by dt people.
> 
> The problem remains; where does the platform code get the MAC address
> from?  The kernel has to devise it from *somewhere* and the best place
> for that should be in the MAC driver itself.
> 
The platform code knows how and where the MAC is stored on a specific
platform. The driver has no business in knowing platform details like
this. Thus only platform code (or the bootloader) can provide the MAC
to the driver.

> I've got no problem with the idea of only specifying the OUI in the DT
> and the driver extracting the remaining 3 bytes from the hardware.
> 
> In principle, that's the design intent for DT systems anyway; it
> describes things which cannot be probed from the hardware, or tells
> the OS how the data is encoded in the hardware.
> 
If all bootloaders were aware of DT this wouldn't be a problem,
because they could place the correct MAC inside the DT blob right
away.
But in the mean time (AKA forever) we need some other solution.


Lothar Waßmann
Lothar Waßmann March 19, 2012, 4:49 p.m. UTC | #28
Hi,

Grant Likely writes:
> On Mon, 19 Mar 2012 07:54:33 +0100, Lothar Waßmann <LW@KARO-electronics.de> wrote:
> > Grant Likely writes:
> > > On Fri, 16 Mar 2012 11:01:35 +0800, Dong Aisheng <aisheng.dong@freescale.com> wrote:
> > > > On Thu, Mar 15, 2012 at 07:22:04PM +0800, Lothar Waßmann wrote:
> > > > > Dong Aisheng writes:
> > > > > > On Thu, Mar 15, 2012 at 02:53:29PM +0800, Lothar Waßmann wrote:
> > > > > Anyway there is no definite spec how the MAC address(es) are stored
> > > > > in the fuse map. Thus reading the MAC from there is more or less
> > > > > platform specific.
> > > > > 
> > > > It's just provide one more option since there are customers storing the MAC
> > > > in the fuse map.
> > > 
> > > That should be straight forward to support; have a property that
> > > specifies the method used for fetching/calculating the MAC.
> > > 
> > Executable code stored inside a DT blob? ;)
> 
> I know you're joking here, but I'm going to answer seriously
> anyway... Absolutely not.  What I'm suggesting is a property that
> specifies the method used to determine the mac address.  Something
> like (off the top of my head):
> 
> 	local-mac-address = [01 02 03 00 00 00];
> 	local-mac-mask = [0xff 0xff 0xff 0 0 0];
> 	mac-encoding = "append-serial-number";
> 
That still does not specify where the remaining part of the MAC is
stored and how it should be retrieved.

> > > > > Currently I'm setting up the MAC address for our TX28 from the fuses
> > > > > in the platform code passed via platform_data, but that will obviously
> > > > > not work with DT.
> > > > > 
> > > > Non-dt can also use my proposal, then you only need to pass the fixed part of
> > > > MAC via platfrom data, the left will be read by fec driver.
> > > > 
> > > > > The correct way would probably be to pass the MAC from the bootloader
> > > > > via a DT blob. But that would require all bootloaders to be updated
> > > > > first to support DTS. :(
> > > > > 
> > > > But uboot will face the same problem that can not define a valid MAC
> > > > in dts, did i undertand correctly?
> > > > 
> > > > > An intermediate solution could be using OF_DEV_AUXDATA to pass a
> > > > > platform_data struct that may contain a MAC address set up by the
> > > > > platform code.
> > > > > 
> > > > Yes, it's a way but i'm afraid will not get accepted by dt people.
> > > 
> > > The problem remains; where does the platform code get the MAC address
> > > from?  The kernel has to devise it from *somewhere* and the best place
> > > for that should be in the MAC driver itself.
> > > 
> > The platform code knows how and where the MAC is stored on a specific
> > platform. The driver has no business in knowing platform details like
> > this. Thus only platform code (or the bootloader) can provide the MAC
> > to the driver.
> 
> Okay, if so then it would be wise to have a reliable function for the
> MAC driver to call to lookup it's address as determined by platform
> code.  Alternately, the platform code can write the correct mac
> address into the device tree node at init time (see
> prom_update_property() and prom_add_property()).
> 
That sounds good. Didn't know about those functions. That could be
used to mimic the current behaviour of supplying the MAC via
platform_data.


Lothar Waßmann
Dong Aisheng March 20, 2012, 12:49 p.m. UTC | #29
On Mon, Mar 19, 2012 at 3:02 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Mon, 19 Mar 2012 17:49:02 +0100, Lothar Waßmann <LW@KARO-electronics.de> wrote:
>> Hi,
>>
>> Grant Likely writes:
>> > On Mon, 19 Mar 2012 07:54:33 +0100, Lothar Waßmann <LW@KARO-electronics.de> wrote:
>> > > Grant Likely writes:
>> > > > On Fri, 16 Mar 2012 11:01:35 +0800, Dong Aisheng <aisheng.dong@freescale.com> wrote:
>> > > > > On Thu, Mar 15, 2012 at 07:22:04PM +0800, Lothar Waßmann wrote:
>> > > > > > Dong Aisheng writes:
>> > > > > > > On Thu, Mar 15, 2012 at 02:53:29PM +0800, Lothar Waßmann wrote:
>> > > > > > Anyway there is no definite spec how the MAC address(es) are stored
>> > > > > > in the fuse map. Thus reading the MAC from there is more or less
>> > > > > > platform specific.
>> > > > > >
>> > > > > It's just provide one more option since there are customers storing the MAC
>> > > > > in the fuse map.
>> > > >
>> > > > That should be straight forward to support; have a property that
>> > > > specifies the method used for fetching/calculating the MAC.
>> > > >
>> > > Executable code stored inside a DT blob? ;)
>> >
>> > I know you're joking here, but I'm going to answer seriously
>> > anyway... Absolutely not.  What I'm suggesting is a property that
>> > specifies the method used to determine the mac address.  Something
>> > like (off the top of my head):
>> >
>> >     local-mac-address = [01 02 03 00 00 00];
>> >     local-mac-mask = [0xff 0xff 0xff 0 0 0];
>> >     mac-encoding = "append-serial-number";
>> >
>> That still does not specify where the remaining part of the MAC is
>> stored and how it should be retrieved.
>
> I'm suggesting that you define a string that means something specific;
> that hopefully can be shared by multiple platforms.  For example,
> "append-serial-number" might mean start with the values selected by
> AND of local-mac-address and local-mac-mask, and OR in the board's
> serial number.  You would need to define something that worked if this
> was the solution you used.
>
I'm intend to Grant's this suggestion.
This can be shared by all other imx platforms which stores mac address in
fuse map and this is commonly used by customer.
Then we do not need keep writing repeat code for different platforms
via platform
data.

For Lothar's question, we can add a property fuse_mac_offset to indicate
read mac from fuse map and where to read it.
For how many bytes to read, we can calculate it from the local-mac-mask.
For example, local-mac-mask = [0xff 0xff 0xff 0 0 0], we can get the size
as 3 bypes.

Then we have three properties:
local-mac-address = [01 02 03 00 00 00];
local-mac-mask = [0xff 0xff 0xff 0 0 0];
fuse_mac_offset = <1>;

In fec driver, the final mac address can be got by:
local-mac-address & local-mac-mask | (read_fuse(1) & 0xffffff)

Lathar,
Do you think if this is ok?

>> > Okay, if so then it would be wise to have a reliable function for the
>> > MAC driver to call to lookup it's address as determined by platform
>> > code.  Alternately, the platform code can write the correct mac
>> > address into the device tree node at init time (see
>> > prom_update_property() and prom_add_property()).
>> >
>> That sounds good. Didn't know about those functions. That could be
>> used to mimic the current behaviour of supplying the MAC via
>> platform_data.
>
> I'm okay with doing this; but make sure you remove the bogus
> local-mac-address from the .dts file.
>
This is a backup solution to me if the above solution you suggested can not
work.

Regards
Dong Aisheng
Lothar Waßmann March 20, 2012, 1:17 p.m. UTC | #30
Hi,

Dong Aisheng writes:
> On Mon, Mar 19, 2012 at 3:02 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> > On Mon, 19 Mar 2012 17:49:02 +0100, Lothar Waßmann <LW@KARO-electronics.de> wrote:
> >> Hi,
> >>
> >> Grant Likely writes:
> >> > On Mon, 19 Mar 2012 07:54:33 +0100, Lothar Waßmann <LW@KARO-electronics.de> wrote:
> >> > > Grant Likely writes:
> >> > > > On Fri, 16 Mar 2012 11:01:35 +0800, Dong Aisheng <aisheng.dong@freescale.com> wrote:
> >> > > > > On Thu, Mar 15, 2012 at 07:22:04PM +0800, Lothar Waßmann wrote:
> >> > > > > > Dong Aisheng writes:
> >> > > > > > > On Thu, Mar 15, 2012 at 02:53:29PM +0800, Lothar Waßmann wrote:
> >> > > > > > Anyway there is no definite spec how the MAC address(es) are stored
> >> > > > > > in the fuse map. Thus reading the MAC from there is more or less
> >> > > > > > platform specific.
> >> > > > > >
> >> > > > > It's just provide one more option since there are customers storing the MAC
> >> > > > > in the fuse map.
> >> > > >
> >> > > > That should be straight forward to support; have a property that
> >> > > > specifies the method used for fetching/calculating the MAC.
> >> > > >
> >> > > Executable code stored inside a DT blob? ;)
> >> >
> >> > I know you're joking here, but I'm going to answer seriously
> >> > anyway... Absolutely not.  What I'm suggesting is a property that
> >> > specifies the method used to determine the mac address.  Something
> >> > like (off the top of my head):
> >> >
> >> >     local-mac-address = [01 02 03 00 00 00];
> >> >     local-mac-mask = [0xff 0xff 0xff 0 0 0];
> >> >     mac-encoding = "append-serial-number";
> >> >
> >> That still does not specify where the remaining part of the MAC is
> >> stored and how it should be retrieved.
> >
> > I'm suggesting that you define a string that means something specific;
> > that hopefully can be shared by multiple platforms.  For example,
> > "append-serial-number" might mean start with the values selected by
> > AND of local-mac-address and local-mac-mask, and OR in the board's
> > serial number.  You would need to define something that worked if this
> > was the solution you used.
> >
> I'm intend to Grant's this suggestion.
> This can be shared by all other imx platforms which stores mac address in
> fuse map and this is commonly used by customer.
> Then we do not need keep writing repeat code for different platforms
> via platform
> data.
> 
> For Lothar's question, we can add a property fuse_mac_offset to indicate
> read mac from fuse map and where to read it.
>
That's not really enough. The format (big-endian vs. little-endian)
may also differ. But it's not really necessary, if the solution with
prom_update_property() works, since then platform code has full
control over the MAC.

> For how many bytes to read, we can calculate it from the local-mac-mask.
> For example, local-mac-mask = [0xff 0xff 0xff 0 0 0], we can get the size
> as 3 bypes.
> 
> Then we have three properties:
> local-mac-address = [01 02 03 00 00 00];
> local-mac-mask = [0xff 0xff 0xff 0 0 0];
> fuse_mac_offset = <1>;
> 
> In fec driver, the final mac address can be got by:
> local-mac-address & local-mac-mask | (read_fuse(1) & 0xffffff)
> 
> Lathar,
> Do you think if this is ok?
>
No. IMO the FEC driver itself should not read any fuses.
Reading fuses is a SoC specific thing and the FEC driver should not
depend on the idiosyncrasies of some SoC wrt. anything else but the
implementation of the FEC itself.


Lothar Waßmann
Dong Aisheng March 21, 2012, 11:06 a.m. UTC | #31
On Tue, Mar 20, 2012 at 09:17:48PM +0800, Lothar Waßmann wrote:
> Hi,
> 
> Dong Aisheng writes:
> > On Mon, Mar 19, 2012 at 3:02 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> > > On Mon, 19 Mar 2012 17:49:02 +0100, Lothar Waßmann <LW@KARO-electronics.de> wrote:
> > >> Hi,
> > >>
> > >> Grant Likely writes:
> > >> > On Mon, 19 Mar 2012 07:54:33 +0100, Lothar Waßmann <LW@KARO-electronics.de> wrote:
> > >> > > Grant Likely writes:
> > >> > > > On Fri, 16 Mar 2012 11:01:35 +0800, Dong Aisheng <aisheng.dong@freescale.com> wrote:
> > >> > > > > On Thu, Mar 15, 2012 at 07:22:04PM +0800, Lothar Waßmann wrote:
> > >> > > > > > Dong Aisheng writes:
> > >> > > > > > > On Thu, Mar 15, 2012 at 02:53:29PM +0800, Lothar Waßmann wrote:
> > >> > > > > > Anyway there is no definite spec how the MAC address(es) are stored
> > >> > > > > > in the fuse map. Thus reading the MAC from there is more or less
> > >> > > > > > platform specific.
> > >> > > > > >
> > >> > > > > It's just provide one more option since there are customers storing the MAC
> > >> > > > > in the fuse map.
> > >> > > >
> > >> > > > That should be straight forward to support; have a property that
> > >> > > > specifies the method used for fetching/calculating the MAC.
> > >> > > >
> > >> > > Executable code stored inside a DT blob? ;)
> > >> >
> > >> > I know you're joking here, but I'm going to answer seriously
> > >> > anyway... Absolutely not.  What I'm suggesting is a property that
> > >> > specifies the method used to determine the mac address.  Something
> > >> > like (off the top of my head):
> > >> >
> > >> >     local-mac-address = [01 02 03 00 00 00];
> > >> >     local-mac-mask = [0xff 0xff 0xff 0 0 0];
> > >> >     mac-encoding = "append-serial-number";
> > >> >
> > >> That still does not specify where the remaining part of the MAC is
> > >> stored and how it should be retrieved.
> > >
> > > I'm suggesting that you define a string that means something specific;
> > > that hopefully can be shared by multiple platforms.  For example,
> > > "append-serial-number" might mean start with the values selected by
> > > AND of local-mac-address and local-mac-mask, and OR in the board's
> > > serial number.  You would need to define something that worked if this
> > > was the solution you used.
> > >
> > I'm intend to Grant's this suggestion.
> > This can be shared by all other imx platforms which stores mac address in
> > fuse map and this is commonly used by customer.
> > Then we do not need keep writing repeat code for different platforms
> > via platform
> > data.
> > 
> > For Lothar's question, we can add a property fuse_mac_offset to indicate
> > read mac from fuse map and where to read it.
> >
> That's not really enough. The format (big-endian vs. little-endian)
> may also differ. But it's not really necessary, if the solution with
> prom_update_property() works, since then platform code has full
> control over the MAC.
> 
> > For how many bytes to read, we can calculate it from the local-mac-mask.
> > For example, local-mac-mask = [0xff 0xff 0xff 0 0 0], we can get the size
> > as 3 bypes.
> > 
> > Then we have three properties:
> > local-mac-address = [01 02 03 00 00 00];
> > local-mac-mask = [0xff 0xff 0xff 0 0 0];
> > fuse_mac_offset = <1>;
> > 
> > In fec driver, the final mac address can be got by:
> > local-mac-address & local-mac-mask | (read_fuse(1) & 0xffffff)
> > 
> > Lathar,
> > Do you think if this is ok?
> >
> No. IMO the FEC driver itself should not read any fuses.
> Reading fuses is a SoC specific thing and the FEC driver should not
> depend on the idiosyncrasies of some SoC wrt. anything else but the
> implementation of the FEC itself.
> 
Yes, i think i'm agree with this point.
Okay, i will do that with prom_update_property.
Thanks for suggestion.

Regards
Dong Aisheng
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/arm/fsl.txt b/Documentation/devicetree/bindings/arm/fsl.txt
index 54bddda..9f21faf 100644
--- a/Documentation/devicetree/bindings/arm/fsl.txt
+++ b/Documentation/devicetree/bindings/arm/fsl.txt
@@ -1,6 +1,10 @@ 
 Freescale i.MX Platforms Device Tree Bindings
 -----------------------------------------------
 
+i.MX28 Evaluation Kit
+Required root node properties:
+    - compatible = "fsl,imx28-evk", "fsl,imx28";
+
 i.MX51 Babbage Board
 Required root node properties:
     - compatible = "fsl,imx51-babbage", "fsl,imx51";
diff --git a/arch/arm/boot/dts/imx28-evk.dts b/arch/arm/boot/dts/imx28-evk.dts
new file mode 100644
index 0000000..9758dc4
--- /dev/null
+++ b/arch/arm/boot/dts/imx28-evk.dts
@@ -0,0 +1,31 @@ 
+/*
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+/dts-v1/;
+/include/ "imx28.dtsi"
+
+/ {
+	model = "Freescale i.MX28 Evaluation Kit";
+	compatible = "fsl,imx28-evk", "fsl,imx28";
+
+	memory {
+		device_type = "memory";
+		reg = <0x40000000 0x08000000>;
+	};
+
+	ahb@80080000 {
+		fec@800f0000 {
+			phy-mode = "rmii";
+			local-mac-address = [00 04 9F 01 7D 5B];
+			status = "okay";
+		};
+	};
+};
diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
new file mode 100644
index 0000000..acf0dab
--- /dev/null
+++ b/arch/arm/boot/dts/imx28.dtsi
@@ -0,0 +1,88 @@ 
+/*
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+/include/ "skeleton.dtsi"
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+	interrupt-parent = <&icoll>;
+
+	aliases {
+		serial0 = &uart1;
+	};
+
+	cpus {
+		cpu@0 {
+			compatible = "arm,arm926ejs";
+		};
+	};
+
+	apb@80000000 {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		reg = <0x80000000 0x80000>;
+		ranges;
+
+		apbh@80000000 {
+			compatible = "simple-bus";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			reg = <0x80000000 0x3c900>;
+			ranges;
+
+			icoll: interrupt-controller@80000000 {
+				compatible = "fsl,imx28-icoll";
+				interrupt-controller;
+				#interrupt-cells = <1>;
+				reg = <0x80000000 0x2000>;
+			};
+		};
+
+		apbx@80040000 {
+			compatible = "simple-bus";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			reg = <0x80040000 0x40000>;
+			ranges;
+
+			uart1: uart@80074000 {
+				compatible = "arm,pl011", "arm,primecell";
+				reg = <0x80074000 0x2000>;
+				interrupts = <47>;
+			};
+		};
+	};
+
+	ahb@80080000 {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		reg = <0x80080000 0x80000>;
+		ranges;
+
+		fec@800f0000 {
+			compatible = "fsl,imx28-fec";
+			reg = <0x800f0000 0x4000>;
+			interrupts = <101>;
+			status = "disabled";
+		};
+
+		fec@800f4000 {
+			compatible = "fsl,imx28-fec";
+			reg = <0x800f4000 0x4000>;
+			interrupts = <102>;
+			status = "disabled";
+		};
+
+	};
+};
diff --git a/arch/arm/mach-mxs/Kconfig b/arch/arm/mach-mxs/Kconfig
index c57f996..6ab66af 100644
--- a/arch/arm/mach-mxs/Kconfig
+++ b/arch/arm/mach-mxs/Kconfig
@@ -17,6 +17,15 @@  config SOC_IMX28
 
 comment "MXS platforms:"
 
+config MACH_IMX28_DT
+	bool "Support i.MX28 platforms from device tree"
+	select SOC_IMX28
+	select USE_OF
+	select MACH_MX28EVK
+	help
+	  Include support for Freescale i.MX28 based platforms
+	  using the device tree for discovery
+
 config MACH_STMP378X_DEVB
 	bool "Support STMP378x_devb Platform"
 	select SOC_IMX23
diff --git a/arch/arm/mach-mxs/Makefile b/arch/arm/mach-mxs/Makefile
index 908bf9a..25a4122 100644
--- a/arch/arm/mach-mxs/Makefile
+++ b/arch/arm/mach-mxs/Makefile
@@ -7,6 +7,7 @@  obj-$(CONFIG_PM) += pm.o
 obj-$(CONFIG_SOC_IMX23) += clock-mx23.o
 obj-$(CONFIG_SOC_IMX28) += clock-mx28.o
 
+obj-$(CONFIG_MACH_IMX28_DT) += imx28-dt.o
 obj-$(CONFIG_MACH_STMP378X_DEVB) += mach-stmp378x_devb.o
 obj-$(CONFIG_MACH_MX23EVK) += mach-mx23evk.o
 obj-$(CONFIG_MACH_MX28EVK) += mach-mx28evk.o
diff --git a/arch/arm/mach-mxs/imx28-dt.c b/arch/arm/mach-mxs/imx28-dt.c
new file mode 100644
index 0000000..78d1129
--- /dev/null
+++ b/arch/arm/mach-mxs/imx28-dt.c
@@ -0,0 +1,67 @@ 
+/*
+ * Copyright 2011 Freescale Semiconductor, Inc.
+ * Copyright 2011 Linaro Ltd.
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include <linux/init.h>
+#include <linux/irqdomain.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <asm/mach/arch.h>
+#include <asm/mach/time.h>
+#include <mach/common.h>
+#include <mach/mx28.h>
+
+static const struct of_dev_auxdata imx28_auxdata_lookup[] __initconst = {
+	OF_DEV_AUXDATA("arm,pl011", MX28_DUART_BASE_ADDR, "duart", NULL),
+	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC0_BASE_ADDR, "imx28-fec.0", NULL),
+	OF_DEV_AUXDATA("fsl,imx28-fec", MX28_ENET_MAC1_BASE_ADDR, "imx28-fec.1", NULL),
+	{ /* sentinel */ }
+};
+
+static void __init imx28_init(void)
+{
+	of_platform_populate(NULL, of_default_bus_match_table,
+			     imx28_auxdata_lookup, NULL);
+}
+
+static const struct of_device_id icoll_of_match[] __initconst = {
+	{ .compatible = "fsl,imx28-icoll", },
+	{},
+};
+
+static void __init imx28_dt_init_irq(void)
+{
+	irq_domain_generate_simple(icoll_of_match, MX28_ICOLL_BASE_ADDR, 0);
+	mx28_init_irq();
+}
+
+static void __init imx28_timer_init(void)
+{
+	mx28_clocks_init();
+}
+
+static struct sys_timer imx28_timer = {
+	.init = imx28_timer_init,
+};
+
+static const char *imx28_dt_compat[] __initdata = {
+	"fsl,imx28-evk",
+	NULL,
+};
+
+DT_MACHINE_START(IMX28, "Freescale i.MX28 (Device Tree)")
+	.map_io		= mx28_map_io,
+	.init_irq	= imx28_dt_init_irq,
+	.timer		= &imx28_timer,
+	.init_machine	= imx28_init,
+	.dt_compat	= imx28_dt_compat,
+	.restart	= mxs_restart,
+MACHINE_END