[PATCH] clk: x86: Add system specific quirk to mark clocks as critical

[PATCH] clk: x86: Add system specific quirk to mark clocks as critical

[David Müller]

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>
---
 drivers/clk/x86/clk-pmc-atom.c                |  9 +++++---
 drivers/platform/x86/pmc_atom.c               | 22 +++++++++++++++++++
 .../linux/platform_data/x86/clk-pmc-atom.h    |  2 ++
 3 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/x86/clk-pmc-atom.c b/drivers/clk/x86/clk-pmc-atom.c
index d977193842df..882ea0c4c050 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)
 {
@@ -183,8 +183,11 @@ static struct clk_plt *plt_clk_register(struct platform_device *pdev, int id,
 	init.parent_names = parent_names;
 	init.num_parents = num_parents;

+	if (test_bit(id, &pmc_data->critclk))
+		init.flags |= CLK_IS_CRITICAL;
+
 	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);

 	ret = devm_clk_hw_register(&pdev->dev, &pclk->hw);
@@ -332,7 +335,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..d25b2423afe0 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,28 @@ 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"),
+		},
+		/* on this platform, pmc_plt_clk_0 is a critical clock */
+		.driver_data = (void *)BIT(0),
+	},
+};
+
 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 +421,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->critclk = (uintptr_t)d->driver_data;
+		pr_info("%s critclk 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..8d00b6bd83b4 100644
--- a/include/linux/platform_data/x86/clk-pmc-atom.h
+++ b/include/linux/platform_data/x86/clk-pmc-atom.h
@@ -35,10 +35,12 @@ struct pmc_clk {
  *
  * @base:	PMC clock register base offset
  * @clks:	pointer to set of registered clocks, typically 0..5
+ * @clkcrit:	bit mask of pmc_plt_clks which are to be marked as critical
  */
 struct pmc_clk_data {
 	void __iomem *base;
 	const struct pmc_clk *clks;
+	unsigned long critclk;
 };

 #endif /* __PLATFORM_DATA_X86_CLK_PMC_ATOM_H */
--
2.20.1

Re: [PATCH] clk: x86: Add system specific quirk to mark clocks as critical

[Andy Shevchenko]

On Thu, Mar 28, 2019 at 2:50 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.
>

I didn't see a reply to this:
https://marc.info/?l=linux-clk&m=155371441300575&w=2

I think we can avoid this hack to be returned back.

> Fixes: 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL")

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] clk: x86: Add system specific quirk to mark clocks as critical

[Hans de Goede]

Hi David,

On 28-03-19 13:49, 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.

The original CLK_IS_CRITICAL patch marked all clocks as critical which
where enabled by the BIOS at boot time. I would prefer that approach
over the code the mask in the quirk method you are now using.

The reason for this is that for example on the embedded pc using 4
igb ethernet controllers we only know that marking the clocks enabled
at boot as CLK_IS_CRITICAL fixes things, we do not know without extra
debugging which clocks are involved.

We can spend some time on figuring this out, but this means that each
time we get a similar bug-report, where we decide a dmi quirk is the
best / only solution, we need to do this. I would prefer for the code
to figure it out itself as it did before.

Can you respin the patch to use the old method of looking at which
clocks are enabled by the BIOS? (note you may want to wait with
this till Andy and I have reached an agreement on how to solve this)

Regards,

Hans




> 
> Fixes: 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL")
> Signed-off-by: David Müller <dave.mueller@gmx.ch>
> ---
>   drivers/clk/x86/clk-pmc-atom.c                |  9 +++++---
>   drivers/platform/x86/pmc_atom.c               | 22 +++++++++++++++++++
>   .../linux/platform_data/x86/clk-pmc-atom.h    |  2 ++
>   3 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/x86/clk-pmc-atom.c b/drivers/clk/x86/clk-pmc-atom.c
> index d977193842df..882ea0c4c050 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)
>   {
> @@ -183,8 +183,11 @@ static struct clk_plt *plt_clk_register(struct platform_device *pdev, int id,
>   	init.parent_names = parent_names;
>   	init.num_parents = num_parents;
> 
> +	if (test_bit(id, &pmc_data->critclk))
> +		init.flags |= CLK_IS_CRITICAL;
> +
>   	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);
> 
>   	ret = devm_clk_hw_register(&pdev->dev, &pclk->hw);
> @@ -332,7 +335,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..d25b2423afe0 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,28 @@ 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"),
> +		},
> +		/* on this platform, pmc_plt_clk_0 is a critical clock */
> +		.driver_data = (void *)BIT(0),
> +	},
> +};
> +
>   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 +421,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->critclk = (uintptr_t)d->driver_data;
> +		pr_info("%s critclk 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..8d00b6bd83b4 100644
> --- a/include/linux/platform_data/x86/clk-pmc-atom.h
> +++ b/include/linux/platform_data/x86/clk-pmc-atom.h
> @@ -35,10 +35,12 @@ struct pmc_clk {
>    *
>    * @base:	PMC clock register base offset
>    * @clks:	pointer to set of registered clocks, typically 0..5
> + * @clkcrit:	bit mask of pmc_plt_clks which are to be marked as critical
>    */
>   struct pmc_clk_data {
>   	void __iomem *base;
>   	const struct pmc_clk *clks;
> +	unsigned long critclk;
>   };
> 
>   #endif /* __PLATFORM_DATA_X86_CLK_PMC_ATOM_H */
> --
> 2.20.1
> 

Re: [PATCH] clk: x86: Add system specific quirk to mark clocks as critical

[David Müller]

Hello

Hans de Goede wrote:
> Can you respin the patch to use the old method of looking at which
> clocks are enabled by the BIOS? (note you may want to wait with
> this till Andy and I have reached an agreement on how to solve this)

Sure, I will wait until consensus is reached. ;-)

Dave

Re: [PATCH] clk: x86: Add system specific quirk to mark clocks as critical

[Hans de Goede]

Hi,

On 28-03-19 16:41, David Müller wrote:
> Hello
> 
> Hans de Goede wrote:
>> Can you respin the patch to use the old method of looking at which
>> clocks are enabled by the BIOS? (note you may want to wait with
>> this till Andy and I have reached an agreement on how to solve this)
> 
> Sure, I will wait until consensus is reached. ;-)

