os/arch/arm/src/amebasmart: Fix I2S abort during PM suspend

[ziyik]

-Check I2S stuct pointer during PM suspend and resume

[sunghan-chang]

I don't get why this is changed. Why is i2s pm under audio i2s?

[seokhun-eom24]

i2s can also be initialized in rtl8730e_syu645b_initialize.
So I think the previous conditional check that calling i2s_pminitialize when CONFIG_AMEBASMART_I2S is defined is correct.

[ziyik]

Since amebasmart_i2s_suspend() and amebasmart_i2s_resume() are based on port number, but currently when pmu_register_sleep_callback() is called, we do not pass in any param. Should we restructure i2s_pminitialize() place it in amebasmart_i2s_initialize() and called with port number pass in? So PM suspend and resume will be trigger based on i2s port which has been initialized?

[seokhun-eom24]

I think it would be a better design for to call i2s_pminitialize in amebasmart_i2s_initialize.
However, in that case we might need to invoke pmu_register_sleep_callback multiple times depending on the port, what do you think about it?

[ziyik]

Yes, that's what I planned to do. The callback will be triggered multiple times as well.
So pmu_register_sleep_callback() will be called during init, and pmu_unregister_sleep_callback() will be called during deinit. Is that okay?

[seokhun-eom24]

Ok, that looks good.

[sunghan-chang]

@ziyik build break on flat_dev_ddr

[seokhun-eom24]

I think the current changes have successfully achieved the purpose of the PR.

However, I’m wondering that in the current implementation all I2S ports are managed by the PM using the same cycle.
What do you think about separating the domain to allow each port to be managed individually by PM?

[ziyik]

I think the current changes have successfully achieved the purpose of the PR.

However, I’m wondering that in the current implementation all I2S ports are managed by the PM using the same cycle. What do you think about separating the domain to allow each port to be managed individually by PM?

By doing so, it will have 2 I2S domains, is it? Currently, only one domain, BSP_I2S_DRV, and driver will handle all I2S ports.
Same for other peripherals, like UART, I2C, SPI and etc. All these peripherals will have multiple channels or interfaces.
This will result in multiple domains (PM) for one peripheral.
In my opinion, the current structure is the most suitable one, with PM control domains, driver register domain, and controls its channel and interfaces. Because only the driver has full control or access to them. (as long as config is set correctly)

[gidori98]

LGTM

[ewoodev]

How about revert it?
it's duplicated calling.

[ziyik]

This is the only place to register the PM domain for i2s.
If this is removed, no other place will register the domain.
I have checked, and it's okay to register 2 times. It will overwrite it.
This will ensure the PM domain is registered when i2s is initialized (without depending/checking which i2s port is initialized)

[sunghan-chang]

@ziyik He asked revert, not remove. Previously, you called the i2s pm init at outside of the i2s init. It called the pm init once, but new change causes twice. We want to prevent duplicated calling. It needs all pm domain string comparison and unnecessary duplicated operation.

[ziyik]

In this case, I will need to revert rtl8730e_boot.c as well.
Otherwise, the PM domain will not be registered.
Is that okay?

[ewoodev]

We believe that if we do not use I2S, it is appropriate not to register I2S-related PM as well.

However, in the current structure, I2S is not differentiated by port-specific PM, so we are currently registering I2S in PM and checking the initialization status of each port based on the presence of priv.

In this structure, it seems clearer for i2s_pminitialize() to be called from board_i2s_initialize(), which is the initialization process for I2S itself, rather than for each port.

Additionally, while the current code only allows overwriting, which does not cause issues with duplicate calls, it would be better to revert rather than allow duplicate calls, as understanding this history may be necessary for future modifications.

[ziyik]

Okay, got it.
I will revert the change, and since the PM sleep/resume has already added the check of the port initialization state (presence of priv), so it's fine.