Re: [PATCH v28 05/11] soc: mediatek: refine code to use mtk_mmsys_update_bits API

[Matthias Brugger <matthias.bgg@gmail.com>]

On 08/11/2022 20:43, Nícolas F. R. A. Prado wrote:
> On Tue, Nov 08, 2022 at 06:37:19PM +0100, Matthias Brugger wrote:
>>
>>
>> On 07/11/2022 08:22, Nancy.Lin wrote:
>>> Simplify code for update  mmsys reg.
>>>
>>> Signed-off-by: Nancy.Lin <nancy.lin@mediatek.com>
>>> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>> Reviewed-by: CK Hu <ck.hu@mediatek.com>
>>> Tested-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>> Tested-by: Bo-Chen Chen <rex-bc.chen@mediatek.com>
>>> Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
>>> ---
>>>    drivers/soc/mediatek/mtk-mmsys.c | 45 ++++++++++++--------------------
>>>    1 file changed, 16 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
>>> index 9a327eb5d9d7..73c8bd27e6ae 100644
>>> --- a/drivers/soc/mediatek/mtk-mmsys.c
>>> +++ b/drivers/soc/mediatek/mtk-mmsys.c
>>> @@ -99,22 +99,27 @@ struct mtk_mmsys {
>>>    	struct reset_controller_dev rcdev;
>>>    };
>>> +static void mtk_mmsys_update_bits(struct mtk_mmsys *mmsys, u32 offset, u32 mask, u32 val)
>>> +{
>>> +	u32 tmp;
>>> +
>>> +	tmp = readl_relaxed(mmsys->regs + offset);
>>> +	tmp = (tmp & ~mask) | (val & mask);
>>
>> I'm not sure about the change in the implementation of
>> mtk_mmsys_update_bits(). Nicolas tried to explain it to me on IRC but I
>> wasn't totally convincing. As we have to go for at least another round of
>> this patches, I'd like to get a clear understanding while it is needed that
>> val bits are set to 1 in the mask.
> 
> The point here was to make sure that mtk_mmsys_update_bits() didn't allow
> setting bits outside of the mask, since that's never what you want: the entire
> point of having a mask is to specify the bits that should be updated (and the
> ones that should be kept unchanged). So for example if you had
> 
> mask = 0x0ff0
> val  = 0x00ff
> 
> the previous implementation would happily overwrite the 4 least significant bits
> on the destination register, despite them not being present in the mask, which
> is wrong.
> 
> This wrong behavior could easily lead to hard to trace bugs as soon as a badly
> formatted/wrong val is passed and an unrelated bit updated due to the mask being
> ignored.
> 
> For reference, _regmap_update_bits() does the same masking of the value [1].
> 
> That said, given that this function already existed and was just being moved,
> it would've been cleaner to make this change in a separate commit.
> 

Would have been better, but we can leave it as it.

Regards,
Matthias

> [1] https://elixir.bootlin.com/linux/latest/source/drivers/base/regmap/regmap.c#L3122
> 
> Thanks,
> Nícolas
> 
>>
>> Regards,
>> Matthias
>>
>>> +	writel_relaxed(tmp, mmsys->regs + offset);
>>> +}
> [..]
>>> -static void mtk_mmsys_update_bits(struct mtk_mmsys *mmsys, u32 offset, u32 mask, u32 val)
>>> -{
>>> -	u32 tmp;
>>> -
>>> -	tmp = readl_relaxed(mmsys->regs + offset);
>>> -	tmp = (tmp & ~mask) | val;
>>> -	writel_relaxed(tmp, mmsys->regs + offset);
>>> -}
>>> -
> [..]