[PATCH] platform/x86: pmc_atom: Match all Beckhoff Automation baytrail boards with critclk_systems DMI table

[PATCH] platform/x86: pmc_atom: Match all Beckhoff Automation baytrail boards with critclk_systems DMI table

[Steffen Dirkwinkel]

From: Steffen Dirkwinkel <s.dirkwinkel@beckhoff.com>

pmc_plt_clk* clocks are used for ethernet controllers so need to stay
turned on. This adds the affected board family to critclk_systems DMI
table so the clocks are marked as CLK_CRITICAL and not turned off.

This replaces the previosly listed boards with a match for the whole
device family. There are new affected boards that would otherwise need
to be listed. There are only few unaffected boards in the family and
having the clocks turned on is not an issue on those.

Fixes: 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL")
Signed-off-by: Steffen Dirkwinkel <s.dirkwinkel@beckhoff.com>
---
 drivers/platform/x86/pmc_atom.c | 28 ++--------------------------
 1 file changed, 2 insertions(+), 26 deletions(-)

diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c
index ca684ed760d1..a9d2a4b98e57 100644
--- a/drivers/platform/x86/pmc_atom.c
+++ b/drivers/platform/x86/pmc_atom.c
@@ -393,34 +393,10 @@ static const struct dmi_system_id critclk_systems[] = {
 	},
 	{
 		/* pmc_plt_clk* - are used for ethernet controllers */
-		.ident = "Beckhoff CB3163",
+		.ident = "Beckhoff Baytrail",
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Beckhoff Automation"),
-			DMI_MATCH(DMI_BOARD_NAME, "CB3163"),
-		},
-	},
-	{
-		/* pmc_plt_clk* - are used for ethernet controllers */
-		.ident = "Beckhoff CB4063",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "Beckhoff Automation"),
-			DMI_MATCH(DMI_BOARD_NAME, "CB4063"),
-		},
-	},
-	{
-		/* pmc_plt_clk* - are used for ethernet controllers */
-		.ident = "Beckhoff CB6263",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "Beckhoff Automation"),
-			DMI_MATCH(DMI_BOARD_NAME, "CB6263"),
-		},
-	},
-	{
-		/* pmc_plt_clk* - are used for ethernet controllers */
-		.ident = "Beckhoff CB6363",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "Beckhoff Automation"),
-			DMI_MATCH(DMI_BOARD_NAME, "CB6363"),
+			DMI_MATCH(DMI_PRODUCT_FAMILY, "CBxx63"),
 		},
 	},
 	{
-- 
2.31.1

Re: [PATCH] platform/x86: pmc_atom: Match all Beckhoff Automation baytrail boards with critclk_systems DMI table

[Andy Shevchenko]

On Mon, Apr 12, 2021 at 12:29 PM Steffen Dirkwinkel
<linux-kernel-dev@beckhoff.com> wrote:
>
> From: Steffen Dirkwinkel <s.dirkwinkel@beckhoff.com>
>
> pmc_plt_clk* clocks are used for ethernet controllers so need to stay
> turned on. This adds the affected board family to critclk_systems DMI
> table so the clocks are marked as CLK_CRITICAL and not turned off.
>
> This replaces the previosly listed boards with a match for the whole

"...previously..."

> device family. There are new affected boards that would otherwise need
> to be listed. There are only few unaffected boards in the family and

"...only a few..."

> having the clocks turned on is not an issue on those.

"...not an issue."

> Fixes: 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL")
> Signed-off-by: Steffen Dirkwinkel <s.dirkwinkel@beckhoff.com>

I'm afraid it's a bit too much. Is there any guarantee all the boards
based on x86 will be Baytrail only?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] platform/x86: pmc_atom: Match all Beckhoff Automation baytrail boards with critclk_systems DMI table

[linux-kernel-dev]

