| Message ID | 20190404141325.21952-1-dave.mueller@gmx.ch (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v3] clk: x86: Add system specific quirk to mark clocks as critical | expand |
Hi, On 04-04-19 16:13, David Müller wrote: > Since commit 648e921888ad ("clk: x86: Stop marking clocks as > CLK_IS_CRITICAL"), the pmc_plt_clocks of the Bay Trail SoC are > unconditionally gated off. Unfortunately this will break systems where these > clocks are used for external purposes beyond the kernel's knowledge. Fix it > by implementing a system specific quirk to mark the necessary pmc_plt_clks > as critical. > > Fixes: 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL") > Signed-off-by: David Müller <dave.mueller@gmx.ch> > --- > Changes in v3: > - add missing sentinel entry to critclk_systems table > Changes in v2: > - restore previous clk detection logic as suggested by Hans de Goede Thank you, patch looks good to me now: Signed-off-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > > drivers/clk/x86/clk-pmc-atom.c | 14 ++++++++++--- > drivers/platform/x86/pmc_atom.c | 21 +++++++++++++++++++ > .../linux/platform_data/x86/clk-pmc-atom.h | 7 +++++-- > 3 files changed, 37 insertions(+), 5 deletions(-) > > diff --git a/drivers/clk/x86/clk-pmc-atom.c b/drivers/clk/x86/clk-pmc-atom.c > index d977193842df..e909720cc2e3 100644 > --- a/drivers/clk/x86/clk-pmc-atom.c > +++ b/drivers/clk/x86/clk-pmc-atom.c > @@ -165,7 +165,7 @@ static const struct clk_ops plt_clk_ops = { > }; > > static struct clk_plt *plt_clk_register(struct platform_device *pdev, int id, > - void __iomem *base, > + const struct pmc_clk_data *pmc_data, > const char **parent_names, > int num_parents) > { > @@ -184,9 +184,17 @@ static struct clk_plt *plt_clk_register(struct platform_device *pdev, int id, > init.num_parents = num_parents; > > pclk->hw.init = &init; > - pclk->reg = base + PMC_CLK_CTL_OFFSET + id * PMC_CLK_CTL_SIZE; > + pclk->reg = pmc_data->base + PMC_CLK_CTL_OFFSET + id * PMC_CLK_CTL_SIZE; > spin_lock_init(&pclk->lock); > > + /* > + * On some systems, the pmc_plt_clocks already enabled by the > + * firmware are being marked as critical to avoid them being > + * gated by the clock framework > + */ > + if (pmc_data->chk_critclks && plt_clk_is_enabled(&pclk->hw)) > + init.flags |= CLK_IS_CRITICAL; > + > ret = devm_clk_hw_register(&pdev->dev, &pclk->hw); > if (ret) { > pclk = ERR_PTR(ret); > @@ -332,7 +340,7 @@ static int plt_clk_probe(struct platform_device *pdev) > return PTR_ERR(parent_names); > > for (i = 0; i < PMC_CLK_NUM; i++) { > - data->clks[i] = plt_clk_register(pdev, i, pmc_data->base, > + data->clks[i] = plt_clk_register(pdev, i, pmc_data, > parent_names, data->nparents); > if (IS_ERR(data->clks[i])) { > err = PTR_ERR(data->clks[i]); > diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c > index 8f018b3f3cd4..d8dcf022909e 100644 > --- a/drivers/platform/x86/pmc_atom.c > +++ b/drivers/platform/x86/pmc_atom.c > @@ -17,6 +17,7 @@ > > #include <linux/debugfs.h> > #include <linux/device.h> > +#include <linux/dmi.h> > #include <linux/init.h> > #include <linux/io.h> > #include <linux/platform_data/x86/clk-pmc-atom.h> > @@ -391,11 +392,27 @@ static int pmc_dbgfs_register(struct pmc_dev *pmc) > } > #endif /* CONFIG_DEBUG_FS */ > > +/* > + * Some systems need one or more of their pmc_plt_clks to be > + * marked as critical > + */ > +static const struct dmi_system_id critclk_systems[] __initconst = { > + { > + .ident = "MPL CEC1x", > + .matches = { > + DMI_MATCH(DMI_SYS_VENDOR, "MPL AG"), > + DMI_MATCH(DMI_PRODUCT_NAME, "CEC10 Family"), > + }, > + }, > + { /*sentinel*/ } > +}; > + > static int pmc_setup_clks(struct pci_dev *pdev, void __iomem *pmc_regmap, > const struct pmc_data *pmc_data) > { > struct platform_device *clkdev; > struct pmc_clk_data *clk_data; > + const struct dmi_system_id *d = dmi_first_match(critclk_systems); > > clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL); > if (!clk_data) > @@ -403,6 +420,10 @@ static int pmc_setup_clks(struct pci_dev *pdev, void __iomem *pmc_regmap, > > clk_data->base = pmc_regmap; /* offset is added by client */ > clk_data->clks = pmc_data->clks; > + if (d) { > + clk_data->chk_critclks = true; > + pr_info("%s critclks quirk enabled\n", d->ident); > + } > > clkdev = platform_device_register_data(&pdev->dev, "clk-pmc-atom", > PLATFORM_DEVID_NONE, > diff --git a/include/linux/platform_data/x86/clk-pmc-atom.h b/include/linux/platform_data/x86/clk-pmc-atom.h > index 3ab892208343..497575a42353 100644 > --- a/include/linux/platform_data/x86/clk-pmc-atom.h > +++ b/include/linux/platform_data/x86/clk-pmc-atom.h > @@ -33,12 +33,15 @@ struct pmc_clk { > /** > * struct pmc_clk_data - common PMC clock configuration > * > - * @base: PMC clock register base offset > - * @clks: pointer to set of registered clocks, typically 0..5 > + * @base: PMC clock register base offset > + * @clks: pointer to set of registered clocks, typically 0..5 > + * @chk_critclks: flag to indicate if firmware enabled pmc_plt_clks > + * should be marked as critial or not > */ > struct pmc_clk_data { > void __iomem *base; > const struct pmc_clk *clks; > + bool chk_critclks; > }; > > #endif /* __PLATFORM_DATA_X86_CLK_PMC_ATOM_H */ > -- > 2.20.1 >
On Thu, Apr 4, 2019 at 5:14 PM David Müller <dave.mueller@gmx.ch> wrote: > > Since commit 648e921888ad ("clk: x86: Stop marking clocks as > CLK_IS_CRITICAL"), the pmc_plt_clocks of the Bay Trail SoC are > unconditionally gated off. Unfortunately this will break systems where these > clocks are used for external purposes beyond the kernel's knowledge. Fix it > by implementing a system specific quirk to mark the necessary pmc_plt_clks > as critical. > > Fixes: 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL") > Signed-off-by: David Müller <dave.mueller@gmx.ch> I'm fine with the patch, though few minor comments below. > + /* > + * On some systems, the pmc_plt_clocks already enabled by the > + * firmware are being marked as critical to avoid them being > + * gated by the clock framework Period would be nice to have at the end. > + */ > + if (d) { > + clk_data->chk_critclks = true; > + pr_info("%s critclks quirk enabled\n", d->ident); > + } I would go with callback, but for now this is okay as well. > - * @base: PMC clock register base offset > - * @clks: pointer to set of registered clocks, typically 0..5 > + * @base: PMC clock register base offset > + * @clks: pointer to set of registered clocks, typically 0..5 > + * @chk_critclks: flag to indicate if firmware enabled pmc_plt_clks > + * should be marked as critial or not Perhaps simple critical? And do not touch the rest here. > */
Hello Thanks for the review. Andy Shevchenko wrote: >> + /* >> + * On some systems, the pmc_plt_clocks already enabled by the >> + * firmware are being marked as critical to avoid them being >> + * gated by the clock framework > > Period would be nice to have at the end. Ok. >> - * @base: PMC clock register base offset >> - * @clks: pointer to set of registered clocks, typically 0..5 >> + * @base: PMC clock register base offset >> + * @clks: pointer to set of registered clocks, typically 0..5 > >> + * @chk_critclks: flag to indicate if firmware enabled pmc_plt_clks >> + * should be marked as critial or not > > Perhaps simple critical? And do not touch the rest here. Ok, I will rename the field. Dave
diff --git a/drivers/clk/x86/clk-pmc-atom.c b/drivers/clk/x86/clk-pmc-atom.c index d977193842df..e909720cc2e3 100644 --- a/drivers/clk/x86/clk-pmc-atom.c +++ b/drivers/clk/x86/clk-pmc-atom.c @@ -165,7 +165,7 @@ static const struct clk_ops plt_clk_ops = { }; static struct clk_plt *plt_clk_register(struct platform_device *pdev, int id, - void __iomem *base, + const struct pmc_clk_data *pmc_data, const char **parent_names, int num_parents) { @@ -184,9 +184,17 @@ static struct clk_plt *plt_clk_register(struct platform_device *pdev, int id, init.num_parents = num_parents; pclk->hw.init = &init; - pclk->reg = base + PMC_CLK_CTL_OFFSET + id * PMC_CLK_CTL_SIZE; + pclk->reg = pmc_data->base + PMC_CLK_CTL_OFFSET + id * PMC_CLK_CTL_SIZE; spin_lock_init(&pclk->lock); + /* + * On some systems, the pmc_plt_clocks already enabled by the + * firmware are being marked as critical to avoid them being + * gated by the clock framework + */ + if (pmc_data->chk_critclks && plt_clk_is_enabled(&pclk->hw)) + init.flags |= CLK_IS_CRITICAL; + ret = devm_clk_hw_register(&pdev->dev, &pclk->hw); if (ret) { pclk = ERR_PTR(ret); @@ -332,7 +340,7 @@ static int plt_clk_probe(struct platform_device *pdev) return PTR_ERR(parent_names); for (i = 0; i < PMC_CLK_NUM; i++) { - data->clks[i] = plt_clk_register(pdev, i, pmc_data->base, + data->clks[i] = plt_clk_register(pdev, i, pmc_data, parent_names, data->nparents); if (IS_ERR(data->clks[i])) { err = PTR_ERR(data->clks[i]); diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c index 8f018b3f3cd4..d8dcf022909e 100644 --- a/drivers/platform/x86/pmc_atom.c +++ b/drivers/platform/x86/pmc_atom.c @@ -17,6 +17,7 @@ #include <linux/debugfs.h> #include <linux/device.h> +#include <linux/dmi.h> #include <linux/init.h> #include <linux/io.h> #include <linux/platform_data/x86/clk-pmc-atom.h> @@ -391,11 +392,27 @@ static int pmc_dbgfs_register(struct pmc_dev *pmc) } #endif /* CONFIG_DEBUG_FS */ +/* + * Some systems need one or more of their pmc_plt_clks to be + * marked as critical + */ +static const struct dmi_system_id critclk_systems[] __initconst = { + { + .ident = "MPL CEC1x", + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "MPL AG"), + DMI_MATCH(DMI_PRODUCT_NAME, "CEC10 Family"), + }, + }, + { /*sentinel*/ } +}; + static int pmc_setup_clks(struct pci_dev *pdev, void __iomem *pmc_regmap, const struct pmc_data *pmc_data) { struct platform_device *clkdev; struct pmc_clk_data *clk_data; + const struct dmi_system_id *d = dmi_first_match(critclk_systems); clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL); if (!clk_data) @@ -403,6 +420,10 @@ static int pmc_setup_clks(struct pci_dev *pdev, void __iomem *pmc_regmap, clk_data->base = pmc_regmap; /* offset is added by client */ clk_data->clks = pmc_data->clks; + if (d) { + clk_data->chk_critclks = true; + pr_info("%s critclks quirk enabled\n", d->ident); + } clkdev = platform_device_register_data(&pdev->dev, "clk-pmc-atom", PLATFORM_DEVID_NONE, diff --git a/include/linux/platform_data/x86/clk-pmc-atom.h b/include/linux/platform_data/x86/clk-pmc-atom.h index 3ab892208343..497575a42353 100644 --- a/include/linux/platform_data/x86/clk-pmc-atom.h +++ b/include/linux/platform_data/x86/clk-pmc-atom.h @@ -33,12 +33,15 @@ struct pmc_clk { /** * struct pmc_clk_data - common PMC clock configuration * - * @base: PMC clock register base offset - * @clks: pointer to set of registered clocks, typically 0..5 + * @base: PMC clock register base offset + * @clks: pointer to set of registered clocks, typically 0..5 + * @chk_critclks: flag to indicate if firmware enabled pmc_plt_clks + * should be marked as critial or not */ struct pmc_clk_data { void __iomem *base; const struct pmc_clk *clks; + bool chk_critclks; }; #endif /* __PLATFORM_DATA_X86_CLK_PMC_ATOM_H */
Since commit 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL"), the pmc_plt_clocks of the Bay Trail SoC are unconditionally gated off. Unfortunately this will break systems where these clocks are used for external purposes beyond the kernel's knowledge. Fix it by implementing a system specific quirk to mark the necessary pmc_plt_clks as critical. Fixes: 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL") Signed-off-by: David Müller <dave.mueller@gmx.ch> --- Changes in v3: - add missing sentinel entry to critclk_systems table Changes in v2: - restore previous clk detection logic as suggested by Hans de Goede drivers/clk/x86/clk-pmc-atom.c | 14 ++++++++++--- drivers/platform/x86/pmc_atom.c | 21 +++++++++++++++++++ .../linux/platform_data/x86/clk-pmc-atom.h | 7 +++++-- 3 files changed, 37 insertions(+), 5 deletions(-) -- 2.20.1