diff for duplicates of <ce68d58f78d12317cab689e0ee1c02dc373d50dc.camel@mediatek.com> diff --git a/N1/2.bin b/N1/2.bin new file mode 100644 index 0000000..7bd5c59 --- /dev/null +++ b/N1/2.bin @@ -0,0 +1,182 @@ +<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><!--type:text--><!--{--><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><!--}--> diff --git a/N1/2.hdr b/N1/2.hdr new file mode 100644 index 0000000..da1f671 --- /dev/null +++ b/N1/2.hdr @@ -0,0 +1,3 @@ +Content-Type: text/html; + charset="utf-8" +Content-Transfer-Encoding: base64 diff --git a/a/content_digest b/N1/content_digest index 231f2ce..89dec9d 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -28,7 +28,7 @@ jassisinghbrar@gmail.com <jassisinghbrar@gmail.com> krzk+dt@kernel.org <krzk+dt@kernel.org> " AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>\0" - "\00:1\0" + "\01:1\0" "b\0" "On Thu, 2024-12-12 at 08:20 +0100, Krzysztof Kozlowski wrote:\n" "> External email : Please do not click links or open attachments until\n" @@ -195,5 +195,189 @@ "\n" "> Best regards,\n" > Krzysztof + "\01:2\0" + "b\0" + "<html><body><p>\r\n" + "<pre>\r\n" + "On Thu, 2024-12-12 at 08:20 +0100, Krzysztof Kozlowski wrote:\r\n" + "> External email : Please do not click links or open attachments until\r\n" + "> you have verified the sender or the content.\r\n" + "> \r\n" + "> \r\n" + "> On 12/12/2024 04:05, Jason-JH Lin (林睿祥) wrote:\r\n" + "> > Hi Krzysztof,\r\n" + "> > \r\n" + "> > Thanks for the reviews.\r\n" + "> > \r\n" + "> > On Wed, 2024-12-11 at 10:37 +0100, Krzysztof Kozlowski wrote:\r\n" + "> > > External email : Please do not click links or open attachments\r\n" + "> > > until\r\n" + "> > > you have verified the sender or the content.\r\n" + "> > > \r\n" + "> > > \r\n" + "> > > On Wed, Dec 11, 2024 at 11:22:49AM +0800, Jason-JH.Lin wrote:\r\n" + "> > > > Add the Global Command Engine (GCE) header file to define the\r\n" + "> > > > GCE\r\n" + "> > > > thread priority, GCE subsys ID and GCE events for MT8196.\r\n" + "> > > \r\n" + "> > > This we see from the diff. What we do not see is why priority is\r\n" + "> > > a\r\n" + "> > > binding. Looking briefly at existing code: it is not a binding,\r\n" + "> > > there\r\n" + "> > > is\r\n" + "> > > no driver user.\r\n" + "> > > \r\n" + "> > \r\n" + "> > This priority value is used to configure the priority level for\r\n" + "> > each\r\n" + "> > GCE hardware thread, so it is a necessary hardware attribute.\r\n" + "> \r\n" + "> I did not say these are not "hardware". I said these are not\r\n" + "> bindings.\r\n" + "> Bring arguments why these are bindings.\r\n" + "> \r\n" + "\r\n" + "Not only bringing arguments, we use it to configure each GCE thread's\r\n" + "priority.\r\n" + "\r\n" + "Please forgive me to ask a trivial question.\r\n" + "Do you mean if there is no driver using it directly, then it can not be\r\n" + "a binding?\r\n" + "Or could you give me an example for what should be binding and what\r\n" + "should not be binding?\r\n" + "\r\n" + "\r\n" + "Considering to these 3 points, I think GCE thread priority is suitable\r\n" + "to be part of the Device Tree Binding:\r\n" + "\r\n" + "1. Describing Hardware Properties\r\n" + "- The Device Tree is a data structure for describing hardware, and GCE\r\n" + "thread priority, as part of the hardware, should be described in the\r\n" + "Device Tree.\r\n" + "\r\n" + "2. Driver Usage\r\n" + "- Device Tree data is used by drivers to initialize and configure\r\n" + "hardware, and GCE thread priority is necessary configuration data for\r\n" + "the driver. After parsing the mboxes args from DTS, CMDQ driver use it\r\n" + "to configure GCE thread.\r\n" + "\r\n" + "3. Standardization\r\n" + "- Device Tree bindings should be standardized, and GCE thread priority\r\n" + "should have consistent meaning and usage across different hardware\r\n" + "platforms. Looking into the latest header: mediatek,mt8188-gce.h,\r\n" + "mediatek,mt6795-gce.h and mt8195-gce.h, they all have defined GCE\r\n" + "thread priority.\r\n" + "\r\n" + "> > \r\n" + "> > It's hard to find where the priority is used in existing driver\r\n" + "> > code\r\n" + "> > because we parsed it from DTS.\r\n" + "> \r\n" + "> So not a binding.\r\n" + "> \r\n" + "> > \r\n" + "> > It is used in all mediaTeks' DTS using the GCE.\r\n" + "> > For example, in mt8195.dts:\r\n" + "> > \r\n" + "> > vdosys0: syscon@1c01a000 {\r\n" + "> >     compatible = "mediatek,mt8195-vdosys0", "mediatek,mt8195-\r\n" + "> > mmsys",\r\n" + "> > "syscon";\r\n" + "> >     reg = <0 0x1c01a000 0 0x1000>;\r\n" + "> >     mboxes = <&gce0 0 CMDQ_THR_PRIO_4>;\r\n" + "> >     #clock-cells = <1>;\r\n" + "> >     mediatek,gce-client-reg = <&gce0 SUBSYS_1c01XXXX 0xa000\r\n" + "> > 0x1000>;\r\n" + "> > }\r\n" + "> > \r\n" + "> > CMDQ driver(mtk-cmdq-mailbox.c) will get the args parsed from\r\n" + "> > mboxes\r\n" + "> > property in cmdq_xlate() and then it will store CMDQ_THR_PRIO_4 to\r\n" + "> > the\r\n" + "> > specific thread structure.\r\n" + "> \r\n" + "> So not a binding.\r\n" + "> \r\n" + "> > The user of CMDQ driver will send command to CMDQ driver by\r\n" + "> > cmdq_mbox_send_data(), and this priority setting will be configured\r\n" + "> > to\r\n" + "> > GCE hardware thread.\r\n" + "> \r\n" + "> And other things there are the same, we do not talk only about this\r\n" + "> one\r\n" + "> thing. I asked last time to drop which is not a binding.\r\n" + "> \r\n" + "> \r\n" + "\r\n" + "I just reference all the previous mediatek,mtXXXX-gce.h to add the same\r\n" + "binding. Except for the GPR part I added this time, I don't know what\r\n" + "else should be dropped.\r\n" + "\r\n" + "> ...\r\n" + "> \r\n" + "> > > > +\r\n" + "> > > > +/*\r\n" + "> > > > + * GCE General Purpose Register (GPR) support\r\n" + "> > > > + * Leave note for scenario usage here\r\n" + "> > > > + */\r\n" + "> > > > +/* GCE: write mask */\r\n" + "> > > \r\n" + "> > > That's a definite no-go. Register masks are not bindings.\r\n" + "> > > \r\n" + "> > \r\n" + "> > I'm sorry to the confusion.\r\n" + "> > \r\n" + "> > These defines are the index of GCE General Purpose Register for\r\n" + "> > generating instructions, they are not register masks.\r\n" + "> \r\n" + "> Index of register is also sounding like register.\r\n" + "> \r\n" + "> > \r\n" + "> > The comment "/* GCE: write mask */" is briefly describe that the\r\n" + "> > usage\r\n" + "> > of GCE_GPR_R0 and GCE_GPR_R01 is used to store the register mask\r\n" + "> > when\r\n" + "> > GCE executing the WRITE instruction. And it can also store the\r\n" + "> > register\r\n" + "> > mask of POLL and READ instruction.\r\n" + "> > \r\n" + "> > I will add more words to make this comment clearer, like this:\r\n" + "> > /*GCE: store the mask of instruction */\r\n" + "> \r\n" + "> Not sure, because I feel you just avoid doing what is right and keep\r\n" + "> pushing your own narrative. Where is it used in the driver?\r\n" + "> \r\n" + "> I just looked for "GCE_GPR_R00" - no usage at all. So not a binding.\r\n" + "> \r\n" + "\r\n" + "Currently, GCE_GPR_R15 is used for generating POLL instruction and it\r\n" + "has been defined as a MACRO `#define CMDQ_POLL_ADDR_GPR (15)`\r\n" + "in mtk-cmdq-helper.c.\r\n" + "\r\n" + "Others GPRs are not used currently and they can be define as MACRO in\r\n" + "the same way of GCE_GPR_R15, so I can drop these GPR define in the next\r\n" + "version. Perhaps the SoCs in the future has changed the rules of GPR\r\n" + "index, we can add it back and get them from DTS.\r\n" + "\r\n" + "Regards,\r\n" + "Jason-JH.Lin\r\n" + "\r\n" + "> Best regards,\r\n" + "> Krzysztof\r\n" + "\r\n" + "</pre>\r\n" + "</p></body></html><!--type:text--><!--{--><pre>************* MEDIATEK Confidentiality Notice ********************\r\n" + "The information contained in this e-mail message (including any \r\n" + "attachments) may be confidential, proprietary, privileged, or otherwise\r\n" + "exempt from disclosure under applicable laws. It is intended to be \r\n" + "conveyed only to the designated recipient(s). Any use, dissemination, \r\n" + "distribution, printing, retaining or copying of this e-mail (including its \r\n" + "attachments) by unintended recipient(s) is strictly prohibited and may \r\n" + "be unlawful. If you are not an intended recipient of this e-mail, or believe \r\n" + "that you have received this e-mail in error, please notify the sender \r\n" + "immediately (by replying to this e-mail), delete any and all copies of \r\n" + "this e-mail (including any attachments) from your system, and do not\r\n" + "disclose the content of this e-mail to any other person. Thank you!\r\n" + </pre><!--}--> -d42b3e91e75808a0cfc0a448597c21ba57a3fe459900408398503c868f2b85aa +8894e750c9c99b0a6b1e4f114fc6175c2dbcd73f2799ce285cb6dbbf5a1d8e49
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.