Merge changes from topic "bk/warnings" into integration

* changes:
  docs: describe the new warning levels
  build: add -Wunused-const-variable=2 to W=2
  build: include -Wextra in generic builds
  docs(porting-guide): update a reference
  fix(st-usb): replace redundant checks with asserts
  fix(brcm): add braces around bodies of conditionals
  fix(renesas): align incompatible function pointers
  fix(zynqmp): remove redundant api_version check
  fix: remove old-style declarations
  fix: unify fallthrough annotations

Author: manish-pandey-arm

Makefile

@@ -350,27 +350,53 @@ ASFLAGS_aarch64		=	$(march64-directive)
 # General warnings
 WARNINGS		:=	-Wall -Wmissing-include-dirs -Wunused	\
 				-Wdisabled-optimization -Wvla -Wshadow	\
-				-Wno-unused-parameter -Wredundant-decls
+				-Wredundant-decls
+# stricter warnings
+WARNINGS		+=	-Wextra -Wno-trigraphs
+# too verbose for generic build
+WARNINGS		+=	-Wno-missing-field-initializers \
+				-Wno-type-limits -Wno-sign-compare \
+# on clang this flag gets reset if -Wextra is set after it. No difference on gcc
+WARNINGS		+=	-Wno-unused-parameter
 
 # Additional warnings
-# Level 1
-WARNING1 := -Wextra
-WARNING1 += -Wmissing-format-attribute
-WARNING1 += -Wmissing-prototypes
-WARNING1 += -Wold-style-definition
-
-# Level 2
-WARNING2 := -Waggregate-return
-WARNING2 += -Wcast-align
-WARNING2 += -Wnested-externs
-
+# Level 1 - infrequent warnings we should have none of
+# full -Wextra
+WARNING1 += -Wsign-compare
+WARNING1 += -Wtype-limits
+WARNING1 += -Wmissing-field-initializers
+
+# Level 2 - problematic warnings that we want
+# zlib, compiler-rt, coreboot, and mbdedtls blow up with these
+# TODO: disable just for them and move into default build
+WARNING2 += -Wold-style-definition
+WARNING2 += -Wmissing-prototypes
+WARNING2 += -Wmissing-format-attribute
+# TF-A aims to comply with this eventually. Effort too large at present
+WARNING2 += -Wundef
+# currently very involved and many platforms set this off
+WARNING2 += -Wunused-const-variable=2
+
+# Level 3 - very pedantic, frequently ignored
 WARNING3 := -Wbad-function-cast
+WARNING3 += -Waggregate-return
+WARNING3 += -Wnested-externs
+WARNING3 += -Wcast-align
 WARNING3 += -Wcast-qual
 WARNING3 += -Wconversion
 WARNING3 += -Wpacked
 WARNING3 += -Wpointer-arith
 WARNING3 += -Wswitch-default
 
+# Setting W is quite verbose and most warnings will be pre-existing issues
+# outside of the contributor's control. Don't fail the build on them so warnings
+# can be seen and hopefully addressed
+ifdef W
+ifneq (${W},0)
+E	 ?= 0
+endif
+endif
+
 ifeq (${W},1)
 WARNINGS += $(WARNING1)
 else ifeq (${W},2)

docs/getting_started/build-options.rst

@@ -213,6 +213,12 @@ Common build options
 
 -  ``E``: Boolean option to make warnings into errors. Default is 1.
 
+   When specifying higher warnings levels (``W=1`` and higher), this option
+   defaults to 0. This is done to encourage contributors to use them, as they
+   are expected to produce warnings that would otherwise fail the build. New
+   contributions are still expected to build with ``W=0`` and ``E=1`` (the
+   default).
+
 -  ``EL3_PAYLOAD_BASE``: This option enables booting an EL3 payload instead of
    the normal boot flow. It must specify the entry point address of the EL3
    payload. Please refer to the "Booting an EL3 payload" section for more
@@ -954,6 +960,43 @@ Common build options
    regrouped and put in the root Makefile. This flag can take the values 0 to 3,
    each level enabling more warning options. Default is 0.
 
