I2S support for 4b ports

[mbanth]

Thank you for submitting an SDK feature request. Please provide as much information you can.

Describe the feature
Currently, I2S communication occurs only over 1b ports. Some processors have a limited number of 1b ports, so a desire exists to enhance I2S to allow the use of 4b ports.

Will this change any current APIs? How?
The I2S API will change. (@ACascarino to supply details)

Who will benefit with this feature?
Any customer or XMOS engineer writing software for a package with a limited number of 1b ports.

Any Other info
This enhancement has been made to lib_i2s. See lib_i2s issue 98 and pull request 107 for details.

[ACascarino]

Here's some additional details!

In lib_i2s, there exists a set of functions i2s_frame_master and i2s_frame_slave.
Since the adjustments required to these functions to have them work on a 4b port are very large, I made the decision to create new functions - i2s_frame_master_4b and i2s_frame_slave_4b.
The majority of the new functions are written in assembly - this is in order to get the performance required to perform these operations in time, as there are significantly tighter timing constraints on this mode of operation.
Therefore, these can be ported very easily to a C-based platform - I can simply modify the existing C implementation to add in the already-written asm files and update tests as appropriate.

Therefore, my proposed change to the API would be non-breaking - I propose simply adding i2s_master_4b and i2s_slave_4b to the existing functions.

An alternative approach would be to alter the existing API to include a four_bit flag (or similar) in the declarations of i2s_master and i2s_slave. This approach, while cleaner to the user, would necessitate either a rather bloated function definition or a tricky #define sequence to determine which of the two underlying implementations are used. This would also be a breaking change, and hence require a major version bump at release.

An ideal implementation would be for this process to be invisible to the user, and for them to be able to supply either an array of 1b ports or a single 4b port and for the function to then decide which implemetation to use. However, this isn't (as far as I'm aware...) cleanly possible in C. If someone has a thought on how to achieve this, I'm all ears!

Very happy to go with either - what are your thoughts @keithm-xmos and @mbanth?

[ACascarino]

Realised I'd had a bit of a moment - putting a flag in the function declaration still presents the problem where the argument describing the I/O ports can't be both an array of ports and a single port simultaneously. I still think they require separate function definitions unless there's some nice trick I can use - any advice appreciated!

[keithm-xmos]

Separate functions has my vote but I defer to @xmos-jmccarthy.

[mbanth]

Separate functions are okay with me too.

[xmos-jmccarthy]

Is the end goal for this to be available in the RTOS driver as well, or just as a hil library for use in bare metal?

[mbanth]

The goal is for it to be available in the RTOS driver as well. Integrating it there is a separate piece of work.

[xmos-jmccarthy]

Is it possible to implement this driver to have a single implementation which supports multiple port widths, similar to how the I2C hil was rewritten?

We have tried to avoid multiple implementations and consolidated I2C and SPI from dozens of implementations into 1 each. This achieves 2 things. 1 the use in bare metal is always the same, just with function arg differences. 2 the use in the rtos can be simplified as we don't need to have layers of code to call 1b port version vs 4b vs 8b etc, which would ultimately end up as just wasted binary space 99% of the time.

If it is not possible to consolidate the implementations then the question is where we want to add the bloat for selection. Either the i2s implementation or the rtos driver instantiation will contain the code to specify and ultimately call the 1b or 4b port version. If that decision is not to be made right now, I would propose the following:
Change the current API i2s_frame_master to i2s_frame_master_1b
Change the current API i2s_frame_slave to i2s_frame_slave_1b
Add i2s_master_4b as stated above
Add i2s_slave_4b as stated above
Add a new i2s_frame_master which has a port selection arg to call the appropriate implementation.
Add a new i2s_frame_slave which has a port selection arg to call the appropriate implementation.
Update the RTOS driver usage of i2s_frame_master to i2s_frame_master_1b until the future driver 4b support is added
Update the RTOS driver usage of i2s_frame_slave to i2s_frame_slave_1b until the future driver 4b support is added

[ACascarino]

After consultation with @mbanth and @xmos-jmccarthy the above approach is what I'll go with.
I/O ports will always need to be supplied as an array, even if a 4b port is used. This is slightly clumsy imo but mirrors existing usage - if the user only wishes to use one 1b port then the same would need to be written.
Additional arguments are going to therefore be needed to i2s_master to decouple the concept of "number of ports used for IO" from "number of streams input/output" - the current assumption with the 1b implementation is that if the user supplies 3 ports, 3 IO streams will be needed, which is obviously not the case with a single 4b port. This does leave open the possibility, although timing would be quite tight, to have two 4b ports as an 8-stream (16 channel) input or output. Additional implementation work would be required to support this.
My proposed API would therefore be along the lines of:

void i2s_master(
        const i2s_callback_group_t *const i2s_cbg,
        const size_t io_port_size,
        const size_t num_data_bits,     // The number of data bits in a channel word can only currently be 32, but this is not specification compliant; an implementation exists in lib_i2s to deal with non-32b data bit lengths in a 1b port. This may or may not be possible in a 4b port implementation.
        const port_t p_dout[],
        const size_t num_out_ports,
        const size_t num_out,
        const port_t p_din[],
        const size_t num_in_ports,
        const size_t num_in,
        const port_t p_bclk,
        const port_t p_lrclk,
        const port_t p_mclk,
        const xclock_t bclk);

with a pseudocode definition along the lines of

{
    if (io_port_size is 4)
    { i2s_master_4b(*args, **kwargs) }
    else if (io_port_size is 1)
    { i2s_master_1b(*args, **kwargs) }
    else 
    { assert something along the lines of "nope" }
}