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.sch...@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.apalodi...@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
