Hi Kosta,

On 20.11.2016 16:38, kostap@marvell.com wrote:

From: Konstantin Porotchkin kostap@marvell.com

Align the Armada-8040-db SPI and I2C DTS settings with latest A8040 DB settings:

• disable i2c0 and spi0 on AP (pins are reserved for eMMC)
• disable cps_i2c0 on CP1
• enable spi1 on CP1 (the new location of the boot flash)

Thanks. I understand that the current SPI / I2C settings are still valid for boards with earlier SoC revisions. Is this correct? Would it make sense to move the old version into a new file then, perhaps:

arch/arm/dts/armada-8040-db-revA.dts

?

This would be handy for users of this version at least for a short period of time. This new file can be removed once its not needed any more in a few months.

If you think this is a good idea, could you please add this new file in a new patch to this patchset in v2?

Change-Id: I54698ce4dc8dbe6a2af14099f5bcc3ca3b21d7e1 Signed-off-by: Konstantin Porotchkin kostap@marvell.com Cc: Stefan Roese sr@denx.de Cc: Nadav Haklai nadavh@marvell.com Cc: Neta Zur Hershkovits neta@marvell.com Cc: Omri Itach omrii@marvell.com Cc: Igal Liberman igall@marvell.com Cc: Haim Boot hayim@marvell.com Cc: Hanna Hawa hannah@marvell.com

---

arch/arm/dts/armada-8040-db.dts | 60 +++++++++++++++++------------------------ 1 file changed, 25 insertions(+), 35 deletions(-)

diff --git a/arch/arm/dts/armada-8040-db.dts b/arch/arm/dts/armada-8040-db.dts index 7fb674b..86666a1 100644 --- a/arch/arm/dts/armada-8040-db.dts +++ b/arch/arm/dts/armada-8040-db.dts @@ -57,7 +57,7 @@

aliases { i2c0 = &cpm_i2c0;

• spi0 = &spi0;
  

• spi0 = &cps_spi1;
  

  };

  memory@00000000 {

@@ -66,38 +66,6 @@ }; };

-&i2c0 {

• status = "okay";
• clock-frequency = <100000>;

-};

-&spi0 {

• status = "okay";
• spi-flash@0 {

• #address-cells = <1>;
  

• #size-cells = <1>;
  

• compatible = "jedec,spi-nor";
  

• reg = <0>;
  

• spi-max-frequency = <10000000>;
  

• partitions {
  

• 	compatible = "fixed-partitions";
  

• 	#address-cells = <1>;
  

• 	#size-cells = <1>;
  

• 	partition@0 {
  

• 		label = "U-Boot";
  

• 		reg = <0 0x200000>;
  

• 	};
  

• 	partition@400000 {
  

• 		label = "Filesystem";
  

• 		reg = <0x200000 0xce0000>;
  

• 	};
  

• };
  

• };

-};

/* Accessible over the mini-USB CON9 connector on the main board */ &uart0 { status = "okay"; @@ -134,9 +102,31 @@ status = "okay"; };

