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

[Ley Foon Tan <lftan@altera.com>]

On Tue, Apr 22, 2014 at 9:42 PM, Arnd Bergmann <arnd@arndb.de> 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