I believe that Andy I agree now that at least your case needs the
DMI based approach, so go ahead and respin it :)

Regards,

Hans

Re: [PATCH] clk: x86: Add system specific quirk to mark clocks as critical

[Andy Shevchenko]

On Thu, Mar 28, 2019 at 5:43 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 28-03-19 16:41, David Müller wrote:
> > Hans de Goede wrote:

> >> Can you respin the patch to use the old method of looking at which
> >> clocks are enabled by the BIOS? (note you may want to wait with
> >> this till Andy and I have reached an agreement on how to solve this)
> >
> > Sure, I will wait until consensus is reached. ;-)
>
> I believe that Andy I agree now that at least your case needs the
> DMI based approach, so go ahead and respin it :)

Confirm.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2] clk: x86: Add system specific quirk to mark clocks as critical

[David Müller]

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 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               | 20 +++++++++++++++++++
 .../linux/platform_data/x86/clk-pmc-atom.h    |  7 +++++--
 3 files changed, 36 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..559b2fdf1419 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,26 @@ 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"),
+		},
+	},
+};
+
 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 +419,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

Re: [PATCH v2] clk: x86: Add system specific quirk to mark clocks as critical

[Hans de Goede]

Hi David,

Thank you for v2 of your patch, I'm afraid there is a small but nasty
bug in there, so we are going to need a v3, see comments inline.

(this might be something which existed in v1 and I missed, sorry)

On 03-04-19 16:08, 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 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               | 20 +++++++++++++++++++
>   .../linux/platform_data/x86/clk-pmc-atom.h    |  7 +++++--
>   3 files changed, 36 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..559b2fdf1419 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,26 @@ 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"),
> +		},
> +	},

You need an empty terminating entry here, otherwise the dmi functions
will keep walking the array, so you need to add:

	{}

here.

> +};
> +
>   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 +419,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,

Normally I would be fine with your choice to use dmi_first_match(), but since
you need to respin anyway, can you change this to:

	if (dmi_check_system(critclk_systems)) {
		clk_data->chk_critclks = true;
		pr_info("%s critclks quirk enabled\n", d->ident);
	}

That will allow you to drop the 'd' variable declaration all together and is
somewhat cleaner IMHO.

Regards,

Hans




> 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
> 

Re: [PATCH v2] clk: x86: Add system specific quirk to mark clocks as critical

[Andy Shevchenko]

On Wed, Apr 3, 2019 at 6:31 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 03-04-19 16:08, David Müller wrote:

> >       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,
>
> Normally I would be fine with your choice to use dmi_first_match(), but since
> you need to respin anyway, can you change this to:
>
>         if (dmi_check_system(critclk_systems)) {
>                 clk_data->chk_critclks = true;
>                 pr_info("%s critclks quirk enabled\n", d->ident);
>         }
>
> That will allow you to drop the 'd' variable declaration all together and is
> somewhat cleaner IMHO.

pr_info() relies on it AFAICS.

If needed we may switch to ->callback() instead, but it would take more lines.
--
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] clk: x86: Add system specific quirk to mark clocks as critical

[Hans de Goede]

Hi,

On 03-04-19 17:36, Andy Shevchenko wrote:
> On Wed, Apr 3, 2019 at 6:31 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 03-04-19 16:08, David Müller wrote:
> 
>>>        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,
>>
>> Normally I would be fine with your choice to use dmi_first_match(), but since
>> you need to respin anyway, can you change this to:
>>
>>          if (dmi_check_system(critclk_systems)) {
>>                  clk_data->chk_critclks = true;
>>                  pr_info("%s critclks quirk enabled\n", d->ident);
>>          }
>>
>> That will allow you to drop the 'd' variable declaration all together and is
>> somewhat cleaner IMHO.
> 
> pr_info() relies on it AFAICS.

Ah right I missed that, I would just do: pr_info("System is on critclks quirk list, critclks quirk enabled\n");
but keeping the solution with ident is fine too.

Regards,

Hans

Re: [PATCH v2] clk: x86: Add system specific quirk to mark clocks as critical

[David Müller]