+   This option is closely related to the ``E`` option, which enables
+   ``-Werror``.
+
+   - ``W=0`` (default)
+
+     Enables a wide assortment of warnings, most notably ``-Wall`` and
+     ``-Wextra``, as well as various bad practices and things that are likely to
+     result in errors. Includes some compiler specific flags. No warnings are
+     expected at this level for any build.
+
+   - ``W=1``
+
+     Enables warnings we want the generic build to include but are too time
+     consuming to fix at the moment. It re-enables warnings taken out for
+     ``W=0`` builds (a few of the ``-Wextra`` additions). This level is expected
+     to eventually be merged into ``W=0``. Some warnings are expected on some
+     builds, but new contributions should not introduce new ones.
+
+   - ``W=2`` (recommended)
+
+    Enables warnings we want the generic build to include but cannot be enabled
+    due to external libraries. This level is expected to eventually be merged
+    into ``W=0``. Lots of warnings are expected, primarily from external
+    libraries like zlib and compiler-rt, but new controbutions should not
+    introduce new ones.
+
+   - ``W=3``
+
+     Enables warnings that are informative but not necessary and generally too
+     verbose and frequently ignored. A very large number of warnings are
+     expected.
+
+   The exact set of warning flags depends on the compiler and TF-A warning
+   level, however they are all succinctly set in the top-level Makefile. Please
+   refer to the `GCC`_ or `Clang`_ documentation for more information on the
+   individual flags.
+
 -  ``WARMBOOT_ENABLE_DCACHE_EARLY`` : Boolean option to enable D-cache early on
    the CPU after warm boot. This is applicable for platforms which do not
    require interconnect programming to enable cache coherency (eg: single
@@ -1161,3 +1204,5 @@ Firmware update options
 .. _DEN0115: https://developer.arm.com/docs/den0115/latest
 .. _PSA FW update specification: https://developer.arm.com/documentation/den0118/a/
 .. _PSA DRTM specification: https://developer.arm.com/documentation/den0113/a
+.. _GCC: https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
+.. _Clang: https://clang.llvm.org/docs/DiagnosticsReference.html

docs/getting_started/porting-guide.rst

@@ -2135,7 +2135,7 @@ CPUs. BL31 executes at EL3 and is responsible for:
 
 #. Providing runtime firmware services. Currently, BL31 only implements a
    subset of the Power State Coordination Interface (PSCI) API as a runtime
-   service. See Section 3.3 below for details of porting the PSCI
+   service. See :ref:`psci_in_bl31` below for details of porting the PSCI
    implementation.
 
 #. Optionally passing control to the BL32 image, pre-loaded at a platform-
@@ -2544,6 +2544,8 @@ Function: bool plat_get_entropy(uint64_t \*out) [mandatory]
 This function writes entropy into storage provided by the caller. If no entropy
 is available, it must return false and the storage must not be written.
 
+.. _psci_in_bl31:
+
 Power State Coordination Interface (in BL31)
 --------------------------------------------
 

docs/process/security-hardening.rst

@@ -131,38 +131,9 @@ Several build options can be used to check for security issues. Refer to the
   overflows.
 
 - The ``W`` build flag can be used to enable a number of compiler warning
-  options to detect potentially incorrect code.
-
-  - W=0 (default value)
-
-    The ``Wunused`` with ``Wno-unused-parameter``, ``Wdisabled-optimization``
-    and ``Wvla`` flags are enabled.
-
-    The ``Wunused-but-set-variable``, ``Wmaybe-uninitialized`` and
-    ``Wpacked-bitfield-compat`` are GCC specific flags that are also enabled.
-
-  - W=1
-
-    Adds ``Wextra``, ``Wmissing-format-attribute``, ``Wmissing-prototypes``,
-    ``Wold-style-definition`` and ``Wunused-const-variable``.
-
-  - W=2
-
-    Adds ``Waggregate-return``, ``Wcast-align``, ``Wnested-externs``,
-    ``Wshadow``, ``Wlogical-op``.
-
-  - W=3
-
-    Adds ``Wbad-function-cast``, ``Wcast-qual``, ``Wconversion``, ``Wpacked``,
-    ``Wpointer-arith``, ``Wredundant-decls`` and
-    ``Wswitch-default``.
-
-  Refer to the GCC or Clang documentation for more information on the individual
-  options: https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html and
-  https://clang.llvm.org/docs/DiagnosticsReference.html.
-
-  NB: The ``Werror`` flag is enabled by default in TF-A and can be disabled by
-  setting the ``E`` build flag to 0.
+  options to detect potentially incorrect code. TF-A is tested with ``W=0`` but
+  it is recommended to develop against ``W=2`` (which will eventually become the
+  default).
 
 .. rubric:: References
 

drivers/brcm/emmc/emmc_csl_sdcard.c

@@ -479,10 +479,11 @@ int init_mmc_card(struct sd_handle *handle)
 			handle->device->cfg.blockSize = 512;
 		}
 
