Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

drivers: spi: Add power management Smartbond SPI #73008

Merged
merged 1 commit into from
May 29, 2024
Merged

drivers: spi: Add power management Smartbond SPI #73008

merged 1 commit into from
May 29, 2024

Conversation

kasjer
Copy link
Contributor

@kasjer kasjer commented May 20, 2024

Code adds pm action function that stores SPI configuration before PD_COM is allowed to be turned off.

@zephyrbot zephyrbot added area: SPI SPI bus platform: Renesas SmartBond Renesas Electronics Corporation, SmartBond labels May 20, 2024
ioannis-karachalios
ioannis-karachalios requested changes May 20, 2024
View reviewed changes
drivers/spi/spi_smartbond.c Outdated
config->regs->SPI_CTRL_REG = data->spi_ctrl_reg;
break;
case PM_DEVICE_ACTION_SUSPEND:
config->regs->SPI_CTRL_REG &= ~SPI_SPI_CTRL_REG_SPI_ON_Msk;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we also gate the SPI clock by setting CRG_COM->RESET_CLK_COM_REG = config->periph_clock_config;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not include this only because power saving was only mentioned in SPI_CTR_REG.SPI_EN bit and when measuring current I did not noticed any difference.
In may case when device went to sleep (all PD_COM was down) register was cleared anyway.
If you think it can reduce power consumption when device is put to sleep and SOC is not, I don't have any problem adding this clock dismissal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's do it right away, suggestion applied

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. As you mentioned, in your case PD_COM should be turned off and so CLK_COM_REG should be cleared anyway. However, and since PD_COM is used by other peripherals, it's wise for each block to explicitly gate off its clock before its suspension.

@ioannis-karachalios
Copy link
Contributor

ioannis-karachalios commented May 20, 2024 •
edited

Some more feedback:

  1. Although not critical, I'd be prefer that controller's pins (MOSI, CLK) are configured back to the GPIO input mode when the controller is not active as done with the I2C use case. I think that should also decrease the sleep current at the expense of a slight overhead as PINCTRL_STATE_SLEEP and PINCTRL_STATE_DEFAULT states are switched back and forth when the PM device runtime is not used.
  2. We should add support for the PM device runtime scheme (besides this should be considered the optimum approach as a device is resumed/suspended only when it's required to do so (see I2C use case).

@kasjer
Copy link
Contributor Author

kasjer commented May 28, 2024

Added PM_DEVICE_RUNTIME base on i2c and other drivers

ioannis-karachalios
ioannis-karachalios reviewed May 28, 2024
View reviewed changes
drivers/spi/spi_smartbond.c Outdated

return ret;
}
#endif /* CONFIG_PM_DEVICE */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant #endif - should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

drivers/spi/spi_smartbond.c
/* Gate SPI clocking */
CRG_COM->RESET_CLK_COM_REG = config->periph_clock_config;

ret = pinctrl_apply_state(config->pcfg, PINCTRL_STATE_SLEEP);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the PINCTRL_STATE_SLEEP defined in da1469x_dk_pro-pinctrl.dtsi?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proper dts* files added

drivers/spi/spi_smartbond.c
switch (action) {
case PM_DEVICE_ACTION_RESUME:
da1469x_pd_acquire(MCU_PD_DOMAIN_COM);
ret = spi_smartbond_resume(dev);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the body of spi_smartbond_resume?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

drivers/spi/spi_smartbond.c Outdated
@@ -33,6 +37,12 @@ struct spi_smartbond_cfg {
struct spi_smartbond_data {
struct spi_context ctx;
uint8_t dfs;
#if defined(CONFIG_PM_DEVICE_RUNTIME)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking some steps forward: I'd prefer that power constraints are also applied when CONFIG_PM_DEVICE. The reason is that there is going to be support for asynchronous- (interrupt-driven) and DMA-based transfers. In such a case, the device should not be allowed to enter sleep (standby mode) as long as a requested I2C transaction is fully exercised.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion applied

drivers/spi/spi_smartbond.c
@@ -273,6 +315,70 @@ static int spi_smartbond_init(const struct device *dev)
return 0;
}

#if defined(CONFIG_PM_DEVICE)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also be available when the device runtime scheme is used so the controller can be resumed/suspended per demand.

Copy link
Contributor Author

@kasjer kasjer May 29, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PM_DEVICE_RUNTIME is only available when PM_DEVICE is present

ioannis-karachalios
ioannis-karachalios requested changes May 29, 2024
View reviewed changes
drivers/spi/spi_smartbond.c Outdated
@@ -106,13 +114,41 @@ static inline int spi_smartbond_set_word_size(const struct spi_smartbond_cfg *cf
return 0;
}

static inline void spi_smartbond_pm_policy_state_lock_get(struct spi_smartbond_data *data)
{
#if defined(CONFIG_PM_DEVICE_RUNTIME)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be changed to CONFIG_PM_DEVICE. The same goes for the *_put routine

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@kasjer
drivers: spi: Add power management Smartbond SPI
284cc04 
Code adds pm action function that stores SPI configuration
before PD_COM is allowed to be turned off.

PM_DEVICE_RUNTIME scheme is also supported

Signed-off-by: Jerzy Kasenberg <jerzy.kasenberg@codecoup.pl>
ioannis-karachalios
ioannis-karachalios reviewed May 29, 2024
View reviewed changes
drivers/spi/spi_smartbond.c
@@ -247,7 +287,7 @@ static const struct spi_driver_api spi_smartbond_driver_api = {
.release = spi_smartbond_release,
};

static int spi_smartbond_init(const struct device *dev)
static int spi_smartbond_resume(const struct device *dev)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isspi_context_cs_configure_all required when spi_smartbond_resume is called? Shouldn't be called only once in spi_smartbond_init? Other than that, the code looks good.

@henrikbrixandersen henrikbrixandersen merged commit 71ed2e4 into zephyrproject-rtos:main May 29, 2024
22 checks passed
@kasjer kasjer deleted the kasjer/da1469x-spi-pm branch May 29, 2024 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blauret blauret blauret approved these changes

@ioannis-karachalios ioannis-karachalios ioannis-karachalios approved these changes

@andrzej-kaczmarek andrzej-kaczmarek Awaiting requested review from andrzej-kaczmarek

@tbursztyka tbursztyka Awaiting requested review from tbursztyka

@teburd teburd Awaiting requested review from teburd

Assignees

@ioannis-karachalios ioannis-karachalios

Labels
area: SPI SPI bus platform: Renesas SmartBond Renesas Electronics Corporation, SmartBond
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@kasjer @ioannis-karachalios @blauret @henrikbrixandersen @zephyrbot