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.
>
> 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.
...
>>> +
>>> +/*
>>> + * 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.
Best regards,
Krzysztof
