Messages in this thread Patch in this message | | | Date | Fri, 8 Jul 2022 17:37:45 +0200 | Subject | Re: [PATCH 1/2] dt-bindings: soc: mediatek: add mdp3 mutex support for mt8186 | From | AngeloGioacchino Del Regno <> |
| |
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 >>>>>>> >>>>>>> >>>>>>> >>>>> >>>>> >
| |