-&cps_i2c0 { +&cps_spi1 { status = "okay";

• clock-frequency = <100000>;

• spi-flash@0 {

• #address-cells = <1>;
  

• #size-cells = <1>;
  

• compatible = "jedec,spi-nor";
  

• reg = <0>;
  

• spi-max-frequency = <10000000>;
  

• partitions {
  

• 	compatible = "fixed-partitions";
  

• 	#address-cells = <1>;
  

• 	#size-cells = <1>;
  

• 	partition@0 {
  

• 		label = "U-Boot";
  

• 		reg = <0 0x200000>;
  

• 	};
  

• 	partition@400000 {
  

• 		label = "Filesystem";
  

• 		reg = <0x200000 0xce0000>;
  

• 	};
  

• };
  

• };

};

/* CON4 on CP1 expansion */

Thanks, Stefan

Hi Kosta,

a general comment: Please use scripts/checkpatch before sending the patches to the list to make sure that all (mostly style related) issues are resolved.

Please find some more mostly nitpicking comments below.

On 20.11.2016 16:38, kostap@marvell.com wrote:

From: Konstantin Porotchkin kostap@marvell.com

Add support for mvebu bubt command for flash image load, check and burn on boot device.

Could you please extent the patch (cmd) description here a bit? Perhaps by adding an example on how this command can be used to load and update image (loading via tftp, buring into SPI...).

Change-Id: If2b971069ae44232761b601bbbcf0162567f5603 Signed-off-by: Konstantin Porotchkin kostap@marvell.com Cc: Stefan Roese sr@denx.de Cc: Nadav Haklai nadavh@marvell.com Cc: Neta Zur Hershkovits neta@marvell.com Cc: Omri Itach omrii@marvell.com Cc: Igal Liberman igall@marvell.com Cc: Haim Boot hayim@marvell.com Cc: Hanna Hawa hannah@marvell.com

---

arch/arm/mach-mvebu/Kconfig | 30 ++ cmd/Kconfig | 3 + cmd/Makefile | 2 + cmd/mvebu/Kconfig | 10 + cmd/mvebu/Makefile | 19 ++ cmd/mvebu/bubt.c | 699 ++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 763 insertions(+) create mode 100644 cmd/mvebu/Kconfig create mode 100644 cmd/mvebu/Makefile create mode 100644 cmd/mvebu/bubt.c

diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig index 7248fd7..6de85d5 100644 --- a/arch/arm/mach-mvebu/Kconfig +++ b/arch/arm/mach-mvebu/Kconfig @@ -99,6 +99,36 @@ config TARGET_THEADORABLE

endchoice

+choice

• prompt "Flash for image"
• default MVEBU_SPI_BOOT
• depends on (TARGET_MVEBU_DB_88F3720 || TARGET_MVEBU_ARMADA_8K)

+config MVEBU_NAND_BOOT

• bool "NAND flash boot"
• depends on NAND_PXA3XX
• help

•  Enable boot from NAND
  

+config MVEBU_SPI_BOOT

• bool "SPI flash boot"
• depends on SPI_FLASH

+config MVEBU_MMC_BOOT

• bool "eMMC flash boot"
• depends on MVEBU_MMC
• help

•  Enable boot from eMMC boot partition
  

+endchoice

+config MVEBU_UBOOT_DFLT_NAME

• string "Default image name for bubt command"
• default "flash-image.bin"
• help

•   This option should contain a default file name to be used with
  

•   MVEBU "bubt" command if the source file name is omitted
  

I'm not sure if these Kconfig options really below in this file - please see below...

config SYS_BOARD default "clearfog" if TARGET_CLEARFOG default "mvebu_db-88f3720" if TARGET_MVEBU_DB_88F3720 diff --git a/cmd/Kconfig b/cmd/Kconfig index e339d86..39dd0a0 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -610,6 +610,9 @@ config CMD_QFW This provides access to the QEMU firmware interface. The main feature is to allow easy loading of files passed to qemu-system via -kernel / -initrd

+source "cmd/mvebu/Kconfig"

endmenu

config CMD_BOOTSTAGE diff --git a/cmd/Makefile b/cmd/Makefile index 9c9a9d1..34bc544 100644 --- a/cmd/Makefile +++ b/cmd/Makefile @@ -163,3 +163,5 @@ obj-$(CONFIG_CMD_BLOB) += blob.o

# core command obj-y += nvedit.o

+obj-$(CONFIG_ARCH_MVEBU) += mvebu/

Do you plan to add move mvebu specific commands?

diff --git a/cmd/mvebu/Kconfig b/cmd/mvebu/Kconfig new file mode 100644 index 0000000..e7fbd20 --- /dev/null +++ b/cmd/mvebu/Kconfig @@ -0,0 +1,10 @@ +menu "MVEBU commands" +depends on ARCH_MVEBU

+config CMD_MVEBU_BUBT

• bool "bubt"
• default n
• help

•  bubt - Burn a u-boot image to flash
  

+endmenu

Here you introduce a new Kconfig file. Wouldn't it be better, to move all Kconfig option into this file instead of having most of them in the mach-mvebu file?

diff --git a/cmd/mvebu/Makefile b/cmd/mvebu/Makefile new file mode 100644 index 0000000..b0817e3 --- /dev/null +++ b/cmd/mvebu/Makefile @@ -0,0 +1,19 @@ +# +# *************************************************************************** +# Copyright (C) 2015 Marvell International Ltd. +# *************************************************************************** +# This program is free software: you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by the Free +# Software Foundation, either version 2 of the License, or any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see http://www.gnu.org/licenses/. +# *************************************************************************** +#

As already mentioned by Simon, please move to using SPDX license headers. You can and should of course keep your copyright notice. Please fix this globally. For SPDX in U-Boot I suggest to take a look at this page:

http://www.denx.de/wiki/U-Boot/Licensing

+obj-$(CONFIG_CMD_MVEBU_BUBT) += bubt.o diff --git a/cmd/mvebu/bubt.c b/cmd/mvebu/bubt.c new file mode 100644 index 0000000..6539063 --- /dev/null +++ b/cmd/mvebu/bubt.c @@ -0,0 +1,699 @@ +/*

• ---

• • Copyright (C) 2015 Marvell International Ltd.

• ---

• • This program is free software: you can redistribute it and/or modify it
• • under the terms of the GNU General Public License as published by the Free
• • Software Foundation, either version 2 of the License, or any later version.
• • This program is distributed in the hope that it will be useful,
• • but WITHOUT ANY WARRANTY; without even the implied warranty of
• • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
• • GNU General Public License for more details.
• • You should have received a copy of the GNU General Public License
• • along with this program. If not, see http://www.gnu.org/licenses/.

• ---

• */

+#include <config.h> +#include <common.h> +#include <command.h> +#include <vsprintf.h> +#include <errno.h> +#include <dm.h>

+#include <spi_flash.h> +#include <spi.h> +#include <nand.h> +#include <usb.h> +#include <fs.h> +#include <mmc.h> +#include <u-boot/sha1.h> +#include <u-boot/sha256.h>

+#if defined(CONFIG_TARGET_MVEBU_ARMADA_8K) +#define MAIN_HDR_MAGIC 0xB105B002

As already mentioned by Simon, please move to using lower case characters for hex numbers.

+struct mvebu_image_header {

• uint32_t magic; /* 0-3 */
• uint32_t prolog_size; /* 4-7 */
• uint32_t prolog_checksum; /* 8-11 */
• uint32_t boot_image_size; /* 12-15 */
• uint32_t boot_image_checksum; /* 16-19 */
• uint32_t rsrvd0; /* 20-23 */
• uint32_t load_addr; /* 24-27 */
• uint32_t exec_addr; /* 28-31 */
• uint8_t uart_cfg; /* 32 */
• uint8_t baudrate; /* 33 */
• uint8_t ext_count; /* 34 */
• uint8_t aux_flags; /* 35 */
• uint32_t io_arg_0; /* 36-39 */
• uint32_t io_arg_1; /* 40-43 */
• uint32_t io_arg_2; /* 43-47 */
• uint32_t io_arg_3; /* 48-51 */
• uint32_t rsrvd1; /* 52-55 */
• uint32_t rsrvd2; /* 56-59 */
• uint32_t rsrvd3; /* 60-63 */

+}; +#elif defined(CONFIG_TARGET_MVEBU_DB_88F3720) /* A3700 */ +#define HASH_SUM_LEN 16 +#define IMAGE_VERSION_3_6_0 0x030600 +#define IMAGE_VERSION_3_5_0 0x030500

+struct common_tim_data {

• u32 version;
• u32 identifier;
• u32 trusted;
• u32 issue_date;
• u32 oem_unique_id;
• u32 reserved[5]; /* Reserve 20 bytes */
• u32 boot_flash_sign;
• u32 num_images;
• u32 num_keys;
• u32 size_of_reserved;

+};

+struct mvebu_image_info {

• u32 image_id;
• u32 next_image_id;
• u32 flash_entry_addr;
• u32 load_addr;
• u32 image_size;
• u32 image_size_to_hash;
• u32 hash_algorithm_id;
• u32 hash[HASH_SUM_LEN]; /* Reserve 512 bits for the hash */
• u32 partition_number;
• u32 enc_algorithm_id;
• u32 encrypt_start_offset;
• u32 encrypt_size;

+}; +#endif

+struct bubt_dev {

• char name[8];
• size_t (*read)(const char *file_name);
• int (*write)(size_t image_size);
• int (*active)(void);

+};

+static ulong get_load_addr(void) +{

• const char *addr_str;
• unsigned long addr;
• addr_str = getenv("loadaddr");
• if (addr_str != NULL)

• addr = simple_strtoul(addr_str, NULL, 16);
  

• else

• addr = CONFIG_SYS_LOAD_ADDR;
  

• return addr;

+}

+/********************************************************************

• • eMMC services
    

• ********************************************************************/

+#ifdef CONFIG_DM_MMC +static int mmc_burn_image(size_t image_size) +{

• struct mmc *mmc;
• lbaint_t start_lba;
• lbaint_t blk_count;
• ulong blk_written;

+#ifdef CONFIG_SYS_MMC_ENV_DEV

• const u8 mmc_dev_num = CONFIG_SYS_MMC_ENV_DEV;

+#else

• const u8 mmc_dev_num = 0;

+#endif

I suggest to move this one up in this file to make the code look a bit "cleaner", like this:

#ifndef CONFIG_SYS_MMC_ENV_DEV #define CONFIG_SYS_MMC_ENV_DEV 0 #endif

Then you can remove the #ifdef from the code here.

• mmc = find_mmc_device(mmc_dev_num);
• if (!mmc) {

• printf("No SD/MMC/eMMC card found\n");
  

• return 1;
  

• }

Does this bubt command only support eMMC or SD/MMC as well? If so, please mention this in the Kconfig options too, as they only list eMMC (IIRC).

• if (mmc_init(mmc)) {

• printf("%s(%d) init failed\n", IS_SD(mmc) ? "SD" : "MMC", mmc_dev_num);
  

• return 1;
  

• }

+#ifdef CONFIG_SYS_MMC_ENV_PART

• if (CONFIG_SYS_MMC_ENV_PART != mmc->part_num) {

• if (mmc_switch_part(mmc_dev_num, CONFIG_SYS_MMC_ENV_PART)) {
  

• 	printf("MMC partition switch failed\n");
  

• 	return 1;
  

• }
  

• }

+#endif

• /* SD reserves LBA-0 for MBR and boots from LBA-1, MMC/eMMC boots from LBA-0 */
• start_lba = IS_SD(mmc) ? 1 : 0;
• blk_count = image_size / mmc->block_dev.blksz;
• if (image_size % mmc->block_dev.blksz)

• blk_count += 1;
  

• blk_written = mmc->block_dev.block_write(mmc_dev_num,

• 				start_lba, blk_count, (void *)get_load_addr());
  

• if (blk_written != blk_count) {

• printf("Error - written %#lx blocks\n", blk_written);
  

• return 1;
  

• } else {

• printf("Done!\n");
  

• }

+#ifdef CONFIG_SYS_MMC_ENV_PART

• if (CONFIG_SYS_MMC_ENV_PART != mmc->part_num)

• mmc_switch_part(mmc_dev_num, mmc->part_num);
  

+#endif

• return 0;

+}

+static size_t mmc_read_file(const char *file_name) +{

• loff_t act_read = 0;
• int rc;
• struct mmc *mmc;

+#ifdef CONFIG_SYS_MMC_ENV_DEV

• const u8 mmc_dev_num = CONFIG_SYS_MMC_ENV_DEV;

+#else

• const u8 mmc_dev_num = 0;

+#endif

Again, please use the construct suggested above.

• mmc = find_mmc_device(mmc_dev_num);
• if (!mmc) {

• printf("No SD/MMC/eMMC card found\n");
  

• return 0;
  

• }
• if (mmc_init(mmc)) {

• printf("%s(%d) init failed\n", IS_SD(mmc) ? "SD" : "MMC", mmc_dev_num);
  

• return 0;
  

• }
• /* Load from data partition (0) */
• if (fs_set_blk_dev("mmc", "0", FS_TYPE_ANY)) {

• printf("Error: MMC 0 not found\n");
  

• return 0;
  

• }
• /* Perfrom file read */
• rc = fs_read(file_name, get_load_addr(), 0, 0, &act_read);
• if (rc)

• return 0;
  

• return act_read;

+}

+int is_mmc_active(void) +{

• return 1;

+} +#else /* CONFIG_DM_MMC */ +#define mmc_burn_image 0 +#define mmc_read_file 0 +#define is_mmc_active 0

Please use empty functions instead.

+#endif /* CONFIG_DM_MMC */

+/********************************************************************

• • SPI services
    

• ********************************************************************/

+#ifdef CONFIG_SPI_FLASH +static int spi_burn_image(size_t image_size) +{

• int ret;
• struct spi_flash *flash;
• uint32_t erase_bytes;
• /* Probe the SPI bus to get the flash device */
• flash = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS,

• 		CONFIG_SF_DEFAULT_SPEED, CONFIG_SF_DEFAULT_MODE);
  

• if (!flash) {

• printf("Failed to probe SPI Flash\n");
  

• return 1;
  

• }

+#ifdef CONFIG_SPI_FLASH_PROTECTION

