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