NXP S32 introduce support PSI5

[congnguyenhuu]

Introduce support Peripheral Sensor Interface (PSI5) driver on S32Z. This driver allows to communication (send, receive) with PSI5 device.
The test result: PSI5-TESTING-RESULT.pptx

RFC: #83982

[zephyrbot]

The following west manifest projects have changed revision in this Pull Request:

Name	Old Revision	New Revision	Diff
hal_nxp	zephyrproject-rtos/hal_nxp@fc8aa27	zephyrproject-rtos/hal_nxp@7a52cbb (master)	zephyrproject-rtos/[email protected]

✅ All manifest checks OK

Note: This message is automatically posted and updated by the Manifest GitHub Action.

[congnguyenhuu]

Addressed review comments from Kartben in SENT PR

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[dkalowsk]

@congnguyenhuu and @mmahadevan108 and @dleach02 I'm a little concerned on this struct. The union type is 32-bit value, yet the serial struct is only 24-bits in size. The union will promise a minimum of 32-bits, but when accessing the serial struct I believe this is going to end up with some undefined behavior and padding. Are you sure this is what you want?

It's easily fixed with something like:

		struct {
			/** Serial message ID */
			uint8_t id;
			/** Serial message data */
			uint16_t data;
			uint8_t rsvd;
		} serial;

Otherwise, the compiler may add the padding where the cacheline is, and it's likely between the id and the data type. Additionally because it falls within the uint32_t reading beyond uint16_t data type (because the name collides with the uint32_t version) won't trigger asan or ubsan for the memory overflow. Again just checking that you really want to do this.

[dleach02]

@dkalowsk This might vary between architectures but this is talking to an on-chip IP block so different architectures is not an issue. But I generally agree though that the sub structure should put the uint8 rsvd field where it explicitly expects it in the byte stream. It could be before the id, between the id & data, or after as you suggested.

But having said that, as implemented, it works with the underlying hardware so the alignment likely meets the expectations of the HAL functions and the IP on chip. We would like to get this merged within the scope of the merge window because it also involved a HAL update that has been merged which complicates things. The risk/exposure to the rest of Zephyr is zero since this is the first introduction of a driver framework for this IP and as such, only exposes the NXP S32 parts.

If we determine there is a bug here, we can patch it after the feature freeze.

[dkalowsk]

@dleach02 I understand your concern and desire to get this merged. And you call out the alternate case I am concerned about.

		struct {
			/** Serial message ID */
			uint8_t id;
			/** Serial message data */
			uint16_t data;
+			uint8_t rsvd;
		} serial;

and

		struct {
			/** Serial message ID */
			uint8_t id;
+			uint8_t rsvd;
			/** Serial message data */
			uint16_t data;
		} serial;

both have different meanings and support/output. I'm not sure I follow the argument that this is an IP block only so it's correct. The compiler will still need to provide the data structure implementation to be used. Running this through CompilerExplorer in a simplified example shows my concern as the second is the case being generated: https://gcc.godbolt.org/z/Prb9b1bjn

[manuargue]

serial and data represent mutually exclusive frame formats, meaning the union is used to hold either a serial frame or a data frame, and they are never meant to be read or written together. So assuming that the consumer follow the intended use, there won't be undefined behaviors due to padding, neither aliasing risks.

That said, I noticed this hasn't been properly documented so it opens possibility of misuse. Additionally, using a tagged union could improve code clarity without sacrificing compactness or requiring to model each frame type as a complete separate struct. For example:

struct psi5_frame {
    enum psi5_frame_type type;
    union {
        struct {
            uint32_t payload;
        } data;  /* access this when .type is PSI5_DATA_FRAME */
        struct {
            uint8_t id;
            uint16_t payload;
        } serial;   /* access this when .type is PSI5_SERIAL_FRAME_4_BIT_ID or PSI5_SERIAL_FRAME_8_BIT_ID */
    };
    uint32_t timestamp;
    uint8_t crc;
    uint8_t slot_number;
};

As a side note, the naming chosen for "data" and "serial" frames is a bit misleading, given that both frame types carry data, except that the former is transmitted periodically (sensor measurements) and the latter is assembled over multiple frames (sensor diagnostics and metadata). I would suggest this to be addressed as well.