[PATCH] pinctrl: rockchip: Fix Data Abort exception in SPL

[Kever Yang]

On 2023/6/8 18:59, Jonas Karlman wrote:
> Using CONFIG_ARMV8_SPL_EXCEPTION_VECTORS=y and CONFIG_OF_LIVE=y triggers
> a Data Abort exception from unaligned memory access when the pinctrl
> driver iterate node properties, e.g. for UART2 on RK3568.
>
>    setting mux of GPIO0-24 to 1
>    setting mux of GPIO0-24 to 1
>    "Synchronous Abort" handler, esr 0x96000021
>    elr: 000000000000e554 lr : 000000000000e54c
>    x 0: 0000000000000a5c x 1: 0000000000000a5c
>    x 2: 0000000000000007 x 3: 0000000000000065
>    x 4: 0000000000000007 x 5: 0000000000022d4e
>    x 6: 0000000000000a7c x 7: 00000000000227a4
>    x 8: 0000000000021cf0 x 9: 0000000000000a7c
>    x10: 0000000000021cf0 x11: 0000000000021cf0
>    x12: 00000000003fda1c x13: 0000000000000007
>    x14: 00000000003fd9ec x15: 000000000001c0ff
>    x16: 0000000007000000 x17: 00000000fdccd028
>    x18: 00000000003fde20 x19: 0000000000000018
>    x20: 0000000000020670 x21: 0000000000000000
>    x22: 00000000003fdb00 x23: 00000000003fef90
>    x24: 0000000000020688 x25: 0000000000000000
>    x26: 0000000000000001 x27: 00000000003ffc50
>    x28: 0000000000000000 x29: 00000000003fda60
>
>    Code: b94083e1 97ffd508 93407c01 37f81260 (f9401038)
>    Resetting CPU ...
>
> Fix this by replacing the loop to access node properties with use of
> ofnode_for_each_prop instead of the current ifdef.
>
> Also continue to next prop instead of aborting at first sign of an
> unknown property.
>
> This fixes the Data Abort exception and also pinconf of e.g. pull and
> drive in SPL, e.g. for UART2 on RK3568.
>
>    setting mux of GPIO0-24 to 1
>    setting mux of GPIO0-24 to 1
>    setting pull of GPIO0-24 to 5
>    setting mux of GPIO0-25 to 1
>    setting mux of GPIO0-25 to 1
>    setting pull of GPIO0-25 to 5
>
> Fixes: e7ae4cf27a6d ("pinctrl: rockchip: Add common rockchip pinctrl driver")
> Signed-off-by: Jonas Karlman <jonas at kwiboo.se>
Reviewed-by: Kever Yang <kever.yang at rock-chips.com>

Thanks,
- Kever
> ---
>   .../pinctrl/rockchip/pinctrl-rockchip-core.c  | 28 ++++---------------
>   1 file changed, 6 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c b/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c
> index d9d61fdb726a..8ef089994f46 100644
> --- a/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c
> +++ b/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c
> @@ -12,7 +12,6 @@
>   #include <fdtdec.h>
>   #include <linux/bitops.h>
>   #include <linux/libfdt.h>
> -#include <asm/global_data.h>
>   
>   #include "pinctrl-rockchip.h"
>   
> @@ -433,13 +432,7 @@ static int rockchip_pinctrl_set_state(struct udevice *dev,
>   	int prop_len, param;
>   	const u32 *data;
>   	ofnode node;
> -#ifdef CONFIG_OF_LIVE
> -	const struct device_node *np;
> -	struct property *pp;
> -#else
> -	int property_offset, pcfg_node;
> -	const void *blob = gd->fdt_blob;
> -#endif
> +	struct ofprop prop;
>   	data = dev_read_prop(config, "rockchip,pins", &count);
>   	if (count < 0) {
>   		debug("%s: bad array size %d\n", __func__, count);
> @@ -473,24 +466,15 @@ static int rockchip_pinctrl_set_state(struct udevice *dev,
>   		node = ofnode_get_by_phandle(conf);
>   		if (!ofnode_valid(node))
>   			return -ENODEV;
> -#ifdef CONFIG_OF_LIVE
> -		np = ofnode_to_np(node);
> -		for (pp = np->properties; pp; pp = pp->next) {
> -			prop_name = pp->name;
> -			prop_len = pp->length;
> -			value = pp->value;
> -#else
> -		pcfg_node = ofnode_to_offset(node);
> -		fdt_for_each_property_offset(property_offset, blob, pcfg_node) {
> -			value = fdt_getprop_by_offset(blob, property_offset,
> -						      &prop_name, &prop_len);
> +		ofnode_for_each_prop(prop, node) {
> +			value = ofprop_get_property(&prop, &prop_name, &prop_len);
>   			if (!value)
> -				return -ENOENT;
> -#endif
> +				continue;
> +
>   			param = rockchip_pinconf_prop_name_to_param(prop_name,
>   								    &default_val);
>   			if (param < 0)
> -				break;
> +				continue;
>   
>   			if (prop_len >= sizeof(fdt32_t))
>   				arg = fdt32_to_cpu(*(fdt32_t *)value);