Hi Jonas, On Thu, 30 Jan 2025 at 23:20, Jonas Karlman <jo...@kwiboo.se> wrote: > > 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.
Ok, I'll post this after the weekend in case I get more review comments. Keep in mind that i'll hide it behind a Kconfig option for now, since we seem to have enough cases of code writing const variables Cheers /Ilias > > Regards, > Jonas > > > > > Cheers, > > Quentin >
