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
