Re: [PATCH 6.12.y-cip 10/23] pinctrl: renesas: rzg2l: Add support for selecting power source for {WDT,AWO,ISO}

[Pavel Machek <pavel@nabladev.com>]

[-- Attachment #1: Type: text/plain, Size: 2173 bytes --]

Hi!

> The RZ/G3L SoC has support for setting power source that are not
> controlled by the following voltage control registers:
>   - SD_CH{0,1,2}_POC, XSPI_POC, ETH{0,1}_POC, I3C_SET.POC
> 
> Add support for selecting voltages using OTHER_POC register for
> setting I/O domain voltage for WDT, ISO and AWO by extending
> rzg2l_caps_to_pwr_reg() with a mask output parameter so that callers
> callers can identify which bit(s) within OTHER_POC correspond to the
> requested domain. Update rzg2l_get_power_source() to extract the
> relevant bit field via field_get() when reading OTHER_POC, and update
> rzg2l_set_power_source() to perform a read-modify-write under the
> spinlock when writing to OTHER_POC, since multiple domains share the
> same register.

I believe more robustness is needed here.

> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -958,25 +983,37 @@ static int rzg2l_set_power_source(struct rzg2l_pinctrl *pctrl, u32 pin, u32 caps
>  
>  	switch (ps) {
>  	case 1800:
> -		val = PVDD_1800;
> +		poc_val = PVDD_1800;
>  		break;
>  	case 2500:
>  		if (!(caps & (PIN_CFG_IO_VMC_ETH0 | PIN_CFG_IO_VMC_ETH1)))
>  			return -EINVAL;
> -		val = PVDD_2500;
> +		poc_val = PVDD_2500;
>  		break;
>  	case 3300:
> -		val = PVDD_3300;
> +		poc_val = PVDD_3300;
>  		break;
>  	default:
>  		return -EINVAL;
>  	}
>  
> -	pwr_reg = rzg2l_caps_to_pwr_reg(regs, caps);
> +	pwr_reg = rzg2l_caps_to_pwr_reg(regs, caps, &mask);
>  	if (pwr_reg < 0)
>  		return pwr_reg;
>  
> -	writeb(val, pctrl->base + pwr_reg);
> +	if (pwr_reg == OTHER_POC) {
> +		scoped_guard(raw_spinlock, &pctrl->lock) {
> +			val = readb(pctrl->base + pwr_reg);
> +			if (poc_val)
> +				val |= mask;
> +			else
> +				val &= ~mask;
> +			writeb(val, pctrl->base + pwr_reg);
> +		}

This is strange. You test that ps is right ammount of milivolts, but
here you just ignore poc_val, no matter if user selected 1800, 2500 or
3300mV.

I believe you should cross-check values here, so that if user requests
1800mV, he does not silenly get 3300mV (or vice versa, I'm not sure
how the hardware work).

Best regards,
								Pavel

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]