[PATCH v2 0/5] Add SIMATIC IOT2050 board support

Fri Jun 11 20:20:58 CEST 2021

On 11.06.21 16:53, Tom Rini wrote:
> On Fri, Jun 11, 2021 at 08:00:17PM +0530, Lokesh Vutla wrote:
>>
>>
>> On 02/06/21 3:07 pm, Jan Kiszka wrote:
>>> This is the baseline support for the SIMATIC IOT2050 devices.
>>>
>>> Changes in v2:
>>>  - rebased
>>>  - sync with upstream-accepted DT
>>>  - add boot switch
>>>  - include watchdog support
>>>
>>> Allows to boot mainline 5.10 kernels, but not the original BSP-derived
>>> kernel we currently ship as reference. This is due to the TI sysfw ABI
>>> breakages between 2.x and 3.x. We will soon provide a transitional
>>> kernel that allows booting both firmware ABIs - as long as full upstream
>>> kernel support is work in progress.
>>>
>>> Note that this baseline support lacks Ethernet drivers. We are working
>>> closely with TI to ensure that the to-be-upstreamed icssg-prueth driver
>>> will work both with new SR2.0 AM65x silicon as well as with SR1.0 which
>>> is used in the currently shipped IOT2050 devices.
>>>
>>> A staging tree for complete IOT2050 support can be found at [1]. Full
>>> image integration is available via [2].
>>>
>>> Regarding patch 4: There has been some doubts on the proposed approach,
>>> but there has been also no suggestion provided for a similarly
>>> lightweight and secure embedding method. Therefore, I'm proposing our
>>> solution once again.
>>
>> There are multiple checkpatch issues with this series. Can you fix them where
>> ever possible?
>>
>> ➜  u-boot-ti git:(for-next) ./scripts/checkpatch.pl --strict siemens/*.patch
>> --------------------------------------------------------
>> siemens/0001-arm-dts-Add-IOT2050-device-tree-files.patch
>> --------------------------------------------------------
>> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
>> #50:
>> new file mode 100644
>>
>> WARNING: line length of 102 exceeds 100 columns
>> #103: FILE: arch/arm/dts/k3-am65-iot2050-boot-image.dtsi:49:
>> +						filename = "arch/arm/dts/k3-am6528-iot2050-basic.dtb";
>>
>> WARNING: line length of 105 exceeds 100 columns
>> #113: FILE: arch/arm/dts/k3-am65-iot2050-boot-image.dtsi:59:
>> +						filename = "arch/arm/dts/k3-am6548-iot2050-advanced.dtb";
>>
>> total: 0 errors, 3 warnings, 0 checks, 1025 lines checked
>>
>> NOTE: For some of the reported defects, checkpatch may be able to
>>       mechanically convert to the typical style using --fix or --fix-inplace.
>>
>> siemens/0001-arm-dts-Add-IOT2050-device-tree-files.patch has style problems,
>> please review.
>> -----------------------------------------------------------------------
>> siemens/0002-board-siemens-Add-support-for-SIMATIC-IOT2050-device.patch
>> -----------------------------------------------------------------------
>> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
>> #53:
>> new file mode 100644
>>
>> WARNING: Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible
>> #282: FILE: board/siemens/iot2050/board.c:86:
>> +#ifdef CONFIG_NET
>>
>> WARNING: Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible
>> #338: FILE: board/siemens/iot2050/board.c:142:
>> +#ifdef CONFIG_SPL_LOAD_FIT
>>
>> WARNING: Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible
>> #361: FILE: board/siemens/iot2050/board.c:165:
>> +#ifdef CONFIG_IOT2050_BOOT_SWITCH
>>
>> WARNING: Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible
>> #404: FILE: board/siemens/iot2050/board.c:208:
>> +#ifdef CONFIG_IOT2050_BOOT_SWITCH
>>
>> WARNING: Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible
>> #413: FILE: board/siemens/iot2050/board.c:217:
>> +#if defined(CONFIG_OF_LIBFDT) && defined(CONFIG_OF_BOARD_SETUP)
>>
>> WARNING: Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible
>> #458: FILE: board/siemens/iot2050/board.c:262:
>> +#if CONFIG_IS_ENABLED(LED)
>>
>> CHECK: Macro argument reuse 'func' - possible side-effects?
>> #683: FILE: include/configs/iot2050.h:43:
>> +#define BOOT_TARGET_DEVICES(func) \
>> +	func(MMC, mmc, 1) \
>> +	func(MMC, mmc, 0) \
>> +	func(USB, usb, 0) \
>> +	func(USB, usb, 1) \
>> +	func(USB, usb, 2)
>>
>> total: 0 errors, 7 warnings, 1 checks, 606 lines checked
>>
>> NOTE: For some of the reported defects, checkpatch may be able to
>>       mechanically convert to the typical style using --fix or --fix-inplace.
>>
>> siemens/0002-board-siemens-Add-support-for-SIMATIC-IOT2050-device.patch has
>> style problems, please review.
>> ------------------------------------------------------------------
>> siemens/0003-arm64-dts-ti-k3-am65-mcu-Add-RTI-watchdog-entry.patch
>> ------------------------------------------------------------------
>> total: 0 errors, 0 warnings, 0 checks, 13 lines checked
>>
>> siemens/0003-arm64-dts-ti-k3-am65-mcu-Add-RTI-watchdog-entry.patch has no
>> obvious style problems and is ready for submission.
>> --------------------------------------------------------------------
>> siemens/0004-watchdog-rti_wdt-Add-support-for-loading-firmware.patch
>> --------------------------------------------------------------------
>> WARNING: externs should be avoided in .c files
>> #95: FILE: drivers/watchdog/rti_wdt.c:47:
>> +extern const u32 rti_wdt_fw[];
>>
>> WARNING: externs should be avoided in .c files
>> #96: FILE: drivers/watchdog/rti_wdt.c:48:
>> +extern const int rti_wdt_fw_size;
>>
>> WARNING: Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible
>> #100: FILE: drivers/watchdog/rti_wdt.c:52:
>> +#ifdef CONFIG_WDT_K3_RTI_LOAD_FW
>>
>> WARNING: Use 'if (IS_ENABLED(CONFIG...))' instead of '#if or #ifdef' where possible
>> #113: FILE: drivers/watchdog/rti_wdt.c:64:
>> +#ifdef CONFIG_WDT_K3_RTI_LOAD_FW
>>
>> WARNING: labels should not be indented
>> #116: FILE: drivers/watchdog/rti_wdt.c:67:
>> +	    dt_error:
>>
>> WARNING: labels should not be indented
>> #137: FILE: drivers/watchdog/rti_wdt.c:88:
>> +	    fw_error:
>>
>> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
>> #163:
>> new file mode 100644
>>
>> WARNING: Improper SPDX comment style for 'drivers/watchdog/rti_wdt_fw.S', please
>> use '/*' instead
>> #168: FILE: drivers/watchdog/rti_wdt_fw.S:1:
>> +// SPDX-License-Identifier: GPL-2.0+
>>
>> WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
>> #168: FILE: drivers/watchdog/rti_wdt_fw.S:1:
>> +// SPDX-License-Identifier: GPL-2.0+
>>
>> total: 0 errors, 9 warnings, 0 checks, 139 lines checked
>>
>> NOTE: For some of the reported defects, checkpatch may be able to
>>       mechanically convert to the typical style using --fix or --fix-inplace.
>>
>> siemens/0004-watchdog-rti_wdt-Add-support-for-loading-firmware.patch has style
>> problems, please review.
>> -----------------------------------------------------------------------
>> siemens/0005-configs-iot2050-Enable-watchdog-support-but-do-not-a.patch
>> -----------------------------------------------------------------------
> 
> Since I just pointed out some checkpatch problems to Lokesh in his last
> PR, I should note that out of all of this list, I only really care about
> the SPDX one.  There are plenty of cases where:
> #ifdef CONFIG_FOO
> ...
> #endif
> 
> is more readable / clear than:
> 	if (IS_ENABLED(CONFIG_FOO)) {
> 		...
> 	}
> 
> 
> Warnings are warning and can be ignored for good reason, errors cannot.
> 

I've done some fixes (including the SPDX one), still need to retest. v3
will follow.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux