Re: [alsa-devel] Patches to bind the SGTL5000 chip to AM33XX McASP
On 03/19/2015 04:34 PM, Greg Knight wrote:
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.
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.
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.
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.
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?
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).
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.
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.
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.
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.
Hi Peter,
On 03/19/2015 06:07 PM, Peter Ujfalusi wrote:
On 03/19/2015 04:34 PM, Greg Knight wrote:
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).
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
On 03/19/2015 08:06 PM, Nikolay Dimitrov wrote:
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.
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:
- 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.
- 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()
- 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.
Hi Peter,
On 03/20/2015 10:05 AM, Peter Ujfalusi wrote:
On 03/19/2015 08:06 PM, Nikolay Dimitrov wrote:
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.
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:
- 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.
- 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()
- 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.
Thanks a lot for your detailed explanation!
Kind regards, Nikolay
On Thu, Mar 19, 2015 at 08:06:15PM +0200, Nikolay Dimitrov wrote:
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.
- 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).
- 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.
participants (3)
-
Mark Brown -
Nikolay Dimitrov -
Peter Ujfalusi