• spi_flash_protect(flash, 0);

+#endif

• erase_bytes = image_size + (flash->erase_size - image_size % flash->erase_size);
• printf("Erasing %d bytes (%d blocks) at offset 0 ...", erase_bytes, erase_bytes/flash->erase_size);
• ret = spi_flash_erase(flash, 0, erase_bytes);
• if (ret)

• printf("Error!\n");
  

• else

• printf("Done!\n");
  

• printf("Writing %d bytes from 0x%lx to offset 0 ...", (int)image_size, get_load_addr());
• ret = spi_flash_write(flash, 0, image_size, (void *)get_load_addr());
• if (ret)

• printf("Error!\n");
  

• else

• printf("Done!\n");
  

+#ifdef CONFIG_SPI_FLASH_PROTECTION

• spi_flash_protect(flash, 1);

+#endif

• return ret;

+}

+int is_spi_active(void) +{

• return 1;

+} +#else /* CONFIG_SPI_FLASH */ +#define spi_burn_image 0 +#define is_spi_active 0

Empty functions please.

+#endif /* CONFIG_SPI_FLASH */

+/********************************************************************

• • NAND services
    

• ********************************************************************/

+#ifdef CONFIG_CMD_NAND +static int nand_burn_image(size_t image_size) +{

• int ret, block_size;
• nand_info_t *nand;
• int dev = nand_curr_device;
• if ((dev < 0) || (dev >= CONFIG_SYS_MAX_NAND_DEVICE) ||

•    (!nand_info[dev].name)) {
  

• puts("\nno devices available\n");
  

• return 1;
  

• }
• nand = &nand_info[dev];
• block_size = nand->erasesize;
• /* Align U-Boot size to currently used blocksize */
• image_size = ((image_size + (block_size - 1)) & (~(block_size-1)));

checkpatch will most likely complain about the missing space before and after the "-" above.

• /* Erase the U-BOOT image space */
• printf("Erasing 0x%x - 0x%x:...", 0, (int)image_size);
• ret = nand_erase(nand, 0, image_size);
• if (ret) {

• printf("Error!\n");
  

• goto error;
  

• }
• printf("Done!\n");
• /* Write the image to flash */
• printf("Writing image:...");
• printf("&image_size = 0x%p\n", (void*)&image_size);
• ret = nand_write(nand, 0, &image_size, (void *)get_load_addr());
• if (ret)

• printf("Error!\n");
  

• else

• printf("Done!\n");
  

+error:

• return ret;

+}

+int is_nand_active(void) +{

• return 1;

+} +#else /* CONFIG_CMD_NAND */ +#define nand_burn_image 0 +#define is_nand_active 0

Empty functions please.

+#endif /* CONFIG_CMD_NAND */

+/********************************************************************

• • USB services
    

• ********************************************************************/

+#if defined(CONFIG_USB_STORAGE) && defined (CONFIG_BLK) +static size_t usb_read_file(const char *file_name) +{

• loff_t act_read = 0;
• struct udevice *dev;
• int rc;
• usb_stop();
• if (usb_init() < 0) {

• printf("Error: usb_init failed\n");
  

• return 0;
  

• }
• /* Try to recognize storage devices immediately */
• blk_first_device(IF_TYPE_USB, &dev);
• if (dev == NULL) {

• printf("Error: USB storage device not found\n");
  

• return 0;
  

• }
• /* Always load from usb 0 */
• if (fs_set_blk_dev("usb", "0", FS_TYPE_ANY)) {

• printf("Error: USB 0 not found\n");
  

• return 0;
  

• }
• /* Perfrom file read */
• rc = fs_read(file_name, get_load_addr(), 0, 0, &act_read);
• if (rc)

• return 0;
  

• return act_read;

+}

+int is_usb_active(void) +{

• return 1;

+} +#else /* defined(CONFIG_USB_STORAGE) && defined (CONFIG_BLK) */ +#define usb_read_file 0 +#define is_usb_active 0 +#endif /* defined(CONFIG_USB_STORAGE) && defined (CONFIG_BLK) */

+/********************************************************************

• • Network services
    

• ********************************************************************/

+#ifdef CONFIG_CMD_NET +static size_t tftp_read_file(const char *file_name) +{

• /* update global variable load_addr before tftp file from network */
• load_addr = get_load_addr();
• return net_loop(TFTPGET);

+}

+int is_tftp_active(void) +{

• return 1;

+} +#else +#define tftp_read_file 0 +#define is_tftp_active 0 +#endif /* CONFIG_CMD_NET */

+enum bubt_devices {

• BUBT_DEV_NET = 0,
• BUBT_DEV_USB,
• BUBT_DEV_MMC,
• BUBT_DEV_SPI,
• BUBT_DEV_NAND,
• BUBT_MAX_DEV

+};

+struct bubt_dev bubt_devs[BUBT_MAX_DEV] = {

• {"tftp", tftp_read_file, NULL, is_tftp_active},
• {"usb", usb_read_file, NULL, is_usb_active},
• {"mmc", mmc_read_file, mmc_burn_image, is_mmc_active},
• {"spi", NULL, spi_burn_image, is_spi_active},
• {"nand", NULL, nand_burn_image, is_nand_active},

+};

+static int bubt_write_file(struct bubt_dev *dst, size_t image_size) +{

• if (!dst->write) {

• printf("Error: Write not supported on device %s\n", dst->name);
  

• return 1;
  

• }
• return dst->write(image_size);

+}

+#if defined(CONFIG_TARGET_MVEBU_ARMADA_8K) +uint32_t do_checksum32(uint32_t *start, int32_t len) +{

• uint32_t sum = 0;
• uint32_t *startp = start;
• do {

• sum += *startp;
  

• startp++;
  

• len -= 4;
  

• } while (len > 0);
• return sum;

+}

+static int check_image_header(void) +{

• struct mvebu_image_header *hdr = (struct mvebu_image_header *)get_load_addr();
• uint32_t header_len = hdr->prolog_size;
• uint32_t checksum;
• uint32_t checksum_ref = hdr->prolog_checksum;
• /*

• * For now compare checksum, and magic. Later we can
  

• * verify more stuff on the header like interface type, etc
  

• */
  

• if (hdr->magic != MAIN_HDR_MAGIC) {

• printf("ERROR: Bad MAGIC 0x%08x != 0x%08x\n", hdr->magic, MAIN_HDR_MAGIC);
  

• return -ENOEXEC;
  

• }
• /* The checksum value is discarded from checksum calculation */
• hdr->prolog_checksum = 0;
• checksum = do_checksum32((uint32_t *)hdr, header_len);
• if (checksum != checksum_ref) {

• printf("Error: Bad Image checksum. 0x%x != 0x%x\n", checksum, checksum_ref);
  

• return -ENOEXEC;
  

• }
• /* Restore the checksum before writing */
• hdr->prolog_checksum = checksum_ref;
• printf("Image checksum...OK!\n");
• return 0;

+} +#elif defined(CONFIG_TARGET_MVEBU_DB_88F3720) /* Armada 3700 */ +static int check_image_header(void) +{