On Mo, 2021-04-12 at 12:43 +0300, Andy Shevchenko wrote:
>
> On Mon, Apr 12, 2021 at 12:29 PM Steffen Dirkwinkel
> <linux-kernel-dev@beckhoff.com> wrote:
> >
> > From: Steffen Dirkwinkel <s.dirkwinkel@beckhoff.com>
> >
> > pmc_plt_clk* clocks are used for ethernet controllers so need to stay
> > turned on. This adds the affected board family to critclk_systems DMI
> > table so the clocks are marked as CLK_CRITICAL and not turned off.
> >
> > This replaces the previosly listed boards with a match for the whole
>
> "...previously..."
thanks

>
> > device family. There are new affected boards that would otherwise need
> > to be listed. There are only few unaffected boards in the family and
>
> "...only a few..."
will drop the phrase

>
> > having the clocks turned on is not an issue on those.
>
> "...not an issue."
Not an issue for these industrial PCs as sleep is an unusual use case.
Having no ethernet after boot/sleep is worse.

>
> > Fixes: 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL")
> > Signed-off-by: Steffen Dirkwinkel <s.dirkwinkel@beckhoff.com>
>
> I'm afraid it's a bit too much. Is there any guarantee all the boards
> based on x86 will be Baytrail only?
>
Sorry, I guess I should make this clearer in the message.
All boards with "CBxx63" are Baytrail.


> --
> With Best Regards,
> Andy Shevchenko
>

Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff Registered office: Verl, Germany | Register court: Guetersloh HRA 7075

Re: [PATCH] platform/x86: pmc_atom: Match all Beckhoff Automation baytrail boards with critclk_systems DMI table

[Andy Shevchenko]

On Mon, Apr 12, 2021 at 1:39 PM linux-kernel-dev
<linux-kernel-dev@beckhoff.com> wrote:
> On Mo, 2021-04-12 at 12:43 +0300, Andy Shevchenko wrote:
> > On Mon, Apr 12, 2021 at 12:29 PM Steffen Dirkwinkel
> > <linux-kernel-dev@beckhoff.com> wrote:

...

> > I'm afraid it's a bit too much. Is there any guarantee all the boards
> > based on x86 will be Baytrail only?
> >
> Sorry, I guess I should make this clearer in the message.
> All boards with "CBxx63" are Baytrail.

Exactly! And this supports my idea that this shouldn't be done like in
this patch.
Are you guaranteeing that *all x86-based* boards produced by your
company will be Baytrail only?
Above tells that the answer is rather "no". So, I think we can't apply
this patch in its current form.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] platform/x86: pmc_atom: Match all Beckhoff Automation baytrail boards with critclk_systems DMI table

[linux-kernel-dev]

On Mo, 2021-04-12 at 13:54 +0300, Andy Shevchenko wrote:
> CAUTION: External Email!!
>
>
> On Mon, Apr 12, 2021 at 1:39 PM linux-kernel-dev
> <linux-kernel-dev@beckhoff.com> wrote:
> > On Mo, 2021-04-12 at 12:43 +0300, Andy Shevchenko wrote:
> > > On Mon, Apr 12, 2021 at 12:29 PM Steffen Dirkwinkel
> > > <linux-kernel-dev@beckhoff.com> wrote:
>
> ...
>
> > > I'm afraid it's a bit too much. Is there any guarantee all the boards
> > > based on x86 will be Baytrail only?
> > >
> > Sorry, I guess I should make this clearer in the message.
> > All boards with "CBxx63" are Baytrail.
>
> Exactly! And this supports my idea that this shouldn't be done like in
> this patch.
> Are you guaranteeing that *all x86-based* boards produced by your
> company will be Baytrail only?
> Above tells that the answer is rather "no". So, I think we can't apply
> this patch in its current form.

All boards with DMI_PRODUCT_FAMILY="CBxx63" are Baytrail boards. We do produce other x86 boards but the family
is exclusive to Baytrail.
I might be misunderstanding how the matching works. Does this match anything other than CBxx63?

 .matches = {
 DMI_MATCH(DMI_SYS_VENDOR, "Beckhoff Automation"),
DMI_MATCH(DMI_PRODUCT_FAMILY, "CBxx63"),
},

