[PATCH 00/14] pinctrl: rockchip: fix editing const struct + constify rockchip_pin_ctrl

[Jonas Karlman]

Hi Quentin,

On 2025-01-30 15:16, Quentin Schulz wrote:
> Hi Jonas,
> 
> On 1/29/25 3:20 PM, Jonas Karlman wrote:
>> Hi Quentin,
>>
>> On 2025-01-29 13:42, Quentin Schulz wrote:
>>> While testing some WIP work done by Ilias Apalodimas on guaranteeing
>>> read-only memory areas truly are handled as read-only[1], my RK3588
>>> Tiger couldn't reach U-Boot CLI anymore because of the pinctrl driver
>>> modifying a const struct rockchip_pin_ctrl, triggering a CPU abort.
>>>
>>> Instead of going the lazy way and unconstify it, let's fix the actual
>>> issue in play.
>>>
>>> The member modified in the const is only ever used for setting a member
>>> from another struct (not const that one). However this other member is
>>> never read! Therefore we can simply afford to remove it which means the
>>> sole reader of the member in the const is now gone, thus making it
>>> possible to remove the member from the const struct as well.
>>>
>>> This also means we should be able to constify the private data of the
>>> controller device for all Rockchip devices, instead of having those only
>>> for RK356x, RK3588 and RV1126. With the constify done on top of Ilias
>>> branch[1], my PX30 Ringneck, RK3399 Puma and RK3588 Tiger all reach
>>> U-Boot CLI. No further test (like booting into Linux userspace) was
>>> done.
>>>
>>> [1] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/tree/fix_memory_permissions
>>>
>>> Signed-off-by: Quentin Schulz <quentin.schulz at cherry.de>
>>> ---
>>> Quentin Schulz (14):
>>>        pinctrl: rockchip: remove unused base_pin bank member
>>>        pinctrl: rockchip: remove unused nr_pins controller member
>>
>> pin_base and nr_pins is used after my "rockchip: pinctrl: Add support
>> for pinmux status cmd" series [1].
>>
>> The pin_base should probably be moved to udevice priv data or similar.
>> nr_pins can probably also be moved to udevice priv data or constify in
>> driver data.
>>
>> Do you want me to re-work/re-store these fields in a different way once
>> I finally send a v3 of that series?
>>
> 
> What I can suggest is to remove the setting of ctrl->nr_pins as I 
> believe we shouldn't be modifying it and we don't need to store it, even 
> in your patch series as we discussed on IRC, and keep pin_base from the 
> bank.
> 
> What I can offer instead is to use a local variable to replace the one 
> from the controller struct.
> 
> """
> diff --git a/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c 
> b/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c
> index d449d07d32e..14aba6f370b 100644
> --- a/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c
> +++ b/drivers/pinctrl/rockchip/pinctrl-rockchip-core.c
> @@ -531,20 +531,22 @@ static struct rockchip_pin_ctrl 
> *rockchip_pinctrl_get_soc_data(struct udevice *d
>   	struct rockchip_pin_ctrl *ctrl =
>   			(struct rockchip_pin_ctrl *)dev_get_driver_data(dev);
>   	struct rockchip_pin_bank *bank;
> -	int grf_offs, pmu_offs, drv_grf_offs, drv_pmu_offs, i, j;
> +	int grf_offs, pmu_offs, drv_grf_offs, drv_pmu_offs, ctrl_nr_pins, i, j;
> 
>   	grf_offs = ctrl->grf_mux_offset;
>   	pmu_offs = ctrl->pmu_mux_offset;
>   	drv_pmu_offs = ctrl->pmu_drv_offset;
>   	drv_grf_offs = ctrl->grf_drv_offset;
>   	bank = ctrl->pin_banks;
> +	ctrl_nr_pins = 0;
> +
> 
>   	for (i = 0; i < ctrl->nr_banks; ++i, ++bank) {
>   		int bank_pins = 0;
> 
>   		bank->priv = priv;
> -		bank->pin_base = ctrl->nr_pins;
> -		ctrl->nr_pins += bank->nr_pins;
> +		bank->pin_base = ctrl_nr_pins;
> +		ctrl_nr_pins += bank->nr_pins;
> 
>   		/* calculate iomux and drv offsets */
>   		for (j = 0; j < 4; j++) {
> """
> 
> Something like that to replace patches 1 and 2. This should allow you to 
> keep working on your other patch series without me making it too much 
> more difficult for you to rebase?

Thanks for taking a look, and I agree this should help when I pick up
and re-spin my series.

> 
> We need this patch series in before 
> https://lore.kernel.org/u-boot/20250130072100.27297-1-ilias.apalodimas@linaro.org/T/#t 
> gets is otherwise we don't be able to boot anymore on RK356x, RK3588 and 
> RV1126. I don't think there's going to be much bikeshedding or 
> controversy about my series here (in v2 if you agree with the above 
> diff), the pinctrl series from last August seems like it could be 
> opening many can of worms so better not rebase this series on top of 
> that series. Do you agree with that?

I fully agree, better to get this in first, I will try to re-spin my
series on top of this sometime later once this has landed.

Regards,
Jonas

> 
> Cheers,
> Quentin