• struct common_tim_data *hdr = (struct common_tim_data *)get_load_addr();
• int image_num;
• u8 hash_160_output[SHA1_SUM_LEN];
• u8 hash_256_output[SHA256_SUM_LEN];
• sha1_context hash1_text;
• sha256_context hash256_text;
• u8 *hash_output;
• u32 hash_algorithm_id;
• u32 image_size_to_hash;
• u32 flash_entry_addr;
• u32 *hash_value;
• u32 internal_hash[HASH_SUM_LEN];
• const uint8_t *buff;
• u32 num_of_image = hdr->num_images;
• u32 version = hdr->version;
• u32 trusted = hdr->trusted;
• /* bubt checksum validation only supports nontrusted images */
• if (trusted == 1) {

• printf("bypass image validation, only untrusted image is supported now\n");
  

• return 0;
  

• }
• /* only supports image version 3.5 and 3.6 */
• if (version != IMAGE_VERSION_3_5_0 && version != IMAGE_VERSION_3_6_0) {

• printf("Error: Unsupported Image version = 0x%08x\n", version);
  

• return -ENOEXEC;
  

• }
• /* validate images hash value */
• for (image_num = 0; image_num < num_of_image; image_num++) {

• struct mvebu_image_info *info = (struct mvebu_image_info *)(get_load_addr()
  

• 	     + sizeof(struct common_tim_data) + image_num * sizeof(struct mvebu_image_info));
  

• hash_algorithm_id = info->hash_algorithm_id;
  

• image_size_to_hash = info->image_size_to_hash;
  

• flash_entry_addr = info->flash_entry_addr;
  

• hash_value = info->hash;
  

• buff = (const uint8_t *)(get_load_addr() + flash_entry_addr);
  

• if (image_num == 0) {
  

• 	/* The first image includes hash values in itself content. For hash calculation, we need
  

• 	 * to save original hash values to local variable that will be copied back for comparsion
  

• 	 * and set all zeros to replace orignal hash values to calculate its new hash value.
  

• 	 * First image original format : x...x (datum1) x...x(original hash values) x...x(datum2)
  

• 	 * Replaced first image format : x...x (datum1) 0...0(hash values) x...x(datum2)
  

• 	 */
  

Multicomment style is:

/* * */

And please note the 80 colums limit.

• 	memcpy(internal_hash, hash_value, sizeof(internal_hash));
  

• 	memset(hash_value, 0, sizeof(internal_hash));
  

• }
  

• if (image_size_to_hash == 0) {
  

• 	printf("Warning: Image_%d hash checksum is disable, skip the image validation.\n", image_num);
  

• 	continue;
  

• }
  

• switch (hash_algorithm_id) {
  

• case SHA1_SUM_LEN:
  

• 	sha1_starts(&hash1_text);
  

• 	sha1_update(&hash1_text, buff, image_size_to_hash);
  

• 	sha1_finish(&hash1_text, hash_160_output);
  

• 	hash_output = hash_160_output;
  

• 	break;
  

• case SHA256_SUM_LEN:
  

• 	sha256_starts(&hash256_text);
  

• 	sha256_update(&hash256_text, buff, image_size_to_hash);
  

• 	sha256_finish(&hash256_text, hash_256_output);
  

• 	hash_output = hash_256_output;
  

• 	break;
  

• default:
  

• 	printf("Error: Unsupported hash_algorithm_id = %d\n", hash_algorithm_id);
  

• 	return -ENOEXEC;
  

• }
  

• if (image_num == 0)
  

• 	memcpy(hash_value, internal_hash, sizeof(internal_hash));
  

• if (memcmp(hash_value, hash_output, hash_algorithm_id) != 0) {
  

• 	printf("Error: Image_%d checksum is not correct\n", image_num);
  

• 	return -ENOEXEC;
  

• }
  

• }
• printf("Image checksum...OK!\n");
• return 0;

+} +#else +static int check_image_header(void) +{

• printf("bubt cmd does not support this SoC device or family!\n");
• return -ENOEXEC;

+} +#endif

+static int bubt_verify(size_t image_size) +{

• /* Check a correct image header exists */
• if (check_image_header()) {

• printf("Error: Image header verification failed\n");
  

• return 1;
  

• }
• return 0;

+}

+static int bubt_read_file(struct bubt_dev *src) +{

• size_t image_size;
• if (!src->read) {

• printf("Error: Read not supported on device \"%s\"\n", src->name);
  

• return 0;
  

• }
• image_size = src->read(net_boot_file_name);
• if (image_size <= 0) {

• printf("Error: Failed to read file %s from %s\n", net_boot_file_name, src->name);
  

• return 0;
  

• }
• return image_size;

+}

+static int bubt_is_dev_active(struct bubt_dev *dev) +{

• if (!dev->active) {

• printf("Device \"%s\" not supported by U-BOOT image\n", dev->name);
  

• return 0;
  

• }
• if (!dev->active()) {

• printf("Device \"%s\" is inactive\n", dev->name);
  

• return 0;
  

• }
• return 1;

+}

+struct bubt_dev *find_bubt_dev(char *dev_name) +{

• int dev;
• for (dev = 0; dev < BUBT_MAX_DEV; dev++) {

• if (strcmp(bubt_devs[dev].name, dev_name) == 0)
  

• 	return &bubt_devs[dev];
  

• }
• return 0;

+}

+#define DEFAULT_BUBT_SRC "tftp"

+#ifndef DEFAULT_BUBT_DST +#ifdef CONFIG_MVEBU_SPI_BOOT +#define DEFAULT_BUBT_DST "spi" +#elif defined(CONFIG_MVEBU_NAND_BOOT) +#define DEFAULT_BUBT_DST "nand" +#elif defined(CONFIG_MVEBU_MMC_BOOT) +#define DEFAULT_BUBT_DST "mmc" +else +#define DEFAULT_BUBT_DST "error" +#endif +#endif /* DEFAULT_BUBT_DST */

+int do_bubt_cmd(cmd_tbl_t *cmdtp, int flag, int argc,

• 	char * const argv[])
  

+{

• struct bubt_dev *src, *dst;
• size_t image_size;
• char src_dev_name[8];
• char dst_dev_name[8];
• char *name;
• if (argc < 2)

• copy_filename(net_boot_file_name, CONFIG_MVEBU_UBOOT_DFLT_NAME, sizeof(net_boot_file_name));
  

• else

• copy_filename(net_boot_file_name, argv[1], sizeof(net_boot_file_name));
  

• if (argc >= 3) {

• strncpy(dst_dev_name, argv[2], 8);
  

• } else {

• name = DEFAULT_BUBT_DST;
  

• strncpy(dst_dev_name, name, 8);
  

• }
• if (argc >= 4)

• strncpy(src_dev_name, argv[3], 8);
  

• else

• strncpy(src_dev_name, DEFAULT_BUBT_SRC, 8);
  

• /* Figure out the destination device */
• dst = find_bubt_dev(dst_dev_name);
• if (!dst) {

• printf("Error: Unknown destination \"%s\"\n", dst_dev_name);
  

• return 1;
  

• }
• if (!bubt_is_dev_active(dst))

• return 1;
  

• /* Figure out the source device */
• src = find_bubt_dev(src_dev_name);
• if (!src) {

• printf("Error: Unknown source \"%s\"\n", src_dev_name);
  

• return 1;
  

• }
• if (!bubt_is_dev_active(src))

• return 1;
  

• printf("Burning U-BOOT image "%s" from "%s" to "%s"\n", net_boot_file_name, src->name, dst->name);
• image_size = bubt_read_file(src);
• if (!image_size)

• return 1;
  

• if (bubt_verify(image_size))

• return 1;
  

• if (bubt_write_file(dst, image_size))

• return 1;
  

• return 0;

+}