I can switch it to DMI_EXACT_MATCH but even substring matching works.

>
> --
> With Best Regards,
> Andy Shevchenko
>

Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff Registered office: Verl, Germany | Register court: Guetersloh HRA 7075

Re: [PATCH] platform/x86: pmc_atom: Match all Beckhoff Automation baytrail boards with critclk_systems DMI table

[Andy Shevchenko]

On Mon, Apr 12, 2021 at 2:07 PM linux-kernel-dev
<linux-kernel-dev@beckhoff.com> wrote:
> On Mo, 2021-04-12 at 13:54 +0300, Andy Shevchenko wrote:
> > CAUTION: External Email!!
> > On Mon, Apr 12, 2021 at 1:39 PM linux-kernel-dev
> > <linux-kernel-dev@beckhoff.com> wrote:
> > > On Mo, 2021-04-12 at 12:43 +0300, Andy Shevchenko wrote:
> > > > On Mon, Apr 12, 2021 at 12:29 PM Steffen Dirkwinkel
> > > > <linux-kernel-dev@beckhoff.com> wrote:

...

> > > > I'm afraid it's a bit too much. Is there any guarantee all the boards
> > > > based on x86 will be Baytrail only?
> > > >
> > > Sorry, I guess I should make this clearer in the message.
> > > All boards with "CBxx63" are Baytrail.
> >
> > Exactly! And this supports my idea that this shouldn't be done like in
> > this patch.
> > Are you guaranteeing that *all x86-based* boards produced by your
> > company will be Baytrail only?
> > Above tells that the answer is rather "no". So, I think we can't apply
> > this patch in its current form.
>
> All boards with DMI_PRODUCT_FAMILY="CBxx63" are Baytrail boards. We do produce other x86 boards but the family
> is exclusive to Baytrail.
> I might be misunderstanding how the matching works. Does this match anything other than CBxx63?
>
>  .matches = {
>  DMI_MATCH(DMI_SYS_VENDOR, "Beckhoff Automation"),
> DMI_MATCH(DMI_PRODUCT_FAMILY, "CBxx63"),
> },
>
> I can switch it to DMI_EXACT_MATCH but even substring matching works.

Sorry, I missed the second match line. Yes, looks correct to me now.

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

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2] platform/x86: pmc_atom: Match all Beckhoff Automation baytrail boards with critclk_systems DMI table

[Steffen Dirkwinkel]

From: Steffen Dirkwinkel <s.dirkwinkel@beckhoff.com>

pmc_plt_clk* clocks are used for ethernet controllers, so need to stay
turned on. This adds the affected board family to critclk_systems DMI
table, so the clocks are marked as CLK_CRITICAL and not turned off.

This replaces the previously listed boards with a match for the whole
device family CBxx63. CBxx63 matches only baytrail devices.
There are new affected boards that would otherwise need to be listed.
There are unaffected boards in the family, but having the clocks
turned on is not an issue.

Fixes: 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL")
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Steffen Dirkwinkel <s.dirkwinkel@beckhoff.com>
---
 drivers/platform/x86/pmc_atom.c | 28 ++--------------------------
 1 file changed, 2 insertions(+), 26 deletions(-)

diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c
index ca684ed760d1..a9d2a4b98e57 100644
--- a/drivers/platform/x86/pmc_atom.c
+++ b/drivers/platform/x86/pmc_atom.c
@@ -393,34 +393,10 @@ static const struct dmi_system_id critclk_systems[] = {
 	},
 	{
 		/* pmc_plt_clk* - are used for ethernet controllers */
-		.ident = "Beckhoff CB3163",
+		.ident = "Beckhoff Baytrail",
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Beckhoff Automation"),
-			DMI_MATCH(DMI_BOARD_NAME, "CB3163"),
-		},
-	},
-	{
-		/* pmc_plt_clk* - are used for ethernet controllers */
-		.ident = "Beckhoff CB4063",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "Beckhoff Automation"),
-			DMI_MATCH(DMI_BOARD_NAME, "CB4063"),
-		},
-	},
-	{
-		/* pmc_plt_clk* - are used for ethernet controllers */
-		.ident = "Beckhoff CB6263",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "Beckhoff Automation"),
-			DMI_MATCH(DMI_BOARD_NAME, "CB6263"),
-		},
-	},
-	{
-		/* pmc_plt_clk* - are used for ethernet controllers */
-		.ident = "Beckhoff CB6363",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "Beckhoff Automation"),
-			DMI_MATCH(DMI_BOARD_NAME, "CB6363"),
+			DMI_MATCH(DMI_PRODUCT_FAMILY, "CBxx63"),
 		},
 	},
 	{
-- 
2.31.1

Re: [PATCH v2] platform/x86: pmc_atom: Match all Beckhoff Automation baytrail boards with critclk_systems DMI table

[Hans de Goede]

Hi,

On 4/12/21 3:30 PM, Steffen Dirkwinkel wrote:
> From: Steffen Dirkwinkel <s.dirkwinkel@beckhoff.com>
> 
> pmc_plt_clk* clocks are used for ethernet controllers, so need to stay
> turned on. This adds the affected board family to critclk_systems DMI
> table, so the clocks are marked as CLK_CRITICAL and not turned off.
> 
> This replaces the previously listed boards with a match for the whole
> device family CBxx63. CBxx63 matches only baytrail devices.
> There are new affected boards that would otherwise need to be listed.
> There are unaffected boards in the family, but having the clocks
> turned on is not an issue.
> 
> Fixes: 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL")
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Steffen Dirkwinkel <s.dirkwinkel@beckhoff.com>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans




> ---
>  drivers/platform/x86/pmc_atom.c | 28 ++--------------------------
>  1 file changed, 2 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c
> index ca684ed760d1..a9d2a4b98e57 100644
> --- a/drivers/platform/x86/pmc_atom.c
> +++ b/drivers/platform/x86/pmc_atom.c
> @@ -393,34 +393,10 @@ static const struct dmi_system_id critclk_systems[] = {
>  	},
>  	{
>  		/* pmc_plt_clk* - are used for ethernet controllers */
> -		.ident = "Beckhoff CB3163",
> +		.ident = "Beckhoff Baytrail",
>  		.matches = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "Beckhoff Automation"),
> -			DMI_MATCH(DMI_BOARD_NAME, "CB3163"),
> -		},
> -	},
> -	{
> -		/* pmc_plt_clk* - are used for ethernet controllers */
> -		.ident = "Beckhoff CB4063",
> -		.matches = {
> -			DMI_MATCH(DMI_SYS_VENDOR, "Beckhoff Automation"),
> -			DMI_MATCH(DMI_BOARD_NAME, "CB4063"),
> -		},
> -	},
> -	{
> -		/* pmc_plt_clk* - are used for ethernet controllers */
> -		.ident = "Beckhoff CB6263",
> -		.matches = {
> -			DMI_MATCH(DMI_SYS_VENDOR, "Beckhoff Automation"),
> -			DMI_MATCH(DMI_BOARD_NAME, "CB6263"),
> -		},
> -	},
> -	{
> -		/* pmc_plt_clk* - are used for ethernet controllers */
> -		.ident = "Beckhoff CB6363",
> -		.matches = {
> -			DMI_MATCH(DMI_SYS_VENDOR, "Beckhoff Automation"),
> -			DMI_MATCH(DMI_BOARD_NAME, "CB6363"),
> +			DMI_MATCH(DMI_PRODUCT_FAMILY, "CBxx63"),
>  		},
>  	},
>  	{
>