<html><body><p>
<pre>
On Wed, 2025-03-05 at 19:08 +0100, AngeloGioacchino Del Regno wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>
>
> Il 05/03/25 17:12, Jason-JH Lin (林睿祥) ha scritto:
> > On Tue, 2025-03-04 at 10:41 +0100, AngeloGioacchino Del Regno
> > wrote:
> > >
> > > External email : Please do not click links or open attachments
> > > until
> > > you have verified the sender or the content.
> > >
> > >
> > > Il 18/02/25 06:41, Jason-JH Lin ha scritto:
> > > > To support hardware without subsys IDs on new SoCs, add a
> > > > programming
> > > > flow that checks whether the subsys ID is valid. If the subsys
> > > > ID
> > > > is
> > > > invalid, the flow will call 2 alternative CMDQ APIs:
> > > > cmdq_pkt_assign() and cmdq_pkt_write_s_value() to achieve the
> > > > same
> > > > functionality.
> > > >
> > > > Signed-off-by: Jason-JH Lin <jason-jh.lin@mediatek.com>
> > > > ---
> > > > drivers/soc/mediatek/mtk-mmsys.c | 14 +++++++++++---
> > > > drivers/soc/mediatek/mtk-mutex.c | 11 +++++++++--
> > > > 2 files changed, 20 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/soc/mediatek/mtk-mmsys.c
> > > > b/drivers/soc/mediatek/mtk-mmsys.c
> > > > index bb4639ca0b8c..ce949b863b05 100644
> > > > --- a/drivers/soc/mediatek/mtk-mmsys.c
> > > > +++ b/drivers/soc/mediatek/mtk-mmsys.c
> > > > @@ -167,9 +167,17 @@ static void mtk_mmsys_update_bits(struct
> > > > mtk_mmsys *mmsys, u32 offset, u32 mask,
> > > > u32 tmp;
> > > >
> > > > if (mmsys->cmdq_base.size && cmdq_pkt) {
> > > > - ret = cmdq_pkt_write_mask(cmdq_pkt, mmsys-
> > > > > cmdq_base.subsys,
> > > > - mmsys->cmdq_base.offset
> > > > +
> > > > offset, val,
> > > > - mask);
> > > > + offset += mmsys->cmdq_base.offset;
> > > > + if (mmsys->cmdq_base.subsys !=
> > > > CMDQ_SUBSYS_INVALID) {
> > >
> > > You're still anyway passing the subsys to cmdq_pkt_write_mask(),
> > > right?!
> > > Why don't you just handle this in cmdq_pkt_write_mask() then? ;-)
> > >
> > > I can see this pattern being repeated over and over in both
> > > drm/mediatek and MDP3
> > > drivers, and it's not necessary to duplicate this many times when
> > > you
> > > can write it
> > > just once.
> > >
> > > Would've also been faster for you to implement... :-D
> > >
> >
> > I think did it in the series V1:
> > https://patchwork.kernel.org/project/linux-mediatek/patch/20241121042602.32730-5-jason-jh.lin@mediatek.com
> >
> > Because it'll need to passing the base_pa and that will need to
> > change
> > the interface for original APIs.
> >
> > And CK think that's not a necessary to change the APIs. It can be
> > done
> > by cmdq_pkt_assign() + cmdq_pkt_write_s_mask_value() in the client
> > drivers. Then you can see this pattern in everywhere. :-)
> >
>
> Using likely(x) and unlikely(x) should be avoided, really, unless
> it's something
> that is really really really really ... really ... rea.... likely or
> unlikely :-)
>
> Btw. Changing the APIs is a bit difficult, but I disagree with CK
> about not
> "inventing" a new API for the unsupported-subsys flow.
>
> It's true, it is not *strictly* needed to add a function, but it's
> good for any
> kind of future maintainability - as I explained, it's easier then to
> fix a problem
> if there's one.... and well, I can see that you agree with me,
> because effectively
> you did it the first time :-)
>
> CK mentioned using cmdq_pkt_write() *or*
> cmdq_pkt_assignwrite/cmdq_pkt_write_pa()
> (however you wanna call it, it's fine for me), in drivers that know
> that there
> always is or there always isn't a subsys ID: that's a good
> suggestion, as this can
> be eventually done with assigning a function pointer, so, no
> conditionals at each
> operation.
>
> My point of view, finally, is:
> - This is just another way of doing cmdq_pkt_write()
> - This, at the end of the day, does exactly what cmdq_pkt_write()
> is doing,
> except it's doing it with two instructions instead of one;
> - The same thing can be done in two different ways (depending on
> SoC)
> - This same thing should have a function that does it.
>
> A function that does it can be
>
> int cmdq_pkt_write_pa(struct cmdq_pkt *pkt, u8 subsys /*unused*/, u32
> pa_base, u16
> offset, u32 value)
> {
> err = cmdq_pkt_assign(pkt, 0, CMDQ_ADDR_HIGH(pa_base));
> if (err < 0)
> return err;
>
> return cmdq_pkt_write_s_value( .... etc)
> }
>
> int cmdq_pkt_write() <--- unchanged, scheduled for removal after all
> drivers migrated
>
> int cmdq_pkt_write_subsys(struct cmdq_pkt *pkt, u8 subsys, u32
> pa_base /*unused*/,
> u16 offset, u32 value)
> {
> /* This function will get the contents of cmdq_pkt_write once
> removed,
> but, in the meanwhile, to avoid duplication we just call
> that: */
>
> return cmdq_pkt_write(pkt, subsys, offset, value);
> }
>
> - Are we adding one more function parameter? Yes
> - Is this impacting performance overall? Not really
>
> After all, we're living in an ARMv8 (actually, ARMv9 for new ones)
> world, so
> one more function param won't hurt anyone.
>
> I think that's the best of both worlds, and makes everyone happy.
> Are you happy with that? :-)
>
Yes, I am happy with that. :-)
And thanks for your detail coding.
I'll change it in the next version.
regards,
Jason-JH Lin
> Cheers,
> Angelo
>
</pre>
</p></body></html><!--type:text--><!--{--><pre>************* MEDIATEK Confidentiality Notice ********************
The information contained in this e-mail message (including any
attachments) may be confidential, proprietary, privileged, or otherwise
exempt from disclosure under applicable laws. It is intended to be
conveyed only to the designated recipient(s). Any use, dissemination,
distribution, printing, retaining or copying of this e-mail (including its
attachments) by unintended recipient(s) is strictly prohibited and may
be unlawful. If you are not an intended recipient of this e-mail, or believe
that you have received this e-mail in error, please notify the sender
immediately (by replying to this e-mail), delete any and all copies of
this e-mail (including any attachments) from your system, and do not
disclose the content of this e-mail to any other person. Thank you!
</pre><!--}-->