Hans de Goede wrote:
> On 03-04-19 17:36, Andy Shevchenko wrote:
>> On Wed, Apr 3, 2019 at 6:31 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>> On 03-04-19 16:08, David Müller wrote:
>>
>>>>        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,
>>>
>>> Normally I would be fine with your choice to use dmi_first_match(),
>>> but since
>>> you need to respin anyway, can you change this to:
>>>
>>>          if (dmi_check_system(critclk_systems)) {
>>>                  clk_data->chk_critclks = true;
>>>                  pr_info("%s critclks quirk enabled\n", d->ident);
>>>          }
>>>
>>> That will allow you to drop the 'd' variable declaration all together
>>> and is
>>> somewhat cleaner IMHO.
>>
>> pr_info() relies on it AFAICS.
> 
> Ah right I missed that, I would just do: pr_info("System is on critclks
> quirk list, critclks quirk enabled\n");
> but keeping the solution with ident is fine too.

If it is OK, I will keep the "dmi_first_match()" implementation.


Dave

Re: [PATCH v2] clk: x86: Add system specific quirk to mark clocks as critical

[Hans de Goede]

Hi,

On 04-04-19 10:00, David Müller wrote:

 > Is there a rule / recommendation how the sentinel entry should look
 > like? In the kernel sources I see a "{}", "{ }", "{ /*sentinel*/ }", ...

Nope, no rule, you can use whatever feels good to you,
although Andy might have a different opinion...

> Hans de Goede wrote:
>> On 03-04-19 17:36, Andy Shevchenko wrote:
>>> On Wed, Apr 3, 2019 at 6:31 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>> On 03-04-19 16:08, David Müller wrote:
>>>
>>>>>         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,
>>>>
>>>> Normally I would be fine with your choice to use dmi_first_match(),
>>>> but since
>>>> you need to respin anyway, can you change this to:
>>>>
>>>>           if (dmi_check_system(critclk_systems)) {
>>>>                   clk_data->chk_critclks = true;
>>>>                   pr_info("%s critclks quirk enabled\n", d->ident);
>>>>           }
>>>>
>>>> That will allow you to drop the 'd' variable declaration all together
>>>> and is
>>>> somewhat cleaner IMHO.
>>>
>>> pr_info() relies on it AFAICS.
>>
>> Ah right I missed that, I would just do: pr_info("System is on critclks
>> quirk list, critclks quirk enabled\n");
>> but keeping the solution with ident is fine too.
> 
> If it is OK, I will keep the "dmi_first_match()" implementation.

Sure that is fine.

Regards,

Hans

Re: [PATCH v2] clk: x86: Add system specific quirk to mark clocks as critical

[David Müller]

Hello

Thanks for the review.

Hans de Goede wrote:
>> +/*
>> + * 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"),
>> +        },
>> +    },
>
> You need an empty terminating entry here, otherwise the dmi functions
> will keep walking the array, so you need to add:
>
>     {}
>
> here.

Arrgh, shame on me.

Is there a rule / recommendation how the sentinel entry should look
like? In the kernel sources I see a "{}", "{ }", "{ /*sentinel*/ }", ...


Dave

[PATCH v3] clk: x86: Add system specific quirk to mark clocks as critical

[David Müller]

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(-)

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

Re: [PATCH v3] clk: x86: Add system specific quirk to mark clocks as critical

[Hans de Goede]

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
> 

Re: [PATCH v3] clk: x86: Add system specific quirk to mark clocks as critical

[Andy Shevchenko]

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.

>   */

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] clk: x86: Add system specific quirk to mark clocks as critical

[David Müller]

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

[PATCH v4] clk: x86: Add system specific quirk to mark clocks as critical

[David Müller]

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>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v4:
- add '.' to comments / rename field as suggested by Andy Shevchenko
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    |  3 +++
 3 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/x86/clk-pmc-atom.c b/drivers/clk/x86/clk-pmc-atom.c
index d977193842df..19174835693b 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->critical && 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..eaec2d306481 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->critical = 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..7a37ac27d0fb 100644
--- a/include/linux/platform_data/x86/clk-pmc-atom.h
+++ b/include/linux/platform_data/x86/clk-pmc-atom.h
@@ -35,10 +35,13 @@ struct pmc_clk {
  *
  * @base:	PMC clock register base offset
  * @clks:	pointer to set of registered clocks, typically 0..5
+ * @critical:	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 critical;
 };

 #endif /* __PLATFORM_DATA_X86_CLK_PMC_ATOM_H */
--
2.20.1

Re: [PATCH v4] clk: x86: Add system specific quirk to mark clocks as critical

[Andy Shevchenko]

On Mon, Apr 8, 2019 at 4:34 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.
>

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

supposed to go via CLK tree.

> Fixes: 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL")
> Signed-off-by: David Müller <dave.mueller@gmx.ch>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v4:
> - add '.' to comments / rename field as suggested by Andy Shevchenko
> 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    |  3 +++
>  3 files changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/x86/clk-pmc-atom.c b/drivers/clk/x86/clk-pmc-atom.c
> index d977193842df..19174835693b 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->critical && 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..eaec2d306481 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->critical = 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..7a37ac27d0fb 100644
> --- a/include/linux/platform_data/x86/clk-pmc-atom.h
> +++ b/include/linux/platform_data/x86/clk-pmc-atom.h
> @@ -35,10 +35,13 @@ struct pmc_clk {
>   *
>   * @base:      PMC clock register base offset
>   * @clks:      pointer to set of registered clocks, typically 0..5
> + * @critical:  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 critical;
>  };
>
>  #endif /* __PLATFORM_DATA_X86_CLK_PMC_ATOM_H */
> --
> 2.20.1
>


-- 
With Best Regards,
Andy Shevchenko