Patches to bind the SGTL5000 chip to AM33XX McASP
Hi, Mark, Jaroslav,
I've attached a couple patches to provide support for the Freescale SGTL5000 chip when using the McASP on the AM3359.
This adds an optional hack, "clock-to-talk" to the McASP subsystem which can start AHCLKX/AHCLKR prior to initializing the codec. This is needed on the SGTL5000, which depends on AHCLKX for its master clock.
These patches are based off of 3.14.31-ti-r49. What is the process for getting these merged?
Patch #3 is a completely unrelated patch in the SPI system, will send with different subject.
Thanks,
Greg
Quoted 1 line
From
fbcdbfb151d54e6b18c0e04eb5f80dd05527e06bMon Sep 17 00:00:00 2001
From: Greg Knight <[email protected]>
Date: Mon, 23 Feb 2015 19:41:05 -0500
Subject: [PATCH 1/3] davinci-mcasp: add clock-to-talk option to mcasp
device-tree which forces enable of AHCLKX/AHCLKR if set. This allows the AHCLKX line to operate as a main clock for chips such as the SGTL5000.
--- include/linux/platform_data/davinci_asp.h | 14 +++++++++++++ sound/soc/davinci/davinci-mcasp.c | 33 +++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+)
include/linux/platform_data/davinci_asp.h +14 -0
@@ -29,6 +29,15 @@ struct davinci_mcasp_pdata {* when compared to previous behavior. */ unsigned enable_channel_combine:1; + +/** +* If set, we can anticipate device-tree failures during boot +* if the high-speed clock(s) are not running. +* Bit 0: Enable AHCLKX at startup +* Bit 1: Enable AHCLKR at startup +*/ + unsigned clock_to_talk:2; +unsigned sram_size_playback;unsigned sram_size_capture;struct gen_pool *sram_pool;
@@ -102,6 +111,11 @@ enum mcbsp_clk_input_pin { MCBSP_CLKS, }; +enum clock_to_talk_bits { +MCASP_CLOCK_TO_TALK_X =0 , +MCASP_CLOCK_TO_TALK_R +}; +#define INACTIVE_MODE 0#define TX_MODE 1#define RX_MODE 2
sound/soc/davinci/davinci-mcasp.c +33 -0
@@ -1326,6 +1326,15 @@ static struct davinci_mcasp_pdata *davinci_mcasp_set_pdata_from_of(if (ret >=0 ) pdata->sram_size_capture = val; + ret =of_property_read_u32 (np,"clock-to-talk" , &val); +if (ret >=0 ) { +if (val >=4 ) { + ret = -EINVAL ; + goto nodata; + } + pdata->clock_to_talk = (unsigned ) val; + } +return pdata; nodata:
@@ -1337,6 +1346,21 @@ nodata:return pdata; } +int davinci_mcasp_clock_to_talk_hack (struct davinci_mcasp * mcasp,int which) +{ +if (which & (1 <<MCASP_CLOCK_TO_TALK_R )) { +mcasp_set_bits (mcasp,DAVINCI_MCASP_AHCLKRCTL_REG ,AHCLKRE ); +mcasp_set_ctl_reg (mcasp,DAVINCI_MCASP_GBLCTLR_REG ,RXHCLKRST ); +mcasp_set_bits (mcasp,DAVINCI_MCASP_PDIR_REG ,AHCLKR ); + } +if (which & (1 <<MCASP_CLOCK_TO_TALK_X )) { +mcasp_set_bits (mcasp,DAVINCI_MCASP_AHCLKXCTL_REG ,AHCLKXE ); +mcasp_set_ctl_reg (mcasp,DAVINCI_MCASP_GBLCTLR_REG ,TXHCLKRST ); +mcasp_set_bits (mcasp,DAVINCI_MCASP_PDIR_REG ,AHCLKX ); + } +return 0 ; +} +static int davinci_mcasp_probe (struct platform_device *pdev) {struct davinci_pcm_dma_params *dma_params;
@@ -1557,6 +1581,15 @@ static int davinci_mcasp_probe(struct platform_device *pdev) goto err; } +if (pdata->clock_to_talk !=0 ) { + ret =davinci_mcasp_clock_to_talk_hack (mcasp, pdata->clock_to_talk); +dev_dbg (&pdev->dev,"clock to talk: %u\n" , pdata->clock_to_talk); +if (ret) { +dev_err (&pdev->dev,"clock to talk hack failed: %d\n" , ret); + goto err; + } + } +return 0 ; err:
Signature · 1 line
1.9.1
Quoted 1 line
From
0cfd020bac9053326c97d273d60864776e7850e8Mon Sep 17 00:00:00 2001
From: Greg Knight <[email protected]>
Date: Mon, 23 Feb 2015 19:41:40 -0500
Subject: [PATCH 2/3] davinci-evm: add evm_sgtl5000_link to bind the SGTL5000
chip to the davinci-evm SOM module
--- sound/soc/davinci/davinci-evm.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
sound/soc/davinci/davinci-evm.c +13 -0
@@ -468,6 +468,15 @@ static struct snd_soc_dai_link dra7xx_evm_link = { SND_SOC_DAIFMT_IB_NF, }; +static struct snd_soc_dai_link evm_sgtl5000_link = { + .name ="Freescale SGTL5000" , + .stream_name ="SGTL" , + .codec_dai_name ="sgtl5000" , + .ops = &evm_ops, + .dai_fmt =SND_SOC_DAIFMT_CBS_CFS |SND_SOC_DAIFMT_I2S | +SND_SOC_DAIFMT_NB_NF |SND_SOC_DAIFMT_CONT , +}; +static const struct of_device_id davinci_evm_dt_ids[] = { { .compatible ="ti,da830-evm-audio" ,
@@ -481,6 +490,10 @@ static const struct of_device_id davinci_evm_dt_ids[] = { .compatible = "ti,dra7xx-evm-audio", .data = (void *) &dra7xx_evm_link, }, + { + .compatible = "ti,sgtl5000-audio", + .data = &evm_sgtl5000_link + }, {/* sentinel */ } };MODULE_DEVICE_TABLE (of, davinci_evm_dt_ids);
Signature · 1 line
1.9.1
Mark Brown
maintainerQuoted 2 lines · Greg Knight
These patches are based off of 3.14.31-ti-r49. What is the process for getting these merged?
Please submit patches using the process in SubmittingPatches, you need to work against current development versions of the kernel rather than an old vendor kernel.
Quoted 4 lines · Greg Knight
Hi, Mark, Jaroslav,
I've attached a couple patches to provide support for the Freescale SGTL5000 chip when using the McASP on the AM3359.
It is more correct to say that this is needed in a custom hardware which uses McASP and SGTL5000 codec.
Quoted 3 lines
This adds an optional hack, "clock-to-talk" to the McASP subsystem which can start AHCLKX/AHCLKR prior to initializing the codec. This is needed on the SGTL5000, which depends on AHCLKX for its master clock.
SO is it so that the codec's MCLK is coming from McASP AHCLKX (or R) and this clock need to be present all the time? W/o the MCLK the registers are not accessible?
Quoted 2 lines
These patches are based off of 3.14.31-ti-r49. What is the process for getting these merged?
First off, please send the patches with git send-email so people can comment on them.
This will not work in mainline kernel. The McASP is turned off when it is not in use (no audio activity) so the AHCLKX/R is not going to be there on the ball output.
How do you configure the frequency of the AHCLKX/R, this hack only enables it, so you are going to have _some_ clock going out.
You should be using set_clkdiv and set_sysclk calls from the machine driver to configure the AHCLKX (we do not have AHCLKR supported in the driver ATM).
Signature · 5 lines
Péter -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 3 lines
SO is it so that the codec's MCLK is coming from McASP AHCLKX (or R) and this clock need to be present all the time? W/o the MCLK the registers are not accessible?
Correct, the SGTL5000's I2C registers are not accessible without MCLK.
Quoted 1 line
From my testing, the clock need only be present when the audio chip is
in use. When not in use, the chip appears perfectly happy to not be clocked.
Is there a better place to pull a ~12MHz clock signal for use as a master clock from an AM335X?
Quoted 3 lines
This will not work in mainline kernel. The McASP is turned off when it is not in use (no audio activity) so the AHCLKX/R is not going to be there on the ball output.
The clock-to-talk hack in the patch enables the AHCLKX line during codec initialization. From my testing, we don't seem to spend much time communicating with the device when we're not using it. Disabling the clock when it's not used should not be a problem.
Quoted 2 lines
How do you configure the frequency of the AHCLKX/R, this hack only enables it, so you are going to have _some_ clock going out.
The frequency is the 12 MHz (I think) I get out from AHCLKX without any further configuration; I have not investigated changing that frequency as it suited my needs. But I can look into doing set_clkdiv/set_sysclk during the clock-to-talk enable if that's more appropriate?
Quoted 2 lines
You should be using
set_clkdivandset_sysclkcalls from the machine driver to configure the AHCLKX (we do not have AHCLKR supported in the driver ATM).
So, ehm, the SGTL5000 component logic (is this the machine driver?) should adjust the McASP's clocks? Am I misunderstanding? Would this not impact binding the SGTL5000 to devices that are *not* McASPs/break abstraction?
TBH I'm not clear on how the SGTL5000 bindings to other hardware work. Is it safe to assume set_sysclk is always going to adjust the I2S MCLK speed? Please pardon my crude questions, I'm not deeply familiar with the structure of ALSA.
I'll reformat the patches and try from a mainline kernel when I get some spare time.
Regards,
Greg
Quoted 5 lines · Greg Knight
Quoted 3 lines
SO is it so that the codec's MCLK is coming from McASP AHCLKX (or R) and this clock need to be present all the time? W/o the MCLK the registers are not accessible?
Correct, the SGTL5000's I2C registers are not accessible without MCLK.
Yeah, according to the datasheet, it needs MCLK in order to communicate with it.
Quoted 3 lines
From my testing, the clock need only be present when the audio chip is in use. When not in use, the chip appears perfectly happy to not be clocked.
If you change audio controls while you don't have audio activity, you will still need to have the MCLK running.
Quoted 2 lines
Is there a better place to pull a ~12MHz clock signal for use as a master clock from an AM335X?
For a codec such as the SGTL5000 I would connect a clock, which is running all the time. If you take a look at the driver, it enables the clock at probe and leaves it running as long as the driver is loaded.
But AM335x has CLKOUT1,2 outputs as well. They are not high quality clocks, but can be enabled/disabled.
Quoted 8 lines
Quoted 3 lines
This will not work in mainline kernel. The McASP is turned off when it is not in use (no audio activity) so the AHCLKX/R is not going to be there on the ball output.
The clock-to-talk hack in the patch enables the AHCLKX line during codec initialization. From my testing, we don't seem to spend much time communicating with the device when we're not using it. Disabling the clock when it's not used should not be a problem.
As I said, this is not going to work in upstream kernel. The McASP is not even powered up during probe time anymore. It is only powered up when it is needed, this means that you will not have clocks for the codec coming from McASP pin. The driver is not designed to support such a use case, when you use one of it's clock as a master clock for external chip. In other words the McASP driver is not a clock driver.
It might be possible to create a clock driver on top of the McASP driver as child, but it is really something which should not be supported.
Quoted 13 lines
Quoted 3 lines
How do you configure the frequency of the AHCLKX/R, this hack only enables it, so you are going to have _some_ clock going out.
The frequency is the 12 MHz (I think) I get out from AHCLKX without any further configuration; I have not investigated changing that frequency as it suited my needs. But I can look into doing set_clkdiv/set_sysclk during the clock-to-talk enable if that's more appropriate?
Quoted 2 lines
You should be using
set_clkdivandset_sysclkcalls from the machine driver to configure the AHCLKX (we do not have AHCLKR supported in the driver ATM).
Quoted 6 lines
So, ehm, the SGTL5000 component logic (is this the machine driver?) should adjust the McASP's clocks? Am I misunderstanding? Would this not impact binding the SGTL5000 to devices that are *not* McASPs/break abstraction?
TBH I'm not clear on how the SGTL5000 bindings to other hardware work.
It works in other HW because the designers connected the SGTL5000's MCLK to a clock source which can be controlled or it is running all the time. In your case the designers chosen a clock line, which is not working as 'normal' clock.
Quoted 3 lines
Is it safe to assume
set_sysclkis always going to adjust the I2S MCLK speed? Please pardon my crude questions, I'm not deeply familiar with the structure of ALSA.
With the setclkdiv/sysclk you can configure the AHCLKX direction and configure the divider to be used to generate the clock which goes out, or to divide down the clock which is coming in.
Another issue this hack has is: what happens if for some reason the SGTL5000 driver loads first? McASP will not enable the AHCLKX so the codec driver will fail reading the CHIP_ID and fails to load. You need to provide a dummy clock node to the codec, so that will not fail, but you will not have the MCLK.
Quoted 2 lines
I'll reformat the patches and try from a mainline kernel when I get some spare time.
I'm not sure if this can be accepted, sorry. It is really a hack and there are too many corner cases where the system can fail.
Signature · 5 lines
Péter -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 8 lines · Peter Ujfalusi
Another issue this hack has is: what happens if for some reason the SGTL5000
driver loads first? McASP will not enable the AHCLKX so the codec driver will
fail reading theCHIP_IDand fails to load. You need to provide a dummy clock
node to the codec, so that will not fail, but you will not have the MCLK.
Fair enough. The second patch, providing the codec binding to the AM3359, may still be applicable, but I couldn't submit it in good conscience without rewiring the clock as per your recommendations and re-testing. Don't have the time for this in the near future, sadly. Sounds like I'll have to come up with a new fix whenever we get around to updating kernel on this platform.
I'm not familiar with the kernel clock management - any advice for reading material on that? Not clear on what a 'dummy clock' is, etc.
Thanks,
Greg
Quoted 1 line
For a codec such as the SGTL5000 I would connect a clock, which is
running all
Quoted 1 line
the time. If you take a look at the driver, it enables the clock at
probe and
Quoted 1 line
leaves it running as long as the driver is loaded.
Speaking briefly to my "electrical" colleague, he's a bit concerned about the extra power load that a crystal oscillator might incur, while I'm concerned about audio quality from a source like CLKOUT2.
Is there a straightforward way we could enable the clock on-demand - when we're communicating with the codec/mixer or actually using the audio subsystem - while keeping it disabled when not in use? We don't expect to be using the audio subsystem frequently.
Worst-case we can set up a GPIO and enable/disable it as needed in user-space - acceptable, if annoying, for our application.
Thanks,
Greg
Greg,
Quoted 10 lines · Greg Knight
Quoted 1 line
For a codec such as the SGTL5000 I would connect a clock, which is
running all
Quoted 1 line
the time. If you take a look at the driver, it enables the clock at
probe and
Quoted 1 line
leaves it running as long as the driver is loaded.
Speaking briefly to my "electrical" colleague, he's a bit concerned about the extra power load that a crystal oscillator might incur, while I'm concerned about audio quality from a source like CLKOUT2.
Quoted 7 lines
Is there a straightforward way we could enable the clock on-demand - when we're communicating with the codec/mixer or actually using the audio subsystem - while keeping it disabled when not in use? We don't expect to be using the audio subsystem frequently.
Worst-case we can set up a GPIO and enable/disable it as needed in user-space - acceptable, if annoying, for our application.
See my reply to Nikolay.
Signature · 5 lines
Péter -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
maintainerQuoted 4 lines · Greg Knight
Is there a straightforward way we could enable the clock on-demand - when we're communicating with the codec/mixer or actually using the audio subsystem - while keeping it disabled when not in use? We don't expect to be using the audio subsystem frequently.
This is what the clock API is there for - the driver can enable the clock whenver it needs it and disable it when it doesn't.
Hi Peter,
Quoted 9 lines · Peter Ujfalusi
Quoted 5 lines · Greg Knight
Quoted 3 lines
SO is it so that the codec's MCLK is coming from McASP AHCLKX (or R) and this clock need to be present all the time? W/o the MCLK the registers are not accessible?
Correct, the SGTL5000's I2C registers are not accessible without MCLK.
Yeah, according to the datasheet, it needs MCLK in order to communicate with it.
Slight clarification here - I can't find any such reference in the SGTL5000 datasheet where it's explicitly written that the I2C bus *requires* the MCLK running. Unfortunately, all of us found this obscure dependency empirically. One more thing - the codec's I2C works with "any" supported MCLK (8-27 MHz), you can even change the clock on the fly (not highly recommended, but it works).
Quoted 6 lines
Quoted 3 lines
From my testing, the clock need only be present when the audio chip is
in use. When not in use, the chip appears perfectly happy to not be clocked.If you change audio controls while you don't have audio activity, you will still need to have the MCLK running.
Correct. And this is a big issue. As far as I know, the kernel drivers control separately the clock domains, and separately i2c devices, so the basic expectation on the kernel side is that there's no connection between these 2. In this specific case, because of the SGTL5000's implementation, there's a dependency. Right now as I see it, there are several ways to resolve it:
1. Run the reference clock all the time, so the SGTL5000 codec is happy, and DAPM widgets can work as-is. We've been doing this all the time - the reference clocks are routinely configured either in the bootloader, or in the DTS iomuxc node. While this can work in some cases (or until someone touches the same clock or one of its parents :D), there are other cases (like battery-powered devices) where people would want more aggressive power management, which means controlling the reference clock at runtime (see #2).
2. Add "hacks" in the DAPM widgets that add control to the codec's reference clocks. While this seems the preferred route to many, the general feeling is that such approach is not very welcome in upstream.
3. Add explicit support in the kernel's audio subsystem for dependencies between i2c devices and clocks, maybe via "DAPM clock widget" or something like this. Provided that the DAPM graphs are defined properly, this will work out-of-the-box for all use cases, without the hacks (until we see even more twisted cases).
Kind regards,
Nikolay
Quoted 4 lines · Nikolay Dimitrov
Slight clarification here - I can't find any such reference in the SGTL5000 datasheet where it's explicitly written that the I2C bus *requires* the MCLK running. Unfortunately, all of us found this obscure dependency empirically.
It is not spelled out as such, but in:
http://cache.freescale.com/files/analog/doc/data_sheet/SGTL5000.pdf?pspll=1
bottom of page9 (note 1) and the power up sequence in page10. See also page13, RESET section.
Quoted 10 lines
Quoted 2 lines
If you change audio controls while you don't have audio activity, you will still need to have the MCLK running.
Correct. And this is a big issue. As far as I know, the kernel drivers control separately the clock domains, and separately i2c devices, so the basic expectation on the kernel side is that there's no connection between these 2. In this specific case, because of the SGTL5000's implementation, there's a dependency. Right now as I see it, there are several ways to resolve it:
Quoted 8 lines
1. Run the reference clock all the time, so the SGTL5000 codec is happy, and DAPM widgets can work as-is. We've been doing this all the time - the reference clocks are routinely configured either in the bootloader, or in the DTS iomuxc node. While this can work in some cases (or until someone touches the same clock or one of its parents :D), there are other cases (like battery-powered devices) where people would want more aggressive power management, which means controlling the reference clock at runtime (see #2).
Note the the codec driver will enable the clock and will not let it to be turned off. You need to work on the codec driver to sort this out.
Quoted 3 lines
2. Add "hacks" in the DAPM widgets that add control to the codec's reference clocks. While this seems the preferred route to many, the general feeling is that such approach is not very welcome in upstream.
SND_SOC_DAPM_CLOCK_SUPPLY()
Quoted 5 lines
3. Add explicit support in the kernel's audio subsystem for dependencies between i2c devices and clocks, maybe via "DAPM clock widget" or something like this. Provided that the DAPM graphs are defined properly, this will work out-of-the-box for all use cases, without the hacks (until we see even more twisted cases).
This can be handled within the codec driver with some changes. If you have external clock which can be enabled/diabled with a GPIO, then you can use that. We already have binding for this type of external clocks (in linux-next): look for "gpio-gate-clock"
You define your external GPIO controlled clock with this binding and use the phandle in the codec driver.
Change the codec driver to enable/disable the clock when needed. When you do not have audio activity, set the regmap to cache only so any change in the controls will not reach the HW. When you power up next time the regmap will sync the changes to the chip, so you will have the changes commited.
Signature · 5 lines
Péter _______________________________________________ Alsa-devel mailing list [email protected] http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Hi Peter,
Quoted 45 lines · Peter Ujfalusi
Quoted 4 lines · Nikolay Dimitrov
Slight clarification here - I can't find any such reference in the SGTL5000 datasheet where it's explicitly written that the I2C bus *requires* the MCLK running. Unfortunately, all of us found this obscure dependency empirically.
It is not spelled out as such, but in:
http://cache.freescale.com/files/analog/doc/data_sheet/SGTL5000.pdf?pspll=1
bottom of page9 (note 1) and the power up sequence in page10. See also page13, RESET section.Quoted 10 lines
Quoted 2 lines
If you change audio controls while you don't have audio activity, you will still need to have the MCLK running.
Correct. And this is a big issue. As far as I know, the kernel drivers control separately the clock domains, and separately i2c devices, so the basic expectation on the kernel side is that there's no connection between these 2. In this specific case, because of the SGTL5000's implementation, there's a dependency. Right now as I see it, there are several ways to resolve it:
Quoted 8 lines
1. Run the reference clock all the time, so the SGTL5000 codec is happy, and DAPM widgets can work as-is. We've been doing this all the time - the reference clocks are routinely configured either in the bootloader, or in the DTS iomuxc node. While this can work in some cases (or until someone touches the same clock or one of its parents :D), there are other cases (like battery-powered devices) where people would want more aggressive power management, which means controlling the reference clock at runtime (see #2).
Note the the codec driver will enable the clock and will not let it to be turned off. You need to work on the codec driver to sort this out.
Quoted 3 lines
2. Add "hacks" in the DAPM widgets that add control to the codec's reference clocks. While this seems the preferred route to many, the general feeling is that such approach is not very welcome in upstream.
SND_SOC_DAPM_CLOCK_SUPPLY()Quoted 5 lines
3. Add explicit support in the kernel's audio subsystem for dependencies between i2c devices and clocks, maybe via "DAPM clock widget" or something like this. Provided that the DAPM graphs are defined properly, this will work out-of-the-box for all use cases, without the hacks (until we see even more twisted cases).
Quoted 10 lines
This can be handled within the codec driver with some changes. If you have external clock which can be enabled/diabled with a GPIO, then you can use that. We already have binding for this type of external clocks (in linux-next): look for "gpio-gate-clock"
You define your external GPIO controlled clock with this binding and use the phandle in the codec driver.
Change the codec driver to enable/disable the clock when needed. When you do not have audio activity, set the regmap to cache only so any change in the controls will not reach the HW. When you power up next time the regmap will sync the changes to the chip, so you will have the changes commited.
Thanks a lot for your detailed explanation!
Kind regards,
Nikolay
Mark Brown
maintainerQuoted 4 lines · Nikolay Dimitrov
Correct. And this is a big issue. As far as I know, the kernel drivers control separately the clock domains, and separately i2c devices, so the basic expectation on the kernel side is that there's no connection between these 2. In this specific case, because of the SGTL5000's
Not really - it's simply that it's the responsibility of the driver for the device with the dependency to ensure that the dependency is satisfied. There are no general rules about what the requirements of devices are.
Quoted 3 lines
2. Add "hacks" in the DAPM widgets that add control to the codec's reference clocks. While this seems the preferred route to many, the general feeling is that such approach is not very welcome in upstream.
There is no need for any hacks, we already know when register accesses are happening so we can easily add clock enables there (by moving the code from the MMIO layer to the generic layer, this is the first device with this unusual requirement).
Quoted 5 lines
3. Add explicit support in the kernel's audio subsystem for dependencies between i2c devices and clocks, maybe via "DAPM clock widget" or something like this. Provided that the DAPM graphs are defined properly, this will work out-of-the-box for all use cases, without the hacks (until we see even more twisted cases).
There's nothing audio specific about the idea of a device needing a clock for register access, doing something at the audio level is the wrong abstraction layer.