Re: [PATCH 1/2] dt-bindings: soc: mediatek: add mdp3 mutex support for mt8186

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 08/07/22 13:58, allen-kh.cheng ha scritto:
> Hi Angelo,
> 
> On Fri, 2022-07-08 at 10:28 +0200, AngeloGioacchino Del Regno wrote:
>> Il 08/07/22 10:19, Matthias Brugger ha scritto:
>>>
>>>
>>> On 08/07/2022 10:14, allen-kh.cheng wrote:
>>>> Hi Angelo,
>>>>
>>>> On Thu, 2022-07-07 at 12:59 +0200, AngeloGioacchino Del Regno
>>>> wrote:
>>>>> Il 07/07/22 12:41, Matthias Brugger ha scritto:
>>>>>>
>>>>>>
>>>>>> On 07/07/2022 10:52, AngeloGioacchino Del Regno wrote:
>>>>>>> Il 05/07/22 14:26, Allen-KH Cheng ha scritto:
>>>>>>>> Add mdp3 mutex compatible for mt8186 SoC.
>>>>>>>>
>>>>>>>> Signed-off-by: Allen-KH Cheng <
>>>>>>>> allen-kh.cheng@mediatek.com>
>>>>>>>> Signed-off-by: Xiandong Wang <xiandong.wang@mediatek.com>
>>>>>>>
>>>>>>>
>>>>>>> Please drop this commit. Adding a mdp3-mutex compatible is
>>>>>>> not
>>>>>>> needed here.
>>>>>>>
>>>>>>
>>>>>> Thanks for checking. We probably would need a fallback
>>>>>> compatible.
>>>>>> We can only know
>>>>>> from the HW engineers that can confirm if the IP block is the
>>>>>> same
>>>>>> as the disp
>>>>>> mutex or a different one.
>>>>>>
>>>>>> I'll drop both patches for now until things got clear.
>>>>>>
>>>>>
>>>>> They're located in a different iospace from each other, but
>>>>> either
>>>>> the platform
>>>>> data needs to *not be* joined together, or if they're together,
>>>>> I
>>>>> would not like
>>>>> having two different compatible strings for essentially the
>>>>> same
>>>>> thing.
>>>>>
>>>>> I would at this point prefer dropping '-disp' from
>>>>> 'mediatek,mt8186-
>>>>> disp-mutex'
>>>>> so that we would be able to declare two 'mediatek,mt8186-mutex'
>>>>> in
>>>>> devicetree...
>>>>> ...or simply have two mediatek,mt8186-disp-mutex (although
>>>>> logically
>>>>> incorrect?).
>>>>>
>>>>> Cheers,
>>>>> Angelo
>>>>>
>>>>
>>>> Thanks for your opinion.
>>>>
>>>> They are two different hardwares for different address spaces.
>>>>
>>>> I think we drop '-disp' from 'mediatek,mt8186-disp-mutex' will be
>>>> excessive because we also need to modify mutex node in all exited
>>>> dts
>>>> files.
>>>>
>>>> I prefer havingg two mediatek,mt8186-disp-mutex.
>>>>
>>>> ex:
>>>> mutex: mutex@14001000 {
>>>>      compatible = "mediatek,mt8186-disp-mutex";
>>>>      ..
>>>> }
>>>>
>>>> mdp3_mutex0: mutex@1b001000 {
>>>>      compatible = "mediatek,mt8186-disp-mutex";
>>>>      ...
>>>> }
>>>>
>>>> What do you think?
>>>
>>> I think that's an acceptable solution.
>>>
>>
>> I'm a bit undecided instead, now... because from what I understand
>> now,
>> the platform data fields
>>
>> 	.mutex_mod  and  .mutex_sof
>>
>> are *not valid* for mutex at 0x1b001000 but only for the instance at
>> 0x14001000.
>>
>> If we go this way, at this point, we would be free (and allowed by
>> the driver)
>> to try to set these for 0x1b001000, and to try to set MDP3 table
>> paths on
>> 0x14001000, which is something that shouldn't be logically allowed,
>> as the
>> hardware does *not* support that.
>>
>> Unless I got that wrong, and these fields for MUTEX_MOD_DISP_xxxx do
>> exist in
>> the mutex instance at 0xb001000, in which case, I fully agree with
>> Matthias.
>>
>> But otherwise, I have my doubts.
>>
>> Cheers,
>> Angelo
>>
> 
> I got your point.
> 
> The disp and mdp3 drivers work with the same data field beacase
> 14001000 (disp mutex) would not use .mutex_table_mod and 1b001000 (mdp3
> mutex) would not use .mutex_mod/.mutex_sof.
> 
> 
> How about ...
> 
> static const struct mtk_mutex_data mt8186_mutex_driver_data = {
> 	.mutex_mod = mt8186_mutex_mod,
> 	.mutex_sof = mt8186_mutex_sof,
> 	.mutex_mod_reg = MT8183_MUTEX0_MOD0,
> 	.mutex_sof_reg = MT8183_MUTEX0_SOF0,
> };
> 
> static const struct mtk_mutex_data mt8186_mutex_mdp_driver_data = {
> 	.mutex_table_mod = mt8186_mutex_table_mod,
> };
> 
> { .compatible = "mediatek,mt8186-disp-mutex",
> .data = &mt8186_mutex_driver_data},
> { .compatible = "mediatek,mt8186-mdp3-mutex",
> .data = &mt8186_mutex_mdp_driver_data},
> 
> 
>   mutex: mutex@14001000 {
>      compatible = "mediatek,mt8186-disp-mutex";
>      ..
>   }
>   mdp3_mutex0: mutex@1b001000 {
>      compatible = "mediatek,mt8186-mdp3-mutex";
>      ...
>   }
> 
> Do you think that is feasible?
> 

