Re: [PATCH 19/28] nios2: Device tree support

[Arnd Bergmann <arnd@arndb.de>]

On Friday 18 April 2014, Ley Foon Tan wrote:
> diff --git a/arch/nios2/boot/dts/3c120_devboard.dts b/arch/nios2/boot/dts/3c120_devboard.dts
> new file mode 100644
> index 0000000..cad29a9
> --- /dev/null
> +++ b/arch/nios2/boot/dts/3c120_devboard.dts
> @@ -0,0 +1,205 @@
> +/*
> + *  Copyright (C) 2013 Altera Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + *
> + * This file is generated by sopc2dts.
> + */
> +
> +/dts-v1/;
> +
> +/ {
> +	model = "ALTR,qsys_ghrd_3c120";
> +	compatible = "ALTR,qsys_ghrd_3c120";

You have a mix of "ALTR" and "altr" prefixes. The general recommendation
is to use lower-case letters, which is also what is used on ARM socfpga,
and what is documented in Documentation/devicetree/bindings/vendor-prefixes.txt
for Altera.

> +	sopc@0 {
> +		device_type = "soc";
> +		ranges;
> +		#address-cells = < 1 >;
> +		#size-cells = < 1 >;
> +		compatible = "ALTR,avalon", "simple-bus";
> +		bus-frequency = < 125000000 >;
> +
> +		pb_cpu_to_io: bridge@0x8000000 {
> +			compatible = "simple-bus";
> +			reg = < 0x08000000 0x00800000 >;

Are these all synthesized devices, or is there also some hardwired
logic? It often makes sense to split out the reusable parts into
a separate .dtsi file that gets included by every implementation.

> +			#address-cells = < 1 >;
> +			#size-cells = < 1 >;
> +			ranges = < 0x00400000 0x08400000 0x00000020
> +				0x00004D40 0x08004D40 0x00000008
> +				0x00004D50 0x08004D50 0x00000008
> +				0x00004000 0x08004000 0x00000400
> +				0x00004400 0x08004400 0x00000040
> +				0x00004800 0x08004800 0x00000040
> +				0x00002000 0x08002000 0x00002000
> +				0x00004C80 0x08004C80 0x00000020
> +				0x00004CC0 0x08004CC0 0x00000010
> +				0x00004CE0 0x08004CE0 0x00000010
> +				0x00004D00 0x08004D00 0x00000010 >;

A few style comments:

- no whitespace in the after '<' or before '>
- put each entry into its own '<...>' group.
- lower-case characters for hex digits
- The ranges should reflect what the bus actually translates,
  which is typically not individual bytes but rather whole
  address ranges.
- sort numerically.

The above could look like

			ranges = <0x00000000 0x08000000 0x00010000>,
				 <0x00400000 0x08400000 0x00001000>;

> +			timer_1ms: timer@0x400000 {
> +				compatible = "ALTR,timer-1.0";
> +
> +			sysid: sysid@0x4d40 {
> +				compatible = "ALTR,sysid-1.0";
> +				reg = < 0x00004D40 0x00000008 >;

> +			jtag_uart: serial@0x4d50 {
> +				compatible = "ALTR,juart-1.0";
> +				reg = < 0x00004D50 0x00000008 >;
> +
> +			tse_mac: ethernet@0x4000 {
> +				compatible = "ALTR,tse-1.0";


Does each one of these have a binding document in
Documentation/devicetree/bindings?

I've looked only at the tse binding, which you seem to be
violating in a few places:

- compatible string is "ALTR,tse-1.0", not "altr,tse-1.0"

> +				reg = < 0x00004000 0x00000400
> +					0x00004400 0x00000040
> +					0x00004800 0x00000040
> +					0x00002000 0x00002000 >;
> +                                reg-names = "control_port", "rx_csr", "tx_csr", "s1";

- wrong order, missing "tx_desc" and "rx_desc" entries

> +				rx-fifo-depth = < 8192 >;
> +				tx-fifo-depth = < 8192 >;
> +				address-bits = < 48 >;

address-bits is not documented

> +				max-frame-size = < 1518 >;
> +				local-mac-address = [ 02 00 00 00 00 00 ];
> +				phy-mode = "rgmii-id";
> +				ALTR,mii-id = < 0 >;

ALTR,mii-id is not documented, and required "phy-addr" is missing.


> diff --git a/arch/nios2/boot/linked_dtb.S b/arch/nios2/boot/linked_dtb.S
> new file mode 100644
> index 0000000..071f922db338e2cb4064bc77bf346f50e584d04f
> --- /dev/null
> +++ b/arch/nios2/boot/linked_dtb.S
> + */
> +.section .dtb.init.rodata,"a"
> +.incbin "arch/nios2/boot/system.dtb"

Linking in the dtb file is really against the point of device trees.
You should require boot loaders to pass the dtb seperately.

> +       soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "%u", NIOS2_ID_DEFAULT);
> +       soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%d",
> +               NIOS2_REVISION_DEFAULT);

These are hardcoded constants. If there is no way to identify the
hardware from looking at the registers, better don't fill these at
all.

> +	soc_dev = soc_device_register(soc_dev_attr);
> +	if (IS_ERR_OR_NULL(soc_dev)) {

Never use IS_ERR_OR_NULL().

If an interface can return an error code, you should rely on
never seeing NULL, or treating it as a valid pointer.

> +static int __init nios2_device_probe(void)
> +{
> +	nios2_soc_device_init();
> +
> +	of_platform_bus_probe(NULL, altera_of_bus_ids, NULL);
> +	return 0;
> +}

This function can get merged into nios2_soc_device_init.

	Arnd