[PATCH v2 1/8] dt-bindings: mailbox: mediatek: Add GCE header file for MT8196
Jason-JH Lin (林睿祥)
Jason-JH.Lin at mediatek.com
Thu Dec 12 10:06:53 PST 2024
On Thu, 2024-12-12 at 13:23 +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 12:24, Jason-JH Lin (林睿祥) wrote:
> > > > >
> > > > > 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?
>
> Here:
> <qqqwdzmcnkuga6qvvszgg7o2myb26sld5i37e4konhln2n4cgc at mwtropwj3ywv>
>
Your reply in this example is really clear:
Register values are not bindings, they do not bind anything. Bindings
are imaginary numbers starting from 0 or 1 which are used between
drivers and DTS, serving as abstraction layer (or abstraction values)
between these two.
Now I can understand what the binding should be. Thank you very much.
From the description here, the GCE priority is not the imaginary
number, so we can just use the real number priority value in dts
instead of defining it in the binding header.
I thought the only advantage we define it here is to restrict the DTS
using the priority value between the CMDQ_THR_PRIO_LOWEST and
CMDQ_THR_PRIO_HIGHEST.
> > Or could you give me an example for what should be binding and what
> > should not be binding?
>
> Look at all clock IDs of recent arm64 SoC clock controllers.
>
I think clock IDs are the same as the actual bit to set into the
hardware register.
> >
> >
> > 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.
>
> I thought we talk about bindings, not DeviceTree. Where is any
> Devicetree here? Please point me to the code which is Devicetree in
> this
> patch.
>
>
> >
> > 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.
>
> We talk about bindings, so why are you describing something else?
>
> >
> > 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.
>
> None of above arguments have anything related to the talk here. You
> invent some stuff like "standardization" or "driver usage".
>
I 'm sorry to keep misunderstanding your question.
Please forget about that.
> >
> > > >
> > > > 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 at 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.
>
> Your commit msg does not help me, either. Why this is supposed to be
> a
> binding? Basically your rationale is: someone added headers, so I
> call
> it binding.
>
> This is invalid argument.
>
> Someone added junk, incorrect style or bugs, so you do the same?
>
>
Definitely not. I just not understand the meaning of binding.
So I just try to recall your memory why the previous headers are
accepted. Sorry about that.
> >
> > > ...
> > >
> > > > > > +
> > > > > > +/*
> > > > > > + * 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.
>
> I search for GCE_GPR_R15. No usage at all.
>
Yes, GCE_GPR_RXX is not used currently. I'll drop them.
> Point me to specific line in your code. Paste it here.
>
> Above #define does not use.
>
> I am finishing replying, because you keep avoiding actual answers and
> bring some false proves forcing me to re-iterate the same over and
> over.
>
> Point to specific line of code and you point to something else and
> then
> claim this is argument in discussion.
>
Since mt8196.dtsi has not sent to upstream, I will post the same
bindings in mediatek,mt8188-gce.h from DTS to the CMDQ driver code.
Please help me to check if it can be token into account as a binding.
1. include/dt-bindings/mailbox/mediatek,mt8188-gce.h: line 14:
#define CMDQ_THR_PRIO_4 4
arch/arm64/boot/dts/mediatek/mt8188.dtsi: line 2622:
vdosys0: syscon at 1c01d000{
mboxes = <&gce 0 CMDQ_THR_PRIO_4>;
}
drivers/gpu/drm/mediatek/mtk_crtc.c: line 1069:
mtk_crtc->cmdq_client.chan =
mbox_request_channel(&mtk_crtc->cmdq_client.client, i);
drivers/mailbox/mailbox.c: line 418:
if (of_parse_phandle_with_args(dev->of_node, "mboxes",
"mbox-cells", index, &spec)) {
...
}
chan = mbox->of_xlate(mbox, &spec);
drivers/mailbox/mtk-cmdq-mailbox.c: line 574:
static struct mbox_chan *cmdq_xlate(struct mbox_controller *mbox,
const struct of_phandle_args *sp){
thread->priority = sp->arg[1];
}
drivers/mailbox/mtk-cmdq-mailbox.c: line 424:
static int cmdq_mbox_send_data(struct mbox_chan *chan, void *data) {
writel(thread->priority, thread->base + CMDQ_THR_PRIORITY);
}
2. include/dt-bindings/mailbox/mediatek,mt8188-gce.h: line 21:
#define SUBSYS_1c00XXXX 3
arch/arm64/boot/dts/mediatek/mt8188.dtsi: line 2490:
ovl0: ovl at 1c000000 {
mediatek,gce-client-reg = <&gce SUBSYS_1c00XXXX 0x0000 0x1000>;
}
drivers/gpu/drm/mediatek/mtk_disp_ovl.c: line 621:
ret = cmdq_dev_get_client_reg(dev, &priv->cmdq_reg, 0);
drivers/soc/mediatek/mtk-cmdq-helper.c: line 59:
int cmdq_dev_get_client_reg(struct device *dev,
struct cmdq_client_reg *client_reg, int idx) {
client_reg->subsys = (u8)spec.arg[0];
}
drivers/gpu/drm/mediatek/mtk_disp_ovl.c: line 361:
void mtk_ovl_layer_on(struct device *dev, unsigned int idx,
struct cmdq_pkt *cmdq_pkt){
mtk_ddp_write_mask(cmdq_pkt, BIT(idx), &ovl->cmdq_reg, ovl->regs,
DISP_REG_OVL_SRC_CON, BIT(idx));
}
drivers/gpu/drm/mediatek/mtk_ddp_comp.c: line 101:
void mtk_ddp_write_mask(struct cmdq_pkt *cmdq_pkt, unsigned int value,
struct cmdq_client_reg *cmdq_reg, void __iomem *regs,
unsigned int offset, unsigned int mask) {
cmdq_pkt_write_mask(cmdq_pkt, cmdq_reg->subsys,
cmdq_reg->offset + offset, value, mask);
}
drivers/soc/mediatek/mtk-cmdq-helper.c: line 177:
cmdq_pkt_write_mask()
cmdq_pkt_write()
cmdq_pkt_append_command() {
// generate instruction to the cmdq buffer that will be executed by GCE
*cmdq_ptr = inst;
}
3. include/dt-bindings/mailbox/mediatek,mt8188-gce.h: line 526:
#define CMDQ_EVENT_VDO0_DISP_STREAM_DONE_0 574
arch/arm64/boot/dts/mediatek/mt8188.dtsi: line 2597:
mutex0: mutex at 1c016000 {
mediatek,gce-events = <CMDQ_EVENT_VDO0_DISP_STREAM_DONE_0>;
}
drivers/gpu/drm/mediatek/mtk_crtc.c: line 1077:
ret = of_property_read_u32_index(priv->mutex_node,
"mediatek,gce-events",
i,
&mtk_crtc->cmdq_event);
drivers/gpu/drm/mediatek/mtk_crtc.c: line 592:
cmdq_pkt_clear_event(cmdq_handle, mtk_crtc->cmdq_event);
drivers/soc/mediatek/mtk-cmdq-helper.c: line 363:
cmdq_pkt_clear_event(s
truct cmdq_pkt *pkt, u16 event) {
struct cmdq_instruction inst = {
.op = CMDQ_CODE_WFE,
.value = CMDQ_WFE_UPDATE,
.event =
event
};
if (event >= CMDQ_MAX_EVENT)
return -EINVAL;
return cmdq_pkt_append_command(pkt, int);
}
drivers/soc/mediatek/mtk-cmdq-helper.c: line 177:
cmdq_pkt_append_command(struct cmdq_pkt *pkt,
struct cmdq_instruction *inst) {
// generate instruction to the cmdq buffer that will be executed by GCE
*cmdq_ptr = inst;
}
I'm appreciate for your patience. Thanks again.
Regards,
Jason-JH.Lin
>
>
> Best regards,
> Krzysztof
More information about the Linux-mediatek
mailing list