-		if (handle->device->ctrl.ocr & SD_CARD_HIGH_CAPACITY)
+		if (handle->device->ctrl.ocr & SD_CARD_HIGH_CAPACITY) {
 			EMMC_TRACE("Sector addressing\n");
-		else
+		} else {
 			EMMC_TRACE("Byte addressing\n");
+		}
 
 		EMMC_TRACE("Ext_CSD_storage[162]: 0x%02X  Ext_CSD_storage[179]: 0x%02X\n",
 			   emmc_global_buf_ptr->u.Ext_CSD_storage[162],

drivers/brcm/emmc/emmc_pboot_hal_memory_drv.c

@@ -278,8 +278,9 @@ struct sd_handle *sdio_init(void)
 
 	SDIO_base = EMMC_CTRL_REGS_BASE_ADDR;
 
-	if (SDIO_base == SDIO0_EMMCSDXC_SYSADDR)
+	if (SDIO_base == SDIO0_EMMCSDXC_SYSADDR) {
 		EMMC_TRACE(" ---> for SDIO 0 Controller\n\n");
+	}
 
 	memset(p_sdhandle, 0, sizeof(struct sd_handle));
 
@@ -290,8 +291,9 @@ struct sd_handle *sdio_init(void)
 	memset(p_sdhandle->card, 0, sizeof(struct sd_card_info));
 
 	if (chal_sd_start((CHAL_HANDLE *) p_sdhandle->device,
-			  SD_PIO_MODE, SDIO_base, SDIO_base) != SD_OK)
+			  SD_PIO_MODE, SDIO_base, SDIO_base) != SD_OK) {
 		return NULL;
+	}
 
 	set_config(p_sdhandle, SD_NORMAL_SPEED, MAX_CMD_RETRY, SD_DMA_OFF,
 		   SD_DMA_BOUNDARY_4K, EMMC_BLOCK_SIZE, EMMC_WFE_RETRY);
