RE: [PATCH v4 1/6] Documentation: firmware-guide: add NXP i.MX95 SCMI documentation

[Peng Fan]

> 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