+U_BOOT_CMD(

• bubt, 4, 0, do_bubt_cmd,
• "Burn a u-boot image to flash",
• "[file-name] [destination [source]]\n"
• "\t-file-name The image file name to burn. Default = u-boot.bin\n"
• "\t-destination Flash to burn to [spi, nand, mmc]. Default = active boot device\n"
• "\t-source The source to load image from [tftp, usb, mmc]. Default = tftp\n"
• "Examples:\n"
• "\tbubt - Burn flash-image.bin from tftp to active boot device\n"
• "\tbubt flash-image-new.bin nand - Burn flash-image-new.bin from tftp to NAND flash\n"
• "\tbubt backup-flash-image.bin mmc usb - Burn backup-flash-image.bin from usb to MMC\n"

Ah, here you have some nice examples. Please add at least one best with a log from running on a board to the commit text as mentioned above.

Thanks, Stefan

Hi,

On 20 November 2016 at 08:38, kostap@marvell.com wrote:

From: Konstantin Porotchkin kostap@marvell.com

Add a port of Marvell pin control driver. The A8K SoC family contains several silicone dies interconnected in a single package. Every die is normally equuipped with its own pin controller unit. Since the UCLASS_PINCTRL device only calls the probe method for the first detected pin controller, a trick similar to used with comphy driver is required. In order to bring up all pin controllers available in A8K SoC, the arch_early_init_r() function sequentially calls the uclass_get_device() function for each UCLASS_PINCTRL device.

Change-Id: Iff143827e8f1558a554d77173562c4b52ce179d7 Signed-off-by: Konstantin Porotchkin kostap@marvell.com Cc: Simon Glass sjg@chromium.org Cc: Stefan Roese sr@denx.de Cc: Nadav Haklai nadavh@marvell.com Cc: Neta Zur Hershkovits neta@marvell.com Cc: Omri Itach omrii@marvell.com Cc: Igal Liberman igall@marvell.com Cc: Haim Boot hayim@marvell.com Cc: Hanna Hawa hannah@marvell.com

---

arch/arm/include/asm/arch-armada8k/soc-info.h | 45 +++++ arch/arm/mach-mvebu/arm64-common.c | 1 - .../pinctrl/marvell,mvebu-pinctrl.txt | 113 ++++++++++++ drivers/pinctrl/Kconfig | 1 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/mvebu/Kconfig | 7 + drivers/pinctrl/mvebu/Makefile | 17 ++ drivers/pinctrl/mvebu/pinctrl-mvebu.c | 195 +++++++++++++++++++++ drivers/pinctrl/mvebu/pinctrl-mvebu.h | 44 +++++ 9 files changed, 423 insertions(+), 1 deletion(-) create mode 100644 arch/arm/include/asm/arch-armada8k/soc-info.h create mode 100644 doc/device-tree-bindings/pinctrl/marvell,mvebu-pinctrl.txt create mode 100644 drivers/pinctrl/mvebu/Kconfig create mode 100644 drivers/pinctrl/mvebu/Makefile create mode 100644 drivers/pinctrl/mvebu/pinctrl-mvebu.c create mode 100644 drivers/pinctrl/mvebu/pinctrl-mvebu.h

Generally looks good but I have a load of nits sorry.

diff --git a/arch/arm/include/asm/arch-armada8k/soc-info.h b/arch/arm/include/asm/arch-armada8k/soc-info.h new file mode 100644 index 0000000..4640deb --- /dev/null +++ b/arch/arm/include/asm/arch-armada8k/soc-info.h @@ -0,0 +1,45 @@ +/*

• ---

• • Copyright (C) 2015 Marvell International Ltd.

• ---

• • This program is free software: you can redistribute it and/or modify it
• • under the terms of the GNU General Public License as published by the Free
• • Software Foundation, either version 2 of the License, or any later version.
• • This program is distributed in the hope that it will be useful,
• • but WITHOUT ANY WARRANTY; without even the implied warranty of
• • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
• • GNU General Public License for more details.
• • You should have received a copy of the GNU General Public License
• • along with this program. If not, see http://www.gnu.org/licenses/.

• ---

Can you please use SPDX?

• */

+#ifndef _SOC_INFO_H_ +#define _SOC_INFO_H_

+/* General MPP definitions */ +#define MAX_MPP_OPTS 7 +#define MAX_MPP_ID 15

+#define MPP_BIT_CNT 4 +#define MPP_FIELD_MASK 0x7 +#define MPP_FIELD_BITS 3 +#define MPP_VAL_MASK 0xF

+#define MPPS_PER_REG (32 / MPP_BIT_CNT) +#define MAX_MPP_REGS ((MAX_MPP_ID + MPPS_PER_REG) / MPPS_PER_REG)

+/* MPP pins and functions correcsponding to UART RX connections

• This information is used for detection of recovery boot mode (boot from UART) */

/* * MPP pins * ... * /

+#define MPP_UART_RX_PINS { 3, 5 } +#define MPP_UART_RX_FUNCTIONS { 1, 2 }

+/* Pin Ctrl driver definitions */ +#define BITS_PER_PIN 4 +#define PIN_FUNC_MASK ((1 << BITS_PER_PIN) - 1) +#define PIN_REG_SHIFT 3 +#define PIN_FIELD_MASK ((1 << PIN_REG_SHIFT) - 1)

+#endif /* _SOC_INFO_H_ */ diff --git a/arch/arm/mach-mvebu/arm64-common.c b/arch/arm/mach-mvebu/arm64-common.c index 1fc2ff2..78fe7a7 100644 --- a/arch/arm/mach-mvebu/arm64-common.c +++ b/arch/arm/mach-mvebu/arm64-common.c @@ -124,7 +124,6 @@ int arch_early_init_r(void) if (ret) break; }

Unrelated change?

    /* Cause the SATA device to do its early init */
    uclass_first_device(UCLASS_AHCI, &dev);

diff --git a/doc/device-tree-bindings/pinctrl/marvell,mvebu-pinctrl.txt b/doc/device-tree-bindings/pinctrl/marvell,mvebu-pinctrl.txt new file mode 100644 index 0000000..0973db8 --- /dev/null +++ b/doc/device-tree-bindings/pinctrl/marvell,mvebu-pinctrl.txt @@ -0,0 +1,113 @@ +The pinctrl driver enables Marvell Armada 8K SoCs to configure the multi-purpose +pins (mpp) to a specific function. +A Marvell SoC pin configuration node is a node of a group of pins which can +be used for a specific device or function. Each node requires one or more +mpp pins or group of pins and a mpp function common to all pins.

+Required properties for the pinctrl driver: +- compatible: "marvell,mvebu-pinctrl",

•           "marvell,armada-ap806-pinctrl",
  

•           "marvell,a70x0-pinctrl",
  

•           "marvell,a80x0-cp0-pinctrl",
  

•           "marvell,a80x0-cp1-pinctrl"
  

+- bank-name: A string defining the pinc controller bank name +- reg: A pair of values defining the pin controller base address

•           and the address space
  

+- pin-count: Numeric value defining the amount of multi purpose pins

•           included in this bank
  

+- max-func: Numeric value defining the maximum function value for

•           pins in this bank
  

+- pin-func: Array of pin function values for every pin in the bank.

•           When the function value for a specific pin equal 0xFF,
  

•           the pin configuration is skipped and a default function
  

•           value is used for this pin.
  

+The A8K is a hybrid SoC that contains several silicon dies interconnected in +a single package. Each such die may have a separate pin controller.

+Example: +/ {

•   ap806 {
  

•           config-space {
  

•                   pinctl: pinctl@6F4000 {
  

•                           compatible = "marvell,mvebu-pinctrl",
  

•                                        "marvell,armada-ap806-pinctrl";
  

•                           bank-name ="apn-806";
  

•                           reg = <0x6F4000 0x10>;
  

•                           pin-count = <20>;
  

•                           max-func = <3>;
  

•                           /* MPP Bus:
  

•                                   SPI0 [0-3]
  

•                                   I2C0 [4-5]
  

•                                   UART0 [11,19]
  

•                           */
  

•                                     /* 0 1 2 3 4 5 6 7 8 9 */
  

•                           pin-func = < 3 3 3 3 3 3 0 0 0 0
  

•                                        0 3 0 0 0 0 0 0 0 3>;
  

•                   };
  

•           };
  

•   };
  

•   cp110-master {
  

•           config-space {
  

•                   cpm_pinctl: pinctl@44000 {
  

•                           compatible = "marvell,mvebu-pinctrl",
  

•                                        "marvell,a70x0-pinctrl",
  

•                                        "marvell,a80x0-cp0-pinctrl";
  

•                           bank-name ="cp0-110";
  

•                           reg = <0x440000 0x20>;
  

•                           pin-count = <63>;
  

•                           max-func = <0xf>;
  

•                           /* MPP Bus:
  

•                              [0-31] = 0xff: Keep default CP0_shared_pins:
  

•                              [11] CLKOUT_MPP_11 (out)
  

•                              [23] LINK_RD_IN_CP2CP (in)
  

•                              [25] CLKOUT_MPP_25 (out)
  

•                              [29] AVS_FB_IN_CP2CP (in)
  

•                              [32,34] SMI
  

•                              [31]    GPIO: push button/Wake
  

•                              [35-36] GPIO
  

•                              [37-38] I2C
  

•                              [40-41] SATA[0/1]_PRESENT_ACTIVEn
  

•                              [42-43] XSMI
  

•                              [44-55] RGMII1
  

•                              [56-62] SD
  

•                           */
  

•                                   /*   0    1    2    3    4    5    6    7    8    9 */
  

•                           pin-func = < 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
  

•                                        0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
  

•                                        0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
  

•                                        0xff 0    7    0    7    0    0    2    2    0
  

•                                        0    0    8    8    1    1    1    1    1    1
  

•                                        1    1    1    1    1    1    0xE  0xE  0xE  0xE
  

•                                        0xE  0xE  0xE>;
  

•                   };
  

•           };
  

•   };
  

•   cp110-slave {
  

•           config-space {
  

•                   cps_pinctl: pinctl@44000 {
  

•                           compatible = "marvell,mvebu-pinctrl",
  

•                                        "marvell,a80x0-cp1-pinctrl";
  

•                           bank-name ="cp1-110";
  

•                           reg = <0x440000 0x20>;
  

•                           pin-count = <63>;
  

•                           max-func = <0xf>;
  

•                           /* MPP Bus:
  

•                              [0-11]  RGMII0
  

•                              [27,31] GE_MDIO/MDC
  

•                              [32-62] = 0xff: Keep default CP1_shared_pins:
  

•                           */
  

•                                   /*   0    1    2    3    4    5    6    7    8    9 */
  

•                           pin-func = < 0x3  0x3  0x3  0x3  0x3  0x3  0x3  0x3  0x3  0x3
  

•                                        0x3  0x3  0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
  

•                                        0xff 0xff 0xff 0xff 0xff 0xff 0xff 0x8  0xff 0xff
  

•                                        0xff 0x8  0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
  

•                                        0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
  

•                                        0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
  

•                                        0xff 0xff 0xff>;
  

•                   };
  

•           };
  

•   };
  

+} diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig index 12be3cf..efcb4c0 100644 --- a/drivers/pinctrl/Kconfig +++ b/drivers/pinctrl/Kconfig @@ -181,5 +181,6 @@ source "drivers/pinctrl/meson/Kconfig" source "drivers/pinctrl/nxp/Kconfig" source "drivers/pinctrl/uniphier/Kconfig" source "drivers/pinctrl/exynos/Kconfig" +source "drivers/pinctrl/mvebu/Kconfig"

endmenu diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile index f28b5c1..512112a 100644 --- a/drivers/pinctrl/Makefile +++ b/drivers/pinctrl/Makefile @@ -15,3 +15,4 @@ obj-$(CONFIG_PINCTRL_UNIPHIER) += uniphier/ obj-$(CONFIG_PIC32_PINCTRL) += pinctrl_pic32.o obj-$(CONFIG_PINCTRL_EXYNOS) += exynos/ obj-$(CONFIG_PINCTRL_MESON) += meson/ +obj-$(CONFIG_PINCTRL_MVEBU) += mvebu/ diff --git a/drivers/pinctrl/mvebu/Kconfig b/drivers/pinctrl/mvebu/Kconfig new file mode 100644 index 0000000..cf9c299 --- /dev/null +++ b/drivers/pinctrl/mvebu/Kconfig @@ -0,0 +1,7 @@ +config PINCTRL_MVEBU

•   depends on ARCH_MVEBU
  

•   bool
  

•   default y
  

•   help
  

•      Support pin multiplexing and pin configuration control on
  

•      Marvell's Armada-8K SoC.
  

diff --git a/drivers/pinctrl/mvebu/Makefile b/drivers/pinctrl/mvebu/Makefile new file mode 100644 index 0000000..7db2b97 --- /dev/null +++ b/drivers/pinctrl/mvebu/Makefile @@ -0,0 +1,17 @@ +#* *************************************************************************** +#* Copyright (C) 2016 Marvell International Ltd. +#* *************************************************************************** +#* This program is free software: you can redistribute it and/or modify it +#* under the terms of the GNU General Public License as published by the Free +#* Software Foundation, either version 2 of the License, or any later version. +#* +#* This program is distributed in the hope that it will be useful, +#* but WITHOUT ANY WARRANTY; without even the implied warranty of +#* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +#* GNU General Public License for more details. +#* +#* You should have received a copy of the GNU General Public License +#* along with this program. If not, see http://www.gnu.org/licenses/. +#* ***************************************************************************

+obj-$(CONFIG_PINCTRL_MVEBU) += pinctrl-mvebu.o diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c b/drivers/pinctrl/mvebu/pinctrl-mvebu.c new file mode 100644 index 0000000..02fcd2f --- /dev/null +++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c @@ -0,0 +1,195 @@ +/*

• ---

• • Copyright (C) 2016 Marvell International Ltd.

• ---

• • This program is free software: you can redistribute it and/or modify it
• • under the terms of the GNU General Public License as published by the Free
• • Software Foundation, either version 2 of the License, or any later version.
• • This program is distributed in the hope that it will be useful,
• • but WITHOUT ANY WARRANTY; without even the implied warranty of
• • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
• • GNU General Public License for more details.
• • You should have received a copy of the GNU General Public License
• • along with this program. If not, see http://www.gnu.org/licenses/.

• ---

• */

+#include <config.h> +#include <common.h> +#include <dm.h> +#include <asm/system.h> +#include <asm/io.h> +#include <dm/pinctrl.h> +#include <dm/root.h> +#include <fdtdec.h> +#include <errno.h> +#include <asm/arch/fdt.h> +#include <asm/arch-armada8k/soc-info.h> +#include "pinctrl-mvebu.h"

Please check include-file ordering here:

http://www.denx.de/wiki/U-Boot/CodingStyle

It is close.

+/*

• • mvebu_pinctrl_set_state: configure pin functions.
• • dev: the pinctrl device to be configured.
• • config: the state to be configured.

/* * mvebu_pinctrl_set_state() - configure pin functions * @dev: the pinctrl device to be configured. * @config: the state to be configured. * @return ... */

Please use that consistently.

• */

+int mvebu_pinctrl_set_state(struct udevice *dev, struct udevice *config) +{

•   const void *blob = gd->fdt_blob;
  

•   int node = config->of_offset;
  

•   struct mvebu_pinctrl_priv *priv;
  

•   u32 pin_arr[CONFIG_MAX_PINS_PER_BANK];
  

•   u32 function;
  

•   int i, pin_count;
  

•   priv = dev_get_priv(dev);
  

•   if (!priv) {
  

•           printf("%s: Failed to get private\n", __func__);
  

debug()

•           return -EINVAL;
  

•   }
  

•   pin_count = fdtdec_get_int_array_count(blob, node, "marvell,pins", pin_arr, CONFIG_MAX_PINS_PER_BANK);
  

Are you sure this passes checkpatch? That line seems to long.

•   if (pin_count <= 0) {
  

•           error("Failed reading pins array for pinconfig %s (%d)\n", config->name, pin_count);
  

debug() to avoid bloating the code? (globally)

•           return -EINVAL;
  

•   }
  

•   function = fdtdec_get_int(blob, node, "marvell,function", 0xFF);
  

nit: Lower case hex throughout

•   for (i = 0; i < pin_count; i++) {
  

•           int reg_offset;
  

•           int field_offset;
  

•           u32 reg, mask;
  

•           int pin = pin_arr[i];
  

•           if (function > priv->max_func) {
  

•                   error("Illegal function %d for pinconfig %s\n", function, config->name);
  

•                   return -EINVAL;
  

•           }
  

•           /* Calculate register address and bit in register */
  

•           reg_offset   = priv->reg_direction * 4 * (pin >> (PIN_REG_SHIFT));
  

•           field_offset = (BITS_PER_PIN) * (pin & PIN_FIELD_MASK);
  

•           mask = ~(PIN_FUNC_MASK << field_offset);
  

•           /* Clip value to field resolution */
  

•           function &= PIN_FUNC_MASK;
  

•           reg = readl(priv->base_reg + reg_offset);
  

•           reg = (reg & mask) | (function << field_offset);
  

•           writel(reg, priv->base_reg + reg_offset);
  

You can use clrsetbits_le32()

•   }
  

•   return 0;
  

+}

+/*

• • mvebu_pinctrl_set_state_all: configure the entire bank pin functions.
• • dev: the pinctrl device to be configured.
• • config: the state to be configured.
• */

+static int mvebu_pinctrl_set_state_all(struct udevice *dev, struct udevice *config) +{

•   const void *blob = gd->fdt_blob;
  

•   int node = config->of_offset;
  

•   struct mvebu_pinctrl_priv *priv;
  

•   u32 func_arr[CONFIG_MAX_PINS_PER_BANK];
  

•   int pin, err;
  

•   priv = dev_get_priv(dev);
  

•   if (!priv) {
  

•           printf("%s: Failed to get private\n", __func__);
  

This cannot happen if the device is probed. Add an assert() if you are paranoid, but it has not benefit.

•           return -EINVAL;
  

•   }
  

•   err = fdtdec_get_int_array(blob, node, "pin-func", func_arr, priv->pin_cnt);
  

•   if (err) {
  

•           error("Failed reading pin functions for bank %s\n", priv->bank_name);
  

•           return -EINVAL;
  

•   }
  

•   for (pin = 0; pin < priv->pin_cnt; pin++) {
  

•           int reg_offset;
  

•           int field_offset;
  

•           u32 reg, mask;
  

•           u32 func = func_arr[pin];
  

•           /* Bypass pins with function 0xFF */
  

•           if (func == 0xFF) {
  

•                   debug("Warning: pin %d value is not modified (kept as default\n", pin);
  

•                   continue;
  

•           } else if (func > priv->max_func) {
  

•                   error("Illegal function %d for pin %d\n", func, pin);
  

•                   return -EINVAL;
  

•           }
  

•           /* Calculate register address and bit in register */
  

•           reg_offset   = priv->reg_direction * 4 * (pin >> (PIN_REG_SHIFT));
  

•           field_offset = (BITS_PER_PIN) * (pin & PIN_FIELD_MASK);
  

•           mask = ~(PIN_FUNC_MASK << field_offset);
  

•           /* Clip value to field resolution */
  

•           func &= PIN_FUNC_MASK;
  

•           reg = readl(priv->base_reg + reg_offset);
  

•           reg = (reg & mask) | (func << field_offset);
  

•           writel(reg, priv->base_reg + reg_offset);
  

clrsetbits_le32()

•   }
  

•   return 0;
  

+}

+int mvebu_pinctl_probe(struct udevice *dev) +{

•   const void *blob = gd->fdt_blob;
  

•   int node = dev->of_offset;
  

•   struct mvebu_pinctrl_priv *priv;
  

•   fdt_addr_t base;
  

•   priv = dev_get_priv(dev);
  

•   if (!priv) {
  

•           printf("%s: Failed to get private\n", __func__);
  

debug() - please fix globally - drivers should not print messages.

•           return -EINVAL;
  

•   }
  

•   base = dev_get_addr(dev);
  

•   if (base == FDT_ADDR_T_NONE) {
  

•           printf("%s: Failed to get base address\n", __func__);
  

•           return -EINVAL;
  

•   }
  

•   priv->base_reg  = (u8 *)base;
  

Or use dev_get_addr_ptr() ?

•   priv->pin_cnt   = fdtdec_get_int(blob, node, "pin-count", CONFIG_MAX_PINS_PER_BANK);
  

•   priv->max_func  = fdtdec_get_int(blob, node, "max-func", CONFIG_MAX_FUNC);
  

•   priv->bank_name = fdt_getprop(blob, node, "bank-name", NULL);
  

•   priv->reg_direction = 1;
  

•   if (fdtdec_get_bool(blob, node, "reverse-reg"))
  

•           priv->reg_direction = -1;
  

•   return mvebu_pinctrl_set_state_all(dev, dev);
  

+}

+static struct pinctrl_ops mvebu_pinctrl_ops = {

•   .set_state      = mvebu_pinctrl_set_state
  

+};

+static const struct udevice_id mvebu_pinctrl_ids[] = {

•   { .compatible = "marvell,mvebu-pinctrl" },
  

•   { .compatible = "marvell,armada-ap806-pinctrl" },
  

•   { .compatible = "marvell,a70x0-pinctrl" },
  

•   { .compatible = "marvell,a80x0-cp0-pinctrl" },
  

•   { .compatible = "marvell,a80x0-cp1-pinctrl" },
  

•   { }
  

+};

+U_BOOT_DRIVER(pinctrl_mvebu) = {

•   .name           = "mvebu_pinctrl",
  

•   .id             = UCLASS_PINCTRL,
  

•   .of_match       = mvebu_pinctrl_ids,
  

•   .priv_auto_alloc_size = sizeof(struct mvebu_pinctrl_priv),
  

•   .ops            = &mvebu_pinctrl_ops,
  

•   .probe          = mvebu_pinctl_probe
  

+}; diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.h b/drivers/pinctrl/mvebu/pinctrl-mvebu.h new file mode 100644 index 0000000..61c84ce --- /dev/null +++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.h @@ -0,0 +1,44 @@ +/*

• ---

• • Copyright (C) 2016 Marvell International Ltd.

• ---

• • This program is free software: you can redistribute it and/or modify it
• • under the terms of the GNU General Public License as published by the Free
• • Software Foundation, either version 2 of the License, or any later version.
• • This program is distributed in the hope that it will be useful,
• • but WITHOUT ANY WARRANTY; without even the implied warranty of
• • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
• • GNU General Public License for more details.
• • You should have received a copy of the GNU General Public License
• • along with this program. If not, see http://www.gnu.org/licenses/.

• ---

• */
• #ifndef __PINCTRL_MVEBU_H_
• #define __PINCTRL_MVEBU_H_
• #define CONFIG_MAX_PINCTL_BANKS 4
• #define CONFIG_MAX_PINS_PER_BANK 100
• #define CONFIG_MAX_FUNC 0xF

Avoid using CONFIG_ prefixes since this looks like a new global CONFIG option. Just dropping the CONFIG_ will do.

+DECLARE_GLOBAL_DATA_PTR;

Please put that in the C file that needs it.

+/*

• • struct mvebu_pin_bank_data: mvebu-pinctrl bank data

* struct mvebu_pin_bank_data - mvebu-pinctrl bank data

• • @base_reg: controller base address for this bank
• • @pin_cnt: number of ping included in this bank

ping?

• • @max_func: maximum configurable function value for pins in this bank
• • @reg_direction:

?

• • @bank_name: the pins bank name

pin's

• */

+struct mvebu_pinctrl_priv {

•   u8              *base_reg;
  

•   u32             pin_cnt;
  

•   u32             max_func;
  

Why are these u32? Can they be uint?

•   int             reg_direction;
  

•   const char      *bank_name;
  

+};

+#endif /* __PINCTRL_MVEBU_H_ */

2.7.4

Regards, Simon

Hi, Stefan,

Thank you for your review! I will put all required changes into second patch version.

Regarding the symbolic names for the pin controller functions and lack of documentation. The problem is that same function number does not have the same meaning for different pins. So if I want to put symbolic names instead of numbers, I have to add large structures defining symbolic names for each function on every pin as a platform data. I think in this case I will loose the driver code compactness provided by the FDT usage. I can create a documentation file with all pin function values taken from SoC HW manual and keep the numeric function IDs for the DTS usage. Will it be good enough?

Best Regards Konstantin ________________________________________ From: Stefan Roese sr@denx.de Sent: Thursday, November 24, 2016 11:13 To: Kostya Porotchkin; u-boot@lists.denx.de Cc: Nadav Haklai; Neta Zur Hershkovits; Omri Itach; Igal Liberman; Haim Boot; Hanna Hawa Subject: Re: [PATCH 4/6] arm64: mvebu: Add pin control nodes to A8K family DTS files

Hi Kosta,

On 20.11.2016 16:38, kostap@marvell.com wrote:

From: Konstantin Porotchkin kostap@marvell.com

Add pin control nodes to APN806, CP-master, CP-slave and Armada-7040 and Armada-8040 boards DTS files

Change-Id: I51522f33f23e3f9e94c6f5a5f9af19f5dc86e6b7 Signed-off-by: Konstantin Porotchkin kostap@marvell.com Cc: Stefan Roese sr@denx.de Cc: Nadav Haklai nadavh@marvell.com Cc: Neta Zur Hershkovits neta@marvell.com Cc: Omri Itach omrii@marvell.com Cc: Igal Liberman igall@marvell.com Cc: Haim Boot hayim@marvell.com Cc: Hanna Hawa hannah@marvell.com

---

arch/arm/dts/armada-7040-db.dts | 38 +++++++++++++++++++++++ arch/arm/dts/armada-8040-db.dts | 57 +++++++++++++++++++++++++++++++++++ arch/arm/dts/armada-ap806.dtsi | 18 +++++++++++ arch/arm/dts/armada-cp110-master.dtsi | 32 ++++++++++++++++++++ arch/arm/dts/armada-cp110-slave.dtsi | 19 ++++++++++++ 5 files changed, 164 insertions(+)

diff --git a/arch/arm/dts/armada-7040-db.dts b/arch/arm/dts/armada-7040-db.dts index b8fe5a9..1bfb5a9 100644 --- a/arch/arm/dts/armada-7040-db.dts +++ b/arch/arm/dts/armada-7040-db.dts @@ -67,6 +67,8 @@ };