@@ -330,14 +332,16 @@ uint32_t sdio_read(struct sd_handle *p_sdhandle,
 	VERBOSE("EMMC READ: dst=0x%lx, src=0x%lx, size=0x%lx\n",
 			storage_addr, mem_addr, bytes_to_read);
 
-	if (storage_size < bytes_to_read)
+	if (storage_size < bytes_to_read) {
 		/* Don't have sufficient storage to complete the operation */
 		return 0;
+	}
 
 	/* Range check non high capacity memory */
 	if ((p_sdhandle->device->ctrl.ocr & SD_CARD_HIGH_CAPACITY) == 0) {
-		if (mem_addr > 0x80000000)
+		if (mem_addr > 0x80000000) {
 			return 0;
+		}
 	}
 
 	/* High capacity card use block address mode */
@@ -384,21 +388,23 @@ uint32_t sdio_read(struct sd_handle *p_sdhandle,
 			/* Update Physical address */
 			outputBuf += manual_copy_size;
 
-			if (p_sdhandle->device->ctrl.ocr & SD_CARD_HIGH_CAPACITY)
+			if (p_sdhandle->device->ctrl.ocr & SD_CARD_HIGH_CAPACITY) {
 				blockAddr++;
-			else
+			} else {
 				blockAddr += blockSize;
+			}
 		} else {
 			return 0;
 		}
 	}
 
 	while (remSize >= blockSize) {
 
-		if (remSize >= SD_MAX_BLK_TRANSFER_LENGTH)
+		if (remSize >= SD_MAX_BLK_TRANSFER_LENGTH) {
 			readLen = SD_MAX_BLK_TRANSFER_LENGTH;
-		else
+		} else {
 			readLen = (remSize / blockSize) * blockSize;
+		}
 
 		/* Check for overflow */
 		if ((rdCount + readLen) > storage_size ||
@@ -409,10 +415,11 @@ uint32_t sdio_read(struct sd_handle *p_sdhandle,
 		}
 
 		if (!read_block(p_sdhandle, outputBuf, blockAddr, readLen)) {
-			if (p_sdhandle->device->ctrl.ocr & SD_CARD_HIGH_CAPACITY)
+			if (p_sdhandle->device->ctrl.ocr & SD_CARD_HIGH_CAPACITY) {
 				blockAddr += (readLen / blockSize);
-			else
+			} else {
 				blockAddr += readLen;
+			}
 
 			remSize -= readLen;
 			rdCount += readLen;
@@ -463,8 +470,9 @@ static uint32_t sdio_write(struct sd_handle *p_sdhandle, uintptr_t mem_addr,
 
 	/* range check non high capacity memory */
 	if ((p_sdhandle->device->ctrl.ocr & SD_CARD_HIGH_CAPACITY) == 0) {
-		if (mem_addr > 0x80000000)
+		if (mem_addr > 0x80000000) {
 			return 0;
+		}
 	}
 
 	/* the high capacity card use block address mode */
@@ -491,11 +499,12 @@ static uint32_t sdio_write(struct sd_handle *p_sdhandle, uintptr_t mem_addr,
 				blockAddr, p_sdhandle->device->cfg.blockSize)) {
 
 			if (remSize <
-			    (p_sdhandle->device->cfg.blockSize - offset))
+			    (p_sdhandle->device->cfg.blockSize - offset)) {
 				manual_copy_size = remSize;
-			else
+			} else {
 				manual_copy_size =
 				    p_sdhandle->device->cfg.blockSize - offset;
+			}
 
 			memcpy((void *)((uintptr_t)
 				(emmc_global_buf_ptr->u.tempbuf + offset)),
@@ -530,11 +539,12 @@ static uint32_t sdio_write(struct sd_handle *p_sdhandle, uintptr_t mem_addr,
 				inputBuf += manual_copy_size;
 
 				if (p_sdhandle->device->ctrl.ocr &
-				    SD_CARD_HIGH_CAPACITY)
+				    SD_CARD_HIGH_CAPACITY) {
 					blockAddr++;
-				else
+				} else {
 					blockAddr +=
 					    p_sdhandle->device->cfg.blockSize;
+				}
 			} else
 				return 0;
 		} else {

drivers/imx/usdhc/imx_usdhc.c

@@ -136,15 +136,17 @@ static int imx_usdhc_send_cmd(struct mmc_cmd *cmd)
 		break;
 	case MMC_CMD(18):
 		multiple = 1;
-		/* fall thru for read op */
+		/* for read op */
+		/* fallthrough */
 	case MMC_CMD(17):
 	case MMC_CMD(8):
 		mixctl |= MIXCTRL_DTDSEL;
 		data = 1;
 		break;
 	case MMC_CMD(25):
 		multiple = 1;
-		/* fall thru for data op flag */
+		/* for data op flag */
+		/* fallthrough */
 	case MMC_CMD(24):
 		data = 1;
 		break;

drivers/nxp/ddr/nxp-ddr/ddr.c

@@ -269,7 +269,7 @@ static int cal_odt(const unsigned int clk,
 	unsigned int i;
 	const struct dynamic_odt *pdodt = NULL;
 
-	const static struct dynamic_odt *table[2][5] = {
+	static const struct dynamic_odt *table[2][5] = {
 		{single_S, single_D, NULL, NULL},
 		{dual_SS, dual_DD, NULL, NULL},
 	};

drivers/nxp/ddr/phy-gen2/messages.h

@@ -13,7 +13,7 @@ struct phy_msg {
 	const char *msg;
 };
 
-const static struct phy_msg messages_1d[] = {
+static const struct phy_msg messages_1d[] = {
 	{0x00000001,
 	 "PMU1:prbsGenCtl:%x\n"
 	},
@@ -1239,7 +1239,7 @@ const static struct phy_msg messages_1d[] = {
 	},
 };
 
-const static struct phy_msg messages_2d[] = {
+static const struct phy_msg messages_2d[] = {
 	{0x00000001,
 	 "PMU0: Converting %d into an MR\n"
 	},

drivers/renesas/common/emmc/emmc_cmd.c

@@ -254,8 +254,7 @@ EMMC_ERROR_CODE emmc_exec_cmd(uint32_t error_mask, uint32_t *response)
 				(SD_INFO2_ALL_ERR | SD_INFO2_CLEAR));
 
 			state = ESTATE_ISSUE_CMD;
-			/* through */
-
+			/* fallthrough */
 		case ESTATE_ISSUE_CMD:
 			/* ARG */
 			SETR_32(SD_ARG, mmc_drv_obj.cmd_info.arg);
@@ -454,8 +453,8 @@ EMMC_ERROR_CODE emmc_exec_cmd(uint32_t error_mask, uint32_t *response)
 				SETR_32(SD_STOP, 0x00000000U);
 				mmc_drv_obj.during_dma_transfer = FALSE;
 			}
-			/* through */
 
+			/* fallthrough */
 		case ESTATE_ERROR:
 			if (err_not_care_flag == TRUE) {
 				mmc_drv_obj.during_cmd_processing = FALSE;