[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID:
<AM6PR04MB5941FC1BB606ED5A2A9FD19088C82@AM6PR04MB5941.eurprd04.prod.outlook.com>
Date: Thu, 20 Jun 2024 03:10:31 +0000
From: Peng Fan <peng.fan@....com>
To: Cristian Marussi <cristian.marussi@....com>, "Peng Fan (OSS)"
<peng.fan@....nxp.com>
CC: Jonathan Corbet <corbet@....net>, Shawn Guo <shawnguo@...nel.org>, Sascha
Hauer <s.hauer@...gutronix.de>, Pengutronix Kernel Team
<kernel@...gutronix.de>, Fabio Estevam <festevam@...il.com>, Sudeep Holla
<sudeep.holla@....com>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
<krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"imx@...ts.linux.dev" <imx@...ts.linux.dev>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>, "devicetree@...r.kernel.org"
<devicetree@...r.kernel.org>
Subject: RE: [PATCH v4 1/6] Documentation: firmware-guide: add NXP i.MX95 SCMI
documentation
> Subject: Re: [PATCH v4 1/6] Documentation: firmware-guide: add NXP
> i.MX95 SCMI documentation
>
......
> > +The SM implements an interface compliant with the Arm SCMI 3.2
> Specification
>
> I would not specify the exact version, since the SCMI protocol is
> anyway
> completely discoverable and in case you evolve your support you will
> have to update endlessly the doc.
Sure. I will fix all in the patchset.
>
...
> > +- Set an alarm (per LM) in seconds
>
> what is an LM ? maybe a worth a note about it above in the intro
Logic Machine, it is i.MX SCMI firmware specific.
I will add in v5.
>
> > +- Get notifications on RTC update, alarm, or rollover.
> > +- Get notification on ON/OFF button activity.
....
> > ++---------------+--------------------------------------------------------------+
> > +|int32 status | See ARM SCMI Specification v3.2 section 4.1.4 for
> status |
> > +| | code definitions. |
>
> I understand that you want to mention a specific table in a specific
> document for a well-defined version, BUT I would drop here and all
> over this
> versioning and refs and just generically say
>
> | See ARM SCMI Specification for status code definitions.
> |
>
> to avoid all the future churn in keeping the refs updated here, since,
> as said, all versions and features are discoverable and the Linux
> kernel SCMI stack takes care usually to downgrade to match the
> detected
> protocol version.
Sure. I will fix in v5
>
>
> > ++---------------+--------------------------------------------------------------+
> > +|uint32 version | For this revision of the specification, this value
> must be |
> > +| | 0x10000. |
> > ++---------------+--------------------------------------------------------------+
> > +
......
> > +|int32 status |SUCCESS: if the button status was read.
> |
> > +| |Other value: ARM SCMI Specification v3.2 section 4.1.4.
> |
> > ++------------------+-----------------------------------------------------------+
> > +|uint32 state |State of the ON/OFF button |
> > ++------------------+-----------------------------------------------------------+
>
> How the states are codified ? 0/1 ? with the remaining bits reserevd ?
0 or 1 for now. other bits reserved.
>
> > +
> > +BBM_RTC_NOTIFY
> > +~~~~~~~~~~~~~~
> > +
.....
> > +|uint32 index |Index of the control |
> > ++------------------+-----------------------------------------------------------+
> > +|uint32 action |Action for the control
> |
> > ++------------------+-----------------------------------------------------------+
> > +|uint32 numarg |Size of the argument data
> |
>
> Max 8, it seems...please specify
>
> > ++------------------+-----------------------------------------------------------+
> > +|uint32 arg[8] |Argument data array
> |
>
> arg[N] with N in [0, numarg -1] ?
>
> ... a bit of formatting issues too above...
>
Yeah. Fix in v5
> > ++------------------+-----------------------------------------------------------+
> > +|Return values |
> > ++------------------+-----------------------------------------------------------+
> > +|Name |Description |
> > ++------------------+-----------------------------------------------------------+
> > +|int32 status |SUCCESS: if the action was set successfully.
> |
> > +| |NOT_FOUND: if the index is not valid. |
> > +| |DENIED: if the agent does not have permission to get
> the |
> > +| |control |
> > ++------------------+-----------------------------------------------------------+
> > +|uint32 num |Size of the return data in words |
>
> max 8 ?
>
> > ++------------------+-----------------------------------------------------------+
> > +|uint32 val[8] |value data array |
> > ++------------------+-----------------------------------------------------------+
>
> val[N] with N in [0, num -1] as above ... I suppose
>
Fix in v5.
> > +
> > +MISC_DISCOVER_BUILD_INFO
> > +~~~~~~~~~~~~~~~~~~~~~~~~
> > +
>
> Which build version this refers to ? the SM fw build version and
> metadata ?
>
Build date, commit hash and etc.
> > +message_id: 0x6
> > +protocol_id: 0x84
> > +
> > ++------------------+-----------------------------------------------------------+
> > +|Return values |
> > ++------------------+-----------------------------------------------------------+
> > +|Name |Description |
> > ++------------------+-----------------------------------------------------------+
> > +|int32 status |SUCCESS: if the build info was got successfully.
> |
> > +| |NOT_SUPPORTED: if the data is not available. |
> > ++------------------+-----------------------------------------------------------+
> > +|uint32 buildnum |Build number |
> > ++------------------+-----------------------------------------------------------+
> > +|uint32 buildcommit|Most significant 32 bits of the git commit
> hash |
> > ++------------------+-----------------------------------------------------------+
> > +|uint8 date[16] |Date of build. Null terminated ASCII string of up
> to 16 |
> > +| |bytes in length |
> > ++------------------+-----------------------------------------------------------+
> > +|uint8 time[16] |Time of build. Null terminated ASCII string of up
> to 16 |
> > +| |bytes in length |
> > ++------------------+-----------------------------------------------------------+
> > +
> > +MISC_ROM_PASSOVER_GET
> > +~~~~~~~~~~~~~~~~~~~~~
> > +
> > +This function is used to obtain the ROM passover data. The returned
> block of
> > +words is structured as defined in the ROM passover section in the
> SoC RM.
> > +
>
> what is a ROM passover exactly ?
>
It is ROM stored some info in RAM that could be used by SCMI firmware,
such ad boot device and etc.
> > +message_id: 0x7
> > +protocol_id: 0x84
> > +
> > ++------------------+-----------------------------------------------------------+
> > +|Return values |
> > ++------------------+-----------------------------------------------------------+
> > +|Name |Description |
> > ++------------------+-----------------------------------------------------------+
> > +|int32 status |SUCCESS: if the data was got successfully.
> |
> > +| |NOT_SUPPORTED: if the data is not available. |
> > ++------------------+-----------------------------------------------------------+
> > +|uint32 num |Size of the passover data in words |
> > ++------------------+-----------------------------------------------------------+
> > +|uint32_t data[8] |Passover data array |
> > ++------------------+-----------------------------------------------------------+
> > +
> > +MISC_CONTROL_NOTIFY
> > +~~~~~~~~~~~~~~~~~~~
> > +
> > +message_id: 0x8
> > +protocol_id: 0x84
> > +
> > ++------------------+-----------------------------------------------------------+
> > +|Parameters |
> > ++------------------+-----------------------------------------------------------+
> > +|Name |Description |
> > ++------------------+-----------------------------------------------------------+
> > +|uint32 index |Index of control |
> > ++------------------+-----------------------------------------------------------+
> > +|uint32 flags |Notification flags, varies by control |
>
> So basically this is to somehow enable notifs on the specified index
> BUT the flag field syntax is completely opaque and varies by the
> control type...
> ...how is this even used in the code ? there should be at least a bit
> dedicatd to enable/disable right ?
Currently the flag is in board device tree. Our current usage is
board specific.
>
> > ++------------------+-----------------------------------------------------------+
> > +|Return values |
> > ++------------------+-----------------------------------------------------------+
> > +|Name |Description |
> > ++------------------+-----------------------------------------------------------+
> > +|int32 status |SUCCESS: notification configuration was
> successfully |
> > +| |updated. |
> > +| |NOT_FOUND: control id not exists. |
> > +| |INVALID_PARAMETERS: if the input attributes flag
> specifies |
> > +| |unsupported or invalid configurations.. |
> > +| |DENIED: if the calling agent is not permitted to request
> |
> > +| |the notification. |
> > ++------------------+-----------------------------------------------------------+
> > +
> > +MISC_REASON_ATTRIBUTES
> > +~~~~~~~~~~~~~~~~~~~~~~
>
> ? as above said... REASON ?
>
It is reset reason. Such as WDOG, FCCU and etc.
> > +
> > +message_id: 0x9
> > +protocol_id: 0x84
> > +
> > ++------------------+-----------------------------------------------------------+
> > +|Parameters |
> > ++------------------+-----------------------------------------------------------+
> > +|Name |Description |
> > ++------------------+-----------------------------------------------------------+
> > +|uint32 reasonid |Identifier for the reason |
> > ++------------------+-----------------------------------------------------------+
> > +|Return values |
> > ++------------------+-----------------------------------------------------------+
> > +|Name |Description |
> > ++------------------+-----------------------------------------------------------+
> > +|int32 status |SUCCESS: if valid reason attributes are returned
> |
> > +| |NOT_FOUND: if reasonId pertains to a non-existent
> reason. |
> > ++------------------+-----------------------------------------------------------+
> > +|uint32 attributes |Reason attributes. This parameter has the
> following |
> > +| |format: Bits[31:0] Reserved, must be zero |
> > +| |Bits[15:0] Number of persistent storage (GPR) words.
> |
> > ++------------------+-----------------------------------------------------------+
> > +|uint8 name[16] |Null-terminated ASCII string of up to 16 bytes in
> length |
> > +| |describing the reason |
> > ++------------------+-----------------------------------------------------------+
> > +
> > +MISC_RESET_REASON
> > +~~~~~~~~~~~~~~~~~
> > +
> > +message_id: 0xA
> > +protocol_id: 0x84
> > +
>
> So is this a GET ? MISC_RESET_REASON_GET ?
Yes.
>
> > ++--------------------+---------------------------------------------------------+
> > +|Parameters |
....
> > ++--------------------+---------------------------------------------------------+
> > +|int32 status |SUCCESS: reset reason return |
>
> ??
Fix in v5.
>
> > ++--------------------+---------------------------------------------------------+
> > +|uint32 numLogflags |Descriptor for the log data returned by this
> call. |
> > +| |Bits[31:20] Number of remaining log words. |
> > +| |Bits[15:12] Reserved, must be zero. |
> > +| |Bits[11:0] Number of log words that are returned by
> this |
> > +| |call |
> > ++--------------------+---------------------------------------------------------+
> > +|uint32 syslog[16] |Log data array |
> > ++--------------------+---------------------------------------------------------+
>
> This should be syslog[N} with N defined by bits[11:0] in numLogflags,
> by
> the definition itself of multi-part SCMI command...the number of
> returned
> entries is limited by the platform dinamically based on the max size
> that
> the configure underlying transport allows....it could be more OR less
> than 16...this way is seems that you always carry around 16 bytes max,
> potentially with unused bytes if returned words are far less...
I need check firmware design, but your question is valid. I will check
and fix in v5.
Thanks,
Peng.
>
> Thanks,
> Cristian
Powered by blists - more mailing lists