Re: [PATCH v3 2/2] arm64: zynqmp: Add support for Xilinx Kria SOM board

[Michal Simek <michal.simek@xilinx.com>]

On 8/13/21 10:09 PM, Rob Herring wrote:
> On Fri, Aug 06, 2021 at 12:12:08PM +0200, Michal Simek wrote:
>> There are couple of revisions of SOMs (k26) and associated carrier cards
>> (kv260).
>> SOM itself has two major versions:
>> sm-k26 - SOM with EMMC
>> smk-k26 - SOM without EMMC used on starter kit with preprogrammed firmware
>> in QSPI.
>>
>> SOMs are describing only devices available on the SOM or connections which
>> are described in specification (for example UART, fwuen).
>>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> ---
>>
>> Changes in v3:
>> - Fix led node name
>> - Fix compatible string for xlnx,zynqmp-sk-kv260-revA/Y/Z
>> - Fix headers alignment
>> - Move USB3 PHY properties from DWC3 node to USB node - reported by Manish
>>   Narani
>> - Change dtb names generated with dtbo
>> - Fix emmc comment style
>> -
>>
>> Changes in v2:
>> - Use sugar syntax - reported by Geert
>> - Update copyright years
>> - Fix SD3.0 comment alignment
>> - Remove one newline from Makefile
>>
>> https://www.xilinx.com/products/som/kria.html
>> ---
>>  .../devicetree/bindings/arm/xilinx.yaml       |  31 ++
>>  arch/arm64/boot/dts/xilinx/Makefile           |  13 +
>>  .../boot/dts/xilinx/zynqmp-sck-kv-g-revA.dts  | 335 ++++++++++++++++++
>>  .../boot/dts/xilinx/zynqmp-sck-kv-g-revB.dts  | 318 +++++++++++++++++
>>  .../boot/dts/xilinx/zynqmp-sm-k26-revA.dts    | 289 +++++++++++++++
>>  .../boot/dts/xilinx/zynqmp-smk-k26-revA.dts   |  21 ++
>>  6 files changed, 1007 insertions(+)
>>  create mode 100644 arch/arm64/boot/dts/xilinx/zynqmp-sck-kv-g-revA.dts
>>  create mode 100644 arch/arm64/boot/dts/xilinx/zynqmp-sck-kv-g-revB.dts
>>  create mode 100644 arch/arm64/boot/dts/xilinx/zynqmp-sm-k26-revA.dts
>>  create mode 100644 arch/arm64/boot/dts/xilinx/zynqmp-smk-k26-revA.dts
>>
>> diff --git a/Documentation/devicetree/bindings/arm/xilinx.yaml b/Documentation/devicetree/bindings/arm/xilinx.yaml
>> index a0b1ae6e3e71..31b86a6363b8 100644
>> --- a/Documentation/devicetree/bindings/arm/xilinx.yaml
>> +++ b/Documentation/devicetree/bindings/arm/xilinx.yaml
>> @@ -116,6 +116,37 @@ properties:
>>            - const: xlnx,zynqmp-zcu111
>>            - const: xlnx,zynqmp
>>  
>> +      - description: Xilinx Kria SOMs
>> +        items:
>> +          - const: xlnx,zynqmp-sm-k26-rev1
>> +          - const: xlnx,zynqmp-sm-k26-revB
>> +          - const: xlnx,zynqmp-sm-k26-revA
>> +          - const: xlnx,zynqmp-sm-k26
> 
> How is having all 4 strings useful? Seems like it should be only 1 of 
> the rev's at a time.

We have added eeprom on SOM and another one on carrier card.
And final DT is combination of them.
And I wanted to keep track which versions are compatible to each other
from software point of view.
revB is fully equal to rev1 HW and it is pretty much just change from
development version to production version.
revA compare to revB has some changes on PCB but still SW compatible.

That's why I wanted to keep track of all versions via compatible strings
and also Xilinx distribution based on this information creates symlinks
to the same DTB to pick up correct DTB based on revision written in eeprom.

Also I don't think this is going against compatible string description
written in DT spec.

"The compatible property value consists of one or more strings that
define the specific programming model for
the device. This list of strings should be used by a client program for
device driver selection. The property
value consists of a concatenated list of null terminated strings, from
most specific to most general. They
allow a device to express its compatibility with a family of similar
devices, potentially allowing a single
device driver to match against several devices."


<snip>

>> diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-sck-kv-g-revB.dts b/arch/arm64/boot/dts/xilinx/zynqmp-sck-kv-g-revB.dts
>> new file mode 100644
>> index 000000000000..df054e152a77
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/xilinx/zynqmp-sck-kv-g-revB.dts
>> @@ -0,0 +1,318 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * dts file for KV260 revA Carrier Card
>> + *
>> + * (C) Copyright 2020 - 2021, Xilinx, Inc.
>> + *
>> + * Michal Simek <michal.simek@xilinx.com>
>> + */
>> +
>> +#include <dt-bindings/gpio/gpio.h>
>> +#include <dt-bindings/net/ti-dp83867.h>
>> +#include <dt-bindings/phy/phy.h>
>> +#include <dt-bindings/pinctrl/pinctrl-zynqmp.h>
>> +
>> +/dts-v1/;
>> +/plugin/;
>> +
>> +&{/} {
>> +	compatible = "xlnx,zynqmp-sk-kv260-rev1",
>> +		     "xlnx,zynqmp-sk-kv260-revB",
>> +		     "xlnx,zynqmp-sk-kv260", "xlnx,zynqmp";
> 
> I don't think changing the root compatible in an overlay is a good 
> policy. Do you need this all to be overlays?

Thanks for opening this up. DT overlay is applied in Linux now but in
near future we will have to apply it earlier in u-boot.
And the main question is if you want to see just SOM board compatible
string with k26 or carrier card compatible string (kv260).

I choose to rather see carrier card compatible string which is that
information which make difference. SOM variants are pretty much the
same. The different is only if EMMC is populated or not.
Different silicon is also recorded in eeprom but it can be detected and
sw is pretty much the same but don't need to be.

The boot works in a way that SOM is booting out of QSPI with only DT
with SOM description. Then carrier card is detected and DT overlay is
applied and never removed. And then in Linux other DT overlays are
applied and removed based on application you choose.

If you think that my thinking about carrier card compatible string is
horribly wrong I have no problem to remove them but I need to find a way
how to keep track of carrier revisions which are compatible with this
overlay.


> 
>> +};
>> +
>> +&i2c1 { /* I2C_SCK C23/C24 - MIO from SOM */
>> +	#address-cells = <1>;
>> +	#size-cells = <0>;
>> +	pinctrl-names = "default", "gpio";
>> +	pinctrl-0 = <&pinctrl_i2c1_default>;
>> +	pinctrl-1 = <&pinctrl_i2c1_gpio>;
>> +	scl-gpios = <&gpio 24 GPIO_ACTIVE_HIGH>;
>> +	sda-gpios = <&gpio 25 GPIO_ACTIVE_HIGH>;
>> +
>> +	u14: ina260@40 { /* u14 */
>> +		compatible = "ti,ina260";
> 
> Not documented. Please run 'make dtbs_check' and don't add new warnings.

I will remove it.
Did you get a warning? When I was looking at it it didn't come because
this is overlays. Was there any update which changed this?

> 
>> +		#io-channel-cells = <1>;
>> +		label = "ina260-u14";
>> +		reg = <0x40>;
>> +	};
>> +	usbhub: usb5744@2d { /* u43 */
>> +		compatible = "microchip,usb5744";
> 
> Not documented.

I will remove this one too.


Thanks,
Michal