From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 19EF5C48BD1 for ; Fri, 11 Jun 2021 18:21:10 +0000 (UTC) Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 18705613AE for ; Fri, 11 Jun 2021 18:21:08 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 18705613AE Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=siemens.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id BD7C780772; Fri, 11 Jun 2021 20:21:06 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=fail (p=none dis=none) header.from=siemens.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Received: by phobos.denx.de (Postfix, from userid 109) id F12B580C96; Fri, 11 Jun 2021 20:21:04 +0200 (CEST) Received: from gecko.sbs.de (gecko.sbs.de [194.138.37.40]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 4BB318060C for ; Fri, 11 Jun 2021 20:21:01 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=siemens.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=jan.kiszka@siemens.com Received: from mail2.sbs.de (mail2.sbs.de [192.129.41.66]) by gecko.sbs.de (8.15.2/8.15.2) with ESMTPS id 15BIKx9I018778 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Fri, 11 Jun 2021 20:20:59 +0200 Received: from [167.87.248.115] ([167.87.248.115]) by mail2.sbs.de (8.15.2/8.15.2) with ESMTP id 15BIKwe3005227; Fri, 11 Jun 2021 20:20:59 +0200 Subject: Re: [PATCH v2 0/5] Add SIMATIC IOT2050 board support To: Tom Rini , Lokesh Vutla Cc: U-Boot Mailing List , Le Jin , Bao Cheng Su , Nian Gao , Chao Zeng References: <989cfad9-769d-01dc-ef36-4092da8b0683@ti.com> <20210611145341.GX9516@bill-the-cat> From: Jan Kiszka Message-ID: <2371beec-0c96-1f5f-bc66-170f658a56a1@siemens.com> Date: Fri, 11 Jun 2021 20:20:58 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.0 MIME-Version: 1.0 In-Reply-To: <20210611145341.GX9516@bill-the-cat> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean 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