Re: [PATCH v2] ARM: imx: romapi: Fix signed integer bitwise ops misuse

[Heiko Schocher]

Hello Marek,

On 02.07.23 03:03, Marek Vasut wrote:
> Bitwise operations on signed integers are not defined,
> replace them with per-call checks.
> 
> Reviewed-by: Peng Fan 
> Signed-off-by: Marek Vasut 
> ---
> Cc: "NXP i.MX U-Boot Team" 
> Cc: Fabio Estevam 
> Cc: Heiko Schocher 
> Cc: Heinrich Schuchardt 
> Cc: Rasmus Villemoes 
> Cc: Simon Glass 
> Cc: Stefano Babic 
> Cc: Tom Rini 
> Cc: Ye Li 
> ---
> V2: - s@then@them@
> - Add RB from Peng
> ---
>  arch/arm/mach-imx/spl_imx_romapi.c | 32 --
>  1 file changed, 21 insertions(+), 11 deletions(-)

good catch!

Reviewed-by: Heiko Schocher 

bye,
Heiko
-- 
DENX Software Engineering GmbH,  Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: h...@denx.de

Re: [PATCH] efi_driver: fix duplicate efiblk#0 issue

[Masahisa Kojima]

Hi Akashi-san,

On Mon, 3 Jul 2023 at 13:32, AKASHI Takahiro  wrote:
>
> On Mon, Jul 03, 2023 at 11:47:18AM +0900, Masahisa Kojima wrote:
> > The devnum value of the blk_desc structure starts from 0,
> > current efi_bl_create_block_device() function creates
> > two "efiblk#0" devices for the cases that blk_find_max_devnum()
> > returns -ENODEV and blk_find_max_devnum() returns 0(one device
> > found in this case).
> >
> > The devnum value for the "efiblk" name needs to be incremented.
> >
> > Signed-off-by: Masahisa Kojima 
> > ---
> >  lib/efi_driver/efi_block_device.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/lib/efi_driver/efi_block_device.c 
> > b/lib/efi_driver/efi_block_device.c
> > index add00eeebb..e37bfe6e80 100644
> > --- a/lib/efi_driver/efi_block_device.c
> > +++ b/lib/efi_driver/efi_block_device.c
> > @@ -129,6 +129,8 @@ efi_bl_create_block_device(efi_handle_t handle, void 
> > *interface)
> >   devnum = 0;
> >   else if (devnum < 0)
> >   return EFI_OUT_OF_RESOURCES;
> > + else
> > + devnum++; /* device found, note that devnum starts from 0 */
>
> So we can use blk_next_free_devnum() instead.

Yes, it is much simpler.
I will revise the patch.

Thanks,
Masahisa Kojima

>
> -Takahiro Akashi
>
> >   name = calloc(1, 18); /* strlen("efiblk#2147483648") + 1 */
> >   if (!name)
> > --
> > 2.34.1
> >

Re: [PATCH] efi_driver: fix duplicate efiblk#0 issue

[AKASHI Takahiro]

On Mon, Jul 03, 2023 at 11:47:18AM +0900, Masahisa Kojima wrote:
> The devnum value of the blk_desc structure starts from 0,
> current efi_bl_create_block_device() function creates
> two "efiblk#0" devices for the cases that blk_find_max_devnum()
> returns -ENODEV and blk_find_max_devnum() returns 0(one device
> found in this case).
> 
> The devnum value for the "efiblk" name needs to be incremented.
> 
> Signed-off-by: Masahisa Kojima 
> ---
>  lib/efi_driver/efi_block_device.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/efi_driver/efi_block_device.c 
> b/lib/efi_driver/efi_block_device.c
> index add00eeebb..e37bfe6e80 100644
> --- a/lib/efi_driver/efi_block_device.c
> +++ b/lib/efi_driver/efi_block_device.c
> @@ -129,6 +129,8 @@ efi_bl_create_block_device(efi_handle_t handle, void 
> *interface)
>   devnum = 0;
>   else if (devnum < 0)
>   return EFI_OUT_OF_RESOURCES;
> + else
> + devnum++; /* device found, note that devnum starts from 0 */

So we can use blk_next_free_devnum() instead.

-Takahiro Akashi

>   name = calloc(1, 18); /* strlen("efiblk#2147483648") + 1 */
>   if (!name)
> -- 
> 2.34.1
> 

Re: [PATCH 5/6] arm: mvebu: Add Allied Telesis x240 board

[Chris Packham]