&i2c0 {

• pinctrl-names = "default";
  

• pinctrl-0 = <&cpm_i2c0_pins>;
  status = "okay";
  clock-frequency = <100000>;
  

}; @@ -98,6 +100,17 @@ }; };

+&ap_pinctl {

•    /* MPP Bus:
  

•       SDIO  [0-10]
  

•       UART0 [11,19]
  

•     */
  

Please use the common multiline comment instead:

/* * MPP Bus: * SDIO [0-10] * UART0 [11,19] */

•           /* 0 1 2 3 4 5 6 7 8 9 */
  

• pin-func = < 1 1 1 1 1 1 1 1 1 1
  

•              1 3 0 0 0 0 0 0 0 3>;
  

Is there any chance to not use those numeric values but some macros instead to make it clearer, which function is selected?

• status = "okay";
  

+};

&uart0 { status = "okay"; }; @@ -112,8 +125,33 @@ clock-frequency = <100000>; };

+&cpm_pinctl {

•         /* MPP Bus:
  

•            TDM   [0-11]
  

•            SPI   [13-16]
  

•            SATA1 [28]
  

•            UART0 [29-30]
  

•            SMI   [32,34]
  

•            XSMI  [35-36]
  

•            I2C   [37-38]
  

•            RGMII1[44-55]
  

•            SD    [56-62]
  

•         */
  

Again, comment style please.

•         /*   0   1   2   3   4   5   6   7   8   9 */
  

• pin-func = < 4   4   4   4   4   4   4   4   4   4
  

•              4   4   0   3   3   3   3   0   0   0
  

•              0   0   0   0   0   0   0   0   9   0xA
  

•              0xA 0   7   0   7   7   7   2   2   0
  

•              0   0   0   0   1   1   1   1   1   1
  

•              1   1   1   1   1   1   0xE 0xE 0xE 0xE
  

•              0xE 0xE 0xE>;
  

Lower case hex values please (globally).

• status = "okay";
  

+};

