Re: [U-Boot] [PATCH] ARM: socfpga: Convert to DM serial
On 30.07.2018 16:04, Marek Vasut wrote: On 07/30/2018 04:03 PM, Simon Goldschmidt wrote: On 12.05.2018 22:28, Marek Vasut wrote: Pull the serial port configuration from DT and use DM serial instead of having the serial configuration in two places, DT and board config. Signed-off-by: Marek Vasut Cc: Chin Liang See Cc: Dinh Nguyen --- arch/arm/Kconfig | 3 +++ arch/arm/dts/socfpga.dtsi | 2 ++ arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts | 1 + include/configs/socfpga_common.h | 8 4 files changed, 6 insertions(+), 8 deletions(-) diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 532aa41a87..2012ac6410 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -737,6 +737,7 @@ config ARCH_SOCFPGA select ARCH_MISC_INIT select CPU_V7A select DM + select DM_SERIAL select ENABLE_ARM_SOC_BOOT0_HOOK select OF_CONTROL select SPL_LIBCOMMON_SUPPORT @@ -746,11 +747,13 @@ config ARCH_SOCFPGA select SPL_NAND_SUPPORT if SPL_NAND_DENALI select SPL_OF_CONTROL select SPL_SERIAL_SUPPORT + select SPL_DM_SERIAL select SPL_SPI_FLASH_SUPPORT if SPL_SPI_SUPPORT select SPL_SPI_SUPPORT if DM_SPI select SPL_WATCHDOG_SUPPORT select SUPPORT_SPL select SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION_TYPE + select SYS_NS16550 select SYS_THUMB_BUILD imply CMD_MTDPARTS imply CRC32_VERIFY diff --git a/arch/arm/dts/socfpga.dtsi b/arch/arm/dts/socfpga.dtsi index e64127fcb2..314449478d 100644 --- a/arch/arm/dts/socfpga.dtsi +++ b/arch/arm/dts/socfpga.dtsi @@ -737,6 +737,7 @@ reg-shift = <2>; reg-io-width = <4>; clocks = <&l4_sp_clk>; + clock-frequency = <1>; }; uart1: serial1@ffc03000 { @@ -746,6 +747,7 @@ reg-shift = <2>; reg-io-width = <4>; clocks = <&l4_sp_clk>; + clock-frequency = <1>; }; rst: rstmgr@ffd05000 { diff --git a/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts b/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts index b573d0e658..06b61cb0af 100644 --- a/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts +++ b/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts @@ -24,6 +24,7 @@ }; &uart1 { + clock-frequency = <5000>; u-boot,dm-pre-reloc; status = "okay"; }; diff --git a/include/configs/socfpga_common.h b/include/configs/socfpga_common.h index 54b9edc97c..a60da85499 100644 --- a/include/configs/socfpga_common.h +++ b/include/configs/socfpga_common.h @@ -173,14 +173,6 @@ unsigned int cm_get_qspi_controller_clk_hz(void); * Serial Driver */ #define CONFIG_SYS_NS16550_SERIAL -#define CONFIG_SYS_NS16550_REG_SIZE -4 -#if defined(CONFIG_TARGET_SOCFPGA_GEN5) -#define CONFIG_SYS_NS16550_COM1 SOCFPGA_UART0_ADDRESS -#define CONFIG_SYS_NS16550_CLK 1 -#elif defined(CONFIG_TARGET_SOCFPGA_ARRIA10) -#define CONFIG_SYS_NS16550_COM1 SOCFPGA_UART1_ADDRESS -#define CONFIG_SYS_NS16550_CLK 5000 -#endif /* * USB Unfortunately I saw this just now, but it seems this breaks GEN5 SPL? At least git-bisect told me that 73172753f4f3351ed1c9d2f6586fc599ce4e728c is the first bad commit. I tested socfpga_socrates_defconfig on my socrates board. Any idea what's wrong there? Nope, this should work fine. Can you investigate ? Ok, so after adding "u-boot,dm-pre-reloc" to uart0 in socfpga_cyclone5_socrates.dts, U-Boot works (combined with an old SPL). SPL still does not work. Any idea? How does SPL get the uart? Thanks for any pointers. Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] ARM: socfpga: Convert to DM serial
On 30.07.2018 16:04, Marek Vasut wrote: On 07/30/2018 04:03 PM, Simon Goldschmidt wrote: On 12.05.2018 22:28, Marek Vasut wrote: Pull the serial port configuration from DT and use DM serial instead of having the serial configuration in two places, DT and board config. Signed-off-by: Marek Vasut Cc: Chin Liang See Cc: Dinh Nguyen --- arch/arm/Kconfig | 3 +++ arch/arm/dts/socfpga.dtsi | 2 ++ arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts | 1 + include/configs/socfpga_common.h | 8 4 files changed, 6 insertions(+), 8 deletions(-) diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 532aa41a87..2012ac6410 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -737,6 +737,7 @@ config ARCH_SOCFPGA select ARCH_MISC_INIT select CPU_V7A select DM + select DM_SERIAL select ENABLE_ARM_SOC_BOOT0_HOOK select OF_CONTROL select SPL_LIBCOMMON_SUPPORT @@ -746,11 +747,13 @@ config ARCH_SOCFPGA select SPL_NAND_SUPPORT if SPL_NAND_DENALI select SPL_OF_CONTROL select SPL_SERIAL_SUPPORT + select SPL_DM_SERIAL select SPL_SPI_FLASH_SUPPORT if SPL_SPI_SUPPORT select SPL_SPI_SUPPORT if DM_SPI select SPL_WATCHDOG_SUPPORT select SUPPORT_SPL select SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION_TYPE + select SYS_NS16550 select SYS_THUMB_BUILD imply CMD_MTDPARTS imply CRC32_VERIFY diff --git a/arch/arm/dts/socfpga.dtsi b/arch/arm/dts/socfpga.dtsi index e64127fcb2..314449478d 100644 --- a/arch/arm/dts/socfpga.dtsi +++ b/arch/arm/dts/socfpga.dtsi @@ -737,6 +737,7 @@ reg-shift = <2>; reg-io-width = <4>; clocks = <&l4_sp_clk>; + clock-frequency = <1>; }; uart1: serial1@ffc03000 { @@ -746,6 +747,7 @@ reg-shift = <2>; reg-io-width = <4>; clocks = <&l4_sp_clk>; + clock-frequency = <1>; }; rst: rstmgr@ffd05000 { diff --git a/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts b/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts index b573d0e658..06b61cb0af 100644 --- a/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts +++ b/arch/arm/dts/socfpga_arria10_socdk_sdmmc.dts @@ -24,6 +24,7 @@ }; &uart1 { + clock-frequency = <5000>; u-boot,dm-pre-reloc; status = "okay"; }; diff --git a/include/configs/socfpga_common.h b/include/configs/socfpga_common.h index 54b9edc97c..a60da85499 100644 --- a/include/configs/socfpga_common.h +++ b/include/configs/socfpga_common.h @@ -173,14 +173,6 @@ unsigned int cm_get_qspi_controller_clk_hz(void); * Serial Driver */ #define CONFIG_SYS_NS16550_SERIAL -#define CONFIG_SYS_NS16550_REG_SIZE -4 -#if defined(CONFIG_TARGET_SOCFPGA_GEN5) -#define CONFIG_SYS_NS16550_COM1 SOCFPGA_UART0_ADDRESS -#define CONFIG_SYS_NS16550_CLK 1 -#elif defined(CONFIG_TARGET_SOCFPGA_ARRIA10) -#define CONFIG_SYS_NS16550_COM1 SOCFPGA_UART1_ADDRESS -#define CONFIG_SYS_NS16550_CLK 5000 -#endif /* * USB Unfortunately I saw this just now, but it seems this breaks GEN5 SPL? At least git-bisect told me that 73172753f4f3351ed1c9d2f6586fc599ce4e728c is the first bad commit. I tested socfpga_socrates_defconfig on my socrates board. Any idea what's wrong there? Nope, this should work fine. Can you investigate ? OK, I'll try. Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v5] u-boot: remove driver lookup loop from env_save()
+ Tom: I don't know via which tree this would go in. I think you took the last env changes directly? v1..v4 are detached threads, I can send you links if you need them. Thanks, Simon On 26.07.2018 16:02, Maxime Ripard wrote: On Thu, Jul 26, 2018 at 07:16:36AM +0200, Simon Goldschmidt wrote: Simon, Maxime, any input on this? Via which tree would this be pushed? Should we CC Tom? It would be nice if 2018.09 had this bug fixed (endless loop in env_save() on error). This looks good to me but Tom will probably end up this patch, so you definitely want to cc him. Maxime ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH] arm: socfpga: correctly reserve SRAM for boot counter
Bootcounter for is1 and sr1500 boards somewhat relied on struct global data alignment gap at the end of internal SRAM. Let's fix this by explicitly reserving some bytes. Signed-off-by: Simon Goldschmidt --- include/configs/socfpga_common.h | 6 +- include/configs/socfpga_is1.h| 3 ++- include/configs/socfpga_sr1500.h | 3 ++- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/include/configs/socfpga_common.h b/include/configs/socfpga_common.h index 4de2aa7929..1934aea86d 100644 --- a/include/configs/socfpga_common.h +++ b/include/configs/socfpga_common.h @@ -35,8 +35,12 @@ #define CONFIG_SYS_INIT_RAM_ADDR 0xFFE0 #define CONFIG_SYS_INIT_RAM_SIZE 0x4 /* 256KB */ #endif +/* Reserve bytes at the end of SRAM? */ +#ifndef SOCFPGA_INIT_RAM_END_RESERVE +#define SOCFPGA_INIT_RAM_END_RESERVE 0 +#endif #define CONFIG_SYS_INIT_SP_OFFSET \ - (CONFIG_SYS_INIT_RAM_SIZE - GENERATED_GBL_DATA_SIZE) + (CONFIG_SYS_INIT_RAM_SIZE - SOCFPGA_INIT_RAM_END_RESERVE) #define CONFIG_SYS_INIT_SP_ADDR\ (CONFIG_SYS_INIT_RAM_ADDR + CONFIG_SYS_INIT_SP_OFFSET) diff --git a/include/configs/socfpga_is1.h b/include/configs/socfpga_is1.h index c233c208a5..b243fd29cd 100644 --- a/include/configs/socfpga_is1.h +++ b/include/configs/socfpga_is1.h @@ -27,8 +27,9 @@ #include /* - * Bootcounter + * Bootcounter (preserve the last 2 lwords for the boot-counter) */ +#define CONFIG_SYS_INIT_RAM_END_RESERVE8 #define CONFIG_SYS_BOOTCOUNT_BE #endif /* __CONFIG_SOCFPGA_IS1_H__ */ diff --git a/include/configs/socfpga_sr1500.h b/include/configs/socfpga_sr1500.h index c835d23235..ff71712566 100644 --- a/include/configs/socfpga_sr1500.h +++ b/include/configs/socfpga_sr1500.h @@ -26,8 +26,9 @@ #define CONFIG_SPI_N25Q256A_RESET /* - * Bootcounter + * Bootcounter (preserve the last 2 lwords for the boot-counter) */ +#define SOCFPGA_INIT_RAM_END_RESERVE 8 #define CONFIG_SYS_BOOTCOUNT_BE /* Environment setting for SPI flash */ -- 2.17.0.windows.1 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 3/4] Convert socfpga: select CONFIG_HW_WATCHDOG support for ARCH_SOCFPGA
On 09.02.2018 23:14, Lukasz Majewski wrote: All Socfpga boards from ./include/configs/socfpga_* define CONFIG_HW_WATCHDOG. To ease CONFIG_HW_WATCHDOG conversion to Kconfig select it in config ARCH_SOCFPGA (arch/arm/Kconfig) section. I do have board configs where the internal watchdog is not used and should be disabled (because there's an external one). Also, given that this is an FPGA, I suppose having non-upstreamed boards is not uncommon. I'm not too familiar with these settings though: can I leave the watchdog disabled when CONFIG_HW_WATCHDOG is off? Before, I just haven't enabled this in my own board config... Simon Signed-off-by: Lukasz Majewski --- Changes in v2: - None arch/arm/Kconfig | 1 + include/configs/socfpga_arria10_socdk.h | 2 -- include/configs/socfpga_arria5_socdk.h | 2 -- include/configs/socfpga_cyclone5_socdk.h | 2 -- include/configs/socfpga_de0_nano_soc.h | 2 -- include/configs/socfpga_de10_nano.h | 2 -- include/configs/socfpga_de1_soc.h| 2 -- include/configs/socfpga_is1.h| 2 -- include/configs/socfpga_mcvevk.h | 2 -- include/configs/socfpga_sockit.h | 2 -- include/configs/socfpga_socrates.h | 2 -- include/configs/socfpga_sr1500.h | 2 -- include/configs/socfpga_vining_fpga.h| 2 -- 13 files changed, 1 insertion(+), 24 deletions(-) diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 225f57e847..b4c79d6499 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -702,6 +702,7 @@ config ARCH_SOCFPGA select DM_SPI_FLASH select DM_SPI select ENABLE_ARM_SOC_BOOT0_HOOK + select HW_WATCHDOG select ARCH_EARLY_INIT_R select ARCH_MISC_INIT select SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION diff --git a/include/configs/socfpga_arria10_socdk.h b/include/configs/socfpga_arria10_socdk.h index 83718dd2c9..82bb48b277 100644 --- a/include/configs/socfpga_arria10_socdk.h +++ b/include/configs/socfpga_arria10_socdk.h @@ -9,8 +9,6 @@ #include -#define CONFIG_HW_WATCHDOG - /* Booting Linux */ #define CONFIG_LOADADDR 0x0100 #define CONFIG_SYS_LOAD_ADDR CONFIG_LOADADDR diff --git a/include/configs/socfpga_arria5_socdk.h b/include/configs/socfpga_arria5_socdk.h index 6b6d54b97b..cd5aac65e9 100644 --- a/include/configs/socfpga_arria5_socdk.h +++ b/include/configs/socfpga_arria5_socdk.h @@ -8,8 +8,6 @@ #include -#define CONFIG_HW_WATCHDOG - /* Memory configurations */ #define PHYS_SDRAM_1_SIZE 0x4000 /* 1GiB on SoCDK */ diff --git a/include/configs/socfpga_cyclone5_socdk.h b/include/configs/socfpga_cyclone5_socdk.h index 018a0c3bb4..9c5bd648e3 100644 --- a/include/configs/socfpga_cyclone5_socdk.h +++ b/include/configs/socfpga_cyclone5_socdk.h @@ -8,8 +8,6 @@ #include -#define CONFIG_HW_WATCHDOG - /* Memory configurations */ #define PHYS_SDRAM_1_SIZE 0x4000 /* 1GiB on SoCDK */ diff --git a/include/configs/socfpga_de0_nano_soc.h b/include/configs/socfpga_de0_nano_soc.h index 275ed7ffeb..e5db00e366 100644 --- a/include/configs/socfpga_de0_nano_soc.h +++ b/include/configs/socfpga_de0_nano_soc.h @@ -8,8 +8,6 @@ #include -#define CONFIG_HW_WATCHDOG - /* Memory configurations */ #define PHYS_SDRAM_1_SIZE 0x4000 /* 1GiB */ diff --git a/include/configs/socfpga_de10_nano.h b/include/configs/socfpga_de10_nano.h index bb50fcf1ff..656af1104d 100644 --- a/include/configs/socfpga_de10_nano.h +++ b/include/configs/socfpga_de10_nano.h @@ -8,8 +8,6 @@ #include -#define CONFIG_HW_WATCHDOG - /* Memory configurations */ #define PHYS_SDRAM_1_SIZE 0x4000 /* 1GiB */ diff --git a/include/configs/socfpga_de1_soc.h b/include/configs/socfpga_de1_soc.h index 05975c9bde..f57b950425 100644 --- a/include/configs/socfpga_de1_soc.h +++ b/include/configs/socfpga_de1_soc.h @@ -8,8 +8,6 @@ #include -#define CONFIG_HW_WATCHDOG - /* Memory configurations */ #define PHYS_SDRAM_1_SIZE 0x4000 /* 1GiB */ diff --git a/include/configs/socfpga_is1.h b/include/configs/socfpga_is1.h index 46f5f135dd..dc318e50dc 100644 --- a/include/configs/socfpga_is1.h +++ b/include/configs/socfpga_is1.h @@ -9,8 +9,6 @@ #include -#define CONFIG_HW_WATCHDOG - /* Memory configurations */ #define PHYS_SDRAM_1_SIZE 0x1000 diff --git a/include/configs/socfpga_mcvevk.h b/include/configs/socfpga_mcvevk.h index 404f064e94..f13463b8b0 100644 --- a/include/configs/socfpga_mcvevk.h +++ b/include/configs/socfpga_mcvevk.h @@ -8,8 +8,6 @@ #include -#define CONFIG_HW_WATCHDOG - /* Memory configurations */ #define PHYS_SDRAM_1_SIZE 0x4000 /* 1GiB on MCV */ diff --git a/include/configs/socfpga_sockit.h b/include/configs/socfpga_sockit.h index b4f31c42c5..0bbc7e0105 100644 --- a/include/configs/socfpga_sockit.h
Re: [U-Boot] [PATCH 2/2] env: Add back default action of get_char in env_get_char()
Maxime, York, I've just sent a patch that removes 'get_char' callback from the env drivers. This should restore the old behaviour we had before supporting multiple environment drivers (for all but eeprom, of course). Simon On 08.02.2018 23:10, Maxime Ripard wrote: On Thu, Feb 08, 2018 at 10:52:20AM +0100, Simon Goldschmidt wrote: On 08.02.2018 09:47, Maxime Ripard wrote: On Wed, Feb 07, 2018 at 02:17:12PM -0800, York Sun wrote: Commit 8a3a7e2270b3 ("env: Pass additional parameters to the env lookup function") dropped the default action if driver doesn't have get_char() defined. This causes failure to get environmental variables from NOR flash. Add back this default action for now. Signed-off-by: York Sun CC: Maxime Ripard --- Limited test on LS1043ARDB. env/env.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/env/env.c b/env/env.c index edfb575..210bae2 100644 --- a/env/env.c +++ b/env/env.c @@ -159,7 +159,7 @@ int env_get_char(int index) int ret; if (!drv->get_char) - continue; + return *(uchar *)(gd->env_addr + index); Thinking more about this, I think this would break the case where the first environment in your list has no get_char method, but the second might. How can we decide which way is wanted? With your patch below, we might end up loading chars from a low-prio environment (which is not CRC checked) in the early boot stage. Later, we load the environment from another env driver with higher priority. Ah, right. That's why I suggested removing the 'get_char' callback from the env drivers :-) Early boot stage environment lookups would still work the old way reading from 'gd->env_addr + index'. If that works on York's board, I'm all in. Maxime ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH] env: restore old env_get_char() behaviour
With multiple environments, the 'get_char' callback for env drivers does not really make sense any more because it is only supported by two drivers (eeprom and nvram). To restore single character loading for these drivers, override 'env_get_char_spec'. Signed-off-by: Simon Goldschmidt --- env/eeprom.c | 6 -- env/env.c | 29 +++-- env/nvram.c | 8 include/environment.h | 11 --- 4 files changed, 15 insertions(+), 39 deletions(-) diff --git a/env/eeprom.c b/env/eeprom.c index 55d19d9d99..63842d6ff3 100644 --- a/env/eeprom.c +++ b/env/eeprom.c @@ -61,7 +61,10 @@ static int eeprom_bus_write(unsigned dev_addr, unsigned offset, return rcode; } -static int env_eeprom_get_char(int index) +/** Call this function from overridden env_get_char_spec() if you need + * this functionality. + */ +int env_eeprom_get_char(int index) { uchar c; unsigned int off = CONFIG_ENV_OFFSET; @@ -228,7 +231,6 @@ static int env_eeprom_save(void) U_BOOT_ENV_LOCATION(eeprom) = { .location = ENVL_EEPROM, ENV_NAME("EEPROM") - .get_char = env_eeprom_get_char, .load = env_eeprom_load, .save = env_save_ptr(env_eeprom_save), }; diff --git a/env/env.c b/env/env.c index 9a89832c1a..ab12606207 100644 --- a/env/env.c +++ b/env/env.c @@ -149,32 +149,17 @@ static struct env_driver *env_driver_lookup(enum env_operation op, int prio) return drv; } -int env_get_char(int index) +__weak int env_get_char_spec(int index) { - struct env_driver *drv; - int prio; + return *(uchar *)(gd->env_addr + index); +} +int env_get_char(int index) +{ if (gd->env_valid == ENV_INVALID) return default_environment[index]; - - for (prio = 0; (drv = env_driver_lookup(ENVOP_GET_CHAR, prio)); prio++) { - int ret; - - if (!drv->get_char) - continue; - - if (!env_has_inited(drv->location)) - continue; - - ret = drv->get_char(index); - if (!ret) - return 0; - - debug("%s: Environment %s failed to load (err=%d)\n", __func__, - drv->name, ret); - } - - return -ENODEV; + else + return env_get_char_spec(index); } int env_load(void) diff --git a/env/nvram.c b/env/nvram.c index 6f76fe4b8d..7cc62b631e 100644 --- a/env/nvram.c +++ b/env/nvram.c @@ -41,7 +41,10 @@ env_t *env_ptr = (env_t *)CONFIG_ENV_ADDR; #endif #ifdef CONFIG_SYS_NVRAM_ACCESS_ROUTINE -static int env_nvram_get_char(int index) +/** Call this function from overridden env_get_char_spec() if you need + * this functionality. + */ +int env_nvram_get_char(int index) { uchar c; @@ -113,9 +116,6 @@ static int env_nvram_init(void) U_BOOT_ENV_LOCATION(nvram) = { .location = ENVL_NVRAM, ENV_NAME("NVRAM") -#ifdef CONFIG_SYS_NVRAM_ACCESS_ROUTINE - .get_char = env_nvram_get_char, -#endif .load = env_nvram_load, .save = env_save_ptr(env_nvram_save), .init = env_nvram_init, diff --git a/include/environment.h b/include/environment.h index 6044b9e1b4..8696573d0d 100644 --- a/include/environment.h +++ b/include/environment.h @@ -217,17 +217,6 @@ struct env_driver { const char *name; enum env_location location; - /** -* get_char() - Read a character from the environment -* -* This method is optional. If not provided, a default implementation -* will read from gd->env_addr. -* -* @index: Index of character to read (0=first) -* @return character read, or -ve on error -*/ - int (*get_char)(int index); - /** * load() - Load the environment from storage * -- 2.14.1 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v3 09/15] env: Support multiple environments
On 02/07/2018 21:18, York Sun wrote: > On 02/07/2018 12:43 AM, Maxime Ripard wrote: >> Hi, >> >> On Tue, Jan 30, 2018 at 11:02:49PM +, York Sun wrote: >>> On 01/30/2018 12:16 PM, York Sun wrote: On 01/30/2018 11:40 AM, York Sun wrote: > On 01/30/2018 12:19 AM, Simon Goldschmidt wrote: >> On 23.01.2018 21:16, Maxime Ripard wrote: >>> Now that we have everything in place to support multiple environment, >>> let's >>> make sure the current code can use it. >>> >>> The priority used between the various environment is the same one that >>> was >>> used in the code previously. >>> >>> At read / init times, the highest priority environment is going to be >>> detected, >> >> Does priority handling really work here? Most env drivers seem to ignore >> the return value of env_import and may thus return success although >> importing the environment failed (only reading the data from the device >> succeeded). >> >> This is from reading the code, I haven't had a chance to test this, yet. > > It is broken on my LS1043ARDB with simply NOR flash. I am trying to > determine what went wrong. > I found the problem. The variable "env_load_location" is static. It is probably not write-able during booting from flash. It is expected to be set during ENVOP_INIT. But if I print this variable, it has ENVL_UNKNOWN. I can make it work by moving env_load_location to global data structure. >> >> That would work for me. >> That being said, this addition of multiple environments really slows down the booting process for me. I see every time env_get_char() is called, env_driver_lookup() runs. A simple call of env_get_f() gets slowed down dramatically. I didn't find out where the most time is spent yet. Does anyone else experience this unbearable slowness? >>> >>> I found the problem. In patch #3 in this series, the default get_char() >>> was dropped so there is no driver for a plain NOR flash. A quick (and >>> maybe dirty) fix is this >>> >>> >>> diff --git a/env/env.c b/env/env.c >>> index edfb575..210bae2 100644 >>> --- a/env/env.c >>> +++ b/env/env.c >>> @@ -159,7 +159,7 @@ int env_get_char(int index) >>> int ret; >>> >>> if (!drv->get_char) >>> - continue; >>> + return *(uchar *)(gd->env_addr + index); >>> >>> if (!env_has_inited(drv->location)) >>> continue; >> >> And this too. > > If you agree with this fix (actually revert your change earlier), I can > send out a patch. I still think we should get rid of the 'get_char' callback for the env drivers. While that could have made sense for some boards before the conversion to multiple environments (although I doubt that, as the environment is *not* checked for validity in this call), its meaning is totally lost when having multiple env drivers active. The whole purpose of multiple env drivers is that we select a valid driver in the 'load' callback. How could we possibly know that the 'get_char' callback of the highest prio env driver is what we want? I'd rather make 'env_get_char' weak and let boards decide if they really want this behaviour. A quick search through the current code base shows me *no* usage of 'get_char' in nvram.c (CONFIG_SYS_NVRAM_ACCESS_ROUTINE is never defined) and only 7 defconfigs that use ENV_IS_IN_EEPROM: imx31_phycore_defconfig imx31_phycore_eet_defconfig km_kirkwood_128m16_defconfig km_kirkwood_defconfig km_kirkwood_pci_defconfig mgcoge3un_defconfig portl2_defconfig Are these seven boards worth keeping this feature? Simon >> >>> With this temporary fix, my flash chip works again and I can boot all >>> the way up in a timely manner. I still don't like to call >>> env_driver_lookup() thousands of times to get a simple env variable. >>> >>> Can Maxime post a quick post soon? >> >> Given that you already made all the debugging, and the patches, and I >> have no way to test, I guess it would make more sense if you did it :) > > Yes, I have tested on my boards. I will send out this patch. > > York ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] cmd: nvedit: env_get_f must check for env_get_char error codes
On 02/02/2018 21:04, York Sun wrote: > On 02/02/2018 10:51 AM, Maxime Ripard wrote: > > > > Simon, This patch looks correct. But it doesn't fix NOR flash. Do you have plan to add .get_char function to other drivers? Without that function, we cannot get env variables before relocation. >>> >>> Ehrm, sorry I don't plan to do that, no: my target seems to run fine >>> without this. >>> >>> Given that only the eeprom and nvram env drivers support the get_char >>> method, I don't know if this is widely used at all. Maybe a better fallback >>> would be to just remove that get_char code path totally and always load from >>> the internal (default) environment until the full environment is available >>> (after relocation). >>> >>> After all, the environment variables loaded via get_char are not CRC checked >>> at all. To me, this is another indication that this code is not really >>> useful and should probably be removed. >> >> To be honest, I'm not really sure what get_char was here for in the >> first place, so getting rid of it sounds like a good idea :) >> > > On almost all my boards, a variable hwconfig is read before relocation > to determine DDR configuration. This has been broken. I don't mind you > remove some dead code. But this is breaking almost all my boards booting > from NOR flash. When talking about removing a feature I meant the code that exists for 2 env drivers only where single characters are loaded from an external environment without checking its crc first. The thing you seem to need is reading from the default environment before relocation. This should not be removed, of course! I might prepare a patch to better show what I mean... Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v3 12/15] env: Allow to build multiple environments in Kconfig
On 01.02.2018 20:47, Maxime Ripard wrote: > On Thu, Feb 01, 2018 at 11:06:14AM +0100, Simon Goldschmidt wrote: >> On 23.01.2018 21:17, Maxime Ripard wrote: >> > Now that we have everything in place in the code, let's allow to build >> > multiple environments backend through Kconfig. >> > >> > Reviewed-by: Andre Przywara >> > Reviewed-by: Lukasz Majewski >> > Reviewed-by: Simon Glass >> > Signed-off-by: Maxime Ripard >> >> I get a build error when enabling CONFIG_ENV_IS_IN_SPI_FLASH and >> CONFIG_ENV_IS_IN_MMC at the same time. > > Is that happening in any of the current defconfig right now? Or is it > only when you add more environments? No, this is with my own config. I'm trying out the new feature :-) > >> The build error is in host tools, not in U-Boot or SPL itself. In fact, this >> is not specific to CONFIG_ENV_IS_IN_SPI_FLASH but to the combination of >> CONFIG_ENV_IS_IN_MMC and any of the environments marked as ENVCRC- in >> tools/Makefile. >> >> The actual error is that the compiler does not know standard types in efi.h >> and mmc.h, e.g.: >> In file included from include/blk.h:11:0, >> from include/part.h:10, >> from include/mmc.h:16, >> from include/environment.h:168, >> from ./tools/../env/embedded.c:16, >> from tools/env/embedded.c:2: >> include/efi.h:32:2: error: unknown type name ‘u8’ >> u8 b[16]; >> ^~ >> >> I can't think of a correct fix right now... > > I'm not sure what it could be either, that file looks like it would > need a quite big rework, in order to be able to operate properly. > > Do you actually need those? Maybe we can just disable those in Kconfig > to forbid such a combination? I planned to have the environment in both SPI flash and eMMC to be able to use a common U-Boot binary accross multiple boards. I don't need 'tools/envcrc' though. And this is where the build error is. Maybe we could disable the combination for 'tools/envcrc' only? Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] ARM: socfpga: Convert callers of cm_write_with_phase for wait_for_bit_le32
On 01/26/2018 17:28, Tom Rini wrote: > Now that we have and use wait_for_bit_le32() available, the callers of > cm_write_with_phase() should not be casting values to u32 and instead we > expect a const void *, so provide that directly. > > Fixes: 48263504c8d5 ("wait_bit: use wait_for_bit_le32 and remove > wait_for_bit") > Cc: Marek Vasut > Signed-off-by: Tom Rini > --- > arch/arm/mach-socfpga/clock_manager_gen5.c | 10 +- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/arch/arm/mach-socfpga/clock_manager_gen5.c > b/arch/arm/mach-socfpga/clock_manager_gen5.c > index 3d048ba3e432..4e5b6d169371 100644 > --- a/arch/arm/mach-socfpga/clock_manager_gen5.c > +++ b/arch/arm/mach-socfpga/clock_manager_gen5.c > @@ -33,7 +33,7 @@ static void cm_write_ctrl(u32 val) > } > > /* function to write a clock register that has phase information */ > -static int cm_write_with_phase(u32 value, u32 reg_address, u32 mask) > +static int cm_write_with_phase(u32 value, const void *reg_address, u32 mask) Shouldn't that better be a non-const void * since it is used for writing a value, too? That 'const' is later casted away by __arch_putl(), but it still looks wrong to me. Simon > { >int ret; > > @@ -268,26 +268,26 @@ int cm_basic_init(const struct cm_config * const cfg) > * are aligned nicely; so we can change any phase. > */ > ret = cm_write_with_phase(cfg->ddrdqsclk, > - (u32)&clock_manager_base->sdr_pll.ddrdqsclk, > + &clock_manager_base->sdr_pll.ddrdqsclk, > CLKMGR_SDRPLLGRP_DDRDQSCLK_PHASE_MASK); > if (ret) > return ret; > > /* SDRAM DDR2XDQSCLK */ > ret = cm_write_with_phase(cfg->ddr2xdqsclk, > - > (u32)&clock_manager_base->sdr_pll.ddr2xdqsclk, > + &clock_manager_base->sdr_pll.ddr2xdqsclk, > CLKMGR_SDRPLLGRP_DDR2XDQSCLK_PHASE_MASK); > if (ret) > return ret; > > ret = cm_write_with_phase(cfg->ddrdqclk, > - (u32)&clock_manager_base->sdr_pll.ddrdqclk, > + &clock_manager_base->sdr_pll.ddrdqclk, > CLKMGR_SDRPLLGRP_DDRDQCLK_PHASE_MASK); > if (ret) > return ret; > > ret = cm_write_with_phase(cfg->s2fuser2clk, > - > (u32)&clock_manager_base->sdr_pll.s2fuser2clk, > + &clock_manager_base->sdr_pll.s2fuser2clk, > CLKMGR_SDRPLLGRP_S2FUSER2CLK_PHASE_MASK); > if (ret) > return ret; > -- > 2.7.4 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [RESEND PATCH] ddr: altera: silence PHY calibration unless in debug mode
This driver has been using printf() including filename since it was added. Convert to using debug() instead. Signed-off-by: Simon Goldschmidt --- drivers/ddr/altera/sequencer.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/ddr/altera/sequencer.c b/drivers/ddr/altera/sequencer.c index 6c6bd90e94..42e87b50d3 100644 --- a/drivers/ddr/altera/sequencer.c +++ b/drivers/ddr/altera/sequencer.c @@ -3534,7 +3534,7 @@ static void debug_mem_calibrate(int pass) u32 debug_info; if (pass) { - printf("%s: CALIBRATION PASSED\n", __FILE__); + debug("%s: CALIBRATION PASSED\n", __FILE__); gbl->fom_in /= 2; gbl->fom_out /= 2; @@ -3553,7 +3553,7 @@ static void debug_mem_calibrate(int pass) writel(debug_info, &phy_mgr_cfg->cal_debug_info); writel(PHY_MGR_CAL_SUCCESS, &phy_mgr_cfg->cal_status); } else { - printf("%s: CALIBRATION FAILED\n", __FILE__); + debug("%s: CALIBRATION FAILED\n", __FILE__); debug_info = gbl->error_stage; debug_info |= gbl->error_substage << 8; @@ -3570,7 +3570,7 @@ static void debug_mem_calibrate(int pass) writel(debug_info, &sdr_reg_file->failing_stage); } - printf("%s: Calibration complete\n", __FILE__); + debug("%s: Calibration complete\n", __FILE__); } /** @@ -3741,7 +3741,7 @@ int sdram_calibration_full(void) initialize_tracking(); - printf("%s: Preparing to start memory calibration\n", __FILE__); + debug("%s: Preparing to start memory calibration\n", __FILE__); debug("%s:%d\n", __func__, __LINE__); debug_cond(DLEVEL >= 1, -- 2.12.2.windows.2 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] ddr: altera: silence PHY calibration unless in debug mode
On 01/11/2018 12:28, Marek Vasut wrote: > On 01/11/2018 08:19 AM, Goldschmidt Simon wrote: >> This driver has been using printf() including filename since it was >> added. Convert to using debug() instead. >> >> Signed-off-by: Simon Goldschmidt > > Applied, thanks. I can't see this one in u-boot-socfpga. Anything wrong here? Regards, Simon >> --- >> >> drivers/ddr/altera/sequencer.c | 8 >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/ddr/altera/sequencer.c b/drivers/ddr/altera/sequencer.c >> index 6c6bd90e94..42e87b50d3 100644 >> --- a/drivers/ddr/altera/sequencer.c >> +++ b/drivers/ddr/altera/sequencer.c >> @@ -3534,7 +3534,7 @@ static void debug_mem_calibrate(int pass) >> u32 debug_info; >> >> if (pass) { >> - printf("%s: CALIBRATION PASSED\n", __FILE__); >> + debug("%s: CALIBRATION PASSED\n", __FILE__); >> >> gbl->fom_in /= 2; >> gbl->fom_out /= 2; >> @@ -3553,7 +3553,7 @@ static void debug_mem_calibrate(int pass) >> writel(debug_info, &phy_mgr_cfg->cal_debug_info); >> writel(PHY_MGR_CAL_SUCCESS, &phy_mgr_cfg->cal_status); >> } else { >> - printf("%s: CALIBRATION FAILED\n", __FILE__); >> + debug("%s: CALIBRATION FAILED\n", __FILE__); <> >> debug_info = gbl->error_stage; >> debug_info |= gbl->error_substage << 8; >> @@ -3570,7 +3570,7 @@ static void debug_mem_calibrate(int pass) >> writel(debug_info, &sdr_reg_file->failing_stage); >> } >> >> - printf("%s: Calibration complete\n", __FILE__); >> + debug("%s: Calibration complete\n", __FILE__); >> } >> >> /** >> @@ -3741,7 +3741,7 @@ int sdram_calibration_full(void) >> >> initialize_tracking(); >> >> - printf("%s: Preparing to start memory calibration\n", __FILE__); >> + debug("%s: Preparing to start memory calibration\n", __FILE__); >> >> debug("%s:%d\n", __func__, __LINE__); >> debug_cond(DLEVEL >= 1, >> > > ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH] ddr: altera: silence PHY calibration unless in debug mode
This driver has been using printf() including filename since it was added. Convert to using debug() instead. Signed-off-by: Simon Goldschmidt --- drivers/ddr/altera/sequencer.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/ddr/altera/sequencer.c b/drivers/ddr/altera/sequencer.c index 6c6bd90e94..42e87b50d3 100644 --- a/drivers/ddr/altera/sequencer.c +++ b/drivers/ddr/altera/sequencer.c @@ -3534,7 +3534,7 @@ static void debug_mem_calibrate(int pass) u32 debug_info; if (pass) { - printf("%s: CALIBRATION PASSED\n", __FILE__); + debug("%s: CALIBRATION PASSED\n", __FILE__); gbl->fom_in /= 2; gbl->fom_out /= 2; @@ -3553,7 +3553,7 @@ static void debug_mem_calibrate(int pass) writel(debug_info, &phy_mgr_cfg->cal_debug_info); writel(PHY_MGR_CAL_SUCCESS, &phy_mgr_cfg->cal_status); } else { - printf("%s: CALIBRATION FAILED\n", __FILE__); + debug("%s: CALIBRATION FAILED\n", __FILE__); debug_info = gbl->error_stage; debug_info |= gbl->error_substage << 8; @@ -3570,7 +3570,7 @@ static void debug_mem_calibrate(int pass) writel(debug_info, &sdr_reg_file->failing_stage); } - printf("%s: Calibration complete\n", __FILE__); + debug("%s: Calibration complete\n", __FILE__); } /** @@ -3741,7 +3741,7 @@ int sdram_calibration_full(void) initialize_tracking(); - printf("%s: Preparing to start memory calibration\n", __FILE__); + debug("%s: Preparing to start memory calibration\n", __FILE__); debug("%s:%d\n", __func__, __LINE__); debug_cond(DLEVEL >= 1, -- 2.11.0 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2 0/3] cadence-quadspi: Fix issues with non 32bit aligned accesses
On Tue 09/01/18 14:19, Vignesh R wrote: > This series reverts use of bounce_buf.c for non-DMA related alignment > restriction > and replaces it with local bounce buffer to handle problems with non 32 bit > aligned > writes on TI platforms. > Based on top of Jason's series: > https://patchwork.ozlabs.org/cover/856431/ > > Tested on K2G EVM. For this whole series: Tested on a socfpga-cyclonev board (with a Micron N25QL256A): Tested-by: Simon Goldschmidt After applying this series on top of Jason's v5, qspi on the socfpga is finally working without local fixes! Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] spl: unbreak CONFIG_SPL_MULTI_DTB_FIT after fixing CONFIG_OF_EMBED
On Wed, 10/01/18 09:48, Lokesh Vutla wrote: > On Wednesday 10 January 2018 12:32 PM, Goldschmidt Simon wrote: > > Commit 9bd76b80 "spl: make CONFIG_OF_EMBED pass dts through fdtgrep" > > moved the fdtgrep code from scripts/Makefile.spl to dts/Makefile so > > that the dtb is stripped in embedded mode, too. > > > > This broke CONFIG_SPL_MULTI_DTB_FIT where fdtgrep is still called from > > scripts/Makefile.spl to strip all dtbs in CONFIG_SPL_OF_LIST. > > To fix that, cmd_fdtgrep is brought back into scripts/Makefile.spl at > > the downside of having it duplicated in two makefiles. > > Missing Signed-off-by? Also a Reported-by credits would be nice :) Right. I'll add that in a next version, sorry. > > > --- > > scripts/Makefile.spl | 16 > > 1 file changed, 16 insertions(+) > > > > diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl index > > 64390e5785..72e74f14c3 100644 > > --- a/scripts/Makefile.spl > > +++ b/scripts/Makefile.spl > > @@ -239,6 +239,22 @@ $(obj)/$(SPL_BIN)-pad.bin: $(obj)/$(SPL_BIN) > > @bss_size_str=$(shell $(NM) $< | awk 'BEGIN {size = 0} /__bss_size/ > > {size > = $$1} END {print "ibase=16; " toupper(size)}' | bc); \ > > dd if=/dev/zero of=$@ bs=1 count=$${bss_size_str} 2>/dev/null; > > > > +# Pass the original device tree file through fdtgrep twice. The first > > +pass # removes any unwanted nodes (i.e. those which don't have the # > > +'u-boot,dm-pre-reloc' property and thus are not needed by SPL. The > > +second # pass removes various unused properties from the remaining nodes. > > +# The output is typically a much smaller device tree file. > > +ifeq ($(CONFIG_TPL_BUILD),y) > > +fdtgrep_props := -b u-boot,dm-pre-reloc -b u-boot,dm-tpl else > > +fdtgrep_props := -b u-boot,dm-pre-reloc -b u-boot,dm-spl endif > > +quiet_cmd_fdtgrep = FDTGREP $@ > > + cmd_fdtgrep = $(objtree)/tools/fdtgrep $(fdtgrep_props) -RT $< \ > > + -n /chosen -n /config -O dtb | \ > > + $(objtree)/tools/fdtgrep -r -O dtb - -o $@ \ > > + $(addprefix -P ,$(subst > $\",,$(CONFIG_OF_SPL_REMOVE_PROPS))) > > + > > hmm..we are duplicating code here. Why can't dtb build for > CONFIG_OF_EMBED be moved to scripts/Makefile.spl? I don't like the code duplication either. Back in November, I tried to create the fdtgrep'ed ftd from Makefile.spl but failed. Seems I'm not a pro in makefiles... Of course I'd appreciate it if you'd find a solution for that! Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH] spl: unbreak CONFIG_SPL_MULTI_DTB_FIT after fixing CONFIG_OF_EMBED
Commit 9bd76b80 "spl: make CONFIG_OF_EMBED pass dts through fdtgrep" moved the fdtgrep code from scripts/Makefile.spl to dts/Makefile so that the dtb is stripped in embedded mode, too. This broke CONFIG_SPL_MULTI_DTB_FIT where fdtgrep is still called from scripts/Makefile.spl to strip all dtbs in CONFIG_SPL_OF_LIST. To fix that, cmd_fdtgrep is brought back into scripts/Makefile.spl at the downside of having it duplicated in two makefiles. --- scripts/Makefile.spl | 16 1 file changed, 16 insertions(+) diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl index 64390e5785..72e74f14c3 100644 --- a/scripts/Makefile.spl +++ b/scripts/Makefile.spl @@ -239,6 +239,22 @@ $(obj)/$(SPL_BIN)-pad.bin: $(obj)/$(SPL_BIN) @bss_size_str=$(shell $(NM) $< | awk 'BEGIN {size = 0} /__bss_size/ {size = $$1} END {print "ibase=16; " toupper(size)}' | bc); \ dd if=/dev/zero of=$@ bs=1 count=$${bss_size_str} 2>/dev/null; +# Pass the original device tree file through fdtgrep twice. The first pass +# removes any unwanted nodes (i.e. those which don't have the +# 'u-boot,dm-pre-reloc' property and thus are not needed by SPL. The second +# pass removes various unused properties from the remaining nodes. +# The output is typically a much smaller device tree file. +ifeq ($(CONFIG_TPL_BUILD),y) +fdtgrep_props := -b u-boot,dm-pre-reloc -b u-boot,dm-tpl +else +fdtgrep_props := -b u-boot,dm-pre-reloc -b u-boot,dm-spl +endif +quiet_cmd_fdtgrep = FDTGREP $@ + cmd_fdtgrep = $(objtree)/tools/fdtgrep $(fdtgrep_props) -RT $< \ + -n /chosen -n /config -O dtb | \ + $(objtree)/tools/fdtgrep -r -O dtb - -o $@ \ + $(addprefix -P ,$(subst $\",,$(CONFIG_OF_SPL_REMOVE_PROPS))) + $(obj)/$(SPL_BIN).dtb: dts/dt-spl.dtb FORCE $(call if_changed,copy) -- 2.12.2.windows.2 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] spl: make CONFIG_OF_EMBED pass dts through fdtgrep
On Tue, 09/01/18 15:43, Lokesh Vutla wrote: > On Sunday 26 November 2017 05:08 PM, Simon Glass wrote: > > On 21 November 2017 at 05:29, Goldschmidt Simon > > wrote: > >> Building spl with CONFIG_OF_EMBED enabled results in an error message > >> on my board: "SPL image too big". This is because the fdtgrep build > >> step is only executed for CONFIG_OF_SEPARATE. > >> > >> Fix this by moving the fdtgrep build step ('cmd_fdtgreo') from > >> scripts/Makefile.spl to dts/Makefile so that the reduced dtb is > >> available for all kinds of spl builds. > >> > >> The resulting variable name for the embedded device tree blob > >> changes, too, which is why common.h and fdtdec.c have tiny changes. > >> > >> Signed-off-by: Simon Goldschmidt > > This is breaking SPL build with CONFIG_SPL_MULTI_DTB_FIT enabled as it tries > to call fdtgrep for shrinking all the dtbs in SPL_OF_LIST. You're right. Sorry for that. I'll send a patch that fixes this. Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 3/3] spi: cadence_qspi_apb: Make flash writes 32 bit aligned
On Mon, 08/01/2018 12:18, Vignesh R wrote: > Make flash writes 32 bit aligned by using bounce buffers to deal with non 32 > bit > aligned buffers. Allocate a 512 byte bounce buffer (max known page size > currently) for this case. Looking at drivers/mtd/spi/sf_dataflash.c, I see at least one chip with 1024 byte page size (at45db642d). This should probably be a constant somewhere, or at least be checked at runtime, see below. > This is required because as per TI K2G TRM[1], the external master is only > permitted to issue 32-bit data interface writes until the last word of an > indirect > transfer. Otherwise indirect writes is known to fail sometimes. > > [1] http://www.ti.com/lit/ug/spruhy8g/spruhy8g.pdf > > Signed-off-by: Vignesh R > --- > drivers/spi/cadence_qspi.c | 13 - > drivers/spi/cadence_qspi.h | 1 + > drivers/spi/cadence_qspi_apb.c | 10 +- > 3 files changed, 18 insertions(+), 6 deletions(-) > > diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c index > 991a7160bdb8..49c9b477e678 100644 > --- a/drivers/spi/cadence_qspi.c > +++ b/drivers/spi/cadence_qspi.c > @@ -158,6 +158,10 @@ static int cadence_spi_probe(struct udevice *bus) > > priv->regbase = plat->regbase; > priv->ahbbase = plat->ahbbase; > + /* Bounce buf to handle unaligned writes. 512B is max pagesize */ > + priv->bounce_buf = malloc(512); This should probably be a named constant. > + if (!priv->bounce_buf) > + return -ENOMEM; > > if (!priv->qspi_is_init) { > cadence_qspi_apb_controller_init(plat); > @@ -259,8 +263,15 @@ static int cadence_spi_xfer(struct udevice *dev, unsigned > int bitlen, > err = cadence_qspi_apb_indirect_write_setup > (plat, priv->cmd_len, cmd_buf); > if (!err) { > + const u8 *txbuf = dout; > + > + if ((uintptr_t)txbuf % 4) { > + memcpy(priv->bounce_buf, dout, > +data_bytes); Without checking the size of the buffer allocated for bounce_buf here, you risk a buffer overflow for chips with larger page size. > + txbuf = priv->bounce_buf; > + } > err = cadence_qspi_apb_indirect_write_execute > - (plat, data_bytes, dout); > + (plat, data_bytes, txbuf); > } > break; > default: > diff --git a/drivers/spi/cadence_qspi.h b/drivers/spi/cadence_qspi.h index > 83154210bd95..c97419af3de3 100644 > --- a/drivers/spi/cadence_qspi.h > +++ b/drivers/spi/cadence_qspi.h > @@ -43,6 +43,7 @@ struct cadence_spi_priv { > unsigned intqspi_calibrated_hz; > unsigned intqspi_calibrated_cs; > unsigned intprevious_hz; > + void*bounce_buf; > }; > > /* Functions call declaration */ > diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c > index fa8af33ae19b..fd3ece4cb129 100644 > --- a/drivers/spi/cadence_qspi_apb.c > +++ b/drivers/spi/cadence_qspi_apb.c > @@ -727,11 +727,11 @@ int cadence_qspi_apb_indirect_write_execute(struct > cadence_spi_platdata *plat, > > while (remaining > 0) { > write_bytes = remaining > page_size ? page_size : remaining; > - /* Handle non-4-byte aligned access to avoid data abort. */ > - if (((uintptr_t)txbuf % 4) || (write_bytes % 4)) > - writesb(plat->ahbbase, txbuf, write_bytes); > - else > - writesl(plat->ahbbase, txbuf, write_bytes >> 2); > + writesl(plat->ahbbase, txbuf, write_bytes >> 2); > + if (write_bytes % 4) > + writesb(plat->ahbbase, > + txbuf + rounddown(write_bytes, 4), > + write_bytes % 4); > > ret = wait_for_bit("QSPI", plat->regbase + > CQSPI_REG_SDRAMLEVEL, > CQSPI_REG_SDRAMLEVEL_WR_MASK << > -- > 2.15.1 Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 0/3] cadence-quadspi: Fix issues with non 32bit aligned accesses
On 08/01/2018 12:18m Vignesh R wrote: > This series reverts use of bounce_buf.c for non-DMA related alignment > restriction > and replaces it with local bounce buffer to handle problems with non 32 bit > aligned > writes on TI platforms. > Based on top of Jason's series: > https://patchwork.ozlabs.org/cover/856431/ > > Tested on K2G EVM. > > Goldschmidt Simon (1): > Revert "spi: cadence_qspi_apb: Use 32 bit indirect read transaction > when possible" > > Vignesh R (2): > Revert "spi: cadence_qspi_apb: Use 32 bit indirect write transaction > when possible" > spi: cadence_qspi_apb: Make flash writes 32 bit aligned > > drivers/spi/cadence_qspi.c | 13 - > drivers/spi/cadence_qspi.h | 1 + > drivers/spi/cadence_qspi_apb.c | 42 > +--- > include/configs/k2g_evm.h| 1 - > include/configs/socfpga_common.h | 1 - > include/configs/stv0991.h| 1 - > 6 files changed, 22 insertions(+), 37 deletions(-) For this whole series: Tested on a socfpga-cyclonev board (with a Micron N25QL256A): Tested-by: Simon Goldschmidt After applying this series on top of Jason's v5, qspi on the socfpga is finally working without local fixes! Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] QSPI "sf probe ...", "sf read ..." on Altera SoC FPGA
On Mon, 08/012018 06:27, Vignesh R wrote: > On Monday 08 January 2018 09:10 AM, Jason Rush wrote: > [...] > >>> 1. The indaddrtrig register was being programmed with an incorrect > >>> value for socfpga as the result of assuming it should be programmed > >>> with the same address as the ahbbase address. This issue is > >>> resolved by adopting the Linux DT bindings, which has an independent > >>> setting for the indaddrtrig register so the register can be set correctly > >>> on all > architectures. Plus it aligns the DT between u-boot and Linux. > >> That should be an easy patch, so this is the patchset 0/5..5/5 that > >> you just submitted ? > > Yes. I saw you Acked it, thank you. > >>> 2. The cadence driver was modified at one point to use the bouncebuf > >>> functions to fix an issue on a TI architecture that expected, where > >>> if I recall correctly all reads except the last have to be 32-bit > >>> reads. However, since the bouncebuf was designed for DMA transfers, > >>> it invalidates the data cache after reading, but since the cadence > >>> is using cpu transfers the newly read data is thrown away when the > >>> cache is invalidated. This issue is resolved by reverting the commit that > introduced using the bounce buffer for read operations, which according to > Vignesh don't cause any issues to the TI architecture. > >> Hm, I wonder why you need bounce buffer at all here. The CQSPI > >> literally reads/writes a register space (or some FIFO in register > >> space), there is no DMA involved at all. I also wonder why we have to > >> manipulate with cache at all here. > > > > I agree, I don't believe this needs a bounce buffer at all. This > > isn't a DMA, there is no need for cache manipulation. Vignesh > > understands the problem better than I do on the TI platform, but I > > believe it was used since it was an easy way to ensure the register > > read/writes > were all 32-bits wide up until the last read/write. > > Yes, that was the intention. Unfortunately, I chose to use common bounce > buffer > implementation which was doing cache manipulations. > > > I believe the bounce buffer should be removed from the CQSPI driver > > and a different solution should be implemented, but Vignesh should > > weigh in on that since it effects his architecture. > > > > CQSPI on TI K2G has problems with non 32 bit aligned write operations. > But read operations are unaffected. Therefore I have Ack'ed Simon's patch > reverting bouncebuf for read. For writes, I have patches to revert common > bouncebuf usage and use a local pagesize buffer for overcome alignment issue. > I > am waiting for current patch backlogs to be merged so that its easy for > testing w/o > specifying bunch of dependent patches. > > Or if Simon agrees, I can add his patch to my series post it to mailing list > (rebased > on top of Jason's series)? Well, it's not really "my" patch, anyway. It reverts a commit of yours, so sure, as long as this does not stand in the way of getting qspi running on 2018.03, go ahead and itegrate it in your patchset. I'd be happy to have this sent now so I can test both patchsets on top of 2018.01(-rc). Thanks, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] QSPI "sf probe ...", "sf read ..." on Altera SoC FPGA
On Fri, 05/01/2018, Marek Vasut wrote: > On 01/05/2018 08:31 PM, Goldschmidt Simon wrote: >> On Fri, 05/01/2018 Marek Vasut wrote: >>> On 01/05/2018 04:49 PM, Goldschmidt Simon wrote: >> >> >> >>>>>> OK, so I need these patches to get qspi work on socfpga: >>>>>> >>>>>> - Series "spi: cadence_spi: Adopt Linux DT bindings" (v4) from Jason >>>>>> Rush: >>>>>> https://patchwork.ozlabs.org/project/uboot/list/?series=13864 >>>>>> - Patch "Revert "spi: cadence_qspi_apb: Use 32 bit indirect read >>>>>> transaction when possible" (v2) >>>>>> https://patchwork.ozlabs.org/patch/838871/ >>>>> >>>>> I've waited for ack/tested-by from marek or someone who usually worked >>>>> on these cadence. >>>> >>>> Vignesh acked, who already did some of the last changes. But Ok, I've >>>> added Marek to the loop. >>>> >>>> Marek, do you see any problems here? Are you running QSPI on the >>>> socfpga platform anywhere? >>> I am not entirely sure what this partial thread is all about, do you >>> need some patches Acked ? Repost them including the Acks collected >>> already and CC me, I want to review them. PW link is not enough. >> >> Marek, you were one of the people addressed in "to:" when Jason Rush >> sent "[PATCH v4 1/5] spi: cadence_spi: Sync DT bindings with Linux" >> and successors on Nov. 16th 2017. >> >> You were also in the "to:" field when I sent "[PATCH v4 1/5] spi: >> cadence_spi: Sync DT bindings with Linux" on Nov. 17th 2017. >> >> Should I resend them anyway? > > I really dunno what to make of this sparse thread, sorry. Also, I don't > quite remember what happened in this thread on November 17th, sorry. > > So maybe you should elaborate what you expect me to do ... or just > resend the patches, yes. OK, I'll resend the patches then. I'll try to combine the 2 series into one but have to check with Jason first. Thanks, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] QSPI "sf probe ...", "sf read ..." on Altera SoC FPGA
On Fri, 05/01/2018 Marek Vasut wrote: > On 01/05/2018 04:49 PM, Goldschmidt Simon wrote: >>>> OK, so I need these patches to get qspi work on socfpga: >>>> >>>> - Series "spi: cadence_spi: Adopt Linux DT bindings" (v4) from Jason Rush: >>>> https://patchwork.ozlabs.org/project/uboot/list/?series=13864 >>>> - Patch "Revert "spi: cadence_qspi_apb: Use 32 bit indirect read >>>> transaction when possible" (v2) >>>> https://patchwork.ozlabs.org/patch/838871/ >>> >>> I've waited for ack/tested-by from marek or someone who usually worked >>> on these cadence. >> >> Vignesh acked, who already did some of the last changes. But Ok, I've >> added Marek to the loop. >> >> Marek, do you see any problems here? Are you running QSPI on the >> socfpga platform anywhere? > I am not entirely sure what this partial thread is all about, do you > need some patches Acked ? Repost them including the Acks collected > already and CC me, I want to review them. PW link is not enough. Marek, you were one of the people addressed in "to:" when Jason Rush sent "[PATCH v4 1/5] spi: cadence_spi: Sync DT bindings with Linux" and successors on Nov. 16th 2017. You were also in the "to:" field when I sent "[PATCH v4 1/5] spi: cadence_spi: Sync DT bindings with Linux" on Nov. 17th 2017. Should I resend them anyway? Thanks, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] QSPI "sf probe ...", "sf read ..." on Altera SoC FPGA
+ Marek (as Jagan wants an ack) On 05/01/2018 Jagan Teki wrote: > On Fri, Jan 5, 2018 at 5:32 PM, Goldschmidt Simon wrote: >> + Vignesh >> + Jason >> >> On Wed, 03/01/2018 16:57, Goldschmidt Simon wrote: >>> On Wed, 03/01/2018 14:51, Jagan Teki wrote: >>> >> There were already patches posted on this list by me and others, but >>> >> unfortunately they haven't made it into the repository, yet. >>> >> >>> >> Jagan, could you comment on the status of these fixes? I can search >>> >> for the patchwork items related if you want me to. >>> > >>> > 2 out of 1 of this[1] have some discussion still going is it? >>> > >>> > [1] https://patchwork.ozlabs.org/patch/838195/ >>> >>> No, that series should be dropped. I don't know if I can do anything about >>> that in >>> patchwork though? >>> >>> Let me check the patches from my upstreaming queue when I'm back at work >>> tomorrow. I'll send a list of patchwork items I needed to get QSPI running >>> on <>> mach-socfpga. >> >> OK, so I need these patches to get qspi work on socfpga: >> >> - Series "spi: cadence_spi: Adopt Linux DT bindings" (v4) from Jason Rush: >> https://patchwork.ozlabs.org/project/uboot/list/?series=13864 >> - Patch "Revert "spi: cadence_qspi_apb: Use 32 bit indirect read transaction >> when possible" (v2) >> https://patchwork.ozlabs.org/patch/838871/ > > I've waited for ack/tested-by from marek or someone who usually worked > on these cadence. Vignesh acked, who already did some of the last changes. But Ok, I've added Marek to the loop. Marek, do you see any problems here? Are you running QSPI on the socfpga platform anywhere? > >> >> All patches were discussed with Vignesh in November. Could we make >> sure these make it into 2018.03 now that we missed 2018.01? >> >> Aside from that, I have this patch running which ensures my QSPI (that >> does not have a reset line) is put into 3 byte address mode that >> U-Boot needs. This would be *very* helpful, too: >> https://patchwork.ozlabs.org/patch/826919/ > > issue discussing with spi-nor changes as well, we will figure it out > and try for best possible. Ok, this is a different issue anyway. It is not related to socfpga or cadence qspi. Maybe I can even trick my Linux to use 4 byte opcodes instead of the 4 byte mode... Thanks, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] QSPI "sf probe ...", "sf read ..." on Altera SoC FPGA
+ Vignesh + Jason On Wed, 03/01/2018 16:57, Goldschmidt Simon wrote: > On Wed, 03/01/2018 14:51, Jagan Teki wrote: > >> There were already patches posted on this list by me and others, but > >> unfortunately they haven't made it into the repository, yet. > >> > >> Jagan, could you comment on the status of these fixes? I can search > >> for the patchwork items related if you want me to. > > > > 2 out of 1 of this[1] have some discussion still going is it? > > > > [1] https://patchwork.ozlabs.org/patch/838195/ > > No, that series should be dropped. I don't know if I can do anything about > that in > patchwork though? > > Let me check the patches from my upstreaming queue when I'm back at work > tomorrow. I'll send a list of patchwork items I needed to get QSPI running on > mach-socfpga. OK, so I need these patches to get qspi work on socfpga: - Series "spi: cadence_spi: Adopt Linux DT bindings" (v4) from Jason Rush: https://patchwork.ozlabs.org/project/uboot/list/?series=13864 - Patch "Revert "spi: cadence_qspi_apb: Use 32 bit indirect read transaction when possible" (v2) https://patchwork.ozlabs.org/patch/838871/ All patches were discussed with Vignesh in November. Could we make sure these make it into 2018.03 now that we missed 2018.01? Aside from that, I have this patch running which ensures my QSPI (that does not have a reset line) is put into 3 byte address mode that U-Boot needs. This would be *very* helpful, too: https://patchwork.ozlabs.org/patch/826919/ Thanks, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] QSPI "sf probe ...", "sf read ..." on Altera SoC FPGA
On Wed, 03/01/2018 14:51, Jagan Teki wrote: >> There were already patches posted on this list by me and others, but >> unfortunately they haven't made it into the repository, yet. >> >> Jagan, could you comment on the status of these fixes? I can search >> for the patchwork items related if you want me to. > > 2 out of 1 of this[1] have some discussion still going is it? > > [1] https://patchwork.ozlabs.org/patch/838195/ No, that series should be dropped. I don't know if I can do anything about that in patchwork though? Let me check the patches from my upstreaming queue when I'm back at work tomorrow. I'll send a list of patchwork items I needed to get QSPI running on mach-socfpga. Thanks, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] QSPI "sf probe ...", "sf read ..." on Altera SoC FPGA
+ Jagan On Wed, 03/01/2018, Mr. goldenstreet wrote: > hey, i have looked at this thread: > http://u-boot.10912.n7.nabble.com/QSPI-quot-sf-probe-quot-quot-sf-read-quot-on- > Altera-SoC-FPGA-td304882.html > > i'm having the same problem with Arria 5, when i try to use the "sf probe" > command on the nor flash, the result i'm getting0 is: > > SF: Detected n25q512 with page size 256 Bytes, erase size 64 KiB, total > 64 MiB > ### ERROR ### Please RESET the board ### > > i have already tried to use the patches for the uboot code and also to add > more > defines before making the uboot, but it still doesn't work for me. > > any ideas? thanks ahead. There were already patches posted on this list by me and others, but unfortunately they haven't made it into the repository, yet. Jagan, could you comment on the status of these fixes? I can search for the patchwork items related if you want me to. I'd really like these fixes to go in without waiting for the new spi-nor code. Thanks, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [RFC 0/5] sf: Update spi-nor framework
On Tuesday 12 December 2017 11:51, Vignesh R wrote: > [...] > Many of the newer SPI NOR flashes such as MT35x do not support U-Boot's > legacy way of accessing >128Mb region. > Are you planning to submit dm-spi-nor anytime soon? If not, then IMO, at > least patch 2/5 is worth considering for now. I haven't tested that patch, but I assume it uses 4-byte mode instead of the bank register? That would certainly improve the situation in my case, too, where the flash is in 4-byte mode after soft reset and we would have to set it back to 3-byte mode. (https://patchwork.ozlabs.org/patch/826919/) Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] Linux hang
Pepperl+Fuchs GmbH, Mannheim Geschaeftsfuehrer/Managing Directors: Dr.-Ing. Gunther Kegel (Vors./CEO), Werner Guthier, Mehmet Hatiboglu Vorsitzender des Aufsichtsrats/Chairman of the supervisory board: Claus Michael Registergericht/Register Court: AG Mannheim HRB 4713 On 2017-12-07 12:01, Siegmund, Jan wrote: > Hi all, > does anybody have an idea for the following problem? > > * FPGA is programmed using an overlay > * FPGA writes to SDRAM via the FPGA2SDRAM-bridge > * Linux hangs and the watchdog resets the board (the FPGA stays programmed) > * After the reset and boot, the FPGA is reprogrammed using the same overlay > * Now, the FPGA can write to the SDRAM without a problem > [..] I haven't tried this method of programming the fpga yet (only programmed from U-Boot for now). But reading the SPL source code, it seems as the bridges are taken out of reset if the fpga is programmed when the SPL runs. That's a difference. And it would mean using your way of programming the fpga, the bridges might still be in reset. Regards, Simon Wichtiger Hinweis: Diese E-Mail einschliesslich ihrer Anhaenge enthaelt vertrauliche und rechtlich geschuetzte Informationen, die nur fuer den Adressaten bestimmt sind. Sollten Sie nicht der bezeichnete Adressat sein, so teilen Sie dies bitte dem Absender umgehend mit und loeschen Sie diese Nachricht und ihre Anhaenge. Die unbefugte Weitergabe, das Anfertigen von Kopien und jede Veraenderung der E-Mail ist untersagt. Der Absender haftet nicht fuer Inhalte von veraenderten E-Mails. Important Information: This e-mail message including its attachments contains confidential and legally protected information solely intended for the addressee. If you are not the intended addressee of this message, please contact the addresser immediately and delete this message including its attachments. The unauthorized dissemination, copying and change of this e-mail are strictly forbidden. The addresser shall not be liable for the content of such changed e-mails. ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] sf: ensure flash device is in 3-byte address mode
On 2017-10-07 09:23 AM, Prabhakar Kushwaha wrote: > Dear Jagan, Simon, > > > -Original Message- > > From: U-Boot [mailto:u-boot-boun...@lists.denx.de] On Behalf Of Jagan > > Teki > > Sent: Thursday, December 07, 2017 11:19 AM > > To: Goldschmidt Simon > > Cc: u-boot@lists.denx.de > > Subject: Re: [U-Boot] [PATCH] sf: ensure flash device is in 3-byte > > address mode > > > > On Tue, Dec 5, 2017 at 11:50 AM, Goldschmidt Simon > > wrote: > > > + Lukasz (as a reviewer of my patch[1]) > > > > > > On Mon, Dec 4, 2017 at 8:20, Jagan Teki wrote: > > >> This is the patch[1] for 4-byte addressing, but I would wonder how > > >> can > > proceed > > >> operations with 4-byte if we disable during probe. > > >> > > >> [1] > > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgit. > > denx > > .de%2F%3Fp%3Du-boot- > > &data=02%7C01%7Cprabhakar.kushwaha%40nxp.com%7Ca37e67c0f5fd431396 > > 5f08d53d3649b8%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6364 > > > 82225771650679&sdata=CBQkKDXTE1g1mvEbYuyiBApW2NTxQFCirGeJV9uzX8E > > %3D&reserved=0 > > >> spi.git;a=commitdiff;h=fd0c22a90772379c4c11ba09347d36cc8ee17dca > > > > > > OK, so your patch does something different than what I did. > > > > > > I was trying to keep the change to U-Boot as small as possible, only > > > fixing this issue I was seeing: > > > > > > After a soft-reboot where the SPI chip was not reset, it is left in > > > 4-byte addressing mode (linux uses this mode, obviously). Remember > > > that 4-byte mode is not a permanent setting, so we can enter and > > > leave it any time we like by issuing a command. > > > > > > U-Boot uses the Bank Address Register (BAR) for spi flash chips with > > > more than 16 MByte, so it impclitly assumes that the chip is in > > > 3-byte address mode. As I see it, your patch is worth a discussion > > > named "should we use 4-byte addressing mode on spi flash chips?". > > > I do think this is a better alternative than writing BAR! But this > > > change probably needs discussion and testing. > > > > OK, will review your patch. > > > > Other solution to this problem could have been "adding support of 4byte > addressing". > > There will always be a requirement of supporting >16MB flash. I would be very happy to have 4-byte addressing support. I just thought it would be better to first fix the soft-reboot issue I am having (and at least one other person on this list also had). Plus, I haven't seen a workign patch for 4-byte addressing on this list, yet. My patch has no side effects, works and could be merged for 2018.01. Thanks, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] fpga: allow programming fpga from FIT image for all FPGA drivers
On 5.12.2017 08:22, Michal Simek wrote: > > Which release will this be in? Does it fit into 2018.01 or has that > > window already closed? > > I believe so. Sorry to bother you again, I'm just not sure I understood your answer. Was that "I beleive so" meant as "yes, 2018.01 unless someone rejects it"? Because Tom just wrote patches already on the ML were OK for that release? Thanks, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] sf: ensure flash device is in 3-byte address mode
+ Lukasz (as a reviewer of my patch[1]) On Mon, Dec 4, 2017 at 8:20, Jagan Teki wrote: > This is the patch[1] for 4-byte addressing, but I would wonder how can proceed > operations with 4-byte if we disable during probe. > > [1] http://git.denx.de/?p=u-boot- > spi.git;a=commitdiff;h=fd0c22a90772379c4c11ba09347d36cc8ee17dca OK, so your patch does something different than what I did. I was trying to keep the change to U-Boot as small as possible, only fixing this issue I was seeing: After a soft-reboot where the SPI chip was not reset, it is left in 4-byte addressing mode (linux uses this mode, obviously). Remember that 4-byte mode is not a permanent setting, so we can enter and leave it any time we like by issuing a command. U-Boot uses the Bank Address Register (BAR) for spi flash chips with more than 16 MByte, so it impclitly assumes that the chip is in 3-byte address mode. As I see it, your patch is worth a discussion named "should we use 4-byte addressing mode on spi flash chips?". I do think this is a better alternative than writing BAR! But this change probably needs discussion and testing. Until we discussed and tested that, could we push my patch[1] into v2018.01? This is really a rather tiny bugfix I need for soft reboot, compared to using 4-byte address mode. [1] https://patchwork.ozlabs.org/patch/826919/ Thanks, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] fpga: allow programming fpga from FIT image for all FPGA drivers
Hi, On 4.12.2017 15:27, Michal Simek wrote: > [..] > ok. Then applied to my xilinx tree. Great to hear that, thanks! Which release will this be in? Does it fit into 2018.01 or has that window already closed? Thanks, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] sf: ensure flash device is in 3-byte address mode
Hi Jagan, On Fri, Nov 10, 2017 08:04, Jagan Teki wrote: >>> I've similar change on my patchwork, since no-one tested Will CC you by re- > basing it please have test? >> >> Yes, of course I'd like to test this. Where do I find your patch? > > Will rebase and send to ML soon. Any progress here? Any chance that this one and the other fixes needed for cadence_qspi to work correctly get included in 2018.01? The following patches would be required for this in addition to the 3-byte mode switch you wanted to submit: "spi: cadence_spi: Adopt Linux DT bindings": https://patchwork.ozlabs.org/project/uboot/list/?series=13864 Revert "spi: cadence_qspi_apb: Use 32 bit indirect read transaction when possible" (this one goes on top of the above) https://patchwork.ozlabs.org/patch/838871/ BTW: the series "spi: cadence_spi_apb: fix using bouncebuf with writeback dcache" can be closed when the reverting patch above is applied. I already sent reviewed-by and tested-by for the first series above but I can't see them in patchwork. Please let me know if there's anything missing or anything I can do to get this pushed. Thanks, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] fpga: allow programming fpga from FIT image for all FPGA drivers
On 28.11.2017 14:46, Michal Simek wrote: > On 28.11.2017 10:08, Goldschmidt Simon wrote: >> Simon Goldschmidt wrote: >>> Hi Simon, >>> >>> Simon Glass wrote: >>>> I see that, although it is adding to the fpga header so presumably >>>> making it harder for someone to move this over. >>> >>> Yes, I'm not happy with changing the header and even xilinx C file to >>> add functionality for altera. However, this is due to the fact that a >>> core file still depends on an dogs implementation, which I removed. >> >> This should have meant "on an fpga implementation". No dogs or offences here. >> My mobile phone does not know the word "fpga", obviously. >> >>> >>>> Does anyone on cc know the plan fr conversion of FPGA to driver model? >>>> It does not look too tricky from a quick look at the header file. >>> >>> It does not look tricky, indeed. However, we should try to match this >>> to the Linux implementation regarding what's needed in the device >>> tree. And I just don't know that part, yet ;-) > > I have not a problem to enable others fpgas to use this functionality. > And transition should be done for all these drivers when someone has a time > to do > it. Great, thanks! So via which tree would this be included? Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] fpga: allow programming fpga from FIT image for all FPGA drivers
Simon Goldschmidt wrote: > Hi Simon, > > Simon Glass wrote: > > I see that, although it is adding to the fpga header so presumably > > making it harder for someone to move this over. > > Yes, I'm not happy with changing the header and even xilinx C file to add > functionality for altera. However, this is due to the fact that a core file > still depends > on an dogs implementation, which I removed. This should have meant "on an fpga implementation". No dogs or offences here. My mobile phone does not know the word "fpga", obviously. > > > Does anyone on cc know the plan fr conversion of FPGA to driver model? > > It does not look too tricky from a quick look at the header file. > > It does not look tricky, indeed. However, we should try to match this to the > Linux > implementation regarding what's needed in the device tree. And I just don't > know > that part, yet ;-) > Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] fpga: allow programming fpga from FIT image for all FPGA drivers
Hi Simon, Simon Glass wrote: > I see that, although it is adding to the fpga header so presumably > making it harder for someone to move this over. Yes, I'm not happy with changing the header and even xilinx C file to add functionality for altera. However, this is due to the fact that a core file still depends on an dogs implementation, which I removed. > Does anyone on cc know the plan fr conversion of FPGA to driver model? > It does not look too tricky from a quick look at the header file. It does not look tricky, indeed. However, we should try to match this to the Linux implementation regarding what's needed in the device tree. And I just don't know that part, yet ;-) Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] spl: make CONFIG_OF_EMBED pass dts through fdtgrep
Hi Lukasz > > Building spl with CONFIG_OF_EMBED enabled results in an error message > ^^^ - is the CONFIG_OF_EMBED a standard > feature for SPL? Well, it's not really a standard feature I want to use in my final product. But if I want to debug SPL or U-Boot on my socfpga (using ARM's DS-5 for Altera), I need the device tree to be included in the binary that the debugger downloads to the target. That's what CONFIG_OF_EMBED is for, right? I even need this to debug U-Boot, as the standard procedure for my target is to first start SPL via the debugger, halt at a specific hardware breakpoint and the load U-Boot which I want to debug. > > on my board: "SPL image too big". This is because the fdtgrep build > > step is only executed for CONFIG_OF_SEPARATE. > > So the problem is with not enough runs of dtb stripping? No, the problem that the dtb is not stripped at all for CONFIG_OF_EMBED. The only thing that is stripped is 'spl/u-boot-spl.dtb' which is only created for CONFIG_OF_SEPARATE. > > > > > Fix this by moving the fdtgrep build step ('cmd_fdtgreo') from > > scripts/Makefile.spl to dts/Makefile so that the reduced dtb is > > available for all kinds of spl builds. > > Ok. > > > > > The resulting variable name for the embedded device tree blob changes, > > too, which is why common.h and fdtdec.c have tiny changes. > > > > Signed-off-by: Simon Goldschmidt > > --- > > dts/Makefile | 35 +++ > > include/common.h | 1 + > > lib/fdtdec.c | 4 > > scripts/Makefile.spl | 20 ++-- > > 4 files changed, 38 insertions(+), 22 deletions(-) > > > > diff --git a/dts/Makefile b/dts/Makefile index 3a93dafb51..c9b2a89441 > > 100644 > > --- a/dts/Makefile > > +++ b/dts/Makefile > > @@ -22,10 +22,29 @@ DTB := $(ARCH_PATH)/$(DEVICE_TREE).dtb > > dtb_depends += $(DTB:.dtb=.dts) endif > > > > +# Pass the original device tree file through fdtgrep twice. The > > first pass +# removes any unwanted nodes (i.e. those which don't have > > the +# 'u-boot,dm-pre-reloc' property and thus are not needed by SPL. > > The second +# pass removes various unused properties from the > > remaining nodes. +# The output is typically a much smaller device tree > > file. +ifeq ($(CONFIG_TPL_BUILD),y) > > +fdtgrep_props := -b u-boot,dm-pre-reloc -b u-boot,dm-tpl else > > +fdtgrep_props := -b u-boot,dm-pre-reloc -b u-boot,dm-spl endif > > +quiet_cmd_fdtgrep = FDTGREP $@ > > + cmd_fdtgrep = $(objtree)/tools/fdtgrep $(fdtgrep_props) -RT $< > > \ > > + -n /chosen -n /config -O dtb | \ > > + $(objtree)/tools/fdtgrep -r -O dtb - -o $@ \ > > + $(addprefix -P ,$(subst > > $\",,$(CONFIG_OF_SPL_REMOVE_PROPS))) + > > +$(obj)/dt-spl.dtb: $(DTB) $(objtree)/tools/fdtgrep FORCE > > + $(call if_changed,fdtgrep) > > + > > $(obj)/dt.dtb: $(DTB) FORCE > > $(call if_changed,shipped) > > > > -targets += dt.dtb > > +targets += dt.dtb dt-spl.dtb > > > > $(DTB): $(dtb_depends) > > ifeq ($(EXT_DTB),) > > @@ -42,14 +61,22 @@ endif > > arch-dtbs: > > $(Q)$(MAKE) $(build)=$(ARCH_PATH) dtbs > > > > -.SECONDARY: $(obj)/dt.dtb.S > > +.SECONDARY: $(obj)/dt.dtb.S $(obj)/dt-spl.dtb.S > > > > + > > +ifeq ($(CONFIG_SPL_BUILD),y) > > +obj-$(CONFIG_OF_EMBED) := dt-spl.dtb.o # support "out-of-tree" build > > +for dtb-spl > > +$(obj)/dt-spl.dtb.o: $(obj)/dt-spl.dtb.S FORCE > > + $(call if_changed_dep,as_o_S) > > +else > > obj-$(CONFIG_OF_EMBED) := dt.dtb.o > > +endif > > > > -dtbs: $(obj)/dt.dtb > > +dtbs: $(obj)/dt.dtb $(obj)/dt-spl.dtb > > @: > > > > -clean-files := dt.dtb.S > > +clean-files := dt.dtb.S dt-spl.dtb.S > > > > # Let clean descend into dts directories > > subdir- > > += ../arch/arm/dts ../arch/microblaze/dts ../arch/mips/dts > > +../arch/sandbox/dts ../arch/x86/dts > > diff --git a/include/common.h b/include/common.h index > > e14e1daa88..6e24545178 100644 --- a/include/common.h > > +++ b/include/common.h > > @@ -201,6 +201,7 @@ int last_stage_init(void); extern ulong > > monitor_flash_len; int mac_read_from_eeprom(void); > > extern u8 __dtb_dt_begin[];/* embedded device tree blob */ > > +extern u8 __dtb_dt_spl_begin[];/* embedded device tree blob > > for SPL/TPL */ int set_cpu_clk_info(void); int mdm_init(void); int > > print_cpuinfo(void); diff --git a/lib/fdtdec.c b/lib/fdtdec.c index > > 45f3fe7baf..0eb0b92261 100644 > > --- a/lib/fdtdec.c > > +++ b/lib/fdtdec.c > > @@ -1266,7 +1266,11 @@ int fdtdec_setup(void) # endif # ifdef > > CONFIG_OF_EMBED > > /* Get a pointer to the FDT */ > > +# ifdef CONFIG_SPL_BUILD > > + gd->fdt_blob = __dtb_dt_spl_begin; > > +# else > > gd->fdt_blob = __dtb_dt_begin; > > +# endif > > # elif defined CONFIG_OF_SEPARATE > > # ifdef CONFIG_SPL_BUILD > > /* FDT is at end of BSS unless it is in a different memory region */ > > diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl index > > 49b27ac926..d3c176d775 100644 > > --- a/
[U-Boot] [PATCH] spl: make CONFIG_OF_EMBED pass dts through fdtgrep
Building spl with CONFIG_OF_EMBED enabled results in an error message on my board: "SPL image too big". This is because the fdtgrep build step is only executed for CONFIG_OF_SEPARATE. Fix this by moving the fdtgrep build step ('cmd_fdtgreo') from scripts/Makefile.spl to dts/Makefile so that the reduced dtb is available for all kinds of spl builds. The resulting variable name for the embedded device tree blob changes, too, which is why common.h and fdtdec.c have tiny changes. Signed-off-by: Simon Goldschmidt --- dts/Makefile | 35 +++ include/common.h | 1 + lib/fdtdec.c | 4 scripts/Makefile.spl | 20 ++-- 4 files changed, 38 insertions(+), 22 deletions(-) diff --git a/dts/Makefile b/dts/Makefile index 3a93dafb51..c9b2a89441 100644 --- a/dts/Makefile +++ b/dts/Makefile @@ -22,10 +22,29 @@ DTB := $(ARCH_PATH)/$(DEVICE_TREE).dtb dtb_depends += $(DTB:.dtb=.dts) endif +# Pass the original device tree file through fdtgrep twice. The first pass +# removes any unwanted nodes (i.e. those which don't have the +# 'u-boot,dm-pre-reloc' property and thus are not needed by SPL. The second +# pass removes various unused properties from the remaining nodes. +# The output is typically a much smaller device tree file. +ifeq ($(CONFIG_TPL_BUILD),y) +fdtgrep_props := -b u-boot,dm-pre-reloc -b u-boot,dm-tpl +else +fdtgrep_props := -b u-boot,dm-pre-reloc -b u-boot,dm-spl +endif +quiet_cmd_fdtgrep = FDTGREP $@ + cmd_fdtgrep = $(objtree)/tools/fdtgrep $(fdtgrep_props) -RT $< \ + -n /chosen -n /config -O dtb | \ + $(objtree)/tools/fdtgrep -r -O dtb - -o $@ \ + $(addprefix -P ,$(subst $\",,$(CONFIG_OF_SPL_REMOVE_PROPS))) + +$(obj)/dt-spl.dtb: $(DTB) $(objtree)/tools/fdtgrep FORCE + $(call if_changed,fdtgrep) + $(obj)/dt.dtb: $(DTB) FORCE $(call if_changed,shipped) -targets += dt.dtb +targets += dt.dtb dt-spl.dtb $(DTB): $(dtb_depends) ifeq ($(EXT_DTB),) @@ -42,14 +61,22 @@ endif arch-dtbs: $(Q)$(MAKE) $(build)=$(ARCH_PATH) dtbs -.SECONDARY: $(obj)/dt.dtb.S +.SECONDARY: $(obj)/dt.dtb.S $(obj)/dt-spl.dtb.S + +ifeq ($(CONFIG_SPL_BUILD),y) +obj-$(CONFIG_OF_EMBED) := dt-spl.dtb.o +# support "out-of-tree" build for dtb-spl +$(obj)/dt-spl.dtb.o: $(obj)/dt-spl.dtb.S FORCE + $(call if_changed_dep,as_o_S) +else obj-$(CONFIG_OF_EMBED) := dt.dtb.o +endif -dtbs: $(obj)/dt.dtb +dtbs: $(obj)/dt.dtb $(obj)/dt-spl.dtb @: -clean-files := dt.dtb.S +clean-files := dt.dtb.S dt-spl.dtb.S # Let clean descend into dts directories subdir- += ../arch/arm/dts ../arch/microblaze/dts ../arch/mips/dts ../arch/sandbox/dts ../arch/x86/dts diff --git a/include/common.h b/include/common.h index e14e1daa88..6e24545178 100644 --- a/include/common.h +++ b/include/common.h @@ -201,6 +201,7 @@ int last_stage_init(void); extern ulong monitor_flash_len; int mac_read_from_eeprom(void); extern u8 __dtb_dt_begin[];/* embedded device tree blob */ +extern u8 __dtb_dt_spl_begin[];/* embedded device tree blob for SPL/TPL */ int set_cpu_clk_info(void); int mdm_init(void); int print_cpuinfo(void); diff --git a/lib/fdtdec.c b/lib/fdtdec.c index 45f3fe7baf..0eb0b92261 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -1266,7 +1266,11 @@ int fdtdec_setup(void) # endif # ifdef CONFIG_OF_EMBED /* Get a pointer to the FDT */ +# ifdef CONFIG_SPL_BUILD + gd->fdt_blob = __dtb_dt_spl_begin; +# else gd->fdt_blob = __dtb_dt_begin; +# endif # elif defined CONFIG_OF_SEPARATE # ifdef CONFIG_SPL_BUILD /* FDT is at end of BSS unless it is in a different memory region */ diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl index 49b27ac926..d3c176d775 100644 --- a/scripts/Makefile.spl +++ b/scripts/Makefile.spl @@ -238,24 +238,8 @@ $(obj)/$(SPL_BIN)-pad.bin: $(obj)/$(SPL_BIN) @bss_size_str=$(shell $(NM) $< | awk 'BEGIN {size = 0} /__bss_size/ {size = $$1} END {print "ibase=16; " toupper(size)}' | bc); \ dd if=/dev/zero of=$@ bs=1 count=$${bss_size_str} 2>/dev/null; -# Pass the original device tree file through fdtgrep twice. The first pass -# removes any unwanted nodes (i.e. those which don't have the -# 'u-boot,dm-pre-reloc' property and thus are not needed by SPL. The second -# pass removes various unused properties from the remaining nodes. -# The output is typically a much smaller device tree file. -ifeq ($(CONFIG_TPL_BUILD),y) -fdtgrep_props := -b u-boot,dm-pre-reloc -b u-boot,dm-tpl -else -fdtgrep_props := -b u-boot,dm-pre-reloc -b u-boot,dm-spl -endif -quiet_cmd_fdtgrep = FDTGREP $@ - cmd_fdtgrep = $(objtree)/tools/fdtgrep $(fdtgrep_props) -RT $< \ - -n /chosen -n /config -O dtb | \ - $(objtree)/tools/fdtgrep -r -O dtb - -o $@ \ - $(addprefix -P ,$(subst $\",,$(CONFIG_OF_SPL_REMOVE_PROPS))) - -$(obj)/$(SPL_BIN).dtb: dts/dt.dtb $(objtree)/tools/fdtgrep FORCE - $(call if_changed,fdtgrep) +$(ob
Re: [U-Boot] [PATCH] fpga: allow programming fpga from FIT image for all FPGA drivers
Hi, > Simon Glass wrote: > On 10 November 2017 at 07:17, Goldschmidt Simon fuchs.com> wrote: > > This drops the limit that fpga is only loaded from FIT images for Xilinx. > > This is done by moving the 'partial' check from 'common/image.c' to > > 'drivers/fpga/xilinx.c' (the only driver supporting partial images > > yet) and supplies a weak default implementation in 'drivers/fpga/fpga.c'. > > > > Signed-off-by: Simon Goldschmidt > > --- > > common/bootm.c| 2 +- > > common/image.c| 6 ++ > > drivers/fpga/fpga.c | 9 + > > drivers/fpga/xilinx.c | 13 + > > include/fpga.h| 1 + > > 5 files changed, 26 insertions(+), 5 deletions(-) > > > > I think the FPGA subsystem should move to driver model. I can understand the need for that. However, I don't have the expertise to do so, I guess. Also, I would have thought this requirement would be raised to someone actually adding/changing fpga code (like the recent Intel activities on this list). In contrast to this, I see my patch more like cleaning the code, moving Xilinx dependent code from 'common/image.c' to 'drivers/fpga/xilinx.c'. This cleanup in the common directory is rather independent of migrating the fpga subsystem to driver model. Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] Working with patchwork
Hi Lukasz, thanks for taking the time :-) > > as I'm quite new to U-Boot and its patch workflow: what do I have to > > do to send a reviewed-by or tested-by tag for a patch? I sent it as a > > reply to this list but I can't see them in patchwork. > > > > Did I miss something here? Or could it be a problem in the email > > format used? > > I can confirm that when I send reviewed-by/acked-by, those are added to > patchwork. > > Which mailing program do you use? Unfortunately, I'm using Outlook. It already took some time with our IT department to get it to send raw text messages. But that seemed to have worked, I thought (at least I could register to a kernel mailing list with majordomo). However, I can't really access the plain text of my own mails sent by Outlook, so I can only check what was sent by searching online list archives and at least this one has "anonymized" my email address: https://www.mail-archive.com/u-boot@lists.denx.de/msg269392.html I'm still glad to hear anything that could help me :-) Best regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] Working with patchwork
Hi, as I'm quite new to U-Boot and its patch workflow: what do I have to do to send a reviewed-by or tested-by tag for a patch? I sent it as a reply to this list but I can't see them in patchwork. Did I miss something here? Or could it be a problem in the email format used? Thanks, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH v2] Revert "spi: cadence_qspi_apb: Use 32 bit indirect read transaction when possible"
This reverts commit b63b46313ed29e9b0c36b3d6b9407f6eade40c8f. This commit changed cadence_qspi_apb to use bouncebuf.c, which invalidates the data cache after reading. This is meant for dma transfers only and breaks the cadence_qspi driver which copies via cpu only: data that is copied by the cpu is in cache only and the cache invalidation at the end throws away this data. Signed-off-by: Simon Goldschmidt --- Changes for v2: - Rebased on top of Jason's patchset v4 "Sync DT bindings with Linux" drivers/spi/cadence_qspi_apb.c | 22 ++ 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c index 8309ab8794..c7cb33aa8f 100644 --- a/drivers/spi/cadence_qspi_apb.c +++ b/drivers/spi/cadence_qspi_apb.c @@ -627,8 +627,6 @@ int cadence_qspi_apb_indirect_read_execute(struct cadence_spi_platdata *plat, { unsigned int remaining = n_rx; unsigned int bytes_to_read = 0; - struct bounce_buffer bb; - u8 *bb_rxbuf; int ret; writel(n_rx, plat->regbase + CQSPI_REG_INDIRECTRDBYTES); @@ -637,11 +635,6 @@ int cadence_qspi_apb_indirect_read_execute(struct cadence_spi_platdata *plat, writel(CQSPI_REG_INDIRECTRD_START, plat->regbase + CQSPI_REG_INDIRECTRD); - ret = bounce_buffer_start(&bb, (void *)rxbuf, n_rx, GEN_BB_WRITE); - if (ret) - return ret; - bb_rxbuf = bb.bounce_buffer; - while (remaining > 0) { ret = cadence_qspi_wait_for_data(plat); if (ret < 0) { @@ -655,13 +648,12 @@ int cadence_qspi_apb_indirect_read_execute(struct cadence_spi_platdata *plat, bytes_to_read *= plat->fifo_width; bytes_to_read = bytes_to_read > remaining ? remaining : bytes_to_read; - readsl(plat->ahbbase, bb_rxbuf, bytes_to_read >> 2); - if (bytes_to_read % 4) - readsb(plat->ahbbase, - bb_rxbuf + rounddown(bytes_to_read, 4), - bytes_to_read % 4); - - bb_rxbuf += bytes_to_read; + /* Handle non-4-byte aligned access to avoid data abort. */ + if (((uintptr_t)rxbuf % 4) || (bytes_to_read % 4)) + readsb(plat->ahbbase, rxbuf, bytes_to_read); + else + readsl(plat->ahbbase, rxbuf, bytes_to_read >> 2); + rxbuf += bytes_to_read; remaining -= bytes_to_read; bytes_to_read = cadence_qspi_get_rd_sram_level(plat); } @@ -678,7 +670,6 @@ int cadence_qspi_apb_indirect_read_execute(struct cadence_spi_platdata *plat, /* Clear indirect completion status */ writel(CQSPI_REG_INDIRECTRD_DONE, plat->regbase + CQSPI_REG_INDIRECTRD); - bounce_buffer_stop(&bb); return 0; @@ -686,7 +677,6 @@ failrd: /* Cancel the indirect read */ writel(CQSPI_REG_INDIRECTRD_CANCEL, plat->regbase + CQSPI_REG_INDIRECTRD); - bounce_buffer_stop(&bb); return ret; } -- 2.12.2.windows.2 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v4 3/5] config: cadence_spi: Remove defines read from DT
> Jason Rush wrote: > Cleanup unused #define values that are read from the DT. > --- > Changes for v4: >- Rebased Reviewed-by: Simon Goldschmidt Tested on a socfpga-cyclonev board: Tested-by: Simon Goldschmidt Best regards, Simon > > include/configs/k2g_evm.h| 1 - > include/configs/socfpga_common.h | 1 - > include/configs/stv0991.h| 1 - > 3 files changed, 3 deletions(-) > > diff --git a/include/configs/k2g_evm.h b/include/configs/k2g_evm.h > index df81c09..535e712 100644 > --- a/include/configs/k2g_evm.h > +++ b/include/configs/k2g_evm.h > @@ -93,7 +93,6 @@ > #ifndef CONFIG_SPL_BUILD > #define CONFIG_CADENCE_QSPI > #define CONFIG_CQSPI_REF_CLK 38400 > -#define CONFIG_CQSPI_DECODER 0x0 > #define CONFIG_BOUNCE_BUFFER > #endif > > diff --git a/include/configs/socfpga_common.h > b/include/configs/socfpga_common.h > index 8a7debb..e5e2f83 100644 > --- a/include/configs/socfpga_common.h > +++ b/include/configs/socfpga_common.h > @@ -185,7 +185,6 @@ unsigned int cm_get_l4_sp_clk_hz(void); > unsigned int cm_get_qspi_controller_clk_hz(void); > #define CONFIG_CQSPI_REF_CLK cm_get_qspi_controller_clk_hz() > #endif > -#define CONFIG_CQSPI_DECODER 0 > #define CONFIG_BOUNCE_BUFFER > > /* > diff --git a/include/configs/stv0991.h b/include/configs/stv0991.h > index c99fb67..fd96979 100644 > --- a/include/configs/stv0991.h > +++ b/include/configs/stv0991.h > @@ -63,7 +63,6 @@ > + * QSPI support > + */ > #ifdef CONFIG_OF_CONTROL /* QSPI is controlled via DT */ > -#define CONFIG_CQSPI_DECODER 0 > #define CONFIG_CQSPI_REF_CLK ((30/4)/2)*1000*1000 > #define CONFIG_BOUNCE_BUFFER > > -- > 2.1.4 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v4 2/5] dts: cadence_spi: Sync DT bindings with Linux
> Jason Rush wrote: > Adopt the Linux DT bindings and clean-up duplicate > and unused values. > --- > Changes for v4: >- Rebased Reviewed-by: Simon Goldschmidt > > arch/arm/dts/keystone-k2g-evm.dts | 8 > arch/arm/dts/keystone-k2g.dtsi| 5 +++-- > arch/arm/dts/socfpga.dtsi | 5 +++-- > arch/arm/dts/socfpga_arria10.dtsi | 4 ++-- > arch/arm/dts/socfpga_arria5_socdk.dts | 9 - > arch/arm/dts/socfpga_cyclone5_is1.dts | 9 - > arch/arm/dts/socfpga_cyclone5_socdk.dts | 9 - > arch/arm/dts/socfpga_cyclone5_sockit.dts | 9 - > arch/arm/dts/socfpga_cyclone5_socrates.dts| 9 - > arch/arm/dts/socfpga_cyclone5_sr1500.dts | 9 - > arch/arm/dts/socfpga_cyclone5_vining_fpga.dts | 18 -- > arch/arm/dts/stv0991.dts | 12 +++- > 12 files changed, 51 insertions(+), 55 deletions(-) > > diff --git a/arch/arm/dts/keystone-k2g-evm.dts b/arch/arm/dts/keystone-k2g- > evm.dts > index de208b3..ba566a4 100644 > --- a/arch/arm/dts/keystone-k2g-evm.dts > +++ b/arch/arm/dts/keystone-k2g-evm.dts > @@ -76,10 +76,10 @@ > spi-max-frequency = <9600>; > #address-cells = <1>; > #size-cells = <1>; > -tshsl-ns = <392>; > -tsd2d-ns = <392>; > -tchsh-ns = <100>; > -tslch-ns = <100>; > +cdns,tshsl-ns = <392>; > +cdns,tsd2d-ns = <392>; > +cdns,tchsh-ns = <100>; > +cdns,tslch-ns = <100>; > block-size = <18>; > > > diff --git a/arch/arm/dts/keystone-k2g.dtsi b/arch/arm/dts/keystone-k2g.dtsi > index 7b2fae6..9bcfea6 100644 > --- a/arch/arm/dts/keystone-k2g.dtsi > +++ b/arch/arm/dts/keystone-k2g.dtsi > @@ -92,8 +92,9 @@ > <0x2400 0x400>; > interrupts = ; > num-cs = <4>; > - fifo-depth = <256>; > - sram-size = <256>; > + cdns,fifo-depth = <256>; > + cdns,fifo-width = <4>; > + cdns,trigger-address = <0x2400>; > status = "disabled"; > }; > > diff --git a/arch/arm/dts/socfpga.dtsi b/arch/arm/dts/socfpga.dtsi > index 8588221..7557aa0 100644 > --- a/arch/arm/dts/socfpga.dtsi > +++ b/arch/arm/dts/socfpga.dtsi > @@ -644,8 +644,9 @@ > clocks = <&qspi_clk>; > ext-decoder = <0>; /* external decoder */ > num-cs = <4>; > - fifo-depth = <128>; > - sram-size = <128>; > + cdns,fifo-depth = <128>; > + cdns,fifo-width = <4>; > + cdns,trigger-address = <0x>; > bus-num = <2>; > status = "disabled"; > }; > diff --git a/arch/arm/dts/socfpga_arria10.dtsi > b/arch/arm/dts/socfpga_arria10.dtsi > index 377700d..abfd0bc 100644 > --- a/arch/arm/dts/socfpga_arria10.dtsi > +++ b/arch/arm/dts/socfpga_arria10.dtsi > @@ -734,8 +734,8 @@ > clocks = <&l4_main_clk>; > ext-decoder = <0>; /* external decoder */ > num-chipselect = <4>; > - fifo-depth = <128>; > - sram-size = <512>; > + cdns,fifo-depth = <128>; > + cdns,fifo-width = <4>; > bus-num = <2>; > status = "disabled"; > }; > diff --git a/arch/arm/dts/socfpga_arria5_socdk.dts > b/arch/arm/dts/socfpga_arria5_socdk.dts > index 7265058..1e91a65 100644 > --- a/arch/arm/dts/socfpga_arria5_socdk.dts > +++ b/arch/arm/dts/socfpga_arria5_socdk.dts > @@ -94,10 +94,9 @@ > m25p,fast-read; > page-size = <256>; > block-size = <16>; /* 2^16, 64KB */ > - read-delay = <4>; /* delay value in read data capture register > */ > - tshsl-ns = <50>; > - tsd2d-ns = <50>; > - tchsh-ns = <4>; > - tslch-ns = <4>; > + cdns,tshsl-ns = <50>; > + cdns,tsd2d-ns = <50>; > + cdns,tchsh-ns = <4>; > + cdns,tslch-ns = <4>; > }; > }; > diff --git a/arch/arm/dts/socfpga_cyclone5_is1.dts > b/arch/arm/dts/socfpga_cyclone5_is1.dts > index 16a3283..2e2b71f 100644 > --- a/arch/arm/dts/socfpga_cyclone5_is1.dts > +++ b/arch/arm/dts/socfpga_cyclone5_is1.dts > @@ -93,11 +93,10 @@ > m25p,fast-read; > page-size = <256>; > block-size = <16>; /* 2^16, 64KB */ > - read-delay = <4>; /* delay value in read data capture register > */ > - tshsl-ns = <50>; > - tsd2d-ns = <50>; > - tchsh-ns
Re: [U-Boot] [PATCH v4 1/5] spi: cadence_spi: Sync DT bindings with Linux
> Jason Rush wrote: > Adopt the Linux DT bindings. This also fixes an issue > with the indaddrtrig register on the Cadence QSPI > device being programmed with the wrong value for the > socfpga arch. > --- > Changes for v4: >- Rebased > Reviewed-by: Simon Goldschmidt Tested on a socfpga-cyclonev board: Tested-by: Simon Goldschmidt Best regards, Simon > drivers/spi/cadence_qspi.c | 20 > drivers/spi/cadence_qspi.h | 6 +- > drivers/spi/cadence_qspi_apb.c | 15 --- > 3 files changed, 21 insertions(+), 20 deletions(-) > > diff --git a/drivers/spi/cadence_qspi.c b/drivers/spi/cadence_qspi.c > index 9a6e41f..991a716 100644 > --- a/drivers/spi/cadence_qspi.c > +++ b/drivers/spi/cadence_qspi.c > @@ -212,7 +212,7 @@ static int cadence_spi_xfer(struct udevice *dev, unsigned > int bitlen, > > /* Set Chip select */ > cadence_qspi_apb_chipselect(base, spi_chip_select(dev), > - CONFIG_CQSPI_DECODER); > + plat->is_decoded_cs); > > if ((flags & SPI_XFER_END) || (flags == 0)) { > if (priv->cmd_len == 0) { > @@ -296,7 +296,11 @@ static int cadence_spi_ofdata_to_platdata(struct udevice > *bus) > > plat->regbase = (void *)data[0]; > plat->ahbbase = (void *)data[2]; > - plat->sram_size = fdtdec_get_int(blob, node, "sram-size", 128); > + plat->is_decoded_cs = fdtdec_get_bool(blob, node, "cdns,is-decoded- > cs"); > + plat->fifo_depth = fdtdec_get_uint(blob, node, "cdns,fifo-depth", 128); > + plat->fifo_width = fdtdec_get_uint(blob, node, "cdns,fifo-width", 4); > + plat->trigger_address = fdtdec_get_uint(blob, node, > + "cdns,trigger-address", 0); > > /* All other paramters are embedded in the child node */ > subnode = fdt_first_subnode(blob, node); > @@ -310,12 +314,12 @@ static int cadence_spi_ofdata_to_platdata(struct > udevice *bus) > 50); > > /* Read other parameters from DT */ > - plat->page_size = fdtdec_get_int(blob, subnode, "page-size", 256); > - plat->block_size = fdtdec_get_int(blob, subnode, "block-size", 16); > - plat->tshsl_ns = fdtdec_get_int(blob, subnode, "tshsl-ns", 200); > - plat->tsd2d_ns = fdtdec_get_int(blob, subnode, "tsd2d-ns", 255); > - plat->tchsh_ns = fdtdec_get_int(blob, subnode, "tchsh-ns", 20); > - plat->tslch_ns = fdtdec_get_int(blob, subnode, "tslch-ns", 20); > + plat->page_size = fdtdec_get_uint(blob, subnode, "page-size", 256); > + plat->block_size = fdtdec_get_uint(blob, subnode, "block-size", 16); > + plat->tshsl_ns = fdtdec_get_uint(blob, subnode, "cdns,tshsl-ns", 200); > + plat->tsd2d_ns = fdtdec_get_uint(blob, subnode, "cdns,tsd2d-ns", 255); > + plat->tchsh_ns = fdtdec_get_uint(blob, subnode, "cdns,tchsh-ns", 20); > + plat->tslch_ns = fdtdec_get_uint(blob, subnode, "cdns,tslch-ns", 20); > > debug("%s: regbase=%p ahbbase=%p max-frequency=%d page- > size=%d\n", > __func__, plat->regbase, plat->ahbbase, plat->max_hz, > diff --git a/drivers/spi/cadence_qspi.h b/drivers/spi/cadence_qspi.h > index d1927a4..8315421 100644 > --- a/drivers/spi/cadence_qspi.h > +++ b/drivers/spi/cadence_qspi.h > @@ -18,14 +18,18 @@ struct cadence_spi_platdata { > unsigned intmax_hz; > void*regbase; > void*ahbbase; > + boolis_decoded_cs; > + u32 fifo_depth; > + u32 fifo_width; > + u32 trigger_address; > > + // Flash parameters > u32 page_size; > u32 block_size; > u32 tshsl_ns; > u32 tsd2d_ns; > u32 tchsh_ns; > u32 tslch_ns; > - u32 sram_size; > }; > > struct cadence_spi_priv { > diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c > index e02f221..8309ab8 100644 > --- a/drivers/spi/cadence_qspi_apb.c > +++ b/drivers/spi/cadence_qspi_apb.c > @@ -37,10 +37,6 @@ > #define CQSPI_REG_RETRY 1 > #define CQSPI_POLL_IDLE_RETRY3 > > -#define CQSPI_FIFO_WIDTH 4 > - > -#define CQSPI_REG_SRAM_THRESHOLD_WORDS 50 > - > /* Transfer mode */ > #define CQSPI_INST_TYPE_SINGLE 0 > #define CQSPI_INST_TYPE_DUAL 1 > @@ -51,9 +47,6 @@ > #define CQSPI_DUMMY_CLKS_PER_BYTE8 > #define CQSPI_DUMMY_BYTES_MAX4 > > -#define CQSPI_REG_SRAM_FILL_THRESHOLD\ > - ((CQSPI_REG_SRAM_SIZE_WORD / 2) * CQSPI_FIFO_WIDTH) > - > / > * Controller's configuration and status register (offset from QSPI_BASE) > > / > @@ -400,7 +393,7 @@ void caden
[U-Boot] [PATCH] Revert "spi: cadence_qspi_apb: Use 32 bit indirect read transaction when possible"
This reverts commit b63b46313ed29e9b0c36b3d6b9407f6eade40c8f. This commit changed cadence_qspi_apb to use bouncebuf.c, which invalidates the data cache after reading. This is meant for dma transfers only and breaks the cadence_qspi driver which copies via cpu only: data that is copied by the cpu is in cache only and the cache invalidation at the end throws away this data. Signed-off-by: Simon Goldschmidt --- drivers/spi/cadence_qspi_apb.c | 22 ++ 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c index e02f2217f4..5d5b6f0d35 100644 --- a/drivers/spi/cadence_qspi_apb.c +++ b/drivers/spi/cadence_qspi_apb.c @@ -634,8 +634,6 @@ int cadence_qspi_apb_indirect_read_execute(struct cadence_spi_platdata *plat, { unsigned int remaining = n_rx; unsigned int bytes_to_read = 0; - struct bounce_buffer bb; - u8 *bb_rxbuf; int ret; writel(n_rx, plat->regbase + CQSPI_REG_INDIRECTRDBYTES); @@ -644,11 +642,6 @@ int cadence_qspi_apb_indirect_read_execute(struct cadence_spi_platdata *plat, writel(CQSPI_REG_INDIRECTRD_START, plat->regbase + CQSPI_REG_INDIRECTRD); - ret = bounce_buffer_start(&bb, (void *)rxbuf, n_rx, GEN_BB_WRITE); - if (ret) - return ret; - bb_rxbuf = bb.bounce_buffer; - while (remaining > 0) { ret = cadence_qspi_wait_for_data(plat); if (ret < 0) { @@ -662,13 +655,12 @@ int cadence_qspi_apb_indirect_read_execute(struct cadence_spi_platdata *plat, bytes_to_read *= CQSPI_FIFO_WIDTH; bytes_to_read = bytes_to_read > remaining ? remaining : bytes_to_read; - readsl(plat->ahbbase, bb_rxbuf, bytes_to_read >> 2); - if (bytes_to_read % 4) - readsb(plat->ahbbase, - bb_rxbuf + rounddown(bytes_to_read, 4), - bytes_to_read % 4); - - bb_rxbuf += bytes_to_read; + /* Handle non-4-byte aligned access to avoid data abort. */ + if (((uintptr_t)rxbuf % 4) || (bytes_to_read % 4)) + readsb(plat->ahbbase, rxbuf, bytes_to_read); + else + readsl(plat->ahbbase, rxbuf, bytes_to_read >> 2); + rxbuf += bytes_to_read; remaining -= bytes_to_read; bytes_to_read = cadence_qspi_get_rd_sram_level(plat); } @@ -685,7 +677,6 @@ int cadence_qspi_apb_indirect_read_execute(struct cadence_spi_platdata *plat, /* Clear indirect completion status */ writel(CQSPI_REG_INDIRECTRD_DONE, plat->regbase + CQSPI_REG_INDIRECTRD); - bounce_buffer_stop(&bb); return 0; @@ -693,7 +684,6 @@ failrd: /* Cancel the indirect read */ writel(CQSPI_REG_INDIRECTRD_CANCEL, plat->regbase + CQSPI_REG_INDIRECTRD); - bounce_buffer_stop(&bb); return ret; } -- 2.12.2.windows.2 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] common: make bouncebuf work for non-DMA transfers
Hi Vignesh, Vignesh R wrote: > [..] > Its not actually unaligned access, cadence QSPI IP on TI platforms do > not support non-byte accesses except for the last word. As per the TRM: > "The external master is only permitted to issue 32-bit data interface > writes until the last word of an indirect > transfer" OK, I thought it was unaligned access because of reading your commit message talking about data aborts. But this was in the write part indeed. >> So given my explanation above, what's the preferred way to fix this? > > Sorry, I overlooked the fact that bounce_buffer_stop() is calling > invalidate_dcache_range(). Somehow, this did not show problems on my > platform although its a writeback cache. > > I would suggest to revert commit b63b46313 ("spi: cadence_qspi_apb: Use > 32 bit indirect read transaction when possible"). I have seen that non > 32 bit accesses cause problems only while writing to flash but not > during read operations although the TRM states that both reads and > writes are affected. > But, since reverting b63b46313 as such does not break TI platforms, I > would prefer sending a revert. > Meanwhile, I will work on a better patch later. I have not actually tested writing with dcache enabled, but from reading the code, it should not be a problem. I'll test that then. > Lets not touch bouncebuf for now. OK. I take it that bouncebuf should be only used for dma transfers. In that case, shouldn't we revert the write patch, too, eventually? Even if it does not break data consistency like in the read direction, messing around with the cache while doing cpu transfers at the very least will drop performance. Plus it seems like a bad example. We can do that later, of course, as it should work like it is now. >> I need this driver fixed, so whatever way will be accepted is fine by >> me, I guess. Just let me know. >> > > Sorry for the trouble. I am okay with reverting the patch affecting read > path. OK. I'll send a patch hat reverts the read path then. Thank you for your help! Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] common: make bouncebuf work for non-DMA transfers
Simon Goldschmidt wrote: >Marek Vasut wrote: >> So what alignment problems do you observe ? If you copy using the CPU >> only, why do you need the bounce buffer at all ? I don't quite get it. > > Sorry for not explaining it good enough: > I don't observe any alignment problems. mach-socfpga can do unaligned > accesses as well. This driver does CPU based copy, but since it transfers > words, not bytes, I guess on other platforms, the CPU might fail when trying > to read a word from an unaligned pointer. > > Vignesh added this about a year ago and from that commit message, it > seems like he was observing alignment errors on his TI platform. > Also, those commits have a reviewed-by tag from you and Jagan. > > For me, reverting these patches would be OK as well, but I guess Vinesh's > TI platform kind of needs them? So to clarify it again, the cadence_spi driver has to transfer 32-bit words to/from the hardware and the driver uses 'readsl' and 'writesl' to do this. Now if the source/destination buffer is not aligned, this results in an error on platforms that do not support unaligned access. We have three options here: a) fix all call stacks calling dm_spi_ops->xfer to use aligned buffers (which could be hard when looking at 'sf read') b) fix the driver by doing malloc + memcpy or loading byte by byte from ram c) add framework functions doing malloc + memcpy (which is nearly the same as bouncebuf) This 32-bit spi transfer mode does not seem to be used too often, all other drivers I looked at are transferring byte by byte and thus can not be used as an example. Additionally, the TI platform Vignesh used obviously does not support unaligned access (which is why he added using bouncebuf here although no dma is used) while mach-socfpga supports unaligned accesses by default. So given my explanation above, what's the preferred way to fix this? I thought a framework solution would be better, which is why I modified bouncebuf to work with this, but as cadence_qspi is the only driver using bouncebuf in this fashion for now, I'm open for suggestions. I need this driver fixed, so whatever way will be accepted is fine by me, I guess. Just let me know. Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] cadence_qspi_apb: cache issues on mach-socfpga
Marek Vasut wrote: >>> I don't believe the patchset I submitted for DT bindings were merged in. >> >> I can confirm that. I'd strongly vote for them to get in as cadence_qspi >> is otherwise not usable on mach socfpga. >> >> How can I ensure a tested-by from me gets related to that patch set? >> I can't reply to it as I was not subscribed to the list at that time. > > You should rebase that patch anyway and then resend it. I could do that work but I guess it would be up to Jason to do that, regarding authorship? Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] common: make bouncebuf work for non-DMA transfers
Marek Vasut wrote: > So what alignment problems do you observe ? If you copy using the CPU > only, why do you need the bounce buffer at all ? I don't quite get it. Sorry for not explaining it good enough: I don't observe any alignment problems. mach-socfpga can do unaligned accesses as well. This driver does CPU based copy, but since it transfers words, not bytes, I guess on other platforms, the CPU might fail when trying to read a word from an unaligned pointer. Vignesh added this about a year ago and from that commit message, it seems like he was observing alignment errors on his TI platform. Also, those commits have a reviewed-by tag from you and Jagan. For me, reverting these patches would be OK as well, but I guess Vinesh's TI platform kind of needs them? Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 1/2] common: make bouncebuf work for non-DMA transfers
Marek Vasut wrote: > Why don't you just fix the cache operations in the driver ? This driver is copying CPU only. There are no cache operations involved! Vignesh added the bounce buffer obviously to fix alignment issues on his platform. > This bounce-buffer for only CPU operations is just wasting CPU cycles > and degrades performance and I just cannot find a good reason for it. I only thought using the bounce buffer here would be better than to add memcpy/memmove for alignment reasons in the driver (in terms of possible code duplication). I can of course implement this in a different way. Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] cadence_qspi_apb: cache issues on mach-socfpga
Rush, Jason A wrote;: > I don't believe the patchset I submitted for DT bindings were merged in. I can confirm that. I'd strongly vote for them to get in as cadence_qspi is otherwise not usable on mach socfpga. How can I ensure a tested-by from me gets related to that patch set? I can't reply to it as I was not subscribed to the list at that time. >> Regarding the bounce buffer change: isn't it wrong to use the generic bounce >> buffer here? Given the dcache calls in it, it seems like this one is for DMA >> but we're doing cpu copy in the cadence qspi driver. > > I tend to agree for the socfpga architecture; however, I'm not very familiar > with > the bounce buffer and dcache. From what I recall, it looked like the data is > copied > from the QSPI by the CPU on the socfpga so the data itself might be in the > dcache? > Then the call to bounce_buffer_stop invalidates the dcache, so the QSPI data > that > was there is now invalidated, and whatever random data in higher level memory > is copied over the QSPI data. Right. That's what my results were, too. I only discovered this after starting this thread and sent a patch later today Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH 1/2] common: make bouncebuf work for non-DMA transfers
Bounce buffer may be used for CPU-only transfers (this is currently the case for cadence_qspi). However, in this case, invalidating the data cache might throw away copied data that is still in the cache only. To make CPU-only transfers work with bouncebuf (but still take advantage of having aligned buffers), a new flag 'GEN_BB_NODMA' is introduced. If this flag is set, cache flushing/invalidation is skipped and only the alignment part of bouncebuf is active. Signed-off-by: Simon Goldschmidt --- common/bouncebuf.c | 9 + include/bouncebuf.h | 9 + 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/common/bouncebuf.c b/common/bouncebuf.c index 054d9e0302..0d18477f13 100644 --- a/common/bouncebuf.c +++ b/common/bouncebuf.c @@ -55,16 +55,17 @@ int bounce_buffer_start(struct bounce_buffer *state, void *data, * Flush data to RAM so DMA reads can pick it up, * and any CPU writebacks don't race with DMA writes */ - flush_dcache_range((unsigned long)state->bounce_buffer, - (unsigned long)(state->bounce_buffer) + - state->len_aligned); + if (!(state->flags & GEN_BB_NODMA)) + flush_dcache_range((unsigned long)state->bounce_buffer, + (unsigned long)(state->bounce_buffer) + + state->len_aligned); return 0; } int bounce_buffer_stop(struct bounce_buffer *state) { - if (state->flags & GEN_BB_WRITE) { + if ((state->flags & (GEN_BB_WRITE|GEN_BB_NODMA)) == GEN_BB_WRITE) { /* Invalidate cache so that CPU can see any newly DMA'd data */ invalidate_dcache_range((unsigned long)state->bounce_buffer, (unsigned long)(state->bounce_buffer) + diff --git a/include/bouncebuf.h b/include/bouncebuf.h index 5ffa99bc13..c6720b3b2e 100644 --- a/include/bouncebuf.h +++ b/include/bouncebuf.h @@ -37,6 +37,15 @@ */ #define GEN_BB_RW (GEN_BB_READ | GEN_BB_WRITE) +/* + * GEN_BB_NODMA -- Data is read/written by CPU only (no DMA), so no cache + * flushing and invalidating is required. Not passing this for GEN_BB_WRITE + * transfers done by the CPU (not DMA) may result in invalid data as the data + * written into the cache is lost by invalidating the dcache after the transfer + * (unless a writethrough cache is used). + */ +#define GEN_BB_NODMA (1 << 2) + struct bounce_buffer { /* Copy of data parameter passed to start() */ void *user_buffer; -- 2.12.2.windows.2 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH 2/2] spi: cadence_spi_apb: fix using bouncebuf with writeback dcache
The last two commits on this file have added bounce buffer handling to indirect read and write transfers. However, these are cpu-only transfers and bouncebuf seems to be written for dma transfers only (it invalidates the dcache in bouncebuf_stop, which throws away data copied by the cpu that are still in cache only). The last two commits resulted in reading random data on mach-socfpga. By using the new flag GEN_BB_NODMA, cadence_qspi is fixed on that platform (although the 'Sync DT bindings with Linux' patches from Jason Rush are still needed to make it work). Signed-off-by: Simon Goldschmidt --- drivers/spi/cadence_qspi_apb.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/spi/cadence_qspi_apb.c b/drivers/spi/cadence_qspi_apb.c index 7d335519b0..a3e1c84758 100644 --- a/drivers/spi/cadence_qspi_apb.c +++ b/drivers/spi/cadence_qspi_apb.c @@ -645,7 +645,7 @@ int cadence_qspi_apb_indirect_read_execute(struct cadence_spi_platdata *plat, writel(CQSPI_REG_INDIRECTRD_START, plat->regbase + CQSPI_REG_INDIRECTRD); - ret = bounce_buffer_start(&bb, (void *)rxbuf, n_rx, GEN_BB_WRITE); + ret = bounce_buffer_start(&bb, (void *)rxbuf, n_rx, GEN_BB_WRITE|GEN_BB_NODMA); if (ret) return ret; bb_rxbuf = bb.bounce_buffer; @@ -743,7 +743,7 @@ int cadence_qspi_apb_indirect_write_execute(struct cadence_spi_platdata *plat, * Handle non-4-byte aligned accesses via bounce buffer to * avoid data abort. */ - ret = bounce_buffer_start(&bb, (void *)txbuf, n_tx, GEN_BB_READ); + ret = bounce_buffer_start(&bb, (void *)txbuf, n_tx, GEN_BB_READ|GEN_BB_NODMA); if (ret) return ret; bb_txbuf = bb.bounce_buffer; -- 2.12.2.windows.2 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH 0/2] spi: cadence_spi_apb: fix using bouncebuf with writeback dcache
Currently, cadence_spi_apb is broken at least on mach-socfpga: Commits 57897c13de03ac0136d64641a3eab526c6810387 and b63b46313ed29e9b0c36b3d6b9407f6eade40c8f added bounce buffer handling to cadence_qspi_apb. This is the first usage of bounce buffers for non-DMA transfer. As it turns out, bounce buffers like they were did not work with writeback data cache. Since the TI K2G SoC seems to need aligned buffers, I chose to make it work with bounce buffers by adding a new flag that tells bouncebuf to not issue dcach commands (instead of reverting the above commits). To make cadence_qspi_apb work on mach-socfpga, we still need the 'Sync DT bindings with Linux' patches from Jason Rush, but this patch is one of the required things we need to get qspi support working again on mach-socfpga. ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] spi: cadence_qspi_apb: cache issues on mach-socfpga
Hi, I ran into the same issue with the cadence qspi driver and dcache as Jason reported (in febuary, I think - I started to monitor U-Boot in july only). May I ask what's the status here? I do need fixes for this to keep mach-socfpga running with qspi. I currently set dcache off globally and it works, but I still need the trigger-address fix (which is fixed by Jason's patch to use linux DT bindings). Regarding the bounce buffer change: isn't it wrong to use the generic bounce buffer here? Given the dcache calls in it, it seems like this one is for DMA but we're doing cpu copy in the cadence qspi driver. Anything I can do to get this pushed? Thanks, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v2] spi: kirkwood: avoid divide by zero crash
On Mon, Nov 13, 2017 at 06:26 AM, Baruch Siach wrote: >On Sun, Nov 12, 2017 at 08:42:28PM +0100, Heinrich Schuchardt wrote: >> On 11/12/2017 04:30 PM, Baruch Siach wrote: >> > Calling .set_speed with zero speed is definitely a bug. Instead of >> > crashing, set hz to 1 so that we get the lowest possible frequency rate. >> >> Did this actually occur? Why? > > Yes. CONFIG_ARCH_MVEBU selects CONFIG_DM_SPI_FLASH. In > cmd/sf.c:do_spi_flash_probe(), > speed is set to 0 when CONFIG_DM_SPI_FLASH, passed to > spi_flash_probe_bus_cs(), > and from there to spi_get_bus_and_cs(). > Unfortunately, since the ClearFog SPI flash node has no "spi-flash" compatible > string, the spi_flash_std driver is not bound, so spi_child_post_bind() > returns > early without calling spi_slave_ofdata_to_platdata(). The > dm_spi_slave_platdata > speed field is left uninitialized, and we end up with > speed=0 when calling spi_set_speed_mode(). > > This should be fixed with http://patchwork.ozlabs.org/patch/837360/ for > ClearFog, > by adding the "spi-flash" compatible. But "spi-flash" is non standard. Most > kernel > DT files use "jedec,spi-nor" instead. So you can expect this issue to show up > again in the future. This is exactly what I just got with the cadence-spi driver when starting my mach-socfpga board with the device tree working for linux. I also 'fixed' it by changing the dts, but I'd rather use the same dts for linux and U-Boot. Should this be fixed in the core code or is it a bug of each driver not detecting the '0'? > A workaround is to provide speed argument in the 'sf probe' command line. This is not a valid workaround for me: the spi-nor is my boot device and I got this error even in SPL when loading U-Boot. Not using 'sf probe'. Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH] fpga: allow programming fpga from FIT image for all FPGA drivers
This drops the limit that fpga is only loaded from FIT images for Xilinx. This is done by moving the 'partial' check from 'common/image.c' to 'drivers/fpga/xilinx.c' (the only driver supporting partial images yet) and supplies a weak default implementation in 'drivers/fpga/fpga.c'. Signed-off-by: Simon Goldschmidt --- common/bootm.c| 2 +- common/image.c| 6 ++ drivers/fpga/fpga.c | 9 + drivers/fpga/xilinx.c | 13 + include/fpga.h| 1 + 5 files changed, 26 insertions(+), 5 deletions(-) diff --git a/common/bootm.c b/common/bootm.c index 9493a306cd..adb12137c7 100644 --- a/common/bootm.c +++ b/common/bootm.c @@ -248,7 +248,7 @@ int bootm_find_images(int flag, int argc, char * const argv[]) #endif #if IMAGE_ENABLE_FIT -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX) +#if defined(CONFIG_FPGA) /* find bitstreams */ ret = boot_get_fpga(argc, argv, &images, IH_ARCH_DEFAULT, NULL, NULL); diff --git a/common/image.c b/common/image.c index 06fdca129c..9fd72b9423 100644 --- a/common/image.c +++ b/common/image.c @@ -1223,7 +1223,7 @@ int boot_get_setup(bootm_headers_t *images, uint8_t arch, } #if IMAGE_ENABLE_FIT -#if defined(CONFIG_FPGA) && defined(CONFIG_FPGA_XILINX) +#if defined(CONFIG_FPGA) int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, uint8_t arch, const ulong *ld_start, ulong * const ld_len) { @@ -1234,8 +1234,6 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, const char *uname, *name; int err; int devnum = 0; /* TODO support multi fpga platforms */ - const fpga_desc * const desc = fpga_get_desc(devnum); - xilinx_desc *desc_xilinx = desc->devdesc; /* Check to see if the images struct has a FIT configuration */ if (!genimg_has_config(images)) { @@ -1280,7 +1278,7 @@ int boot_get_fpga(int argc, char * const argv[], bootm_headers_t *images, return fit_img_result; } - if (img_len >= desc_xilinx->size) { + if (!fpga_is_partial_data(devnum, img_len)) { name = "full"; err = fpga_loadbitstream(devnum, (char *)img_data, img_len, BIT_FULL); diff --git a/drivers/fpga/fpga.c b/drivers/fpga/fpga.c index e0fb1b4e78..6aead27f16 100644 --- a/drivers/fpga/fpga.c +++ b/drivers/fpga/fpga.c @@ -171,6 +171,15 @@ int fpga_add(fpga_type devtype, void *desc) } /* + * Return 1 if the fpga data is partial. + * This is only required for fpga drivers that support bitstream_type. + */ +int __weak fpga_is_partial_data(int devnum, size_t img_len) +{ + return 0; +} + +/* * Convert bitstream data and load into the fpga */ int __weak fpga_loadbitstream(int devnum, char *fpgadata, size_t size, diff --git a/drivers/fpga/xilinx.c b/drivers/fpga/xilinx.c index 941f30010a..3c05760969 100644 --- a/drivers/fpga/xilinx.c +++ b/drivers/fpga/xilinx.c @@ -24,6 +24,19 @@ static int xilinx_validate(xilinx_desc *desc, char *fn); /* - */ +int fpga_is_partial_data(int devnum, size_t img_len) +{ + const fpga_desc * const desc = fpga_get_desc(devnum); + xilinx_desc *desc_xilinx = desc->devdesc; + + /* Check datasize against FPGA size */ + if (img_len >= desc_xilinx->size) + return 0; + + /* datasize is smaller, must be partial data */ + return 1; +} + int fpga_loadbitstream(int devnum, char *fpgadata, size_t size, bitstream_type bstype) { diff --git a/include/fpga.h b/include/fpga.h index d768fb1417..4d6da790b7 100644 --- a/include/fpga.h +++ b/include/fpga.h @@ -54,6 +54,7 @@ void fpga_init(void); int fpga_add(fpga_type devtype, void *desc); int fpga_count(void); const fpga_desc *const fpga_get_desc(int devnum); +int fpga_is_partial_data(int devnum, size_t img_len); int fpga_load(int devnum, const void *buf, size_t bsize, bitstream_type bstype); int fpga_fsload(int devnum, const void *buf, size_t size, -- 2.12.2.windows.2 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH] rockchip: doc: update U-Boot location info
The U-Boot location has been moved to block 16384. This is 8MB, not 4MB. Signed-off-by: Simon Goldschmidt --- doc/README.rockchip | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/README.rockchip b/doc/README.rockchip index 9d5af3d53d..a7761af3e8 100644 --- a/doc/README.rockchip +++ b/doc/README.rockchip @@ -102,7 +102,7 @@ To write an image that boots from an SD card (assumed to be /dev/sdc): sudo dd if=firefly-rk3288/u-boot-dtb.img of=/dev/sdc seek=16384 This puts the Rockchip header and SPL image first and then places the U-Boot -image at block 16384 (i.e. 4MB from the start of the SD card). This +image at block 16384 (i.e. 8MB from the start of the SD card). This corresponds with this setting in U-Boot: #define CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR 0x4000 -- 2.12.2.windows.2 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [U-Boot, 2/3] rockchip: doc: update U-Boot location info
>> I just found this commit has calculated the size wrong. 16384 blocks should >> be 8MB, not 4MB. > > Yes, 8MB is what expected here and even Kever[1] commented the same, what is > 4MB here? can you elaborate. I know. It's only wrong in 'doc/README.rockchip' at line 105 where it says: "image at block 16384 (i.e. 4MB from the start of the SD card)" Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [U-Boot, 2/3] rockchip: doc: update U-Boot location info
>> Update rockchip U-Boot location to 0x4000/16384. >> >> Signed-off-by: Kever Yang >> Acked-by: Philipp Tomsich >> Reviewed-by: Philipp Tomsich >> --- >> >> doc/README.rockchip | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> > > Applied to u-boot-rockchip, thanks! I just found this commit has calculated the size wrong. 16384 blocks should be 8MB, not 4MB. Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] sf: ensure flash device is in 3-byte address mode
On Mon, Oct 30, 2017 at 7:26 AM, Jagan Teki wrote: > I've similar change on my patchwork, since no-one tested Will CC you by > re-basing it please have test? Yes, of course I'd like to test this. Where do I find your patch? Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH] sf: ensure flash device is in 3-byte address mode
On some boards where the spi flash is not reset during warm reboot, the chip has to be manually set into 3-byte address mode. Signed-off-by: Simon Goldschmidt --- drivers/mtd/spi/sf_internal.h | 2 ++ drivers/mtd/spi/spi_flash.c | 53 +++ 2 files changed, 55 insertions(+) diff --git a/drivers/mtd/spi/sf_internal.h b/drivers/mtd/spi/sf_internal.h index 839cdbe1b0..06dee0a4ea 100644 --- a/drivers/mtd/spi/sf_internal.h +++ b/drivers/mtd/spi/sf_internal.h @@ -62,6 +62,8 @@ enum spi_nor_option_flags { #define CMD_READ_STATUS1 0x35 #define CMD_READ_CONFIG0x35 #define CMD_FLAG_STATUS0x70 +#define CMD_EN4B 0xB7 +#define CMD_EX4B 0xE9 /* Bank addr access commands */ #ifdef CONFIG_SPI_FLASH_BAR diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c index 34f68881ed..8db2882075 100644 --- a/drivers/mtd/spi/spi_flash.c +++ b/drivers/mtd/spi/spi_flash.c @@ -165,6 +165,55 @@ bar_end: } #endif +static int set_4byte(struct spi_flash *flash, const struct spi_flash_info *info, + u8 enable) +{ + int ret; + bool need_wren = false; + u8 cmd; + + if (flash->size <= SPI_FLASH_16MB_BOUN) + return 0; + + switch (JEDEC_MFR(info)) { + case SPI_FLASH_CFI_MFR_STMICRO: + /* Some Micron need WREN command; all will accept it */ + need_wren = true; + case SPI_FLASH_CFI_MFR_MACRONIX: + case SPI_FLASH_CFI_MFR_WINBOND: + ret = spi_claim_bus(flash->spi); + if (ret) { + debug("SF: Unable to claim SPI bus\n"); + return ret; + } + + if (need_wren) { + ret = spi_flash_cmd_write_enable(flash); + if (ret < 0) { + debug("SF: enabling write failed\n"); + spi_release_bus(flash->spi); + return ret; + } + } + + cmd = enable ? CMD_EN4B : CMD_EX4B; + ret = spi_flash_cmd_write(flash->spi, &cmd, 1, NULL, 0); + if (ret) { + debug("SF: fail to %s 4-byte address mode\n", + enable ? "enter" : "exit"); + } + if (need_wren) + if (spi_flash_cmd_write_disable(flash) < 0) + debug("SF: disabling write failed\n"); + spi_release_bus(flash->spi); + return ret; + default: + /* Spansion style handled by bar_write */ + break; + } + return 0; +} + #ifdef CONFIG_SF_DUAL_FLASH static void spi_flash_dual(struct spi_flash *flash, u32 *addr) { @@ -1086,6 +1135,10 @@ int spi_flash_scan(struct spi_flash *flash) flash->flags |= SNOR_F_USE_FSR; #endif + /* disable 4-byte addressing if the device exceeds 16MiB */ + if (flash->size > SPI_FLASH_16MB_BOUN) + set_4byte(flash, info, 0); + /* Configure the BAR - discover bank cmds and read current bank */ #ifdef CONFIG_SPI_FLASH_BAR ret = read_bar(flash, info); -- 2.12.2.windows.2 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] Antwort: Re: QSPI "sf probe ...", "sf read ..." on Altera SoC FPGA
Hi Clémént, > Did you also test the saveenv and sf unlock ? I did test saveenv and it works. I did not test sf protection. > Did you get some strange behaviors after a "warm reboot" from linux ? Indeed, warm reboot fails. When rebooting via "reboot" command from linux, the last thing I see is SPL writing "Trying to boot from SPI". I haven't been able to debug this further, yet. Also, I still can't sf read without disabling the data cache :-( Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] Antwort: Re: QSPI "sf probe ...", "sf read ..." on Altera SoC FPGA
Hi Clement, > Did you also test the saveenv and sf unlock ? Not yet. > Did you get some strange behaviors after a "warm reboot" from linux ? Unfortunately, I'm not that far, yet. My Linux image only uses the sd-card for now. Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
[U-Boot] Debugging SPL
Hi, When trying to debug SPL on the socfpga platform with CONFIG_OF_EMBED, I get an error message "SPL image too big" when linking SPL. It seems this is because the U-Boot DTB (~16 Kbyte) is not stripped for SPL when building this way. Is this a known issue? Is there a workaround other than manually creating a dedicated DTB for debugging SPL in the OF_EMBED mode? Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] Antwort: Re: QSPI "sf probe ...", "sf read ..." on Altera SoC FPGA
On 09/27/2017 06:54 AM, Hannes Schmelzer wrote: > On 09/22/2017 02:20 PM, Clément Péron wrote: >> Sorry these are my local commits you can find them here : >> >> https://patchwork.ozlabs.org/patch/765992/ >> https://patchwork.ozlabs.org/patch/765996/ >> https://patchwork.ozlabs.org/patch/765997/ >> https://patchwork.ozlabs.org/patch/765998/ > Hi, > just tested this on my cyclone5 board, but unfortunately without success. > > --- > U-Boot SPL 2017.09-00354-g824def8 (Sep 27 2017 - 06:47:19) > () > Hit any key to stop autoboot: 0 > => sf probe > SF: Detected n25q512 with page size 256 Bytes, erase size 64 KiB, total > 64 MiB > ### ERROR ### Please RESET the board ### > > > same behavior as before. Have you compared your dts to the other socfpga_cyclone5_*.dts files? I have tested this on the socrates board and it would also give me the above error until I change the "compatible" string of the flash chip from "n25q00" to "spi-flash". Without that, the sf code seems to try to use the first child node as flash chip which results in a divide-by-zero error because it passes a frequency of 0 Hz to the set_speed callback of the qspi driver, which it is not prepared to handle: If cadence_qspi_apb_config_baudrate_div is called with sclk_hz == 0, the DIV_ROUND_UP macro fails to check the denominator for zero. I wonder where this should be fixed: in the core sf code or in this driver... Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] Antwort: Re: QSPI "sf probe ...", "sf read ..." on Altera SoC FPGA
On 09/22/2017 02:20 PM, Clément Péron wrote: > Sorry these are my local commits you can find them here : > > https://patchwork.ozlabs.org/patch/765992/ > https://patchwork.ozlabs.org/patch/765996/ > https://patchwork.ozlabs.org/patch/765997/ > https://patchwork.ozlabs.org/patch/765998/ Tested on socfpga_cyclone5_socrates by applying these 4 patches to 2017.09. Works as expected. I had to disable the data cache to actually get the data from qspi to ram, but that's a totally different issue. Tested-by: Simon Goldschmidt As I'm rather new to this list and project, when can we expect to have this patch committed? Regards, Simon ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot