<html><body><p> <pre> On Thu, 2024-12-12 at 08:20 +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 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. > 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? Or could you give me an example for what should be binding and what should not be binding? 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. 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. 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. > > > > 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. > ... > > > > > + > > > > +/* > > > > + * 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. Others GPRs are not used currently and they can be define as MACRO in the same way of GCE_GPR_R15, so I can drop these GPR define in the next version. Perhaps the SoCs in the future has changed the rules of GPR index, we can add it back and get them from DTS. Regards, Jason-JH.Lin > Best regards, > Krzysztof </pre> </p></body></html><pre>************* MEDIATEK Confidentiality Notice ******************** The information contained in this e-mail message (including any attachments) may be confidential, proprietary, privileged, or otherwise exempt from disclosure under applicable laws. It is intended to be conveyed only to the designated recipient(s). Any use, dissemination, distribution, printing, retaining or copying of this e-mail (including its attachments) by unintended recipient(s) is strictly prohibited and may be unlawful. If you are not an intended recipient of this e-mail, or believe that you have received this e-mail in error, please notify the sender immediately (by replying to this e-mail), delete any and all copies of this e-mail (including any attachments) from your system, and do not disclose the content of this e-mail to any other person. Thank you! </pre>