&cpm_spi1 { status = "okay";

• pinctrl-names = "default";
  

• pinctrl-0 = <&cpm_spi0_pins>;
  
  spi-flash@0 {
          #address-cells = <0x1>;
  

diff --git a/arch/arm/dts/armada-8040-db.dts b/arch/arm/dts/armada-8040-db.dts index 86666a1..30fd364 100644 --- a/arch/arm/dts/armada-8040-db.dts +++ b/arch/arm/dts/armada-8040-db.dts @@ -71,6 +71,42 @@ status = "okay"; };

+&ap_pinctl {

• /* MPP Bus:
  

•         SPI0 [0-3]
  

•         I2C0 [4-5]
  

•         UART0 [11,19]
  

• */
  

•           /* 0 1 2 3 4 5 6 7 8 9 */
  

• pin-func = < 3 3 3 3 3 3 0 0 0 0
  

•              0 3 0 0 0 0 0 0 0 3>;
  

+};

+&cpm_pinctl {

• /* MPP Bus:
  

•    [0-31] = 0xff: Keep default CP0_shared_pins:
  

•    [11] CLKOUT_MPP_11 (out)
  

•    [23] LINK_RD_IN_CP2CP (in)
  

•    [25] CLKOUT_MPP_25 (out)
  

•    [29] AVS_FB_IN_CP2CP (in)
  

•    [32,34] SMI
  

•    [31]    GPIO: push button/Wake
  

•    [35-36] GPIO
  

•    [37-38] I2C
  

•    [40-41] SATA[0/1]_PRESENT_ACTIVEn
  

•    [42-43] XSMI
  

•    [44-55] RGMII1
  

•    [56-62] SD
  

• */
  

•         /*   0    1    2    3    4    5    6    7    8    9 */
  

• pin-func = < 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
  

•              0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
  

•              0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
  

•              0xff 0    7    0    7    0    0    2    2    0
  

•              0    0    8    8    1    1    1    1    1    1
  

•              1    1    1    1    1    1    0xE  0xE  0xE  0xE
  

•              0xE  0xE  0xE>;
  

+};

/* CON5 on CP0 expansion */ &cpm_pcie2 { @@ -78,6 +114,8 @@ };

&cpm_i2c0 {

• pinctrl-names = "default";
  

• pinctrl-0 = <&cpm_i2c0_pins>;
  status = "okay";
  clock-frequency = <100000>;
  

}; @@ -97,12 +135,31 @@ status = "okay"; };

+&cps_pinctl {

• /* MPP Bus:
  

•    [0-11]  RGMII0
  

•    [13-16] SPI1
  

•    [27,31] GE_MDIO/MDC
  

•    [32-62] = 0xff: Keep default CP1_shared_pins:
  

• */
  

•         /*   0    1    2    3    4    5    6    7    8    9 */
  

• pin-func = < 0x3  0x3  0x3  0x3  0x3  0x3  0x3  0x3  0x3  0x3
  

•              0x3  0x3  0xff 0x3  0x3  0x3  0x3  0xff 0xff 0xff
  

•              0xff 0xff 0xff 0xff 0xff 0xff 0xff 0x8  0xff 0xff
  

•              0xff 0x8  0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
  

•              0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
  

•              0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff 0xff
  

•              0xff 0xff 0xff>;
  

+};

/* CON5 on CP1 expansion */ &cps_pcie2 { status = "okay"; };

&cps_spi1 {

• pinctrl-names = "default";
  

• pinctrl-0 = <&cps_spi1_pins>;
  status = "okay";
  
  spi-flash@0 {
  

diff --git a/arch/arm/dts/armada-ap806.dtsi b/arch/arm/dts/armada-ap806.dtsi index d315b29..efb383b 100644 --- a/arch/arm/dts/armada-ap806.dtsi +++ b/arch/arm/dts/armada-ap806.dtsi @@ -140,6 +140,24 @@ marvell,spi-base = <128>, <136>, <144>, <152>; };

•                 ap_pinctl: ap-pinctl@6F4000 {
  

•                         compatible = "marvell,armada-ap806-pinctrl";
  

•                         bank-name ="apn-806";
  

•                         reg = <0x6F4000 0x10>;
  

•                         pin-count = <20>;
  

•                         max-func = <3>;
  

•                         ap_i2c0_pins: i2c-pins-0 {
  

•                                 marvell,pins = < 4 5 >;
  

•                                 marvell,function = <3>;
  

So what are these marvell,pins/functions? They are not listed in the DT bindings documentation.

•                         };
  

•                         ap_emmc_pins: emmc-pins-0 {
  

•                                 marvell,pins = < 0 1 2 3 4 5 6 7
  

•                                                  8 9 10 >;
  

•                                 marvell,function = <1>;
  

•                         };
  

•                 };
  

•                 xor@400000 {
                          compatible = "marvell,mv-xor-v2";
                          reg = <0x400000 0x1000>,
  

diff --git a/arch/arm/dts/armada-cp110-master.dtsi b/arch/arm/dts/armada-cp110-master.dtsi index 422d754..d637867 100644 --- a/arch/arm/dts/armada-cp110-master.dtsi +++ b/arch/arm/dts/armada-cp110-master.dtsi @@ -81,6 +81,38 @@ "cpm-usb3dev", "cpm-eip150", "cpm-eip197"; };

•                 cpm_pinctl: cpm-pinctl@440000 {
  

•                         compatible = "marvell,mvebu-pinctrl",
  

•                                      "marvell,a70x0-pinctrl",
  

•                                      "marvell,a80x0-cp0-pinctrl";
  

•                         bank-name ="cp0-110";
  

•                         reg = <0x440000 0x20>;
  

•                         pin-count = <63>;
  

•                         max-func = <0xf>;
  

•                         cpm_i2c0_pins: cpm-i2c-pins-0 {
  

•                                 marvell,pins = < 37 38 >;
  

•                                 marvell,function = <2>;
  

•                         };
  

•                         cpm_ge2_rgmii_pins: cpm-ge-rgmii-pins-0 {
  

•                                 marvell,pins = < 44 45 46 47 48 49 50 51
  

•                                                  52 53 54 55 >;
  

•                                 marvell,function = <1>;
  

•                         };
  

•                         pca0_pins: cpm-pca0_pins {
  

•                                 marvell,pins = <62>;
  

•                                 marvell,function = <0>;
  

•                         };
  

•                         cpm_sdhci_pins: cpm-sdhi-pins-0 {
  

•                                 marvell,pins = < 56 57 58 59 60 61 >;
  

•                                 marvell,function = <14>;
  

•                         };
  

•                         cpm_spi0_pins: cpm-spi-pins-0 {
  

•                                 marvell,pins = < 13 14 15 16 >;
  

•                                 marvell,function = <3>;
  

•                         };
  

•                 };
  

•                 cpm_sata0: sata@540000 {
                          compatible = "marvell,armada-8k-ahci";
                          reg = <0x540000 0x30000>;
  

diff --git a/arch/arm/dts/armada-cp110-slave.dtsi b/arch/arm/dts/armada-cp110-slave.dtsi index a7f77b9..92ef55c 100644 --- a/arch/arm/dts/armada-cp110-slave.dtsi +++ b/arch/arm/dts/armada-cp110-slave.dtsi @@ -81,6 +81,25 @@ "cps-usb3dev", "cps-eip150", "cps-eip197"; };

•                 cps_pinctl: cps-pinctl@440000 {
  

•                         compatible = "marvell,mvebu-pinctrl",
  

•                                      "marvell,a80x0-cp1-pinctrl";
  

•                         bank-name ="cp1-110";
  

•                         reg = <0x440000 0x20>;
  

•                         pin-count = <63>;
  

•                         max-func = <0xf>;
  

•                         cps_ge1_rgmii_pins: cps-ge-rgmii-pins-0 {
  

•                                 marvell,pins = < 0  1  2  3  4  5  6  7
  

•                                                  8  9  10 11 >;
  

•                                 marvell,function = <3>;
  

•                         };
  

•                         cps_spi1_pins: cps-spi-pins-1 {
  

•                                 marvell,pins = < 13 14 15 16 >;
  

•                                 marvell,function = <3>;
  

•                         };
  

•                 };
  

•                 cps_sata0: sata@540000 {
                          compatible = "marvell,armada-8k-ahci";
                          reg = <0x540000 0x30000>;
  
  

Thanks, Stefan