Re: [PATCH v2 1/8] dt-bindings: mailbox: mediatek: Add GCE header file for MT8196

[Krzysztof Kozlowski <krzk@kernel.org>]

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

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

> 
> 
> 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".



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


> 
>> ...
>>
>>>>> +
>>>>> +/*
>>>>> + * 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.

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.



Best regards,
Krzysztof