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

[allen-kh.cheng <allen-kh.cheng@mediatek.com>]

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?

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
> > > > > > 
> > > > > > 
> > > > > > 
> > > > 
> > > >