| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <201404221542.16481.arnd@arndb.de>
Date: Tue, 22 Apr 2014 15:42:16 +0200
From: Arnd Bergmann <arnd@...db.de>
To: Ley Foon Tan <lftan@...era.com>
Cc: linux-arch@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-doc@...r.kernel.org, lftan.linux@...il.com,
cltang@...esourcery.com
Subject: Re: [PATCH 19/28] nios2: Device tree support
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@...000000 {
> + 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@...00000 {
> + compatible = "ALTR,timer-1.0";
> +
> + sysid: sysid@...d40 {
> + compatible = "ALTR,sysid-1.0";
> + reg = < 0x00004D40 0x00000008 >;
> + jtag_uart: serial@...d50 {
> + compatible = "ALTR,juart-1.0";
> + reg = < 0x00004D50 0x00000008 >;
> +
> + tse_mac: ethernet@...000 {
> + 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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists