linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: elvis.wang <elvis.wang@mediatek.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
	Jassi Brar <jassisinghbrar@gmail.com>,
	Rob Herring <robh+dt@kernel.org>,
	"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	CK Hu <ck.hu@mediatek.com>
Cc: <linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-mediatek@lists.infradead.org>,
	<Project_Global_Chrome_Upstream_Group@mediatek.com>
Subject: Re: [PATCH 2/2] dt-bindings: gce: add gce header file for mt8188
Date: Wed, 28 Sep 2022 10:20:31 +0800	[thread overview]
Message-ID: <26ecca71249f93893b659b604a0e18d479598145.camel@mediatek.com> (raw)
In-Reply-To: <747079f8-cf39-a78c-6b8a-51a14991b5c9@linaro.org>

On Fri, 2022-09-09 at 09:27 +0200, Krzysztof Kozlowski wrote:
> On 09/09/2022 08:46, elvis.wang wrote:
> > Hello Krzysztof,
> > 
> > On Tue, 2022-08-02 at 10:08 +0200, Krzysztof Kozlowski wrote:
> > > On 29/07/2022 10:43, Elvis Wang wrote:
> > > > Add gce header file to define the gce subsys id, hardware event
> > > > id
> > > > and
> > > > constant for mt8188.
> > > > 
> > > > Signed-off-by: Elvis Wang <Elvis.Wang@mediatek.com>
> > > > ---
> > > >  include/dt-bindings/gce/mt8188-gce.h | 1079
> > > > ++++++++++++++++++++++++++
> > > >  1 file changed, 1079 insertions(+)
> > > >  create mode 100644 include/dt-bindings/gce/mt8188-gce.h
> > > > 
> > > > diff --git a/include/dt-bindings/gce/mt8188-gce.h b/include/dt-
> > > > bindings/gce/mt8188-gce.h
> > > > new file mode 100644
> > > > index 000000000000..b15e965fe671
> > > > --- /dev/null
> > > > +++ b/include/dt-bindings/gce/mt8188-gce.h
> > > 
> > > Use vendor in filename, so mediatek,mt8188-gce.h
> > 
> > yes, mediatek,mt8188-gce.h as filename would be better, will change
> > in
> > next version.
> > 
> > > 
> > > > @@ -0,0 +1,1079 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > 
> > > Dual license.
> > 
> > ok, will use /* SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause */
> > in
> > next version. Also ask if it is possible to use BSD-3-CLause?
> 
> Please use GPL-2.0 OR BSD-2-Clause.

Thanks for your suggestion, we will change the usage in the next
version.

> 
> > > 
> > > > +
> > > > +/* GCE thread priority */
> > > > +#define CMDQ_THR_PRIO_LOWEST	0
> > > > +#define CMDQ_THR_PRIO_1		1
> > > > +#define CMDQ_THR_PRIO_2		2
> > > > +#define CMDQ_THR_PRIO_3		3
> > > > +#define CMDQ_THR_PRIO_4		4
> > > > +#define CMDQ_THR_PRIO_5		5
> > > > +#define CMDQ_THR_PRIO_6		6
> > > > +#define CMDQ_THR_PRIO_HIGHEST	7
> > > > +
> > > > +/* CPR count in 32bit register */
> > > > +#define GCE_CPR_COUNT		1312
> > > 
> > > No register values in the bindings.
> > 
> > Those thread priority will be referenced in the device node and
> > congfigured to the hw when trigger hw work.
> 
> Using something in DTS does not mean you have to encode it in
> bindings.
> Bindings describe the DTS, not the hardware programming model.

Thanks for your explanation, although these definitions of gpr and cpr
are indeed hw-related, but it seems inappropriate to put them in
binding and will be removed in the next version. We will try to keep
only the definitions used in dts node(such as subsys_id, hw/sw event
and thread priority).

> 
> > 
> > > 
> > > 
> > > 
> > > > +
> > > > +/* GCE subsys table */
> > > > +#define SUBSYS_1400XXXX		0
> > > > +#define SUBSYS_1401XXXX		1
> > > > +#define SUBSYS_1402XXXX		2
> > > > +#define SUBSYS_1c00XXXX		3
> > > > +#define SUBSYS_1c01XXXX		4
> > > > +#define SUBSYS_1c02XXXX		5
> > > > +#define SUBSYS_1c10XXXX		6
> > > > +#define SUBSYS_1c11XXXX		7
> > > > +#define SUBSYS_1c12XXXX		8
> > > > +#define SUBSYS_14f0XXXX		9
> > > > +#define SUBSYS_14f1XXXX		10
> > > > +#define SUBSYS_14f2XXXX		11
> > > > +#define SUBSYS_1800XXXX		12
> > > > +#define SUBSYS_1801XXXX		13
> > > > +#define SUBSYS_1802XXXX		14
> > > > +#define SUBSYS_1803XXXX		15
> > > > +#define SUBSYS_1032XXXX		16
> > > > +#define SUBSYS_1033XXXX		17
> > > > +#define SUBSYS_1600XXXX		18
> > > > +#define SUBSYS_1601XXXX		19
> > > > +#define SUBSYS_14e0XXXX		20
> > > > +#define SUBSYS_1c20XXXX		21
> > > > +#define SUBSYS_1c30XXXX		22
> > > > +#define SUBSYS_1c40XXXX		23
> > > > +#define SUBSYS_1c50XXXX		24
> > > > +#define SUBSYS_1c60XXXX		25
> > > > +#define SUBSYS_NO_SUPPORT	99
> > > > +
> > > > +/* GCE General Purpose Register (GPR) support
> > > > + * Leave note for scenario usage here
> > > > + */
> > > > +/* GCE: write mask */
> > > > +#define GCE_GPR_R00		0x00
> > > > +#define GCE_GPR_R01		0x01
> > > 
> > > No. These are no bindings. Do not embed device programming model
> > > into
> > > bindings header. I'll stop review.
> > 
> > This GCE_GPR define is an attribute of hw, and different hw
> > versions
> > may have different gpr number.
> 
> It does not matter that different devices have different programming
> model. The bindings are not a place for it.
> 
> Best regards,
> Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

      reply	other threads:[~2022-09-28  2:22 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-07-29  8:43 [PATCH 0/2] Add gce support for mt8188 Elvis Wang
2022-07-29  8:43 ` [PATCH 1/2] dt-bindings: mailbox: add definition " Elvis Wang
2022-08-02  8:06   ` Krzysztof Kozlowski
2022-08-03 22:25   ` Rob Herring
2022-09-09  5:40     ` elvis.wang
2022-07-29  8:43 ` [PATCH 2/2] dt-bindings: gce: add gce header file " Elvis Wang
2022-08-02  3:17   ` Rex-BC Chen
2022-09-09  4:04     ` elvis.wang
2022-08-02  8:08   ` Krzysztof Kozlowski
2022-09-09  6:46     ` elvis.wang
2022-09-09  7:27       ` Krzysztof Kozlowski
2022-09-28  2:20         ` elvis.wang [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=26ecca71249f93893b659b604a0e18d479598145.camel@mediatek.com \
    --to=elvis.wang@mediatek.com \
    --cc=Project_Global_Chrome_Upstream_Group@mediatek.com \
    --cc=ck.hu@mediatek.com \
    --cc=devicetree@vger.kernel.org \
    --cc=jassisinghbrar@gmail.com \
    --cc=krzysztof.kozlowski+dt@linaro.org \
    --cc=krzysztof.kozlowski@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=matthias.bgg@gmail.com \
    --cc=robh+dt@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).