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

[Quentin Schulz]

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?

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?

Cheers,
Quentin