Re: [U-Boot] [PATCH 1/2] ARM: dts: zynq: Update dts for Z-turn board

2018-03-15 Thread Michal Simek
On 14.3.2018 23:40, Anton Gerasimov wrote:
> Hi Alexander,
> 
> 
>>>   device_type = "memory";
>>>   reg = <0x0 0x4000>;
>>>   };
>>>     chosen {
>>> -    stdout-path = "serial0:115200n8";
>> Nack. By default graphical output is quite unusable on this board, so we
>> want to output to serial.
>>
>> If your Linux submitted device tree doesn't contain this part, please
>> fix it there.
>>
>>> +    bootargs = "console=ttyPS0,115200 earlyprintk
>>> root=/dev/mmcblk0p2 rootwait";
>> This is even worse. Please don't prepopulate any bootargs, otherwise
>> people may end up assuming that they're actually getting used.
> 
> The older one is much cleaner I agree, thank you. I'll just need to test
> with both u-boot and linux.
> 
>>>   };
>>>     gpio-leds {
>>>   compatible = "gpio-leds";
>>> -    led_r {
>>> -    label = "led_r";
>>> -    gpios = < 0x72 0x1>;
>>> -    default-state = "on";
>>> -    linux,default-trigger = "heartbeat";
>>> -    };
>>> -
>>> -    led_g {
>>> -    label = "led_g";
>>> -    gpios = < 0x73 0x1>;
>>> -    default-state = "on";
>>> -    linux,default-trigger = "heartbeat";
>>> -    };
>>> -
>>> -    led_b {
>>> -    label = "led_b";
>>> -    gpios = < 0x74 0x1>;
>>> -    default-state = "on";
>>> -    linux,default-trigger = "heartbeat";
>>> -    };
>> Why remove them? They're hard wired on the board, no?
> No this RGB LED is connected to PL, that's for sure.
> 
>>>   ps-clk-frequency = <>;
>>> -    fclk-enable = <0xf>;
>> Why?
> 
>> IIRC on my Z-Turn, I had to take a PL clock back into the PS as AXI
>> reference clock. See "Connect clocks" here:
>>
>>
>>
>>    https://wiki.hackerspace.pl/projects:zturn-hackers:helloworld
>>
>>
>>
>> So we need to have the PL clock enabled, no? Or is that only needed for
>> PL AXI peripherals?
>>
> 
> As far as I understand M_AXI_GPn connects AXI slaves implemented on PL
> to AXI and S_AXI_HPn provides access for AXI slaves in PS to PL slaves.
> So both involve PL and can be disconnected if PL is not clocked.

As you see that connection is done via PL. I am booting zynq boards
without fclk-enable and without PL and they are booting properly.
I have never had a time to investing what it is broken if you don't load
bitstream. Definitely feel free to test.
And as you see only one clk is used not all 4.


> 
>>>   };
>>>      {
>>> @@ -152,8 +114,8 @@
>>>   reg = <0x49>;
>>>   };
>>>   -    adxl345@53 {
>>> -    compatible = "adi,adxl34x", "adxl34x";
>>> +    accelerometer@53 {
>>> +    compatible = "adi,adxl345", "adxl345";
>> You can't just remove compatibles. Device trees are supposed to be
>> compatible with whatever used them before someone thought they want to
>> prettify them, so in this case you'd have to add the concrete names in
>> the list before the abstract ones:
>>
>>    compatible = "adi,adxl345", "adxl345", "adi,adxl34x", "adxl34x";
> Yes sorry, I didn't realize that Linux kernel had both.

Ok. I think it will be good to sync these two dts with the kernel. It
means can you please send new updated version for linux kernel too?

Thanks,
Michal

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot


Re: [U-Boot] [PATCH 1/2] ARM: dts: zynq: Update dts for Z-turn board

2018-03-14 Thread Anton Gerasimov

Hi Alexander,



device_type = "memory";
reg = <0x0 0x4000>;
};
  
  	chosen {

-   stdout-path = "serial0:115200n8";

Nack. By default graphical output is quite unusable on this board, so we
want to output to serial.

If your Linux submitted device tree doesn't contain this part, please
fix it there.


+   bootargs = "console=ttyPS0,115200 earlyprintk root=/dev/mmcblk0p2 
rootwait";

This is even worse. Please don't prepopulate any bootargs, otherwise
people may end up assuming that they're actually getting used.


The older one is much cleaner I agree, thank you. I'll just need to test 
with both u-boot and linux.



};
  
  	gpio-leds {

compatible = "gpio-leds";
-   led_r {
-   label = "led_r";
-   gpios = < 0x72 0x1>;
-   default-state = "on";
-   linux,default-trigger = "heartbeat";
-   };
-
-   led_g {
-   label = "led_g";
-   gpios = < 0x73 0x1>;
-   default-state = "on";
-   linux,default-trigger = "heartbeat";
-   };
-
-   led_b {
-   label = "led_b";
-   gpios = < 0x74 0x1>;
-   default-state = "on";
-   linux,default-trigger = "heartbeat";
-   };

Why remove them? They're hard wired on the board, no?

No this RGB LED is connected to PL, that's for sure.


ps-clk-frequency = <>;
-   fclk-enable = <0xf>;

Why?



IIRC on my Z-Turn, I had to take a PL clock back into the PS as AXI
reference clock. See "Connect clocks" here:



   https://wiki.hackerspace.pl/projects:zturn-hackers:helloworld



So we need to have the PL clock enabled, no? Or is that only needed for
PL AXI peripherals?



As far as I understand M_AXI_GPn connects AXI slaves implemented on PL 
to AXI and S_AXI_HPn provides access for AXI slaves in PS to PL slaves. 
So both involve PL and can be disconnected if PL is not clocked.



  };
  
   {

@@ -152,8 +114,8 @@
reg = <0x49>;
};
  
-	adxl345@53 {

-   compatible = "adi,adxl34x", "adxl34x";
+   accelerometer@53 {
+   compatible = "adi,adxl345", "adxl345";

You can't just remove compatibles. Device trees are supposed to be
compatible with whatever used them before someone thought they want to
prettify them, so in this case you'd have to add the concrete names in
the list before the abstract ones:

   compatible = "adi,adxl345", "adxl345", "adi,adxl34x", "adxl34x";

Yes sorry, I didn't realize that Linux kernel had both.

Best,
Anton
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot


Re: [U-Boot] [PATCH 1/2] ARM: dts: zynq: Update dts for Z-turn board

2018-03-14 Thread Alexander Graf

On 03/14/2018 02:05 PM, Michal Simek wrote:

On 13.3.2018 22:21, Alexander Graf wrote:


On 13.03.18 21:35, tos...@gmail.com wrote:

From: Anton Gerasimov 

Delete devices implemented in PL, stylistic changes.

Signed-off-by: Anton Gerasimov 
---
  arch/arm/dts/zynq-zturn-myir.dts | 64 
  1 file changed, 13 insertions(+), 51 deletions(-)

diff --git a/arch/arm/dts/zynq-zturn-myir.dts b/arch/arm/dts/zynq-zturn-myir.dts
index a5ecfcc1d7..b6661d0205 100644
--- a/arch/arm/dts/zynq-zturn-myir.dts
+++ b/arch/arm/dts/zynq-zturn-myir.dts
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0
  /*
   *  Copyright (C) 2015 Andrea Merello 
   *  Copyright (C) 2017 Alexander Graf 
@@ -6,87 +7,49 @@
   *  Copyright (C) 2011 - 2014 Xilinx
   *  Copyright (C) 2012 National Instruments Corp.
   *
- * This software is licensed under the terms of the GNU General Public
- * License version 2, as published by the Free Software Foundation, and
- * may be copied, distributed, and modified under those terms.
- *
- * 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.
   */
+
  /dts-v1/;
  /include/ "zynq-7000.dtsi"
  
  / {

model = "Zynq Z-Turn MYIR Board";
-   compatible = "xlnx,zynq-7000";
+   compatible = "myir,zynq-zturn", "xlnx,zynq-7000";

ack.

  
  	aliases {

ethernet0 = 
serial0 = 
serial1 = 
-   spi0 = 
-   mmc0 = 
};
  
-	memory {

+   memory@0 {

Why?

node with regs should have @number. DTC is checking that W=1.


device_type = "memory";
reg = <0x0 0x4000>;
};
  
  	chosen {

-   stdout-path = "serial0:115200n8";

Nack. By default graphical output is quite unusable on this board, so we
want to output to serial.

If your Linux submitted device tree doesn't contain this part, please
fix it there.


+   bootargs = "console=ttyPS0,115200 earlyprintk root=/dev/mmcblk0p2 
rootwait";

This is even worse. Please don't prepopulate any bootargs, otherwise
people may end up assuming that they're actually getting used.


};
  
  	gpio-leds {

compatible = "gpio-leds";
-   led_r {
-   label = "led_r";
-   gpios = < 0x72 0x1>;
-   default-state = "on";
-   linux,default-trigger = "heartbeat";
-   };
-
-   led_g {
-   label = "led_g";
-   gpios = < 0x73 0x1>;
-   default-state = "on";
-   linux,default-trigger = "heartbeat";
-   };
-
-   led_b {
-   label = "led_b";
-   gpios = < 0x74 0x1>;
-   default-state = "on";
-   linux,default-trigger = "heartbeat";
-   };

Why remove them? They're hard wired on the board, no?

zynq has 54 MIO and all numbers above are routed via PL. It means this
depends on PL.



-
-   usr_led1 {
-   label = "usr_led1";
+   usr-led1 {
+   label = "usr-led1";
gpios = < 0x0 0x1>;
default-state = "off";
-   linux,default-trigger = "none";
};
  
-		usr_led2 {

-   label = "usr_led2";
+   usr-led2 {
+   label = "usr-led2";
gpios = < 0x9 0x1>;
default-state = "off";
-   linux,default-trigger = "none";
};
};
  
-	gpio-beep {

-   compatible = "gpio-beeper";
-   label = "pl-beep";
-   gpios = < 0x75 0x0>;
-   };

This one is in PL, so ack.


-
gpio-keys {
compatible = "gpio-keys";
-   #address-cells = <0x1>;
-   #size-cells = <0x0>;
+   #address-cells = <1>;
+   #size-cells = <0>;

ack


autorepeat;
K1 {
label = "K1";
@@ -100,7 +63,6 @@
  
   {

ps-clk-frequency = <>;
-   fclk-enable = <0xf>;

Why?

fclk-enable is enabling clocks for PL. It exactly means enable all 4
clocks to PL which is PL dependent that's why it shouldn't be here.


IIRC on my Z-Turn, I had to take a PL clock back into the PS as AXI 
reference clock. See "Connect clocks" here:


  https://wiki.hackerspace.pl/projects:zturn-hackers:helloworld

So we need to have the PL clock enabled, no? Or is that only needed for 
PL AXI peripherals?



Alex


Re: [U-Boot] [PATCH 1/2] ARM: dts: zynq: Update dts for Z-turn board

2018-03-14 Thread Michal Simek
On 13.3.2018 22:21, Alexander Graf wrote:
> 
> 
> On 13.03.18 21:35, tos...@gmail.com wrote:
>> From: Anton Gerasimov 
>>
>> Delete devices implemented in PL, stylistic changes.
>>
>> Signed-off-by: Anton Gerasimov 
>> ---
>>  arch/arm/dts/zynq-zturn-myir.dts | 64 
>> 
>>  1 file changed, 13 insertions(+), 51 deletions(-)
>>
>> diff --git a/arch/arm/dts/zynq-zturn-myir.dts 
>> b/arch/arm/dts/zynq-zturn-myir.dts
>> index a5ecfcc1d7..b6661d0205 100644
>> --- a/arch/arm/dts/zynq-zturn-myir.dts
>> +++ b/arch/arm/dts/zynq-zturn-myir.dts
>> @@ -1,3 +1,4 @@
>> +// SPDX-License-Identifier: GPL-2.0
>>  /*
>>   *  Copyright (C) 2015 Andrea Merello 
>>   *  Copyright (C) 2017 Alexander Graf 
>> @@ -6,87 +7,49 @@
>>   *  Copyright (C) 2011 - 2014 Xilinx
>>   *  Copyright (C) 2012 National Instruments Corp.
>>   *
>> - * This software is licensed under the terms of the GNU General Public
>> - * License version 2, as published by the Free Software Foundation, and
>> - * may be copied, distributed, and modified under those terms.
>> - *
>> - * 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.
>>   */
>> +
>>  /dts-v1/;
>>  /include/ "zynq-7000.dtsi"
>>  
>>  / {
>>  model = "Zynq Z-Turn MYIR Board";
>> -compatible = "xlnx,zynq-7000";
>> +compatible = "myir,zynq-zturn", "xlnx,zynq-7000";
> 
> ack.
> 
>>  
>>  aliases {
>>  ethernet0 = 
>>  serial0 = 
>>  serial1 = 
>> -spi0 = 
>> -mmc0 = 
>>  };
>>  
>> -memory {
>> +memory@0 {
> 
> Why?

node with regs should have @number. DTC is checking that W=1.

> 
>>  device_type = "memory";
>>  reg = <0x0 0x4000>;
>>  };
>>  
>>  chosen {
>> -stdout-path = "serial0:115200n8";
> 
> Nack. By default graphical output is quite unusable on this board, so we
> want to output to serial.
> 
> If your Linux submitted device tree doesn't contain this part, please
> fix it there.
> 
>> +bootargs = "console=ttyPS0,115200 earlyprintk 
>> root=/dev/mmcblk0p2 rootwait";
> 
> This is even worse. Please don't prepopulate any bootargs, otherwise
> people may end up assuming that they're actually getting used.
> 
>>  };
>>  
>>  gpio-leds {
>>  compatible = "gpio-leds";
>> -led_r {
>> -label = "led_r";
>> -gpios = < 0x72 0x1>;
>> -default-state = "on";
>> -linux,default-trigger = "heartbeat";
>> -};
>> -
>> -led_g {
>> -label = "led_g";
>> -gpios = < 0x73 0x1>;
>> -default-state = "on";
>> -linux,default-trigger = "heartbeat";
>> -};
>> -
>> -led_b {
>> -label = "led_b";
>> -gpios = < 0x74 0x1>;
>> -default-state = "on";
>> -linux,default-trigger = "heartbeat";
>> -};
> Why remove them? They're hard wired on the board, no?

zynq has 54 MIO and all numbers above are routed via PL. It means this
depends on PL.


> 
>> -
>> -usr_led1 {
>> -label = "usr_led1";
>> +usr-led1 {
>> +label = "usr-led1";
>>  gpios = < 0x0 0x1>;
>>  default-state = "off";
>> -linux,default-trigger = "none";
>>  };
>>  
>> -usr_led2 {
>> -label = "usr_led2";
>> +usr-led2 {
>> +label = "usr-led2";
>>  gpios = < 0x9 0x1>;
>>  default-state = "off";
>> -linux,default-trigger = "none";
>>  };
>>  };
>>  
>> -gpio-beep {
>> -compatible = "gpio-beeper";
>> -label = "pl-beep";
>> -gpios = < 0x75 0x0>;
>> -};
> 
> This one is in PL, so ack.
> 
>> -
>>  gpio-keys {
>>  compatible = "gpio-keys";
>> -#address-cells = <0x1>;
>> -#size-cells = <0x0>;
>> +#address-cells = <1>;
>> +#size-cells = <0>;
> 
> ack
> 
>>  autorepeat;
>>  K1 {
>>  label = "K1";
>> @@ -100,7 +63,6 @@
>>  
>>   {
>>  ps-clk-frequency = <>;
>> -fclk-enable = <0xf>;
> 
> Why?

fclk-enable is enabling clocks for PL. It exactly means enable all 4
clocks to PL which is PL dependent that's why it shouldn't be here.

Thanks,
MIchal
___
U-Boot mailing list
U-Boot@lists.denx.de

Re: [U-Boot] [PATCH 1/2] ARM: dts: zynq: Update dts for Z-turn board

2018-03-13 Thread Alexander Graf


On 13.03.18 21:35, tos...@gmail.com wrote:
> From: Anton Gerasimov 
> 
> Delete devices implemented in PL, stylistic changes.
> 
> Signed-off-by: Anton Gerasimov 
> ---
>  arch/arm/dts/zynq-zturn-myir.dts | 64 
> 
>  1 file changed, 13 insertions(+), 51 deletions(-)
> 
> diff --git a/arch/arm/dts/zynq-zturn-myir.dts 
> b/arch/arm/dts/zynq-zturn-myir.dts
> index a5ecfcc1d7..b6661d0205 100644
> --- a/arch/arm/dts/zynq-zturn-myir.dts
> +++ b/arch/arm/dts/zynq-zturn-myir.dts
> @@ -1,3 +1,4 @@
> +// SPDX-License-Identifier: GPL-2.0
>  /*
>   *  Copyright (C) 2015 Andrea Merello 
>   *  Copyright (C) 2017 Alexander Graf 
> @@ -6,87 +7,49 @@
>   *  Copyright (C) 2011 - 2014 Xilinx
>   *  Copyright (C) 2012 National Instruments Corp.
>   *
> - * This software is licensed under the terms of the GNU General Public
> - * License version 2, as published by the Free Software Foundation, and
> - * may be copied, distributed, and modified under those terms.
> - *
> - * 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.
>   */
> +
>  /dts-v1/;
>  /include/ "zynq-7000.dtsi"
>  
>  / {
>   model = "Zynq Z-Turn MYIR Board";
> - compatible = "xlnx,zynq-7000";
> + compatible = "myir,zynq-zturn", "xlnx,zynq-7000";

ack.

>  
>   aliases {
>   ethernet0 = 
>   serial0 = 
>   serial1 = 
> - spi0 = 
> - mmc0 = 
>   };
>  
> - memory {
> + memory@0 {

Why?

>   device_type = "memory";
>   reg = <0x0 0x4000>;
>   };
>  
>   chosen {
> - stdout-path = "serial0:115200n8";

Nack. By default graphical output is quite unusable on this board, so we
want to output to serial.

If your Linux submitted device tree doesn't contain this part, please
fix it there.

> + bootargs = "console=ttyPS0,115200 earlyprintk 
> root=/dev/mmcblk0p2 rootwait";

This is even worse. Please don't prepopulate any bootargs, otherwise
people may end up assuming that they're actually getting used.

>   };
>  
>   gpio-leds {
>   compatible = "gpio-leds";
> - led_r {
> - label = "led_r";
> - gpios = < 0x72 0x1>;
> - default-state = "on";
> - linux,default-trigger = "heartbeat";
> - };
> -
> - led_g {
> - label = "led_g";
> - gpios = < 0x73 0x1>;
> - default-state = "on";
> - linux,default-trigger = "heartbeat";
> - };
> -
> - led_b {
> - label = "led_b";
> - gpios = < 0x74 0x1>;
> - default-state = "on";
> - linux,default-trigger = "heartbeat";
> - };
Why remove them? They're hard wired on the board, no?

> -
> - usr_led1 {
> - label = "usr_led1";
> + usr-led1 {
> + label = "usr-led1";
>   gpios = < 0x0 0x1>;
>   default-state = "off";
> - linux,default-trigger = "none";
>   };
>  
> - usr_led2 {
> - label = "usr_led2";
> + usr-led2 {
> + label = "usr-led2";
>   gpios = < 0x9 0x1>;
>   default-state = "off";
> - linux,default-trigger = "none";
>   };
>   };
>  
> - gpio-beep {
> - compatible = "gpio-beeper";
> - label = "pl-beep";
> - gpios = < 0x75 0x0>;
> - };

This one is in PL, so ack.

> -
>   gpio-keys {
>   compatible = "gpio-keys";
> - #address-cells = <0x1>;
> - #size-cells = <0x0>;
> + #address-cells = <1>;
> + #size-cells = <0>;

ack

>   autorepeat;
>   K1 {
>   label = "K1";
> @@ -100,7 +63,6 @@
>  
>   {
>   ps-clk-frequency = <>;
> - fclk-enable = <0xf>;

Why?

>  };
>  
>   {
> @@ -152,8 +114,8 @@
>   reg = <0x49>;
>   };
>  
> - adxl345@53 {
> - compatible = "adi,adxl34x", "adxl34x";
> + accelerometer@53 {
> + compatible = "adi,adxl345", "adxl345";

You can't just remove compatibles. Device trees are supposed to be
compatible with whatever used them before someone thought they want to
prettify them, so in this case you'd have to add the concrete names in
the list before the abstract ones:

  compatible = "adi,adxl345", "adxl345", "adi,adxl34x", "adxl34x";


Alex

>