[PATCH] sunxi: restore modified memory
On A64 with 2G of RAM words at following addresses were modified with 'aa55aa55' value: - 5000 - 6000 - 8400 - 8800 - 9000 - A000 - A200 That made harder to pick memory range for persistent storage in RAM. Signed-off-by: Andrey Skvortsov --- arch/arm/mach-sunxi/dram_helpers.c | 16 ++-- arch/arm/mach-sunxi/dram_sunxi_dw.c | 16 ++-- 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/arch/arm/mach-sunxi/dram_helpers.c b/arch/arm/mach-sunxi/dram_helpers.c index 2c873192e6..e3d236a4b3 100644 --- a/arch/arm/mach-sunxi/dram_helpers.c +++ b/arch/arm/mach-sunxi/dram_helpers.c @@ -32,12 +32,24 @@ void mctl_await_completion(u32 *reg, u32 mask, u32 val) #ifndef CONFIG_MACH_SUNIV bool mctl_mem_matches(u32 offset) { + unsigned long tmp_base; + unsigned long tmp_offset; + bool ret; + +/* Save original values */ + tmp_base = readl(CFG_SYS_SDRAM_BASE); + tmp_offset = readl((ulong)CFG_SYS_SDRAM_BASE + offset); + /* Try to write different values to RAM at two addresses */ writel(0, CFG_SYS_SDRAM_BASE); writel(0xaa55aa55, (ulong)CFG_SYS_SDRAM_BASE + offset); dsb(); /* Check if the same value is actually observed when reading back */ - return readl(CFG_SYS_SDRAM_BASE) == - readl((ulong)CFG_SYS_SDRAM_BASE + offset); + ret = readl(CFG_SYS_SDRAM_BASE) == readl((ulong)CFG_SYS_SDRAM_BASE + offset); + + /* Restore original values */ + writel(tmp_base, CFG_SYS_SDRAM_BASE); + writel(tmp_offset, (ulong)CFG_SYS_SDRAM_BASE + offset); + return ret; } #endif diff --git a/arch/arm/mach-sunxi/dram_sunxi_dw.c b/arch/arm/mach-sunxi/dram_sunxi_dw.c index 9107b114df..6245815fa2 100644 --- a/arch/arm/mach-sunxi/dram_sunxi_dw.c +++ b/arch/arm/mach-sunxi/dram_sunxi_dw.c @@ -657,13 +657,25 @@ static int mctl_channel_init(uint16_t socid, struct dram_para *para) */ static bool mctl_mem_matches_base(u32 offset, ulong base) { + unsigned long tmp_base; + unsigned long tmp_offset; + bool ret; + + /* Save original values */ + tmp_base = readl(base); + tmp_offset = readl((ulong)base + offset); + /* Try to write different values to RAM at two addresses */ writel(0, base); writel(0xaa55aa55, base + offset); dsb(); /* Check if the same value is actually observed when reading back */ - return readl(base) == - readl(base + offset); + ret = readl(base) == readl(base + offset); + + /* Restore original values */ + writel(tmp_base, base); + writel(tmp_offset, (ulong)base + offset); + return ret; } static void mctl_auto_detect_dram_size_rank(uint16_t socid, struct dram_para *para, ulong base, struct rank_para *rank) -- 2.39.2
Re: [PATCH] pstore: Use root address-cells/size-cells as defaults for reserved-memory
Hi, On 23-08-26 19:41, Andre Przywara wrote: > On Sat, 26 Aug 2023 15:10:10 +0200 > Heinrich Schuchardt wrote: > > Hi, > > > On 8/26/23 14:16, Andrey Skvortsov wrote: > > > u-boot adds reserve-memory node, if it's missing, with following > > > properties: > > > > > > ``` > > > reserved-memory { > > > #address-cells = <2>; > > > #size-cells = <2>; > > > > This defines the size of cells for the children of reserved-memory and > > and for the ranges property. If you set the cell sizes to 1 you can no > > longer encode 64 bit addresses. > > Yes, this is expected in this case, the Allwinner A64 SoC has its memory > map (including DRAM) completely below 4GB, hence the root node can go > with 1/1. > > > > > > ranges; > > > } > > > ``` > > > > > > But with these default address-cells and size-cells values, pstore > > > isn't working on A64. Root node for A64 defines 'address-cells' and > > > 'size-cells' as 1. > > > > > > dtc complains if reserved-memory has different address-cells and > > > size-cells. > > > > > > ``` > > > Warning (ranges_format): /reserved-memory:ranges: empty "ranges" > > > property but its #address-cells (2) differs from / (1) > > > > I cannot find any such requirement in the Devicetree Specification 1.4. > > Is this a dtc bug? > > I think the culprit here is the *empty* ranges property: > "If the property is defined with an value, it specifies that the > parent and child address space is identical, and no address translation > is required." > As this is contradicted by the differing #a-c/#s-z properties, it looks > like dtc has good reasons to warn. > > > > > > ``` > > > > > > This patch takes into account address-cells and size-cells of the root > > > node and uses them as values for new reserved-memory node. > > > > Reservations may be above 4 GiB. How does your patch consider this? > > If the root #a-c/#s-c don't allow for more than 4GB, then it's for a > reason (in this case, the A64 being 32-bit only, even with AArch64 > capable cores), and keeping it isn't restricting it further. So I think > copying the root properties is the right thing to do. I think almost > every other 64-bit core uses 2/2 anyway, so it's just the odd outlier > here. > > Cheers, > Andre Are there any comments I should fix to get this change merged or maybe another way of solving this problem? I see, that patch requires review from Tom Rini currently on patchwork [1]. 1. https://patchwork.ozlabs.org/project/uboot/patch/20230826121652.2487643-1-andrej.skvort...@gmail.com/ -- Best regards, Andrey Skvortsov
[PATCH] pstore: Use root address-cells/size-cells as defaults for reserved-memory
u-boot adds reserve-memory node, if it's missing, with following properties: ``` reserved-memory { #address-cells = <2>; #size-cells = <2>; ranges; } ``` But with these default address-cells and size-cells values, pstore isn't working on A64. Root node for A64 defines 'address-cells' and 'size-cells' as 1. dtc complains if reserved-memory has different address-cells and size-cells. ``` Warning (ranges_format): /reserved-memory:ranges: empty "ranges" property but its #address-cells (2) differs from / (1) ``` This patch takes into account address-cells and size-cells of the root node and uses them as values for new reserved-memory node. Signed-off-by: Andrey Skvortsov --- cmd/pstore.c | 10 -- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/cmd/pstore.c b/cmd/pstore.c index cd6f6feb2f..9795eea2db 100644 --- a/cmd/pstore.c +++ b/cmd/pstore.c @@ -486,6 +486,8 @@ void fdt_fixup_pstore(void *blob) { char node[32]; int nodeoffset;/* node offset from libfdt */ + u32 addr_cells_root; + u32 size_cells_root; u32 addr_cells; u32 size_cells; @@ -495,6 +497,8 @@ void fdt_fixup_pstore(void *blob) log_err("fdt_path_offset() returned %s\n", fdt_strerror(nodeoffset)); return; } + addr_cells_root = fdt_getprop_u32_default_node(blob, nodeoffset, 0, "#address-cells", 2); + size_cells_root = fdt_getprop_u32_default_node(blob, nodeoffset, 0, "#size-cells", 2); nodeoffset = fdt_find_or_add_subnode(blob, nodeoffset, "reserved-memory"); if (nodeoffset < 0) { @@ -503,8 +507,10 @@ void fdt_fixup_pstore(void *blob) return; } - addr_cells = fdt_getprop_u32_default_node(blob, nodeoffset, 0, "#address-cells", 2); - size_cells = fdt_getprop_u32_default_node(blob, nodeoffset, 0, "#size-cells", 2); + addr_cells = fdt_getprop_u32_default_node(blob, nodeoffset, 0, + "#address-cells", addr_cells_root); + size_cells = fdt_getprop_u32_default_node(blob, nodeoffset, 0, + "#size-cells", size_cells_root); fdt_setprop_u32(blob, nodeoffset, "#address-cells", addr_cells); fdt_setprop_u32(blob, nodeoffset, "#size-cells", size_cells); -- 2.40.1
Re: [PATCH] pstore: Use root address-cells/size-cells as defaults for reserved-memory
Hi Heinrich, On 23-08-26 15:10, Heinrich Schuchardt wrote: > On 8/26/23 14:16, Andrey Skvortsov wrote: > > u-boot adds reserve-memory node, if it's missing, with following > > properties: > > > > ``` > > reserved-memory { > > #address-cells = <2>; > > #size-cells = <2>; > > This defines the size of cells for the children of reserved-memory and > and for the ranges property. If you set the cell sizes to 1 you can no > longer encode 64 bit addresses. This will be limited to 32 bit only for platforms, that don't support 64 bit. These platforms explicitly defines #address-cells = 1 and #size-cells = 1 in root node. > > ranges; > > } > > ``` > > > > But with these default address-cells and size-cells values, pstore > > isn't working on A64. Root node for A64 defines 'address-cells' and > > 'size-cells' as 1. > > > > dtc complains if reserved-memory has different address-cells and > > size-cells. > > > > ``` > > Warning (ranges_format): /reserved-memory:ranges: empty "ranges" > > property but its #address-cells (2) differs from / (1) > > I cannot find any such requirement in the Devicetree Specification 1.4. > Is this a dtc bug? This is current behavior of Linux kernel. When root #address-cells and reserved-memory #address-cells don't match, then reserved-memory region is ignored. See [1]. dmesg output: ``` OF: fdt: Reserved memory: unsupported node format, ignoring ``` > > This patch takes into account address-cells and size-cells of the root > > node and uses them as values for new reserved-memory node. > > Reservations may be above 4 GiB. How does your patch consider this? It's entirely possible to do on platforms, that support that. Default fallback value is still 2 for #address-cells and #size-cells. 1. https://elixir.bootlin.com/linux/latest/source/drivers/of/fdt.c#L548 -- Best regards, Andrey Skvortsov
[PATCH] sunxi: board: provide CPU idle states to loaded OS
When using SCPI as the PSCI backend, firmware can wake up the CPUs and cluster from sleep, so CPU idle states are available for loaded OS to use. TF-A modifies DTB to advertise available CPU idle states, when SCPI is detected. This change copies nodes added by TF-A to any new dtb that is used for loaded OS. Signed-off-by: Andrey Skvortsov --- board/sunxi/board.c | 120 1 file changed, 120 insertions(+) diff --git a/board/sunxi/board.c b/board/sunxi/board.c index f321cd58a6..e88bd11a99 100644 --- a/board/sunxi/board.c +++ b/board/sunxi/board.c @@ -870,6 +870,125 @@ int board_late_init(void) return 0; } +static int cpuidle_dt_fixup_copy_node(ofnode src, int dst_offset, void *dst_blob, + u32 *count, u32 *phandle) +{ + int offs, len, ret; + struct ofprop prop; + ofnode subnode; + + ofnode_for_each_prop(prop, src) { + const char *name; + const char *val; + + val = ofprop_get_property(&prop, &name, &len); + if (!val) + return -EINVAL; + if (!strcmp(name, "phandle")) { + if (ofnode_device_is_compatible(src, "arm,idle-state")) { + fdt_setprop_u32(dst_blob, dst_offset, name, *phandle); + (*phandle)++; + (*count)++; + } else { + log_err("Unexpected phandle node: %s\n", name); + return -EINVAL; + } + } else { + ret = fdt_setprop(dst_blob, dst_offset, name, val, len); + if (ret < 0) + return ret; + } + } + + ofnode_for_each_subnode(subnode, src) { + const char *name = ofnode_get_name(subnode); + + if (!name) + return -EINVAL; + offs = fdt_add_subnode(dst_blob, dst_offset, name); + if (offs < 0) + return offs; + ret = cpuidle_dt_fixup_copy_node(subnode, offs, dst_blob, count, phandle); + if (ret < 0) + return ret; + } + return 0; +} + +static int cpuidle_dt_fixup(void *blob) +{ + ofnode idle_states_node; + ofnode subnode; + u32 count, phandle; + const struct property *prop; + int offs, len, ret; + + idle_states_node = ofnode_path("/cpus/idle-states"); + if (!ofnode_valid(idle_states_node)) { + log_err("No idle-states node in old fdt, nothing to do"); + return 0; + } + + /* +* Do not proceed if the target dt already has an idle-states node. +* In this case assume that the system knows better somehow, +* so do not interfere. +*/ + if (fdt_path_offset(blob, "/cpus/idle-states") >= 0) { + log_err("idle-states node already exists in target"); + return 0; + } + + offs = fdt_path_offset(blob, "/cpus"); + if (offs < 0) + return offs; + + offs = fdt_add_subnode(blob, offs, "idle-states"); + if (offs < 0) + return offs; + + /* copy "/cpus/idle-states" node to destination fdt */ + ret = fdt_find_max_phandle(blob, &phandle); + if (ret < 0) + return ret; + phandle++; + count = 0; + ret = cpuidle_dt_fixup_copy_node(idle_states_node, offs, blob, &count, &phandle); + if (ret < 0) + return ret; + + /* copy "cpu-idle-state" property for all cpus */ + ofnode_for_each_subnode(subnode, ofnode_path("/cpus")) { + char path[32]; + fdt32_t *value; + + prop = ofnode_get_property(subnode, "cpu-idle-states", &len); + if (!prop) + continue; + + /* find the same node in a new device-tree */ + ret = ofnode_get_path(subnode, path, sizeof(path)); + if (ret) + return ret; + + offs = fdt_path_offset(blob, path); + if (offs < 0) + return offs; + + /* Allocate space for the list of phandles. */ + ret = fdt_setprop_placeholder(blob, offs, "cpu-idle-states", + count * sizeof(phandle), + (void **)&value); + if (ret < 0) + return ret; + + /* Fill in the phandles of the idle state nodes. */ + for (u32 i = 0U;
Re: [PATCH] sunxi: board: provide CPU idle states to loaded OS
Hi Andrey, On 23-09-05 09:27, Andre Przywara wrote: > On Mon, 4 Sep 2023 23:54:30 +0300 > Andrey Skvortsov wrote: > > Hi Andrey, > > > When using SCPI as the PSCI backend, firmware can wake up the CPUs and > > cluster from sleep, so CPU idle states are available for loaded OS to > > use. TF-A modifies DTB to advertise available CPU idle states, when > > SCPI is detected. This change copies nodes added by TF-A to any new > > dtb that is used for loaded OS. > > Why do you need that, exactly? Why not just use $fdtcontroladdr for the > kernel? We now keep the U-Boot copy of the .dts files in sync with the > kernel. If you need to modify the DT in U-Boot, for instance by applying > overlays, you can copy that DTB into a better suitable location first: > => fdt move $fdtcontroladdr $fdt_addr_r > > In any case, there shall be only one DT, that one in the U-Boot image. Why > do you need to load another one for the kernel? extlinux is used by distributions (sometimes with device-specific changes especially for platforms not fully supported by mainline yet), then U-Boot loads DT defined in extlinux.conf file. u-boot scripts are not used in case of extlinux at all. -- Best regards, Andrey Skvortsov
Re: [PATCH] sunxi: board: provide CPU idle states to loaded OS
Hi Andre, On 23-09-06 01:12, Andre Przywara wrote: > On Tue, 5 Sep 2023 11:37:31 +0300 > Andrey Skvortsov wrote: > > Hi, > > > On 23-09-05 09:27, Andre Przywara wrote: > > > On Mon, 4 Sep 2023 23:54:30 +0300 > > > Andrey Skvortsov wrote: > > > > > > Hi Andrey, > > > > > > > When using SCPI as the PSCI backend, firmware can wake up the CPUs and > > > > cluster from sleep, so CPU idle states are available for loaded OS to > > > > use. TF-A modifies DTB to advertise available CPU idle states, when > > > > SCPI is detected. This change copies nodes added by TF-A to any new > > > > dtb that is used for loaded OS. > > > > > > Why do you need that, exactly? Why not just use $fdtcontroladdr for the > > > kernel? We now keep the U-Boot copy of the .dts files in sync with the > > > kernel. If you need to modify the DT in U-Boot, for instance by applying > > > overlays, you can copy that DTB into a better suitable location first: > > > => fdt move $fdtcontroladdr $fdt_addr_r > > > > > > In any case, there shall be only one DT, that one in the U-Boot image. Why > > > do you need to load another one for the kernel? > > > > extlinux is used by distributions (sometimes with device-specific changes > > especially > > What distros are that? I guess some special ones, targeting embedded > devices, like the Pinephone? It's more likely universal operating system. In my particular case this is Mobian (several device-specific packages on top of Debian). It's very-very close to Debian with a goal to be pure Debian in the near future. Rhino Linux is using extlinux as well and will benefit for this change as well. > And who is generating extlinux.conf then? Is that some distro specific > scripting, similar to how grub is configured? extlinux.conf is automatically generated by standard Debian u-boot-menu package. See [1] and [2]. > Honest questions, I am not a user of extlinux, I mostly use UEFI > booting, or type U-Boot commands directly for experiments, or use > boot.scr, as a quick-and-dirty hack. > > for platforms not fully supported by mainline yet), > > Do you need any changes to the DT? Do you need to apply overlays? > If you run on a non-mainlined platform, you could still put your DT > into the U-Boot tree, then you wouldn't need to load another DTB, which > also simplifies the deployment on the kernel/distro side. Currently Mobian linux kernel for sunxi-devices contains 36 extra patches with DT modifications (add/remove nodes, modify existing properties). One of them unconditionally adds cpuidle states to DT, that I'm trying to fix upstream here with the proposed change. DT overlays are not used. Honestly I don't think that using dtb from u-boot will simplify distribution deployment and maintenance. IMHO, it will make things more complex. Currently on my device there are records in extlinux.conf for 5.9, 5.10, 5.15, several 6.1 kernels, that I'm working on. All of them have slightly (or sometimes more) different device-trees. Having kernel and dtb deployed together makes life so much easier. Mobian's dtb can't be put into u-boot. Mobian doesn't provide device-specific bootloader (u-boot) and there is on-going work on making device-independent universal images. As many other mobile Linux distributions Mobian relies on Tow-Boot. That is used as a cross-distribution firmware (u-boot/tf-a/crust). [3] > > then U-Boot loads DT defined in > > extlinux.conf file. u-boot scripts are not used in case of extlinux at all. > > That's fine, you don't need any U-Boot scripts for this to work. If > there is no "fdt" or "fdtdir" label in extlinux.conf, then the U-Boot > PXE code will eventually fall back to $fdtcontroladdr - I just tested > that. > So could you make that work for you? I guess all you need to change is > to remove any fdtdir label from extlinux.conf? > > Cheers, > Andre Regardless proposed change. Changes to dtb nodes already copied in u-boot on the fly in boot/image-fdt.c:image_setup_libfdt. For example, created optee nodes are copied there. I've just put platform specific changes into platform specific ft_board_setup, that was made apparently exactly for that. 1. https://packages.debian.org/trixie/u-boot-menu 2. https://salsa.debian.org/debian/u-boot-menu 3. https://tow-boot.org/ -- Best regards, Andrey Skvortsov
Re: [PATCH] sunxi: board: provide CPU idle states to loaded OS
Hi Andre, On 23-09-11 23:15, Andre Przywara wrote: > On Wed, 6 Sep 2023 23:53:51 +0300 > Andrey Skvortsov wrote: > > > > > > > When using SCPI as the PSCI backend, firmware can wake up the CPUs > > > > > > and > > > > > > cluster from sleep, so CPU idle states are available for loaded OS > > > > > > to > > > > > > use. TF-A modifies DTB to advertise available CPU idle states, when > > > > > > SCPI is detected. This change copies nodes added by TF-A to any new > > > > > > dtb that is used for loaded OS. > > > > > > > > > > Why do you need that, exactly? Why not just use $fdtcontroladdr for > > > > > the > > > > > kernel? We now keep the U-Boot copy of the .dts files in sync with the > > > > > kernel. If you need to modify the DT in U-Boot, for instance by > > > > > applying > > > > > overlays, you can copy that DTB into a better suitable location > > > > > first: > > > > > => fdt move $fdtcontroladdr $fdt_addr_r > > > > > > > > > > In any case, there shall be only one DT, that one in the U-Boot > > > > > image. Why > > > > > do you need to load another one for the kernel? > > > > > > > > extlinux is used by distributions (sometimes with device-specific > > > > changes especially > > > > > > What distros are that? I guess some special ones, targeting embedded > > > devices, like the Pinephone? > > > > It's more likely universal operating system. In my particular case > > this is Mobian (several device-specific packages on top of > > Debian). It's very-very close to Debian with a goal to be > > pure Debian in the near future. > > > > Rhino Linux is using extlinux as well and will benefit for this change > > as well. > > We seem to have a very different understanding of "universal operating > systems" ;-) > You seem to talk about specific distros for embedded devices, whereas I > think of Ubuntu/Debian/Fedora/Arch/you-name-it. I was talking about Debian (The universal operating system ;-) initially. Even if PinePhone in my case runs Mobian, it uses the same packages and scripts and repositories from Debian. The difference is quite small (kernel and some other small changes). > And I still don't understand what a particular distribution has to do > with that: this is purely a question of boot firmware and who provides > the DTB. > > > > > > for platforms not fully supported by mainline yet), > > > > > > Do you need any changes to the DT? Do you need to apply overlays? > > > If you run on a non-mainlined platform, you could still put your DT > > > into the U-Boot tree, then you wouldn't need to load another DTB, which > > > also simplifies the deployment on the kernel/distro side. > > > > Currently Mobian linux kernel for sunxi-devices contains 36 extra patches > > with DT > > That does not sound good. Why are those patches not upstream? Agree, I ask the same question. Some of the patches were posted for review and stalled, some of them haven't been posted. I'm preparing table to track current status of the patches. Maybe ping some of them. There are some big and complex changes like wlan/bt driver, that it's hard to upstream in the current shape. > > modifications (add/remove nodes, modify existing properties). One of > > them unconditionally adds cpuidle states to DT, that I'm trying to fix > > upstream here with the proposed change. > > That's very honourable, but not every patch that works(TM) is the right > approach to take upstream. Sure, this one will probably stay in our patches until all DT changes are upstream. > > Mobian doesn't provide > > device-specific bootloader (u-boot) and there is on-going work on > > making device-independent universal images. > > With "universal image" you mean a single rootfs, and a single kernel for > every device? Of course, this is how Linux works everywhere, and the > arm64 kernel was a single-image kernel from day one (same as x86). And > taking the DTs out there and into firmware is just even more > consequential. > > > > > Regardless proposed change. Changes to dtb nodes already copied in > > u-boot on the fly in boot/image-fdt.c:image_setup_libfdt. For example, > > created optee nodes are copied there. > > Yes, that is an example, just not a particularly good one. This seems > to originate from 2019, again building on the idea that you need to load > a DTB for the kernel from somewhere. I don't like to proliferate those > ideas anymore. > > > I've just put platform specific > > changes into platform specific ft_board_setup, that was made > > apparently exactly for that. > > Not really, that's for platform specific runtime *adjustments*, like > adding a unique MAC address, or adding simplefb DT nodes. Thanks again for taking time and explaining your position. I agree, that in a long term dtb should be provided by u-boot. In that case proposed change will not be needed. Hopefully we'll get all changes upstream and could use $fdtcontroladdr some day. -- Best regards, Andrey Skvortsov
[PATCH] distro_bootcmd: Always check for custom boot scripts first
If extlinux.conf is used, then it's not possible to customize boot environment, because scripts are not loaded. Usually it's possible to make some changes manually using command line and save boot environment. But if exlinux.conf is loaded from ext4 partition (for example on PinePhone), then environment are not saved/loaded at boot time from boot partition and it's not possible to persistently change boot environment without recompiling u-boot. Signed-off-by: Andrey Skvortsov --- include/config_distro_bootcmd.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h index 5506f3168f..7f4ef960a1 100644 --- a/include/config_distro_bootcmd.h +++ b/include/config_distro_bootcmd.h @@ -477,8 +477,8 @@ "echo Scanning ${devtype} " \ "${devnum}:${distro_bootpart}...; " \ "for prefix in ${boot_prefixes}; do " \ - "run scan_dev_for_extlinux; " \ "run scan_dev_for_scripts; " \ + "run scan_dev_for_extlinux; " \ "done;" \ SCAN_DEV_FOR_EFI \ "\0" \ -- 2.20.1
Re: [PATCH] sunxi: restore modified memory
Hi Andre, On 23-11-01 09:50, Andre Przywara wrote: > On Fri, 21 Jul 2023 17:57:21 +0300 > Andrey Skvortsov wrote: > > Hi Andrey, > > sorry for the late reply! > > > On A64 with 2G of RAM words at following addresses were modified with > > 'aa55aa55' value: > > - 5000 > > - 6000 > > - 8400 > > - 8800 > > - 9000 > > - A000 > > - A200 > > > > That made harder to pick memory range for persistent storage in RAM. > > In general I now think this patch is fine, since DRAM content can indeed > be preserved across some reboot types. > Some things below: > > > Signed-off-by: Andrey Skvortsov > > --- > > arch/arm/mach-sunxi/dram_helpers.c | 16 ++-- > > arch/arm/mach-sunxi/dram_sunxi_dw.c | 16 ++-- > > 2 files changed, 28 insertions(+), 4 deletions(-) > > > > diff --git a/arch/arm/mach-sunxi/dram_helpers.c > > b/arch/arm/mach-sunxi/dram_helpers.c > > index 2c873192e6..e3d236a4b3 100644 > > --- a/arch/arm/mach-sunxi/dram_helpers.c > > +++ b/arch/arm/mach-sunxi/dram_helpers.c > > @@ -32,12 +32,24 @@ void mctl_await_completion(u32 *reg, u32 mask, u32 val) > > #ifndef CONFIG_MACH_SUNIV > > bool mctl_mem_matches(u32 offset) > > { > > + unsigned long tmp_base; > > + unsigned long tmp_offset; > > The type doesn't match the return type of readl. > Please use uint32_t here. > And the names are somewhat misleading, at least to my mind, since they are > not a base address and an offset, but rather just values at a base > address and at an offset from that. > So I think val_base and val_offset would be better. Thanks for the review. > > + bool ret; > > + > > +/* Save original values */ > > + tmp_base = readl(CFG_SYS_SDRAM_BASE); > > + tmp_offset = readl((ulong)CFG_SYS_SDRAM_BASE + offset); > > + > > /* Try to write different values to RAM at two addresses */ > > writel(0, CFG_SYS_SDRAM_BASE); > > writel(0xaa55aa55, (ulong)CFG_SYS_SDRAM_BASE + offset); > > dsb(); > > /* Check if the same value is actually observed when reading back */ > > - return readl(CFG_SYS_SDRAM_BASE) == > > - readl((ulong)CFG_SYS_SDRAM_BASE + offset); > > + ret = readl(CFG_SYS_SDRAM_BASE) == readl((ulong)CFG_SYS_SDRAM_BASE + > > offset); > > + > > + /* Restore original values */ > > + writel(tmp_base, CFG_SYS_SDRAM_BASE); > > + writel(tmp_offset, (ulong)CFG_SYS_SDRAM_BASE + offset); > > + return ret; > > } > > #endif > > diff --git a/arch/arm/mach-sunxi/dram_sunxi_dw.c > > b/arch/arm/mach-sunxi/dram_sunxi_dw.c > > index 9107b114df..6245815fa2 100644 > > --- a/arch/arm/mach-sunxi/dram_sunxi_dw.c > > +++ b/arch/arm/mach-sunxi/dram_sunxi_dw.c > > @@ -657,13 +657,25 @@ static int mctl_channel_init(uint16_t socid, struct > > dram_para *para) > > */ > > static bool mctl_mem_matches_base(u32 offset, ulong base) > > Mmh, somehow I dimly remember commenting on this before (some other > patch?), but this function is essentially the same as above, isn't it? > If someone has some spare cycles, they could be merged, implementing > mctl_mem_matches() as a tiny wrapper around mctl_mem_matches_base(), to > preserve all callers. > > Shouldn't block this patch, though. Good point. I'll do this as part of v2 as a separate patch. -- Best regards, Andrey Skvortsov
[PATCH v2 0/2] sunxi: restore modified memory
Changes in v2: - rename temporary variables - fix types for temporary variables Andrey Skvortsov (2): sunxi: restore modified memory sunxi: reorganize mctl_mem_matches_* functions arch/arm/include/asm/arch-sunxi/dram.h | 1 + arch/arm/mach-sunxi/dram_helpers.c | 32 +- arch/arm/mach-sunxi/dram_sunxi_dw.c| 13 --- 3 files changed, 27 insertions(+), 19 deletions(-) -- 2.42.0
[PATCH v2 1/2] sunxi: restore modified memory
On A64 with 2G of RAM words at following addresses were modified with 'aa55aa55' value: - 5000 - 6000 - 8400 - 8800 - 9000 - A000 - A200 That made harder to pick memory range for persistent storage in RAM. Signed-off-by: Andrey Skvortsov --- arch/arm/mach-sunxi/dram_helpers.c | 16 ++-- arch/arm/mach-sunxi/dram_sunxi_dw.c | 16 ++-- 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/arch/arm/mach-sunxi/dram_helpers.c b/arch/arm/mach-sunxi/dram_helpers.c index cdf2750f1c..61a6da84e3 100644 --- a/arch/arm/mach-sunxi/dram_helpers.c +++ b/arch/arm/mach-sunxi/dram_helpers.c @@ -32,12 +32,24 @@ void mctl_await_completion(u32 *reg, u32 mask, u32 val) #ifndef CONFIG_MACH_SUNIV bool mctl_mem_matches(u32 offset) { + u32 val_base; + u32 val_offset; + bool ret; + + /* Save original values */ + val_base = readl(CFG_SYS_SDRAM_BASE); + val_offset = readl((ulong)CFG_SYS_SDRAM_BASE + offset); + /* Try to write different values to RAM at two addresses */ writel(0, CFG_SYS_SDRAM_BASE); writel(0xaa55aa55, (ulong)CFG_SYS_SDRAM_BASE + offset); dsb(); /* Check if the same value is actually observed when reading back */ - return readl(CFG_SYS_SDRAM_BASE) == - readl((ulong)CFG_SYS_SDRAM_BASE + offset); + ret = readl(CFG_SYS_SDRAM_BASE) == readl((ulong)CFG_SYS_SDRAM_BASE + offset); + + /* Restore original values */ + writel(val_base, CFG_SYS_SDRAM_BASE); + writel(val_offset, (ulong)CFG_SYS_SDRAM_BASE + offset); + return ret; } #endif diff --git a/arch/arm/mach-sunxi/dram_sunxi_dw.c b/arch/arm/mach-sunxi/dram_sunxi_dw.c index 9382d3d0be..905a43c918 100644 --- a/arch/arm/mach-sunxi/dram_sunxi_dw.c +++ b/arch/arm/mach-sunxi/dram_sunxi_dw.c @@ -657,13 +657,25 @@ static int mctl_channel_init(uint16_t socid, struct dram_para *para) */ static bool mctl_mem_matches_base(u32 offset, ulong base) { + u32 val_base; + u32 val_offset; + bool ret; + + /* Save original values */ + val_base = readl(base); + val_offset = readl((ulong)base + offset); + /* Try to write different values to RAM at two addresses */ writel(0, base); writel(0xaa55aa55, base + offset); dsb(); /* Check if the same value is actually observed when reading back */ - return readl(base) == - readl(base + offset); + ret = readl(base) == readl(base + offset); + + /* Restore original values */ + writel(val_base, base); + writel(val_offset, (ulong)base + offset); + return ret; } static void mctl_auto_detect_dram_size_rank(uint16_t socid, struct dram_para *para, ulong base, struct rank_para *rank) -- 2.42.0
[PATCH v2 2/2] sunxi: reorganize mctl_mem_matches_* functions
mctl_mem_matches and mctl_mem_matches_base identical functions. To avoid code duplication move them to dram_helpers and make mctl_mem_matches use generic mctl_mem_matches_base. Signed-off-by: Andrey Skvortsov --- arch/arm/include/asm/arch-sunxi/dram.h | 1 + arch/arm/mach-sunxi/dram_helpers.c | 26 +- arch/arm/mach-sunxi/dram_sunxi_dw.c| 25 - 3 files changed, 18 insertions(+), 34 deletions(-) diff --git a/arch/arm/include/asm/arch-sunxi/dram.h b/arch/arm/include/asm/arch-sunxi/dram.h index 682daae6b1..9d21b49241 100644 --- a/arch/arm/include/asm/arch-sunxi/dram.h +++ b/arch/arm/include/asm/arch-sunxi/dram.h @@ -40,5 +40,6 @@ unsigned long sunxi_dram_init(void); void mctl_await_completion(u32 *reg, u32 mask, u32 val); bool mctl_mem_matches(u32 offset); +bool mctl_mem_matches_base(u32 offset, ulong base); #endif /* _SUNXI_DRAM_H */ diff --git a/arch/arm/mach-sunxi/dram_helpers.c b/arch/arm/mach-sunxi/dram_helpers.c index 61a6da84e3..c86d20d22b 100644 --- a/arch/arm/mach-sunxi/dram_helpers.c +++ b/arch/arm/mach-sunxi/dram_helpers.c @@ -25,31 +25,39 @@ void mctl_await_completion(u32 *reg, u32 mask, u32 val) } /* - * Test if memory at offset offset matches memory at begin of DRAM + * Test if memory at offset matches memory at a certain base * * Note: dsb() is not available on ARMv5 in Thumb mode */ #ifndef CONFIG_MACH_SUNIV -bool mctl_mem_matches(u32 offset) +bool mctl_mem_matches_base(u32 offset, ulong base) { u32 val_base; u32 val_offset; bool ret; /* Save original values */ - val_base = readl(CFG_SYS_SDRAM_BASE); - val_offset = readl((ulong)CFG_SYS_SDRAM_BASE + offset); + val_base = readl(base); + val_offset = readl((ulong)base + offset); /* Try to write different values to RAM at two addresses */ - writel(0, CFG_SYS_SDRAM_BASE); - writel(0xaa55aa55, (ulong)CFG_SYS_SDRAM_BASE + offset); + writel(0, base); + writel(0xaa55aa55, base + offset); dsb(); /* Check if the same value is actually observed when reading back */ - ret = readl(CFG_SYS_SDRAM_BASE) == readl((ulong)CFG_SYS_SDRAM_BASE + offset); + ret = readl(base) == readl(base + offset); /* Restore original values */ - writel(val_base, CFG_SYS_SDRAM_BASE); - writel(val_offset, (ulong)CFG_SYS_SDRAM_BASE + offset); + writel(val_base, base); + writel(val_offset, (ulong)base + offset); return ret; } + +/* + * Test if memory at offset matches memory at begin of DRAM + */ +bool mctl_mem_matches(u32 offset) +{ + return mctl_mem_matches_base(offset, CFG_SYS_SDRAM_BASE); +} #endif diff --git a/arch/arm/mach-sunxi/dram_sunxi_dw.c b/arch/arm/mach-sunxi/dram_sunxi_dw.c index 905a43c918..2e8dd40b97 100644 --- a/arch/arm/mach-sunxi/dram_sunxi_dw.c +++ b/arch/arm/mach-sunxi/dram_sunxi_dw.c @@ -652,31 +652,6 @@ static int mctl_channel_init(uint16_t socid, struct dram_para *para) return 0; } -/* - * Test if memory at offset offset matches memory at a certain base - */ -static bool mctl_mem_matches_base(u32 offset, ulong base) -{ - u32 val_base; - u32 val_offset; - bool ret; - - /* Save original values */ - val_base = readl(base); - val_offset = readl((ulong)base + offset); - - /* Try to write different values to RAM at two addresses */ - writel(0, base); - writel(0xaa55aa55, base + offset); - dsb(); - /* Check if the same value is actually observed when reading back */ - ret = readl(base) == readl(base + offset); - - /* Restore original values */ - writel(val_base, base); - writel(val_offset, (ulong)base + offset); - return ret; -} static void mctl_auto_detect_dram_size_rank(uint16_t socid, struct dram_para *para, ulong base, struct rank_para *rank) { -- 2.42.0
[PATCH v3 0/2] sunxi: restore modified memory
Changes in v3: - reorder patches - remove casts - change commit message for 'sunxi: restore modified memory' patch Changes in v2: - rename temporary variables - fix types for temporary variables Andrey Skvortsov (2): sunxi: reorganize mctl_mem_matches_* functions sunxi: restore modified memory arch/arm/include/asm/arch-sunxi/dram.h | 1 + arch/arm/mach-sunxi/dram_helpers.c | 32 +- arch/arm/mach-sunxi/dram_sunxi_dw.c| 13 --- 3 files changed, 27 insertions(+), 19 deletions(-) -- 2.42.0
[PATCH v3 1/2] sunxi: reorganize mctl_mem_matches_* functions
mctl_mem_matches and mctl_mem_matches_base identical functions. To avoid code duplication move them to dram_helpers and make mctl_mem_matches use generic mctl_mem_matches_base. Signed-off-by: Andrey Skvortsov --- arch/arm/include/asm/arch-sunxi/dram.h | 1 + arch/arm/mach-sunxi/dram_helpers.c | 20 ++-- arch/arm/mach-sunxi/dram_sunxi_dw.c| 13 - 3 files changed, 15 insertions(+), 19 deletions(-) diff --git a/arch/arm/include/asm/arch-sunxi/dram.h b/arch/arm/include/asm/arch-sunxi/dram.h index 682daae6b1..9d21b49241 100644 --- a/arch/arm/include/asm/arch-sunxi/dram.h +++ b/arch/arm/include/asm/arch-sunxi/dram.h @@ -40,5 +40,6 @@ unsigned long sunxi_dram_init(void); void mctl_await_completion(u32 *reg, u32 mask, u32 val); bool mctl_mem_matches(u32 offset); +bool mctl_mem_matches_base(u32 offset, ulong base); #endif /* _SUNXI_DRAM_H */ diff --git a/arch/arm/mach-sunxi/dram_helpers.c b/arch/arm/mach-sunxi/dram_helpers.c index cdf2750f1c..661186b648 100644 --- a/arch/arm/mach-sunxi/dram_helpers.c +++ b/arch/arm/mach-sunxi/dram_helpers.c @@ -25,19 +25,27 @@ void mctl_await_completion(u32 *reg, u32 mask, u32 val) } /* - * Test if memory at offset offset matches memory at begin of DRAM + * Test if memory at offset matches memory at a certain base * * Note: dsb() is not available on ARMv5 in Thumb mode */ #ifndef CONFIG_MACH_SUNIV -bool mctl_mem_matches(u32 offset) +bool mctl_mem_matches_base(u32 offset, ulong base) { /* Try to write different values to RAM at two addresses */ - writel(0, CFG_SYS_SDRAM_BASE); - writel(0xaa55aa55, (ulong)CFG_SYS_SDRAM_BASE + offset); + writel(0, base); + writel(0xaa55aa55, base + offset); dsb(); /* Check if the same value is actually observed when reading back */ - return readl(CFG_SYS_SDRAM_BASE) == - readl((ulong)CFG_SYS_SDRAM_BASE + offset); + return readl(base) == + readl(base + offset); +} + +/* + * Test if memory at offset matches memory at begin of DRAM + */ +bool mctl_mem_matches(u32 offset) +{ + return mctl_mem_matches_base(offset, CFG_SYS_SDRAM_BASE); } #endif diff --git a/arch/arm/mach-sunxi/dram_sunxi_dw.c b/arch/arm/mach-sunxi/dram_sunxi_dw.c index 9382d3d0be..2e8dd40b97 100644 --- a/arch/arm/mach-sunxi/dram_sunxi_dw.c +++ b/arch/arm/mach-sunxi/dram_sunxi_dw.c @@ -652,19 +652,6 @@ static int mctl_channel_init(uint16_t socid, struct dram_para *para) return 0; } -/* - * Test if memory at offset offset matches memory at a certain base - */ -static bool mctl_mem_matches_base(u32 offset, ulong base) -{ - /* Try to write different values to RAM at two addresses */ - writel(0, base); - writel(0xaa55aa55, base + offset); - dsb(); - /* Check if the same value is actually observed when reading back */ - return readl(base) == - readl(base + offset); -} static void mctl_auto_detect_dram_size_rank(uint16_t socid, struct dram_para *para, ulong base, struct rank_para *rank) { -- 2.42.0
[PATCH v3 2/2] sunxi: restore modified memory
Current sunxi DRAM initialisation code does several test accesses to the DRAM array to detect aliasing effects and so determine the correct row/column configuration. This changes the DRAM content, which breaks use cases like soft reset and Linux's ramoops mechanism. Fix this problem by saving and restoring the content of the DRAM cells that is used for the test writes. Signed-off-by: Andrey Skvortsov --- arch/arm/mach-sunxi/dram_helpers.c | 16 ++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/arch/arm/mach-sunxi/dram_helpers.c b/arch/arm/mach-sunxi/dram_helpers.c index 661186b648..e487f87bf3 100644 --- a/arch/arm/mach-sunxi/dram_helpers.c +++ b/arch/arm/mach-sunxi/dram_helpers.c @@ -32,13 +32,25 @@ void mctl_await_completion(u32 *reg, u32 mask, u32 val) #ifndef CONFIG_MACH_SUNIV bool mctl_mem_matches_base(u32 offset, ulong base) { + u32 val_base; + u32 val_offset; + bool ret; + + /* Save original values */ + val_base = readl(base); + val_offset = readl(base + offset); + /* Try to write different values to RAM at two addresses */ writel(0, base); writel(0xaa55aa55, base + offset); dsb(); /* Check if the same value is actually observed when reading back */ - return readl(base) == - readl(base + offset); + ret = readl(base) == readl(base + offset); + + /* Restore original values */ + writel(val_base, base); + writel(val_offset, base + offset); + return ret; } /* -- 2.42.0
[PATCH RFC] sunxi: pinephone: detect existed magnetometer and fixup dtb
In newer 1.2 PinePhone board revisions LIS3MDL magnetometer was replaced by AF8133J. They use the same PB1 pin in different modes. LIS3MDL uses it as an gpio input to handle interrupt. AF8133J uses it as an gpio output as a reset signal. It wasn't possible at runtime to enable both device tree nodes and detect supported sensor at probe time. AF8133J has reset pin (PB1) connected to the SoC. By default AF8133J is in a reset state and don't respond to probe request on I2C bus. Extra code would be needed to handle reset signal. Therefore this code uses LIS3MDL magnetometer instead of AF8133J. Introducing new dts 1.2b with AF8133J sensor would require probing in SPL. That would lead to pulling in into SPL I2C controller driver, RSB controller driver, introducing new AXP803 driver to power-up sensors for probe. It's working, but SPL is pretty size-constrained on A64 and doesn't have much space. Therefore fdt fixup is done in U-Boot proper without introducing new board revision and new dts. Signed-off-by: Andrey Skvortsov --- AF8133J's driver isn't upstreamed yet, but it will be soon. Therefore this is RFC patch. I'd like to know whether selected approach will be accepted in u-boot before submiting coresponding dts changes to the Linux kernel. Any feedback on this change would be very welcome. board/sunxi/board.c | 26 ++ configs/pinephone_defconfig | 1 + 2 files changed, 27 insertions(+) diff --git a/board/sunxi/board.c b/board/sunxi/board.c index 1305302060..a4bfa24400 100644 --- a/board/sunxi/board.c +++ b/board/sunxi/board.c @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -920,6 +921,28 @@ static void bluetooth_dt_fixup(void *blob) "local-bd-address", bdaddr, ETH_ALEN, 1); } +#ifdef CONFIG_PINEPHONE_DT_SELECTION + +#define PINEPHONE_LIS3MDL_I2C_ADDR 0x1E +#define PINEPHONE_LIS3MDL_I2C_BUS 1 /* I2C1 */ + +static void board_dt_fixup(void *blob) +{ + struct udevice *bus, *dev; + + if (!fdt_node_check_compatible(blob, 0, "pine64,pinephone-1.2")) + if (!uclass_get_device_by_seq(UCLASS_I2C, PINEPHONE_LIS3MDL_I2C_BUS, &bus)) { + dm_i2c_probe(bus, PINEPHONE_LIS3MDL_I2C_ADDR, 0, &dev); + fdt_set_status_by_compatible( + blob, "st,lis3mdl-magn", + dev ? FDT_STATUS_OKAY : FDT_STATUS_DISABLED); + fdt_set_status_by_compatible( + blob, "voltafield,af8133j", + dev ? FDT_STATUS_DISABLED : FDT_STATUS_OKAY); + } +} +#endif + int ft_board_setup(void *blob, struct bd_info *bd) { int __maybe_unused r; @@ -934,6 +957,9 @@ int ft_board_setup(void *blob, struct bd_info *bd) bluetooth_dt_fixup(blob); +#ifdef CONFIG_PINEPHONE_DT_SELECTION + board_dt_fixup(blob); +#endif #ifdef CONFIG_VIDEO_DT_SIMPLEFB r = sunxi_simplefb_setup(blob); if (r) diff --git a/configs/pinephone_defconfig b/configs/pinephone_defconfig index eebc676901..457e7ee1e7 100644 --- a/configs/pinephone_defconfig +++ b/configs/pinephone_defconfig @@ -12,6 +12,7 @@ CONFIG_MMC_SUNXI_SLOT_EXTRA=2 CONFIG_PINEPHONE_DT_SELECTION=y # CONFIG_SYS_MALLOC_CLEAR_ON_INIT is not set CONFIG_OF_LIST="sun50i-a64-pinephone-1.1 sun50i-a64-pinephone-1.2" +CONFIG_SYS_I2C_MVTWSI=y CONFIG_LED_STATUS=y CONFIG_LED_STATUS_GPIO=y CONFIG_LED_STATUS0=y -- 2.43.0
[PATCH] sunxi: defconfig: Add pstore support for pinephone
In general any DRAM address, that isn't overwritten during a boot is suitable for pstore. Range from 0x4000 - 0x5000 is heavily used by u-boot for internal use and to load kernel, fdt, fdto, scripts, pxefile and ramdisk later in the boot process. Ramdisk start address is 0x4FF0, initramfs for kernel with some hacking features and debug info enabled can take more than 100Mb and final address will be around 0x5800. Address 0x6100 will most likely not overlap with that. Signed-off-by: Andrey Skvortsov --- configs/pinephone_defconfig | 2 ++ 1 file changed, 2 insertions(+) diff --git a/configs/pinephone_defconfig b/configs/pinephone_defconfig index 9d39204a43..7b80cc3131 100644 --- a/configs/pinephone_defconfig +++ b/configs/pinephone_defconfig @@ -10,6 +10,8 @@ CONFIG_DRAM_ZQ=3881949 CONFIG_MMC_SUNXI_SLOT_EXTRA=2 CONFIG_PINEPHONE_DT_SELECTION=y # CONFIG_SYS_MALLOC_CLEAR_ON_INIT is not set +CONFIG_CMD_PSTORE=y +CONFIG_CMD_PSTORE_MEM_ADDR=0x6100 CONFIG_OF_LIST="sun50i-a64-pinephone-1.1 sun50i-a64-pinephone-1.2" CONFIG_LED_STATUS=y CONFIG_LED_STATUS_GPIO=y -- 2.43.0
Re: [PATCH] sunxi: defconfig: Add pstore support for pinephone
Hi Peter, On 24-02-11 16:11, Peter Robinson wrote: > On Sun, 11 Feb 2024 at 16:02, Andrey Skvortsov > wrote: > > > > In general any DRAM address, that isn't overwritten during a boot is > > suitable for pstore. > > What's this functionality providing? The details below give the > technical reasons for the memory address but I'm not sure what > enabling pstore provides to PinePhone users. > Good point. pstore allows users to catch kernel crashes and report them to developers. It's enabled by default in distribution kernels like Debian. CONFIG_PSTORE=y CONFIG_PSTORE_RAM=m systemd has service that automatically handles pstore and saves them in /var/lib/pstore for later usage. Modern (Android) phones have pstore enabled usually to get information about kernel crash, since it's the simplest way to get kernel backtrace on mobile device without serial console. > > Range from 0x4000 - 0x5000 is heavily used by u-boot for > > internal use and to load kernel, fdt, fdto, scripts, pxefile and ramdisk > > later in the boot process. Ramdisk start address is 0x4FF0, > > initramfs for kernel with some hacking features and debug info enabled > > can take more than 100Mb and final address will be around 0x5800. > > Address 0x6100 will most likely not overlap with that. > > > > Signed-off-by: Andrey Skvortsov > > --- > > configs/pinephone_defconfig | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/configs/pinephone_defconfig b/configs/pinephone_defconfig > > index 9d39204a43..7b80cc3131 100644 > > --- a/configs/pinephone_defconfig > > +++ b/configs/pinephone_defconfig > > @@ -10,6 +10,8 @@ CONFIG_DRAM_ZQ=3881949 > > CONFIG_MMC_SUNXI_SLOT_EXTRA=2 > > CONFIG_PINEPHONE_DT_SELECTION=y > > # CONFIG_SYS_MALLOC_CLEAR_ON_INIT is not set > > +CONFIG_CMD_PSTORE=y > > +CONFIG_CMD_PSTORE_MEM_ADDR=0x6100 > > CONFIG_OF_LIST="sun50i-a64-pinephone-1.1 sun50i-a64-pinephone-1.2" > > CONFIG_LED_STATUS=y > > CONFIG_LED_STATUS_GPIO=y > > -- > > 2.43.0 > > -- Best regards, Andrey Skvortsov
Re: [PATCH RFC] sunxi: pinephone: detect existed magnetometer and fixup dtb
Hi Andre, thank you for the valuable feedback! On 24-02-11 13:13, Andre Przywara wrote: > On Sun, 11 Feb 2024 12:28:24 +0300 > Andrey Skvortsov wrote: > > Hi Andrey, > > thanks for taking care of this upstream! > > > In newer 1.2 PinePhone board revisions LIS3MDL magnetometer was replaced by > > AF8133J. They use the same PB1 pin in different modes. > > > > AF8133J's driver isn't upstreamed yet, but it will be soon. Therefore this > > is RFC patch. I'd like to know whether selected approach will be accepted > > in u-boot before submiting coresponding dts changes to the Linux kernel. > > Any feedback on this change would be very welcome. > > So I think this is the right approach: Since the SPL has no use of this > device, and it's a rather small DT change, we definitely want to do this > in U-Boot proper. And I believe that U-Boot (proper) is indeed the best > place to do those adjustments, since it's built for one board anyway, > so anything board specific belongs into there (and not into TF-A or the > SPL, which are more tailored to one *SoC*). Also we are not so size > sensitive in proper. > And I also like the fact that it's protected by a board specific > definition, and even better that it's an already existing one. > > Oh, and I am not really convinced this is the right approach here, but > maybe have a look at doc/usage/cmd/extension.rst, for another related > mechanism. Thanks for the tip. I've looked at it after your suggestion and tried to implement the same logic using extension command. Here are my thoughts about that: 1. integrated magnetometer is part of the device and making it extension with separate dtbo sounds a bit strange. 2. if I'd create dtbo for new AF8133J magnetometer, then I'd call it like other dts files for the device: 'sun50i-a64-pinephone-1.2-af8133j.dtbo. This is 38 characters and currently overlay name is limited to 32 bytes. What do you think about increasing overlay name? 3. extensions are not supported for extlinux boot flow at all. This is Debian case, that I'm working with. And this is a major problem for me. 4. I've looked how EFI boot flow is made and I see that extensions are not applied to fdtcontroladdr, only to loaded fdt to fdt_addr_r. Extlinux uses fdtcontroladdr always. 5. When distribution supplies fdt with extlinux, when supplied fdt is used and no extensions are applied to it. This is my case as well. 6. I can't apply extensions to fdtcontroladdr. When I've tried to apply working extension to fdtcontroladdr, then I get a crash. I have to copy fdt from fdtcontroladdr to fdt_addr_r and then apply extension to fdt_addr_r and when it works. Maybe this is something sunxi-specific. Overall extensions seems like a nice feature for capes, extension boards for pogo pins and so on. But I'm not sure, that it's the right choice in this case. > Some smaller comments below ... > > > > > board/sunxi/board.c | 26 ++ > > configs/pinephone_defconfig | 1 + > > 2 files changed, 27 insertions(+) > > > > diff --git a/board/sunxi/board.c b/board/sunxi/board.c > > index 1305302060..a4bfa24400 100644 > > --- a/board/sunxi/board.c > > +++ b/board/sunxi/board.c > > @@ -15,6 +15,7 @@ > > #include > > #include > > #include > > +#include > > #include > > #include > > #include > > @@ -920,6 +921,28 @@ static void bluetooth_dt_fixup(void *blob) > >"local-bd-address", bdaddr, ETH_ALEN, 1); > > } > > > > +#ifdef CONFIG_PINEPHONE_DT_SELECTION > > Can you move that line into the function? That would for one avoid the > protection in the caller below, and secondly make it easier to add > other boards, if a need arises for them. The two #defines don't hurt or > change the code, so just keep them outside. > > + > > +#define PINEPHONE_LIS3MDL_I2C_ADDR 0x1E > > +#define PINEPHONE_LIS3MDL_I2C_BUS 1 /* I2C1 */ > > + > > +static void board_dt_fixup(void *blob) > > +{ > > + struct udevice *bus, *dev; > > + > > (have the #ifdef here) Ok, then I mark local variables with __maybe_unused and remove #ifdef at board_dt_fixup call. > > > + if (!fdt_node_check_compatible(blob, 0, "pine64,pinephone-1.2")) > > Please have curly braces around that. I'll fix that in v2. > So I'd say please send the (disabled) DT node addition patch to the > kernel MLs, then send this patch to U-Boot. Do you mean patch with disabled AF8133J DT node? Right? If so, then it was the plan. 1. upstream AF8133J driver to the Linux kernel (on-going) 2. find accep
[U-Boot] [PATCH] kconfig: remove duplicated CMD_DNS option
two CMD_DNS options were added by commit 60296a835cb17 ("commands: add more command entries in Kconfig") Signed-off-by: Andrey Skvortsov Cc: Masahiro Yamada --- common/Kconfig | 5 - 1 file changed, 5 deletions(-) diff --git a/common/Kconfig b/common/Kconfig index 4cde4b0..ea0d0ef 100644 --- a/common/Kconfig +++ b/common/Kconfig @@ -300,11 +300,6 @@ config CMD_DNS help Lookup the IP of a hostname -config CMD_DNS - bool "dns" - help - Lookup the IP of a hostname - config CMD_LINK_LOCAL bool "linklocal" help -- 2.1.4 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH] imx_watchdog: always set minimal timeout in reset_cpu
The problem is that timeout bits in WCR register were leaved unchanged. So previously set timeout value was applied and therefore 'reset' command takes any value up to two minutes, depending on previous watchdog settings, instead of minimal 0.5 seconds. Signed-off-by: Andrey Skvortsov --- drivers/watchdog/imx_watchdog.c | 2 +- include/fsl_wdog.h | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/watchdog/imx_watchdog.c b/drivers/watchdog/imx_watchdog.c index 0d77595..f9f8175 100644 --- a/drivers/watchdog/imx_watchdog.c +++ b/drivers/watchdog/imx_watchdog.c @@ -43,7 +43,7 @@ void reset_cpu(ulong addr) { struct watchdog_regs *wdog = (struct watchdog_regs *)WDOG1_BASE_ADDR; - clrsetbits_le16(&wdog->wcr, 0, WCR_WDE); + clrsetbits_le16(&wdog->wcr, WCR_WT_MSK, WCR_WDE); writew(0x, &wdog->wsr); writew(0x, &wdog->wsr); /* load minimum 1/2 second timeout */ diff --git a/include/fsl_wdog.h b/include/fsl_wdog.h index d15a70c..f698d4d 100644 --- a/include/fsl_wdog.h +++ b/include/fsl_wdog.h @@ -16,3 +16,4 @@ struct watchdog_regs { #define WCR_WDT0x08 #define WCR_SRS0x10 #define SET_WCR_WT(x) (x << 8) +#define WCR_WT_MSK SET_WCR_WT(0xFF) -- 2.6.4 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[PATCH v2] sunxi: defconfig: Add pstore support for pinephone
pstore will allow users to catch kernel crashes and report them to developers. Modern (Android) phones have pstore usually enabled to get information about kernel crash, since it's the simplest way to get kernel backtrace on mobile device without serial console. Usually it's enabled by default in distribution kernels like Debian. CONFIG_PSTORE=y CONFIG_PSTORE_RAM=m systemd has service that automatically handles pstore and saves them in /var/lib/pstore for later usage. In general any DRAM address, that isn't overwritten during a boot is suitable for pstore. Range from 0x4000 - 0x5000 is heavily used by u-boot for internal use and to load kernel, fdt, fdto, scripts, pxefile and ramdisk later in the boot process. Ramdisk start address is 0x4FF0, initramfs for kernel with some hacking features and debug info enabled can take more than 100Mb and final address will be around 0x5800. Address 0x6100 will most likely not overlap with that. Signed-off-by: Andrey Skvortsov --- Changes since v1: - updated description with pstore usage details configs/pinephone_defconfig | 2 ++ 1 file changed, 2 insertions(+) diff --git a/configs/pinephone_defconfig b/configs/pinephone_defconfig index 9d39204a439..7b80cc3131b 100644 --- a/configs/pinephone_defconfig +++ b/configs/pinephone_defconfig @@ -10,6 +10,8 @@ CONFIG_DRAM_ZQ=3881949 CONFIG_MMC_SUNXI_SLOT_EXTRA=2 CONFIG_PINEPHONE_DT_SELECTION=y # CONFIG_SYS_MALLOC_CLEAR_ON_INIT is not set +CONFIG_CMD_PSTORE=y +CONFIG_CMD_PSTORE_MEM_ADDR=0x6100 CONFIG_OF_LIST="sun50i-a64-pinephone-1.1 sun50i-a64-pinephone-1.2" CONFIG_LED_STATUS=y CONFIG_LED_STATUS_GPIO=y -- 2.45.2
[PATCH] sunxi: pinephone: detect existed magnetometer and fixup dtb
In newer 1.2 PinePhone board revisions LIS3MDL magnetometer was replaced by AF8133J. They use the same PB1 pin in different modes. LIS3MDL uses it as an gpio input to handle interrupt. AF8133J uses it as an gpio output as a reset signal. It wasn't possible at runtime to enable both device tree nodes and detect supported sensor at probe time. AF8133J has reset pin (PB1) connected to the SoC. By default AF8133J is in a reset state and don't respond to probe request on I2C bus. Extra code would be needed to handle reset signal. Therefore this code uses LIS3MDL magnetometer instead of AF8133J. Introducing new dts 1.2b with AF8133J sensor would require probing in SPL. That would lead to pulling in into SPL I2C controller driver, RSB controller driver, introducing new AXP803 driver to power-up sensors for probe. It's working, but SPL is pretty size-constrained on A64 and doesn't have much space. Therefore fdt fixup is done in U-Boot proper without introducing new board revision and new dts. Signed-off-by: Andrey Skvortsov Link: https://lore.kernel.org/all/20240908214718.36316-1-andrej.skvort...@gmail.com/ Link: https://lists.denx.de/pipermail/u-boot/2024-February/545700.html --- This patch was discussed previously as RFC. [1] All relevant kernel device-tree changes are merged. [2] 1. https://lists.denx.de/pipermail/u-boot/2024-February/545700.html 2. https://lore.kernel.org/all/20240908214718.36316-1-andrej.skvort...@gmail.com/ board/sunxi/board.c | 23 +++ configs/pinephone_defconfig | 1 + 2 files changed, 24 insertions(+) diff --git a/board/sunxi/board.c b/board/sunxi/board.c index 824c322a0dc..703081b403f 100644 --- a/board/sunxi/board.c +++ b/board/sunxi/board.c @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -876,6 +877,27 @@ static void bluetooth_dt_fixup(void *blob) "local-bd-address", bdaddr, ETH_ALEN, 1); } +#define PINEPHONE_LIS3MDL_I2C_ADDR 0x1E +#define PINEPHONE_LIS3MDL_I2C_BUS 1 /* I2C1 */ + +static void board_dt_fixup(void *blob) +{ + struct udevice *bus, *dev; + + if (IS_ENABLED(CONFIG_PINEPHONE_DT_SELECTION) && + !fdt_node_check_compatible(blob, 0, "pine64,pinephone-1.2")) { + if (!uclass_get_device_by_seq(UCLASS_I2C, PINEPHONE_LIS3MDL_I2C_BUS, &bus)) { + dm_i2c_probe(bus, PINEPHONE_LIS3MDL_I2C_ADDR, 0, &dev); + fdt_set_status_by_compatible( + blob, "st,lis3mdl-magn", + dev ? FDT_STATUS_OKAY : FDT_STATUS_DISABLED); + fdt_set_status_by_compatible( + blob, "voltafield,af8133j", + dev ? FDT_STATUS_DISABLED : FDT_STATUS_OKAY); + } + } +} + int ft_board_setup(void *blob, struct bd_info *bd) { int __maybe_unused r; @@ -889,6 +911,7 @@ int ft_board_setup(void *blob, struct bd_info *bd) fdt_fixup_ethernet(blob); bluetooth_dt_fixup(blob); + board_dt_fixup(blob); #ifdef CONFIG_VIDEO_DT_SIMPLEFB r = sunxi_simplefb_setup(blob); diff --git a/configs/pinephone_defconfig b/configs/pinephone_defconfig index 9d39204a439..0f9ab0c4662 100644 --- a/configs/pinephone_defconfig +++ b/configs/pinephone_defconfig @@ -11,6 +11,7 @@ CONFIG_MMC_SUNXI_SLOT_EXTRA=2 CONFIG_PINEPHONE_DT_SELECTION=y # CONFIG_SYS_MALLOC_CLEAR_ON_INIT is not set CONFIG_OF_LIST="sun50i-a64-pinephone-1.1 sun50i-a64-pinephone-1.2" +CONFIG_SYS_I2C_MVTWSI=y CONFIG_LED_STATUS=y CONFIG_LED_STATUS_GPIO=y CONFIG_LED_STATUS0=y -- 2.45.2