| Message ID | 20200409195841.18901-3-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | ASoC/SOF/clk/gpio/dt: add Hifiberry DAC+ PRO support | expand |
On Thu, Apr 09, 2020 at 02:58:27PM -0500, Pierre-Louis Bossart wrote: > Using devm_clk_get() with a NULL string fails on ACPI platforms, use > the "sclk" string as a fallback. This is fishy a bit. Do we have this name in the bindings? If no, why not simple switch to devm_clk_get_optional()?
On Thu, Apr 09, 2020 at 02:58:27PM -0500, Pierre-Louis Bossart wrote: > Using devm_clk_get() with a NULL string fails on ACPI platforms, use > the "sclk" string as a fallback. Is this something that could be fixed at the ACPI level?
On 4/14/20 12:11 PM, Andy Shevchenko wrote: > On Thu, Apr 09, 2020 at 02:58:27PM -0500, Pierre-Louis Bossart wrote: >> Using devm_clk_get() with a NULL string fails on ACPI platforms, use >> the "sclk" string as a fallback. > > This is fishy a bit. I didn't find a single example where we use a NULL string in ACPI cases? > Do we have this name in the bindings? No, that's probably a miss > If no, why not simple switch to devm_clk_get_optional()? Not sure what that would change?
On 4/14/20 12:45 PM, Mark Brown wrote: > On Thu, Apr 09, 2020 at 02:58:27PM -0500, Pierre-Louis Bossart wrote: >> Using devm_clk_get() with a NULL string fails on ACPI platforms, use >> the "sclk" string as a fallback. > > Is this something that could be fixed at the ACPI level? I guess to fix this we'd need some sort of ACPI-level connection or description of the clock, and I've never seen such a description? All the examples I've seen use an explicit 'mclk' string (that's e.g. what we did for the PMC clocks for Baytrail/Cherrytrail machine drivers, we added a lookup). Here I used 'sclk' since it's what TI refers to in their documentation. I vaguely recall AMD had similar issues with another codec.
On Tue, Apr 14, 2020 at 01:14:41PM -0500, Pierre-Louis Bossart wrote: > On 4/14/20 12:45 PM, Mark Brown wrote: > > On Thu, Apr 09, 2020 at 02:58:27PM -0500, Pierre-Louis Bossart wrote: > > > Using devm_clk_get() with a NULL string fails on ACPI platforms, use > > > the "sclk" string as a fallback. > > Is this something that could be fixed at the ACPI level? > I guess to fix this we'd need some sort of ACPI-level connection or > description of the clock, and I've never seen such a description? Wait, so SCLK is in the *global* namespace and the provider has to register the same name? That doesn't sound clever. It might be better to have the board register the connection from the clock provider to the device rather than hard code global namespace strings like this, that sounds like a recipie for misery. It is really sad that nobody involved in producing these systems that don't work with the current limitations in ACPI has been able to make progress on improving ACPI so it can cope with modern hardware and we're having to deal with this stuff. > All the examples I've seen use an explicit 'mclk' string (that's e.g. what > we did for the PMC clocks for Baytrail/Cherrytrail machine drivers, we added > a lookup). Here I used 'sclk' since it's what TI refers to in their > documentation. They appear to call it SCK not SCLK.
On 4/14/20 1:27 PM, Mark Brown wrote: > On Tue, Apr 14, 2020 at 01:14:41PM -0500, Pierre-Louis Bossart wrote: >> On 4/14/20 12:45 PM, Mark Brown wrote: >>> On Thu, Apr 09, 2020 at 02:58:27PM -0500, Pierre-Louis Bossart wrote: > >>>> Using devm_clk_get() with a NULL string fails on ACPI platforms, use >>>> the "sclk" string as a fallback. > >>> Is this something that could be fixed at the ACPI level? > >> I guess to fix this we'd need some sort of ACPI-level connection or >> description of the clock, and I've never seen such a description? > > Wait, so SCLK is in the *global* namespace and the provider has to > register the same name? That doesn't sound clever. It might be better > to have the board register the connection from the clock provider to the > device rather than hard code global namespace strings like this, that > sounds like a recipie for misery. I believe this change has zero impact on DT platforms. The 'sclk' is a fallback here. If you find a clock with the NULL string, it's what gets used. Likewise for the clock provider, the 'sclk' is a lookup - an alias in other words. The use of the references and phandles should work just fine for Device Tree. > It is really sad that nobody involved in producing these systems that > don't work with the current limitations in ACPI has been able to make > progress on improving ACPI so it can cope with modern hardware and we're > having to deal with this stuff. I can't disagree but I have to live with what's available to me as an audio guy...I had a solution two years ago where I could set the clock directly from the machine driver. The recommendation at the time was to use the clk framework, but that clk framework is limited for ACPI platforms, so we can only use it with these global names. We had the same problem on Baytrail/Cherrytrail devices some 4 years ago and we had to use an 'mclk' alias. We are going to have the same problem when we expose the SSP MCLK, BLCK and FSYNC clocks - and that's also what the Skylake driver did - we don't have a solution without global names. >> All the examples I've seen use an explicit 'mclk' string (that's e.g. what >> we did for the PMC clocks for Baytrail/Cherrytrail machine drivers, we added >> a lookup). Here I used 'sclk' since it's what TI refers to in their >> documentation. > > They appear to call it SCK not SCLK. Yes indeed, will change.
On Tue, Apr 14, 2020 at 02:15:16PM -0500, Pierre-Louis Bossart wrote: > On 4/14/20 1:27 PM, Mark Brown wrote: > > Wait, so SCLK is in the *global* namespace and the provider has to > > register the same name? That doesn't sound clever. It might be better > > to have the board register the connection from the clock provider to the > > device rather than hard code global namespace strings like this, that > > sounds like a recipie for misery. > I believe this change has zero impact on DT platforms. > The 'sclk' is a fallback here. If you find a clock with the NULL string, > it's what gets used. Likewise for the clock provider, the 'sclk' is a lookup > - an alias in other words. The use of the references and phandles should > work just fine for Device Tree. It's not just DT platforms that I'm worried about here, it's also ACPI systems - all it takes is for a system to have a second device and a name collision could happen, especially with such generic names. We tried to avoid doing this for board files for the same reason. > > It is really sad that nobody involved in producing these systems that > > don't work with the current limitations in ACPI has been able to make > > progress on improving ACPI so it can cope with modern hardware and we're > > having to deal with this stuff. > I can't disagree but I have to live with what's available to me as an audio > guy...I had a solution two years ago where I could set the clock directly > from the machine driver. The recommendation at the time was to use the clk > framework, but that clk framework is limited for ACPI platforms, so we can > only use it with these global names. My understanding is that ACPI just doesn't have clock bindings (or audio bindings or...) so you're basically using board files here and board files can definitely do more than we're seeing here. > We had the same problem on Baytrail/Cherrytrail devices some 4 years ago and > we had to use an 'mclk' alias. We are going to have the same problem when we > expose the SSP MCLK, BLCK and FSYNC clocks - and that's also what the > Skylake driver did - we don't have a solution without global names. You should be able to register links between devices using the clock API, or add that functionality if it's not there but AFAIK clkdev still works.
On 4/14/20 2:50 PM, Mark Brown wrote: > On Tue, Apr 14, 2020 at 02:15:16PM -0500, Pierre-Louis Bossart wrote: >> On 4/14/20 1:27 PM, Mark Brown wrote: > >>> Wait, so SCLK is in the *global* namespace and the provider has to >>> register the same name? That doesn't sound clever. It might be better >>> to have the board register the connection from the clock provider to the >>> device rather than hard code global namespace strings like this, that >>> sounds like a recipie for misery. > >> I believe this change has zero impact on DT platforms. > >> The 'sclk' is a fallback here. If you find a clock with the NULL string, >> it's what gets used. Likewise for the clock provider, the 'sclk' is a lookup >> - an alias in other words. The use of the references and phandles should >> work just fine for Device Tree. > > It's not just DT platforms that I'm worried about here, it's also ACPI > systems - all it takes is for a system to have a second device and a > name collision could happen, especially with such generic names. We > tried to avoid doing this for board files for the same reason. I am on the paranoid side but here I don't see much potential for conflicts: a) this only works for the Up2 board with a HAT connector b) this only work with the Hifiberry DAC+ PRO board. This codec is not used in any traditional client devices. > >>> It is really sad that nobody involved in producing these systems that >>> don't work with the current limitations in ACPI has been able to make >>> progress on improving ACPI so it can cope with modern hardware and we're >>> having to deal with this stuff. > >> I can't disagree but I have to live with what's available to me as an audio >> guy...I had a solution two years ago where I could set the clock directly >> from the machine driver. The recommendation at the time was to use the clk >> framework, but that clk framework is limited for ACPI platforms, so we can >> only use it with these global names. > > My understanding is that ACPI just doesn't have clock bindings (or audio > bindings or...) so you're basically using board files here and board > files can definitely do more than we're seeing here. I don't understand your definition of board file, sorry. We've never had one, the only thing that's board-specific is the machine driver. >> We had the same problem on Baytrail/Cherrytrail devices some 4 years ago and >> we had to use an 'mclk' alias. We are going to have the same problem when we >> expose the SSP MCLK, BLCK and FSYNC clocks - and that's also what the >> Skylake driver did - we don't have a solution without global names. > > You should be able to register links between devices using the clock > API, or add that functionality if it's not there but AFAIK clkdev still > works. The machine driver has no information whatsoever on who provides the clock. I just don't see how I might link stuff without at least some amount of information? All I needed was to toggle 2 gpios to select 44.1 or 48kHz...Looks like it's going to take two more years, oh well.
>>>> Wait, so SCLK is in the *global* namespace and the provider has to >>>> register the same name? That doesn't sound clever. It might be better >>>> to have the board register the connection from the clock provider to >>>> the >>>> device rather than hard code global namespace strings like this, that >>>> sounds like a recipie for misery. Thinking a bit more on this, is the objection on the notion of using a fixed string, on the way it's registered or the lack of support for clocks in ACPI? From a quick look, the use of a fixed string is rather prevalent, see below. Less than 10% of codec drivers rely on a NULL string, so is it really a dangerous precedent so use "sclk" in this case? It seems to me that all clk providers need to use a unique string - what am I missing here? adau17x1.c: adau->mclk = devm_clk_get(dev, "mclk"); cs42l51.c: cs42l51->mclk_handle = devm_clk_get(dev, "MCLK"); cs42xx8.c: cs42xx8->clk = devm_clk_get(dev, "mclk"); cs53l30.c: cs53l30->mclk = devm_clk_get(dev, "mclk"); cx2072x.c: cx2072x->mclk = devm_clk_get(cx2072x->dev, "mclk"); da7213.c: da7213->mclk = devm_clk_get(component->dev, "mclk"); da7218.c: da7218->mclk = devm_clk_get(component->dev, "mclk"); da7219.c: da7219->mclk = devm_clk_get(component->dev, "mclk"); es8316.c: es8316->mclk = devm_clk_get_optional(component->dev, "mclk"); es8328.c: es8328->clk = devm_clk_get(component->dev, NULL); inno_rk3036.c: priv->pclk = devm_clk_get(&pdev->dev, "acodec_pclk"); jz4725b.c: icdc->clk = devm_clk_get(&pdev->dev, "aic"); jz4770.c: codec->clk = devm_clk_get(dev, "aic"); lochnagar-sc.c: priv->mclk = devm_clk_get(&pdev->dev, "mclk"); max98088.c: max98088->mclk = devm_clk_get(&i2c->dev, "mclk"); max98090.c: max98090->mclk = devm_clk_get(component->dev, "mclk"); max98095.c: max98095->mclk = devm_clk_get(component->dev, "mclk"); max9860.c: mclk = clk_get(dev, "mclk"); msm8916-wcd-analog.c: priv->mclk = devm_clk_get(dev, "mclk"); msm8916-wcd-digital.c: priv->ahbclk = devm_clk_get(dev, "ahbix-clk"); msm8916-wcd-digital.c: priv->mclk = devm_clk_get(dev, "mclk"); msm8916-wcd-digital.c: mclk_rate = clk_get_rate(msm8916_wcd->mclk); nau8825.c: nau8825->mclk = devm_clk_get(nau8825->dev, "mclk"); nau8825.c: nau8825->mclk = devm_clk_get(dev, "mclk"); pcm3168a.c: pcm3168a->scki = devm_clk_get(dev, "scki"); pcm512x.c: pcm512x->sclk = devm_clk_get(dev, NULL); rk3328_codec.c: rk3328->mclk = devm_clk_get(&pdev->dev, "mclk"); rk3328_codec.c: rk3328->pclk = devm_clk_get(&pdev->dev, "pclk"); rt5514.c: rt5514->mclk = devm_clk_get(component->dev, "mclk"); rt5616.c: rt5616->mclk = devm_clk_get(component->dev, "mclk"); rt5640.c: rt5640->mclk = devm_clk_get(component->dev, "mclk"); rt5659.c: rt5659->mclk = devm_clk_get(&i2c->dev, "mclk"); rt5660.c: rt5660->mclk = devm_clk_get(&i2c->dev, "mclk"); rt5682.c: rt5682->mclk = devm_clk_get(component->dev, "mclk"); sirf-audio-codec.c: sirf_audio_codec->clk = devm_clk_get(&pdev->dev, NULL); sta32x.c: sta32x->xti_clk = devm_clk_get(dev, "xti"); tas571x.c: priv->mclk = devm_clk_get(dev, "mclk"); tlv320aic32x4.c: pll = devm_clk_get(component->dev, "pll"); tscs42xx.c: tscs42xx->sysclk = devm_clk_get(&i2c->dev, src_names[src]); tscs454.c: freq = clk_get_rate(tscs454->sysclk); tscs454.c: tscs454->sysclk = devm_clk_get(&i2c->dev, src_names[src]); wcd9335.c: wcd->mclk = devm_clk_get(dev, "mclk"); wcd9335.c: wcd->native_clk = devm_clk_get(dev, "slimbus"); wm2000.c: wm2000->mclk = devm_clk_get(&i2c->dev, "MCLK"); wm8731.c: wm8731->mclk = devm_clk_get(&spi->dev, "mclk"); wm8731.c: wm8731->mclk = devm_clk_get(&i2c->dev, "mclk"); wm8904.c: wm8904->mclk = devm_clk_get(&i2c->dev, "mclk"); wm8960.c: wm8960->mclk = devm_clk_get(&i2c->dev, "mclk"); wm8962.c: pdata->mclk = devm_clk_get(&i2c->dev, NULL);
On Tue, Apr 14, 2020 at 12:54:25PM -0500, Pierre-Louis Bossart wrote: > On 4/14/20 12:11 PM, Andy Shevchenko wrote: > > On Thu, Apr 09, 2020 at 02:58:27PM -0500, Pierre-Louis Bossart wrote: > > > Using devm_clk_get() with a NULL string fails on ACPI platforms, use > > > the "sclk" string as a fallback. > > > > This is fishy a bit. > > I didn't find a single example where we use a NULL string in ACPI cases? ... > > If no, why not simple switch to devm_clk_get_optional()? > > Not sure what that would change? Hmm... Who is the provider of this clock?
On Tue, Apr 14, 2020 at 04:02:00PM -0500, Pierre-Louis Bossart wrote: > Thinking a bit more on this, is the objection on the notion of using a fixed > string, on the way it's registered or the lack of support for clocks in > ACPI? The issue is using a clock named in the global namespace. Like I keep saying you're not using ACPI here, you're using board files and board files can do better. > From a quick look, the use of a fixed string is rather prevalent, see below. > Less than 10% of codec drivers rely on a NULL string, so is it really a > dangerous precedent so use "sclk" in this case? It seems to me that all clk > providers need to use a unique string - what am I missing here? > adau17x1.c: adau->mclk = devm_clk_get(dev, "mclk"); Notice how all the clock lookup functions take both a device and a string - the device is important here, the string is namespaced with the device in most usage (including board file usage) so if two different devices ask for the same name they might get different clocks. > wm8962.c: pdata->mclk = devm_clk_get(&i2c->dev, NULL); This is how lookups that don't even specify a name can work. You seem to want to rely on the name only which is very much not good practice, even on board files.
On Tue, Apr 14, 2020 at 03:13:01PM -0500, Pierre-Louis Bossart wrote: > On 4/14/20 2:50 PM, Mark Brown wrote: > > It's not just DT platforms that I'm worried about here, it's also ACPI > > systems - all it takes is for a system to have a second device and a > > name collision could happen, especially with such generic names. We > > tried to avoid doing this for board files for the same reason. > I am on the paranoid side but here I don't see much potential for conflicts: > a) this only works for the Up2 board with a HAT connector > b) this only work with the Hifiberry DAC+ PRO board. > This codec is not used in any traditional client devices. That's what you're doing right now but someone else can use the same devices, or adopt the same approaches on something like a Chromebook. > > My understanding is that ACPI just doesn't have clock bindings (or audio > > bindings or...) so you're basically using board files here and board > > files can definitely do more than we're seeing here. > I don't understand your definition of board file, sorry. We've never had > one, the only thing that's board-specific is the machine driver. Architectures that don't have firmware bindings use straight C code to register and set things up. Machine drivers are essentially board files, they're just audio specific bits of board file that use audio APIs and so are in the sound directory. > > You should be able to register links between devices using the clock > > API, or add that functionality if it's not there but AFAIK clkdev still > > works. > The machine driver has no information whatsoever on who provides the clock. > I just don't see how I might link stuff without at least some amount of > information? The machine driver must have this information, it knows exactly what hardware it runs on. The whole point of a machine driver is that it's board specific. > All I needed was to toggle 2 gpios to select 44.1 or 48kHz...Looks like it's > going to take two more years, oh well. I think you're giving up way too easily here. The kernel has really good support for systems that don't have any firmware description at all, this shouldn't be complex or breaking new ground.
On 4/15/20 4:52 AM, Andy Shevchenko wrote: > On Tue, Apr 14, 2020 at 12:54:25PM -0500, Pierre-Louis Bossart wrote: >> On 4/14/20 12:11 PM, Andy Shevchenko wrote: >>> On Thu, Apr 09, 2020 at 02:58:27PM -0500, Pierre-Louis Bossart wrote: >>>> Using devm_clk_get() with a NULL string fails on ACPI platforms, use >>>> the "sclk" string as a fallback. >>> >>> This is fishy a bit. >> >> I didn't find a single example where we use a NULL string in ACPI cases? > > ... > >>> If no, why not simple switch to devm_clk_get_optional()? >> >> Not sure what that would change? > > Hmm... Who is the provider of this clock? Well, at the hardware level, the clock is provided by two local oscillators controlled by the codec GPIOs. So you could consider that the codec is both the provider and consumer of the clock. In the Linux world, the PCM512x codec driver creates a gpiochip. And the clk driver uses the gpios to expose a clk used by the PCM512x codec driver. I am not fully happy with this design because it creates a double dependency which makes it impossible to remove modules. But I don't know how to model it otherwise. But to go back to your question, the two parts are really joined at the hip since the same gpios exposed by one is used by the other.
On 4/15/20 6:36 AM, Mark Brown wrote: > On Tue, Apr 14, 2020 at 03:13:01PM -0500, Pierre-Louis Bossart wrote: >> On 4/14/20 2:50 PM, Mark Brown wrote: > >>> It's not just DT platforms that I'm worried about here, it's also ACPI >>> systems - all it takes is for a system to have a second device and a >>> name collision could happen, especially with such generic names. We >>> tried to avoid doing this for board files for the same reason. > >> I am on the paranoid side but here I don't see much potential for conflicts: > >> a) this only works for the Up2 board with a HAT connector >> b) this only work with the Hifiberry DAC+ PRO board. > >> This codec is not used in any traditional client devices. > > That's what you're doing right now but someone else can use the same > devices, or adopt the same approaches on something like a Chromebook. > >>> My understanding is that ACPI just doesn't have clock bindings (or audio >>> bindings or...) so you're basically using board files here and board >>> files can definitely do more than we're seeing here. > >> I don't understand your definition of board file, sorry. We've never had >> one, the only thing that's board-specific is the machine driver. > > Architectures that don't have firmware bindings use straight C code to > register and set things up. Machine drivers are essentially board > files, they're just audio specific bits of board file that use audio > APIs and so are in the sound directory. Humm, we may have a conceptual disconnect here. In the ACPI world, there is no support for the machine driver - unlike Device Tree. It is probed when the SST/SOF driver creates a platform device using the codec _HID as a key to hard-coded lookup tables in sound/soc/intel/common/soc-acpi*.c - it will be probed *after* the codec driver probes. I really don't see how to use the machine driver as currently implemented to establish board-level connections that would influence the codec driver probe and its use of a clock. > >>> You should be able to register links between devices using the clock >>> API, or add that functionality if it's not there but AFAIK clkdev still >>> works. > >> The machine driver has no information whatsoever on who provides the clock. >> I just don't see how I might link stuff without at least some amount of >> information? > > The machine driver must have this information, it knows exactly what > hardware it runs on. The whole point of a machine driver is that it's > board specific. > >> All I needed was to toggle 2 gpios to select 44.1 or 48kHz...Looks like it's >> going to take two more years, oh well. > > I think you're giving up way too easily here. The kernel has really > good support for systems that don't have any firmware description at > all, this shouldn't be complex or breaking new ground. See above, I don't think the machine driver can do what you had in mind? I don't see how to proceed unless we remove all support for ACPI, both for codec and clock driver, and trigger their probe "manually" with a board-level initialization. And btw there's already a precedent for using global names, it's what the Skylake driver does for the mclk and ssp clocks. To the best of my knowledge the device specific namespacing does not exist on any ACPI platform. We have a request from Dialog to implement the same thing for SOF to solve dependencies on the clock being stable before turning on the codec, so if global names are not acceptable we have a real problem.
On Wed, Apr 15, 2020 at 09:19:10AM -0500, Pierre-Louis Bossart wrote: > On 4/15/20 4:52 AM, Andy Shevchenko wrote: > > On Tue, Apr 14, 2020 at 12:54:25PM -0500, Pierre-Louis Bossart wrote: > > > On 4/14/20 12:11 PM, Andy Shevchenko wrote: > > > > On Thu, Apr 09, 2020 at 02:58:27PM -0500, Pierre-Louis Bossart wrote: > > > > > Using devm_clk_get() with a NULL string fails on ACPI platforms, use > > > > > the "sclk" string as a fallback. > > > > > > > > This is fishy a bit. > > > > > > I didn't find a single example where we use a NULL string in ACPI cases? > > > > ... > > > > > > If no, why not simple switch to devm_clk_get_optional()? > > > > > > Not sure what that would change? > > > > Hmm... Who is the provider of this clock? > > Well, at the hardware level, the clock is provided by two local oscillators > controlled by the codec GPIOs. So you could consider that the codec is both > the provider and consumer of the clock. Is it internal component to the codec or discrete (outside of codec chip)? I bet it is the latter. Thus, codec is not provider. Board, where this configuration is used, *is* provider of the clock(s). > In the Linux world, the PCM512x codec driver creates a gpiochip. And the clk > driver uses the gpios to expose a clk used by the PCM512x codec driver. Yeah, hardware cyclic dependency :-) > I am not fully happy with this design because it creates a double dependency > which makes it impossible to remove modules. But I don't know how to model > it otherwise. I guess it should be clock provider which uses GPIO as a parameter to switch clock rates, and codec driver, which provides GPIO chip and instantiates (after) the clock provider instance. One module or several, is an implementation detail. > But to go back to your question, the two parts are really joined at the hip > since the same gpios exposed by one is used by the other. I got it, thanks for explanation.
On Wed, Apr 15, 2020 at 09:44:12AM -0500, Pierre-Louis Bossart wrote: > On 4/15/20 6:36 AM, Mark Brown wrote: > > Architectures that don't have firmware bindings use straight C code to > > register and set things up. Machine drivers are essentially board > > files, they're just audio specific bits of board file that use audio > > APIs and so are in the sound directory. > Humm, we may have a conceptual disconnect here. In the ACPI world, there is > no support for the machine driver - unlike Device Tree. It is probed when This is nothing to do with device tree except in that it has useful firmware descriptions of the hardware. > the SST/SOF driver creates a platform device using the codec _HID as a key > to hard-coded lookup tables in sound/soc/intel/common/soc-acpi*.c - it will > be probed *after* the codec driver probes. I really don't see how to use the > machine driver as currently implemented to establish board-level connections > that would influence the codec driver probe and its use of a clock. You have the opportunity to run whatever code you want to run at the point where you're registering your drivers with the system on module init, things like DMI quirk tables (which is what you're going to need to do here AFAICT) should work just as well there as they do later on when the driver loads. > > I think you're giving up way too easily here. The kernel has really > > good support for systems that don't have any firmware description at > > all, this shouldn't be complex or breaking new ground. > See above, I don't think the machine driver can do what you had in mind? > I don't see how to proceed unless we remove all support for ACPI, both for > codec and clock driver, and trigger their probe "manually" with a > board-level initialization. The clkdev stuff can use dev_name() so so long as the devices appear with predictable names you should be fine. If not IIRC everything in ACPI is named in the AML so clkdev could be extended to be able to find things based on the names it gives. > And btw there's already a precedent for using global names, it's what the > Skylake driver does for the mclk and ssp clocks. To the best of my knowledge > the device specific namespacing does not exist on any ACPI platform. We have No machine description at all exists on board file systems other than what we write in C and they manage to cope with this, I'm sure we can find a way to do it with ACPI. I mentioned clkdev before, that is something that's done entirely at the Linux level. > a request from Dialog to implement the same thing for SOF to solve > dependencies on the clock being stable before turning on the codec, so if > global names are not acceptable we have a real problem. If existing usages that have ended up getting merged are going to be used to push for additional adoption then that's not encouraging.
>> the SST/SOF driver creates a platform device using the codec _HID as a key >> to hard-coded lookup tables in sound/soc/intel/common/soc-acpi*.c - it will >> be probed *after* the codec driver probes. I really don't see how to use the >> machine driver as currently implemented to establish board-level connections >> that would influence the codec driver probe and its use of a clock. > > You have the opportunity to run whatever code you want to run at the > point where you're registering your drivers with the system on module > init, things like DMI quirk tables (which is what you're going to need > to do here AFAICT) should work just as well there as they do later on > when the driver loads. The idea here was to have one single build, and then rely on what the user configured with initrd override to probe the right I2C codec driver and indirectly the machine driver. It's similar to device tree overlays. With the same up2 board, I change the .aml file in /lib/firmware/acpi-upgrades, swap one HAT board for another and the new board is handled automagically. I don't see how I can use hard-coded DMI tables or board-specific things without losing the single build? >>> I think you're giving up way too easily here. The kernel has really >>> good support for systems that don't have any firmware description at >>> all, this shouldn't be complex or breaking new ground. > >> See above, I don't think the machine driver can do what you had in mind? > >> I don't see how to proceed unless we remove all support for ACPI, both for >> codec and clock driver, and trigger their probe "manually" with a >> board-level initialization. > > The clkdev stuff can use dev_name() so so long as the devices appear > with predictable names you should be fine. If not IIRC everything in > ACPI is named in the AML so clkdev could be extended to be able to find > things based on the names it gives. I had a discussion with Andy Shevchenko that we should precisely not be using dev_name() since we don't control the names that ACPI selects for us. And since I was using the generic PRP0001 thing for the clock device to probe using the 'compatible' string it's actually even less reliable and unique... >> And btw there's already a precedent for using global names, it's what the >> Skylake driver does for the mclk and ssp clocks. To the best of my knowledge >> the device specific namespacing does not exist on any ACPI platform. We have > > No machine description at all exists on board file systems other than > what we write in C and they manage to cope with this, I'm sure we can > find a way to do it with ACPI. I mentioned clkdev before, that is > something that's done entirely at the Linux level. > >> a request from Dialog to implement the same thing for SOF to solve >> dependencies on the clock being stable before turning on the codec, so if >> global names are not acceptable we have a real problem. > > If existing usages that have ended up getting merged are going to be > used to push for additional adoption then that's not encouraging. I wasn't trying to push this against your will, rather I wanted to highlight that we should be clear on the direction for all these uses of the clk API in an ACPI context. If what I suggested here is not the right path, then how do we deal with all the existing cases? This PCM512x use is not a mainstream usage, we use this board mainly for validation and for community support, the other cases with 'mclk' and 'sspX_fsync' are critical and impact devices shipping in large volumes.
On Wed, Apr 15, 2020 at 12:26:57PM -0500, Pierre-Louis Bossart wrote: > > You have the opportunity to run whatever code you want to run at the > > point where you're registering your drivers with the system on module > > init, things like DMI quirk tables (which is what you're going to need > > to do here AFAICT) should work just as well there as they do later on > > when the driver loads. > The idea here was to have one single build, and then rely on what the user > configured with initrd override to probe the right I2C codec driver and > indirectly the machine driver. It's similar to device tree overlays. > With the same up2 board, I change the .aml file in > /lib/firmware/acpi-upgrades, swap one HAT board for another and the new > board is handled automagically. > I don't see how I can use hard-coded DMI tables or board-specific things > without losing the single build? Ugh, so you change for another machine description and don't update any of the DMI information? Perhaps you can't... That doesn't seem very ACPI given how reliant it is on doing quirks based on DMI data for modern systems :/ > > The clkdev stuff can use dev_name() so so long as the devices appear > > with predictable names you should be fine. If not IIRC everything in > > ACPI is named in the AML so clkdev could be extended to be able to find > > things based on the names it gives. > I had a discussion with Andy Shevchenko that we should precisely not be > using dev_name() since we don't control the names that ACPI selects for us. It's not ideal and you should definitely use something better if it's available but if it's all you've got... You could also search for the device by binding string at runtime, that'd blow up if you've got more than one of the same device but it's a bit less likely. > And since I was using the generic PRP0001 thing for the clock device to > probe using the 'compatible' string it's actually even less reliable and > unique... I see, though actually that might be a place to set up links from now I think about it - if you know what the I2C device is going to be called you could have the platform device for the clock source register the links too. > > If existing usages that have ended up getting merged are going to be > > used to push for additional adoption then that's not encouraging. > I wasn't trying to push this against your will, rather I wanted to highlight > that we should be clear on the direction for all these uses of the clk API > in an ACPI context. If what I suggested here is not the right path, then how > do we deal with all the existing cases? This PCM512x use is not a mainstream > usage, we use this board mainly for validation and for community support, > the other cases with 'mclk' and 'sspX_fsync' are critical and impact devices > shipping in large volumes. The ideal thing would of course be to extend ACPI to encode things like this in the firmware description so that it is able to reflect the reality of modern systems, the graph bits of this were already specified so most of the work has been done. However it's a bit late for the shipping systems. Normal shipping systems are in a lot better position here in that they do have some hope of being able to patch up the links after the fact with DMI based quirks. It really would be good to see a way of doing that deployed, especially in cases where you might otherwise have to modify the CODEC driver. I *think* that should be doable, assuming there's some stable or runtime discoverable naming for the ACPI devices. In the case of this driver could you look at registering the link from the device for the clocks? Have it say "I supply SCK on device X" as it registers. That should be fairly straightforward I think, we do that for one of the regulators. The main thing I want to avoid is having to have the CODEC drivers know platform specific strings that they're supposed to look up, or general approaches where that ends up being a thing that looks idiomatic. That was something board files did for a while, it didn't work very well and we did something better with clkdev instead. I'm a lot less worried about this for cases where it's two devices that are part of the SoC talking to each other, that's relatively well controled and doesn't affect non-x86 platforms. When it starts touching the CODECs it's a lot more worrying. I think by now there's ample evidence that it's worth investing in better firmware descriptions :(
> In the case of this driver could you look at registering the link from > the device for the clocks? Have it say "I supply SCK on device X" as it > registers. That should be fairly straightforward I think, we do that > for one of the regulators. When you wrote 'in the case of this driver', were you referring to the clock provider, saying 'I support SCK on device i2c-104C5122:00' ? If you have a pointer on the regulator example, I'd appreciate it, I am really way beyond my comfort zone. > The main thing I want to avoid is having to have the CODEC drivers know > platform specific strings that they're supposed to look up, or general > approaches where that ends up being a thing that looks idiomatic. That > was something board files did for a while, it didn't work very well and > we did something better with clkdev instead. I'm a lot less worried > about this for cases where it's two devices that are part of the SoC > talking to each other, that's relatively well controled and doesn't > affect non-x86 platforms. When it starts touching the CODECs it's a lot > more worrying. I see the nuance, thanks for the clarification. Maybe an alternate suggestion if you want to avoid hard-coded strings in the kernel: what if we added optional properties for the clock lookup name in both the codec and clock driver, and set the name in a _DSD blob. That would move the platform-specific names to platform firmware, and avoid the links described above that are probably ACPI-only. In this case we can add whatever we want, the DSDT table contains absolutely nothing for audio so we can add things as needed, and in case another usage of this codec happens in a future device they'd have to define their own clock name and store it in platform firmware. > I think by now there's ample evidence that it's worth investing in > better firmware descriptions :( Indeed, and tools to check they are correct! Most of the stuff we defined for SoundWire ends-up wrong or undefined, still an uphill battle...
On Wed, Apr 15, 2020 at 03:22:50PM -0500, Pierre-Louis Bossart wrote: > > In the case of this driver could you look at registering the link from > > the device for the clocks? Have it say "I supply SCK on device X" as it > > registers. That should be fairly straightforward I think, we do that > > for one of the regulators. > When you wrote 'in the case of this driver', were you referring to the clock > provider, saying 'I support SCK on device i2c-104C5122:00' ? Yes. > If you have a pointer on the regulator example, I'd appreciate it, I am > really way beyond my comfort zone. The arizona driver is what I was thinking of, that's more complex than you proabably need as it's a MFD. > Maybe an alternate suggestion if you want to avoid hard-coded strings in the > kernel: what if we added optional properties for the clock lookup name in > both the codec and clock driver, and set the name in a _DSD blob. That would > move the platform-specific names to platform firmware, and avoid the links > described above that are probably ACPI-only. If you read the lookup information from firmware somehow that's probably fine I think. It's not nice but if it's baked into firmware... > > I think by now there's ample evidence that it's worth investing in > > better firmware descriptions :( > Indeed, and tools to check they are correct! Most of the stuff we defined > for SoundWire ends-up wrong or undefined, still an uphill battle... The audio-graph-card appears to be working really well FWIW, Morimoto-san did an excellent job there.
diff --git a/sound/soc/codecs/pcm512x.c b/sound/soc/codecs/pcm512x.c index 4f895a588c31..1df291b84925 100644 --- a/sound/soc/codecs/pcm512x.c +++ b/sound/soc/codecs/pcm512x.c @@ -1603,6 +1603,7 @@ static const struct gpio_chip template_chip = { int pcm512x_probe(struct device *dev, struct regmap *regmap) { + const char * const clk_name[] = {NULL, "sclk"}; struct pcm512x_priv *pcm512x; int i, ret; @@ -1671,17 +1672,28 @@ int pcm512x_probe(struct device *dev, struct regmap *regmap) goto err; } - pcm512x->sclk = devm_clk_get(dev, NULL); - if (PTR_ERR(pcm512x->sclk) == -EPROBE_DEFER) { - ret = -EPROBE_DEFER; - goto err; - } - if (!IS_ERR(pcm512x->sclk)) { - ret = clk_prepare_enable(pcm512x->sclk); - if (ret != 0) { - dev_err(dev, "Failed to enable SCLK: %d\n", ret); + for (i = 0; i < ARRAY_SIZE(clk_name); i++) { + pcm512x->sclk = devm_clk_get(dev, clk_name[i]); + if (PTR_ERR(pcm512x->sclk) == -EPROBE_DEFER) { + ret = -EPROBE_DEFER; goto err; } + if (!IS_ERR(pcm512x->sclk)) { + dev_dbg(dev, "SCLK detected by devm_clk_get\n"); + ret = clk_prepare_enable(pcm512x->sclk); + if (ret != 0) { + dev_err(dev, "Failed to enable SCLK: %d\n", + ret); + goto err; + } + break; + } + + if (!clk_name[i]) + dev_dbg(dev, "no SCLK detected with NULL string\n"); + else + dev_dbg(dev, "no SCLK detected for %s string\n", + clk_name[i]); } /* Default to standby mode */
Using devm_clk_get() with a NULL string fails on ACPI platforms, use the "sclk" string as a fallback. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- sound/soc/codecs/pcm512x.c | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-)