Re: [PATCH 2/7] arm64: zynqmp: Add support for Xilinx zcu102

[Michal Simek]

Hi Rob,



On 1.2.2018 17:46, Rob Herring wrote:
> On Fri, Jan 19, 2018 at 6:55 AM, Michal Simek <michal.simek@xxxxxxxxxx> wrote:
>> This patch is adding revA, revB and rev1.0. There are also other
>> revisions between which should be backward compatible with previous
>> versions. Unfortunately all revs are still in use.
> 
> Similar comments to the 1st patch. I won't repeat them here.
> 
>>
>> Signed-off-by: Michal Simek <michal.simek@xxxxxxxxxx>
>> ---
>>
>>  arch/arm64/boot/dts/xilinx/Makefile                |   3 +
>>  .../arm64/boot/dts/xilinx/zynqmp-zcu102-rev1.0.dts |  36 ++
>>  arch/arm64/boot/dts/xilinx/zynqmp-zcu102-revA.dts  | 556 +++++++++++++++++++++
>>  arch/arm64/boot/dts/xilinx/zynqmp-zcu102-revB.dts  |  42 ++
>>  4 files changed, 637 insertions(+)
>>  create mode 100644 arch/arm64/boot/dts/xilinx/zynqmp-zcu102-rev1.0.dts
>>  create mode 100644 arch/arm64/boot/dts/xilinx/zynqmp-zcu102-revA.dts
>>  create mode 100644 arch/arm64/boot/dts/xilinx/zynqmp-zcu102-revB.dts
>>
>> diff --git a/arch/arm64/boot/dts/xilinx/Makefile b/arch/arm64/boot/dts/xilinx/Makefile
>> index 7266a6a9c0cd..24e3ce801304 100644
>> --- a/arch/arm64/boot/dts/xilinx/Makefile
>> +++ b/arch/arm64/boot/dts/xilinx/Makefile
>> @@ -1,3 +1,6 @@
>>  # SPDX-License-Identifier: GPL-2.0
>>  dtb-$(CONFIG_ARCH_ZYNQMP) += zynqmp-ep108.dtb
>>  dtb-$(CONFIG_ARCH_ZYNQMP) += zynqmp-zcu100-revC.dtb
>> +dtb-$(CONFIG_ARCH_ZYNQMP) += zynqmp-zcu102-revA.dtb
>> +dtb-$(CONFIG_ARCH_ZYNQMP) += zynqmp-zcu102-revB.dtb
>> +dtb-$(CONFIG_ARCH_ZYNQMP) += zynqmp-zcu102-rev1.0.dtb
>> diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-zcu102-rev1.0.dts b/arch/arm64/boot/dts/xilinx/zynqmp-zcu102-rev1.0.dts
>> new file mode 100644
>> index 000000000000..4b7477795fbd
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/xilinx/zynqmp-zcu102-rev1.0.dts
>> @@ -0,0 +1,36 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * dts file for Xilinx ZynqMP ZCU102 Rev1.0
>> + *
>> + * (C) Copyright 2016 - 2018, Xilinx, Inc.
>> + *
>> + * Michal Simek <michal.simek@xxxxxxxxxx>
>> + */
>> +
>> +#include "zynqmp-zcu102-revB.dts"
>> +
>> +/ {
>> +       model = "ZynqMP ZCU102 Rev1.0";
>> +       compatible = "xlnx,zynqmp-zcu102-rev1.0", "xlnx,zynqmp-zcu102", "xlnx,zynqmp";
> 
> Documented?

Still waiting what exactly should be documented and where this should be
added.
I have seen that broadcom was doing this properly for arm32
(bindings/arm/bcm) but for example brcm,stingray is not documented.

I am happy to document our strings but I would like to do it just once
when I know proper location and style.

> 
>> +};
>> +
>> +&eeprom {
>> +       #address-cells = <1>;
>> +       #size-cells = <1>;
>> +
>> +       board_sn: board_sn@0 {
> 
> Use '-' rather than '_' in node and property names.

I will change this in node name even based on spec and Table 2.1
underscore is also permitted. Maybe priorities should be reflected in
that table too.

>> +       i2cswitch@75 { /* u60 */
>> +               compatible = "nxp,pca9544";
>> +               #address-cells = <1>;
>> +               #size-cells = <0>;
>> +               reg = <0x75>;
>> +               i2c@0 { /* i2c mw 75 0 1 */
>> +                       #address-cells = <1>;
>> +                       #size-cells = <0>;
>> +                       reg = <0>;
>> +                       /* PS_PMBUS */
>> +                       ina226@40 { /* u76 */
> 
> Should be what the device does, not part numbers if possible. Standard
> names are defined in the DT spec (additions welcome).

I have suggested power-monitor name for this case. Pull request #15 for
that.

Thanks,
Michal