mfd: arizona: Factor out SYSCLK enable from wm5102 hardware patch
Subsystems (3)
PATCH 1/5 mfd: arizona: Factor out SYSCLK enable from wm5102 hardware patch
wm5102 applies a custom hardware boot sequence, for this the SYSCLK needs to be enabled. This patch factors out the code that enables SYSCLK for this sequence such that it can be used for other boot time operations that require SYSCLK.
--- drivers/mfd/arizona-core.c | 48 ++++++++++++++++++++++++++++--------------- 1 files changed, 31 insertions(+), 17 deletions(-)
drivers/mfd/arizona-core.c +31 -17
@@ -250,7 +250,8 @@ static int arizona_wait_for_boot(struct arizona *arizona)return ret; } -static int arizona_apply_hardware_patch (struct arizona * arizona) +static int arizona_exec_with_sysclk (struct arizona *arizona, +int (*exec)(struct arizona *)) {unsigned int fll, sysclk;int ret, err;
@@ -292,23 +293,8 @@ static int arizona_apply_hardware_patch(struct arizona* arizona) goto err_fll; } -/* Start the write sequencer and wait for it to finish */ - ret =regmap_write (arizona->regmap,ARIZONA_WRITE_SEQUENCER_CTRL_0 , -ARIZONA_WSEQ_ENA |ARIZONA_WSEQ_START |160 ); -if (ret !=0 ) { -dev_err (arizona->dev,"Failed to start write sequencer: %d\n" , - ret); - goto err_sysclk; - } - ret =arizona_poll_reg (arizona,5 ,ARIZONA_WRITE_SEQUENCER_CTRL_1 , -ARIZONA_WSEQ_BUSY ,0 ); -if (ret !=0 ) { -regmap_write (arizona->regmap,ARIZONA_WRITE_SEQUENCER_CTRL_0 , -ARIZONA_WSEQ_ABORT ); - ret = -ETIMEDOUT ; - } + ret =exec (arizona); -err_sysclk: err =regmap_write (arizona->regmap,ARIZONA_SYSTEM_CLOCK_1 , sysclk);if (err !=0 ) { dev_err(arizona->dev,
@@ -330,6 +316,34 @@ err_fll:return err; } +static int arizona_hardware_patch_wseq (struct arizona *arizona) +{ +int ret; + +/* Start the write sequencer and wait for it to finish */ + ret =regmap_write (arizona->regmap,ARIZONA_WRITE_SEQUENCER_CTRL_0 , +ARIZONA_WSEQ_ENA |ARIZONA_WSEQ_START |160 ); +if (ret !=0 ) { +dev_err (arizona->dev,"Failed to start write sequencer: %d\n" , + ret); +return ret; + } + ret =arizona_poll_reg (arizona,5 ,ARIZONA_WRITE_SEQUENCER_CTRL_1 , +ARIZONA_WSEQ_BUSY ,0 ); +if (ret !=0 ) { +regmap_write (arizona->regmap,ARIZONA_WRITE_SEQUENCER_CTRL_0 , +ARIZONA_WSEQ_ABORT ); + ret = -ETIMEDOUT ; + } + +return ret; +} + +static inline int arizona_apply_hardware_patch (struct arizona *arizona) +{ +return arizona_exec_with_sysclk (arizona, arizona_hardware_patch_wseq); +} +#ifdef CONFIG_PM static int arizona_runtime_resume (struct device *dev) {
Signature · 1 line
1.7.2.5
PATCH 2/5 mfd: wm5110: Add register patch required for low power sleep
Some register settings must be applied before the first time low power sleep mode is entered on the wm5110 to ensure optimium performance. These settings require SYSCLK to be enabled whilst they are being applied. This patch applies the settings using the recently factored out boot time SYSCLK functionality.
--- drivers/mfd/arizona-core.c | 29 +++++++++++++++++++++++++++++ 1 files changed, 29 insertions(+), 0 deletions(-)
drivers/mfd/arizona-core.c +29 -0
@@ -344,6 +344,25 @@ static inline int arizona_apply_hardware_patch(struct arizona *arizona)return arizona_exec_with_sysclk (arizona, arizona_hardware_patch_wseq); } +static const struct reg_default wm5110_sleep_patch[] = { + {0x337A ,0xC100 }, + {0x337B ,0x0041 }, + {0x3300 ,0xA210 }, + {0x3301 ,0x050C }, +}; + +static inline int wm5110_sleep_patch_wseq (struct arizona *arizona) +{ +return regmap_multi_reg_write_bypassed (arizona->regmap, + wm5110_sleep_patch, +ARRAY_SIZE (wm5110_sleep_patch)); +} + +static inline int wm5110_apply_sleep_patch (struct arizona *arizona) +{ +return arizona_exec_with_sysclk (arizona, wm5110_sleep_patch_wseq); +} +#ifdef CONFIG_PM static int arizona_runtime_resume (struct device *dev) {
@@ -913,6 +932,16 @@ int arizona_dev_init(struct arizona *arizona) goto err_reset; }break ; +case WM5110 : +case WM8280 : + ret =wm5110_apply_sleep_patch (arizona); +if (ret !=0 ) { +dev_err (arizona->dev, +"Failed to apply sleep patch: %d\n" , + ret); + goto err_reset; + } +break ;default :break ; }
Signature · 1 line
1.7.2.5
PATCH 3/5 mfd: wm5110: Add delay before releasing reset line on cold boot
In reply to Charles KeepaxOn the first boot of the wm5110 it is important the reset line is held for slightly longer to ensure the device starts up well. This patch adds a 5mS delay for this.
--- drivers/mfd/arizona-core.c | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-)
drivers/mfd/arizona-core.c +9 -0
@@ -784,6 +784,15 @@ int arizona_dev_init(struct arizona *arizona) goto err_enable; } +switch (arizona->type) { +case WM5110 : +case WM8280 : +msleep (5 ); +break ; +default : +break ; + } +if (arizona->pdata.reset) {gpio_set_value_cansleep (arizona->pdata.reset,1 );msleep (1 );
Signature · 1 line
1.7.2.5
Mark Brown
maintainerRe: [PATCH 3/5] mfd: wm5110: Add delay before releasing reset line on cold boot
Quoted 3 lines · Charles Keepax
On the first boot of the wm5110 it is important the reset line is held for slightly longer to ensure the device starts up well. This patch adds a 5mS delay for this.
How can we tell what first boot is - what happens if the device is fully powered off during system suspend for example? I'd expect to see this done for system resume as well if we don't know power was maintained (or whatever else the distinction is).
Re: [PATCH 3/5] mfd: wm5110: Add delay before releasing reset line on cold boot
Quoted 10 lines · Mark Brown
Quoted 3 lines · Charles Keepax
On the first boot of the wm5110 it is important the reset line is held for slightly longer to ensure the device starts up well. This patch adds a 5mS delay for this.
How can we tell what first boot is - what happens if the device is fully powered off during system suspend for example? I'd expect to see this done for system resume as well if we don't know power was maintained (or whatever else the distinction is).
Internally the device has some state, so effectively we define the first boot as the first time DCVDD is applied since either the last physical reset or time the core_supplies were missing.
I think your suspend example is pretty tricky, we enable the regulators for the core_supplies at boot, so I guess we have requested that the system never removes those so if it does so anyway perhaps that is a system problem? There isn't really anyway to tell if they were removed, since we can't talk to the CODEC without DCVDD (even if we could I don't think we can find out) and we need to know before we power it up. Also presumably if the system removed the regulator when we told it not to it won't report anything through the regulator framework.
That would leave the only possible solution being a hard reset during every runtime resume but that makes me very nervous about the AoD interrupts as state for those would be lost upon that reset.
All in all, I really struggle to see what more the driver could do here.
Thanks,
Charles
Mark Brown
maintainerRe: [PATCH 3/5] mfd: wm5110: Add delay before releasing reset line on cold boot
Quoted 4 lines · Charles Keepax
I think your suspend example is pretty tricky, we enable the regulators for the
core_suppliesat boot, so I guess we have requested that the system never removes those so if it does so anyway perhaps that is a system problem? There isn't really
No, there's no guarantee that the current state is maintained over system suspend - system suspend can turn anything off (at least from an API point of view).
Quoted 4 lines
That would leave the only possible solution being a hard reset during every runtime resume but that makes me very nervous about the AoD interrupts as state for those would be lost upon that reset.
No, you're guaranteed that the supply will stay on while the system is running so runtime PM is not an issue - it's system suspend that's an issue.
Quoted 2 lines
All in all, I really struggle to see what more the driver could do here.
As I suggested in my original reply handle system suspend.
Re: [PATCH 3/5] mfd: wm5110: Add delay before releasing reset line on cold boot
Quoted 16 lines · Mark Brown
Quoted 4 lines · Charles Keepax
I think your suspend example is pretty tricky, we enable the regulators for the
core_suppliesat boot, so I guess we have requested that the system never removes those so if it does so anyway perhaps that is a system problem? There isn't reallyNo, there's no guarantee that the current state is maintained over system suspend - system suspend can turn anything off (at least from an API point of view).
Quoted 4 lines
That would leave the only possible solution being a hard reset during every runtime resume but that makes me very nervous about the AoD interrupts as state for those would be lost upon that reset.
Quoted 8 lines
No, you're guaranteed that the supply will stay on while the system is running so runtime PM is not an issue - it's system suspend that's an issue.
Quoted 2 lines
All in all, I really struggle to see what more the driver could do here.
As I suggested in my original reply handle system suspend.
Just to confirm when we say system suspend here we are talking about the suspend and resume callbacks in dev_pm_ops? As I am slightly concerned I have my wires very crossed here.
Assuming I don't have my wires crossed.
There are use-cases were we expect the CODEC to be powered up across system suspend, is that something we should not expect to be guaranteed possible? For example compressed off-load or always on voice.
If these use-cases are expected to be reasonable then it would be reasonable to assume the regulators would not be removed if a runtime reference to the CODEC is held. If we can't assume that then how do we know if we should reset the CODEC?
So I guess we could reset the chip in system resume if no runtime references are held, but that still has the same problem as resetting in runtime resume, the chip may have been active running jack detection and you have just blatted the settings/state. I guess you could have the extcon driver restore the settings at least in its system resume, but it really feels like we are likely to be introducing issues worse than those this patch is there to fix.
Apologies if I am missing something very basic here. Does this really need to be done before this patch could be accepted?
Thanks,
Charles
Mark Brown
maintainerRe: [PATCH 3/5] mfd: wm5110: Add delay before releasing reset line on cold boot
Quoted 7 lines · Charles Keepax
Quoted 1 line · Mark Brown
As I suggested in my original reply handle system suspend.
Just to confirm when we say system suspend here we are talking about the suspend and resume callbacks in
dev_pm_ops? As I am slightly concerned I have my wires very crossed here.
Yes.
Quoted 4 lines
There are use-cases were we expect the CODEC to be powered up across system suspend, is that something we should not expect to be guaranteed possible? For example compressed off-load or always on voice.
Sure, that's perfectly fine - jack detection is also very common.
Quoted 4 lines
If these use-cases are expected to be reasonable then it would be reasonable to assume the regulators would not be removed if a runtime reference to the CODEC is held. If we can't assume that then how do we know if we should reset the CODEC?
Given jack detection I don't think you can base this purely on the CODEC part of the device.
Quoted 11 lines
So I guess we could reset the chip in system resume if no runtime references are held, but that still has the same problem as resetting in runtime resume, the chip may have been active running jack detection and you have just blatted the settings/state. I guess you could have the extcon driver restore the settings at least in its system resume, but it really feels like we are likely to be introducing issues worse than those this patch is there to fix.
Apologies if I am missing something very basic here. Does this really need to be done before this patch could be accepted?
It seems easy enough to check if the device is active and it's very common for drivers to do this (wm8994 has an example) - for most uses you should be able to check pm_runtime_is_enabled() and there should just be a single bitfield you can look at to figure out if jack detection was running. Looking at the extcon driver briefly ARIZONA_JD1_ENA looks hopeful.
Re: [PATCH 3/5] mfd: wm5110: Add delay before releasing reset line on cold boot
Quoted 2 lines · Mark Brown
Quoted 1 line · Charles Keepax
On Mon, Mar 16, 2015 at 08:47:10PM +0000, Mark Brown wrote:
<snip>
Quoted 6 lines
It seems easy enough to check if the device is active and it's very common for drivers to do this (wm8994 has an example) - for most uses you should be able to check
pm_runtime_is_enabled()and there should just be a single bitfield you can look at to figure out if jack detection was running. Looking at the extcon driver brieflyARIZONA_JD1_ENAlooks hopeful.
Cool ok, sorry about that. All makes more sense now, thanks. I will respin.
Thanks,
Charles
PATCH 4/5 regulator: arizona-ldo1: Add additional supported voltage
In reply to Charles KeepaxThis patch adds support for the 1.175V mode on the LDO1 regulator on the wm5110. This is need as part of the low power sleep mode operation.
--- drivers/regulator/arizona-ldo1.c | 15 +++++++++++++++ 1 files changed, 15 insertions(+), 0 deletions(-)
drivers/regulator/arizona-ldo1.c +15 -0
@@ -178,6 +178,16 @@ static const struct regulator_init_data arizona_ldo1_default = { .num_consumer_supplies = 1, }; +static const struct regulator_init_data arizona_ldo1_wm5110 = { + .constraints = { + .min_uV =1175000 , + .max_uV =1200000 , + .valid_ops_mask =REGULATOR_CHANGE_STATUS | +REGULATOR_CHANGE_VOLTAGE , + }, + .num_consumer_supplies =1 , +}; +static int arizona_ldo1_of_get_pdata (struct arizona *arizona,struct regulator_config *config,const struct regulator_desc *desc)
@@ -243,6 +253,11 @@ static int arizona_ldo1_probe(struct platform_device *pdev) desc = &arizona_ldo1_hc; ldo1->init_data = arizona_ldo1_dvfs;break ; +case WM5110 : +case WM8280 : + desc = &arizona_ldo1; + ldo1->init_data = arizona_ldo1_wm5110; +break ;default : desc = &arizona_ldo1; ldo1->init_data = arizona_ldo1_default;
Signature · 1 line
1.7.2.5
Re: [PATCH 4/5] regulator: arizona-ldo1: Add additional supported voltage
Quoted 2 lines · Charles Keepax
This patch adds support for the 1.175V mode on the LDO1 regulator on the wm5110. This is need as part of the low power sleep mode operation.
PATCH 5/5 mfd: wm5110: Set DCVDD voltage to 1.175V before entering sleep mode
In reply to Charles KeepaxThe low power sleep mode on wm5110 requires that the LDO1 regulator be set to 1.175V prior to entering sleep, then returned to 1.2V after exiting sleep mode. This patch apply these regulator settings.
--- drivers/mfd/arizona-core.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 42 insertions(+), 0 deletions(-)
drivers/mfd/arizona-core.c +42 -0
@@ -407,6 +407,32 @@ static int arizona_runtime_resume(struct device *dev) goto err; }break ; +case WM5110 : +case WM8280 : + ret =arizona_wait_for_boot (arizona); +if (ret !=0 ) + goto err; + +if (arizona->external_dcvdd) { + ret =regmap_update_bits (arizona->regmap, +ARIZONA_ISOLATION_CONTROL , +ARIZONA_ISOLATE_DCVDD1 ,0 ); +if (ret !=0 ) { +dev_err (arizona->dev, +"Failed to connect DCVDD: %d\n" , ret); + goto err; + } + }else { + ret =regulator_set_voltage (arizona->dcvdd, +1200000 ,1200000 ); +if (ret <0 ) { +dev_err (arizona->dev, +"Failed to set resume voltage: %d\n" , + ret); + goto err; + } + } +break ;default : ret =arizona_wait_for_boot (arizona);if (ret !=0 ) {
@@ -457,6 +483,22 @@ static int arizona_runtime_suspend(struct device *dev) ret);return ret; } + }else { +switch (arizona->type) { +case WM5110 : +case WM8280 : + ret =regulator_set_voltage (arizona->dcvdd, +1175000 ,1175000 ); +if (ret <0 ) { +dev_err (arizona->dev, +"Failed to set suspend voltage: %d\n" , + ret); +return ret; + } +break ; +default : +break ; + } }regcache_cache_only (arizona->regmap, true);
Signature · 1 line
1.7.2.5
Mark Brown
maintainerRe: [PATCH 5/5] mfd: wm5110: Set DCVDD voltage to 1.175V before entering sleep mode
drivers/mfd/arizona-core.c static int arizona_runtime_suspend(struct device *dev) · line 492
Quoted 7 lines · Charles Keepax
drivers/mfd/arizona-core.c +7
-0+ }else { +switch (arizona->type) { +case WM5110 : +case WM8280 : + ret =regulator_set_voltage (arizona->dcvdd, +1175000 ,1175000 ); +if (ret <0 ) {
Can we please have a comment about this being the internal LDO and us therefore knowing exactly what voltages it supports (or just that it's the on chip LDO)? Going for a single voltage with zero tolerance is usually bad practice since it causes needless interoperability issues (eg, here the chances that the hardware would mind if an external regulator were only able to deliver 1.174V rather than 1.175V are minimal). This is fine here but people might be looking for examples and get the wrong idea.
Re: [PATCH 5/5] mfd: wm5110: Set DCVDD voltage to 1.175V before entering sleep mode
Quoted 18 lines · Mark Brown
drivers/mfd/arizona-core.c static int arizona_runtime_suspend(struct device *dev) · line 492
Quoted 7 lines · Charles Keepax
drivers/mfd/arizona-core.c +7
-0+ }else { +switch (arizona->type) { +case WM5110 : +case WM8280 : + ret =regulator_set_voltage (arizona->dcvdd, +1175000 ,1175000 ); +if (ret <0 ) {Can we please have a comment about this being the internal LDO and us therefore knowing exactly what voltages it supports (or just that it's the on chip LDO)? Going for a single voltage with zero tolerance is usually bad practice since it causes needless interoperability issues (eg, here the chances that the hardware would mind if an external regulator were only able to deliver 1.174V rather than 1.175V are minimal). This is fine here but people might be looking for examples and get the wrong idea.
Yeah no problem I will respin and add that.
Thanks,
Charles