* Rebase on latest 6.0 clk linux tree and resend patch series because of
no response from anyone so far. Please let me know if anyone has any
concerns about these changes. Thanks.
MxL's CGU driver was found to be lacking below required features. Add these
required lacking features:
1. Since it is a core driver, it has to conform to secure boot & secure
access architecture. In order for the register accesses to be secure
access compliant, it needs regmap support as per our security architecture.
Hence, replace direct read/writel with regmap based IO. Also remove spinlocks
because they are no longer necessary because regmap uses its own lock.
2. There are some gate clocks which are used by the power mgmt IP to gate when
a certain power saving mode is activated. These gate clocks can also be
gated from CGU clk driver. This creates a conflict. To avoid the conflict,
by default disable gating such gate registers from CGU clk driver. But keep
a flag to do so for other older IP's which uses same CGU clk IP but does not
use same power mgmt IP.
3. Fix two functional bugs found during testing.
This patch series is based on below git tree:
git://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git
Rahul Tanwar (5):
clk: mxl: Switch from direct readl/writel based IO to regmap based IO
clk: mxl: Remove unnecessary spinlocks
clk: mxl: Avoid disabling gate clocks from clk driver
clk: mxl: Add validation for register reads/writes
clk: mxl: Add a missing flag to allow parent clock rate change
drivers/clk/x86/Kconfig | 5 +-
drivers/clk/x86/clk-cgu-pll.c | 23 ++-----
drivers/clk/x86/clk-cgu.c | 117 +++++++++++-----------------------
drivers/clk/x86/clk-cgu.h | 71 +++++++++++++--------
drivers/clk/x86/clk-lgm.c | 16 +++--
5 files changed, 101 insertions(+), 131 deletions(-)
--
2.17.1
In MxL's LGM SoC, gate clocks are supposed to be enabled or disabled
from EPU (power management IP) in certain power saving modes. If gate
clocks are allowed to be enabled/disabled from CGU clk driver, then
there arises a conflict where in case clk driver disables a gate clk,
and then EPU tries to disable the same gate clk, then it will hang
polling for the clk gated successful status.
To avoid such a conflict, disable gate clocks enabling/disabling from
CGU clk driver. But add a GATE_CLK_HW flag to control this in order to
be backward compatible with other SoCs which share the same CGU IP but
not the same EPU IP.
Signed-off-by: Rahul Tanwar <[email protected]>
---
drivers/clk/x86/clk-cgu.c | 32 ++++++++++++++++++++++++--------
drivers/clk/x86/clk-cgu.h | 1 +
2 files changed, 25 insertions(+), 8 deletions(-)
diff --git a/drivers/clk/x86/clk-cgu.c b/drivers/clk/x86/clk-cgu.c
index 1f7e93de67bc..d24173cfe0b0 100644
--- a/drivers/clk/x86/clk-cgu.c
+++ b/drivers/clk/x86/clk-cgu.c
@@ -258,8 +258,12 @@ static int lgm_clk_gate_enable(struct clk_hw *hw)
struct lgm_clk_gate *gate = to_lgm_clk_gate(hw);
unsigned int reg;
- reg = GATE_HW_REG_EN(gate->reg);
- lgm_set_clk_val(gate->membase, reg, gate->shift, 1, 1);
+ if (gate->flags & GATE_CLK_HW) {
+ reg = GATE_HW_REG_EN(gate->reg);
+ lgm_set_clk_val(gate->membase, reg, gate->shift, 1, 1);
+ } else {
+ gate->reg = 1;
+ }
return 0;
}
@@ -269,8 +273,12 @@ static void lgm_clk_gate_disable(struct clk_hw *hw)
struct lgm_clk_gate *gate = to_lgm_clk_gate(hw);
unsigned int reg;
- reg = GATE_HW_REG_DIS(gate->reg);
- lgm_set_clk_val(gate->membase, reg, gate->shift, 1, 1);
+ if (gate->flags & GATE_CLK_HW) {
+ reg = GATE_HW_REG_DIS(gate->reg);
+ lgm_set_clk_val(gate->membase, reg, gate->shift, 1, 1);
+ } else {
+ gate->reg = 0;
+ }
}
static int lgm_clk_gate_is_enabled(struct clk_hw *hw)
@@ -278,8 +286,12 @@ static int lgm_clk_gate_is_enabled(struct clk_hw *hw)
struct lgm_clk_gate *gate = to_lgm_clk_gate(hw);
unsigned int reg, ret;
- reg = GATE_HW_REG_STAT(gate->reg);
- ret = lgm_get_clk_val(gate->membase, reg, gate->shift, 1);
+ if (gate->flags & GATE_CLK_HW) {
+ reg = GATE_HW_REG_STAT(gate->reg);
+ ret = lgm_get_clk_val(gate->membase, reg, gate->shift, 1);
+ } else {
+ ret = gate->reg;
+ }
return ret;
}
@@ -315,7 +327,8 @@ lgm_clk_register_gate(struct lgm_clk_provider *ctx,
init.num_parents = pname ? 1 : 0;
gate->membase = ctx->membase;
- gate->reg = reg;
+ if (cflags & GATE_CLK_HW)
+ gate->reg = reg;
gate->shift = shift;
gate->flags = cflags;
gate->hw.init = &init;
@@ -326,7 +339,10 @@ lgm_clk_register_gate(struct lgm_clk_provider *ctx,
return ERR_PTR(ret);
if (cflags & CLOCK_FLAG_VAL_INIT) {
- lgm_set_clk_val(gate->membase, reg, shift, 1, list->gate_val);
+ if (cflags & GATE_CLK_HW)
+ lgm_set_clk_val(gate->membase, reg, shift, 1, list->gate_val);
+ else
+ gate->reg = 1;
}
return hw;
diff --git a/drivers/clk/x86/clk-cgu.h b/drivers/clk/x86/clk-cgu.h
index 0aa0f35d63a0..73ce84345f81 100644
--- a/drivers/clk/x86/clk-cgu.h
+++ b/drivers/clk/x86/clk-cgu.h
@@ -197,6 +197,7 @@ struct lgm_clk_branch {
/* clock flags definition */
#define CLOCK_FLAG_VAL_INIT BIT(16)
#define MUX_CLK_SW BIT(17)
+#define GATE_CLK_HW BIT(18)
#define LGM_MUX(_id, _name, _pdata, _f, _reg, \
_shift, _width, _cf, _v) \
--
2.17.1
Earlier version of driver used direct io remapped register read
writes using readl/writel. But we need secure boot access which
is only possible when registers are read & written using regmap.
This is because the security bus/hook is written & coupled only
with regmap layer.
Switch the driver from direct readl/writel based register accesses
to regmap based register accesses.
Additionally, update the license headers to latest status.
Signed-off-by: Rahul Tanwar <[email protected]>
---
drivers/clk/x86/Kconfig | 5 +++--
drivers/clk/x86/clk-cgu-pll.c | 10 +++++----
drivers/clk/x86/clk-cgu.c | 5 +++--
drivers/clk/x86/clk-cgu.h | 38 +++++++++++++++++++----------------
drivers/clk/x86/clk-lgm.c | 13 ++++++++----
5 files changed, 42 insertions(+), 29 deletions(-)
diff --git a/drivers/clk/x86/Kconfig b/drivers/clk/x86/Kconfig
index 69642e15fcc1..ced99e082e3d 100644
--- a/drivers/clk/x86/Kconfig
+++ b/drivers/clk/x86/Kconfig
@@ -1,8 +1,9 @@
# SPDX-License-Identifier: GPL-2.0-only
config CLK_LGM_CGU
depends on OF && HAS_IOMEM && (X86 || COMPILE_TEST)
+ select MFD_SYSCON
select OF_EARLY_FLATTREE
bool "Clock driver for Lightning Mountain(LGM) platform"
help
- Clock Generation Unit(CGU) driver for Intel Lightning Mountain(LGM)
- network processor SoC.
+ Clock Generation Unit(CGU) driver for MaxLinear's x86 based
+ Lightning Mountain(LGM) network processor SoC.
diff --git a/drivers/clk/x86/clk-cgu-pll.c b/drivers/clk/x86/clk-cgu-pll.c
index 3179557b5f78..c83083affe88 100644
--- a/drivers/clk/x86/clk-cgu-pll.c
+++ b/drivers/clk/x86/clk-cgu-pll.c
@@ -1,8 +1,9 @@
// SPDX-License-Identifier: GPL-2.0
/*
+ * Copyright (C) 2020-2022 MaxLinear, Inc.
* Copyright (C) 2020 Intel Corporation.
- * Zhu YiXin <[email protected]>
- * Rahul Tanwar <[email protected]>
+ * Zhu Yixin <[email protected]>
+ * Rahul Tanwar <[email protected]>
*/
#include <linux/clk-provider.h>
@@ -76,8 +77,9 @@ static int lgm_pll_enable(struct clk_hw *hw)
spin_lock_irqsave(&pll->lock, flags);
lgm_set_clk_val(pll->membase, pll->reg, 0, 1, 1);
- ret = readl_poll_timeout_atomic(pll->membase + pll->reg,
- val, (val & 0x1), 1, 100);
+ ret = regmap_read_poll_timeout_atomic(pll->membase, pll->reg,
+ val, (val & 0x1), 1, 100);
+
spin_unlock_irqrestore(&pll->lock, flags);
return ret;
diff --git a/drivers/clk/x86/clk-cgu.c b/drivers/clk/x86/clk-cgu.c
index 33de600e0c38..f5f30a18f486 100644
--- a/drivers/clk/x86/clk-cgu.c
+++ b/drivers/clk/x86/clk-cgu.c
@@ -1,8 +1,9 @@
// SPDX-License-Identifier: GPL-2.0
/*
+ * Copyright (C) 2020-2022 MaxLinear, Inc.
* Copyright (C) 2020 Intel Corporation.
- * Zhu YiXin <[email protected]>
- * Rahul Tanwar <[email protected]>
+ * Zhu Yixin <[email protected]>
+ * Rahul Tanwar <[email protected]>
*/
#include <linux/clk-provider.h>
#include <linux/device.h>
diff --git a/drivers/clk/x86/clk-cgu.h b/drivers/clk/x86/clk-cgu.h
index 4e22bfb22312..dbcb66468797 100644
--- a/drivers/clk/x86/clk-cgu.h
+++ b/drivers/clk/x86/clk-cgu.h
@@ -1,18 +1,19 @@
/* SPDX-License-Identifier: GPL-2.0 */
/*
- * Copyright(c) 2020 Intel Corporation.
- * Zhu YiXin <[email protected]>
- * Rahul Tanwar <[email protected]>
+ * Copyright (C) 2020-2022 MaxLinear, Inc.
+ * Copyright (C) 2020 Intel Corporation.
+ * Zhu Yixin <[email protected]>
+ * Rahul Tanwar <[email protected]>
*/
#ifndef __CLK_CGU_H
#define __CLK_CGU_H
-#include <linux/io.h>
+#include <linux/regmap.h>
struct lgm_clk_mux {
struct clk_hw hw;
- void __iomem *membase;
+ struct regmap *membase;
unsigned int reg;
u8 shift;
u8 width;
@@ -22,7 +23,7 @@ struct lgm_clk_mux {
struct lgm_clk_divider {
struct clk_hw hw;
- void __iomem *membase;
+ struct regmap *membase;
unsigned int reg;
u8 shift;
u8 width;
@@ -35,7 +36,7 @@ struct lgm_clk_divider {
struct lgm_clk_ddiv {
struct clk_hw hw;
- void __iomem *membase;
+ struct regmap *membase;
unsigned int reg;
u8 shift0;
u8 width0;
@@ -53,7 +54,7 @@ struct lgm_clk_ddiv {
struct lgm_clk_gate {
struct clk_hw hw;
- void __iomem *membase;
+ struct regmap *membase;
unsigned int reg;
u8 shift;
unsigned long flags;
@@ -77,7 +78,7 @@ enum lgm_clk_type {
* @clk_data: array of hw clocks and clk number.
*/
struct lgm_clk_provider {
- void __iomem *membase;
+ struct regmap *membase;
struct device_node *np;
struct device *dev;
struct clk_hw_onecell_data clk_data;
@@ -92,7 +93,7 @@ enum pll_type {
struct lgm_clk_pll {
struct clk_hw hw;
- void __iomem *membase;
+ struct regmap *membase;
unsigned int reg;
unsigned long flags;
enum pll_type type;
@@ -300,29 +301,32 @@ struct lgm_clk_branch {
.div = _d, \
}
-static inline void lgm_set_clk_val(void __iomem *membase, u32 reg,
+static inline void lgm_set_clk_val(struct regmap *membase, u32 reg,
u8 shift, u8 width, u32 set_val)
{
u32 mask = (GENMASK(width - 1, 0) << shift);
- u32 regval;
- regval = readl(membase + reg);
- regval = (regval & ~mask) | ((set_val << shift) & mask);
- writel(regval, membase + reg);
+ regmap_update_bits(membase, reg, mask, set_val << shift);
}
-static inline u32 lgm_get_clk_val(void __iomem *membase, u32 reg,
+static inline u32 lgm_get_clk_val(struct regmap *membase, u32 reg,
u8 shift, u8 width)
{
u32 mask = (GENMASK(width - 1, 0) << shift);
u32 val;
- val = readl(membase + reg);
+ if (regmap_read(membase, reg, &val)) {
+ WARN_ONCE(1, "Failed to read clk reg: 0x%x\n", reg);
+ return 0;
+ }
+
val = (val & mask) >> shift;
return val;
}
+
+
int lgm_clk_register_branches(struct lgm_clk_provider *ctx,
const struct lgm_clk_branch *list,
unsigned int nr_clk);
diff --git a/drivers/clk/x86/clk-lgm.c b/drivers/clk/x86/clk-lgm.c
index 020f4e83a5cc..4fa2bcaf71c8 100644
--- a/drivers/clk/x86/clk-lgm.c
+++ b/drivers/clk/x86/clk-lgm.c
@@ -1,10 +1,12 @@
// SPDX-License-Identifier: GPL-2.0
/*
+ * Copyright (C) 2020-2022 MaxLinear, Inc.
* Copyright (C) 2020 Intel Corporation.
- * Zhu YiXin <[email protected]>
- * Rahul Tanwar <[email protected]>
+ * Zhu Yixin <[email protected]>
+ * Rahul Tanwar <[email protected]>
*/
#include <linux/clk-provider.h>
+#include <linux/mfd/syscon.h>
#include <linux/of.h>
#include <linux/platform_device.h>
#include <dt-bindings/clock/intel,lgm-clk.h>
@@ -433,9 +435,12 @@ static int lgm_cgu_probe(struct platform_device *pdev)
ctx->clk_data.num = CLK_NR_CLKS;
- ctx->membase = devm_platform_ioremap_resource(pdev, 0);
- if (IS_ERR(ctx->membase))
+ ctx->membase = syscon_node_to_regmap(np);
+ if (IS_ERR_OR_NULL(ctx->membase)) {
+ dev_err(dev, "Failed to get clk CGU iomem\n");
return PTR_ERR(ctx->membase);
+ }
+
ctx->np = np;
ctx->dev = dev;
--
2.17.1
Quoting Rahul Tanwar (2022-09-21 23:24:24)
> diff --git a/drivers/clk/x86/clk-cgu-pll.c b/drivers/clk/x86/clk-cgu-pll.c
> index 3179557b5f78..c83083affe88 100644
> --- a/drivers/clk/x86/clk-cgu-pll.c
> +++ b/drivers/clk/x86/clk-cgu-pll.c
> @@ -1,8 +1,9 @@
> // SPDX-License-Identifier: GPL-2.0
> /*
> + * Copyright (C) 2020-2022 MaxLinear, Inc.
> * Copyright (C) 2020 Intel Corporation.
> - * Zhu YiXin <[email protected]>
> - * Rahul Tanwar <[email protected]>
> + * Zhu Yixin <[email protected]>
Does Zhu Yixin approve? They're not Cced on this patch.
> + * Rahul Tanwar <[email protected]>
> */
>
> #include <linux/clk-provider.h>
Quoting Rahul Tanwar (2022-09-21 23:24:26)
> In MxL's LGM SoC, gate clocks are supposed to be enabled or disabled
> from EPU (power management IP) in certain power saving modes. If gate
> clocks are allowed to be enabled/disabled from CGU clk driver, then
> there arises a conflict where in case clk driver disables a gate clk,
> and then EPU tries to disable the same gate clk, then it will hang
> polling for the clk gated successful status.
Is there any point in registering these clks when they're not supposed
to be controlled from Linux?
Hi Stephen,
Thanks for review and response.
On 29/9/2022 8:14 am, Stephen Boyd wrote:
> This email was sent from outside of MaxLinear.
>
>
> Quoting Rahul Tanwar (2022-09-21 23:24:24)
>> diff --git a/drivers/clk/x86/clk-cgu-pll.c b/drivers/clk/x86/clk-cgu-pll.c
>> index 3179557b5f78..c83083affe88 100644
>> --- a/drivers/clk/x86/clk-cgu-pll.c
>> +++ b/drivers/clk/x86/clk-cgu-pll.c
>> @@ -1,8 +1,9 @@
>> // SPDX-License-Identifier: GPL-2.0
>> /*
>> + * Copyright (C) 2020-2022 MaxLinear, Inc.
>> * Copyright (C) 2020 Intel Corporation.
>> - * Zhu YiXin <[email protected]>
>> - * Rahul Tanwar <[email protected]>
>> + * Zhu Yixin <[email protected]>
>
> Does Zhu Yixin approve? They're not Cced on this patch.
Zhu Yixin is part of [email protected] mail list which is CCed
here. In v3, i will separately CC Yixin and request him to review again
and provide a Reviewed-by tag if no concerns.
Thanks,
Rahul
>
>> + * Rahul Tanwar <[email protected]>
>> */
>>
>> #include <linux/clk-provider.h>
>
>
On 29/9/2022 8:17 am, Stephen Boyd wrote:
> This email was sent from outside of MaxLinear.
>
>
> Quoting Rahul Tanwar (2022-09-21 23:24:26)
>> In MxL's LGM SoC, gate clocks are supposed to be enabled or disabled
>> from EPU (power management IP) in certain power saving modes. If gate
>> clocks are allowed to be enabled/disabled from CGU clk driver, then
>> there arises a conflict where in case clk driver disables a gate clk,
>> and then EPU tries to disable the same gate clk, then it will hang
>> polling for the clk gated successful status.
>
> Is there any point in registering these clks when they're not supposed
> to be controlled from Linux?
As mentioned in the full commit log, only reason to register these clks
is to be backward compatible with older versions of similar SoC's which
reuse the same clk CGU IP but do not use same power management IP. Such
older SoCs also use the same clk driver and for them these clks are
required to be controlled by clk ops from Linux.
Thanks,
Rahul
>
>
[Resend due to mail delivery failure in earlier reply - one email id got
corrupted somehow in earlier reply]
Hi Stephen,
On 30/9/2022 9:01 am, Stephen Boyd wrote:
> This email was sent from outside of MaxLinear.
>
>
> Quoting Rahul Tanwar (2022-09-28 22:45:59)
>> On 29/9/2022 8:17 am, Stephen Boyd wrote:
>>> This email was sent from outside of MaxLinear.
>>>
>>>
>>> Quoting Rahul Tanwar (2022-09-21 23:24:26)
>>>> In MxL's LGM SoC, gate clocks are supposed to be enabled or disabled
>>>> from EPU (power management IP) in certain power saving modes. If gate
>>>> clocks are allowed to be enabled/disabled from CGU clk driver, then
>>>> there arises a conflict where in case clk driver disables a gate clk,
>>>> and then EPU tries to disable the same gate clk, then it will hang
>>>> polling for the clk gated successful status.
>>>
>>> Is there any point in registering these clks when they're not supposed
>>> to be controlled from Linux?
>>
>>
>> As mentioned in the full commit log, only reason to register these clks
>> is to be backward compatible with older versions of similar SoC's which
>> reuse the same clk CGU IP but do not use same power management IP. Such
>> older SoCs also use the same clk driver and for them these clks are
>> required to be controlled by clk ops from Linux.
>>
>
> Why is the clk driver probing on the new SoCs? Is it providing
> something? Can we detect that the power management IP exists and not
> register these clks?
>
We discussed in the team about not registering gate clks at all as you
mentioned. But if we do that, all peripheral drivers that use these clks
would need modifications so their probe does not fail due to failure
returns of clk related standard calls for e.g devm_clk_get(),
clk_prepare_enable(). These are standard calls in probe for all the
drivers and a lot of them use gate clks. So this would need a lot of
changes with possibility of breaking working functionalities.
Also, i incorrectly mentioned about the reason being backward
compatibility with older SoCs. Main reason is to support different power
profiles use cases as per end product requirements some of which might
control it from clk framework i.e. this driver. We keep a internal
driver flag just for this purpose to provide this flexibility depending
on the use case which is what we have used here.
I am sending v3 with more clear & correct description about it to
justify the need for these changes.
Thanks,
Rahul
>
Hi Stephen,
On 6/10/2022 4:20 am, Stephen Boyd wrote:
> This email was sent from outside of MaxLinear.
>
>
> Quoting Rahul Tanwar (2022-10-05 02:36:00)
>>>>
>>>
>>> Why is the clk driver probing on the new SoCs? Is it providing
>>> something? Can we detect that the power management IP exists and not
>>> register these clks?
>>>
>>
>> We discussed in the team about not registering gate clks at all as you
>> mentioned. But if we do that, all peripheral drivers that use these clks
>> would need modifications so their probe does not fail due to failure
>> returns of clk related standard calls for e.g devm_clk_get(),
>> clk_prepare_enable(). These are standard calls in probe for all the
>> drivers and a lot of them use gate clks. So this would need a lot of
>> changes with possibility of breaking working functionalities.
>
> Why? We can have clk_get() return NULL when these clks can't be used.
> This is how we implement "dummy" clks on systems where the clk provider
> can provide the clk but there's nothing to do for the clk operations.
> The clk API treats NULL as a valid clk but bails out early when calling
> any consumer APIs.
>
I agree that what you are suggesting is the ideal way to handle such
clks. If i understand you correctly, you are suggesting below (please
correct me if i am mistaken):
1. Disable/remove such clocks from corresponding driver's devicetree
nodes. This will make devm_clk_get() or clk_get() return failure.
2. Modify all drivers which use such clks
from:
clk = devm_clk_get(...);
if (IS_ERR(clk))
return -ERROR;
clk_prepare_enable(clk);
clk_get/set_rate();
...
to:
clk = devm_clk_get(...);
if (!(IS_ERR(clk)) {
clk_prepare_enable(clk);
clk_get/set_rate();
...
} else {
// print error info - do nothing, no clk_ops calls
}
But the problem that we have is that 80% of the clks that we use fall
under this category (around 65 clks). And if we follow this approach,
then we will have to make above changes in all of the drivers which use
these clks & retest them again. That seems like a overhaul. We already
keep a internal driver flag in each clk entry data structure and we
already use it in the driver for other types of clks for e.g MUX_CLKs.
So using the internal flag to handle this becomes a preferable &
existing driver design aligned approach for us.
Thanks,
Rahul
>>
>> Also, i incorrectly mentioned about the reason being backward
>> compatibility with older SoCs. Main reason is to support different power
>> profiles use cases as per end product requirements some of which might
>> control it from clk framework i.e. this driver. We keep a internal
>> driver flag just for this purpose to provide this flexibility depending
>> on the use case which is what we have used here.
>>
>> I am sending v3 with more clear & correct description about it to
>> justify the need for these changes.
>>
>
> Ok
>
>