| Message ID | 20170710203801.17496-1-carlo@caione.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hi, 2017-07-10 22:38 GMT+02:00 Carlo Caione <carlo@caione.org>: > From: Carlo Caione <carlo@endlessm.com> > > Read the enable register to determine if the clock is already in use by > the firmware. In this case avoid gating the clock. > > Signed-off-by: Carlo Caione <carlo@endlessm.com> > --- > drivers/clk/x86/clk-pmc-atom.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/clk/x86/clk-pmc-atom.c b/drivers/clk/x86/clk-pmc-atom.c > index 2b60577703ef..46a8b6216fca 100644 > --- a/drivers/clk/x86/clk-pmc-atom.c > +++ b/drivers/clk/x86/clk-pmc-atom.c > @@ -185,6 +185,9 @@ static struct clk_plt *plt_clk_register(struct platform_device *pdev, int id, > pclk->reg = base + PMC_CLK_CTL_OFFSET + id * PMC_CLK_CTL_SIZE; > spin_lock_init(&pclk->lock); > > + if (plt_clk_is_enabled(&pclk->hw)) > + init.flags |= CLK_IGNORE_UNUSED; > + > ret = devm_clk_hw_register(&pdev->dev, &pclk->hw); > if (ret) { > pclk = ERR_PTR(ret); > -- > 2.13.2 > It also fixes the issue introduced in commit 282a4e4 ("platform/x86: Enable Atom PMC platform clocks") that causes no audio on Baytrail. Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> Thanks Carlo to send this. -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jul 11, 2017 at 12:31:28AM +0200, Enric Balletbo Serra wrote: > Hi, > > 2017-07-10 22:38 GMT+02:00 Carlo Caione <carlo@caione.org>: > > From: Carlo Caione <carlo@endlessm.com> > > > > Read the enable register to determine if the clock is already in use by > > the firmware. In this case avoid gating the clock. > > > > Signed-off-by: Carlo Caione <carlo@endlessm.com> > > --- > > drivers/clk/x86/clk-pmc-atom.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/clk/x86/clk-pmc-atom.c b/drivers/clk/x86/clk-pmc-atom.c > > index 2b60577703ef..46a8b6216fca 100644 > > --- a/drivers/clk/x86/clk-pmc-atom.c > > +++ b/drivers/clk/x86/clk-pmc-atom.c > > @@ -185,6 +185,9 @@ static struct clk_plt *plt_clk_register(struct platform_device *pdev, int id, > > pclk->reg = base + PMC_CLK_CTL_OFFSET + id * PMC_CLK_CTL_SIZE; > > spin_lock_init(&pclk->lock); > > > > + if (plt_clk_is_enabled(&pclk->hw)) > > + init.flags |= CLK_IGNORE_UNUSED; > > + > > ret = devm_clk_hw_register(&pdev->dev, &pclk->hw); > > if (ret) { > > pclk = ERR_PTR(ret); > > -- > > 2.13.2 > > > > It also fixes the issue introduced in commit 282a4e4 ("platform/x86: > Enable Atom PMC platform clocks") that causes no audio on Baytrail. > > Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> Excellent, thank you Enric. Pierre, any objections to this going in to 4.13 now and stable back to 4.12?
On 07/10, Darren Hart wrote: > On Tue, Jul 11, 2017 at 12:31:28AM +0200, Enric Balletbo Serra wrote: > > 2017-07-10 22:38 GMT+02:00 Carlo Caione <carlo@caione.org>: > > > pclk->reg = base + PMC_CLK_CTL_OFFSET + id * PMC_CLK_CTL_SIZE; > > > spin_lock_init(&pclk->lock); > > > > > > + if (plt_clk_is_enabled(&pclk->hw)) > > > + init.flags |= CLK_IGNORE_UNUSED; Can you add a comment to the effect of why we're adding ignore unused here? That will make it clearer when reading the code a year from now why we're not turning these clks off by default and also allow us to recall why it isn't marked as a critical clk. Probably, we want some sort of handoff mechanism here so that the clk is left on until a driver comes into the picture and acquires a handle to this clk? > > > + > > > ret = devm_clk_hw_register(&pdev->dev, &pclk->hw); > > > if (ret) { > > > pclk = ERR_PTR(ret); > > > -- > > > 2.13.2 > > > > > > > It also fixes the issue introduced in commit 282a4e4 ("platform/x86: > > Enable Atom PMC platform clocks") that causes no audio on Baytrail. > > > > Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > > Excellent, thank you Enric. Pierre, any objections to this going in to > 4.13 now and stable back to 4.12? > You mean v4.11? That's when commit 282a4e4 was released. Typically we punt these sorts of things to the next release because it isn't a regression in the release that's being worked on (i.e. it wasn't introduced in this merge window), but in this case it seems like a small enough patch plus it's a bother to keep it out of the release to make it alright to merge now.
On Mon, Jul 10, 2017 at 06:29:08PM -0700, Stephen Boyd wrote: > On 07/10, Darren Hart wrote: > > On Tue, Jul 11, 2017 at 12:31:28AM +0200, Enric Balletbo Serra wrote: > > > 2017-07-10 22:38 GMT+02:00 Carlo Caione <carlo@caione.org>: > > > > pclk->reg = base + PMC_CLK_CTL_OFFSET + id * PMC_CLK_CTL_SIZE; > > > > spin_lock_init(&pclk->lock); > > > > > > > > + if (plt_clk_is_enabled(&pclk->hw)) > > > > + init.flags |= CLK_IGNORE_UNUSED; > > Can you add a comment to the effect of why we're adding ignore > unused here? That will make it clearer when reading the code a > year from now why we're not turning these clks off by default and > also allow us to recall why it isn't marked as a critical clk. Yes please, good point. > > Probably, we want some sort of handoff mechanism here so that the > clk is left on until a driver comes into the picture and acquires > a handle to this clk? Presumably optionally - it will stay on permanently if nothing claims it, as in the case where the firmware sets up for the platform, and nothing else touches it. > > > > > + > > > > ret = devm_clk_hw_register(&pdev->dev, &pclk->hw); > > > > if (ret) { > > > > pclk = ERR_PTR(ret); > > > > -- > > > > 2.13.2 > > > > > > > > > > It also fixes the issue introduced in commit 282a4e4 ("platform/x86: > > > Enable Atom PMC platform clocks") that causes no audio on Baytrail. > > > > > > Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > > > > Excellent, thank you Enric. Pierre, any objections to this going in to > > 4.13 now and stable back to 4.12? > > > > You mean v4.11? That's when commit 282a4e4 was released. Yes indeed. > Typically we punt these sorts of things to the next release > because it isn't a regression in the release that's being worked > on (i.e. it wasn't introduced in this merge window), but in this > case it seems like a small enough patch plus it's a bother to > keep it out of the release to make it alright to merge now. > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project >
On Tue, Jul 11, 2017 at 3:29 AM, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 07/10, Darren Hart wrote: >> On Tue, Jul 11, 2017 at 12:31:28AM +0200, Enric Balletbo Serra wrote: >> > 2017-07-10 22:38 GMT+02:00 Carlo Caione <carlo@caione.org>: >> > > pclk->reg = base + PMC_CLK_CTL_OFFSET + id * PMC_CLK_CTL_SIZE; >> > > spin_lock_init(&pclk->lock); >> > > >> > > + if (plt_clk_is_enabled(&pclk->hw)) >> > > + init.flags |= CLK_IGNORE_UNUSED; > > Can you add a comment to the effect of why we're adding ignore > unused here? That will make it clearer when reading the code a > year from now why we're not turning these clks off by default and > also allow us to recall why it isn't marked as a critical clk. This is actually a good point. Probably here we want to mark the clock as CLK_IS_CRITICAL instead of CLK_IGNORE_UNUSED. Michael, any preference here?
On 7/11/17 1:54 AM, Darren Hart wrote: > On Tue, Jul 11, 2017 at 12:31:28AM +0200, Enric Balletbo Serra wrote: >> Hi, >> >> 2017-07-10 22:38 GMT+02:00 Carlo Caione <carlo@caione.org>: >>> From: Carlo Caione <carlo@endlessm.com> >>> >>> Read the enable register to determine if the clock is already in use by >>> the firmware. In this case avoid gating the clock. >>> >>> Signed-off-by: Carlo Caione <carlo@endlessm.com> >>> --- >>> drivers/clk/x86/clk-pmc-atom.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/drivers/clk/x86/clk-pmc-atom.c b/drivers/clk/x86/clk-pmc-atom.c >>> index 2b60577703ef..46a8b6216fca 100644 >>> --- a/drivers/clk/x86/clk-pmc-atom.c >>> +++ b/drivers/clk/x86/clk-pmc-atom.c >>> @@ -185,6 +185,9 @@ static struct clk_plt *plt_clk_register(struct platform_device *pdev, int id, >>> pclk->reg = base + PMC_CLK_CTL_OFFSET + id * PMC_CLK_CTL_SIZE; >>> spin_lock_init(&pclk->lock); >>> >>> + if (plt_clk_is_enabled(&pclk->hw)) >>> + init.flags |= CLK_IGNORE_UNUSED; >>> + >>> ret = devm_clk_hw_register(&pdev->dev, &pclk->hw); >>> if (ret) { >>> pclk = ERR_PTR(ret); >>> -- >>> 2.13.2 >>> >> >> It also fixes the issue introduced in commit 282a4e4 ("platform/x86: >> Enable Atom PMC platform clocks") that causes no audio on Baytrail. >> >> Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > > Excellent, thank you Enric. Pierre, any objections to this going in to > 4.13 now and stable back to 4.12? I saw an update of this patch being merged while I was away on an extended summer break, but out of curiosity what was the problem with no audio on Baytrail? the code was added precisely to add support for 19.2 MHz on Baytrail and finally solve audio issues. I have no reports of broken functionality with this 282a4e4 commit... Enric, do you have a bugzilla or pointers? -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2017-07-24 at 15:49 +0200, Pierre-Louis Bossart wrote: > On 7/11/17 1:54 AM, Darren Hart wrote: > > On Tue, Jul 11, 2017 at 12:31:28AM +0200, Enric Balletbo Serra > > wrote: > > > Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > > > > Excellent, thank you Enric. Pierre, any objections to this going in > > to > > 4.13 now and stable back to 4.12? > > I saw an update of this patch being merged while I was away on an > extended summer break, but out of curiosity what was the problem with > no > audio on Baytrail? the code was added precisely to add support for > 19.2 > MHz on Baytrail and finally solve audio issues. I have no reports of > broken functionality with this 282a4e4 commit... Enric, do you have a > bugzilla or pointers? TL;DR: We should respect firmware settings before blindly shut down "unused" resources. (We used to have similar stuff in GPIO/pinctrl driver for direct IRQ pins, where we shut up the pins which are used directly by IOAPIC)
On 7/24/17 4:02 PM, Andy Shevchenko wrote: > On Mon, 2017-07-24 at 15:49 +0200, Pierre-Louis Bossart wrote: >> On 7/11/17 1:54 AM, Darren Hart wrote: >>> On Tue, Jul 11, 2017 at 12:31:28AM +0200, Enric Balletbo Serra >>> wrote: > >>>> Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> >>> >>> Excellent, thank you Enric. Pierre, any objections to this going in >>> to >>> 4.13 now and stable back to 4.12? >> >> I saw an update of this patch being merged while I was away on an >> extended summer break, but out of curiosity what was the problem with >> no >> audio on Baytrail? the code was added precisely to add support for >> 19.2 >> MHz on Baytrail and finally solve audio issues. I have no reports of >> broken functionality with this 282a4e4 commit... Enric, do you have a >> bugzilla or pointers? > > TL;DR: We should respect firmware settings before blindly shut down > "unused" resources. (We used to have similar stuff in GPIO/pinctrl > driver for direct IRQ pins, where we shut up the pins which are used > directly by IOAPIC) I wasn't arguing against those patches, just curious as to which platforms were broken by setting pmc_plat_3 blindly in the driver. -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2017-07-24 16:07 GMT+02:00 Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>: > On 7/24/17 4:02 PM, Andy Shevchenko wrote: >> >> On Mon, 2017-07-24 at 15:49 +0200, Pierre-Louis Bossart wrote: >>> >>> On 7/11/17 1:54 AM, Darren Hart wrote: >>>> >>>> On Tue, Jul 11, 2017 at 12:31:28AM +0200, Enric Balletbo Serra >>>> wrote: >> >> >>>>> Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> >>>> >>>> >>>> Excellent, thank you Enric. Pierre, any objections to this going in >>>> to >>>> 4.13 now and stable back to 4.12? >>> >>> >>> I saw an update of this patch being merged while I was away on an >>> extended summer break, but out of curiosity what was the problem with >>> no >>> audio on Baytrail? the code was added precisely to add support for >>> 19.2 >>> MHz on Baytrail and finally solve audio issues. I have no reports of >>> broken functionality with this 282a4e4 commit... Enric, do you have a >>> bugzilla or pointers? >> >> >> TL;DR: We should respect firmware settings before blindly shut down >> "unused" resources. (We used to have similar stuff in GPIO/pinctrl >> driver for direct IRQ pins, where we shut up the pins which are used >> directly by IOAPIC) > > > I wasn't arguing against those patches, just curious as to which platforms > were broken by setting pmc_plat_3 blindly in the driver. > After your patch the clock framework disables the required clocks because the driver does not request then and audio stopped to work.This is not the solution as is the codec who *must* request these clocks. We should implement this in the codec driver.. But now, at least, if firmware enables the clocks audio continues working on Baytrail. Regards, Enric -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7/24/17 5:02 PM, Enric Balletbo Serra wrote: > 2017-07-24 16:07 GMT+02:00 Pierre-Louis Bossart > <pierre-louis.bossart@linux.intel.com>: >> On 7/24/17 4:02 PM, Andy Shevchenko wrote: >>> >>> On Mon, 2017-07-24 at 15:49 +0200, Pierre-Louis Bossart wrote: >>>> >>>> On 7/11/17 1:54 AM, Darren Hart wrote: >>>>> >>>>> On Tue, Jul 11, 2017 at 12:31:28AM +0200, Enric Balletbo Serra >>>>> wrote: >>> >>> >>>>>> Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> >>>>> >>>>> >>>>> Excellent, thank you Enric. Pierre, any objections to this going in >>>>> to >>>>> 4.13 now and stable back to 4.12? >>>> >>>> >>>> I saw an update of this patch being merged while I was away on an >>>> extended summer break, but out of curiosity what was the problem with >>>> no >>>> audio on Baytrail? the code was added precisely to add support for >>>> 19.2 >>>> MHz on Baytrail and finally solve audio issues. I have no reports of >>>> broken functionality with this 282a4e4 commit... Enric, do you have a >>>> bugzilla or pointers? >>> >>> >>> TL;DR: We should respect firmware settings before blindly shut down >>> "unused" resources. (We used to have similar stuff in GPIO/pinctrl >>> driver for direct IRQ pins, where we shut up the pins which are used >>> directly by IOAPIC) >> >> >> I wasn't arguing against those patches, just curious as to which platforms >> were broken by setting pmc_plat_3 blindly in the driver. >> > > After your patch the clock framework disables the required clocks > because the driver does not request then and audio stopped to > work.This is not the solution as is the codec who *must* request these > clocks. We should implement this in the codec driver.. But now, at > least, if firmware enables the clocks audio continues working on > Baytrail. Clocks are not enabled/managed in any of the BIOSes I've seen for Baytrail, are you talking about a Chromebook by any chance with the legacy non-DPCM byt-max98080 driver? -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, 2017-07-24 18:26 GMT+02:00 Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>: > On 7/24/17 5:02 PM, Enric Balletbo Serra wrote: >> >> 2017-07-24 16:07 GMT+02:00 Pierre-Louis Bossart >> <pierre-louis.bossart@linux.intel.com>: >>> >>> On 7/24/17 4:02 PM, Andy Shevchenko wrote: >>>> >>>> >>>> On Mon, 2017-07-24 at 15:49 +0200, Pierre-Louis Bossart wrote: >>>>> >>>>> >>>>> On 7/11/17 1:54 AM, Darren Hart wrote: >>>>>> >>>>>> >>>>>> On Tue, Jul 11, 2017 at 12:31:28AM +0200, Enric Balletbo Serra >>>>>> wrote: >>>> >>>> >>>> >>>>>>> Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> >>>>>> >>>>>> >>>>>> >>>>>> Excellent, thank you Enric. Pierre, any objections to this going in >>>>>> to >>>>>> 4.13 now and stable back to 4.12? >>>>> >>>>> >>>>> >>>>> I saw an update of this patch being merged while I was away on an >>>>> extended summer break, but out of curiosity what was the problem with >>>>> no >>>>> audio on Baytrail? the code was added precisely to add support for >>>>> 19.2 >>>>> MHz on Baytrail and finally solve audio issues. I have no reports of >>>>> broken functionality with this 282a4e4 commit... Enric, do you have a >>>>> bugzilla or pointers? >>>> >>>> >>>> >>>> TL;DR: We should respect firmware settings before blindly shut down >>>> "unused" resources. (We used to have similar stuff in GPIO/pinctrl >>>> driver for direct IRQ pins, where we shut up the pins which are used >>>> directly by IOAPIC) >>> >>> >>> >>> I wasn't arguing against those patches, just curious as to which >>> platforms >>> were broken by setting pmc_plat_3 blindly in the driver. >>> >> >> After your patch the clock framework disables the required clocks >> because the driver does not request then and audio stopped to >> work.This is not the solution as is the codec who *must* request these >> clocks. We should implement this in the codec driver.. But now, at >> least, if firmware enables the clocks audio continues working on >> Baytrail. > > > Clocks are not enabled/managed in any of the BIOSes I've seen for Baytrail, > are you talking about a Chromebook by any chance with the legacy non-DPCM > byt-max98080 driver? > Yes the issue is with a Chromebook with the legacy byt-max98080 driver. -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 7/24/17 6:33 PM, Enric Balletbo Serra wrote: > Hi, > > 2017-07-24 18:26 GMT+02:00 Pierre-Louis Bossart > <pierre-louis.bossart@linux.intel.com>: >> On 7/24/17 5:02 PM, Enric Balletbo Serra wrote: >>> >>> 2017-07-24 16:07 GMT+02:00 Pierre-Louis Bossart >>> <pierre-louis.bossart@linux.intel.com>: >>>> >>>> On 7/24/17 4:02 PM, Andy Shevchenko wrote: >>>>> >>>>> >>>>> On Mon, 2017-07-24 at 15:49 +0200, Pierre-Louis Bossart wrote: >>>>>> >>>>>> >>>>>> On 7/11/17 1:54 AM, Darren Hart wrote: >>>>>>> >>>>>>> >>>>>>> On Tue, Jul 11, 2017 at 12:31:28AM +0200, Enric Balletbo Serra >>>>>>> wrote: >>>>> >>>>> >>>>> >>>>>>>> Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> >>>>>>> >>>>>>> >>>>>>> >>>>>>> Excellent, thank you Enric. Pierre, any objections to this going in >>>>>>> to >>>>>>> 4.13 now and stable back to 4.12? >>>>>> >>>>>> >>>>>> >>>>>> I saw an update of this patch being merged while I was away on an >>>>>> extended summer break, but out of curiosity what was the problem with >>>>>> no >>>>>> audio on Baytrail? the code was added precisely to add support for >>>>>> 19.2 >>>>>> MHz on Baytrail and finally solve audio issues. I have no reports of >>>>>> broken functionality with this 282a4e4 commit... Enric, do you have a >>>>>> bugzilla or pointers? >>>>> >>>>> >>>>> >>>>> TL;DR: We should respect firmware settings before blindly shut down >>>>> "unused" resources. (We used to have similar stuff in GPIO/pinctrl >>>>> driver for direct IRQ pins, where we shut up the pins which are used >>>>> directly by IOAPIC) >>>> >>>> >>>> >>>> I wasn't arguing against those patches, just curious as to which >>>> platforms >>>> were broken by setting pmc_plat_3 blindly in the driver. >>>> >>> >>> After your patch the clock framework disables the required clocks >>> because the driver does not request then and audio stopped to >>> work.This is not the solution as is the codec who *must* request these >>> clocks. We should implement this in the codec driver.. But now, at >>> least, if firmware enables the clocks audio continues working on >>> Baytrail. >> >> >> Clocks are not enabled/managed in any of the BIOSes I've seen for Baytrail, >> are you talking about a Chromebook by any chance with the legacy non-DPCM >> byt-max98080 driver? >> > > Yes the issue is with a Chromebook with the legacy byt-max98080 driver. ah, yes I never tested on those (couldn't make the device boot). We may be able to do more simplifications, in machine drivers I test for Baytrail vs. Cherrytrail to manage the clocks at the kernel level, but maybe I should just look at what the firmware does and use the kernel clock management as a fallback. -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/clk/x86/clk-pmc-atom.c b/drivers/clk/x86/clk-pmc-atom.c index 2b60577703ef..46a8b6216fca 100644 --- a/drivers/clk/x86/clk-pmc-atom.c +++ b/drivers/clk/x86/clk-pmc-atom.c @@ -185,6 +185,9 @@ static struct clk_plt *plt_clk_register(struct platform_device *pdev, int id, pclk->reg = base + PMC_CLK_CTL_OFFSET + id * PMC_CLK_CTL_SIZE; spin_lock_init(&pclk->lock); + if (plt_clk_is_enabled(&pclk->hw)) + init.flags |= CLK_IGNORE_UNUSED; + ret = devm_clk_hw_register(&pdev->dev, &pclk->hw); if (ret) { pclk = ERR_PTR(ret);