Re: [PATCH v4 02/10] pinctrl: k1: add IO power domain configuration support

["Junhui Liu" <junhui.liu@pigmoral.tech>]

Hi Eric,

Thanks for your patch. However, I found a couple of issues while
testing.

On Tue Jul 7, 2026 at 11:21 PM CST, Eric Chung wrote:
> Dual-voltage GPIO banks default to 3.3V, but when externally supplied
> with 1.8V the internal logic must be explicitly reconfigured to match.
>
> Add the ability to program IO power domain control registers through the
> APBC block. These registers require unlocking the AIB Secure Access
> Register (ASAR) before every read/write, since configuring a 1.8V domain
> while 3.3V is externally supplied can cause back-powering and pin damage.
>
> Signed-off-by: Eric Chung <eric.chung@riscstar.com>
>
> ---
> v3:
> - Add SYSCON dependency in Kconfig.
> - Fix not sorted issue in the driver.
> ---
>  drivers/pinctrl/spacemit/Kconfig      |  2 +-
>  drivers/pinctrl/spacemit/pinctrl-k1.c | 83 ++++++++++++++++++++++++++++++++++-
>  2 files changed, 82 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pinctrl/spacemit/Kconfig b/drivers/pinctrl/spacemit/Kconfig
> index 6aab89e160c..ff754f5839c 100644
> --- a/drivers/pinctrl/spacemit/Kconfig
> +++ b/drivers/pinctrl/spacemit/Kconfig
> @@ -1,6 +1,6 @@
>  config PINCTRL_SPACEMIT_K1
>  	bool "Spacemit K1 SoC pinctrl driver"
> -	depends on PINCTRL_GENERIC && DM
> +	depends on PINCTRL_GENERIC && DM && SYSCON
>  	help
>  	  Supports pin multiplexing control on Spacemit K1 SoCs.
>  
> diff --git a/drivers/pinctrl/spacemit/pinctrl-k1.c b/drivers/pinctrl/spacemit/pinctrl-k1.c
> index 3ebc397213b..95344f4ec72 100644
> --- a/drivers/pinctrl/spacemit/pinctrl-k1.c
> +++ b/drivers/pinctrl/spacemit/pinctrl-k1.c

[...]

> +
>  static int spacemit_pinconf_set(struct udevice *dev, unsigned int pin_selector,
>  				unsigned int param, unsigned int argument)
>  {
> @@ -456,6 +526,9 @@ static int spacemit_pinconf_set(struct udevice *dev, unsigned int pin_selector,
>  			dev_err(dev, "Invalid power source (%d)\n", argument);
>  			return -EINVAL;
>  		}
> +		if (found)
> +			spacemit_set_io_power_domain(dev, pin_selector,

CONFIG_PINCONF is not enabled in spacemit_k1_defconfig. Therefore, with
the default configuration, the generic pinctrl code skips pin
configuration properties and spacemit_pinconf_set() is never called.
Consequently, the IO power domain configuration added here is not
exercised when building with spacemit_k1_defconfig.

> +						     priv->io_pins[i].io_type);
>  		break;
>  	default:
>  		return -EOPNOTSUPP;
> @@ -485,6 +558,11 @@ static int spacemit_pinctrl_probe(struct udevice *dev)
>  		dev_err(dev, "Fail to allocate memory\n");
>  		return -ENOMEM;
>  	}
> +	priv->regmap = syscon_regmap_lookup_by_phandle(dev, "spacemit,apbc");
> +	if (IS_ERR(priv->regmap)) {
> +		dev_warn(dev, "no syscon found, disable IO power domain switching\n");
> +		priv->regmap = NULL;
> +	}

The APBC node with compatible "spacemit,k1-syscon-apbc" is bound by
drivers/clk/spacemit/clk-k1.c as a UCLASS_CLK device. It is not
registered as a UCLASS_SYSCON device, and the node does not have the
"syscon" compatible required by the fallback path in
syscon_regmap_lookup_by_phandle(). Therefore, this lookup fails and
priv->regmap is set to NULL.

I added a direct MMIO write to the UART registers here to confirm the
failure path. The original dev_warn() is not visible because this probe
happens before the normal console is initialized.

To fix this, I suggest either making the clock driver register or expose
the APBC regmap through the syscon infrastructure, or avoiding syscon
here and obtaining the regmap directly using dev_read_phandle_with_args()
followed by regmap_init_mem().

>  
>  	ret = clk_get_bulk(dev, &clks);
>  	if (ret) {
> @@ -512,6 +590,7 @@ static const struct spacemit_pinctrl_data k1_pinctrl_data = {
>  	.get_pins	= k1_get_pins,
>  	.get_functions	= k1_get_functions,
>  	.get_io_type	= k1_get_io_type,
> +	.pin_to_io_pd_offset = spacemit_k1_pin_to_io_pd_offset,
>  };
>  
>  static const struct udevice_id spacemit_pinctrl_ids[] = {

-- 
Best regards,
Junhui Liu