On Mon, Jul 3, 2023 at 3:39 PM Chris Packham  wrote:
>
> The x240 and SE240 are a series of L2+ switches from Allied Telesis.
> There are a number of them in the range but as far as U-Boot is
> concerned all the CPU block components are the same so there's only one
> board defined.
>
> Signed-off-by: Chris Packham 
> ---
>  arch/arm/dts/Makefile  |   3 +-
>  arch/arm/dts/ac5-98dx35xx-atl-x240.dts | 212 +
>  arch/arm/mach-mvebu/Kconfig|   7 +
>  board/alliedtelesis/x240/MAINTAINERS   |   7 +
>  board/alliedtelesis/x240/Makefile  |   6 +
>  board/alliedtelesis/x240/x240.c|  13 ++
>  configs/x240_defconfig |  87 ++
>  include/configs/x240.h |  37 +
>  8 files changed, 371 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm/dts/ac5-98dx35xx-atl-x240.dts
>  create mode 100644 board/alliedtelesis/x240/MAINTAINERS
>  create mode 100644 board/alliedtelesis/x240/Makefile
>  create mode 100644 board/alliedtelesis/x240/x240.c
>  create mode 100644 configs/x240_defconfig
>  create mode 100644 include/configs/x240.h
>
> diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile
> index 480269fa6065..38d878a0f853 100644
> --- a/arch/arm/dts/Makefile
> +++ b/arch/arm/dts/Makefile
> @@ -303,7 +303,8 @@ dtb-$(CONFIG_ARCH_MVEBU) += \
> cn9132-db-B.dtb \
> cn9130-crb-A.dtb\
> cn9130-crb-B.dtb\
> -   ac5-98dx35xx-rd.dtb
> +   ac5-98dx35xx-rd.dtb \
> +   ac5-98dx35xx-atl-x240.dtb
>  endif
>
>  dtb-$(CONFIG_ARCH_SYNQUACER) += synquacer-sc2a11-developerbox.dtb
> diff --git a/arch/arm/dts/ac5-98dx35xx-atl-x240.dts 
> b/arch/arm/dts/ac5-98dx35xx-atl-x240.dts
> new file mode 100644
> index ..820ec18b4355
> --- /dev/null
> +++ b/arch/arm/dts/ac5-98dx35xx-atl-x240.dts
> @@ -0,0 +1,212 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +
> +/dts-v1/;
> +
> +#include 
> +#include 
> +#include "ac5-98dx35xx.dtsi"
> +
> +/ {
> +   model = "Allied Telesis x240";
> +   compatible = "alliedtelesis,x240", "marvell,ac5x", "marvell,ac5";
> +
> +   aliases {
> +   serial0 = 
> +   spiflash0 = 
> +   gpio0 = 
> +   gpio1 = 
> +   spi0 = 
> +   i2c0 = 
> +   usb0 = 
> +   pinctrl0 = 
> +   };
> +
> +   chosen {
> +   stdout-path = "serial0:115200n8";
> +   };
> +
> +   boot-board {
> +   compatible = "atl,boot-board";
> +   present-gpio = < 6 GPIO_ACTIVE_HIGH>;
> +   override-gpio = < 2 GPIO_ACTIVE_HIGH>;
> +   };
> +
> +   gpio-leds {
> +   compatible = "gpio-leds";
> +
> +   fault {
> +   label = "fault:red";
> +   gpios = <_gpio 11 GPIO_ACTIVE_LOW>;
> +   default-state = "on";
> +   };
> +   };
> +};
> +
> + {
> +   pinctrl-names = "default";
> +   pinctrl-0 = <_pins>;
> +
> +   nand-ecc-strength = <4>;
> +   nand-ecc-step-size = <512>;
> +   status = "okay";
> +
> +   partitions {
> +   compatible = "fixed-partitions";
> +   #address-cells = <1>;
> +   #size-cells = <1>;
> +
> +   partition@user {
> +   reg = <0x 0x1000>;
> +   label = "user";
> +   };
> +   };
> +};
> +
> + {
> +   status = "okay";
> +};
> +
> + {
> +   status = "okay";
> +};
> +
> + {
> +   status = "okay";
> +
> +   mux@71 {
> +   #address-cells = <1>;
> +   #size-cells = <0>;
> +   compatible = "nxp,pca9546";
> +   reg = <0x71>;
> +   i2c-mux-idle-disconnect;
> +   reset-gpios = < 4 GPIO_ACTIVE_LOW>;   /* 
> MPP36 */
> +   status = "okay";
> +
> +   i2c@1 {
> +   #address-cells = <1>;
> +   #size-cells = <0>;
> +   reg = <1>;
> +
> +   hwmon@2e {
> +   compatible = "adi,adt7476";
> +   reg = <0x2e>;
> +   };
> +
> +   rtc@68 {
> +   compatible = "adi,max31331";
> +   reg = <0x68>;
> +   };
> +
> +   system_gpio: gpio@27 {
> +   compatible = "nxp,pca9555";
> +   gpio-controller;
> +   #gpio-cells= <2>;
> +   reg = <0x27>;
> +   interrupt-parent = <>;
> +   interrupts = <25 IRQ_TYPE_LEVEL_LOW>;   /* 
> MPP25 */
> +   

[PATCH 6/6] arm: mvebu: Remove unused alias from RC AC5X dts

[Chris Packham]

The sar-reg0 alias was left over from an earlier iteration of the
patches adding support for this board. Remove the unused alias.

Fixes: 6cc8b5db40 ("arm: mvebu: Add RD-AC5X board")
Signed-off-by: Chris Packham 
---
 arch/arm/dts/ac5-98dx35xx-rd.dts | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm/dts/ac5-98dx35xx-rd.dts b/arch/arm/dts/ac5-98dx35xx-rd.dts
index d9f217cd4a5f..1dc85bb7ef24 100644
--- a/arch/arm/dts/ac5-98dx35xx-rd.dts
+++ b/arch/arm/dts/ac5-98dx35xx-rd.dts
@@ -31,7 +31,6 @@
usb0 = 
usb1 = 
pinctrl0 = 
-   sar-reg0 = "/config-space/sar-reg";
};
 
usb1phy: usb-phy {
-- 
2.41.0

[PATCH 5/6] arm: mvebu: Add Allied Telesis x240 board

[Chris Packham]

The x240 and SE240 are a series of L2+ switches from Allied Telesis.
There are a number of them in the range but as far as U-Boot is
concerned all the CPU block components are the same so there's only one
board defined.

Signed-off-by: Chris Packham 
---
 arch/arm/dts/Makefile  |   3 +-
 arch/arm/dts/ac5-98dx35xx-atl-x240.dts | 212 +
 arch/arm/mach-mvebu/Kconfig|   7 +
 board/alliedtelesis/x240/MAINTAINERS   |   7 +
 board/alliedtelesis/x240/Makefile  |   6 +
 board/alliedtelesis/x240/x240.c|  13 ++
 configs/x240_defconfig |  87 ++
 include/configs/x240.h |  37 +
 8 files changed, 371 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/dts/ac5-98dx35xx-atl-x240.dts
 create mode 100644 board/alliedtelesis/x240/MAINTAINERS
 create mode 100644 board/alliedtelesis/x240/Makefile
 create mode 100644 board/alliedtelesis/x240/x240.c
 create mode 100644 configs/x240_defconfig
 create mode 100644 include/configs/x240.h

diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile
index 480269fa6065..38d878a0f853 100644
--- a/arch/arm/dts/Makefile
+++ b/arch/arm/dts/Makefile
@@ -303,7 +303,8 @@ dtb-$(CONFIG_ARCH_MVEBU) += \
cn9132-db-B.dtb \
cn9130-crb-A.dtb\
cn9130-crb-B.dtb\
-   ac5-98dx35xx-rd.dtb
+   ac5-98dx35xx-rd.dtb \
+   ac5-98dx35xx-atl-x240.dtb
 endif
 
 dtb-$(CONFIG_ARCH_SYNQUACER) += synquacer-sc2a11-developerbox.dtb
diff --git a/arch/arm/dts/ac5-98dx35xx-atl-x240.dts 
b/arch/arm/dts/ac5-98dx35xx-atl-x240.dts
new file mode 100644
index ..820ec18b4355
--- /dev/null
+++ b/arch/arm/dts/ac5-98dx35xx-atl-x240.dts
@@ -0,0 +1,212 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+
+/dts-v1/;
+
+#include 
+#include 
+#include "ac5-98dx35xx.dtsi"
+
+/ {
+   model = "Allied Telesis x240";
+   compatible = "alliedtelesis,x240", "marvell,ac5x", "marvell,ac5";
+
+   aliases {
+   serial0 = 
+   spiflash0 = 
+   gpio0 = 
+   gpio1 = 
+   spi0 = 
+   i2c0 = 
+   usb0 = 
+   pinctrl0 = 
+   };
+
+   chosen {
+   stdout-path = "serial0:115200n8";
+   };
+
+   boot-board {
+   compatible = "atl,boot-board";
+   present-gpio = < 6 GPIO_ACTIVE_HIGH>;
+   override-gpio = < 2 GPIO_ACTIVE_HIGH>;
+   };
+
+   gpio-leds {
+   compatible = "gpio-leds";
+
+   fault {
+   label = "fault:red";
+   gpios = <_gpio 11 GPIO_ACTIVE_LOW>;
+   default-state = "on";
+   };
+   };
+};
+
+ {
+   pinctrl-names = "default";
+   pinctrl-0 = <_pins>;
+
+   nand-ecc-strength = <4>;
+   nand-ecc-step-size = <512>;
+   status = "okay";
+
+   partitions {
+   compatible = "fixed-partitions";
+   #address-cells = <1>;
+   #size-cells = <1>;
+
+   partition@user {
+   reg = <0x 0x1000>;
+   label = "user";
+   };
+   };
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+
+   mux@71 {
+   #address-cells = <1>;
+   #size-cells = <0>;
+   compatible = "nxp,pca9546";
+   reg = <0x71>;
+   i2c-mux-idle-disconnect;
+   reset-gpios = < 4 GPIO_ACTIVE_LOW>;   /* 
MPP36 */
+   status = "okay";
+
+   i2c@1 {
+   #address-cells = <1>;
+   #size-cells = <0>;
+   reg = <1>;
+
+   hwmon@2e {
+   compatible = "adi,adt7476";
+   reg = <0x2e>;
+   };
+
+   rtc@68 {
+   compatible = "adi,max31331";
+   reg = <0x68>;
+   };
+
+   system_gpio: gpio@27 {
+   compatible = "nxp,pca9555";
+   gpio-controller;
+   #gpio-cells= <2>;
+   reg = <0x27>;
+   interrupt-parent = <>;
+   interrupts = <25 IRQ_TYPE_LEVEL_LOW>;   /* 
MPP25 */
+   };
+   };
+   };
+};
+
+ {
+   status = "okay";
+   spiflash0: flash@0 {
+   compatible = "jedec,spi-nor";
+   spi-max-frequency = <5000>;
+   spi-tx-bus-width = <1>; /* 1-single, 2-dual, 4-quad */
+   spi-rx-bus-width = <1>; /* 1-single, 2-dual, 4-quad */
+   reg = 

[PATCH 4/6] mtd: nand: pxa3xx: Enable devbus/nand arbiter on Armada 8K

[Chris Packham]

The CN9130 SoC (an ARMADA 8K type) has both a NAND Flash Controller and
a generic local bus controller (Device Bus Controller) that share common
pins.

With a board design that incorporates both a NAND flash and uses
the Device Bus (in our case for an SRAM) accessing the Device Bus device
fails unless the NfArbiterEn bit is set. Setting the bit enables
arbitration between the Device Bus and the NAND flash.

Since there is no obvious downside in enabling this for designs that
don't require arbitration, we always enable it.

Signed-off-by: Chris Packham 
---
 drivers/mtd/nand/raw/pxa3xx_nand.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/raw/pxa3xx_nand.c 
b/drivers/mtd/nand/raw/pxa3xx_nand.c
index 9dee580ab9c2..d502e967f92c 100644
--- a/drivers/mtd/nand/raw/pxa3xx_nand.c
+++ b/drivers/mtd/nand/raw/pxa3xx_nand.c
@@ -125,6 +125,7 @@ DECLARE_GLOBAL_DATA_PTR;
 /* System control register and bit to enable NAND on some SoCs */
 #define GENCONF_SOC_DEVICE_MUX 0x208
 #define GENCONF_SOC_DEVICE_MUX_NFC_EN BIT(0)
+#define GENCONF_SOC_DEVICE_MUX_NFC_DEVBUS_ARB_EN BIT(27)
 
 /*
  * This should be large enough to read 'ONFI' and 'JEDEC'.
@@ -1739,7 +1740,7 @@ static int alloc_nand_resource(struct udevice *dev, 
struct pxa3xx_nand_info *inf
return PTR_ERR(sysctrl_base);
 
regmap_read(sysctrl_base, GENCONF_SOC_DEVICE_MUX, );
-   reg |= GENCONF_SOC_DEVICE_MUX_NFC_EN;
+   reg |= GENCONF_SOC_DEVICE_MUX_NFC_EN | 
GENCONF_SOC_DEVICE_MUX_NFC_DEVBUS_ARB_EN;
regmap_write(sysctrl_base, GENCONF_SOC_DEVICE_MUX, reg);
}
 
-- 
2.41.0

[PATCH 3/6] mtd: nand: pxa3xx: Add support for the Marvell AC5 SoC

[Chris Packham]

The NAND flash controller (NFC) on the AC5/AC5X SoC is the same as
the NFC used on other Marvell SoCs. It does have the additional
restriction of only supporting SDR timing modes up to 3.

Signed-off-by: Chris Packham 
---
 drivers/mtd/nand/raw/pxa3xx_nand.c | 17 ++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/raw/pxa3xx_nand.c 
b/drivers/mtd/nand/raw/pxa3xx_nand.c
index fcd1b9c63614..9dee580ab9c2 100644
--- a/drivers/mtd/nand/raw/pxa3xx_nand.c
+++ b/drivers/mtd/nand/raw/pxa3xx_nand.c
@@ -167,6 +167,7 @@ enum pxa3xx_nand_variant {
PXA3XX_NAND_VARIANT_PXA,
PXA3XX_NAND_VARIANT_ARMADA370,
PXA3XX_NAND_VARIANT_ARMADA_8K,
+   PXA3XX_NAND_VARIANT_AC5,
 };
 
 struct pxa3xx_nand_host {
@@ -391,6 +392,10 @@ static const struct udevice_id pxa3xx_nand_dt_ids[] = {
.compatible = "marvell,armada-8k-nand-controller",
.data = PXA3XX_NAND_VARIANT_ARMADA_8K,
},
+   {
+   .compatible = "marvell,mvebu-ac5-pxa3xx-nand",
+   .data = PXA3XX_NAND_VARIANT_AC5,
+   },
{}
 };
 
@@ -505,6 +510,9 @@ static int pxa3xx_nand_init_timings(struct pxa3xx_nand_host 
*host)
if (mode < 0)
mode = 0;
 
+   if (info->variant == PXA3XX_NAND_VARIANT_AC5)
+   mode = min(mode, 3);
+
timings = onfi_async_timing_mode_to_sdr_timings(mode);
if (IS_ERR(timings))
return PTR_ERR(timings);
@@ -730,7 +738,8 @@ static irqreturn_t pxa3xx_nand_irq(struct pxa3xx_nand_info 
*info)
 
/* NDCB3 register is available in NFCv2 (Armada 370/XP SoC) */
if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370 ||
-   info->variant == PXA3XX_NAND_VARIANT_ARMADA_8K)
+   info->variant == PXA3XX_NAND_VARIANT_ARMADA_8K ||
+   info->variant == PXA3XX_NAND_VARIANT_AC5)
nand_writel(info, NDCB0, info->ndcb3);
}
 
@@ -1579,7 +1588,8 @@ static int pxa3xx_nand_scan(struct mtd_info *mtd)
 
/* Device detection must be done with ECC disabled */
if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370 ||
-   info->variant == PXA3XX_NAND_VARIANT_ARMADA_8K)
+   info->variant == PXA3XX_NAND_VARIANT_ARMADA_8K ||
+   info->variant == PXA3XX_NAND_VARIANT_AC5)
nand_writel(info, NDECCCTRL, 0x0);
 
if (nand_scan_ident(mtd, 1, NULL))
@@ -1630,7 +1640,8 @@ static int pxa3xx_nand_scan(struct mtd_info *mtd)
 */
if (mtd->writesize > info->chunk_size) {
if (info->variant == PXA3XX_NAND_VARIANT_ARMADA370 ||
-   info->variant == PXA3XX_NAND_VARIANT_ARMADA_8K) {
+   info->variant == PXA3XX_NAND_VARIANT_ARMADA_8K ||
+   info->variant == PXA3XX_NAND_VARIANT_AC5) {
chip->cmdfunc = nand_cmdfunc_extended;
} else {
dev_err(mtd->dev,
-- 
2.41.0

[PATCH 1/6] arm: mvebu: ac5: Add nand-controller node

[Chris Packham]

The AC5/AC5X SoC has a NAND flash controller. Add this to the
SoC device tree.

Signed-off-by: Chris Packham 
---
 arch/arm/dts/ac5-98dx25xx.dtsi | 9 +
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/dts/ac5-98dx25xx.dtsi b/arch/arm/dts/ac5-98dx25xx.dtsi
index 3c68355f323a..f53b4781d7fd 100644
--- a/arch/arm/dts/ac5-98dx25xx.dtsi
+++ b/arch/arm/dts/ac5-98dx25xx.dtsi
@@ -251,6 +251,15 @@
status = "disabled";
};
 
+   nand: nand-controller@805b {
+   compatible = "marvell,mvebu-ac5-pxa3xx-nand";
+   reg = <0x0 0x805b 0x0 0x54>;
+   #address-cells = <0x0001>;
+   marvell,nand-enable-arbiter;
+   num-cs = <0x0001>;
+   status = "disabled";
+   };
+
gic: interrupt-controller@8060 {
compatible = "arm,gic-v3";
#interrupt-cells = <3>;
-- 
2.41.0

[PATCH 2/6] arm: mvebu: ac5: Define mvebu_get_nand_clock()

[Chris Packham]

The NF_CLK for the AC5 SoC runs at 400MHz. There's no strapping
or gating require so just add a mvebu_get_nand_clock() that
returns this value.

Signed-off-by: Chris Packham 
---
 arch/arm/mach-mvebu/alleycat5/soc.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/mach-mvebu/alleycat5/soc.c 
b/arch/arm/mach-mvebu/alleycat5/soc.c
index dc69f46eedb2..734b0a87dd49 100644
--- a/arch/arm/mach-mvebu/alleycat5/soc.c
+++ b/arch/arm/mach-mvebu/alleycat5/soc.c
@@ -255,6 +255,12 @@ void soc_print_clock_info(void)
printf("\tMSS %4d MHz\n", 200);
 }
 
+/* Return NAND clock in Hz */
+u32 mvebu_get_nand_clock(void)
+{
+   return 400 * 100;
+}
+
 /*
  * Override of __weak int mach_cpu_init(void) :
  * SoC/machine dependent CPU setup
-- 
2.41.0

[PATCH 0/6] Support for AC5X NAND and AT-x240 board

[Chris Packham]

This series adds support for the NAND flash controller on the AC5X SoC
and adds support for the Allied Telesis x240 board.

I've also included 2 unrelated changes. "arm: mvebu: Remove unused alias
from RC AC5X dts" removes an unused alias from the dts. This was in the
neighborhood of the x240 so I included it.

"mtd: nand: pxa3xx: Enable devbus/nand arbiter on Armada 8K" allows
using the NFC and device bus at the same time. This is needed for
another board (CN9130 based) that I'll hopefully get upstream in the
near future. I figured I'd include it now since I was touching the NAND
driver.

Chris Packham (6):
  arm: mvebu: ac5: Add nand-controller node
  arm: mvebu: ac5: Define mvebu_get_nand_clock()
  mtd: nand: pxa3xx: Add support for the Marvell AC5 SoC
  mtd: nand: pxa3xx: Enable devbus/nand arbiter on Armada 8K
  arm: mvebu: Add Allied Telesis x240 board
  arm: mvebu: Remove unused alias from RC AC5X dts

 arch/arm/dts/Makefile  |   3 +-
 arch/arm/dts/ac5-98dx25xx.dtsi |   9 ++
 arch/arm/dts/ac5-98dx35xx-atl-x240.dts | 212 +
 arch/arm/dts/ac5-98dx35xx-rd.dts   |   1 -
 arch/arm/mach-mvebu/Kconfig|   7 +
 arch/arm/mach-mvebu/alleycat5/soc.c|   6 +
 board/alliedtelesis/x240/MAINTAINERS   |   7 +
 board/alliedtelesis/x240/Makefile  |   6 +
 board/alliedtelesis/x240/x240.c|  13 ++
 configs/x240_defconfig |  87 ++
 drivers/mtd/nand/raw/pxa3xx_nand.c |  20 ++-
 include/configs/x240.h |  37 +
 12 files changed, 402 insertions(+), 6 deletions(-)
 create mode 100644 arch/arm/dts/ac5-98dx35xx-atl-x240.dts
 create mode 100644 board/alliedtelesis/x240/MAINTAINERS
 create mode 100644 board/alliedtelesis/x240/Makefile
 create mode 100644 board/alliedtelesis/x240/x240.c
 create mode 100644 configs/x240_defconfig
 create mode 100644 include/configs/x240.h

-- 
2.41.0

[PATCH] efi_driver: fix duplicate efiblk#0 issue

[Masahisa Kojima]

The devnum value of the blk_desc structure starts from 0,
current efi_bl_create_block_device() function creates
two "efiblk#0" devices for the cases that blk_find_max_devnum()
returns -ENODEV and blk_find_max_devnum() returns 0(one device
found in this case).

The devnum value for the "efiblk" name needs to be incremented.

Signed-off-by: Masahisa Kojima 
---
 lib/efi_driver/efi_block_device.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/efi_driver/efi_block_device.c 
b/lib/efi_driver/efi_block_device.c
index add00eeebb..e37bfe6e80 100644
--- a/lib/efi_driver/efi_block_device.c
+++ b/lib/efi_driver/efi_block_device.c
@@ -129,6 +129,8 @@ efi_bl_create_block_device(efi_handle_t handle, void 
*interface)
devnum = 0;
else if (devnum < 0)
return EFI_OUT_OF_RESOURCES;
+   else
+   devnum++; /* device found, note that devnum starts from 0 */
 
name = calloc(1, 18); /* strlen("efiblk#2147483648") + 1 */
if (!name)
-- 
2.34.1

Re: [PATCH 09/10] doc: cmd: add documentation for scmi

[AKASHI Takahiro]

On Thu, Jun 29, 2023 at 08:10:02PM +0100, Simon Glass wrote:
> Hi AKASHI,
> 
> On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
>  wrote:
> >
> > This is a help text for scmi command.
> >
> > Signed-off-by: AKASHI Takahiro 
> > ---
> >  doc/usage/cmd/scmi.rst | 98 ++
> >  1 file changed, 98 insertions(+)
> >  create mode 100644 doc/usage/cmd/scmi.rst
> >
> > diff --git a/doc/usage/cmd/scmi.rst b/doc/usage/cmd/scmi.rst
> > new file mode 100644
> > index ..20cdae4b877d
> > --- /dev/null
> > +++ b/doc/usage/cmd/scmi.rst
> > @@ -0,0 +1,98 @@
> > +.. SPDX-License-Identifier: GPL-2.0+:
> > +
> > +scmi command
> > +
> > +
> > +Synopsis
> > +
> > +
> > +::
> > +
> > +scmi base info
> > +scmi base perm_dev   
> > +scmi base perm_proto
> > +scmi base reset  
> > +
> > +Description
> > +---
> > +
> > +The scmi command is used to access and operate on SCMI server.
> > +
> > +scmi base info
> > +~~
> > +Show base information about SCMI server and supported protocols
> > +
> > +scmi base perm_dev
> > +~~
> > +Allow or deny access permission to the device
> > +
> > +scmi base perm_proto
> > +
> > +Allow or deny access to the protocol on the device
> > +
> > +scmi base reset
> > +~~~
> > +Reset the existing configurations
> > +
> > +Parameters are used as follows:
> > +
> > +
> > +Agent ID
> 
> what is this?

I thought that the meaning was trivial in SCMI context.
Will change it to "SCMI Agent ID".


> > +
> > +
> > +Device ID
> 
> what is this?

Again, will change it to "SCMI Device ID".

> > +
> > +
> > +Protocol ID, should not be 0x10 (base protocol)
> 
> what is this? Please add more detail

Again, will change it to "SCMI Protocol ID".
I think that users should be familiar with those terms
if they want to use these interfaces.

> > +
> > +
> > +Flags to control the action. See SCMI specification for
> > +defined values.
> 
> ?
> 
> Please add the flags here, or at the very least provide a URL and page
> number, etc.

I intentionally avoid providing details here because a set of flags
acceptable to a specific SCMI server may depend on the server
and its implementation version.
The interface on U-Boot is just a wrapper to make a call to SCMI server
via a transport layer and doesn't care what the parameters means.
That said, I agree to referring to a URL to SCMI specification somewhere
in this document.

Thanks,
-Takahiro Akashi

> > +
> > +Example
> > +---
> > +
> > +Obtain basic information about SCMI server:
> > +
> > +::
> > +
> > +=> scmi base info
> > +SCMI device: scmi
> > +  protocol version: 0x2
> > +  # of agents: 3
> > +  0: platform
> > +> 1: OSPM
> > +  2: PSCI
> > +  # of protocols: 4
> > +  Power domain management
> > +  Performance domain management
> > +  Clock management
> > +  Sensor management
> > +  vendor: Linaro
> > +  sub vendor: PMWG
> > +  impl version: 0x20b
> > +
> > +Ask for access permission to device#0:
> > +
> > +::
> > +
> > +=> scmi base perm_dev 1 0 1
> > +
> > +Reset configurations with all access permission settings retained:
> > +
> > +::
> > +
> > +=> scmi base reset 1 0
> > +
> > +Configuration
> > +-
> > +
> > +The scmi command is only available if CONFIG_CMD_SCMI=y.
> > +Default n because this command is mainly for debug purpose.
> > +
> > +Return value
> > +
> > +
> > +The return value ($?) is set to 0 if the operation succeeded,
> > +1 if the operation failed or -1 if the operation failed due to
> > +a syntax error.
> > --
> > 2.41.0
> >
> 
> Regards,
> Simon

Re: Pull request efi-2023-07-rc6

[Tom Rini]

On Sat, Jul 01, 2023 at 08:08:33PM +0200, Heinrich Schuchardt wrote:

> Dear Tom,
> 
> The following changes since commit 5fa30f2351ac3c0458069896bc868eae927df410:
> 
>   smegw01: Fix wrong symbol override (2023-06-29 09:55:50 -0400)
> 
> are available in the Git repository at:
> 
>   https://source.denx.de/u-boot/custodians/u-boot-efi.git
> tags/efi-2023-07-rc6
> 
> for you to fetch changes up to 2b17dd1d9d8ca2cfc7f5e964384d4d8418904c35:
> 
>   ARM: arm11: Add C wrapper for allow_unaligned() (2023-07-01 17:29:15
> +0200)
> 
> Gitlab CI showed no issues:
> https://source.denx.de/u-boot/custodians/u-boot-efi/-/pipelines/16741
> 

Applied to u-boot/master, thanks!

-- 
Tom


signature.asc
Description: PGP signature

Re: Current way of using pinctrl-mvebu

[Chris Packham]

Answering my own question (I think)

On Mon, Jul 3, 2023 at 12:10 PM Chris Packham  wrote:
>
> Hi,
>
> I'm looking to upstream support for a new board with the Marvell AC5X
> SoC and some NAND driver changes to support the SoC/board. I've got
> things working when chain loading vendor based u-boot -> upstream
> u-boot but when I boot directly the NAND controller reports
> "pxa3xx-nand nand-controller@805b: Ready timeout!!!".
>
> I think this is because the multi purpose pins are not in NAND/DEV
> mode. When chainloading the vendor u-boot has already done this so the
> driver works.
>
> I notice that pinctrl-mvebu.c just expects a single fixed pin-func
> property the covers all the functions required. How is the probe for
> this supposed to be configured?

If I define the proper pinconf subnodes and associate them with the
device then the pinmux does get probed. I still need to define a
pin-func property otherwise
mvebu_pinctl_probe()/mvebu_pinctrl_set_state_all() will fail with
-EINVAL.

>
> Thanks,
> Chris

Re: [PATCH 07/10] test: dm: add SCMI base protocol test

[AKASHI Takahiro]

On Thu, Jun 29, 2023 at 08:09:58PM +0100, Simon Glass wrote:
> Hi AKASHI,
> 
> On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
>  wrote:
> >
> > Added is a new unit test for SCMI base protocol, which will exercise all
> > the commands provided by the protocol, except SCMI_BASE_NOTIFY_ERRORS.
> >   $ ut dm scmi_base
> > It is assumed that test.dtb is used as sandbox's device tree.
> >
> > Signed-off-by: AKASHI Takahiro 
> > ---
> >  test/dm/scmi.c | 112 +
> >  1 file changed, 112 insertions(+)
> >
> > diff --git a/test/dm/scmi.c b/test/dm/scmi.c
> > index 881be3171b7c..563017bb63e0 100644
> > --- a/test/dm/scmi.c
> > +++ b/test/dm/scmi.c
> > @@ -16,6 +16,9 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> > +#include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -95,6 +98,115 @@ static int dm_test_scmi_sandbox_agent(struct 
> > unit_test_state *uts)
> >  }
> >  DM_TEST(dm_test_scmi_sandbox_agent, UT_TESTF_SCAN_FDT);
> >
> > +static int dm_test_scmi_base(struct unit_test_state *uts)
> > +{
> > +   struct udevice *agent_dev, *base;
> > +   struct scmi_agent_priv *priv;
> > +   const struct scmi_base_ops *ops;
> > +   u32 version, num_agents, num_protocols, impl_version;
> > +   u32 attributes, agent_id;
> > +   char vendor[SCMI_BASE_NAME_LENGTH_MAX],
> > +agent_name[SCMI_BASE_NAME_LENGTH_MAX];
> > +   u8 *protocols;
> > +   int ret;
> > +
> > +   /* preparation */
> > +   ut_assertok(uclass_get_device_by_name(UCLASS_SCMI_AGENT, "scmi",
> > + _dev));
> > +   ut_assertnonnull(agent_dev);
> > +   ut_assertnonnull(priv = dev_get_uclass_plat(agent_dev));
> > +   ut_assertnonnull(base = scmi_get_protocol(agent_dev,
> > + SCMI_PROTOCOL_ID_BASE));
> > +   ut_assertnonnull(ops = dev_get_driver_ops(base));
> > +
> > +   /* version */
> > +   ret = (*ops->protocol_version)(base, );
> 
> Can you add uclass helpers to call each of the methods? That is how it
> is commonly done. You should not be calling ops->xxx directly here.

Yes, I will add inline functions instead.

-Takahiro Akashi

> > +   ut_assertok(ret);
> > +   ut_asserteq(priv->version, version);
> > +
> > +   /* protocol attributes */
> > +   ret = (*ops->protocol_attrs)(base, _agents, _protocols);
> > +   ut_assertok(ret);
> > +   ut_asserteq(priv->num_agents, num_agents);
> > +   ut_asserteq(priv->num_protocols, num_protocols);
> > +
> > +   /* discover vendor */
> > +   ret = (*ops->base_discover_vendor)(base, vendor);
> > +   ut_assertok(ret);
> > +   ut_asserteq_str(priv->vendor, vendor);
> > +
> > +   /* message attributes */
> > +   ret = (*ops->protocol_message_attrs)(base,
> > +SCMI_BASE_DISCOVER_SUB_VENDOR,
> > +);
> > +   ut_assertok(ret);
> > +   ut_assertok(attributes);
> > +
> > +   /* discover sub vendor */
> > +   ret = (*ops->base_discover_sub_vendor)(base, vendor);
> > +   ut_assertok(ret);
> > +   ut_asserteq_str(priv->sub_vendor, vendor);
> > +
> > +   /* impl version */
> > +   ret = (*ops->base_discover_impl_version)(base, _version);
> > +   ut_assertok(ret);
> > +   ut_asserteq(priv->impl_version, impl_version);
> > +
> > +   /* discover agent (my self) */
> > +   ret = (*ops->base_discover_agent)(base, 0x,
> > + _id, agent_name);
> > +   ut_assertok(ret);
> > +   ut_asserteq(priv->agent_id, agent_id);
> > +   ut_asserteq_str(priv->agent_name, agent_name);
> > +
> > +   /* discover protocols */
> > +   ret = (*ops->base_discover_list_protocols)(base, );
> > +   ut_asserteq(num_protocols, ret);
> > +   ut_asserteq_mem(priv->protocols, protocols, sizeof(u8) * 
> > num_protocols);
> > +   free(protocols);
> > +
> > +   /*
> > +* NOTE: Sandbox SCMI driver handles device-0 only. It supports 
> > setting
> > +* access and protocol permissions, but doesn't allow unsetting 
> > them nor
> > +* resetting the configurations.
> > +*/
> > +   /* set device permissions */
> > +   ret = (*ops->base_set_device_permissions)(base, agent_id, 0,
> > + 
> > SCMI_BASE_SET_DEVICE_PERMISSIONS_ACCESS);
> > +   ut_assertok(ret); /* SCMI_SUCCESS */
> > +   ret = (*ops->base_set_device_permissions)(base, agent_id, 1,
> > + 
> > SCMI_BASE_SET_DEVICE_PERMISSIONS_ACCESS);
> > +   ut_asserteq(-ENOENT, ret); /* SCMI_NOT_FOUND */
> > +   ret = (*ops->base_set_device_permissions)(base, agent_id, 0, 0);
> > +   ut_asserteq(-EACCES, ret); /* SCMI_DENIED */
> > +
> > +   /* set protocol permissions */
> 

Re: [PATCH 08/10] cmd: add scmi command for SCMI firmware

[AKASHI Takahiro]

On Thu, Jun 29, 2023 at 08:10:00PM +0100, Simon Glass wrote:
> Hi AKASHI,
> 
> On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
>  wrote:
> >
> > This command, "scmi", provides a command line interface to various SCMI
> > protocols. It supports at least initially SCMI base protocol with the sub
> > command, "base", and is intended mainly for debug purpose.
> >
> > Signed-off-by: AKASHI Takahiro 
> > ---
> >  cmd/Kconfig  |   8 ++
> >  cmd/Makefile |   1 +
> >  cmd/scmi.c   | 373 +++
> >  3 files changed, 382 insertions(+)
> >  create mode 100644 cmd/scmi.c
> >
> > diff --git a/cmd/Kconfig b/cmd/Kconfig
> > index 02e54f1e50fe..065470a76295 100644
> > --- a/cmd/Kconfig
> > +++ b/cmd/Kconfig
> > @@ -2504,6 +2504,14 @@ config CMD_CROS_EC
> >   a number of sub-commands for performing EC tasks such as
> >   updating its flash, accessing a small saved context area
> >   and talking to the I2C bus behind the EC (if there is one).
> > +
> > +config CMD_SCMI
> > +   bool "Enable scmi command"
> > +   depends on SCMI_FIRMWARE
> > +   default n
> > +   help
> > + This command provides user interfaces to several SCMI protocols,
> > + including Base protocol.
> 
> Please mention what is SCMI

I will give a full spelling.

> >  endmenu
> >
> >  menu "Filesystem commands"
> > diff --git a/cmd/Makefile b/cmd/Makefile
> > index 6c37521b4e2b..826e0b74b587 100644
> > --- a/cmd/Makefile
> > +++ b/cmd/Makefile
> > @@ -156,6 +156,7 @@ obj-$(CONFIG_CMD_SATA) += sata.o
> >  obj-$(CONFIG_CMD_NVME) += nvme.o
> >  obj-$(CONFIG_SANDBOX) += sb.o
> >  obj-$(CONFIG_CMD_SF) += sf.o
> > +obj-$(CONFIG_CMD_SCMI) += scmi.o
> >  obj-$(CONFIG_CMD_SCSI) += scsi.o disk.o
> >  obj-$(CONFIG_CMD_SHA1SUM) += sha1sum.o
> >  obj-$(CONFIG_CMD_SEAMA) += seama.o
> > diff --git a/cmd/scmi.c b/cmd/scmi.c
> > new file mode 100644
> > index ..c97f77e97d95
> > --- /dev/null
> > +++ b/cmd/scmi.c
> > @@ -0,0 +1,373 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + *  SCMI utility command
> > + *
> > + *  Copyright (c) 2023 Linaro Limited
> > + * Author: AKASHI Takahiro
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include  /* uclass_get_device */
> 
> Goes before linux/bitfield.h

Can you tell me why?

> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +static struct udevice *agent;
> > +static struct udevice *base_proto;
> > +static const struct scmi_base_ops *ops;
> > +
> > +struct {
> > +   enum scmi_std_protocol id;
> > +   const char *name;
> > +} protocol_name[] = {
> > +   {SCMI_PROTOCOL_ID_BASE, "Base"},
> > +   {SCMI_PROTOCOL_ID_POWER_DOMAIN, "Power domain management"},
> > +   {SCMI_PROTOCOL_ID_SYSTEM, "System power management"},
> > +   {SCMI_PROTOCOL_ID_PERF, "Performance domain management"},
> > +   {SCMI_PROTOCOL_ID_CLOCK, "Clock management"},
> > +   {SCMI_PROTOCOL_ID_SENSOR, "Sensor management"},
> > +   {SCMI_PROTOCOL_ID_RESET_DOMAIN, "Reset domain management"},
> > +   {SCMI_PROTOCOL_ID_VOLTAGE_DOMAIN, "Voltage domain management"},
> > +};
> > +
> > +/**
> > + * scmi_convert_id_to_string() - get the name of SCMI protocol
> > + *
> > + * @id:Protocol ID
> > + *
> > + * Get the printable name of the protocol, @id
> > + *
> > + * Return: Name string on success, NULL on failure
> > + */
> > +static const char *scmi_convert_id_to_string(enum scmi_std_protocol id)
> 
> scmi_lookup_id?

Not sure your intention.
The name above gives us exactly what this function does for pretty printing
instead of a number.
I think that 'lookup' implies we try to determine if the id exists or not.

> > +{
> > +   int i;
> > +
> > +   for (i = 0; i < ARRAY_SIZE(protocol_name); i++)
> > +   if (id == protocol_name[i].id)
> > +   return protocol_name[i].name;
> > +
> > +   return NULL;
> > +}
> > +
> > +/**
> > + * do_scmi_base_info() - get the information of SCMI services
> > + *
> > + * @cmdtp: Command table
> > + * @flag:  Command flag
> > + * @argc:  Number of arguments
> > + * @argv:  Argument array
> > + *
> > + * Get the information of SCMI services using various interfaces
> > + * provided by the Base protocol.
> > + *
> > + * Return: CMD_RET_SUCCESS on success, CMD_RET_RET_FAILURE on failure
> > + */
> > +static int do_scmi_base_info(struct cmd_tbl *cmdtp, int flag, int argc,
> > +char * const argv[])
> > +{
> > +   u32 agent_id, num_protocols;
> > +   u8 agent_name[SCMI_BASE_NAME_LENGTH_MAX], *protocols;
> > +   int i, ret;
> > +
> > +   if (argc != 1)
> > +   return CMD_RET_USAGE;
> > +
> > +   printf("SCMI device: %s\n", agent->name);
> > +   printf("  protocol version: 0x%x\n", scmi_version(agent));
> > +   printf("  # of agents: %d\n", scmi_num_agents(agent));
> > + 

Re: [PATCH 01/10] firmware: scmi: implement SCMI base protocol

[AKASHI Takahiro]

On Thu, Jun 29, 2023 at 08:09:45PM +0100, Simon Glass wrote:
> Hi AKASHI,
> 
> On Wed, 28 Jun 2023 at 01:49, AKASHI Takahiro
>  wrote:
> >
> > SCMI base protocol is mandatory according to the SCMI specification.
> >
> > With this patch, SCMI base protocol can be accessed via SCMI transport
> > layers. All the commands, except SCMI_BASE_NOTIFY_ERRORS, are supported.
> > This is because U-Boot doesn't support interrupts and the current transport
> > layers are not able to handle asynchronous messages properly.
> >
> > Signed-off-by: AKASHI Takahiro 
> > ---
> >  drivers/firmware/scmi/Makefile |   1 +
> >  drivers/firmware/scmi/base.c   | 517 +
> >  include/dm/uclass-id.h |   1 +
> >  include/scmi_protocols.h   | 201 -
> >  4 files changed, 718 insertions(+), 2 deletions(-)
> >  create mode 100644 drivers/firmware/scmi/base.c
> >
> > diff --git a/drivers/firmware/scmi/Makefile b/drivers/firmware/scmi/Makefile
> > index b2ff483c75a1..1a23d4981709 100644
> > --- a/drivers/firmware/scmi/Makefile
> > +++ b/drivers/firmware/scmi/Makefile
> > @@ -1,4 +1,5 @@
> >  obj-y  += scmi_agent-uclass.o
> > +obj-y  += base.o
> >  obj-y  += smt.o
> >  obj-$(CONFIG_SCMI_AGENT_SMCCC) += smccc_agent.o
> >  obj-$(CONFIG_SCMI_AGENT_MAILBOX)   += mailbox_agent.o
> 
> [..]
> 
> > diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> > index 307ad6931ca7..f7a110852321 100644
> > --- a/include/dm/uclass-id.h
> > +++ b/include/dm/uclass-id.h
> > @@ -116,6 +116,7 @@ enum uclass_id {
> > UCLASS_RNG, /* Random Number Generator */
> > UCLASS_RTC, /* Real time clock device */
> > UCLASS_SCMI_AGENT,  /* Interface with an SCMI server */
> > +   UCLASS_SCMI_BASE,   /* Interface for SCMI Base protocol */
> > UCLASS_SCSI,/* SCSI device */
> > UCLASS_SERIAL,  /* Serial UART */
> > UCLASS_SIMPLE_BUS,  /* Bus with child devices */
> > diff --git a/include/scmi_protocols.h b/include/scmi_protocols.h
> > index a220cb2a91ad..769041654534 100644
> > --- a/include/scmi_protocols.h
> > +++ b/include/scmi_protocols.h
> > @@ -15,6 +15,8 @@
> >   * https://developer.arm.com/docs/den0056/b
> >   */
> >
> > +#define SCMI_BASE_NAME_LENGTH_MAX 16
> > +
> >  enum scmi_std_protocol {
> > SCMI_PROTOCOL_ID_BASE = 0x10,
> > SCMI_PROTOCOL_ID_POWER_DOMAIN = 0x11,
> > @@ -41,12 +43,207 @@ enum scmi_status_code {
> >  };
> >
> >  /*
> > - * Generic message IDs
> > + * SCMI Base Protocol
> >   */
> > -enum scmi_discovery_id {
> > +#define SCMI_BASE_PROTOCOL_VERSION 0x2
> > +
> > +enum scmi_base_message_id {
> > SCMI_PROTOCOL_VERSION = 0x0,
> > SCMI_PROTOCOL_ATTRIBUTES = 0x1,
> > SCMI_PROTOCOL_MESSAGE_ATTRIBUTES = 0x2,
> > +   SCMI_BASE_DISCOVER_VENDOR = 0x3,
> > +   SCMI_BASE_DISCOVER_SUB_VENDOR = 0x4,
> > +   SCMI_BASE_DISCOVER_IMPL_VERSION = 0x5,
> > +   SCMI_BASE_DISCOVER_LIST_PROTOCOLS = 0x6,
> > +   SCMI_BASE_DISCOVER_AGENT = 0x7,
> > +   SCMI_BASE_NOTIFY_ERRORS = 0x8,
> > +   SCMI_BASE_SET_DEVICE_PERMISSIONS = 0x9,
> > +   SCMI_BASE_SET_PROTOCOL_PERMISSIONS = 0xa,
> > +   SCMI_BASE_RESET_AGENT_CONFIGURATION = 0xb,
> > +};
> > +
> > +/**
> > + * struct scmi_protocol_version_out - Response for SCMI_PROTOCOL_VERSION
> > + * command
> > + * @status:SCMI command status
> > + * @version:   Protocol version
> > + */
> > +struct scmi_protocol_version_out {
> > +   s32 status;
> > +   u32 version;
> > +};
> > +
> > +/**
> > + * struct scmi_protocol_attrs_out - Response for SCMI_PROTOCOL_ATTRIBUTES
> > + * command
> > + * @status:SCMI command status
> > + * @attributes:Protocol attributes or implementation details
> > + */
> > +struct scmi_protocol_attrs_out {
> > +   s32 status;
> > +   u32 attributes;
> > +};
> > +
> > +#define SCMI_PROTOCOL_ATTRS_NUM_AGENTS(attributes) \
> > +   (((attributes) & GENMASK(15, 8)) >> 8)
> > +#define SCMI_PROTOCOL_ATTRS_NUM_PROTOCOLS(attributes) \
> > +   ((attributes) & GENMASK(7, 0))
> > +
> > +/**
> > + * struct scmi_protocol_msg_attrs_out - Response for
> > + * SCMI_PROTOCOL_MESSAGE_ATTRIBUTES 
> > command
> > + * @status:SCMI command status
> > + * @attributes:Message-specific attributes
> > + */
> > +struct scmi_protocol_msg_attrs_out {
> > +   s32 status;
> > +   u32 attributes;
> > +};
> > +
> > +/**
> > + * struct scmi_base_discover_vendor_out - Response for
> > + *   SCMI_BASE_DISCOVER_VENDOR or
> > + *   SCMI_BASE_DISCOVER_SUB_VENDOR 
> > command
> > + * @status:SCMI command status
> > + * @vendor_identifier: Identifier of vendor or sub-vendor in string
> > + */
> > +struct 

Current way of using pinctrl-mvebu

[Chris Packham]

Hi,

I'm looking to upstream support for a new board with the Marvell AC5X
SoC and some NAND driver changes to support the SoC/board. I've got
things working when chain loading vendor based u-boot -> upstream
u-boot but when I boot directly the NAND controller reports
"pxa3xx-nand nand-controller@805b: Ready timeout!!!".

I think this is because the multi purpose pins are not in NAND/DEV
mode. When chainloading the vendor u-boot has already done this so the
driver works.

I notice that pinctrl-mvebu.c just expects a single fixed pin-func
property the covers all the functions required. How is the probe for
this supposed to be configured?

Thanks,
Chris

[PATCH 2/2] board: rockchip: Add Hardkernel ODROID-M1

[Jonas Karlman]

Hardkernel ODROID-M1 is a single board computer with a RK3568B2 SoC,
a slightly modified version of the RK3568 SoC.

Features tested on a ODROID-M1 8GB v1.0 2022-06-13:
- SD-card boot
- eMMC boot
- SPI Flash boot
- PCIe/NVMe/AHCI
- SATA port
- USB host

Device tree is imported from linux v6.4.

Signed-off-by: Jonas Karlman 
---
 arch/arm/dts/Makefile |   1 +
 arch/arm/dts/rk3568-odroid-m1-u-boot.dtsi |  46 ++
 arch/arm/dts/rk3568-odroid-m1.dts | 744 ++
 board/rockchip/evb_rk3568/MAINTAINERS |   7 +
 configs/odroid-m1-rk3568_defconfig| 103 +++
 doc/board/rockchip/rockchip.rst   |   1 +
 6 files changed, 902 insertions(+)
 create mode 100644 arch/arm/dts/rk3568-odroid-m1-u-boot.dtsi
 create mode 100644 arch/arm/dts/rk3568-odroid-m1.dts
 create mode 100644 configs/odroid-m1-rk3568_defconfig

diff --git a/arch/arm/dts/Makefile b/arch/arm/dts/Makefile
index 480269fa6065..334c1bafda17 100644
--- a/arch/arm/dts/Makefile
+++ b/arch/arm/dts/Makefile
@@ -169,6 +169,7 @@ dtb-$(CONFIG_ROCKCHIP_RK3568) += \
rk3566-anbernic-rgxx3.dtb \
rk3566-radxa-cm3-io.dtb \
rk3568-evb.dtb \
+   rk3568-odroid-m1.dtb \
rk3568-rock-3a.dtb
 
 dtb-$(CONFIG_ROCKCHIP_RK3588) += \
diff --git a/arch/arm/dts/rk3568-odroid-m1-u-boot.dtsi 
b/arch/arm/dts/rk3568-odroid-m1-u-boot.dtsi
new file mode 100644
index ..dc8ad98715ce
--- /dev/null
+++ b/arch/arm/dts/rk3568-odroid-m1-u-boot.dtsi
@@ -0,0 +1,46 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include "rk356x-u-boot.dtsi"
+
+/ {
+   aliases {
+   spi0 = 
+   };
+
+   chosen {
+   stdout-path = 
+   };
+};
+
+_dual_io_pins {
+   bootph-all;
+};
+
+ {
+   cap-mmc-highspeed;
+   mmc-ddr-1_8v;
+   mmc-hs200-1_8v;
+   mmc-hs400-1_8v;
+   mmc-hs400-enhanced-strobe;
+   pinctrl-0 = <_bus8 _clk _cmd _datastrobe>;
+};
+
+ {
+   bootph-pre-ram;
+   u-boot,spl-sfc-no-dma;
+
+   flash@0 {
+   bootph-pre-ram;
+   };
+};
+
+ {
+   bootph-all;
+   clock-frequency = <2400>;
+   status = "okay";
+};
+
+_usb_host {
+   /* Workaround until regulator implement basic reference counter */
+   regulator-always-on;
+};
diff --git a/arch/arm/dts/rk3568-odroid-m1.dts 
b/arch/arm/dts/rk3568-odroid-m1.dts
new file mode 100644
index ..59ecf868dbd0
--- /dev/null
+++ b/arch/arm/dts/rk3568-odroid-m1.dts
@@ -0,0 +1,744 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (c) 2022 Hardkernel Co., Ltd.
+ *
+ */
+
+/dts-v1/;
+#include 
+#include 
+#include 
+#include 
+#include "rk3568.dtsi"
+
+/ {
+   model = "Hardkernel ODROID-M1";
+   compatible = "rockchip,rk3568-odroid-m1", "rockchip,rk3568";
+
+   aliases {
+   ethernet0 = 
+   i2c0 = 
+   i2c3 = 
+   mmc0 = 
+   mmc1 = 
+   serial0 = 
+   serial1 = 
+   };
+
+   chosen {
+   stdout-path = "serial2:150n8";
+   };
+
+   dc_12v: dc-12v-regulator {
+   compatible = "regulator-fixed";
+   regulator-name = "dc_12v";
+   regulator-always-on;
+   regulator-boot-on;
+   regulator-min-microvolt = <1200>;
+   regulator-max-microvolt = <1200>;
+   };
+
+   hdmi-con {
+   compatible = "hdmi-connector";
+   type = "a";
+
+   port {
+   hdmi_con_in: endpoint {
+   remote-endpoint = <_out_con>;
+   };
+   };
+   };
+
+   ir-receiver {
+   compatible = "gpio-ir-receiver";
+   gpios = < RK_PC2 GPIO_ACTIVE_LOW>;
+   pinctrl-names = "default";
+   pinctrl-0 = <_receiver_pin>;
+   };
+
+   leds {
+   compatible = "gpio-leds";
+
+   led_power: led-0 {
+   gpios = < RK_PC6 GPIO_ACTIVE_HIGH>;
+   function = LED_FUNCTION_POWER;
+   color = ;
+   default-state = "keep";
+   linux,default-trigger = "default-on";
+   pinctrl-names = "default";
+   pinctrl-0 = <_power_pin>;
+   };
+   led_work: led-1 {
+   gpios = < RK_PB7 GPIO_ACTIVE_HIGH>;
+   function = LED_FUNCTION_HEARTBEAT;
+   color = ;
+   linux,default-trigger = "heartbeat";
+   pinctrl-names = "default";
+   pinctrl-0 = <_work_pin>;
+   };
+   };
+
+   rk809-sound {
+   compatible = "simple-audio-card";
+   pinctrl-names = "default";
+   pinctrl-0 = <_det_pin>;
+   simple-audio-card,name = "Analog RK817";
+   

[PATCH 1/2] ata: dwc_ahci: Fix support for other platforms

[Jonas Karlman]

The dwc_ahci driver use platform specific defines, place the platform
specific code behind a ifdef CONFIG_ARCH_OMAP2PLUS to allow build and
use of the driver on Rockchip platform.

Fixes: 02a4b4297901 ("drivers: block: dwc_ahci: Implement a driver for Synopsys 
DWC sata device")
Signed-off-by: Jonas Karlman 
---
 drivers/ata/dwc_ahci.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/ata/dwc_ahci.c b/drivers/ata/dwc_ahci.c
index 826fea71cc5b..1dc91e7fce70 100644
--- a/drivers/ata/dwc_ahci.c
+++ b/drivers/ata/dwc_ahci.c
@@ -13,7 +13,9 @@
 #include 
 #include 
 #include 
+#ifdef CONFIG_ARCH_OMAP2PLUS
 #include 
+#endif
 #include 
 #include 
 
@@ -72,12 +74,14 @@ static int dwc_ahci_probe(struct udevice *dev)
return ret;
}
 
+#ifdef CONFIG_ARCH_OMAP2PLUS
if (priv->wrapper_base) {
u32 val = TI_SATA_IDLE_NO | TI_SATA_STANDBY_NO;
 
/* Enable SATA module, No Idle, No Standby */
writel(val, priv->wrapper_base + TI_SATA_SYSCONFIG);
}
+#endif
 
return ahci_probe_scsi(dev, (ulong)priv->base);
 }
-- 
2.41.0

[PATCH 0/2] board: rockchip: Add Hardkernel ODROID-M1

[Jonas Karlman]

This series add support for Hardkernel ODROID-M1, a single board
computer with a RK3568B2 SoC.

First patch fixes a build issue in the dwc_ahci driver.

Second patch import the device tree from linux v6.4 and add a defconfig
for Hardkernel ODROID-M1.

Following was tested on a ODROID-M1 8GB v1.0 2022-06-13:
- SD-card boot
- eMMC boot
- SPI Flash boot
- PCIe/NVMe/AHCI
- SATA port
- USB host

This series have loose dependencies on the following series:
- rockchip: Fix PCIe and NVMe support on RK3568 [1]
- rockchip: rk3568: Use dwc3-generic driver [2]
- rockchip: rk3568: Fix alloc space exhausted in SPL [3]
- rockchip: rk3568: Device Tree updates [4]

[1] https://patchwork.ozlabs.org/project/uboot/list/?series=355486
[2] https://patchwork.ozlabs.org/project/uboot/list/?series=357200
[3] https://patchwork.ozlabs.org/project/uboot/list/?series=361999
[4] https://patchwork.ozlabs.org/project/uboot/list/?series=362030

Jonas Karlman (2):
  ata: dwc_ahci: Fix support for other platforms
  board: rockchip: Add Hardkernel ODROID-M1

 arch/arm/dts/Makefile |   1 +
 arch/arm/dts/rk3568-odroid-m1-u-boot.dtsi |  46 ++
 arch/arm/dts/rk3568-odroid-m1.dts | 744 ++
 board/rockchip/evb_rk3568/MAINTAINERS |   7 +
 configs/odroid-m1-rk3568_defconfig| 103 +++
 doc/board/rockchip/rockchip.rst   |   1 +
 drivers/ata/dwc_ahci.c|   4 +
 7 files changed, 906 insertions(+)
 create mode 100644 arch/arm/dts/rk3568-odroid-m1-u-boot.dtsi
 create mode 100644 arch/arm/dts/rk3568-odroid-m1.dts
 create mode 100644 configs/odroid-m1-rk3568_defconfig

-- 
2.41.0

Re: [PATCH] phy: phy-uclass: Add a missing error handling path

[Jonas Karlman]

On 2023-07-02 20:47, Bhupesh Sharma wrote:
> Since function 'phy_get_counts()' can return NULL,
> add handling for that error path inside callers of
> this function.

Do you have any example where this can/has happened?

Counts should never be NULL in a normal working call flow. I am a bit
worried that this patch may hide some other bug or use of an
uninitialized phy struct.

The phy struct is initialized in generic_phy_get_by_index_nodev. That
function should fail when counts cannot be allocated.

Maybe generic_phy_get_by_index_nodev should unset phy->dev if it fails,
or generic_phy_valid should be extended to also check phy->counts.
That way generic_phy_valid would return false and these functions
should return earlier before trying to use counts.

Regards,
Jonas

> 
> Signed-off-by: Bhupesh Sharma 
> ---
>  drivers/phy/phy-uclass.c | 20 
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/phy/phy-uclass.c b/drivers/phy/phy-uclass.c
> index 629ef3aa3d..c523a0b45e 100644
> --- a/drivers/phy/phy-uclass.c
> +++ b/drivers/phy/phy-uclass.c
> @@ -229,6 +229,11 @@ int generic_phy_init(struct phy *phy)
>   if (!generic_phy_valid(phy))
>   return 0;
>   counts = phy_get_counts(phy);
> + if (!counts) {
> + debug("phy_get_counts returns NULL\n");
> + return -ENODEV;
> + }
> +
>   if (counts->init_count > 0) {
>   counts->init_count++;
>   return 0;
> @@ -275,6 +280,11 @@ int generic_phy_exit(struct phy *phy)
>   if (!generic_phy_valid(phy))
>   return 0;
>   counts = phy_get_counts(phy);
> + if (!counts) {
> + debug("phy_get_counts returns NULL\n");
> + return -ENODEV;
> + }
> +
>   if (counts->init_count == 0)
>   return 0;
>   if (counts->init_count > 1) {
> @@ -305,6 +315,11 @@ int generic_phy_power_on(struct phy *phy)
>   if (!generic_phy_valid(phy))
>   return 0;
>   counts = phy_get_counts(phy);
> + if (!counts) {
> + debug("phy_get_counts returns NULL\n");
> + return -ENODEV;
> + }
> +
>   if (counts->power_on_count > 0) {
>   counts->power_on_count++;
>   return 0;
> @@ -341,6 +356,11 @@ int generic_phy_power_off(struct phy *phy)
>   if (!generic_phy_valid(phy))
>   return 0;
>   counts = phy_get_counts(phy);
> + if (!counts) {
> + debug("phy_get_counts returns NULL\n");
> + return -ENODEV;
> + }
> +
>   if (counts->power_on_count == 0)
>   return 0;
>   if (counts->power_on_count > 1) {

[PATCH] ufs: Use 'TASK_TAG' to construct the ucd_req_ptr->header.dword_0

[Bhupesh Sharma]

Instead of using the hard-coded value of 0x1f, use 'TASK_TAG'
macro instead to construct the ucd_req_ptr->header.dword_0

This is in sync with what the Linux UFS driver does, i.e.
set the byte0 equal to TASK_TAG (see [1]).

Setting it to a fixed value of 0x1f is wrong as we define
TASK_TAG as 0 inside u-boot ufs framework. So, instead we
should  use the macro value directly.

[1]. 
https://github.com/torvalds/linux/blob/master/drivers/ufs/core/ufshcd.c#L2705

Signed-off-by: Bhupesh Sharma 
---
 drivers/ufs/ufs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ufs/ufs.c b/drivers/ufs/ufs.c
index 8dd29edd3d..3bf1a95e7f 100644
--- a/drivers/ufs/ufs.c
+++ b/drivers/ufs/ufs.c
@@ -775,7 +775,7 @@ static inline void ufshcd_prepare_utp_nop_upiu(struct 
ufs_hba *hba)
 
/* command descriptor fields */
ucd_req_ptr->header.dword_0 =
-   UPIU_HEADER_DWORD(UPIU_TRANSACTION_NOP_OUT, 0, 0, 0x1f);
+   UPIU_HEADER_DWORD(UPIU_TRANSACTION_NOP_OUT, 0, 0, 
TASK_TAG);
/* clear rest of the fields of basic header */
ucd_req_ptr->header.dword_1 = 0;
ucd_req_ptr->header.dword_2 = 0;
-- 
2.38.1

[PATCH] phy: phy-uclass: Add a missing error handling path

[Bhupesh Sharma]

Since function 'phy_get_counts()' can return NULL,
add handling for that error path inside callers of
this function.

Signed-off-by: Bhupesh Sharma 
---
 drivers/phy/phy-uclass.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/drivers/phy/phy-uclass.c b/drivers/phy/phy-uclass.c
index 629ef3aa3d..c523a0b45e 100644
--- a/drivers/phy/phy-uclass.c
+++ b/drivers/phy/phy-uclass.c
@@ -229,6 +229,11 @@ int generic_phy_init(struct phy *phy)
if (!generic_phy_valid(phy))
return 0;
counts = phy_get_counts(phy);
+   if (!counts) {
+   debug("phy_get_counts returns NULL\n");
+   return -ENODEV;
+   }
+
if (counts->init_count > 0) {
counts->init_count++;
return 0;
@@ -275,6 +280,11 @@ int generic_phy_exit(struct phy *phy)
if (!generic_phy_valid(phy))
return 0;
counts = phy_get_counts(phy);
+   if (!counts) {
+   debug("phy_get_counts returns NULL\n");
+   return -ENODEV;
+   }
+
if (counts->init_count == 0)
return 0;
if (counts->init_count > 1) {
@@ -305,6 +315,11 @@ int generic_phy_power_on(struct phy *phy)
if (!generic_phy_valid(phy))
return 0;
counts = phy_get_counts(phy);
+   if (!counts) {
+   debug("phy_get_counts returns NULL\n");
+   return -ENODEV;
+   }
+
if (counts->power_on_count > 0) {
counts->power_on_count++;
return 0;
@@ -341,6 +356,11 @@ int generic_phy_power_off(struct phy *phy)
if (!generic_phy_valid(phy))
return 0;
counts = phy_get_counts(phy);
+   if (!counts) {
+   debug("phy_get_counts returns NULL\n");
+   return -ENODEV;
+   }
+
if (counts->power_on_count == 0)
return 0;
if (counts->power_on_count > 1) {
-- 
2.38.1

[PATCH 3/4] rockchip: rk356x-u-boot: Add bootph-all to common pinctrl nodes

[Jonas Karlman]

Add bootph-all prop to common pinctrl nodes for eMMC, FSPI, SD-card and
UART2 that are typically used by multiple boards. Unreferenced nodes are
removed from the SPL device tree during a normal build.

Signed-off-by: Jonas Karlman 
---
 arch/arm/dts/rk3566-radxa-cm3-io-u-boot.dtsi | 56 -
 arch/arm/dts/rk3568-rock-3a-u-boot.dtsi  | 54 -
 arch/arm/dts/rk356x-u-boot.dtsi  | 64 
 3 files changed, 64 insertions(+), 110 deletions(-)

diff --git a/arch/arm/dts/rk3566-radxa-cm3-io-u-boot.dtsi 
b/arch/arm/dts/rk3566-radxa-cm3-io-u-boot.dtsi
index 57b77151c57c..c925439f71cd 100644
--- a/arch/arm/dts/rk3566-radxa-cm3-io-u-boot.dtsi
+++ b/arch/arm/dts/rk3566-radxa-cm3-io-u-boot.dtsi
@@ -11,67 +11,11 @@
};
 };
 
-_bus8 {
-   bootph-all;
-};
-
-_clk {
-   bootph-all;
-};
-
-_cmd {
-   bootph-all;
-};
-
-_datastrobe {
-   bootph-all;
-};
-
- {
-   bootph-all;
-};
-
-_pull_none {
-   bootph-all;
-};
-
-_pull_up_drv_level_2 {
-   bootph-all;
-};
-
-_pull_up {
-   bootph-all;
-};
-
-_bus4 {
-   bootph-all;
-};
-
-_clk {
-   bootph-all;
-};
-
-_cmd {
-   bootph-all;
-};
-
-_det {
-   bootph-all;
-};
-
-_pwren {
-   bootph-all;
-};
-
  {
cap-mmc-highspeed;
mmc-ddr-1_8v;
 };
 
-_xfer {
-   bootph-all;
-};
-
  {
clock-frequency = <2400>;
bootph-all;
diff --git a/arch/arm/dts/rk3568-rock-3a-u-boot.dtsi 
b/arch/arm/dts/rk3568-rock-3a-u-boot.dtsi
index 0ba38851c25e..45e9390f202d 100644
--- a/arch/arm/dts/rk3568-rock-3a-u-boot.dtsi
+++ b/arch/arm/dts/rk3568-rock-3a-u-boot.dtsi
@@ -16,26 +16,6 @@
};
 };
 
-_bus8 {
-   bootph-all;
-};
-
-_clk {
-   bootph-all;
-};
-
-_cmd {
-   bootph-all;
-};
-
-_datastrobe {
-   bootph-all;
-};
-
-_pins {
-   bootph-all;
-};
-
  {
pinctrl-0 = <_pins _reset_h>;
/* Shared vpcie3v3-supply may cause a sys freeze, disable for now */
@@ -47,8 +27,6 @@
 };
 
  {
-   bootph-all;
-
pcie {
pcie3x2_reset_h: pcie3x2-reset-h {
rockchip,pins = <2 RK_PD6 RK_FUNC_GPIO _pull_none>;
@@ -56,34 +34,6 @@
};
 };
 
-_pull_none {
-   bootph-all;
-};
-
-_pull_up_drv_level_2 {
-   bootph-all;
-};
-
-_pull_up {
-   bootph-all;
-};
-
-_bus4 {
-   bootph-all;
-};
-
-_clk {
-   bootph-all;
-};
-
-_cmd {
-   bootph-all;
-};
-
-_det {
-   bootph-all;
-};
-
  {
cap-mmc-highspeed;
mmc-ddr-1_8v;
@@ -117,10 +67,6 @@
status = "disabled";
 };
 
-_xfer {
-   bootph-all;
-};
-
  {
clock-frequency = <2400>;
bootph-all;
diff --git a/arch/arm/dts/rk356x-u-boot.dtsi b/arch/arm/dts/rk356x-u-boot.dtsi
index c340c2bba6ff..89c0d830b632 100644
--- a/arch/arm/dts/rk356x-u-boot.dtsi
+++ b/arch/arm/dts/rk356x-u-boot.dtsi
@@ -59,6 +59,70 @@
status = "okay";
 };
 
+ {
+   bootph-all;
+};
+
+_pull_none {
+   bootph-all;
+};
+
+_pull_up_drv_level_2 {
+   bootph-all;
+};
+
+_pull_up {
+   bootph-all;
+};
+
+_bus8 {
+   bootph-all;
+};
+
+_clk {
+   bootph-all;
+};
+
+_cmd {
+   bootph-all;
+};
+
+_datastrobe {
+   bootph-all;
+};
+
+_rstnout {
+   bootph-all;
+};
+
+_pins {
+   bootph-all;
+};
+
+_bus4 {
+   bootph-all;
+};
+
+_clk {
+   bootph-all;
+};
+
+_cmd {
+   bootph-all;
+};
+
+_det {
+   bootph-all;
+};
+
+_pwren {
+   bootph-all;
+};
+
+_xfer {
+   bootph-all;
+};
+
  {
bootph-pre-ram;
status = "okay";
-- 
2.41.0

[PATCH 4/4] rockchip: rk356x-u-boot: Use relaxed u-boot,spl-boot-order

[Jonas Karlman]

BootRom will try to load TPL+SPL from media in the following order:
- SPI NOR Flash
- SPI NAND Flash
- NAND Flash
- eMMC
- SDMMC

SPL will try to load FIT from media in the order defined in the device
tree u-boot,spl-boot-order property.

Change the default order to load FIT from to:
- same media as TPL+SPL
- SDMMC
- eMMC

Boards with strict load order requirements should override the
u-boot,spl-boot-order property in the board specific u-boot.dtsi.

Fixes: 42f67fb51cb4 ("rockchip: rk3568: Fix boot device detection")
Signed-off-by: Jonas Karlman 
---
 arch/arm/dts/rk356x-u-boot.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/dts/rk356x-u-boot.dtsi b/arch/arm/dts/rk356x-u-boot.dtsi
index 89c0d830b632..5644f78ec774 100644
--- a/arch/arm/dts/rk356x-u-boot.dtsi
+++ b/arch/arm/dts/rk356x-u-boot.dtsi
@@ -12,7 +12,7 @@
};
 
chosen {
-   u-boot,spl-boot-order = "same-as-spl", , 
+   u-boot,spl-boot-order = "same-as-spl", , 
};
 
dmc: dmc {
-- 
2.41.0

[PATCH 2/4] rockchip: rk3566-radxa-cm3-io: Sync dts from linux v6.4

[Jonas Karlman]

Sync rk3566-radxa-cm3-io.dts from linux v6.4.

Signed-off-by: Jonas Karlman 
---
 arch/arm/dts/rk3566-radxa-cm3-io.dts | 8 
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/dts/rk3566-radxa-cm3-io.dts 
b/arch/arm/dts/rk3566-radxa-cm3-io.dts
index d89d5263cb5e..5e4236af4fcb 100644
--- a/arch/arm/dts/rk3566-radxa-cm3-io.dts
+++ b/arch/arm/dts/rk3566-radxa-cm3-io.dts
@@ -254,6 +254,14 @@
status = "okay";
 };
 
+_otg {
+   status = "okay";
+};
+
+_host0_xhci {
+   status = "okay";
+};
+
  {
assigned-clocks = < DCLK_VOP0>, < DCLK_VOP1>;
assigned-clock-parents = < PLL_HPLL>, < PLL_VPLL>;
-- 
2.41.0

[PATCH 1/4] rockchip: rk356x: Sync dtsi from linux v6.4

[Jonas Karlman]

Sync rk356x.dtsi from linux v6.4.

Signed-off-by: Jonas Karlman 
---
 arch/arm/dts/rk3568-pinctrl.dtsi | 94 
 arch/arm/dts/rk356x.dtsi | 14 +++--
 2 files changed, 102 insertions(+), 6 deletions(-)

diff --git a/arch/arm/dts/rk3568-pinctrl.dtsi b/arch/arm/dts/rk3568-pinctrl.dtsi
index 8f90c66dd9e9..0a979bfb63d9 100644
--- a/arch/arm/dts/rk3568-pinctrl.dtsi
+++ b/arch/arm/dts/rk3568-pinctrl.dtsi
@@ -3117,4 +3117,98 @@
<0 RK_PA1 0 _pull_none>;
};
};
+
+   lcdc {
+   /omit-if-no-ref/
+   lcdc_clock: lcdc-clock {
+   rockchip,pins =
+   /* lcdc_clk */
+   <3 RK_PA0 1 _pull_none>,
+   /* lcdc_den */
+   <3 RK_PC3 1 _pull_none>,
+   /* lcdc_hsync */
+   <3 RK_PC1 1 _pull_none>,
+   /* lcdc_vsync */
+   <3 RK_PC2 1 _pull_none>;
+   };
+
+   /omit-if-no-ref/
+   lcdc_data16: lcdc-data16 {
+   rockchip,pins =
+   /* lcdc_d3 */
+   <2 RK_PD3 1 _pull_none>,
+   /* lcdc_d4 */
+   <2 RK_PD4 1 _pull_none>,
+   /* lcdc_d5 */
+   <2 RK_PD5 1 _pull_none>,
+   /* lcdc_d6 */
+   <2 RK_PD6 1 _pull_none>,
+   /* lcdc_d7 */
+   <2 RK_PD7 1 _pull_none>,
+   /* lcdc_d10 */
+   <3 RK_PA3 1 _pull_none>,
+   /* lcdc_d11 */
+   <3 RK_PA4 1 _pull_none>,
+   /* lcdc_d12 */
+   <3 RK_PA5 1 _pull_none>,
+   /* lcdc_d13 */
+   <3 RK_PA6 1 _pull_none>,
+   /* lcdc_d14 */
+   <3 RK_PA7 1 _pull_none>,
+   /* lcdc_d15 */
+   <3 RK_PB0 1 _pull_none>,
+   /* lcdc_d19 */
+   <3 RK_PB4 1 _pull_none>,
+   /* lcdc_d20 */
+   <3 RK_PB5 1 _pull_none>,
+   /* lcdc_d21 */
+   <3 RK_PB6 1 _pull_none>,
+   /* lcdc_d22 */
+   <3 RK_PB7 1 _pull_none>,
+   /* lcdc_d23 */
+   <3 RK_PC0 1 _pull_none>;
+   };
+
+   /omit-if-no-ref/
+   lcdc_data18: lcdc-data18 {
+   rockchip,pins =
+   /* lcdc_d2 */
+   <2 RK_PD2 1 _pull_none>,
+   /* lcdc_d3 */
+   <2 RK_PD3 1 _pull_none>,
+   /* lcdc_d4 */
+   <2 RK_PD4 1 _pull_none>,
+   /* lcdc_d5 */
+   <2 RK_PD5 1 _pull_none>,
+   /* lcdc_d6 */
+   <2 RK_PD6 1 _pull_none>,
+   /* lcdc_d7 */
+   <2 RK_PD7 1 _pull_none>,
+   /* lcdc_d10 */
+   <3 RK_PA3 1 _pull_none>,
+   /* lcdc_d11 */
+   <3 RK_PA4 1 _pull_none>,
+   /* lcdc_d12 */
+   <3 RK_PA5 1 _pull_none>,
+   /* lcdc_d13 */
+   <3 RK_PA6 1 _pull_none>,
+   /* lcdc_d14 */
+   <3 RK_PA7 1 _pull_none>,
+   /* lcdc_d15 */
+   <3 RK_PB0 1 _pull_none>,
+   /* lcdc_d18 */
+   <3 RK_PB3 1 _pull_none>,
+   /* lcdc_d19 */
+   <3 RK_PB4 1 _pull_none>,
+   /* lcdc_d20 */
+   <3 RK_PB5 1 _pull_none>,
+   /* lcdc_d21 */
+   <3 RK_PB6 1 _pull_none>,
+   /* lcdc_d22 */
+   <3 RK_PB7 1 _pull_none>,
+   /* lcdc_d23 */
+   <3 RK_PC0 1 _pull_none>;
+   };
+   };
+
 };
diff --git a/arch/arm/dts/rk356x.dtsi b/arch/arm/dts/rk356x.dtsi

[PATCH 0/4] rockchip: rk3568: Device Tree updates

[Jonas Karlman]

This series sync rk356x.dtsi and rk3566-radxa-cm3-io.dts from linux
v6.4, add bootph-all to common pinctrl nodes and relax FIT load order
in rk356x-u-boot.dtsi.

Patch 1-2 sync rk356x device tree from linux v6.4.
Patch 3 move common pinctrl nodes to rk356x-u-boot.dtsi.
Patch 4 change to try and load FIT from SD-card before eMMC when loading
FIT from TPL+SPL media fails.

Jonas Karlman (4):
  rockchip: rk356x: Sync dtsi from linux v6.4
  rockchip: rk3566-radxa-cm3-io: Sync dts from linux v6.4
  rockchip: rk356x-u-boot: Add bootph-all to common pinctrl nodes
  rockchip: rk356x-u-boot: Use relaxed u-boot,spl-boot-order

 arch/arm/dts/rk3566-radxa-cm3-io-u-boot.dtsi | 56 
 arch/arm/dts/rk3566-radxa-cm3-io.dts |  8 ++
 arch/arm/dts/rk3568-pinctrl.dtsi | 94 
 arch/arm/dts/rk3568-rock-3a-u-boot.dtsi  | 54 ---
 arch/arm/dts/rk356x-u-boot.dtsi  | 66 +-
 arch/arm/dts/rk356x.dtsi | 14 +--
 6 files changed, 175 insertions(+), 117 deletions(-)

-- 
2.41.0

Re: [PATCH v2 2/3] X86: Add support for distro boot

[Simon Glass]

Hi Thomas,

On Wed, 10 May 2023 at 07:40, Mittelstaedt Thomas (XC-CT/EBV3)
 wrote:
>
> Hello Simon,
>
> I've integrated your patches (1-3-usb-Tidy-up-the-usb_start-flag.patch, 
> bootstd-Correct-default-boot-command.patch)
>
> The system with CONFIG_BOOTSTD_DEFAULT instead of CONFIG_BOOTSTD_FULL doesn't 
> start like before with boot script. The extlinux configuration still works.
>
> === Bootlog at scenario with boot script 
> =
> U-Boot 2023.04-01138-g210b6bbad7-dirty (May 09 2023 - 09:26:08 +)
>
> CPU: x86_64, vendor Intel, device a0652h
> DRAM:  8 GiB
> Core:  12 devices, 11 uclasses, devicetree: separate
> MMC:
> Loading Environment from FAT... scanning bus for devices...
> Target spinup took 0 ms.
> AHCI 0001.0100 32 slots 1 ports 3 Gbps 0x1 impl SATA mode
> flags: 64bit ncq stag only ccc
>   Device 0: (0:0) Vendor: ATA Prod.: VBOX HARDDISK Rev: 1.0
> Type: Hard Disk
> Capacity: 32768.0 MB = 32.0 GB (67108864 x 512)
> OK
> No video mode configured in EFI!
> No video mode configured in EFI!
> Model: EFI x86 Payload
> Net:   e1000: 08:00:27:1c:36:6f
>
> Warning: e1000#0 failed to set MAC address
> eth0: e1000#0
> Bus xhci_pci: Register e000820 NbrPorts 14
> Starting the controller
> USB XHCI 1.00
> scanning bus xhci_pci for devices... 2 USB Device(s) found
> Hit any key to stop autoboot:  0
> scanning bus for devices...
>   Device 0: (0:0) Vendor: ATA Prod.: VBOX HARDDISK Rev: 1.0
> Type: Hard Disk
> Capacity: 32768.0 MB = 32.0 GB (67108864 x 512)
> Bus 0: OK Bus 1: not available
>   Device 0: Model: VBOX Firm: 1.0 Ser#: CD-ROM
> Type: Removable CD ROM
> Capacity: 50.5 MB = 0.0 GB (25902 x 2048)
>   Device 1: not available
> Error (no IRQ) dev 0 blk 0: status 0x11
> Error (no IRQ) dev 0 blk 0: status 0x11
> Error (no IRQ) dev 0 blk 10: status 0x11
> Error (no IRQ) dev 0 blk 0: status 0x11
> =>
>
> ===
>
> Mit freundlichen Grüßen / Best regards
>
> Thomas Mittelstaedt
>
> Cross-Domain Computing Solutions
>
> > -Ursprüngliche Nachricht-
> > Von: Simon Glass 
> > Gesendet: Montag, 8. Mai 2023 23:23
> > An: Mittelstaedt Thomas (XC-CT/EBV3) 
> > Cc: Heinrich Schuchardt ; u-boot@lists.denx.de; Niel
> > Armstrong ; Patrick Delaunay
> > ; Ramon Fried ; Marek
> > Vasut ; Manuel Traut ; Bin Meng
> > 
> > Betreff: Re: [PATCH v2 2/3] X86: Add support for distro boot
> >
> > Hi,
> >
> > On Fri, 5 May 2023 at 02:23, Mittelstaedt Thomas (XC-CT/EBV3)
> >  wrote:
> > >
> > > Hi Simon,
> > >
> > > U-Boot can't start with boot script boot.scr, if BOOTSTD_DEFAULT is set. 
> > > With
> > this setting "bootflow scan -lb" is not supported.
> > > Booting with extlinux.conf works somehow.
> > >
> > > = Bootlog
> > ===
> > > U-Boot 2023.04-01135-g4ea415461d-dirty (May 05 2023 - 05:56:36 +)
> > >
> > > CPU: x86_64, vendor Intel, device a0652h
> > > DRAM:  8 GiB
> > > Core:  12 devices, 11 uclasses, devicetree: separate
> > > MMC:
> > > Loading Environment from FAT... scanning bus for devices...
> > > Target spinup took 0 ms.
> > > AHCI 0001.0100 32 slots 1 ports 3 Gbps 0x1 impl SATA mode
> > > flags: 64bit ncq stag only ccc
> > >   Device 0: (0:0) Vendor: ATA Prod.: VBOX HARDDISK Rev: 1.0
> > > Type: Hard Disk
> > > Capacity: 32768.0 MB = 32.0 GB (67108864 x 512) Unable to
> > > read "uboot.env" from scsi0:1...
> > > No video mode configured in EFI!
> > > No video mode configured in EFI!
> > > Model: EFI x86 Payload
> > > Net:   e1000: 08:00:27:1c:36:6f
> > >
> > > Warning: e1000#0 failed to set MAC address
> > > eth0: e1000#0
> > > Bus xhci_pci: Register e000820 NbrPorts 14 Starting the controller USB
> > > XHCI 1.00 scanning bus xhci_pci for devices... 2 USB Device(s) found
> > > Hit any key to stop autoboot:  0 scanning bus for devices...
> > >   Device 0: (0:0) Vendor: ATA Prod.: VBOX HARDDISK Rev: 1.0
> > > Type: Hard Disk
> > > Capacity: 32768.0 MB = 32.0 GB (67108864 x 512) Bus 0: OK
> > > Bus 1: not available
> > >   Device 0: Model: VBOX Firm: 1.0 Ser#: CD-ROM
> > > Type: Removable CD ROM
> > > Capacity: 50.5 MB = 0.0 GB (25902 x 2048)
> > >   Device 1: not available
> > > Error (no IRQ) dev 0 blk 0: status 0x11 Error (no IRQ) dev 0 blk 0:
> > > status 0x11 Error (no IRQ) dev 0 blk 10: status 0x11 Error (no IRQ)
> > > dev 0 blk 0: status 0x11 Bus xhci_pci: scanning bus xhci_pci for
> > > devices... Device not responding to set address.
> > >
> > >   USB device not accepting new address (error=8000) failed,
> > > error -71
> > >
> > > => printenv bootcmd
> > > bootcmd=bootflow scan
> > > => bootflow scan -lb
> > > Flags not supported: enable CONFIG_BOOTFLOW_FULL bootflow - Boot flows
> > >
> > > Usage:
> > > bootflow scan - boot first available bootflow
> > >
> > >
> > 

Re: modified UBoot to include I2C communication

[Simon Glass]

Hi Andy,

On Thu, 25 May 2023 at 13:34, Andy Goh  wrote:
>
> Dear Sir,
> I am working on a project based on NXP's iMX8M mini board, to include I2C 
> library in the UBoot , I had browse thru UBoot website below, could anyone 
> kindly advise which source file to amend and what are the code to include for 
> I2C? like CONFIG_SYS_NUM_I2C_BUSES etc... is the modification to be done on 
> Yocto project?
> Any help on source code or documentation are welcome, thanks.

U-Boot supports I2C, so long as your board has a devicetree with it
enabled, etc. You can find examples that use this, e.g. firefly-rk3288

The 'SYS_NUM_I2C_BUSES' thing is not used for modern boards.

Regards,
Simon


>
> https://source.denx.de/u-boot/u-boot/tree/master/
>
> Cheers,
> Andy
> andy@stengg.com
>
> This email is confidential and may also be privileged. If this email has been 
> sent to you in error, please delete it immediately and notify us. Please do 
> not copy, distribute, or disseminate part or whole of this email if you are 
> not the intended recipient or if you have not been authorized to do so. We 
> reserve the right, to the extent and under circumstances permitted by 
> applicable laws, to retain, monitor, and intercept email messages to and from 
> our systems. Thank you.

Re: [PATCH 10/12] binman: btool: Add Xilinx Bootgen btool

[Simon Glass]

Hi Lukas,

On Fri, 30 Jun 2023 at 13:28, Lukas Funke
 wrote:
>
> On 30.06.2023 06:18, Simon Glass wrote:
> > Hi,
> >
> > On Thu, 29 Jun 2023 at 15:59,  wrote:
> >>
> >> From: Lukas Funke 
> >>
> >> Add the Xilinx Bootgen as bintool. Xilinx Bootgen is used to create
> >> bootable SPL (FSBL in Xilinx terms) images for Zynq/ZynqMP devices. The
> >> btool creates a signed version of the SPL.
> >>
> >> Signed-off-by: Lukas Funke 
> >> ---
> >>
> >>   tools/binman/btool/bootgen.py | 82 +++
> >>   1 file changed, 82 insertions(+)
> >>   create mode 100644 tools/binman/btool/bootgen.py
> >>
> >> diff --git a/tools/binman/btool/bootgen.py b/tools/binman/btool/bootgen.py
> >> new file mode 100644
> >> index 00..8bc727a54f
> >> --- /dev/null
> >> +++ b/tools/binman/btool/bootgen.py
> >> @@ -0,0 +1,82 @@
> >> +# SPDX-License-Identifier: GPL-2.0+
> >> +# Copyright (C) 2023 Weidmüller Interface GmbH & Co. KG
> >> +# Lukas Funke 
> >> +#
> >> +"""Bintool implementation for bootgen
> >> +
> >> +bootgen allows creating bootable SPL for Zynq(MP)
> >> +
> >> +Documentation is available via::
> >> +https://www.xilinx.com/support/documents/sw_manuals/xilinx2022_1/ug1283-bootgen-user-guide.pdf
> >
> > But you need to create some info here. It is good to have that link,
> > but we also need some about of info about it here.
> >
> > Overall this whole patch needs more docs and detail.
> >
> >> +
> >> +Source code is available at:
> >> +
> >> +https://github.com/Xilinx/bootgen
> >> +
> >> +"""
> >> +import tempfile
> >> +
> >> +from binman import bintool
> >> +from u_boot_pylib import tools
> >> +
> >> +# pylint: disable=C0103
> >> +class Bintoolbootgen(bintool.Bintool):
> >> +"""Generate bootable fsbl image for zynq/zynqmp
> >> +
> >> +This bintools supports running Xilinx "bootgen" in order
> >> +to generate a bootable, authenticated image form an SPL.
> >> +
> >> +"""
> >> +def __init__(self, name):
> >> +super().__init__(name, 'Xilinx Bootgen',
> >> + version_regex=r'^\*\*\*\*\*\* Xilinx Bootgen',
> >> + version_args='')
> >> +
> >> +# pylint: disable=R0913
> >> +def sign(self, arch, spl_elf_fname, pmufw_elf_fname,
> >> + psk_fname, ssk_fname, fsbl_config, auth_params, 
> >> output_fname):
> >> +""" Sign FSBL(SPL) elf file and bundle it with pmu firmware
> >> +to a bootable image
> >> +
> >> +Args:
> >> +arch (str): Xilinx SoC architecture
> >
> > what options are valid?
> >
> >> +spl_elf_fname (str): Filename of FSBL ELF file
> >
> > what is FSBL?
> >
> >> +pmufw_elf_fname (str): Filename pmu firmware
> >
> > what is pmu?
> >
> >> +psk_fname (str): Filename of the primary secret key
> >> +ssk_fname (str): Filename of the secondary secret key
> >
> > why are there two keys? What format is used for these keys?
> >
> >> +fsbl_config (str): FSBL config options
> >
> > what options are available? What are valid vaulues for this arg?
> >
> >> +auth_params (str): Authentication parameter
> >
> > what are valid values?
> >
> >> +output_fname (str): Filename where bootgen should write the 
> >> result
> >
> > This should really be handled automatically, I think. See how the
> > mkimage etype creates an output filename.
>
> Simon, thanks for the review!
>
> What to you mean by automatically? The mkimage btool also has an
> 'output_fname' argument as well. And the output filename is passed down
> from the mkimage etype to the mkimage btool. Should the bootgen btool
> generate the output_fname by itself and pass it back to the caller?

Oh I think I was getting confused with the 'filename' property used by
the mkimage entry type. What you are doing here looks fine to me.

>
> >
> >> +"""
> >> +
> >> +_fsbl_config = f"[fsbl_config] {fsbl_config}" if fsbl_config else 
> >> ""
> >> +_auth_params = f"[auth_params] {auth_params}" if auth_params else 
> >> ""
> >> +
> >> +bif_template = f"""u_boot_spl_aes_rsa: {{
> >> +[pskfile] {psk_fname}
> >> +[sskfile] {ssk_fname}
> >> +{_fsbl_config}
> >> +{_auth_params}
> >> +[ bootloader,
> >> +  authentication = rsa,
> >> +  destination_cpu=a53-0] {spl_elf_fname}
> >> +[pmufw_image] {pmufw_elf_fname}
> >> +}}"""
> >> +args = ["-arch", arch]
> >> +
> >> +with tempfile.NamedTemporaryFile(suffix=".bif",
> >> + dir=tools.get_output_dir()) as 
> >> bif:
> >
> > Please use a deterministic name - see mkimage etype for an example.
>
> The .bif file is a temporary input file passed to 'bootgen'.
>
> My intention was to make sure that the file is deleted afterwards. Would
> you prefer that the intermediate files are kept and the output file is
> deterministically 

Re: [PATCH v13 04/10] arm_ffa: introduce Arm FF-A support

[Simon Glass]

Hi Abdellatif,

On Fri, 30 Jun 2023 at 13:49, Abdellatif El Khlifi
 wrote:
>
> Hi Simon,
>
> On Tue, Jun 20, 2023 at 11:27:20AM +0100, Simon Glass wrote:
> >  wrote:
> > >
> > > Add Arm FF-A support implementing Arm Firmware Framework for Armv8-A v1.0
> > >
> > > The Firmware Framework for Arm A-profile processors (FF-A v1.0) [1]
> > > describes interfaces (ABIs) that standardize communication
> > > between the Secure World and Normal World leveraging TrustZone
> > > technology.
> > >
> > > This driver uses 64-bit registers as per SMCCCv1.2 spec and comes
> > > on top of the SMCCC layer. The driver provides the FF-A ABIs needed for
> > > querying the FF-A framework from the secure world.
> > >
> > > The driver uses SMC32 calling convention which means using the first
> > > 32-bit data of the Xn registers.
> > >
> > > All supported ABIs come with their 32-bit version except FFA_RXTX_MAP
> > > which has 64-bit version supported.
> > >
> > > Both 32-bit and 64-bit direct messaging are supported which allows both
> > > 32-bit and 64-bit clients to use the FF-A bus.
> > >
> > > FF-A is a discoverable bus and similar to architecture features.
> > > FF-A bus is discovered using ARM_SMCCC_FEATURES mechanism performed
> > > by the PSCI driver.
> > >
> > > Clients are able to probe then use the FF-A bus by calling the DM class
> > > searching APIs (e.g: uclass_first_device).
> > >
> > > The Secure World is considered as one entity to communicate with
> > > using the FF-A bus. FF-A communication is handled by one device and
> > > one instance (the bus). This FF-A driver takes care of all the
> > > interactions between Normal world and Secure World.
> > >
> > > The driver exports its operations to be used by upper layers.
> > >
> > > Exported operations:
> > >
> > > - ffa_partition_info_get
> > > - ffa_sync_send_receive
> > > - ffa_rxtx_unmap
> > >
> > > Generic FF-A methods are implemented in the Uclass (arm-ffa-uclass.c).
> > > Arm specific methods are implemented in the Arm driver (arm-ffa.c).
> > >
> > > For more details please refer to the driver documentation [2].
> > >
> > > [1]: https://developer.arm.com/documentation/den0077/latest/
> > > [2]: doc/arch/arm64.ffa.rst
> > >
> > > Signed-off-by: Abdellatif El Khlifi 
> > > Cc: Tom Rini 
> > > Cc: Simon Glass 
> > > Cc: Ilias Apalodimas 
> > > Cc: Jens Wiklander 
> > > Cc: Heinrich Schuchardt 
> > >
> > > ---
> > >
> > > Changelog:
> > > ===
> > >
> > > v13:
> > >
> > > * doc minor change: specify in the readme that the user
> > >should call ffa_rxtx_unmap() driver operation to unmap
> > >the RX/TX buffers on demand.
>
> Are you happy with this commit please ? May I add a Reviewed-by ?

Sorry I think I did something wrong on the previous reply.

Reviewed-by: Simon Glass 

Regards,
Simon

Re: [PATCH 09/10] binman: Add support for u-boot-nodtb.bin.lzma as an input binary

[Simon Glass]

Hi Manoj,

On Fri, 30 Jun 2023 at 13:12, Manoj Sai
 wrote:
>
> Add an entry type for u-boot-nodtb.bin.lzma, which is a LZMA compressed
> raw u-boot binary and a simple test.
>
> Signed-off-by: Manoj Sai 
> Signed-off-by: Suniel Mahesh 
> ---
>  tools/binman/etype/283_u_boot_nodtb_lzma.dts | 11 
>  tools/binman/etype/u_boot_nodtb_lzma.py  | 28 
>  tools/binman/ftest.py|  6 +
>  3 files changed, 45 insertions(+)
>  create mode 100644 tools/binman/etype/283_u_boot_nodtb_lzma.dts
>  create mode 100644 tools/binman/etype/u_boot_nodtb_lzma.py

The same comment applies to this patch.

>
> diff --git a/tools/binman/etype/283_u_boot_nodtb_lzma.dts 
> b/tools/binman/etype/283_u_boot_nodtb_lzma.dts
> new file mode 100644
> index 00..d9a834acf6
> --- /dev/null
> +++ b/tools/binman/etype/283_u_boot_nodtb_lzma.dts
> @@ -0,0 +1,11 @@
> +/dts-v1/;
> +
> +/ {
> +   #address-cells = <1>;
> +   #size-cells = <1>;
> +
> +   binman {
> +   u-boot-nodtb-bin-lzma {
> +   };
> +   };
> +};
> diff --git a/tools/binman/etype/u_boot_nodtb_lzma.py 
> b/tools/binman/etype/u_boot_nodtb_lzma.py
> new file mode 100644
> index 00..c2874aa6e9
> --- /dev/null
> +++ b/tools/binman/etype/u_boot_nodtb_lzma.py
> @@ -0,0 +1,28 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +# Copyright (c) 2023 Amarula Solutions India
> +# Written by Suniel Mahesh 
> +# Reference from Simon Glass 
> +#
> +# Entry-type module for 'u-boot-nodtb.bin.lzma'
> +#
> +
> +from binman.entry import Entry
> +from binman.etype.blob import Entry_blob
> +
> +class Entry_u_boot_nodtb_lzma(Entry_blob):
> +"""U-Boot compressed flat binary without device tree appended
> +
> +Properties / Entry arguments:
> +- filename: Filename to include ('u-boot-nodtb.bin.lzma')
> +
> +This is the U-Boot compressed raw binary, before allowing it to relocate
> +itself at runtime it should be decompressed. It does not include a device
> +tree blob at the end of it so normally cannot work without it. You can 
> add a
> +u-boot-dtb entry after this one, or use a u-boot entry instead, normally
> +expands to a section containing u-boot and u-boot-dtb
> +"""
> +def __init__(self, section, etype, node):
> +super().__init__(section, etype, node)
> +
> +def GetDefaultFilename(self):
> +return 'u-boot-nodtb.bin.lzma'
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index 933ebcbd35..d1715523d2 100644
> --- a/tools/binman/ftest.py
> +++ b/tools/binman/ftest.py
> @@ -43,6 +43,7 @@ from u_boot_pylib import tout
>  U_BOOT_DATA   = b'1234'
>  U_BOOT_IMG_DATA   = b'img'
>  U_BOOT_NODTB_GZ_DATA  = b'uboot nodtb gz'
> +U_BOOT_NODTB_LZMA_DATA  = b'uboot nodtb lzma'
>  U_BOOT_SPL_DATA   = b'56780123456789abcdefghijklm'
>  U_BOOT_TPL_DATA   = b'tpl9876543210fedcbazywvuts'
>  U_BOOT_VPL_DATA   = b'vpl76543210fedcbazywxyz_'
> @@ -6682,5 +6683,10 @@ fdt fdtmapExtract the 
> devicetree blob from the fdtmap
> data = self._DoReadFile('279_u_boot_nodtb_gzip.dts')
> self.assertEqual(U_BOOT_NODTB_GZ_DATA, data)
>
> +   def testUBootnodtbBinLzma(self):
> +   """Test that u-boot-nodtb.bin.lzma can be put in a file"""
> +   data = self._DoReadFile('280_u_boot_nodtb_lzma.dts')
> +   self.assertEqual(U_BOOT_NODTB_LZMA_DATA, data)
> +
>  if __name__ == "__main__":
>  unittest.main()
> --
> 2.25.1
>

Regards,
Simon

Re: [PATCH 08/10] rockchip: Add GZIP compressed uboot raw binary to FIT image

[Simon Glass]

Hi Manoj,

On Fri, 30 Jun 2023 at 13:12, Manoj Sai
 wrote:
>
> Add GZIP compressed uboot raw binary to FIT image.
>
> Signed-off-by: Manoj Sai 
> Signed-off-by: Suniel Mahesh 
> ---
>  arch/arm/dts/rockchip-u-boot.dtsi | 10 ++
>  1 file changed, 10 insertions(+)
>
> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi 
> b/arch/arm/dts/rockchip-u-boot.dtsi
> index 2878b80926..6e95738923 100644
> --- a/arch/arm/dts/rockchip-u-boot.dtsi
> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> @@ -48,11 +48,21 @@
> type = "standalone";
> os = "U-Boot";
> arch = "arm64";
> +#if defined(CONFIG_SPL_GZIP)
> +   compression = "gzip";
> +#else
> compression = "none";
> +#endif
> load = ;
> entry = ;
> +#if defined(CONFIG_SPL_GZIP)
> +   u-boot-nodtb-gzip {
> +   };
> +#else
> u-boot-nodtb {

You should be able to do:

+#if defined(CONFIG_SPL_GZIP)
   compress = "gzip";
#endif

> };
> +#endif
> +
>  #ifdef CONFIG_SPL_FIT_SIGNATURE
> hash {
> algo = "sha256";
> --
> 2.25.1
>

Regards,
Simon

Re: [PATCH 07/10] binman: Add support for u-boot-nodtb.bin.gz as an input binary

[Simon Glass]

Hi Manoj,

On Fri, 30 Jun 2023 at 13:12, Manoj Sai
 wrote:
>
> Add an entry type for u-boot-nodtb.bin.gz, which is a GZIP compressed raw 
> u-boot
> binary and a simple test.
>

A better way to do this is to have binman do the compression.

> Signed-off-by: Manoj Sai 
> Signed-off-by: Suniel Mahesh 
> ---
>  tools/binman/etype/u_boot_nodtb_gzip.py | 28 +
>  tools/binman/ftest.py   |  5 
>  tools/binman/test/282_u_boot_nodtb_gzip.dts | 11 
>  3 files changed, 44 insertions(+)
>  create mode 100644 tools/binman/etype/u_boot_nodtb_gzip.py
>  create mode 100644 tools/binman/test/282_u_boot_nodtb_gzip.dts

Binman supports a 'compressed' property for blobs, so you should be able to add:

compress = 'gzip';

to your node

>
> diff --git a/tools/binman/etype/u_boot_nodtb_gzip.py 
> b/tools/binman/etype/u_boot_nodtb_gzip.py
> new file mode 100644
> index 00..e8afd3de57
> --- /dev/null
> +++ b/tools/binman/etype/u_boot_nodtb_gzip.py
> @@ -0,0 +1,28 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +# Copyright (c) 2023 Amarula Solutions India
> +# Written by Suniel Mahesh 
> +# Reference from Simon Glass 
> +#
> +# Entry-type module for 'u-boot-nodtb.bin.gz'
> +#
> +
> +from binman.entry import Entry
> +from binman.etype.blob import Entry_blob
> +
> +class Entry_u_boot_nodtb_gzip(Entry_blob):
> +"""U-Boot compressed flat binary without device tree appended
> +
> +Properties / Entry arguments:
> +- filename: Filename to include ('u-boot-nodtb.bin.gz')
> +
> +This is the U-Boot compressed raw binary, before allowing it to relocate
> +itself at runtime it should be decompressed. It does not include a device
> +tree blob at the end of it so normally cannot work without it. You can 
> add a
> +u-boot-dtb entry after this one, or use a u-boot entry instead, normally
> +expands to a section containing u-boot and u-boot-dtb
> +"""
> +def __init__(self, section, etype, node):
> +super().__init__(section, etype, node)
> +
> +def GetDefaultFilename(self):
> +return 'u-boot-nodtb.bin.gz'
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index 43b4f850a6..933ebcbd35 100644
> --- a/tools/binman/ftest.py
> +++ b/tools/binman/ftest.py
> @@ -42,6 +42,7 @@ from u_boot_pylib import tout
>  # Contents of test files, corresponding to different entry types
>  U_BOOT_DATA   = b'1234'
>  U_BOOT_IMG_DATA   = b'img'
> +U_BOOT_NODTB_GZ_DATA  = b'uboot nodtb gz'
>  U_BOOT_SPL_DATA   = b'56780123456789abcdefghijklm'
>  U_BOOT_TPL_DATA   = b'tpl9876543210fedcbazywvuts'
>  U_BOOT_VPL_DATA   = b'vpl76543210fedcbazywxyz_'
> @@ -6676,6 +6677,10 @@ fdt fdtmapExtract the 
> devicetree blob from the fdtmap
>  ['fit'])
>  self.assertIn("Node '/fit': Missing tool: 'mkimage'", 
> str(e.exception))
>
> +def testUBootnodtbBinGz(self):
> +   """Test that u-boot-nodtb.bin.gz can be put in a file"""
> +   data = self._DoReadFile('279_u_boot_nodtb_gzip.dts')
> +   self.assertEqual(U_BOOT_NODTB_GZ_DATA, data)
>
>  if __name__ == "__main__":
>  unittest.main()
> diff --git a/tools/binman/test/282_u_boot_nodtb_gzip.dts 
> b/tools/binman/test/282_u_boot_nodtb_gzip.dts
> new file mode 100644
> index 00..79eecea202
> --- /dev/null
> +++ b/tools/binman/test/282_u_boot_nodtb_gzip.dts
> @@ -0,0 +1,11 @@
> +/dts-v1/;
> +
> +/ {
> +   #address-cells = <1>;
> +   #size-cells = <1>;
> +
> +   binman {
> +   u-boot-nodtb-bin-gz {
> +   };
> +   };
> +};
> --
> 2.25.1
>

If you still really want to add an entry type for a compressed U-Boot,
please let me know.

Regards,
Simon

Re: [PATCH 06/10] spl: fit: support for booting a LZMA-compressed U-boot raw binary

[Simon Glass]

Hi Manoj,

On Fri, 30 Jun 2023 at 13:12, Manoj Sai
 wrote:
>
> If LZMA Compression support is enabled, LZMA compressed U-Boot
> raw binary will be at a specified RAM location which is defined
> at UBOOT_COMPRESSED_BINARY_FIT_USER_DEF_ADDR and will be assign it as
> the source address.
>
> lzmaBuffToBuffDecompress function in spl_load_fit_image ,will decompress
> the LZMA compressed U-Boot raw binary which is placed at source address
> to the default CONFIG_SYS_TEXT_BASE location.
>
> spl_load_fit_image function will load the decompressed U-Boot
> raw binary, which is placed at the CONFIG_SYS_TEXT_BASE location.
>
> Signed-off-by: Manoj Sai 
> Signed-off-by: Suniel Mahesh 
> ---
>  common/spl/spl_fit.c | 20 
>  1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> index e2101099ef..b4e95d3fb5 100644
> --- a/common/spl/spl_fit.c
> +++ b/common/spl/spl_fit.c
> @@ -17,6 +17,9 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
> +#include 
>
>  DECLARE_GLOBAL_DATA_PTR;
>
> @@ -239,14 +242,15 @@ static int spl_load_fit_image(struct spl_load_info 
> *info, ulong sector,
> bool external_data = false;
>
> if (IS_ENABLED(CONFIG_SPL_FPGA) ||
> -   (IS_ENABLED(CONFIG_SPL_OS_BOOT) && IS_ENABLED(CONFIG_SPL_GZIP))) {
> +   (IS_ENABLED(CONFIG_SPL_OS_BOOT) && IS_ENABLED(CONFIG_SPL_GZIP)) ||
> +   (IS_ENABLED(CONFIG_SPL_OS_BOOT) && IS_ENABLED(CONFIG_SPL_LZMA))) {

This needs a refactor. You could do:

 if (IS_ENABLED(CONFIG_SPL_FPGA) ||
  (IS_ENABLED(CONFIG_SPL_OS_BOOT) &&
   (IS_ENABLED(CONFIG_SPL_GZIP) || IS_ENABLED(CONFIG_SPL_LZMA))

But I think it is better to creating a static inline functoin in spl.h, like:

static bool spl_compress_enabled()
{
return IS_ENABLED(CONFIG_SPL_GZIP) || IS_ENABLED(CONFIG_SPL_LZMA);
}

and then use that in this patch.

> if (fit_image_get_type(fit, node, ))
> puts("Cannot get image type.\n");
> else
> debug("%s ", genimg_get_type_name(type));
> }
>
> -   if (IS_ENABLED(CONFIG_SPL_GZIP)) {
> +   if (IS_ENABLED(CONFIG_SPL_GZIP) || IS_ENABLED(CONFIG_SPL_LZMA)) {
> fit_image_get_comp(fit, node, _comp);
> debug("%s ", genimg_get_comp_name(image_comp));
> }
> @@ -281,7 +285,8 @@ static int spl_load_fit_image(struct spl_load_info *info, 
> ulong sector,
> return 0;
> }
>
> -   if ((IS_ENABLED(CONFIG_SPL_GZIP) && image_comp == 
> IH_COMP_GZIP)) {
> +   if ((IS_ENABLED(CONFIG_SPL_GZIP) && image_comp == 
> IH_COMP_GZIP) ||
> +   (IS_ENABLED(CONFIG_SPL_LZMA) && image_comp == 
> IH_COMP_LZMA)) {
> src_ptr = 
> map_sysmem(ALIGN(CONFIG_VAL(UBOOT_COMPRESSED_BINARY_FIT_USER_DEF_ADDR),

I think you should drop the ALIGN() and tell the user that the address
must be cached-aligned, in the Kconfig help, since it will be
confusing if it uses a different request from what was requested.

> ARCH_DMA_MINALIGN), len);
> } else {
> @@ -325,13 +330,20 @@ static int spl_load_fit_image(struct spl_load_info 
> *info, ulong sector,
>
> load_ptr = map_sysmem(load_addr, length);
>
> -   if ((IS_ENABLED(CONFIG_SPL_GZIP) && image_comp == IH_COMP_GZIP)) {
> +   if ((IS_ENABLED(CONFIG_SPL_GZIP) && image_comp == IH_COMP_GZIP) ||
> +   (IS_ENABLED(CONFIG_SPL_LZMA) && image_comp == IH_COMP_LZMA)) {
> if ((IS_ENABLED(CONFIG_SPL_GZIP) && image_comp == 
> IH_COMP_GZIP)) {
> size = length;
> if (gunzip(load_ptr, CONFIG_SYS_BOOTM_LEN, src, 
> )) {
> puts("Uncompressing error\n");
> return -EIO;
> }
> +   } else if ((IS_ENABLED(CONFIG_SPL_LZMA) && image_comp == 
> IH_COMP_LZMA)) {
> +   size = CONFIG_SYS_BOOTM_LEN;
> +   if (lzmaBuffToBuffDecompress(load_ptr, , src, 
> length)) {
> +   puts("Uncompressing error\n");
> +   return -EIO;
> +   }
> }
> length = size;
> } else {
> --
> 2.25.1
>

Can you use the existing image_decomp() instead?

Regards,
Simon

Re: [PATCH 05/10] Makefile: Add support to generate LZMA compressed raw u-boot binary

[Simon Glass]

Hi Manoj,

On Fri, 30 Jun 2023 at 13:12, Manoj Sai
 wrote:
>
> Add support for generating LZMA compressed raw u-boot binary.
>
> Signed-off-by: Manoj Sai 
> Signed-off-by: Suniel Mahesh 
> ---
>  Makefile | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/Makefile b/Makefile
> index 6e15ebd116..d4f453cce1 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1315,6 +1315,9 @@ u-boot-nodtb.bin: u-boot FORCE
>  ifeq ($(CONFIG_SPL_GZIP),y)
> @gzip -k u-boot-nodtb.bin
>  endif
> +ifeq ($(CONFIG_SPL_LZMA),y)
> +   @lzma -k u-boot-nodtb.bin
> +endif
>
>  u-boot.ldr:u-boot
> $(CREATE_LDR_ENV)
> --
> 2.25.1
>

Again, this should be in binman.

Regards,
Simon

Re: [PATCH 03/10] rockchip: RAM location for the compressed U-BOOT raw binary

[Simon Glass]

Hi Manoj,

On Fri, 30 Jun 2023 at 13:12, Manoj Sai
 wrote:
>
> Add the support that ,if compression support is enabled in SPL
> a RAM location needs to be defined as the source address where
> compressed U-BOOT raw binary will be stored.
>
> spl_load_fit_image function takes care that, compressed U-Boot raw
> binary which is placed at this address, will be uncompressed to default
> CONFIG_SYS_TEXT_BASE location.

But isn't that where U-Boot runs from? So if you load it there, how
can you uncompress it to the same place?
Reviewed-by: Simon Glass 
Anyway I agree this is a sensible default.

>
> Signed-off-by: Manoj Sai 
> ---
>  arch/arm/mach-rockchip/Kconfig | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
> index 9d6d20bf8e..d8d9c1964d 100644
> --- a/arch/arm/mach-rockchip/Kconfig
> +++ b/arch/arm/mach-rockchip/Kconfig
> @@ -520,6 +520,9 @@ config ROCKCHIP_SPI_IMAGE
>  config LNX_KRNL_IMG_TEXT_OFFSET_BASE
> default TEXT_BASE
>
> +config SPL_UBOOT_COMPRESSED_BINARY_FIT_USER_DEF_ADDR
> +   default 0x500 if ROCKCHIP_RK3399 || ROCKCHIP_RK3328
> +
>  source "arch/arm/mach-rockchip/px30/Kconfig"
>  source "arch/arm/mach-rockchip/rk3036/Kconfig"
>  source "arch/arm/mach-rockchip/rk3066/Kconfig"
> --
> 2.25.1
>

Reviewed-by: Simon Glass 

Regards,
Simon

Re: [PATCH 02/10] spl: Kconfig: Address support for compressed U-BOOT raw binary

[Simon Glass]

Hi Manoj,

On Fri, 30 Jun 2023 at 13:12, Manoj Sai
 wrote:
>
> Add the support that ,if compression support is enabled in SPL
> a location needs to be defined as the source address where
> compressed U-BOOT raw binary will be stored.
>
> spl_load_fit_image function takes care that, compressed U-Boot raw
> binary which is placed at this address, will be uncompressed to default
> CONFIG_SYS_TEXT_BASE location.
>
> Signed-off-by: Manoj Sai 
> Signed-off-by: Suniel Mahesh 
> ---
>  common/spl/Kconfig | 15 +++
>  1 file changed, 15 insertions(+)
>
> diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> index 2c042ad306..5dd1f3c469 100644
> --- a/common/spl/Kconfig
> +++ b/common/spl/Kconfig
> @@ -1547,6 +1547,21 @@ config SPL_AT91_MCK_BYPASS
>   The external source has to provide a stable clock on the XIN pin.
>   If this option is disabled, the SoC expects a crystal oscillator
>   that needs driving on both XIN and XOUT lines.

blank line here

> +choice
> +   prompt "Location for compressed U-BOOT raw binary"

compressed U-Boot binary

> +   depends on SPL_GZIP || SPL_LZMA
> +   default SPL_UBOOT_COMPRESSED_BINARY_FIT_USER_DEFINED_AREA

missing help

This is too long and there is a lot ot redundancy.. How about SPL_GZIP_FROM_ADDR

> +
> +config SPL_UBOOT_COMPRESSED_BINARY_FIT_USER_DEFINED_AREA
> +   bool "compressed U-BOOT raw binary user-defined location"

missing help

blank line

> +endchoice

But you only have one thing in the choice, so can you just drop the
choice and use a bool instead?

> +
> +config SPL_UBOOT_COMPRESSED_BINARY_FIT_USER_DEF_ADDR

How about SPL_GZIP_LOADADDR?


> +   hex "Address of memory where compressed U-BOOT raw binary is stored"

U-Boot (please fix throughout)

> +   depends on SPL_UBOOT_COMPRESSED_BINARY_FIT_USER_DEFINED_AREA
> +   help
> + The FIT image containing the compressed U-BOOT raw binary will be 
> stored
> + in an area defined at compilation time. This is the address for 
> this area.
>  endmenu
>
>  config TPL
> --
> 2.25.1
>

Regards,
Simon

Re: [PATCH 01/10] Makefile: Add support to generate GZIP compressed raw u-boot binary

[Simon Glass]

Hi Manoj,

On Fri, 30 Jun 2023 at 13:12, Manoj Sai
 wrote:
>
> Add support for generating a GZIP-compressed raw U-boot binary.
>
> Signed-off-by: Manoj Sai 
> Signed-off-by: Suniel Mahesh 
> ---
>  Makefile | 3 +++
>  1 file changed, 3 insertions(+)

Please can you use binman to do that?

We are trying to remove the extra build rules in the Makefile.

>
> diff --git a/Makefile b/Makefile
> index 444baaefd0..6e15ebd116 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1312,6 +1312,9 @@ endif
>  u-boot-nodtb.bin: u-boot FORCE
> $(call if_changed,objcopy_uboot)
> $(BOARD_SIZE_CHECK)
> +ifeq ($(CONFIG_SPL_GZIP),y)
> +   @gzip -k u-boot-nodtb.bin
> +endif
>
>  u-boot.ldr:u-boot
> $(CREATE_LDR_ENV)
> --
> 2.25.1
>

Regards,
Simon

[PATCH 58/58] buildman: Move copy_files() out ot BuilderThread class

[Simon Glass]

This does not need to be in the class. Move it out to avoid a pylint
warning.

Signed-off-by: Simon Glass 
---

 tools/buildman/builderthread.py | 47 +
 1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index ed84a0f6baa3..25f460c207db 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -64,6 +64,27 @@ def _remove_old_outputs(out_dir):
 os.remove(fname)
 
 
+def copy_files(out_dir, build_dir, dirname, patterns):
+"""Copy files from the build directory to the output.
+
+Args:
+out_dir (str): Path to output directory containing the files
+build_dir (str): Place to copy the files
+dirname (str): Source directory, '' for normal U-Boot, 'spl' for SPL
+patterns (list of str): A list of filenames to copy, each relative
+   to the build directory
+"""
+for pattern in patterns:
+file_list = glob.glob(os.path.join(out_dir, dirname, pattern))
+for fname in file_list:
+target = os.path.basename(fname)
+if dirname:
+base, ext = os.path.splitext(target)
+if ext:
+target = f'{base}-{dirname}{ext}'
+shutil.copy(fname, os.path.join(build_dir, target))
+
+
 # pylint: disable=R0903
 class BuilderJob:
 """Holds information about a job to be performed by a thread
@@ -592,7 +613,7 @@ class BuilderThread(threading.Thread):
 capture_stderr=True, cwd=result.out_dir,
 raise_on_error=False, env=env)
 if not work_in_output:
-self.copy_files(result.out_dir, build_dir, '', ['uboot.env'])
+copy_files(result.out_dir, build_dir, '', ['uboot.env'])
 
 # Write out the image sizes file. This is similar to the output
 # of binutil's 'size' utility, but it omits the header line and
@@ -607,7 +628,7 @@ class BuilderThread(threading.Thread):
 if not work_in_output:
 # Write out the configuration files, with a special case for SPL
 for dirname in ['', 'spl', 'tpl']:
-self.copy_files(
+copy_files(
 result.out_dir, build_dir, dirname,
 ['u-boot.cfg', 'spl/u-boot-spl.cfg', 'tpl/u-boot-tpl.cfg',
  '.config', 'include/autoconf.mk',
@@ -615,31 +636,11 @@ class BuilderThread(threading.Thread):
 
 # Now write the actual build output
 if keep_outputs:
-self.copy_files(
+copy_files(
 result.out_dir, build_dir, '',
 ['u-boot*', '*.bin', '*.map', '*.img', 'MLO', 'SPL',
  'include/autoconf.mk', 'spl/u-boot-spl*'])
 
-def copy_files(self, out_dir, build_dir, dirname, patterns):
-"""Copy files from the build directory to the output.
-
-Args:
-out_dir (str): Path to output directory containing the files
-build_dir (str): Place to copy the files
-dirname (str): Source directory, '' for normal U-Boot, 'spl' for 
SPL
-patterns (list of str): A list of filenames to copy, each relative
-   to the build directory
-"""
-for pattern in patterns:
-file_list = glob.glob(os.path.join(out_dir, dirname, pattern))
-for fname in file_list:
-target = os.path.basename(fname)
-if dirname:
-base, ext = os.path.splitext(target)
-if ext:
-target = f'{base}-{dirname}{ext}'
-shutil.copy(fname, os.path.join(build_dir, target))
-
 def _send_result(self, result):
 """Send a result to the builder for processing
 
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 54/58] buildman: Create a function to handle config and build

[Simon Glass]

Move this code into a _config_and_build() function, so reduce the size of
run_commit().

Signed-off-by: Simon Glass 
---

 tools/buildman/builderthread.py | 97 +
 1 file changed, 61 insertions(+), 36 deletions(-)

diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index 0c73b8646b1c..aa4a9d9919c6 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -345,6 +345,64 @@ class BuilderThread(threading.Thread):
 commit = 'current'
 return commit
 
+def _config_and_build(self, commit_upto, brd, work_dir, do_config,
+  config_only, adjust_cfg, commit, out_dir, 
out_rel_dir,
+  result):
+"""Do the build, configuring first if necessary
+
+Args:
+commit_upto (int): Commit number to build (0...n-1)
+brd (Board): Board to create arguments for
+work_dir (str): Directory to which the source will be checked out
+do_config (bool): True to run a make _defconfig on the 
source
+config_only (bool): Only configure the source, do not build it
+adjust_cfg (list of str): See the cfgutil module and run_commit()
+commit (Commit): Commit only being built
+out_dir (str): Output directory for the build
+out_rel_dir (str): Output directory relatie to the current dir
+result (CommandResult): Previous result
+
+Returns:
+tuple:
+result (CommandResult): Result of the build
+do_config (bool): indicates whether 'make config' is needed on
+the next incremental build
+"""
+# Set up the environment and command line
+env = self.toolchain.MakeEnvironment(self.builder.full_path)
+mkdir(out_dir)
+
+args, cwd, src_dir = self._build_args(brd, out_dir, out_rel_dir,
+  work_dir, commit_upto)
+config_args = [f'{brd.target}_defconfig']
+config_out = io.StringIO()
+
+_remove_old_outputs(out_dir)
+
+# If we need to reconfigure, do that now
+cfg_file = os.path.join(out_dir, '.config')
+cmd_list = []
+if do_config or adjust_cfg:
+result = self._reconfigure(
+commit, brd, cwd, args, env, config_args, config_out, cmd_list)
+do_config = False   # No need to configure next time
+if adjust_cfg:
+cfgutil.adjust_cfg_file(cfg_file, adjust_cfg)
+
+# Now do the build, if everything looks OK
+if result.return_code == 0:
+result = self._build(commit, brd, cwd, args, env, cmd_list,
+ config_only)
+if adjust_cfg:
+errs = cfgutil.check_cfg_file(cfg_file, adjust_cfg)
+if errs:
+result.stderr += errs
+result.return_code = 1
+result.stderr = result.stderr.replace(src_dir + '/', '')
+if self.builder.verbose_build:
+result.stdout = config_out.getvalue() + result.stdout
+result.cmd_list = cmd_list
+return result, do_config
 
 def run_commit(self, commit_upto, brd, work_dir, do_config, config_only,
   force_build, force_build_failures, work_in_output,
@@ -402,42 +460,9 @@ class BuilderThread(threading.Thread):
 
 if self.toolchain:
 commit = self._checkout(commit_upto, work_dir)
-
-# Set up the environment and command line
-env = self.toolchain.MakeEnvironment(self.builder.full_path)
-mkdir(out_dir)
-
-args, cwd, src_dir = self._build_args(brd, out_dir, 
out_rel_dir,
-  work_dir, commit_upto)
-config_args = [f'{brd.target}_defconfig']
-config_out = io.StringIO()
-
-_remove_old_outputs(out_dir)
-
-# If we need to reconfigure, do that now
-cfg_file = os.path.join(out_dir, '.config')
-cmd_list = []
-if do_config or adjust_cfg:
-result = self._reconfigure(
-commit, brd, cwd, args, env, config_args, config_out,
-cmd_list)
-do_config = False   # No need to configure next time
-if adjust_cfg:
-cfgutil.adjust_cfg_file(cfg_file, adjust_cfg)
-
-# Now do the build, if everything looks OK
-if result.return_code == 0:
-result = self._build(commit, brd, cwd, args, env, cmd_list,
- config_only)
-if adjust_cfg:
-errs = cfgutil.check_cfg_file(cfg_file, adjust_cfg)
-if errs:
-  

[PATCH 43/58] buildman: Drop unnecessary assignment of config_out

[Simon Glass]

This is already set up earlier in the function, so drop the extra
assignment.

Signed-off-by: Simon Glass 
---

 tools/buildman/builderthread.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index f110137e8f6e..ad12e9ede7f4 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -286,7 +286,6 @@ class BuilderThread(threading.Thread):
 cfg_file = os.path.join(out_dir, '.config')
 cmd_list = []
 if do_config or adjust_cfg:
-config_out = ''
 if self.mrproper:
 result = self.make(commit, brd, 'mrproper', cwd,
 'mrproper', *args, env=env)
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 57/58] buildman: Tidy up some comments in builderthread

[Simon Glass]

Make sure all functions have full argument documentation.

Signed-off-by: Simon Glass 
---

 tools/buildman/builderthread.py | 66 ++---
 1 file changed, 36 insertions(+), 30 deletions(-)

diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index 043e92b6d92d..ed84a0f6baa3 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -23,11 +23,15 @@ from u_boot_pylib import command
 RETURN_CODE_RETRY = -1
 BASE_ELF_FILENAMES = ['u-boot', 'spl/u-boot-spl', 'tpl/u-boot-tpl']
 
-def mkdir(dirname, parents = False):
+def mkdir(dirname, parents=False):
 """Make a directory if it doesn't already exist.
 
 Args:
-dirname: Directory to create
+dirname (str): Directory to create
+parents (bool): True to also make parent directories
+
+Raises:
+OSError: File already exists
 """
 try:
 if parents:
@@ -141,14 +145,16 @@ class BuilderThread(threading.Thread):
 argument is only for information.
 
 Args:
-commit: Commit object that is being built
-brd: Board object that is being built
-stage: Stage of the build. Valid stages are:
+commit (Commit): Commit that is being built
+brd (Board): Board that is being built
+stage (str): Stage of the build. Valid stages are:
 mrproper - can be called to clean source
 config - called to configure for a board
 build - the main make invocation - it does the build
-args: A list of arguments to pass to 'make'
-kwargs: A list of keyword arguments to pass to command.run_pipe()
+cwd (str): Working directory to set, or None to leave it alone
+*args (list of str): Arguments to pass to 'make'
+**kwargs (dict): A list of keyword arguments to pass to
+command.run_pipe()
 
 Returns:
 CommandResult object
@@ -418,16 +424,16 @@ class BuilderThread(threading.Thread):
 the build and just return the previously-saved results.
 
 Args:
-commit_upto: Commit number to build (0...n-1)
-brd: Board object to build
-work_dir: Directory to which the source will be checked out
-do_config: True to run a make _defconfig on the source
-config_only: Only configure the source, do not build it
-force_build: Force a build even if one was previously done
-force_build_failures: Force a bulid if the previous result showed
-failure
-work_in_output: Use the output directory as the work directory and
-don't write to a separate output directory.
+commit_upto (int): Commit number to build (0...n-1)
+brd (Board): Board to build
+work_dir (str): Directory to which the source will be checked out
+do_config (bool): True to run a make _defconfig on the 
source
+config_only (bool): Only configure the source, do not build it
+force_build (bool): Force a build even if one was previously done
+force_build_failures (bool): Force a bulid if the previous result
+showed failure
+work_in_output (bool) : Use the output directory as the work
+directory and don't write to a separate output directory.
 adjust_cfg (list of str): List of changes to make to .config file
 before building. Each is one of (where C is either CONFIG_xxx
 or just xxx):
@@ -476,11 +482,11 @@ class BuilderThread(threading.Thread):
 """Write a built result to the output directory.
 
 Args:
-result: CommandResult object containing result to write
-keep_outputs: True to store the output binaries, False
+result (CommandResult): result to write
+keep_outputs (bool): True to store the output binaries, False
 to delete them
-work_in_output: Use the output directory as the work directory and
-don't write to a separate output directory.
+work_in_output (bool): Use the output directory as the work
+directory and don't write to a separate output directory.
 """
 # If we think this might have been aborted with Ctrl-C, record the
 # failure but not that we are 'done' with this board. A retry may fix
@@ -618,10 +624,10 @@ class BuilderThread(threading.Thread):
 """Copy files from the build directory to the output.
 
 Args:
-out_dir: Path to output directory containing the files
-build_dir: Place to copy the files
-dirname: Source directory, '' for normal U-Boot, 'spl' for SPL
-patterns: A list of filenames (strings) to copy, each relative
+out_dir (str): 

[PATCH 55/58] buildman: Avoid passing result into _read_done_file()

[Simon Glass]

Move the creating of the result object into the function which sets it
up, to simplify the code.

Signed-off-by: Simon Glass 
---

 tools/buildman/builderthread.py | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index aa4a9d9919c6..d3912390bc4f 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -262,21 +262,26 @@ class BuilderThread(threading.Thread):
 result.return_code = 0
 return result
 
-def _read_done_file(self, commit_upto, brd, result, force_build,
+def _read_done_file(self, commit_upto, brd, force_build,
 force_build_failures):
 """Check the 'done' file and see if this commit should be built
 
 Args:
 commit (Commit): Commit only being built
 brd (Board): Board being built
-result (CommandResult): result object to update
 force_build (bool): Force a build even if one was previously done
 force_build_failures (bool): Force a bulid if the previous result
 showed failure
 
 Returns:
-bool: True if build should be built
+tuple:
+bool: True if build should be built
+CommandResult: if there was a previous run:
+- already_done set to True
+- return_code set to return code
+- result.stderr set to 'bad' if stderr output was recorded
 """
+result = command.CommandResult()
 done_file = self.builder.get_done_file(commit_upto, brd.target)
 result.already_done = os.path.exists(done_file)
 will_build = (force_build or force_build_failures or
@@ -300,7 +305,7 @@ class BuilderThread(threading.Thread):
 elif not force_build:
 # The build passed, so no need to build it again
 will_build = False
-return will_build
+return will_build, result
 
 def _decide_dirs(self, brd, work_dir, work_in_output):
 """Decide the output directory to use
@@ -438,13 +443,11 @@ class BuilderThread(threading.Thread):
 """
 # Create a default result - it will be overwritte by the call to
 # self.make() below, in the event that we do a build.
-result = command.CommandResult()
-result.return_code = 0
 out_dir, out_rel_dir = self._decide_dirs(brd, work_dir, work_in_output)
 
 # Check if the job was already completed last time
-will_build = self._read_done_file(commit_upto, brd, result, 
force_build,
-  force_build_failures)
+will_build, result = self._read_done_file(commit_upto, brd, 
force_build,
+  force_build_failures)
 
 if will_build:
 # We are going to have to build it. First, get a toolchain
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 56/58] buildman: Tidy up reporting of a toolchain error

[Simon Glass]

Provide the text of the exception when something goes wrong.

Signed-off-by: Simon Glass 
---

 tools/buildman/builderthread.py | 7 +--
 tools/buildman/func_test.py | 6 --
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index d3912390bc4f..043e92b6d92d 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -457,18 +457,13 @@ class BuilderThread(threading.Thread):
 except ValueError as err:
 result.return_code = 10
 result.stdout = ''
-result.stderr = str(err)
-# TODO(s...@chromium.org): This gets swallowed, but needs
-# to be reported.
+result.stderr = f'Tool chain error for {brd.arch}: 
{str(err)}'
 
 if self.toolchain:
 commit = self._checkout(commit_upto, work_dir)
 result, do_config = self._config_and_build(
 commit_upto, brd, work_dir, do_config, config_only,
 adjust_cfg, commit, out_dir, out_rel_dir, result)
-else:
-result.return_code = 1
-result.stderr = f'No tool chain for {brd.arch}\n'
 result.already_done = False
 
 result.toolchain = self.toolchain
diff --git a/tools/buildman/func_test.py b/tools/buildman/func_test.py
index b9f99c76cdb4..dbaca7fd1094 100644
--- a/tools/buildman/func_test.py
+++ b/tools/buildman/func_test.py
@@ -503,8 +503,10 @@ Some images are invalid'''
 if brd.arch != 'sandbox':
   errfile = self._builder.get_err_file(commit, brd.target)
   fd = open(errfile)
-  self.assertEqual(fd.readlines(),
-  ['No tool chain for %s\n' % brd.arch])
+  self.assertEqual(
+  fd.readlines(),
+  [f'Tool chain error for {brd.arch}: '
+   f"No tool chain found for arch '{brd.arch}'"])
   fd.close()
 
 def testBranch(self):
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 52/58] buildman: Move code to decide output dirs

[Simon Glass]

Put this in its own function to reduce the size of the run_commit()
function.

Signed-off-by: Simon Glass 
---

 tools/buildman/builderthread.py | 34 -
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index 4abad86ebc7b..78405956ef51 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -302,6 +302,30 @@ class BuilderThread(threading.Thread):
 will_build = False
 return will_build
 
+def _decide_dirs(self, brd, work_dir, work_in_output):
+"""Decide the output directory to use
+
+Args:
+work_dir (str): Directory to which the source will be checked out
+work_in_output (bool): Use the output directory as the work
+directory and don't write to a separate output directory.
+
+Returns:
+tuple:
+out_dir (str): Output directory for the build
+out_rel_dir (str): Output directory relatie to the current dir
+"""
+if work_in_output or self.builder.in_tree:
+out_rel_dir = None
+out_dir = work_dir
+else:
+if self.per_board_out_dir:
+out_rel_dir = os.path.join('..', brd.target)
+else:
+out_rel_dir = 'build'
+out_dir = os.path.join(work_dir, out_rel_dir)
+return out_dir, out_rel_dir
+
 def run_commit(self, commit_upto, brd, work_dir, do_config, config_only,
   force_build, force_build_failures, work_in_output,
   adjust_cfg):
@@ -338,15 +362,7 @@ class BuilderThread(threading.Thread):
 # self.make() below, in the event that we do a build.
 result = command.CommandResult()
 result.return_code = 0
-if work_in_output or self.builder.in_tree:
-out_rel_dir = None
-out_dir = work_dir
-else:
-if self.per_board_out_dir:
-out_rel_dir = os.path.join('..', brd.target)
-else:
-out_rel_dir = 'build'
-out_dir = os.path.join(work_dir, out_rel_dir)
+out_dir, out_rel_dir = self._decide_dirs(brd, work_dir, work_in_output)
 
 # Check if the job was already completed last time
 will_build = self._read_done_file(commit_upto, brd, result, 
force_build,
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 53/58] buildman: Move checkout code to a separate function

[Simon Glass]

Put this in its own function to reduce the size of the run_commit()
function

Signed-off-by: Simon Glass 
---

 tools/buildman/builderthread.py | 30 +-
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index 78405956ef51..0c73b8646b1c 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -326,6 +326,26 @@ class BuilderThread(threading.Thread):
 out_dir = os.path.join(work_dir, out_rel_dir)
 return out_dir, out_rel_dir
 
+def _checkout(self, commit_upto, work_dir):
+"""Checkout the right commit
+
+Args:
+commit_upto (int): Commit number to build (0...n-1)
+work_dir (str): Directory to which the source will be checked out
+
+Returns:
+Commit: Commit being built, or 'current' for current source
+"""
+if self.builder.commits:
+commit = self.builder.commits[commit_upto]
+if self.builder.checkout:
+git_dir = os.path.join(work_dir, '.git')
+gitutil.checkout(commit.hash, git_dir, work_dir, force=True)
+else:
+commit = 'current'
+return commit
+
+
 def run_commit(self, commit_upto, brd, work_dir, do_config, config_only,
   force_build, force_build_failures, work_in_output,
   adjust_cfg):
@@ -381,15 +401,7 @@ class BuilderThread(threading.Thread):
 # to be reported.
 
 if self.toolchain:
-# Checkout the right commit
-if self.builder.commits:
-commit = self.builder.commits[commit_upto]
-if self.builder.checkout:
-git_dir = os.path.join(work_dir, '.git')
-gitutil.checkout(commit.hash, git_dir, work_dir,
- force=True)
-else:
-commit = 'current'
+commit = self._checkout(commit_upto, work_dir)
 
 # Set up the environment and command line
 env = self.toolchain.MakeEnvironment(self.builder.full_path)
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 44/58] buildman: Start a function to set up the make arguments

[Simon Glass]

Move some of this code into a new funciion, to help reduce the size of the
run_commits() function.

Signed-off-by: Simon Glass 
---

 tools/buildman/builderthread.py | 38 -
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index ad12e9ede7f4..47ebf4dcdd90 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -137,6 +137,28 @@ class BuilderThread(threading.Thread):
 return self.builder.do_make(commit, brd, stage, cwd, *args,
 **kwargs)
 
+def _build_args(self, args):
+"""Set up arguments to the args list based on the settings
+
+Args:
+args (list of str): List of string arguments to add things to
+"""
+if self.builder.verbose_build:
+args.append('V=1')
+else:
+args.append('-s')
+if self.builder.num_jobs is not None:
+args.extend(['-j', str(self.builder.num_jobs)])
+if self.builder.warnings_as_errors:
+args.append('KCFLAGS=-Werror')
+args.append('HOSTCFLAGS=-Werror')
+if self.builder.allow_missing:
+args.append('BINMAN_ALLOW_MISSING=1')
+if self.builder.no_lto:
+args.append('NO_LTO=1')
+if self.builder.reproducible_builds:
+args.append('SOURCE_DATE_EPOCH=0')
+
 def run_commit(self, commit_upto, brd, work_dir, do_config, config_only,
   force_build, force_build_failures, work_in_output,
   adjust_cfg):
@@ -252,21 +274,7 @@ class BuilderThread(threading.Thread):
 src_dir = os.getcwd()
 else:
 args.append(f'O={out_rel_dir}')
-if self.builder.verbose_build:
-args.append('V=1')
-else:
-args.append('-s')
-if self.builder.num_jobs is not None:
-args.extend(['-j', str(self.builder.num_jobs)])
-if self.builder.warnings_as_errors:
-args.append('KCFLAGS=-Werror')
-args.append('HOSTCFLAGS=-Werror')
-if self.builder.allow_missing:
-args.append('BINMAN_ALLOW_MISSING=1')
-if self.builder.no_lto:
-args.append('NO_LTO=1')
-if self.builder.reproducible_builds:
-args.append('SOURCE_DATE_EPOCH=0')
+self._build_args(args)
 config_args = [f'{brd.target}_defconfig']
 config_out = ''
 args.extend(self.builder.toolchains.GetMakeArguments(brd))
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 51/58] buildman: Move code to remove old outputs

[Simon Glass]

Put this in its own function to reduce the size of the run_commit()
function.

Signed-off-by: Simon Glass 
---

 tools/buildman/builderthread.py | 28 +++-
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index b4891059b6db..4abad86ebc7b 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -42,6 +42,24 @@ def mkdir(dirname, parents = False):
 else:
 raise
 
+
+def _remove_old_outputs(out_dir):
+"""Remove any old output-target files
+
+Args:
+out_dir (str): Output directory for the build
+
+Since we use a build directory that was previously used by another
+board, it may have produced an SPL image. If we don't remove it (i.e.
+see do_config and self.mrproper below) then it will appear to be the
+output of this build, even if it does not produce SPL images.
+"""
+for elf in BASE_ELF_FILENAMES:
+fname = os.path.join(out_dir, elf)
+if os.path.exists(fname):
+os.remove(fname)
+
+
 # pylint: disable=R0903
 class BuilderJob:
 """Holds information about a job to be performed by a thread
@@ -366,15 +384,7 @@ class BuilderThread(threading.Thread):
 config_args = [f'{brd.target}_defconfig']
 config_out = io.StringIO()
 
-# Remove any output targets. Since we use a build directory 
that
-# was previously used by another board, it may have produced an
-# SPL image. If we don't remove it (i.e. see do_config and
-# self.mrproper below) then it will appear to be the output of
-# this build, even if it does not produce SPL images.
-for elf in BASE_ELF_FILENAMES:
-fname = os.path.join(out_dir, elf)
-if os.path.exists(fname):
-os.remove(fname)
+_remove_old_outputs(out_dir)
 
 # If we need to reconfigure, do that now
 cfg_file = os.path.join(out_dir, '.config')
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 50/58] buildman: Move reading of the done file into a function

[Simon Glass]

Move this logic into its own function to reduce the size of the
run_commt() function.

Signed-off-by: Simon Glass 
---

 tools/buildman/builderthread.py | 66 +
 1 file changed, 42 insertions(+), 24 deletions(-)

diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index 2d54e6283023..b4891059b6db 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -244,6 +244,46 @@ class BuilderThread(threading.Thread):
 result.return_code = 0
 return result
 
+def _read_done_file(self, commit_upto, brd, result, force_build,
+force_build_failures):
+"""Check the 'done' file and see if this commit should be built
+
+Args:
+commit (Commit): Commit only being built
+brd (Board): Board being built
+result (CommandResult): result object to update
+force_build (bool): Force a build even if one was previously done
+force_build_failures (bool): Force a bulid if the previous result
+showed failure
+
+Returns:
+bool: True if build should be built
+"""
+done_file = self.builder.get_done_file(commit_upto, brd.target)
+result.already_done = os.path.exists(done_file)
+will_build = (force_build or force_build_failures or
+not result.already_done)
+if result.already_done:
+with open(done_file, 'r', encoding='utf-8') as outf:
+try:
+result.return_code = int(outf.readline())
+except ValueError:
+# The file may be empty due to running out of disk space.
+# Try a rebuild
+result.return_code = RETURN_CODE_RETRY
+
+# Check the signal that the build needs to be retried
+if result.return_code == RETURN_CODE_RETRY:
+will_build = True
+elif will_build:
+err_file = self.builder.get_err_file(commit_upto, brd.target)
+if os.path.exists(err_file) and os.stat(err_file).st_size:
+result.stderr = 'bad'
+elif not force_build:
+# The build passed, so no need to build it again
+will_build = False
+return will_build
+
 def run_commit(self, commit_upto, brd, work_dir, do_config, config_only,
   force_build, force_build_failures, work_in_output,
   adjust_cfg):
@@ -291,30 +331,8 @@ class BuilderThread(threading.Thread):
 out_dir = os.path.join(work_dir, out_rel_dir)
 
 # Check if the job was already completed last time
-done_file = self.builder.get_done_file(commit_upto, brd.target)
-result.already_done = os.path.exists(done_file)
-will_build = (force_build or force_build_failures or
-not result.already_done)
-if result.already_done:
-# Get the return code from that build and use it
-with open(done_file, 'r', encoding='utf-8') as outf:
-try:
-result.return_code = int(outf.readline())
-except ValueError:
-# The file may be empty due to running out of disk space.
-# Try a rebuild
-result.return_code = RETURN_CODE_RETRY
-
-# Check the signal that the build needs to be retried
-if result.return_code == RETURN_CODE_RETRY:
-will_build = True
-elif will_build:
-err_file = self.builder.get_err_file(commit_upto, brd.target)
-if os.path.exists(err_file) and os.stat(err_file).st_size:
-result.stderr = 'bad'
-elif not force_build:
-# The build passed, so no need to build it again
-will_build = False
+will_build = self._read_done_file(commit_upto, brd, result, 
force_build,
+  force_build_failures)
 
 if will_build:
 # We are going to have to build it. First, get a toolchain
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 49/58] buildman: Move bulid code into its own function

[Simon Glass]

Split this into its own function so reduce the size of run_commit().

Signed-off-by: Simon Glass 
---

 tools/buildman/builderthread.py | 40 -
 1 file changed, 30 insertions(+), 10 deletions(-)

diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index 80fca2a61bb3..2d54e6283023 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -218,6 +218,32 @@ class BuilderThread(threading.Thread):
 config_out.write(result.combined)
 return result
 
+def _build(self, commit, brd, cwd, args, env, cmd_list, config_only):
+"""Perform the build
+
+Args:
+commit (Commit): Commit only being built
+brd (Board): Board being built
+cwd (str): Current working directory
+args (list of str): Arguments to pass to make
+env (dict): Environment strings
+cmd_list (list of str): List to add the commands to, for logging
+config_only (bool): True if this is a config-only build (using the
+'make cfg' target)
+
+Returns:
+CommandResult object
+"""
+if config_only:
+args.append('cfg')
+result = self.make(commit, brd, 'build', cwd, *args, env=env)
+cmd_list.append([self.builder.gnu_make] + args)
+if (result.return_code == 2 and
+('Some images are invalid' in result.stderr)):
+# This is handled later by the check for output in stderr
+result.return_code = 0
+return result
+
 def run_commit(self, commit_upto, brd, work_dir, do_config, config_only,
   force_build, force_build_failures, work_in_output,
   adjust_cfg):
@@ -342,17 +368,11 @@ class BuilderThread(threading.Thread):
 do_config = False   # No need to configure next time
 if adjust_cfg:
 cfgutil.adjust_cfg_file(cfg_file, adjust_cfg)
+
+# Now do the build, if everything looks OK
 if result.return_code == 0:
-if config_only:
-args.append('cfg')
-result = self.make(commit, brd, 'build', cwd, *args,
-env=env)
-cmd_list.append([self.builder.gnu_make] + args)
-if (result.return_code == 2 and
-('Some images are invalid' in result.stderr)):
-# This is handled later by the check for output in
-# stderr
-result.return_code = 0
+result = self._build(commit, brd, cwd, args, env, cmd_list,
+ config_only)
 if adjust_cfg:
 errs = cfgutil.check_cfg_file(cfg_file, adjust_cfg)
 if errs:
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 48/58] buildman: Move reconfigure code into its own function

[Simon Glass]

Split this into its own function so reduce the size of run_commit().

Signed-off-by: Simon Glass 
---

 tools/buildman/builderthread.py | 41 -
 1 file changed, 30 insertions(+), 11 deletions(-)

diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index 9c2938d1b071..80fca2a61bb3 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -191,6 +191,33 @@ class BuilderThread(threading.Thread):
 args.extend(self.toolchain.MakeArgs())
 return args, cwd, src_dir
 
+def _reconfigure(self, commit, brd, cwd, args, env, config_args, 
config_out,
+ cmd_list):
+"""Reconfigure the build
+
+Args:
+commit (Commit): Commit only being built
+brd (Board): Board being built
+cwd (str): Current working directory
+args (list of str): Arguments to pass to make
+env (dict): Environment strings
+config_args (list of str): defconfig arg for this board
+cmd_list (list of str): List to add the commands to, for logging
+
+Returns:
+CommandResult object
+"""
+if self.mrproper:
+result = self.make(commit, brd, 'mrproper', cwd, 'mrproper', *args,
+   env=env)
+config_out.write(result.combined)
+cmd_list.append([self.builder.gnu_make, 'mrproper', *args])
+result = self.make(commit, brd, 'config', cwd, *(args + config_args),
+   env=env)
+cmd_list.append([self.builder.gnu_make] + args + config_args)
+config_out.write(result.combined)
+return result
+
 def run_commit(self, commit_upto, brd, work_dir, do_config, config_only,
   force_build, force_build_failures, work_in_output,
   adjust_cfg):
@@ -309,17 +336,9 @@ class BuilderThread(threading.Thread):
 cfg_file = os.path.join(out_dir, '.config')
 cmd_list = []
 if do_config or adjust_cfg:
-if self.mrproper:
-result = self.make(commit, brd, 'mrproper', cwd,
-'mrproper', *args, env=env)
-config_out.write(result.combined)
-cmd_list.append([self.builder.gnu_make, 'mrproper',
- *args])
-result = self.make(commit, brd, 'config', cwd,
-*(args + config_args), env=env)
-cmd_list.append([self.builder.gnu_make] + args +
-config_args)
-config_out.write(result.combined)
+result = self._reconfigure(
+commit, brd, cwd, args, env, config_args, config_out,
+cmd_list)
 do_config = False   # No need to configure next time
 if adjust_cfg:
 cfgutil.adjust_cfg_file(cfg_file, adjust_cfg)
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 47/58] buildman: Convert config_out to string IO

[Simon Glass]

This is probably a little more efficient and it allows passing the object
to another function to write data. Convert config_out to use a string I/O
device.

Signed-off-by: Simon Glass 
---

 tools/buildman/builderthread.py | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index 42af5197dace..9c2938d1b071 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -10,6 +10,7 @@ based on the jobs provided.
 
 import errno
 import glob
+import io
 import os
 import shutil
 import sys
@@ -292,7 +293,7 @@ class BuilderThread(threading.Thread):
 args, cwd, src_dir = self._build_args(brd, out_dir, 
out_rel_dir,
   work_dir, commit_upto)
 config_args = [f'{brd.target}_defconfig']
-config_out = ''
+config_out = io.StringIO()
 
 # Remove any output targets. Since we use a build directory 
that
 # was previously used by another board, it may have produced an
@@ -311,14 +312,14 @@ class BuilderThread(threading.Thread):
 if self.mrproper:
 result = self.make(commit, brd, 'mrproper', cwd,
 'mrproper', *args, env=env)
-config_out += result.combined
+config_out.write(result.combined)
 cmd_list.append([self.builder.gnu_make, 'mrproper',
  *args])
 result = self.make(commit, brd, 'config', cwd,
 *(args + config_args), env=env)
 cmd_list.append([self.builder.gnu_make] + args +
 config_args)
-config_out += result.combined
+config_out.write(result.combined)
 do_config = False   # No need to configure next time
 if adjust_cfg:
 cfgutil.adjust_cfg_file(cfg_file, adjust_cfg)
@@ -340,7 +341,7 @@ class BuilderThread(threading.Thread):
 result.return_code = 1
 result.stderr = result.stderr.replace(src_dir + '/', '')
 if self.builder.verbose_build:
-result.stdout = config_out + result.stdout
+result.stdout = config_out.getvalue() + result.stdout
 result.cmd_list = cmd_list
 else:
 result.return_code = 1
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 42/58] buildman: Correct invalid use of out_dir variable

[Simon Glass]

This variable has a different meaning in the outer scrop. Use a different
name to avoid confusion, or bugs.

Signed-off-by: Simon Glass 
---

 tools/buildman/builderthread.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index 45ae6edf9f48..f110137e8f6e 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -246,8 +246,8 @@ class BuilderThread(threading.Thread):
 #
 # Symlinks can confuse U-Boot's Makefile since
 # we may use '..' in our path, so remove them.
-out_dir = os.path.realpath(out_dir)
-args.append(f'O={out_dir}')
+real_dir = os.path.realpath(out_dir)
+args.append(f'O={real_dir}')
 cwd = None
 src_dir = os.getcwd()
 else:
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 46/58] buildman: Move more things into _build_args()

[Simon Glass]

Move more of the argument-building code into this function. Fix a missing
assignment for out_rel_dir too.

Rename the function since it now builds all the arguments.

Signed-off-by: Simon Glass 
---

 tools/buildman/builderthread.py | 55 -
 1 file changed, 34 insertions(+), 21 deletions(-)

diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index e7d9a29d03ef..42af5197dace 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -137,13 +137,40 @@ class BuilderThread(threading.Thread):
 return self.builder.do_make(commit, brd, stage, cwd, *args,
 **kwargs)
 
-def _build_args(self, args, brd):
+def _build_args(self, brd, out_dir, out_rel_dir, work_dir, commit_upto):
 """Set up arguments to the args list based on the settings
 
 Args:
-args (list of str): List of string arguments to add things to
 brd (Board): Board to create arguments for
+out_dir (str): Path to output directory containing the files
+out_rel_dir (str): Output directory relative to the current dir
+work_dir (str): Directory to which the source will be checked out
+commit_upto (int): Commit number to build (0...n-1)
+
+Returns:
+tuple:
+list of str: Arguments to pass to make
+str: Current working directory, or None if no commit
+str: Source directory (typically the work directory)
 """
+args = []
+cwd = work_dir
+src_dir = os.path.realpath(work_dir)
+if not self.builder.in_tree:
+if commit_upto is None:
+# In this case we are building in the original source directory
+# (i.e. the current directory where buildman is invoked. The
+# output directory is set to this thread's selected work
+# directory.
+#
+# Symlinks can confuse U-Boot's Makefile since we may use '..'
+# in our path, so remove them.
+real_dir = os.path.realpath(out_dir)
+args.append(f'O={real_dir}')
+cwd = None
+src_dir = os.getcwd()
+else:
+args.append(f'O={out_rel_dir}')
 if self.builder.verbose_build:
 args.append('V=1')
 else:
@@ -161,6 +188,7 @@ class BuilderThread(threading.Thread):
 args.append('SOURCE_DATE_EPOCH=0')
 args.extend(self.builder.toolchains.GetMakeArguments(brd))
 args.extend(self.toolchain.MakeArgs())
+return args, cwd, src_dir
 
 def run_commit(self, commit_upto, brd, work_dir, do_config, config_only,
   force_build, force_build_failures, work_in_output,
@@ -199,6 +227,7 @@ class BuilderThread(threading.Thread):
 result = command.CommandResult()
 result.return_code = 0
 if work_in_output or self.builder.in_tree:
+out_rel_dir = None
 out_dir = work_dir
 else:
 if self.per_board_out_dir:
@@ -259,25 +288,9 @@ class BuilderThread(threading.Thread):
 # Set up the environment and command line
 env = self.toolchain.MakeEnvironment(self.builder.full_path)
 mkdir(out_dir)
-args = []
-cwd = work_dir
-src_dir = os.path.realpath(work_dir)
-if not self.builder.in_tree:
-if commit_upto is None:
-# In this case we are building in the original source
-# directory (i.e. the current directory where buildman
-# is invoked. The output directory is set to this
-# thread's selected work directory.
-#
-# Symlinks can confuse U-Boot's Makefile since
-# we may use '..' in our path, so remove them.
-real_dir = os.path.realpath(out_dir)
-args.append(f'O={real_dir}')
-cwd = None
-src_dir = os.getcwd()
-else:
-args.append(f'O={out_rel_dir}')
-self._build_args(args, brd)
+
+args, cwd, src_dir = self._build_args(brd, out_dir, 
out_rel_dir,
+  work_dir, commit_upto)
 config_args = [f'{brd.target}_defconfig']
 config_out = ''
 
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 45/58] buildman: Move setting of toolchain arguments to _build_args()

[Simon Glass]

Move a few more pieces to this new function.

Signed-off-by: Simon Glass 
---

 tools/buildman/builderthread.py | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index 47ebf4dcdd90..e7d9a29d03ef 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -137,11 +137,12 @@ class BuilderThread(threading.Thread):
 return self.builder.do_make(commit, brd, stage, cwd, *args,
 **kwargs)
 
-def _build_args(self, args):
+def _build_args(self, args, brd):
 """Set up arguments to the args list based on the settings
 
 Args:
 args (list of str): List of string arguments to add things to
+brd (Board): Board to create arguments for
 """
 if self.builder.verbose_build:
 args.append('V=1')
@@ -158,6 +159,8 @@ class BuilderThread(threading.Thread):
 args.append('NO_LTO=1')
 if self.builder.reproducible_builds:
 args.append('SOURCE_DATE_EPOCH=0')
+args.extend(self.builder.toolchains.GetMakeArguments(brd))
+args.extend(self.toolchain.MakeArgs())
 
 def run_commit(self, commit_upto, brd, work_dir, do_config, config_only,
   force_build, force_build_failures, work_in_output,
@@ -274,11 +277,9 @@ class BuilderThread(threading.Thread):
 src_dir = os.getcwd()
 else:
 args.append(f'O={out_rel_dir}')
-self._build_args(args)
+self._build_args(args, brd)
 config_args = [f'{brd.target}_defconfig']
 config_out = ''
-args.extend(self.builder.toolchains.GetMakeArguments(brd))
-args.extend(self.toolchain.MakeArgs())
 
 # Remove any output targets. Since we use a build directory 
that
 # was previously used by another board, it may have produced an
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 39/58] buildman: Convert camel case in builderthread.py

[Simon Glass]

Convert this file to snake case and update all files which use it.

Signed-off-by: Simon Glass 
---

 tools/buildman/builder.py   |  8 +++---
 tools/buildman/builderthread.py | 50 -
 2 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/tools/buildman/builder.py b/tools/buildman/builder.py
index 80c05fbd1e73..4cc266ff3049 100644
--- a/tools/buildman/builder.py
+++ b/tools/buildman/builder.py
@@ -1648,7 +1648,7 @@ class Builder:
'worktree' to set up a git worktree
 """
 thread_dir = self.get_thread_dir(thread_num)
-builderthread.Mkdir(thread_dir)
+builderthread.mkdir(thread_dir)
 git_dir = os.path.join(thread_dir, '.git')
 
 # Create a worktree or a git repo clone for this thread if it
@@ -1696,7 +1696,7 @@ class Builder:
 work
 setup_git: True to set up a git worktree or a git clone
 """
-builderthread.Mkdir(self._working_dir)
+builderthread.mkdir(self._working_dir)
 if setup_git and self.git_dir:
 src_dir = os.path.abspath(self.git_dir)
 if gitutil.check_worktree_is_available(src_dir):
@@ -1772,7 +1772,7 @@ class Builder:
 self._verbose = verbose
 
 self.reset_result_summary(board_selected)
-builderthread.Mkdir(self.base_dir, parents = True)
+builderthread.mkdir(self.base_dir, parents = True)
 self._prepare_working_space(min(self.num_threads, len(board_selected)),
 commits is not None)
 self._prepare_output_space()
@@ -1793,7 +1793,7 @@ class Builder:
 if self.num_threads:
 self.queue.put(job)
 else:
-self._single_builder.RunJob(job)
+self._single_builder.run_job(job)
 
 if self.num_threads:
 term = threading.Thread(target=self.queue.join)
diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index 5f1200ae890f..3f52d95fed9f 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -16,7 +16,7 @@ from u_boot_pylib import command
 RETURN_CODE_RETRY = -1
 BASE_ELF_FILENAMES = ['u-boot', 'spl/u-boot-spl', 'tpl/u-boot-tpl']
 
-def Mkdir(dirname, parents = False):
+def mkdir(dirname, parents = False):
 """Make a directory if it doesn't already exist.
 
 Args:
@@ -108,7 +108,7 @@ class BuilderThread(threading.Thread):
 self.per_board_out_dir = per_board_out_dir
 self.test_exception = test_exception
 
-def Make(self, commit, brd, stage, cwd, *args, **kwargs):
+def make(self, commit, brd, stage, cwd, *args, **kwargs):
 """Run 'make' on a particular commit and board.
 
 The source code will already be checked out, so the 'commit'
@@ -130,7 +130,7 @@ class BuilderThread(threading.Thread):
 return self.builder.do_make(commit, brd, stage, cwd, *args,
 **kwargs)
 
-def RunCommit(self, commit_upto, brd, work_dir, do_config, config_only,
+def run_commit(self, commit_upto, brd, work_dir, do_config, config_only,
   force_build, force_build_failures, work_in_output,
   adjust_cfg):
 """Build a particular commit.
@@ -163,7 +163,7 @@ class BuilderThread(threading.Thread):
 - boolean indicating whether 'make config' is still needed
 """
 # Create a default result - it will be overwritte by the call to
-# self.Make() below, in the event that we do a build.
+# self.make() below, in the event that we do a build.
 result = command.CommandResult()
 result.return_code = 0
 if work_in_output or self.builder.in_tree:
@@ -226,7 +226,7 @@ class BuilderThread(threading.Thread):
 
 # Set up the environment and command line
 env = self.toolchain.MakeEnvironment(self.builder.full_path)
-Mkdir(out_dir)
+mkdir(out_dir)
 args = []
 cwd = work_dir
 src_dir = os.path.realpath(work_dir)
@@ -282,12 +282,12 @@ class BuilderThread(threading.Thread):
 if do_config or adjust_cfg:
 config_out = ''
 if self.mrproper:
-result = self.Make(commit, brd, 'mrproper', cwd,
+result = self.make(commit, brd, 'mrproper', cwd,
 'mrproper', *args, env=env)
 config_out += result.combined
 cmd_list.append([self.builder.gnu_make, 'mrproper',
  *args])
-result = self.Make(commit, brd, 'config', cwd,
+result = self.make(commit, brd, 'config', cwd,
 *(args + config_args), env=env)
 cmd_list.append([self.builder.gnu_make] + args +
 

[PATCH 41/58] buildman: Export _get_output_dir() to avoid warnings

[Simon Glass]

Make this a public memory since it is used outside the class.

Signed-off-by: Simon Glass 
---

 tools/buildman/builder.py   | 8 
 tools/buildman/builderthread.py | 2 +-
 tools/buildman/test.py  | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/buildman/builder.py b/tools/buildman/builder.py
index 4cc266ff3049..ecbd368c47a5 100644
--- a/tools/buildman/builder.py
+++ b/tools/buildman/builder.py
@@ -565,7 +565,7 @@ class Builder:
 terminal.print_clear()
 tprint(line, newline=False, limit_to_line=True)
 
-def _get_output_dir(self, commit_upto):
+def get_output_dir(self, commit_upto):
 """Get the name of the output directory for a commit number
 
 The output directory is typically ...//.
@@ -598,7 +598,7 @@ class Builder:
 commit_upto: Commit number to use (0..self.count-1)
 target: Target name
 """
-output_dir = self._get_output_dir(commit_upto)
+output_dir = self.get_output_dir(commit_upto)
 if self.work_in_output:
 return output_dir
 return os.path.join(output_dir, target)
@@ -1717,7 +1717,7 @@ class Builder:
 
 Figure out what needs to be deleted in the output directory before it
 can be used. We only delete old buildman directories which have the
-expected name pattern. See _get_output_dir().
+expected name pattern. See get_output_dir().
 
 Returns:
 List of full paths of directories to remove
@@ -1726,7 +1726,7 @@ class Builder:
 return
 dir_list = []
 for commit_upto in range(self.commit_count):
-dir_list.append(self._get_output_dir(commit_upto))
+dir_list.append(self.get_output_dir(commit_upto))
 
 to_remove = []
 for dirname in glob.glob(os.path.join(self.base_dir, '*')):
diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index 179fc477c008..45ae6edf9f48 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -351,7 +351,7 @@ class BuilderThread(threading.Thread):
 return
 
 # Write the output and stderr
-output_dir = self.builder._get_output_dir(result.commit_upto)
+output_dir = self.builder.get_output_dir(result.commit_upto)
 mkdir(output_dir)
 build_dir = self.builder.get_build_dir(result.commit_upto,
 result.brd.target)
diff --git a/tools/buildman/test.py b/tools/buildman/test.py
index 7ac67260c337..bdd3d84158a6 100644
--- a/tools/buildman/test.py
+++ b/tools/buildman/test.py
@@ -528,7 +528,7 @@ class TestBuild(unittest.TestCase):
'sandbox']),
  ({'all': ['board4'], 'sandbox': ['board4']}, []))
 def CheckDirs(self, build, dirname):
-self.assertEqual('base%s' % dirname, build._get_output_dir(1))
+self.assertEqual('base%s' % dirname, build.get_output_dir(1))
 self.assertEqual('base%s/fred' % dirname,
  build.get_build_dir(1, 'fred'))
 self.assertEqual('base%s/fred/done' % dirname,
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 40/58] buildman: Correct most pylint warnings in builderthread

[Simon Glass]

Fix the easy warnings in this file.

Signed-off-by: Simon Glass 
---

 tools/buildman/builderthread.py | 95 ++---
 1 file changed, 51 insertions(+), 44 deletions(-)

diff --git a/tools/buildman/builderthread.py b/tools/buildman/builderthread.py
index 3f52d95fed9f..179fc477c008 100644
--- a/tools/buildman/builderthread.py
+++ b/tools/buildman/builderthread.py
@@ -2,6 +2,12 @@
 # Copyright (c) 2014 Google, Inc
 #
 
+"""Implementation the bulider threads
+
+This module provides the BuilderThread class, which handles calling the builder
+based on the jobs provided.
+"""
+
 import errno
 import glob
 import os
@@ -30,12 +36,12 @@ def mkdir(dirname, parents = False):
 except OSError as err:
 if err.errno == errno.EEXIST:
 if os.path.realpath('.') == os.path.realpath(dirname):
-print("Cannot create the current working directory '%s'!" % 
dirname)
+print(f"Cannot create the current working directory 
'{dirname}'!")
 sys.exit(1)
-pass
 else:
 raise
 
+# pylint: disable=R0903
 class BuilderJob:
 """Holds information about a job to be performed by a thread
 
@@ -107,6 +113,7 @@ class BuilderThread(threading.Thread):
 self.mrproper = mrproper
 self.per_board_out_dir = per_board_out_dir
 self.test_exception = test_exception
+self.toolchain = None
 
 def make(self, commit, brd, stage, cwd, *args, **kwargs):
 """Run 'make' on a particular commit and board.
@@ -182,9 +189,9 @@ class BuilderThread(threading.Thread):
 not result.already_done)
 if result.already_done:
 # Get the return code from that build and use it
-with open(done_file, 'r') as fd:
+with open(done_file, 'r', encoding='utf-8') as outf:
 try:
-result.return_code = int(fd.readline())
+result.return_code = int(outf.readline())
 except ValueError:
 # The file may be empty due to running out of disk space.
 # Try a rebuild
@@ -240,11 +247,11 @@ class BuilderThread(threading.Thread):
 # Symlinks can confuse U-Boot's Makefile since
 # we may use '..' in our path, so remove them.
 out_dir = os.path.realpath(out_dir)
-args.append('O=%s' % out_dir)
+args.append(f'O={out_dir}')
 cwd = None
 src_dir = os.getcwd()
 else:
-args.append('O=%s' % out_rel_dir)
+args.append(f'O={out_rel_dir}')
 if self.builder.verbose_build:
 args.append('V=1')
 else:
@@ -260,7 +267,7 @@ class BuilderThread(threading.Thread):
 args.append('NO_LTO=1')
 if self.builder.reproducible_builds:
 args.append('SOURCE_DATE_EPOCH=0')
-config_args = ['%s_defconfig' % brd.target]
+config_args = [f'{brd.target}_defconfig']
 config_out = ''
 args.extend(self.builder.toolchains.GetMakeArguments(brd))
 args.extend(self.toolchain.MakeArgs())
@@ -270,7 +277,6 @@ class BuilderThread(threading.Thread):
 # SPL image. If we don't remove it (i.e. see do_config and
 # self.mrproper below) then it will appear to be the output of
 # this build, even if it does not produce SPL images.
-build_dir = self.builder.get_build_dir(commit_upto, brd.target)
 for elf in BASE_ELF_FILENAMES:
 fname = os.path.join(out_dir, elf)
 if os.path.exists(fname):
@@ -317,7 +323,7 @@ class BuilderThread(threading.Thread):
 result.cmd_list = cmd_list
 else:
 result.return_code = 1
-result.stderr = 'No tool chain for %s\n' % brd.arch
+result.stderr = f'No tool chain for {brd.arch}\n'
 result.already_done = False
 
 result.toolchain = self.toolchain
@@ -352,15 +358,15 @@ class BuilderThread(threading.Thread):
 mkdir(build_dir)
 
 outfile = os.path.join(build_dir, 'log')
-with open(outfile, 'w') as fd:
+with open(outfile, 'w', encoding='utf-8') as outf:
 if result.stdout:
-fd.write(result.stdout)
+outf.write(result.stdout)
 
 errfile = self.builder.get_err_file(result.commit_upto,
 result.brd.target)
 if result.stderr:
-with open(errfile, 'w') as fd:
-fd.write(result.stderr)
+with open(errfile, 'w', encoding='utf-8') as outf:
+outf.write(result.stderr)
 elif os.path.exists(errfile):
 

[PATCH 37/58] buildman: Convert camel case in builder.py

[Simon Glass]

Convert this file to snake case and update all files which use it.

Signed-off-by: Simon Glass 
---

 tools/buildman/builder.py   | 228 
 tools/buildman/builderthread.py |  26 ++--
 tools/buildman/control.py   |  13 +-
 tools/buildman/func_test.py |   2 +-
 tools/buildman/test.py  |  22 +--
 5 files changed, 145 insertions(+), 146 deletions(-)

diff --git a/tools/buildman/builder.py b/tools/buildman/builder.py
index 620b7b8c31a8..80c05fbd1e73 100644
--- a/tools/buildman/builder.py
+++ b/tools/buildman/builder.py
@@ -134,7 +134,7 @@ class Config:
 for fname in config_filename:
 self.config[fname] = {}
 
-def Add(self, fname, key, value):
+def add(self, fname, key, value):
 self.config[fname][key] = value
 
 def __hash__(self):
@@ -151,7 +151,7 @@ class Environment:
 self.target = target
 self.environment = {}
 
-def Add(self, key, value):
+def add(self, key, value):
 self.environment[key] = value
 
 class Builder:
@@ -315,7 +315,7 @@ class Builder:
 else:
 self._working_dir = os.path.join(base_dir, '.bm-work')
 self.threads = []
-self.do_make = make_func or self.Make
+self.do_make = make_func or self.make
 self.gnu_make = gnu_make
 self.checkout = checkout
 self.num_threads = num_threads
@@ -401,7 +401,7 @@ class Builder:
 def signal_handler(self, signal, frame):
 sys.exit(1)
 
-def SetDisplayOptions(self, show_errors=False, show_sizes=False,
+def set_display_options(self, show_errors=False, show_sizes=False,
   show_detail=False, show_bloat=False,
   list_error_boards=False, show_config=False,
   show_environment=False, filter_dtb_warnings=False,
@@ -434,7 +434,7 @@ class Builder:
 self._filter_migration_warnings = filter_migration_warnings
 self._ide = ide
 
-def _AddTimestamp(self):
+def _add_timestamp(self):
 """Add a new timestamp to the list and record the build period.
 
 The build period is the length of time taken to perform a single
@@ -463,14 +463,14 @@ class Builder:
 self._timestamps.popleft()
 count -= 1
 
-def SelectCommit(self, commit, checkout=True):
+def select_commit(self, commit, checkout=True):
 """Checkout the selected commit for this build
 """
 self.commit = commit
 if checkout and self.checkout:
 gitutil.checkout(commit.hash)
 
-def Make(self, commit, brd, stage, cwd, *args, **kwargs):
+def make(self, commit, brd, stage, cwd, *args, **kwargs):
 """Run make
 
 Args:
@@ -515,7 +515,7 @@ class Builder:
 result.combined = '%s\n' % (' '.join(cmd)) + result.combined
 return result
 
-def ProcessResult(self, result):
+def process_result(self, result):
 """Process the result of a build, showing progress information
 
 Args:
@@ -536,8 +536,8 @@ class Builder:
 if self._verbose:
 terminal.print_clear()
 boards_selected = {target : result.brd}
-self.ResetResultSummary(boards_selected)
-self.ProduceResultSummary(result.commit_upto, self.commits,
+self.reset_result_summary(boards_selected)
+self.produce_result_summary(result.commit_upto, self.commits,
   boards_selected)
 else:
 target = '(starting)'
@@ -556,7 +556,7 @@ class Builder:
 line += ' ' * 8
 
 # Add our current completion time estimate
-self._AddTimestamp()
+self._add_timestamp()
 if self._complete_delay:
 line += '%s  : ' % self._complete_delay
 
@@ -565,7 +565,7 @@ class Builder:
 terminal.print_clear()
 tprint(line, newline=False, limit_to_line=True)
 
-def _GetOutputDir(self, commit_upto):
+def _get_output_dir(self, commit_upto):
 """Get the name of the output directory for a commit number
 
 The output directory is typically ...//.
@@ -580,7 +580,7 @@ class Builder:
 if self.commits:
 commit = self.commits[commit_upto]
 subject = commit.subject.translate(trans_valid_chars)
-# See _GetOutputSpaceRemovals() which parses this name
+# See _get_output_space_removals() which parses this name
 commit_dir = ('%02d_g%s_%s' % (commit_upto + 1,
 commit.hash, subject[:20]))
 elif not self.no_subdirs:
@@ -589,7 +589,7 @@ class Builder:
 return self.base_dir
 return os.path.join(self.base_dir, commit_dir)
 
-def GetBuildDir(self, commit_upto, target):
+def get_build_dir(self, commit_upto, target):
 """Get the name of the build directory for a commit number
 
 The build directory 

[PATCH 35/58] buildman: Convert to argparse

[Simon Glass]

Use argparse to parse the arguments, since OptionParser is deprecated now.

Signed-off-by: Simon Glass 
---

 tools/buildman/cmdline.py   | 125 
 tools/buildman/control.py   | 140 ++--
 tools/buildman/func_test.py |   6 +-
 tools/buildman/main.py  |  16 ++---
 4 files changed, 145 insertions(+), 142 deletions(-)

diff --git a/tools/buildman/cmdline.py b/tools/buildman/cmdline.py
index 91571a5adb1d..6b9c1db14af7 100644
--- a/tools/buildman/cmdline.py
+++ b/tools/buildman/cmdline.py
@@ -7,7 +7,7 @@
 This creates the argument parser and uses it to parse the arguments passed in
 """
 
-from optparse import OptionParser
+import argparse
 import os
 import pathlib
 
@@ -22,135 +22,138 @@ def parse_args():
 options: command line options
 args: command lin arguments
 """
-parser = OptionParser()
-parser.add_option('-a', '--adjust-cfg', type=str, action='append',
+epilog = """ [list of target/arch/cpu/board/vendor/soc to build]
+
+Build U-Boot for all commits in a branch. Use -n to do a dry run"""
+
+parser = argparse.ArgumentParser(epilog=epilog)
+parser.add_argument('-a', '--adjust-cfg', type=str, action='append',
   help='Adjust the Kconfig settings in .config before building')
-parser.add_option('-A', '--print-prefix', action='store_true',
+parser.add_argument('-A', '--print-prefix', action='store_true',
   help='Print the tool-chain prefix for a board (CROSS_COMPILE=)')
-parser.add_option('-b', '--branch', type='string',
+parser.add_argument('-b', '--branch', type=str,
   help='Branch name to build, or range of commits to build')
-parser.add_option('-B', '--bloat', dest='show_bloat',
+parser.add_argument('-B', '--bloat', dest='show_bloat',
   action='store_true', default=False,
   help='Show changes in function code size for each board')
-parser.add_option('--boards', type='string', action='append',
+parser.add_argument('--boards', type=str, action='append',
   help='List of board names to build separated by comma')
-parser.add_option('-c', '--count', dest='count', type='int',
+parser.add_argument('-c', '--count', dest='count', type=int,
   default=-1, help='Run build on the top n commits')
-parser.add_option('-C', '--force-reconfig', dest='force_reconfig',
+parser.add_argument('-C', '--force-reconfig', dest='force_reconfig',
   action='store_true', default=False,
   help='Reconfigure for every commit (disable incremental build)')
-parser.add_option('-d', '--detail', dest='show_detail',
+parser.add_argument('-d', '--detail', dest='show_detail',
   action='store_true', default=False,
   help='Show detailed size delta for each board in the -S summary')
-parser.add_option('-D', '--config-only', action='store_true', 
default=False,
+parser.add_argument('-D', '--config-only', action='store_true',
+default=False,
   help="Don't build, just configure each commit")
-parser.add_option('--debug', action='store_true',
+parser.add_argument('--debug', action='store_true',
 help='Enabling debugging (provides a full traceback on error)')
-parser.add_option('-e', '--show_errors', action='store_true',
+parser.add_argument('-e', '--show_errors', action='store_true',
   default=False, help='Show errors and warnings')
-parser.add_option('-E', '--warnings-as-errors', action='store_true',
+parser.add_argument('-E', '--warnings-as-errors', action='store_true',
   default=False, help='Treat all compiler warnings as errors')
-parser.add_option('-f', '--force-build', dest='force_build',
+parser.add_argument('-f', '--force-build', dest='force_build',
   action='store_true', default=False,
   help='Force build of boards even if already built')
-parser.add_option('-F', '--force-build-failures', 
dest='force_build_failures',
+parser.add_argument('-F', '--force-build-failures', 
dest='force_build_failures',
   action='store_true', default=False,
   help='Force build of previously-failed build')
-parser.add_option('--fetch-arch', type='string',
+parser.add_argument('--fetch-arch', type=str,
   help="Fetch a toolchain for architecture FETCH_ARCH ('list' to 
list)."
   ' You can also fetch several toolchains separate by comma, or'
   " 'all' to download all")
-parser.add_option('-g', '--git', type='string',
+parser.add_argument('-g', '--git', type=str,
   help='Git repo containing branch to build', default='.')
-parser.add_option('-G', '--config-file', type='string',
+parser.add_argument('-G', '--config-file', type=str,
   help='Path to buildman config file', default='')
-parser.add_option('-H', '--full-help', action='store_true', 
dest='full_help',
+

[PATCH 38/58] buildman: Split parser creation in two

[Simon Glass]

Split this into two functions to avoid a warning about too many
statements.

Signed-off-by: Simon Glass 
---

 tools/buildman/cmdline.py | 44 +--
 1 file changed, 33 insertions(+), 11 deletions(-)

diff --git a/tools/buildman/cmdline.py b/tools/buildman/cmdline.py
index 6b9c1db14af7..e295c7aef1a0 100644
--- a/tools/buildman/cmdline.py
+++ b/tools/buildman/cmdline.py
@@ -14,19 +14,14 @@ import pathlib
 BUILDMAN_DIR = pathlib.Path(__file__).parent
 HAS_TESTS = os.path.exists(BUILDMAN_DIR / "test.py")
 
-def parse_args():
-"""Parse command line arguments from sys.argv[]
-
-Returns:
-tuple containing:
-options: command line options
-args: command lin arguments
-"""
-epilog = """ [list of target/arch/cpu/board/vendor/soc to build]
+def add_upto_m(parser):
+"""Add arguments up to 'M'
 
-Build U-Boot for all commits in a branch. Use -n to do a dry run"""
+Args:
+parser (ArgumentParser): Parse to add to
 
-parser = argparse.ArgumentParser(epilog=epilog)
+This is split out to avoid having too many statements in one function
+"""
 parser.add_argument('-a', '--adjust-cfg', type=str, action='append',
   help='Adjust the Kconfig settings in .config before building')
 parser.add_argument('-A', '--print-prefix', action='store_true',
@@ -104,6 +99,16 @@ def parse_args():
 parser.add_argument('-N', '--no-subdirs', action='store_true', 
dest='no_subdirs',
   default=False,
   help="Don't create subdirectories when building current source for a 
single board")
+
+
+def add_after_m(parser):
+"""Add arguments after 'M'
+
+Args:
+parser (ArgumentParser): Parse to add to
+
+This is split out to avoid having too many statements in one function
+"""
 parser.add_argument('-o', '--output-dir', type=str, dest='output_dir',
   help='Directory where all builds happen and buildman has its 
workspace (default is ../)')
 parser.add_argument('-O', '--override-toolchain', type=str,
@@ -153,6 +158,23 @@ def parse_args():
 parser.add_argument('-Y', '--filter-migration-warnings', 
action='store_true',
   default=False,
   help='Filter out migration warnings from output')
+
+
+def parse_args():
+"""Parse command line arguments from sys.argv[]
+
+Returns:
+tuple containing:
+options: command line options
+args: command lin arguments
+"""
+epilog = """ [list of target/arch/cpu/board/vendor/soc to build]
+
+Build U-Boot for all commits in a branch. Use -n to do a dry run"""
+
+parser = argparse.ArgumentParser(epilog=epilog)
+add_upto_m(parser)
+add_after_m(parser)
 parser.add_argument('terms', type=str, nargs='*',
 help='Board / SoC names to build')
 
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 36/58] buildman: Convert camel case in bsettings.py

[Simon Glass]

Convert this file to snake case and update all files which use it.

Signed-off-by: Simon Glass 
---

 tools/buildman/bsettings.py | 14 +++---
 tools/buildman/control.py   |  2 +-
 tools/buildman/func_test.py | 12 ++--
 tools/buildman/main.py  |  2 +-
 tools/buildman/test.py  |  4 ++--
 tools/buildman/toolchain.py | 14 +++---
 tools/moveconfig.py |  2 +-
 7 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/tools/buildman/bsettings.py b/tools/buildman/bsettings.py
index 0eb894a558c1..612ec0c28464 100644
--- a/tools/buildman/bsettings.py
+++ b/tools/buildman/bsettings.py
@@ -7,7 +7,7 @@ import io
 
 config_fname = None
 
-def Setup(fname=''):
+def setup(fname=''):
 """Set up the buildman settings module by reading config files
 
 Args:
@@ -23,15 +23,15 @@ def Setup(fname=''):
 config_fname = '%s/.buildman' % os.getenv('HOME')
 if not os.path.exists(config_fname):
 print('No config file found ~/.buildman\nCreating one...\n')
-CreateBuildmanConfigFile(config_fname)
+create_buildman_config_file(config_fname)
 print('To install tool chains, please use the --fetch-arch option')
 if config_fname:
 settings.read(config_fname)
 
-def AddFile(data):
+def add_file(data):
 settings.readfp(io.StringIO(data))
 
-def GetItems(section):
+def get_items(section):
 """Get the items from a section of the config.
 
 Args:
@@ -47,7 +47,7 @@ def GetItems(section):
 except:
 raise
 
-def GetGlobalItemValue(name):
+def get_global_item_value(name):
 """Get an item from the 'global' section of the config.
 
 Args:
@@ -58,7 +58,7 @@ def GetGlobalItemValue(name):
 """
 return settings.get('global', name, fallback=None)
 
-def SetItem(section, tag, value):
+def set_item(section, tag, value):
 """Set an item and write it back to the settings file"""
 global settings
 global config_fname
@@ -68,7 +68,7 @@ def SetItem(section, tag, value):
 with open(config_fname, 'w') as fd:
 settings.write(fd)
 
-def CreateBuildmanConfigFile(config_fname):
+def create_buildman_config_file(config_fname):
 """Creates a new config file with no tool chain information.
 
 Args:
diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index fa0fd616fe2d..d51b5fb0ad4a 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -157,7 +157,7 @@ def get_allow_missing(opt_allow, opt_no_allow, 
num_selected, has_branch):
 external blobs are used
 """
 allow_missing = False
-am_setting = bsettings.GetGlobalItemValue('allow-missing')
+am_setting = bsettings.get_global_item_value('allow-missing')
 if am_setting:
 if am_setting == 'always':
 allow_missing = True
diff --git a/tools/buildman/func_test.py b/tools/buildman/func_test.py
index 5cfe99bf5f6b..5646b59f162d 100644
--- a/tools/buildman/func_test.py
+++ b/tools/buildman/func_test.py
@@ -184,8 +184,8 @@ class TestFunctional(unittest.TestCase):
 self._buildman_pathname = sys.argv[0]
 self._buildman_dir = os.path.dirname(os.path.realpath(sys.argv[0]))
 command.test_result = self._HandleCommand
-bsettings.Setup(None)
-bsettings.AddFile(settings_data)
+bsettings.setup(None)
+bsettings.add_file(settings_data)
 self.setupToolchains()
 self._toolchains.Add('arm-gcc', test=False)
 self._toolchains.Add('powerpc-gcc', test=False)
@@ -691,7 +691,7 @@ Some images are invalid'''
 
 def testBlobSettingsAlways(self):
 """Test the 'always' policy"""
-bsettings.SetItem('global', 'allow-missing', 'always')
+bsettings.set_item('global', 'allow-missing', 'always')
 self.assertEqual(True,
  control.get_allow_missing(False, False, 1, False))
 self.assertEqual(False,
@@ -699,7 +699,7 @@ Some images are invalid'''
 
 def testBlobSettingsBranch(self):
 """Test the 'branch' policy"""
-bsettings.SetItem('global', 'allow-missing', 'branch')
+bsettings.set_item('global', 'allow-missing', 'branch')
 self.assertEqual(False,
  control.get_allow_missing(False, False, 1, False))
 self.assertEqual(True,
@@ -709,7 +709,7 @@ Some images are invalid'''
 
 def testBlobSettingsMultiple(self):
 """Test the 'multiple' policy"""
-bsettings.SetItem('global', 'allow-missing', 'multiple')
+bsettings.set_item('global', 'allow-missing', 'multiple')
 self.assertEqual(False,
  control.get_allow_missing(False, False, 1, False))
 self.assertEqual(True,
@@ -719,7 +719,7 @@ Some images are invalid'''
 
 def testBlobSettingsBranchMultiple(self):
 """Test the 'branch multiple' policy"""
-bsettings.SetItem('global', 'allow-missing', 'branch multiple')
+

[PATCH 34/58] buildman: Add a test for --boards

[Simon Glass]

Add a simple functional test for the --boards option. Fix the example in
the docs while we are here. Also improve the docs for Builder.count so it
is clearer what it contains.

Signed-off-by: Simon Glass 
---

 tools/buildman/builder.py   |  3 ++-
 tools/buildman/buildman.rst |  2 +-
 tools/buildman/func_test.py | 11 +++
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/tools/buildman/builder.py b/tools/buildman/builder.py
index cb3628a8a085..620b7b8c31a8 100644
--- a/tools/buildman/builder.py
+++ b/tools/buildman/builder.py
@@ -163,7 +163,8 @@ class Builder:
 checkout: True to check out source, False to skip that step.
 This is used for testing.
 col: terminal.Color() object
-count: Number of commits to build
+count: Total number of commits to build, which is the number of commits
+multiplied by the number of boards
 do_make: Method to call to invoke Make
 fail: Number of builds that failed due to error
 force_build: Force building even if a build already exists
diff --git a/tools/buildman/buildman.rst b/tools/buildman/buildman.rst
index b28aea477dfe..d8c3957097e8 100644
--- a/tools/buildman/buildman.rst
+++ b/tools/buildman/buildman.rst
@@ -159,7 +159,7 @@ on the command line:
 
 .. code-block:: bash
 
-  buildman --boards sandbox,snow --boards
+  buildman --boards sandbox,snow --boards firefly-rk3399
 
 It is convenient to use the -n option to see what will be built based on
 the subset given. Use -v as well to get an actual list of boards.
diff --git a/tools/buildman/func_test.py b/tools/buildman/func_test.py
index ad48e3b2159f..966ad178a902 100644
--- a/tools/buildman/func_test.py
+++ b/tools/buildman/func_test.py
@@ -792,3 +792,14 @@ CONFIG_LOCALVERSION=y
 os.remove(outfile)
 result = self._RunControl('-R', outfile, brds=None, get_builder=False)
 self.assertTrue(os.path.exists(outfile))
+
+def testSingleBoards(self):
+"""Test building single boards"""
+self._RunControl('--boards', 'board1')
+self.assertEqual(1, self._builder.count)
+
+self._RunControl('--boards', 'board1', '--boards', 'board2')
+self.assertEqual(2, self._builder.count)
+
+self._RunControl('--boards', 'board1,board2', '--boards', 'board4')
+self.assertEqual(3, self._builder.count)
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 33/58] buildman: Correct most pylint warnings in cmdline

[Simon Glass]

Tidu up warnings in this file.

Signed-off-by: Simon Glass 
---

 tools/buildman/cmdline.py | 15 +++
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/tools/buildman/cmdline.py b/tools/buildman/cmdline.py
index 3066a0b38bbf..91571a5adb1d 100644
--- a/tools/buildman/cmdline.py
+++ b/tools/buildman/cmdline.py
@@ -2,6 +2,11 @@
 # Copyright (c) 2014 Google, Inc
 #
 
+"""Handles parsing of buildman arguments
+
+This creates the argument parser and uses it to parse the arguments passed in
+"""
+
 from optparse import OptionParser
 import os
 import pathlib
@@ -71,7 +76,8 @@ def parse_args():
 parser.add_option('-k', '--keep-outputs', action='store_true',
   default=False, help='Keep all build output files (e.g. binaries)')
 parser.add_option('-K', '--show-config', action='store_true',
-  default=False, help='Show configuration changes in summary (both 
board config files and Kconfig)')
+  default=False,
+  help='Show configuration changes in summary (both board config files 
and Kconfig)')
 parser.add_option('--preserve-config-y', action='store_true',
   default=False, help="Don't convert y to 1 in configs")
 parser.add_option('-l', '--list-error-boards', action='store_true',
@@ -84,14 +90,15 @@ def parse_args():
   default=False, help="Run 'make mrproper before reconfiguring")
 parser.add_option(
   '-M', '--allow-missing', action='store_true', default=False,
-  help='Tell binman to allow missing blobs and generate fake ones as 
needed'),
+  help='Tell binman to allow missing blobs and generate fake ones as 
needed')
 parser.add_option(
   '--no-allow-missing', action='store_true', default=False,
-  help='Disable telling binman to allow missing blobs'),
+  help='Disable telling binman to allow missing blobs')
 parser.add_option('-n', '--dry-run', action='store_true', dest='dry_run',
   default=False, help="Do a dry run (describe actions, but do 
nothing)")
 parser.add_option('-N', '--no-subdirs', action='store_true', 
dest='no_subdirs',
-  default=False, help="Don't create subdirectories when building 
current source for a single board")
+  default=False,
+  help="Don't create subdirectories when building current source for a 
single board")
 parser.add_option('-o', '--output-dir', type='string', dest='output_dir',
   help='Directory where all builds happen and buildman has its 
workspace (default is ../)')
 parser.add_option('-O', '--override-toolchain', type='string',
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 32/58] buildman: Convert camel case in cmdline.py

[Simon Glass]

Convert this file to snake case and update all files which use it.

Signed-off-by: Simon Glass 
---

 tools/buildman/cmdline.py   | 2 +-
 tools/buildman/func_test.py | 2 +-
 tools/buildman/main.py  | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/buildman/cmdline.py b/tools/buildman/cmdline.py
index ea43685e394d..3066a0b38bbf 100644
--- a/tools/buildman/cmdline.py
+++ b/tools/buildman/cmdline.py
@@ -9,7 +9,7 @@ import pathlib
 BUILDMAN_DIR = pathlib.Path(__file__).parent
 HAS_TESTS = os.path.exists(BUILDMAN_DIR / "test.py")
 
-def ParseArgs():
+def parse_args():
 """Parse command line arguments from sys.argv[]
 
 Returns:
diff --git a/tools/buildman/func_test.py b/tools/buildman/func_test.py
index d9b4c41ec68c..ad48e3b2159f 100644
--- a/tools/buildman/func_test.py
+++ b/tools/buildman/func_test.py
@@ -244,7 +244,7 @@ class TestFunctional(unittest.TestCase):
 result code from buildman
 """
 sys.argv = [sys.argv[0]] + list(args)
-options, args = cmdline.ParseArgs()
+options, args = cmdline.parse_args()
 if brds == False:
 brds = self._boards
 result = control.do_buildman(
diff --git a/tools/buildman/main.py b/tools/buildman/main.py
index 2253401709a8..0f711c8653b3 100755
--- a/tools/buildman/main.py
+++ b/tools/buildman/main.py
@@ -59,7 +59,7 @@ def run_buildman():
 This is the main program. It collects arguments and runs either the tests 
or
 the control module.
 """
-options, args = cmdline.ParseArgs()
+options, args = cmdline.parse_args()
 
 if not options.debug:
 sys.tracebacklimit = 0
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 31/58] buildman: Create a function to get number of built commits

[Simon Glass]

Move this code into a function. This removes the last pylint error in
the control module.

Signed-off-by: Simon Glass 
---

 tools/buildman/control.py | 34 +-
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index 612045bc89d5..346a40f26e9e 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -29,7 +29,24 @@ def get_plural(count):
 """Returns a plural 's' if count is not 1"""
 return 's' if count != 1 else ''
 
-def get_action_summary(is_summary, commits, selected, step, threads, jobs):
+
+def count_build_commits(commits, step):
+"""Calculate the number of commits to be built
+
+Args:
+commits (list of Commit): Commits to build or None
+step (int): Step value for commits, typically 1
+
+Returns:
+Number of commits that will be built
+"""
+if commits:
+count = len(commits)
+return (count + step - 1) // step
+return 0
+
+
+def get_action_summary(is_summary, commit_count, selected, threads, jobs):
 """Return a string summarising the intended action.
 
 Args:
@@ -43,10 +60,8 @@ def get_action_summary(is_summary, commits, selected, step, 
threads, jobs):
 Returns:
 Summary string.
 """
-if commits:
-count = len(commits)
-count = (count + step - 1) // step
-commit_str = f'{count} commit{get_plural(count)}'
+if commit_count:
+commit_str = f'{commit_count} commit{get_plural(commit_count)}'
 else:
 commit_str = 'current source'
 msg = (f"{'Summary of' if is_summary else 'Building'} "
@@ -83,8 +98,8 @@ def show_actions(series, why_selected, boards_selected, 
output_dir,
 commits = series.commits
 else:
 commits = None
-print(get_action_summary(False, commits, boards_selected, step, threads,
- jobs))
+print(get_action_summary(False, count_build_commits(commits, step),
+ boards_selected, threads, jobs))
 print(f'Build directory: {output_dir}')
 if commits:
 for upto in range(0, len(series.commits), step):
@@ -486,8 +501,9 @@ def run_builder(builder, commits, board_selected, options):
 builder.gnu_make = gnu_make
 
 if not options.ide:
-tprint(get_action_summary(options.summary, commits, board_selected,
-  options.step, options.threads, options.jobs))
+commit_count = count_build_commits(commits, options.step)
+tprint(get_action_summary(options.summary, commit_count, 
board_selected,
+  options.threads, options.jobs))
 
 builder.SetDisplayOptions(
 options.show_errors, options.show_sizes, options.show_detail,
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 30/58] buildman: Use get_alow_missing() directly to avoid var

[Simon Glass]

Avoid an unnecessary local variable by moving this code to a function.
This fixes the pylint warning about too many local variables.

Signed-off-by: Simon Glass 
---

 tools/buildman/control.py | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index e60f4f156abd..612045bc89d5 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -594,10 +594,6 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
  options.verbose)
 return 0
 
-allow_missing = get_allow_missing(options.allow_missing,
-  options.no_allow_missing, len(selected),
-  options.branch)
-
 # Create a new builder with the selected options
 builder = Builder(toolchains, output_dir, git_dir,
 options.threads, options.jobs, checkout=True,
@@ -613,7 +609,10 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 test_thread_exceptions=test_thread_exceptions,
 adjust_cfg=calc_adjust_cfg(options.adjust_cfg,
options.reproducible_builds),
-allow_missing=allow_missing, no_lto=options.no_lto,
+allow_missing=get_allow_missing(options.allow_missing,
+options.no_allow_missing,
+len(selected), options.branch),
+no_lto=options.no_lto,
 reproducible_builds=options.reproducible_builds,
 force_build = options.force_build,
 force_build_failures = options.force_build_failures,
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 29/58] buildman: Move getting the adjust_cfg into run_builder()

[Simon Glass]

Move this into its own function to reduce the size of do_buildman().

Signed-off-by: Simon Glass 
---

 tools/buildman/control.py | 38 +++---
 1 file changed, 27 insertions(+), 11 deletions(-)

diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index a1dc5334193c..e60f4f156abd 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -507,6 +507,31 @@ def run_builder(builder, commits, board_selected, options):
 return 101
 return 0
 
+
+def calc_adjust_cfg(adjust_cfg, reproducible_builds):
+"""Calculate the value to use for adjust_cfg
+
+Args:
+adjust_cfg (list of str): List of configuration changes. See cfgutil 
for
+details
+reproducible_builds (bool): True to adjust the configuration to get
+reproduceable builds
+
+Returns:
+adjust_cfg (list of str): List of configuration changes
+"""
+adjust_cfg = cfgutil.convert_list_to_dict(adjust_cfg)
+
+# Drop LOCALVERSION_AUTO since it changes the version string on every 
commit
+if reproducible_builds:
+# If these are mentioned, leave the local version alone
+if 'LOCALVERSION' in adjust_cfg or 'LOCALVERSION_AUTO' in adjust_cfg:
+print('Not dropping LOCALVERSION_AUTO for reproducible build')
+else:
+adjust_cfg['LOCALVERSION_AUTO'] = '~'
+return adjust_cfg
+
+
 def do_buildman(options, args, toolchains=None, make_func=None, brds=None,
 clean_dir=False, test_thread_exceptions=False):
 """The main control code for buildman
@@ -573,16 +598,6 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
   options.no_allow_missing, len(selected),
   options.branch)
 
-adjust_cfg = cfgutil.convert_list_to_dict(options.adjust_cfg)
-
-# Drop LOCALVERSION_AUTO since it changes the version string on every 
commit
-if options.reproducible_builds:
-# If these are mentioned, leave the local version alone
-if 'LOCALVERSION' in adjust_cfg or 'LOCALVERSION_AUTO' in adjust_cfg:
-print('Not dropping LOCALVERSION_AUTO for reproducible build')
-else:
-adjust_cfg['LOCALVERSION_AUTO'] = '~'
-
 # Create a new builder with the selected options
 builder = Builder(toolchains, output_dir, git_dir,
 options.threads, options.jobs, checkout=True,
@@ -596,7 +611,8 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 warnings_as_errors=options.warnings_as_errors,
 work_in_output=options.work_in_output,
 test_thread_exceptions=test_thread_exceptions,
-adjust_cfg=adjust_cfg,
+adjust_cfg=calc_adjust_cfg(options.adjust_cfg,
+   options.reproducible_builds),
 allow_missing=allow_missing, no_lto=options.no_lto,
 reproducible_builds=options.reproducible_builds,
 force_build = options.force_build,
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 28/58] buildman: Move checking for make into run_builder()

[Simon Glass]

This is not needed until the builder is run. Move it there to reduce the
size of the do_buildman() function.

Signed-off-by: Simon Glass 
---

 tools/buildman/control.py | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index d520e01334ce..a1dc5334193c 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -465,6 +465,7 @@ def setup_output_dir(output_dir, work_in_output, branch, 
no_subdirs, col,
 shutil.rmtree(output_dir)
 return output_dir
 
+
 def run_builder(builder, commits, board_selected, options):
 """Run the builder or show the summary
 
@@ -478,6 +479,12 @@ def run_builder(builder, commits, board_selected, options):
 Returns:
 int: Return code for buildman
 """
+gnu_make = command.output(os.path.join(options.git,
+'scripts/show-gnu-make'), raise_on_error=False).rstrip()
+if not gnu_make:
+sys.exit('GNU Make not found')
+builder.gnu_make = gnu_make
+
 if not options.ide:
 tprint(get_action_summary(options.summary, commits, board_selected,
   options.step, options.threads, options.jobs))
@@ -562,11 +569,6 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
  options.verbose)
 return 0
 
-gnu_make = command.output(os.path.join(options.git,
-'scripts/show-gnu-make'), raise_on_error=False).rstrip()
-if not gnu_make:
-sys.exit('GNU Make not found')
-
 allow_missing = get_allow_missing(options.allow_missing,
   options.no_allow_missing, len(selected),
   options.branch)
@@ -583,7 +585,7 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 
 # Create a new builder with the selected options
 builder = Builder(toolchains, output_dir, git_dir,
-options.threads, options.jobs, gnu_make=gnu_make, checkout=True,
+options.threads, options.jobs, checkout=True,
 show_unknown=options.show_unknown, step=options.step,
 no_subdirs=options.no_subdirs, full_path=options.full_path,
 verbose_build=options.verbose_build,
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 26/58] buildman: Drop some unnecessary variables

[Simon Glass]

Drop some variables at the end of the do_bulidman() function.

Signed-off-by: Simon Glass 
---

 tools/buildman/control.py | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index 3c5b7128bf20..db8377e9ac42 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -607,9 +607,5 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 
 TEST_BUILDER = builder
 
-# Work out which boards to build
-board_selected = brds.get_selected_dict()
-
-commits = series.commits if series else None
-retval = run_builder(builder, commits, board_selected, options)
-return retval
+return run_builder(builder, series.commits if series else None,
+   brds.get_selected_dict(), options)
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 27/58] buildman: Adjust show_toolchain_prefix() to not return

[Simon Glass]

This function does not need to return. Simplify the code by exiting
immediately.

Signed-off-by: Simon Glass 
---

 tools/buildman/control.py | 7 ++-
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index db8377e9ac42..d520e01334ce 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -121,10 +121,9 @@ def show_toolchain_prefix(brds, toolchains):
 for brd in board_selected.values():
 tc_set.add(toolchains.Select(brd.arch))
 if len(tc_set) != 1:
-return 'Supplied boards must share one toolchain'
+sys.exit('Supplied boards must share one toolchain')
 tchain = tc_set.pop()
 print(tchain.GetEnvArgs(toolchain.VAR_CROSS_COMPILE))
-return None
 
 def get_allow_missing(opt_allow, opt_no_allow, num_selected, has_branch):
 """Figure out whether to allow external blobs
@@ -545,9 +544,7 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 return brds
 
 if options.print_prefix:
-err = show_toolchain_prefix(brds, toolchains)
-if err:
-sys.exit(col.build(col.RED, err))
+show_toolchain_prefix(brds, toolchains)
 return 0
 
 selected, why_selected, board_warnings = determine_boards(
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 25/58] buildman: Moving running of the builder into a function

[Simon Glass]

Move this code into a new function. This removes the pylint warning about
too many branches.

Signed-off-by: Simon Glass 
---

 tools/buildman/control.py | 56 ---
 1 file changed, 35 insertions(+), 21 deletions(-)

diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index a0b5033bc878..3c5b7128bf20 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -466,6 +466,40 @@ def setup_output_dir(output_dir, work_in_output, branch, 
no_subdirs, col,
 shutil.rmtree(output_dir)
 return output_dir
 
+def run_builder(builder, commits, board_selected, options):
+"""Run the builder or show the summary
+
+Args:
+commits (list of Commit): List of commits being built, None if no 
branch
+boards_selected (dict): Dict of selected boards:
+key: target name
+value: Board object
+options (Options): Options to use
+
+Returns:
+int: Return code for buildman
+"""
+if not options.ide:
+tprint(get_action_summary(options.summary, commits, board_selected,
+  options.step, options.threads, options.jobs))
+
+builder.SetDisplayOptions(
+options.show_errors, options.show_sizes, options.show_detail,
+options.show_bloat, options.list_error_boards, options.show_config,
+options.show_environment, options.filter_dtb_warnings,
+options.filter_migration_warnings, options.ide)
+if options.summary:
+builder.ShowSummary(commits, board_selected)
+else:
+fail, warned, excs = builder.BuildBoards(
+commits, board_selected, options.keep_outputs, options.verbose)
+if excs:
+return 102
+if fail:
+return 100
+if warned and not options.ignore_warnings:
+return 101
+return 0
 
 def do_buildman(options, args, toolchains=None, make_func=None, brds=None,
 clean_dir=False, test_thread_exceptions=False):
@@ -577,25 +611,5 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 board_selected = brds.get_selected_dict()
 
 commits = series.commits if series else None
-if not options.ide:
-tprint(get_action_summary(options.summary, commits, board_selected,
-  options.step, options.threads, options.jobs))
-
-builder.SetDisplayOptions(
-options.show_errors, options.show_sizes, options.show_detail,
-options.show_bloat, options.list_error_boards, options.show_config,
-options.show_environment, options.filter_dtb_warnings,
-options.filter_migration_warnings, options.ide)
-retval = 0
-if options.summary:
-builder.ShowSummary(commits, board_selected)
-else:
-fail, warned, excs = builder.BuildBoards(
-commits, board_selected, options.keep_outputs, options.verbose)
-if excs:
-retval = 102
-if fail:
-retval = 100
-if warned and not options.ignore_warnings:
-retval = 101
+retval = run_builder(builder, commits, board_selected, options)
 return retval
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 24/58] buildman: Tweak commits and show_bloat

[Simon Glass]

Move setting of show_bloat to adjust_options() and adjust how the commits
variable is set. Together these remove the pylint warning about too many
statements.

Signed-off-by: Simon Glass 
---

 tools/buildman/control.py | 13 +
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index c0274f5463dc..a0b5033bc878 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -433,6 +433,10 @@ def adjust_options(options, series, selected):
 if not options.step:
 options.step = len(series.commits) - 1
 
+# We can't show function sizes without board details at present
+if options.show_bloat:
+options.show_detail = True
+
 
 def setup_output_dir(output_dir, work_in_output, branch, no_subdirs, col,
  clean_dir):
@@ -572,18 +576,11 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 # Work out which boards to build
 board_selected = brds.get_selected_dict()
 
-if series:
-commits = series.commits
-else:
-commits = None
-
+commits = series.commits if series else None
 if not options.ide:
 tprint(get_action_summary(options.summary, commits, board_selected,
   options.step, options.threads, options.jobs))
 
-# We can't show function sizes without board details at present
-if options.show_bloat:
-options.show_detail = True
 builder.SetDisplayOptions(
 options.show_errors, options.show_sizes, options.show_detail,
 options.show_bloat, options.list_error_boards, options.show_config,
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 23/58] buildman: Move remaining builder properties to constructor

[Simon Glass]

Do these all in the constructor, so it is consistent.

Move the stray builder comment into the correct place.

Signed-off-by: Simon Glass 
---

 tools/buildman/builder.py | 25 ++---
 tools/buildman/control.py | 18 +++---
 2 files changed, 25 insertions(+), 18 deletions(-)

diff --git a/tools/buildman/builder.py b/tools/buildman/builder.py
index d81752e99438..cb3628a8a085 100644
--- a/tools/buildman/builder.py
+++ b/tools/buildman/builder.py
@@ -255,7 +255,10 @@ class Builder:
  config_only=False, squash_config_y=False,
  warnings_as_errors=False, work_in_output=False,
  test_thread_exceptions=False, adjust_cfg=None,
- allow_missing=False, no_lto=False, reproducible_builds=False):
+ allow_missing=False, no_lto=False, reproducible_builds=False,
+ force_build=False, force_build_failures=False,
+ force_reconfig=False, in_tree=False,
+ force_config_on_failure=False, make_func=None):
 """Create a new Builder object
 
 Args:
@@ -295,7 +298,14 @@ class Builder:
 a string Kconfig
 allow_missing: Run build with BINMAN_ALLOW_MISSING=1
 no_lto (bool): True to set the NO_LTO flag when building
-
+force_build (bool): Rebuild even commits that are already built
+force_build_failures (bool): Rebuild commits that have not been
+built, or failed to build
+force_reconfig (bool): Reconfigure on each commit
+in_tree (bool): Bulid in tree instead of out-of-tree
+force_config_on_failure (bool): Reconfigure the build before
+retrying a failed build
+make_func (function): Function to call to run 'make'
 """
 self.toolchains = toolchains
 self.base_dir = base_dir
@@ -304,7 +314,7 @@ class Builder:
 else:
 self._working_dir = os.path.join(base_dir, '.bm-work')
 self.threads = []
-self.do_make = self.Make
+self.do_make = make_func or self.Make
 self.gnu_make = gnu_make
 self.checkout = checkout
 self.num_threads = num_threads
@@ -318,11 +328,7 @@ class Builder:
 self._complete_delay = None
 self._next_delay_update = datetime.now()
 self._start_time = datetime.now()
-self.force_config_on_failure = True
-self.force_build_failures = False
-self.force_reconfig = False
 self._step = step
-self.in_tree = False
 self._error_lines = 0
 self.no_subdirs = no_subdirs
 self.full_path = full_path
@@ -336,6 +342,11 @@ class Builder:
 self._ide = False
 self.no_lto = no_lto
 self.reproducible_builds = reproducible_builds
+self.force_build = force_build
+self.force_build_failures = force_build_failures
+self.force_reconfig = force_reconfig
+self.in_tree = in_tree
+self.force_config_on_failure = force_config_on_failure
 
 if not self.squash_config_y:
 self.config_filenames += EXTRA_CONFIG_FILENAMES
diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index 3b07a94ecbac..c0274f5463dc 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -536,8 +536,6 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
   options.no_allow_missing, len(selected),
   options.branch)
 
-# Create a new builder with the selected options.
-
 adjust_cfg = cfgutil.convert_list_to_dict(options.adjust_cfg)
 
 # Drop LOCALVERSION_AUTO since it changes the version string on every 
commit
@@ -548,6 +546,7 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 else:
 adjust_cfg['LOCALVERSION_AUTO'] = '~'
 
+# Create a new builder with the selected options
 builder = Builder(toolchains, output_dir, git_dir,
 options.threads, options.jobs, gnu_make=gnu_make, checkout=True,
 show_unknown=options.show_unknown, step=options.step,
@@ -562,16 +561,13 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 test_thread_exceptions=test_thread_exceptions,
 adjust_cfg=adjust_cfg,
 allow_missing=allow_missing, no_lto=options.no_lto,
-reproducible_builds=options.reproducible_builds)
+reproducible_builds=options.reproducible_builds,
+force_build = options.force_build,
+force_build_failures = options.force_build_failures,
+force_reconfig = options.force_reconfig, in_tree = options.in_tree,
+force_config_on_failure=not options.quick, make_func=make_func)
+
 TEST_BUILDER = builder
-builder.force_config_on_failure = not options.quick
-if make_func:
-

[PATCH 22/58] buildman: Avoid too many returns in do_buildman()

[Simon Glass]

Fix the pylint warning by using a variable instead of lots of 'return'
statements.

Signed-off-by: Simon Glass 
---

 tools/buildman/control.py | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index f2e1cfa88e94..3b07a94ecbac 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -593,15 +593,16 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 options.show_bloat, options.list_error_boards, options.show_config,
 options.show_environment, options.filter_dtb_warnings,
 options.filter_migration_warnings, options.ide)
+retval = 0
 if options.summary:
 builder.ShowSummary(commits, board_selected)
 else:
 fail, warned, excs = builder.BuildBoards(
 commits, board_selected, options.keep_outputs, options.verbose)
 if excs:
-return 102
+retval = 102
 if fail:
-return 100
+retval = 100
 if warned and not options.ignore_warnings:
-return 101
-return 0
+retval = 101
+return retval
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 21/58] buildman: Move commit numbering into determine_series()

[Simon Glass]

Commits are numbered for use in tests. Do this in determine_series() since
it is already dealing with the series.

Signed-off-by: Simon Glass 
---

 tools/buildman/control.py | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index 5cb0c3f24465..f2e1cfa88e94 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -202,6 +202,9 @@ def count_commits(branch, count, col, git_dir):
 def determine_series(selected, col, git_dir, count, branch, work_in_output):
 """Determine the series which is to be built, if any
 
+If there is a series, the commits in that series are numbered by setting
+their sequence value (starting from 0). This is used by tests.
+
 Args:
 selected (list of Board): List of Board objects that are marked
 selected
@@ -254,6 +257,10 @@ def determine_series(selected, col, git_dir, count, 
branch, work_in_output):
 # Honour the count
 series = patchstream.get_metadata_for_list(branch,
 git_dir, count, series=None, allow_overwrite=True)
+
+# Number the commits for test purposes
+for i, commit in enumerate(series.commits):
+commit.sequence = i
 else:
 series = None
 return series
@@ -571,9 +578,6 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 
 if series:
 commits = series.commits
-# Number the commits for test purposes
-for i, commit in enumerate(commits):
-commit.sequence = i
 else:
 commits = None
 
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 20/58] buildman: Move setting up the output dir into a function

[Simon Glass]

Move this code into a separate function to reduce the size of the main
do_buildman() directory.

Signed-off-by: Simon Glass 
---

 tools/buildman/control.py | 45 ---
 1 file changed, 33 insertions(+), 12 deletions(-)

diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index c6abf96a4a14..5cb0c3f24465 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -426,6 +426,36 @@ def adjust_options(options, series, selected):
 if not options.step:
 options.step = len(series.commits) - 1
 
+
+def setup_output_dir(output_dir, work_in_output, branch, no_subdirs, col,
+ clean_dir):
+"""Set up the output directory
+
+Args:
+output_dir (str): Output directory provided by the user, or None if 
none
+work_in_output (bool): True to work in the output directory
+branch (str): Name of branch to build, or None if none
+no_subdirs (bool): True to put the output in the top-level output dir
+clean_dir: Used for tests only, indicates that the existing output_dir
+should be removed before starting the build
+
+Returns:
+str: Updated output directory pathname
+"""
+if not output_dir:
+if work_in_output:
+sys.exit(col.build(col.RED, '-w requires that you specify -o'))
+output_dir = '..'
+if branch and not no_subdirs:
+# As a special case allow the board directory to be placed in the
+# output directory itself rather than any subdirectory.
+dirname = branch.replace('/', '_')
+output_dir = os.path.join(output_dir, dirname)
+if clean_dir and os.path.exists(output_dir):
+shutil.rmtree(output_dir)
+return output_dir
+
+
 def do_buildman(options, args, toolchains=None, make_func=None, brds=None,
 clean_dir=False, test_thread_exceptions=False):
 """The main control code for buildman
@@ -458,18 +488,9 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 toolchains = get_toolchains(toolchains, col, options.override_toolchain,
 options.fetch_arch, options.list_tool_chains,
 options.verbose)
-output_dir = options.output_dir
-if not output_dir:
-if options.work_in_output:
-sys.exit(col.build(col.RED, '-w requires that you specify -o'))
-output_dir = '..'
-if options.branch and not options.no_subdirs:
-# As a special case allow the board directory to be placed in the
-# output directory itself rather than any subdirectory.
-dirname = options.branch.replace('/', '_')
-output_dir = os.path.join(output_dir, dirname)
-if clean_dir and os.path.exists(output_dir):
-shutil.rmtree(output_dir)
+output_dir = setup_output_dir(
+options.output_dir, options.work_in_output, options.branch,
+options.no_subdirs, col, clean_dir)
 
 # Work out what subset of the boards we are building
 if not brds:
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 19/58] buildman: Move counting of commits into a function

[Simon Glass]

Move this code into a separate function to avoid a pylint warning in
determine_series().

Signed-off-by: Simon Glass 
---

 tools/buildman/control.py | 63 +--
 1 file changed, 41 insertions(+), 22 deletions(-)

diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index 7c56a0f8ed40..c6abf96a4a14 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -159,6 +159,46 @@ def get_allow_missing(opt_allow, opt_no_allow, 
num_selected, has_branch):
 return allow_missing
 
 
+def count_commits(branch, count, col, git_dir):
+"""Could the number of commits in the branch/ranch being built
+
+Args:
+branch (str): Name of branch to build, or None if none
+count (int): Number of commits to build, or -1 for all
+col (Terminal.Color): Color object to use
+git_dir (str): Git directory to use, e.g. './.git'
+
+Returns:
+tuple:
+Number of commits being built
+True if the 'branch' string contains a range rather than a simple
+name
+"""
+has_range = branch and '..' in branch
+if count == -1:
+if not branch:
+count = 1
+else:
+if has_range:
+count, msg = gitutil.count_commits_in_range(git_dir, branch)
+else:
+count, msg = gitutil.count_commits_in_branch(git_dir, branch)
+if count is None:
+sys.exit(col.build(col.RED, msg))
+elif count == 0:
+sys.exit(col.build(col.RED,
+   f"Range '{branch}' has no commits"))
+if msg:
+print(col.build(col.YELLOW, msg))
+count += 1   # Build upstream commit also
+
+if not count:
+msg = (f"No commits found to process in branch '{branch}': "
+   "set branch's upstream or use -c flag")
+sys.exit(col.build(col.RED, msg))
+return count, has_range
+
+
 def determine_series(selected, col, git_dir, count, branch, work_in_output):
 """Determine the series which is to be built, if any
 
@@ -189,28 +229,7 @@ def determine_series(selected, col, git_dir, count, 
branch, work_in_output):
 # Work out how many commits to build. We want to build everything on the
 # branch. We also build the upstream commit as a control so we can see
 # problems introduced by the first commit on the branch.
-has_range = branch and '..' in branch
-if count == -1:
-if not branch:
-count = 1
-else:
-if has_range:
-count, msg = gitutil.count_commits_in_range(git_dir, branch)
-else:
-count, msg = gitutil.count_commits_in_branch(git_dir, branch)
-if count is None:
-sys.exit(col.build(col.RED, msg))
-elif count == 0:
-sys.exit(col.build(col.RED,
-   f"Range '{branch}' has no commits"))
-if msg:
-print(col.build(col.YELLOW, msg))
-count += 1   # Build upstream commit also
-
-if not count:
-msg = (f"No commits found to process in branch '{branch}': "
-   "set branch's upstream or use -c flag")
-sys.exit(col.build(col.RED, msg))
+count, has_range = count_commits(branch, count, col, git_dir)
 if work_in_output:
 if len(selected) != 1:
 sys.exit(col.build(col.RED,
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 18/58] buildman: Build option-adjusting into a function

[Simon Glass]

Create a separate function to adjust options. Also move show_actions() up
as far as we can in the function.

Signed-off-by: Simon Glass 
---

 tools/buildman/control.py | 53 ---
 1 file changed, 33 insertions(+), 20 deletions(-)

diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index b15ab8270e9d..7c56a0f8ed40 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -381,6 +381,32 @@ def determine_boards(brds, args, col, opt_boards, exclude):
 return selected, why_selected, board_warnings
 
 
+def adjust_options(options, series, selected):
+"""Adjust options according to various constraints
+
+Updates verbose, show_errors, threads, jobs and step
+
+Args:
+options (Options): Options object to adjust
+series (Series): Series being built / summarised
+selected (list of Board): List of Board objects that are marked
+"""
+if not series and not options.dry_run:
+options.verbose = True
+if not options.summary:
+options.show_errors = True
+
+# By default we have one thread per CPU. But if there are not enough jobs
+# we can have fewer threads and use a high '-j' value for make.
+if options.threads is None:
+options.threads = min(multiprocessing.cpu_count(), len(selected))
+if not options.jobs:
+options.jobs = max(1, (multiprocessing.cpu_count() +
+len(selected) - 1) // len(selected))
+
+if not options.step:
+options.step = len(series.commits) - 1
+
 def do_buildman(options, args, toolchains=None, make_func=None, brds=None,
 clean_dir=False, test_thread_exceptions=False):
 """The main control code for buildman
@@ -444,21 +470,15 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 
 series = determine_series(selected, col, git_dir, options.count,
   options.branch, options.work_in_output)
-if not series and not options.dry_run:
-options.verbose = True
-if not options.summary:
-options.show_errors = True
 
-# By default we have one thread per CPU. But if there are not enough jobs
-# we can have fewer threads and use a high '-j' value for make.
-if options.threads is None:
-options.threads = min(multiprocessing.cpu_count(), len(selected))
-if not options.jobs:
-options.jobs = max(1, (multiprocessing.cpu_count() +
-len(selected) - 1) // len(selected))
+adjust_options(options, series, selected)
 
-if not options.step:
-options.step = len(series.commits) - 1
+# For a dry run, just show our actions as a sanity check
+if options.dry_run:
+show_actions(series, why_selected, selected, output_dir, 
board_warnings,
+ options.step, options.threads, options.jobs,
+ options.verbose)
+return 0
 
 gnu_make = command.output(os.path.join(options.git,
 'scripts/show-gnu-make'), raise_on_error=False).rstrip()
@@ -471,13 +491,6 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 
 # Create a new builder with the selected options.
 
-# For a dry run, just show our actions as a sanity check
-if options.dry_run:
-show_actions(series, why_selected, selected, output_dir, 
board_warnings,
- options.step, options.threads, options.jobs,
- options.verbose)
-return 0
-
 adjust_cfg = cfgutil.convert_list_to_dict(options.adjust_cfg)
 
 # Drop LOCALVERSION_AUTO since it changes the version string on every 
commit
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 17/58] buildman: Pass option values to show_actions()

[Simon Glass]

Pass in the individual values rather than the whole options object, so we
can see what is needed.

Signed-off-by: Simon Glass 
---

 tools/buildman/control.py | 22 +-
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index 07c9921b9e7f..b15ab8270e9d 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -56,8 +56,8 @@ def get_action_summary(is_summary, commits, selected, step, 
threads, jobs):
 return msg
 
 # pylint: disable=R0913
-def show_actions(series, why_selected, boards_selected, output_dir, options,
- board_warnings):
+def show_actions(series, why_selected, boards_selected, output_dir,
+ board_warnings, step, threads, jobs, verbose):
 """Display a list of actions that we would take, if not a dry run.
 
 Args:
@@ -70,8 +70,11 @@ def show_actions(series, why_selected, boards_selected, 
output_dir, options,
 boards_selected: Dict of selected boards, key is target name,
 value is Board object
 output_dir (str): Output directory for builder
-options: Command line options object
 board_warnings: List of warnings obtained from board selected
+step (int): Step increment through commits
+threads (int): Number of processor threads being used
+jobs (int): Number of jobs to build at once
+verbose (bool): True to indicate why each board was selected
 """
 col = terminal.Color()
 print('Dry run, so not doing much. But I would do this:')
@@ -80,11 +83,11 @@ def show_actions(series, why_selected, boards_selected, 
output_dir, options,
 commits = series.commits
 else:
 commits = None
-print(get_action_summary(False, commits, boards_selected,
- options.step, options.threads, options.jobs))
+print(get_action_summary(False, commits, boards_selected, step, threads,
+ jobs))
 print(f'Build directory: {output_dir}')
 if commits:
-for upto in range(0, len(series.commits), options.step):
+for upto in range(0, len(series.commits), step):
 commit = series.commits[upto]
 print('   ', col.build(col.YELLOW, commit.hash[:8], bright=False), 
end=' ')
 print(commit.subject)
@@ -92,7 +95,7 @@ def show_actions(series, why_selected, boards_selected, 
output_dir, options,
 for arg in why_selected:
 if arg != 'all':
 print(arg, f': {len(why_selected[arg])} boards')
-if options.verbose:
+if verbose:
 print(f"   {' '.join(why_selected[arg])}")
 print('Total boards to build for each '
   f"commit: {len(why_selected['all'])}\n")
@@ -470,8 +473,9 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 
 # For a dry run, just show our actions as a sanity check
 if options.dry_run:
-show_actions(series, why_selected, selected, output_dir, options,
-board_warnings)
+show_actions(series, why_selected, selected, output_dir, 
board_warnings,
+ options.step, options.threads, options.jobs,
+ options.verbose)
 return 0
 
 adjust_cfg = cfgutil.convert_list_to_dict(options.adjust_cfg)
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 16/58] buildman: Pass option values to get_action_summary()

[Simon Glass]

Pass in the individual values rather than the whole options object, so we
can see what is needed.

Signed-off-by: Simon Glass 
---

 tools/buildman/control.py | 22 +++---
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index 200f4eb898ac..07c9921b9e7f 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -29,22 +29,30 @@ def get_plural(count):
 """Returns a plural 's' if count is not 1"""
 return 's' if count != 1 else ''
 
-def get_action_summary(is_summary, commits, selected, options):
+def get_action_summary(is_summary, commits, selected, step, threads, jobs):
 """Return a string summarising the intended action.
 
+Args:
+is_summary (bool): True if this is a summary (otherwise it is building)
+commits (list): List of commits being built
+selected (list of Board): List of Board objects that are marked
+step (int): Step increment through commits
+threads (int): Number of processor threads being used
+jobs (int): Number of jobs to build at once
+
 Returns:
 Summary string.
 """
 if commits:
 count = len(commits)
-count = (count + options.step - 1) // options.step
+count = (count + step - 1) // step
 commit_str = f'{count} commit{get_plural(count)}'
 else:
 commit_str = 'current source'
 msg = (f"{'Summary of' if is_summary else 'Building'} "
f'{commit_str} for {len(selected)} boards')
-msg += (f' ({options.threads} thread{get_plural(options.threads)}, '
-f'{options.jobs} job{get_plural(options.jobs)} per thread)')
+msg += (f' ({threads} thread{get_plural(threads)}, '
+f'{jobs} job{get_plural(jobs)} per thread)')
 return msg
 
 # pylint: disable=R0913
@@ -73,7 +81,7 @@ def show_actions(series, why_selected, boards_selected, 
output_dir, options,
 else:
 commits = None
 print(get_action_summary(False, commits, boards_selected,
-options))
+ options.step, options.threads, options.jobs))
 print(f'Build directory: {output_dir}')
 if commits:
 for upto in range(0, len(series.commits), options.step):
@@ -152,7 +160,7 @@ def determine_series(selected, col, git_dir, count, branch, 
work_in_output):
 """Determine the series which is to be built, if any
 
 Args:
-selected (list of Board(: List of Board objects that are marked
+selected (list of Board): List of Board objects that are marked
 selected
 col (Terminal.Color): Color object to use
 git_dir (str): Git directory to use, e.g. './.git'
@@ -514,7 +522,7 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 
 if not options.ide:
 tprint(get_action_summary(options.summary, commits, board_selected,
-options))
+  options.step, options.threads, options.jobs))
 
 # We can't show function sizes without board details at present
 if options.show_bloat:
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 15/58] buildman: Move output-file setup into one place

[Simon Glass]

Collect the two parts of the output-file handling into single place.

Signed-off-by: Simon Glass 
---

 tools/buildman/control.py | 15 +++
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index 86b00e5e5ae4..200f4eb898ac 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -407,6 +407,13 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 if options.work_in_output:
 sys.exit(col.build(col.RED, '-w requires that you specify -o'))
 output_dir = '..'
+if options.branch and not options.no_subdirs:
+# As a special case allow the board directory to be placed in the
+# output directory itself rather than any subdirectory.
+dirname = options.branch.replace('/', '_')
+output_dir = os.path.join(output_dir, dirname)
+if clean_dir and os.path.exists(output_dir):
+shutil.rmtree(output_dir)
 
 # Work out what subset of the boards we are building
 if not brds:
@@ -452,14 +459,6 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
   options.branch)
 
 # Create a new builder with the selected options.
-if options.branch:
-dirname = options.branch.replace('/', '_')
-# As a special case allow the board directory to be placed in the
-# output directory itself rather than any subdirectory.
-if not options.no_subdirs:
-output_dir = os.path.join(output_dir, dirname)
-if clean_dir and os.path.exists(output_dir):
-shutil.rmtree(output_dir)
 
 # For a dry run, just show our actions as a sanity check
 if options.dry_run:
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 14/58] bulldman: Set up output_dir earlier

[Simon Glass]

Set up output_dir at the start of the main function, instead of updating
the options.output_dir option.

Signed-off-by: Simon Glass 
---

 tools/buildman/control.py | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index 13fc4799cf8b..86b00e5e5ae4 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -402,14 +402,15 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 toolchains = get_toolchains(toolchains, col, options.override_toolchain,
 options.fetch_arch, options.list_tool_chains,
 options.verbose)
-if not options.output_dir:
+output_dir = options.output_dir
+if not output_dir:
 if options.work_in_output:
 sys.exit(col.build(col.RED, '-w requires that you specify -o'))
-options.output_dir = '..'
+output_dir = '..'
 
 # Work out what subset of the boards we are building
 if not brds:
-brds = get_boards_obj(options.output_dir, options.regen_board_list,
+brds = get_boards_obj(output_dir, options.regen_board_list,
   options.threads, options.verbose)
 if isinstance(brds, int):
 return brds
@@ -451,13 +452,12 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
   options.branch)
 
 # Create a new builder with the selected options.
-output_dir = options.output_dir
 if options.branch:
 dirname = options.branch.replace('/', '_')
 # As a special case allow the board directory to be placed in the
 # output directory itself rather than any subdirectory.
 if not options.no_subdirs:
-output_dir = os.path.join(options.output_dir, dirname)
+output_dir = os.path.join(output_dir, dirname)
 if clean_dir and os.path.exists(output_dir):
 shutil.rmtree(output_dir)
 
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 13/58] bulidman: Move toolchain handling to a functoin

[Simon Glass]

Move the code for dealing with toolchains out into its own function, to
reduce the size of the main function.

Signed-off-by: Simon Glass 
---

 tools/buildman/control.py | 53 ---
 1 file changed, 38 insertions(+), 15 deletions(-)

diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index a409c4882972..13fc4799cf8b 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -259,6 +259,41 @@ def do_fetch_arch(toolchains, col, fetch_arch):
 return 0
 
 
+def get_toolchains(toolchains, col, override_toolchain, fetch_arch,
+   list_tool_chains, verbose):
+"""Get toolchains object to use
+
+Args:
+toolchains (Toolchains or None): Toolchains to use. If None, then a
+Toolchains object will be created and scanned
+col (Terminal.Color): Color object
+override_toolchain (str or None): Override value for toolchain, or None
+fetch_arch (bool): True to fetch the toolchain for the architectures
+list_tool_chains (bool): True to list all tool chains
+verbose (bool): True for verbose output when listing toolchains
+
+Returns:
+Either:
+int: Operation completed and buildman should exit with exit code
+Toolchains: Toolchains object to use
+"""
+no_toolchains = toolchains is None
+if no_toolchains:
+toolchains = toolchain.Toolchains(override_toolchain)
+
+if fetch_arch:
+return do_fetch_arch(toolchains, col, fetch_arch)
+
+if no_toolchains:
+toolchains.GetSettings()
+toolchains.Scan(list_tool_chains and verbose)
+if list_tool_chains:
+toolchains.List()
+print()
+return 0
+return toolchains
+
+
 def get_boards_obj(output_dir, regen_board_list, threads, verbose):
 """Object the Boards object to use
 
@@ -364,21 +399,9 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 
 git_dir = os.path.join(options.git, '.git')
 
-no_toolchains = toolchains is None
-if no_toolchains:
-toolchains = toolchain.Toolchains(options.override_toolchain)
-
-if options.fetch_arch:
-return do_fetch_arch(toolchains, col, options.fetch_arch)
-
-if no_toolchains:
-toolchains.GetSettings()
-toolchains.Scan(options.list_tool_chains and options.verbose)
-if options.list_tool_chains:
-toolchains.List()
-print()
-return 0
-
+toolchains = get_toolchains(toolchains, col, options.override_toolchain,
+options.fetch_arch, options.list_tool_chains,
+options.verbose)
 if not options.output_dir:
 if options.work_in_output:
 sys.exit(col.build(col.RED, '-w requires that you specify -o'))
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 12/58] buildman: Move Boards-object code into a function

[Simon Glass]

Move the code which obtains a Boards object into its own function, to
reduce the size of the main function.

Signed-off-by: Simon Glass 
---

 tools/buildman/control.py | 54 ---
 1 file changed, 39 insertions(+), 15 deletions(-)

diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index b88862a22c4a..a409c4882972 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -259,6 +259,41 @@ def do_fetch_arch(toolchains, col, fetch_arch):
 return 0
 
 
+def get_boards_obj(output_dir, regen_board_list, threads, verbose):
+"""Object the Boards object to use
+
+Creates the output directory and ensures there is a boards.cfg file, then
+read it in.
+
+Args:
+output_dir (str): Output directory to use
+regen_board_list (bool): True to just regenerate the board list
+threads (int or None): Number of threads to use to create boards file
+verbose (bool): False to suppress output from boards-file generation
+
+Returns:
+Either:
+int: Operation completed and buildman should exit with exit code
+Boards: Boards object to use
+"""
+if not os.path.exists(output_dir):
+os.makedirs(output_dir)
+board_file = os.path.join(output_dir, 'boards.cfg')
+if regen_board_list and regen_board_list != '-':
+board_file = regen_board_list
+
+brds = boards.Boards()
+okay = brds.ensure_board_list(
+board_file,
+threads or multiprocessing.cpu_count(),
+force=regen_board_list,
+quiet=not verbose)
+if regen_board_list:
+return 0 if okay else 2
+brds.read_boards(board_file)
+return brds
+
+
 def determine_boards(brds, args, col, opt_boards, exclude):
 """Determine which boards to build
 
@@ -351,21 +386,10 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 
 # Work out what subset of the boards we are building
 if not brds:
-if not os.path.exists(options.output_dir):
-os.makedirs(options.output_dir)
-board_file = os.path.join(options.output_dir, 'boards.cfg')
-if options.regen_board_list and options.regen_board_list != '-':
-board_file = options.regen_board_list
-
-brds = boards.Boards()
-okay = brds.ensure_board_list(
-board_file,
-options.threads or multiprocessing.cpu_count(),
-force=options.regen_board_list,
-quiet=not options.verbose)
-if options.regen_board_list:
-return 0 if okay else 2
-brds.read_boards(board_file)
+brds = get_boards_obj(options.output_dir, options.regen_board_list,
+  options.threads, options.verbose)
+if isinstance(brds, int):
+return brds
 
 if options.print_prefix:
 err = show_toolchain_prefix(brds, toolchains)
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 11/58] bulidman: Move more code to determine_series()

[Simon Glass]

Move some more series-related code here, to reduce the size of the main
function.

Signed-off-by: Simon Glass 
---

 tools/buildman/control.py | 82 ---
 1 file changed, 42 insertions(+), 40 deletions(-)

diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index 4055e7aee6ce..b88862a22c4a 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -148,14 +148,17 @@ def get_allow_missing(opt_allow, opt_no_allow, 
num_selected, has_branch):
 return allow_missing
 
 
-def determine_series(count, has_range, branch, git_dir):
+def determine_series(selected, col, git_dir, count, branch, work_in_output):
 """Determine the series which is to be built, if any
 
 Args:
+selected (list of Board(: List of Board objects that are marked
+selected
+col (Terminal.Color): Color object to use
+git_dir (str): Git directory to use, e.g. './.git'
 count (int): Number of commits in branch
-has_range (bool): True if a range of commits ('xx..yy') is being built
 branch (str): Name of branch to build, or None if none
-git_dir (str): Git directory to use, e.g. './.git'
+work_in_output (bool): True to work in the output directory
 
 Returns:
 Series: Series to build, or None for none
@@ -171,6 +174,40 @@ def determine_series(count, has_range, branch, git_dir):
 other hand conflicting tags will cause an error. So allow later tags
 to overwrite earlier ones by setting allow_overwrite=True
 """
+
+# Work out how many commits to build. We want to build everything on the
+# branch. We also build the upstream commit as a control so we can see
+# problems introduced by the first commit on the branch.
+has_range = branch and '..' in branch
+if count == -1:
+if not branch:
+count = 1
+else:
+if has_range:
+count, msg = gitutil.count_commits_in_range(git_dir, branch)
+else:
+count, msg = gitutil.count_commits_in_branch(git_dir, branch)
+if count is None:
+sys.exit(col.build(col.RED, msg))
+elif count == 0:
+sys.exit(col.build(col.RED,
+   f"Range '{branch}' has no commits"))
+if msg:
+print(col.build(col.YELLOW, msg))
+count += 1   # Build upstream commit also
+
+if not count:
+msg = (f"No commits found to process in branch '{branch}': "
+   "set branch's upstream or use -c flag")
+sys.exit(col.build(col.RED, msg))
+if work_in_output:
+if len(selected) != 1:
+sys.exit(col.build(col.RED,
+   '-w can only be used with a single board'))
+if count != 1:
+sys.exit(col.build(col.RED,
+   '-w can only be used with a single commit'))
+
 if branch:
 if count == -1:
 if has_range:
@@ -339,43 +376,8 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 selected, why_selected, board_warnings = determine_boards(
 brds, args, col, options.boards, options.exclude)
 
-# Work out how many commits to build. We want to build everything on the
-# branch. We also build the upstream commit as a control so we can see
-# problems introduced by the first commit on the branch.
-count = options.count
-has_range = options.branch and '..' in options.branch
-if count == -1:
-if not options.branch:
-count = 1
-else:
-if has_range:
-count, msg = gitutil.count_commits_in_range(git_dir,
-options.branch)
-else:
-count, msg = gitutil.count_commits_in_branch(git_dir,
- options.branch)
-if count is None:
-sys.exit(col.build(col.RED, msg))
-elif count == 0:
-sys.exit(col.build(col.RED,
-   f"Range '{options.branch}' has no commits"))
-if msg:
-print(col.build(col.YELLOW, msg))
-count += 1   # Build upstream commit also
-
-if not count:
-msg = (f"No commits found to process in branch '{options.branch}': "
-   "set branch's upstream or use -c flag")
-sys.exit(col.build(col.RED, msg))
-if options.work_in_output:
-if len(selected) != 1:
-sys.exit(col.build(col.RED,
-   '-w can only be used with a single board'))
-if count != 1:
-sys.exit(col.build(col.RED,
-   '-w can only be used with a single commit'))
-
-series = determine_series(count, has_range, options.branch, git_dir)
+series = 

[PATCH 10/58] buildman: Move board-selection code into a function

[Simon Glass]

Create a new determine_boards() function to hold the code which selects
which boards to build.

Signed-off-by: Simon Glass 
---

 tools/buildman/control.py | 59 ---
 1 file changed, 43 insertions(+), 16 deletions(-)

diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index 9a53d70f4344..4055e7aee6ce 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -222,6 +222,47 @@ def do_fetch_arch(toolchains, col, fetch_arch):
 return 0
 
 
+def determine_boards(brds, args, col, opt_boards, exclude):
+"""Determine which boards to build
+
+Each element of args and exclude can refer to a board name, arch or SoC
+
+Args:
+brds (Boards): Boards object
+args (list of str): Arguments describing boards to build
+col (Terminal.Color): Color object
+opt_boards (list of str): Specific boards to build, or None for all
+exclude (list of str): Arguments describing boards to exclude
+
+Returns:
+tuple:
+list of Board: List of Board objects that are marked selected
+why_selected: Dictionary where each key is a buildman argument
+provided by the user, and the value is the list of boards
+brought in by that argument. For example, 'arm' might bring
+in 400 boards, so in this case the key would be 'arm' and
+the value would be a list of board names.
+board_warnings: List of warnings obtained from board selected
+"""
+exclude = []
+if exclude:
+for arg in exclude:
+exclude += arg.split(',')
+
+if opt_boards:
+requested_boards = []
+for brd in opt_boards:
+requested_boards += brd.split(',')
+else:
+requested_boards = None
+why_selected, board_warnings = brds.select_boards(args, exclude,
+  requested_boards)
+selected = brds.get_selected()
+if not selected:
+sys.exit(col.build(col.RED, 'No matching boards found'))
+return selected, why_selected, board_warnings
+
+
 def do_buildman(options, args, toolchains=None, make_func=None, brds=None,
 clean_dir=False, test_thread_exceptions=False):
 """The main control code for buildman
@@ -295,22 +336,8 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 sys.exit(col.build(col.RED, err))
 return 0
 
-exclude = []
-if options.exclude:
-for arg in options.exclude:
-exclude += arg.split(',')
-
-if options.boards:
-requested_boards = []
-for brd in options.boards:
-requested_boards += brd.split(',')
-else:
-requested_boards = None
-why_selected, board_warnings = brds.select_boards(args, exclude,
-  requested_boards)
-selected = brds.get_selected()
-if not selected:
-sys.exit(col.build(col.RED, 'No matching boards found'))
+selected, why_selected, board_warnings = determine_boards(
+brds, args, col, options.boards, options.exclude)
 
 # Work out how many commits to build. We want to build everything on the
 # branch. We also build the upstream commit as a control so we can see
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 09/58] buildman: Move dry-run handling higher in do_buildman()

[Simon Glass]

Move this up above where the builder is created, since it no-longer makes
use of the builder.

Signed-off-by: Simon Glass 
---

 tools/buildman/control.py | 88 ---
 1 file changed, 45 insertions(+), 43 deletions(-)

diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index 8ecd124fdcf3..9a53d70f4344 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -384,6 +384,13 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 output_dir = os.path.join(options.output_dir, dirname)
 if clean_dir and os.path.exists(output_dir):
 shutil.rmtree(output_dir)
+
+# For a dry run, just show our actions as a sanity check
+if options.dry_run:
+show_actions(series, why_selected, selected, output_dir, options,
+board_warnings)
+return 0
+
 adjust_cfg = cfgutil.convert_list_to_dict(options.adjust_cfg)
 
 # Drop LOCALVERSION_AUTO since it changes the version string on every 
commit
@@ -414,48 +421,43 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 if make_func:
 builder.do_make = make_func
 
-# For a dry run, just show our actions as a sanity check
-if options.dry_run:
-show_actions(series, why_selected, selected, output_dir, options,
-board_warnings)
+builder.force_build = options.force_build
+builder.force_build_failures = options.force_build_failures
+builder.force_reconfig = options.force_reconfig
+builder.in_tree = options.in_tree
+
+# Work out which boards to build
+board_selected = brds.get_selected_dict()
+
+if series:
+commits = series.commits
+# Number the commits for test purposes
+for i, commit in enumerate(commits):
+commit.sequence = i
 else:
-builder.force_build = options.force_build
-builder.force_build_failures = options.force_build_failures
-builder.force_reconfig = options.force_reconfig
-builder.in_tree = options.in_tree
-
-# Work out which boards to build
-board_selected = brds.get_selected_dict()
-
-if series:
-commits = series.commits
-# Number the commits for test purposes
-for i, commit in enumerate(commits):
-commit.sequence = i
-else:
-commits = None
-
-if not options.ide:
-tprint(get_action_summary(options.summary, commits, board_selected,
-options))
-
-# We can't show function sizes without board details at present
-if options.show_bloat:
-options.show_detail = True
-builder.SetDisplayOptions(
-options.show_errors, options.show_sizes, options.show_detail,
-options.show_bloat, options.list_error_boards, options.show_config,
-options.show_environment, options.filter_dtb_warnings,
-options.filter_migration_warnings, options.ide)
-if options.summary:
-builder.ShowSummary(commits, board_selected)
-else:
-fail, warned, excs = builder.BuildBoards(
-commits, board_selected, options.keep_outputs, options.verbose)
-if excs:
-return 102
-if fail:
-return 100
-if warned and not options.ignore_warnings:
-return 101
+commits = None
+
+if not options.ide:
+tprint(get_action_summary(options.summary, commits, board_selected,
+options))
+
+# We can't show function sizes without board details at present
+if options.show_bloat:
+options.show_detail = True
+builder.SetDisplayOptions(
+options.show_errors, options.show_sizes, options.show_detail,
+options.show_bloat, options.list_error_boards, options.show_config,
+options.show_environment, options.filter_dtb_warnings,
+options.filter_migration_warnings, options.ide)
+if options.summary:
+builder.ShowSummary(commits, board_selected)
+else:
+fail, warned, excs = builder.BuildBoards(
+commits, board_selected, options.keep_outputs, options.verbose)
+if excs:
+return 102
+if fail:
+return 100
+if warned and not options.ignore_warnings:
+return 101
 return 0
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 08/58] buildman: Drop use of builder in show_actions()

[Simon Glass]

This function only needs the output directory from the builder. This is
passed into the builder, so just pass the same value to show_actions().
The avoids needing a builder to call show_actions().

Signed-off-by: Simon Glass 
---

 tools/buildman/control.py | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index cde705c1c956..8ecd124fdcf3 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -48,7 +48,7 @@ def get_action_summary(is_summary, commits, selected, 
options):
 return msg
 
 # pylint: disable=R0913
-def show_actions(series, why_selected, boards_selected, bldr, options,
+def show_actions(series, why_selected, boards_selected, output_dir, options,
  board_warnings):
 """Display a list of actions that we would take, if not a dry run.
 
@@ -61,7 +61,7 @@ def show_actions(series, why_selected, boards_selected, bldr, 
options,
 the value would be a list of board names.
 boards_selected: Dict of selected boards, key is target name,
 value is Board object
-bldr: The builder that will be used to build the commits
+output_dir (str): Output directory for builder
 options: Command line options object
 board_warnings: List of warnings obtained from board selected
 """
@@ -74,7 +74,7 @@ def show_actions(series, why_selected, boards_selected, bldr, 
options,
 commits = None
 print(get_action_summary(False, commits, boards_selected,
 options))
-print(f'Build directory: {bldr.base_dir}')
+print(f'Build directory: {output_dir}')
 if commits:
 for upto in range(0, len(series.commits), options.step):
 commit = series.commits[upto]
@@ -416,7 +416,7 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 
 # For a dry run, just show our actions as a sanity check
 if options.dry_run:
-show_actions(series, why_selected, selected, builder, options,
+show_actions(series, why_selected, selected, output_dir, options,
 board_warnings)
 else:
 builder.force_build = options.force_build
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 07/58] buildman: Move logic for -A up a little

[Simon Glass]

Move this up to just after the boards are calculated, so we can refactor
the code now below this function.

Signed-off-by: Simon Glass 
---

 tools/buildman/control.py | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index fd136e54baf9..cde705c1c956 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -289,6 +289,12 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 return 0 if okay else 2
 brds.read_boards(board_file)
 
+if options.print_prefix:
+err = show_toolchain_prefix(brds, toolchains)
+if err:
+sys.exit(col.build(col.RED, err))
+return 0
+
 exclude = []
 if options.exclude:
 for arg in options.exclude:
@@ -306,12 +312,6 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 if not selected:
 sys.exit(col.build(col.RED, 'No matching boards found'))
 
-if options.print_prefix:
-err = show_toolchain_prefix(brds, toolchains)
-if err:
-sys.exit(col.build(col.RED, err))
-return 0
-
 # Work out how many commits to build. We want to build everything on the
 # branch. We also build the upstream commit as a control so we can see
 # problems introduced by the first commit on the branch.
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 05/58] buildman: Move series calculations into a separate function

[Simon Glass]

Reduce the size of the do_buildman() function a little by moving the code
that figures out the series into a separate function.

Signed-off-by: Simon Glass 
---

 tools/buildman/control.py | 95 +++
 1 file changed, 56 insertions(+), 39 deletions(-)

diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index b27a6b08477f..7a81c2ced8b3 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -147,6 +147,51 @@ def get_allow_missing(opt_allow, opt_no_allow, 
num_selected, has_branch):
 allow_missing = False
 return allow_missing
 
+
+def determine_series(count, has_range, branch, git_dir):
+"""Determine the series which is to be built, if any
+
+Args:
+count (int): Number of commits in branch
+has_range (bool): True if a range of commits ('xx..yy') is being built
+branch (str): Name of branch to build, or None if none
+git_dir (str): Git directory to use, e.g. './.git'
+
+Returns:
+Series: Series to build, or None for none
+
+Read the metadata from the commits. First look at the upstream commit,
+then the ones in the branch. We would like to do something like
+upstream/master~..branch but that isn't possible if upstream/master is
+a merge commit (it will list all the commits that form part of the
+merge)
+
+Conflicting tags are not a problem for buildman, since it does not use
+them. For example, Series-version is not useful for buildman. On the
+other hand conflicting tags will cause an error. So allow later tags
+to overwrite earlier ones by setting allow_overwrite=True
+"""
+if branch:
+if count == -1:
+if has_range:
+range_expr = branch
+else:
+range_expr = gitutil.get_range_in_branch(git_dir, branch)
+upstream_commit = gitutil.get_upstream(git_dir, branch)
+series = patchstream.get_metadata_for_list(upstream_commit,
+git_dir, 1, series=None, allow_overwrite=True)
+
+series = patchstream.get_metadata_for_list(range_expr,
+git_dir, None, series, allow_overwrite=True)
+else:
+# Honour the count
+series = patchstream.get_metadata_for_list(branch,
+git_dir, count, series=None, allow_overwrite=True)
+else:
+series = None
+return series
+
+
 def do_buildman(options, args, toolchains=None, make_func=None, brds=None,
 clean_dir=False, test_thread_exceptions=False):
 """The main control code for buildman
@@ -174,7 +219,7 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 gitutil.setup()
 col = terminal.Color()
 
-options.git_dir = os.path.join(options.git, '.git')
+git_dir = os.path.join(options.git, '.git')
 
 no_toolchains = toolchains is None
 if no_toolchains:
@@ -264,11 +309,11 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 count = 1
 else:
 if has_range:
-count, msg = gitutil.count_commits_in_range(options.git_dir,
- options.branch)
+count, msg = gitutil.count_commits_in_range(git_dir,
+options.branch)
 else:
-count, msg = gitutil.count_commits_in_branch(options.git_dir,
-  options.branch)
+count, msg = gitutil.count_commits_in_branch(git_dir,
+ options.branch)
 if count is None:
 sys.exit(col.build(col.RED, msg))
 elif count == 0:
@@ -290,39 +335,11 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 sys.exit(col.build(col.RED,
'-w can only be used with a single commit'))
 
-# Read the metadata from the commits. First look at the upstream commit,
-# then the ones in the branch. We would like to do something like
-# upstream/master~..branch but that isn't possible if upstream/master is
-# a merge commit (it will list all the commits that form part of the
-# merge)
-# Conflicting tags are not a problem for buildman, since it does not use
-# them. For example, Series-version is not useful for buildman. On the
-# other hand conflicting tags will cause an error. So allow later tags
-# to overwrite earlier ones by setting allow_overwrite=True
-if options.branch:
-if count == -1:
-if has_range:
-range_expr = options.branch
-else:
-range_expr = gitutil.get_range_in_branch(options.git_dir,
-  options.branch)
-upstream_commit = 

[PATCH 06/58] buildman: Move fetch-arch code into a separate function

[Simon Glass]

Reduce the size of the do_buildman() function a little by moving the code
that handles --fetch-arch into a separate function.

Signed-off-by: Simon Glass 
---

 tools/buildman/control.py | 49 +--
 1 file changed, 31 insertions(+), 18 deletions(-)

diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index 7a81c2ced8b3..fd136e54baf9 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -192,6 +192,36 @@ def determine_series(count, has_range, branch, git_dir):
 return series
 
 
+def do_fetch_arch(toolchains, col, fetch_arch):
+"""Handle the --fetch-arch option
+
+Args:
+toolchains (Toolchains): Tool chains to use
+col (terminal.Color): Color object to build
+fetch_arch (str): Argument passed to the --fetch-arch option
+
+Returns:
+int: Return code for buildman
+"""
+if fetch_arch == 'list':
+sorted_list = toolchains.ListArchs()
+print(col.build(
+col.BLUE,
+f"Available architectures: {' '.join(sorted_list)}\n"))
+return 0
+
+if fetch_arch == 'all':
+fetch_arch = ','.join(toolchains.ListArchs())
+print(col.build(col.CYAN,
+f'\nDownloading toolchains: {fetch_arch}'))
+for arch in fetch_arch.split(','):
+print()
+ret = toolchains.FetchAndInstall(arch)
+if ret:
+return ret
+return 0
+
+
 def do_buildman(options, args, toolchains=None, make_func=None, brds=None,
 clean_dir=False, test_thread_exceptions=False):
 """The main control code for buildman
@@ -226,24 +256,7 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 toolchains = toolchain.Toolchains(options.override_toolchain)
 
 if options.fetch_arch:
-if options.fetch_arch == 'list':
-sorted_list = toolchains.ListArchs()
-print(col.build(
-col.BLUE,
-f"Available architectures: {' '.join(sorted_list)}\n"))
-return 0
-
-fetch_arch = options.fetch_arch
-if fetch_arch == 'all':
-fetch_arch = ','.join(toolchains.ListArchs())
-print(col.build(col.CYAN,
-f'\nDownloading toolchains: {fetch_arch}'))
-for arch in fetch_arch.split(','):
-print()
-ret = toolchains.FetchAndInstall(arch)
-if ret:
-return ret
-return 0
+return do_fetch_arch(toolchains, col, options.fetch_arch)
 
 if no_toolchains:
 toolchains.GetSettings()
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 04/58] buildman: Move full-help processing to main

[Simon Glass]

This does not need any of the control features. Move it out of main to
reduce the size of the do_buildman() function.

For Python 3.6 the -H feature will not work, but this does not seem to be
a huge problem, as it dates from 2016.

Signed-off-by: Simon Glass 
---

 tools/buildman/control.py | 11 ---
 tools/buildman/main.py|  9 +
 2 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index c890f778f501..b27a6b08477f 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -8,11 +8,6 @@ This holds the main control logic for buildman, when not 
running tests.
 """
 
 import multiprocessing
-try:
-import importlib.resources
-except ImportError:
-# for Python 3.6
-import importlib_resources
 import os
 import shutil
 import sys
@@ -26,7 +21,6 @@ from patman import gitutil
 from patman import patchstream
 from u_boot_pylib import command
 from u_boot_pylib import terminal
-from u_boot_pylib import tools
 from u_boot_pylib.terminal import tprint
 
 TEST_BUILDER = None
@@ -177,11 +171,6 @@ def do_buildman(options, args, toolchains=None, 
make_func=None, brds=None,
 # Used so testing can obtain the builder: pylint: disable=W0603
 global TEST_BUILDER
 
-if options.full_help:
-with importlib.resources.path('buildman', 'README.rst') as readme:
-tools.print_full_help(str(readme))
-return 0
-
 gitutil.setup()
 col = terminal.Color()
 
diff --git a/tools/buildman/main.py b/tools/buildman/main.py
index 9c84e16e7df0..2253401709a8 100755
--- a/tools/buildman/main.py
+++ b/tools/buildman/main.py
@@ -6,6 +6,11 @@
 
 """See README for more information"""
 
+try:
+from importlib.resources import files
+except ImportError:
+# for Python 3.6
+import importlib_resources
 import os
 import sys
 
@@ -19,6 +24,7 @@ from buildman import bsettings
 from buildman import cmdline
 from buildman import control
 from u_boot_pylib import test_util
+from u_boot_pylib import tools
 
 def run_tests(skip_net_tests, verbose, args):
 """Run the buildman tests
@@ -62,6 +68,9 @@ def run_buildman():
 if cmdline.HAS_TESTS and options.test:
 run_tests(options.skip_net_tests, options.verbose, args)
 
+elif options.full_help:
+tools.print_full_help(str(files('buildman').joinpath('README.rst')))
+
 # Build selected commits for selected boards
 else:
 bsettings.Setup(options.config_file)
-- 
2.41.0.255.g8b1d071c50-goog

[PATCH 02/58] buildman: Convert camel case in control.py

[Simon Glass]

Convert this file to snake case and update all files which use it.

Signed-off-by: Simon Glass 
---

 tools/buildman/control.py   | 31 ++-
 tools/buildman/func_test.py |  7 ---
 tools/buildman/main.py  |  2 +-
 tools/buildman/test.py  |  2 +-
 4 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index b8286e185411..bfb02834e8fb 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -2,6 +2,11 @@
 # Copyright (c) 2013 The Chromium OS Authors.
 #
 
+"""Control module for buildman
+
+This holds the main control logic for buildman, when not running tests.
+"""
+
 import multiprocessing
 try:
 import importlib.resources
@@ -25,11 +30,11 @@ from u_boot_pylib import terminal
 from u_boot_pylib import tools
 from u_boot_pylib.terminal import tprint
 
-def GetPlural(count):
+def get_plural(count):
 """Returns a plural 's' if count is not 1"""
 return 's' if count != 1 else ''
 
-def GetActionSummary(is_summary, commits, selected, options):
+def get_action_summary(is_summary, commits, selected, options):
 """Return a string summarising the intended action.
 
 Returns:
@@ -38,18 +43,18 @@ def GetActionSummary(is_summary, commits, selected, 
options):
 if commits:
 count = len(commits)
 count = (count + options.step - 1) // options.step
-commit_str = '%d commit%s' % (count, GetPlural(count))
+commit_str = '%d commit%s' % (count, get_plural(count))
 else:
 commit_str = 'current source'
 str = '%s %s for %d boards' % (
 'Summary of' if is_summary else 'Building', commit_str,
 len(selected))
 str += ' (%d thread%s, %d job%s per thread)' % (options.threads,
-GetPlural(options.threads), options.jobs, GetPlural(options.jobs))
+get_plural(options.threads), options.jobs, 
get_plural(options.jobs))
 return str
 
-def ShowActions(series, why_selected, boards_selected, builder, options,
-board_warnings):
+def show_actions(series, why_selected, boards_selected, builder, options,
+ board_warnings):
 """Display a list of actions that we would take, if not a dry run.
 
 Args:
@@ -72,7 +77,7 @@ def ShowActions(series, why_selected, boards_selected, 
builder, options,
 commits = series.commits
 else:
 commits = None
-print(GetActionSummary(False, commits, boards_selected,
+print(get_action_summary(False, commits, boards_selected,
 options))
 print('Build directory: %s' % builder.base_dir)
 if commits:
@@ -92,7 +97,7 @@ def ShowActions(series, why_selected, boards_selected, 
builder, options,
 for warning in board_warnings:
 print(col.build(col.YELLOW, warning))
 
-def ShowToolchainPrefix(brds, toolchains):
+def show_toolchain_prefix(brds, toolchains):
 """Show information about a the tool chain used by one or more boards
 
 The function checks that all boards use the same toolchain, then prints
@@ -133,8 +138,8 @@ def get_allow_missing(opt_allow, opt_no_allow, 
num_selected, has_branch):
 allow_missing = False
 return allow_missing
 
-def DoBuildman(options, args, toolchains=None, make_func=None, brds=None,
-   clean_dir=False, test_thread_exceptions=False):
+def do_buildman(options, args, toolchains=None, make_func=None, brds=None,
+clean_dir=False, test_thread_exceptions=False):
 """The main control code for buildman
 
 Args:
@@ -237,7 +242,7 @@ def DoBuildman(options, args, toolchains=None, 
make_func=None, brds=None,
 sys.exit(col.build(col.RED, 'No matching boards found'))
 
 if options.print_prefix:
-err = ShowToolchainPrefix(brds, toolchains)
+err = show_toolchain_prefix(brds, toolchains)
 if err:
 sys.exit(col.build(col.RED, err))
 return 0
@@ -373,7 +378,7 @@ def DoBuildman(options, args, toolchains=None, 
make_func=None, brds=None,
 
 # For a dry run, just show our actions as a sanity check
 if options.dry_run:
-ShowActions(series, why_selected, selected, builder, options,
+show_actions(series, why_selected, selected, builder, options,
 board_warnings)
 else:
 builder.force_build = options.force_build
@@ -393,7 +398,7 @@ def DoBuildman(options, args, toolchains=None, 
make_func=None, brds=None,
 commits = None
 
 if not options.ide:
-tprint(GetActionSummary(options.summary, commits, board_selected,
+tprint(get_action_summary(options.summary, commits, board_selected,
 options))
 
 # We can't show function sizes without board details at present
diff --git a/tools/buildman/func_test.py b/tools/buildman/func_test.py
index 08b2714b7be2..052153043b21 100644
--- a/tools/buildman/func_test.py
+++ b/tools/buildman/func_test.py
@@ 

[PATCH 03/58] buildman: Fix most pylint warnings in control

[Simon Glass]

Tidy up the easier-to-fix pylint warnings in module 'control'.

Signed-off-by: Simon Glass 
---

 tools/buildman/control.py   | 119 +---
 tools/buildman/func_test.py |   2 +-
 2 files changed, 70 insertions(+), 51 deletions(-)

diff --git a/tools/buildman/control.py b/tools/buildman/control.py
index bfb02834e8fb..c890f778f501 100644
--- a/tools/buildman/control.py
+++ b/tools/buildman/control.py
@@ -15,7 +15,6 @@ except ImportError:
 import importlib_resources
 import os
 import shutil
-import subprocess
 import sys
 
 from buildman import boards
@@ -30,6 +29,8 @@ from u_boot_pylib import terminal
 from u_boot_pylib import tools
 from u_boot_pylib.terminal import tprint
 
+TEST_BUILDER = None
+
 def get_plural(count):
 """Returns a plural 's' if count is not 1"""
 return 's' if count != 1 else ''
@@ -43,17 +44,17 @@ def get_action_summary(is_summary, commits, selected, 
options):
 if commits:
 count = len(commits)
 count = (count + options.step - 1) // options.step
-commit_str = '%d commit%s' % (count, get_plural(count))
+commit_str = f'{count} commit{get_plural(count)}'
 else:
 commit_str = 'current source'
-str = '%s %s for %d boards' % (
-'Summary of' if is_summary else 'Building', commit_str,
-len(selected))
-str += ' (%d thread%s, %d job%s per thread)' % (options.threads,
-get_plural(options.threads), options.jobs, 
get_plural(options.jobs))
-return str
-
-def show_actions(series, why_selected, boards_selected, builder, options,
+msg = (f"{'Summary of' if is_summary else 'Building'} "
+   f'{commit_str} for {len(selected)} boards')
+msg += (f' ({options.threads} thread{get_plural(options.threads)}, '
+f'{options.jobs} job{get_plural(options.jobs)} per thread)')
+return msg
+
+# pylint: disable=R0913
+def show_actions(series, why_selected, boards_selected, bldr, options,
  board_warnings):
 """Display a list of actions that we would take, if not a dry run.
 
@@ -66,7 +67,7 @@ def show_actions(series, why_selected, boards_selected, 
builder, options,
 the value would be a list of board names.
 boards_selected: Dict of selected boards, key is target name,
 value is Board object
-builder: The builder that will be used to build the commits
+bldr: The builder that will be used to build the commits
 options: Command line options object
 board_warnings: List of warnings obtained from board selected
 """
@@ -79,7 +80,7 @@ def show_actions(series, why_selected, boards_selected, 
builder, options,
 commits = None
 print(get_action_summary(False, commits, boards_selected,
 options))
-print('Build directory: %s' % builder.base_dir)
+print(f'Build directory: {bldr.base_dir}')
 if commits:
 for upto in range(0, len(series.commits), options.step):
 commit = series.commits[upto]
@@ -88,11 +89,11 @@ def show_actions(series, why_selected, boards_selected, 
builder, options,
 print()
 for arg in why_selected:
 if arg != 'all':
-print(arg, ': %d boards' % len(why_selected[arg]))
+print(arg, f': {len(why_selected[arg])} boards')
 if options.verbose:
-print('   %s' % ' '.join(why_selected[arg]))
-print(('Total boards to build for each commit: %d\n' %
-len(why_selected['all'])))
+print(f"   {' '.join(why_selected[arg])}")
+print('Total boards to build for each '
+  f"commit: {len(why_selected['all'])}\n")
 if board_warnings:
 for warning in board_warnings:
 print(col.build(col.YELLOW, warning))
@@ -116,12 +117,26 @@ def show_toolchain_prefix(brds, toolchains):
 tc_set.add(toolchains.Select(brd.arch))
 if len(tc_set) != 1:
 return 'Supplied boards must share one toolchain'
-return False
-tc = tc_set.pop()
-print(tc.GetEnvArgs(toolchain.VAR_CROSS_COMPILE))
+tchain = tc_set.pop()
+print(tchain.GetEnvArgs(toolchain.VAR_CROSS_COMPILE))
 return None
 
 def get_allow_missing(opt_allow, opt_no_allow, num_selected, has_branch):
+"""Figure out whether to allow external blobs
+
+Uses the allow-missing setting and the provided arguments to decide whether
+missing external blobs should be allowed
+
+Args:
+opt_allow (bool): True if --allow-missing flag is set
+opt_no_allow (bool): True if --no-allow-missing flag is set
+num_selected (int): Number of selected board
+has_branch (bool): True if a git branch (to build) has been provided
+
+Returns:
+bool: True to allow missing external blobs, False to produce an error 
if
+external blobs are used
+"""
 allow_missing = False
 am_setting = bsettings.GetGlobalItemValue('allow-missing')
 if am_setting:
@@ -159,7 

[PATCH 01/58] buildman: Tidy up pylint warnings in main

[Simon Glass]

Tidy up the various pylint warnings in module 'main'.

Signed-off-by: Simon Glass 
---

 tools/buildman/main.py | 23 +++
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/tools/buildman/main.py b/tools/buildman/main.py
index a2ffbc9073e7..ca1beb098292 100755
--- a/tools/buildman/main.py
+++ b/tools/buildman/main.py
@@ -6,29 +6,21 @@
 
 """See README for more information"""
 
-import doctest
-import multiprocessing
 import os
-import re
 import sys
 
 # Bring in the patman libraries
+# pylint: disable=C0413
 our_path = os.path.dirname(os.path.realpath(__file__))
 sys.path.insert(1, os.path.join(our_path, '..'))
 
 # Our modules
-from buildman import board
 from buildman import bsettings
-from buildman import builder
 from buildman import cmdline
 from buildman import control
-from buildman import toolchain
-from patman import patchstream
-from patman import gitutil
-from u_boot_pylib import terminal
 from u_boot_pylib import test_util
 
-def RunTests(skip_net_tests, verbose, args):
+def run_tests(skip_net_tests, verbose, args):
 """Run the buildman tests
 
 Args:
@@ -36,9 +28,11 @@ def RunTests(skip_net_tests, verbose, args):
 verbosity (int): Verbosity level to use (0-4)
 args (list of str): List of tests to run, empty to run all
 """
+# These imports are here since tests are not available when buildman is
+# installed as a Python module
+# pylint: disable=C0415
 from buildman import func_test
 from buildman import test
-import doctest
 
 test_name = args and args[0] or None
 if skip_net_tests:
@@ -54,6 +48,11 @@ def RunTests(skip_net_tests, verbose, args):
 return (0 if result.wasSuccessful() else 1)
 
 def run_buildman():
+"""Run bulidman
+
+This is the main program. It collects arguments and runs either the tests 
or
+the control module.
+"""
 options, args = cmdline.ParseArgs()
 
 if not options.debug:
@@ -61,7 +60,7 @@ def run_buildman():
 
 # Run our meagre tests
 if cmdline.HAS_TESTS and options.test:
-RunTests(options.skip_net_tests, options.verbose, args)
+run_tests(options.skip_net_tests, options.verbose, args)
 
 # Build selected commits for selected boards
 else:
-- 
2.41.0.255.g8b1d071c50-goog