This makes a lot more sense to me.

Though, you have to also add the mod and sof regs, because the mutex instance
for MDP_MUTEX does have these registers, even though they are used for different
mods/sofs.

static const struct mtk_mutex_data mt8186_mutex_driver_data = {
	.mutex_mod = mt8186_mutex_mod,
	.mutex_sof = mt8186_mutex_sof,
	.mutex_mod_reg = MT8183_MUTEX0_MOD0,
	.mutex_sof_reg = MT8183_MUTEX0_SOF0,
};

static const struct mtk_mutex_data mt8186_mdp_mutex_driver_data = {
  	.mutex_mod_reg = MT8183_MUTEX0_MOD0,
  	.mutex_sof_reg = MT8183_MUTEX0_SOF0,
	.mutex_table_mod = mt8186_mdp_mutex_table_mod,
};

P.S.: Notice that mt8186_mdp_mutex_driver_data instead of
       mt8186_mutex_mdp_driver_data was chosen on purpose:
       like that, we're referencing to real block names.

Regards,
Angelo

> Best Regards,
> Allen
> 
>>> Regards,
>>> Matthias
>>>
>>>>
>>>> Best regards,
>>>> Allen
>>>>
>>>>>> Regards,
>>>>>> Matthias
>>>>>>
>>>>>>>> ---
>>>>>>>> .../devicetree/bindings/soc/mediatek/mediatek,mutex.yaml
>>>>>>>>     | 1 +
>>>>>>>>     1 file changed, 1 insertion(+)
>>>>>>>>
>>>>>>>> diff --git
>>>>>>>> a/Documentation/devicetree/bindings/soc/mediatek/mediatek
>>>>>>>> ,mutex
>>>>>>>> .yaml
>>>>>>>> b/Documentation/devicetree/bindings/soc/mediatek/mediatek
>>>>>>>> ,mutex
>>>>>>>> .yaml
>>>>>>>> index 627dcc3e8b32..234fa5dc07c2 100644
>>>>>>>> ---
>>>>>>>> a/Documentation/devicetree/bindings/soc/mediatek/mediatek
>>>>>>>> ,mutex
>>>>>>>> .yaml
>>>>>>>> +++
>>>>>>>> b/Documentation/devicetree/bindings/soc/mediatek/mediatek
>>>>>>>> ,mutex
>>>>>>>> .yaml
>>>>>>>> @@ -30,6 +30,7 @@ properties:
>>>>>>>>           - mediatek,mt8173-disp-mutex
>>>>>>>>           - mediatek,mt8183-disp-mutex
>>>>>>>>           - mediatek,mt8186-disp-mutex
>>>>>>>> +      - mediatek,mt8186-mdp3-mutex
>>>>>>>>           - mediatek,mt8192-disp-mutex
>>>>>>>>           - mediatek,mt8195-disp-mutex
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>