On Thu, 2024-12-12 at 08:20 +0100, Krzysztof Kozlowski wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>
>
> On 12/12/2024 04:05, Jason-JH Lin (林睿祥) wrote:
> > Hi Krzysztof,
> >
> > Thanks for the reviews.
> >
> > On Wed, 2024-12-11 at 10:37 +0100, Krzysztof Kozlowski wrote:
> > > External email : Please do not click links or open attachments
> > > until
> > > you have verified the sender or the content.
> > >
> > >
> > > On Wed, Dec 11, 2024 at 11:22:49AM +0800, Jason-JH.Lin wrote:
> > > > Add the Global Command Engine (GCE) header file to define the
> > > > GCE
> > > > thread priority, GCE subsys ID and GCE events for MT8196.
> > >
> > > This we see from the diff. What we do not see is why priority is
> > > a
> > > binding. Looking briefly at existing code: it is not a binding,
> > > there
> > > is
> > > no driver user.
> > >
> >
> > This priority value is used to configure the priority level for
> > each
> > GCE hardware thread, so it is a necessary hardware attribute.
>
> I did not say these are not "hardware". I said these are not
> bindings.
> Bring arguments why these are bindings.
>
Not only bringing arguments, we use it to configure each GCE thread's
priority.
Please forgive me to ask a trivial question.
Do you mean if there is no driver using it directly, then it can not be
a binding?
Or could you give me an example for what should be binding and what
should not be binding?
Considering to these 3 points, I think GCE thread priority is suitable
to be part of the Device Tree Binding:
1. Describing Hardware Properties
- The Device Tree is a data structure for describing hardware, and GCE
thread priority, as part of the hardware, should be described in the
Device Tree.
2. Driver Usage
- Device Tree data is used by drivers to initialize and configure
hardware, and GCE thread priority is necessary configuration data for
the driver. After parsing the mboxes args from DTS, CMDQ driver use it
to configure GCE thread.
3. Standardization
- Device Tree bindings should be standardized, and GCE thread priority
should have consistent meaning and usage across different hardware
platforms. Looking into the latest header: mediatek,mt8188-gce.h,
mediatek,mt6795-gce.h and mt8195-gce.h, they all have defined GCE
thread priority.
> >
> > It's hard to find where the priority is used in existing driver
> > code
> > because we parsed it from DTS.
>
> So not a binding.
>
> >
> > It is used in all mediaTeks' DTS using the GCE.
> > For example, in mt8195.dts:
> >
> > vdosys0: syscon@1c01a000 {
> > compatible = "mediatek,mt8195-vdosys0", "mediatek,mt8195-
> > mmsys",
> > "syscon";
> > reg = <0 0x1c01a000 0 0x1000>;
> > mboxes = <&gce0 0 CMDQ_THR_PRIO_4>;
> > #clock-cells = <1>;
> > mediatek,gce-client-reg = <&gce0 SUBSYS_1c01XXXX 0xa000
> > 0x1000>;
> > }
> >
> > CMDQ driver(mtk-cmdq-mailbox.c) will get the args parsed from
> > mboxes
> > property in cmdq_xlate() and then it will store CMDQ_THR_PRIO_4 to
> > the
> > specific thread structure.
>
> So not a binding.
>
> > The user of CMDQ driver will send command to CMDQ driver by
> > cmdq_mbox_send_data(), and this priority setting will be configured
> > to
> > GCE hardware thread.
>
> And other things there are the same, we do not talk only about this
> one
> thing. I asked last time to drop which is not a binding.
>
>
I just reference all the previous mediatek,mtXXXX-gce.h to add the same
binding. Except for the GPR part I added this time, I don't know what
else should be dropped.
> ...
>
> > > > +
> > > > +/*
> > > > + * GCE General Purpose Register (GPR) support
> > > > + * Leave note for scenario usage here
> > > > + */
> > > > +/* GCE: write mask */
> > >
> > > That's a definite no-go. Register masks are not bindings.
> > >
> >
> > I'm sorry to the confusion.
> >
> > These defines are the index of GCE General Purpose Register for
> > generating instructions, they are not register masks.
>
> Index of register is also sounding like register.
>
> >
> > The comment "/* GCE: write mask */" is briefly describe that the
> > usage
> > of GCE_GPR_R0 and GCE_GPR_R01 is used to store the register mask
> > when
> > GCE executing the WRITE instruction. And it can also store the
> > register
> > mask of POLL and READ instruction.
> >
> > I will add more words to make this comment clearer, like this:
> > /*GCE: store the mask of instruction */
>
> Not sure, because I feel you just avoid doing what is right and keep
> pushing your own narrative. Where is it used in the driver?
>
> I just looked for "GCE_GPR_R00" - no usage at all. So not a binding.
>
Currently, GCE_GPR_R15 is used for generating POLL instruction and it
has been defined as a MACRO `#define CMDQ_POLL_ADDR_GPR (15)`
in mtk-cmdq-helper.c.
Others GPRs are not used currently and they can be define as MACRO in
the same way of GCE_GPR_R15, so I can drop these GPR define in the next
version. Perhaps the SoCs in the future has changed the rules of GPR
index, we can add it back and get them from DTS.
Regards,
Jason-JH.Lin
> Best regards,
> Krzysztof
