| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
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