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

From: Arnd Bergmann
Date: Tue Apr 22 2014 - 09:42:31 EST


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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/