Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754129AbaDWGwv (ORCPT ); Wed, 23 Apr 2014 02:52:51 -0400 Received: from mail-ie0-f180.google.com ([209.85.223.180]:41591 "EHLO mail-ie0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753442AbaDWGwt (ORCPT ); Wed, 23 Apr 2014 02:52:49 -0400 MIME-Version: 1.0 In-Reply-To: <201404221542.16481.arnd@arndb.de> References: <1397824031-4892-1-git-send-email-lftan@altera.com> <1397824031-4892-16-git-send-email-lftan@altera.com> <201404221542.16481.arnd@arndb.de> Date: Wed, 23 Apr 2014 14:52:47 +0800 X-Google-Sender-Auth: 90JpEunl3dJ4UC_Fl2SOTFBGPnc Message-ID: Subject: Re: [PATCH 19/28] nios2: Device tree support From: Ley Foon Tan To: Arnd Bergmann Cc: Linux-Arch , "linux-kernel@vger.kernel.org" , "linux-doc@vger.kernel.org" , cltang@codesourcery.com Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Apr 22, 2014 at 9:42 PM, Arnd Bergmann wrote: > 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 >> +/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. Yes, the vendor prefix should be changed to lower case. FYI, this dts file is generated by our dts generator tool called sopc2dts. Our tool already aware of this requirement, but haven't support this yet. I can edit this file manually to change 'ALTR' to 'altr', but I will update nios2 code to match for both "ALTR" and "altr" for backward compatibility. "ALTR" will be deprecated later. Are you okay with this? > >> + 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. All these are synthesized devices and the system hierarchy is also not fixed and it is depends on user hardware design. It is highly configurable. So, we can't have a common .dtsi file. > >> + #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 Okay, I will change this and I will feedback this to our dts generator tool team to support these format in future. > - The ranges should reflect what the bus actually translates, > which is typically not individual bytes but rather whole > address ranges. The ranges here reflect the address range translate by each device and user can assign any base address they desired in our FPGA. The address ranges might not in continuous regions as well. So, we will keep this translation way. > - sort numerically. > > The above could look like > > ranges = <0x00000000 0x08000000 0x00010000>, > <0x00400000 0x08400000 0x00001000>; Okay, I will sort this. > >> + 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? jtag_uart and tse drivers are upstreamed. They already have their own bindings doc: Documentation/devicetree/bindings/serial/altera_jtaguart.txt Documentation/devicetree/bindings/net/altera_tse.txt I will add the binding doc for timer, but sysid driver is not in mainline kernel yet. I will remove sysid entry from this file. > > 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" I will change to "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 FYI, TSE driver supports both SGDMA and MSGDMA soft IP and "tx_desc" and "rx_desc" entries are for MSGDMA (see below). But this 3c120 hardware design is using TSE + SGDMA. So, the expect entries are "control_port", "rx_csr", "tx_csr", "s1". I can sort the entries order by their addresses. Excerpt from Documentation/devicetree/bindings/net/altera_tse.txt - reg-names: Should contain the reg names "control_port": MAC configuration space region "tx_csr": xDMA Tx dispatcher control and status space region "tx_desc": MSGDMA Tx dispatcher descriptor space region "rx_csr" : xDMA Rx dispatcher control and status space region "rx_desc": MSGDMA Rx dispatcher descriptor space region "rx_resp": MSGDMA Rx dispatcher response space region "s1": SGDMA descriptor memory > >> + rx-fifo-depth = < 8192 >; >> + tx-fifo-depth = < 8192 >; >> + address-bits = < 48 >; > > address-bits is not documented Will remove this. > >> + 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. Will remove ALTR,mii-id. "phy-addr" is not required if we provide "phy-handle". Excerpt from Documentation/devicetree/bindings/net/altera_tse.txt: - phy-addr: See ethernet.txt in the same directory. A configuration should include phy-handle or phy-addr. > > >> 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. We do support pass the dtb from boot loaders. This is optional feature that allow user to linking in dtb into kernel image if CONFIG_DTB_SOURCE_BOOL is enabled. This is very useful for develpment stage/debugging stage, where we can download vmlinux image directly into SDRAM and boot without boot loader. Noticed that other architectures have linking in dtd file as well, eg: c6x and microblaze. > >> + 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. Okay, I will leave them empty. > >> + 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. Okay, will change it to IS_ERR(). > >> +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. Okay, will merge nios2_device_probe() into nios2_soc_device_init(). Thanks. Regards Ley Foon -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/