[PATCH v2 0/4] clocksource: Add MCT support for ARTPEC-8

[PATCH v2 0/4] clocksource: Add MCT support for ARTPEC-8

[Vincent Whitchurch]

This series add supports for the timer block on ARTPEC-8.  The block itself is
fully compatible with the existing exynos4210-mct driver.  The ARTPEC-8 SoC
uses this block from two separate processors running Linux (AMP) so it needs
some extra code to allow this sharing.

v2:
- The series is now rebased on top of Krzysztof's patch "dt-bindings: timer:
  exynos4210-mct: describe known hardware and its interrupts".
- Combine the Kconfig change and the local timer change into one series
- Use devicetree property rather than module parameter for the local timer handling
- Add specific compatible with the correct number of interrupts.

Vincent Whitchurch (4):
  dt-bindings: timer: exynos4210-mct: Add ARTPEC-8 MCT
  dt-bindings: timer: exynos4210-mct: Support using only local timer
  clocksource/drivers/exynos_mct: Support local-timer-index property
  clocksource/drivers/exynos_mct: Enable building on ARTPEC

 .../timer/samsung,exynos4210-mct.yaml         | 11 ++++++++
 drivers/clocksource/Kconfig                   |  2 +-
 drivers/clocksource/exynos_mct.c              | 25 +++++++++++++++----
 3 files changed, 32 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH v2 1/4] dt-bindings: timer: exynos4210-mct: Add ARTPEC-8 MCT

[Vincent Whitchurch]

This SoC has an MCT with 4 global and 8 local timer interrupts, add a
specific compatible to match it as is done for the other platforms with
this hardware block.

Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---

Notes:
    v2: New.  Requires Krzysztof's "dt-bindings: timer: exynos4210-mct: describe
        hardware and its interrupts".

 .../devicetree/bindings/timer/samsung,exynos4210-mct.yaml       | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.yaml b/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.yaml
index 1584944c7ac4..dce42f1f7574 100644
--- a/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.yaml
+++ b/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.yaml
@@ -25,6 +25,7 @@ properties:
           - samsung,exynos4412-mct
       - items:
           - enum:
+              - axis,artpec8-mct
               - samsung,exynos3250-mct
               - samsung,exynos5250-mct
               - samsung,exynos5260-mct
@@ -102,6 +103,7 @@ allOf:
         compatible:
           contains:
             enum:
+              - axis,artpec8-mct
               - samsung,exynos5260-mct
               - samsung,exynos5420-mct
               - samsung,exynos5433-mct
-- 
2.34.1

[PATCH v2 2/4] dt-bindings: timer: exynos4210-mct: Support using only local timer

[Vincent Whitchurch]

The ARTPEC-8 SoC has a quad-core Cortex-A53 and a single-core Cortex-A5
which share one MCT with one global and eight local timers.  The
Cortex-A53 and Cortex-A5 do not have cache-coherency between them, and
therefore run two separate kernels.

The Cortex-A53 boots first and starts the global FRC and also registers
a clock events device using the global timer.  (This global timer clock
events is usually replaced by arch timer clock events for each of the
cores.)

When the A5 boots, we should not use the global timer interrupts or
write to the global timer registers.  This is because even if there are
four global comparators, the control bits for all four are in the same
registers, and we would need to synchronize between the cpus.  Instead,
the global timer FRC (already started by the A53) should be used as the
clock source, and one of the local timers which are not used by the A53
can be used for clock events on the A5.

To support this usecase, add a property to the binding to specify the
first local timer index to be used. If this parameter is non-zero, the
global timer interrupts will also not be used.

Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---

Notes:
    v2: New.

 .../bindings/timer/samsung,exynos4210-mct.yaml           | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.yaml b/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.yaml
index dce42f1f7574..46f466081836 100644
--- a/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.yaml
+++ b/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.yaml
@@ -47,6 +47,15 @@ properties:
   reg:
     maxItems: 1
 
+  local-timer-index:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    default: 0
+    maximum: 15     # Last local timer index
+    description: |
+      If present, sets the first local timer index to use.  If this value is
+      set to a non-default value, the global timer will not be used for
+      interrupts.
+
   interrupts:
     description: |
       Interrupts should be put in specific order. This is, the local timer
-- 
2.34.1

[PATCH v2 3/4] clocksource/drivers/exynos_mct: Support local-timer-index property

[Vincent Whitchurch]

Support the documented semantics of the local-timer-index property: Use
it as the first index of the local timer, ensure that global timer clock
events device is not registered, and don't write to the global FRC if it
is already started.

Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---

Notes:
    v2: Use devicetree property instead of module parameter.

 drivers/clocksource/exynos_mct.c | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index f29c812b70c9..5f8b516614eb 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -33,7 +33,7 @@
 #define EXYNOS4_MCT_G_INT_ENB		EXYNOS4_MCTREG(0x248)
 #define EXYNOS4_MCT_G_WSTAT		EXYNOS4_MCTREG(0x24C)
 #define _EXYNOS4_MCT_L_BASE		EXYNOS4_MCTREG(0x300)
-#define EXYNOS4_MCT_L_BASE(x)		(_EXYNOS4_MCT_L_BASE + (0x100 * x))
+#define EXYNOS4_MCT_L_BASE(x)		(_EXYNOS4_MCT_L_BASE + (0x100 * (x)))
 #define EXYNOS4_MCT_L_MASK		(0xffffff00)
 
 #define MCT_L_TCNTB_OFFSET		(0x00)
@@ -75,6 +75,7 @@ enum {
 static void __iomem *reg_base;
 static unsigned long clk_rate;
 static unsigned int mct_int_type;
+static unsigned int mct_local_idx;
 static int mct_irqs[MCT_NR_IRQS];
 
 struct mct_clock_event_device {
@@ -157,6 +158,17 @@ static void exynos4_mct_frc_start(void)
 	u32 reg;
 
 	reg = readl_relaxed(reg_base + EXYNOS4_MCT_G_TCON);
+
+	/*
+	 * If the FRC is already running, we don't need to start it again.  We
+	 * could probably just do this on all systems, but, to avoid any risk
+	 * for regressions, we only do it on systems where it's absolutely
+	 * necessary (i.e., on systems where writes to the global registers
+	 * need to be avoided).
+	 */
+	if (mct_local_idx && (reg & MCT_G_TCON_START))
+		return;
+
 	reg |= MCT_G_TCON_START;
 	exynos4_mct_write(reg, EXYNOS4_MCT_G_TCON);
 }
@@ -449,7 +461,7 @@ static int exynos4_mct_starting_cpu(unsigned int cpu)
 		per_cpu_ptr(&percpu_mct_tick, cpu);
 	struct clock_event_device *evt = &mevt->evt;
 
-	mevt->base = EXYNOS4_MCT_L_BASE(cpu);
+	mevt->base = EXYNOS4_MCT_L_BASE(mct_local_idx + cpu);
 	snprintf(mevt->name, sizeof(mevt->name), "mct_tick%d", cpu);
 
 	evt->name = mevt->name;
@@ -554,13 +566,14 @@ static int __init exynos4_timer_interrupts(struct device_node *np,
 	} else {
 		for_each_possible_cpu(cpu) {
 			int mct_irq;
+			unsigned int irqidx = MCT_L0_IRQ + mct_local_idx + cpu;
 			struct mct_clock_event_device *pcpu_mevt =
 				per_cpu_ptr(&percpu_mct_tick, cpu);
 
 			pcpu_mevt->evt.irq = -1;
-			if (MCT_L0_IRQ + cpu >= ARRAY_SIZE(mct_irqs))
+			if (irqidx >= ARRAY_SIZE(mct_irqs))
 				break;
-			mct_irq = mct_irqs[MCT_L0_IRQ + cpu];
+			mct_irq = mct_irqs[irqidx];
 
 			irq_set_status_flags(mct_irq, IRQ_NOAUTOEN);
 			if (request_irq(mct_irq,
@@ -607,6 +620,8 @@ static int __init mct_init_dt(struct device_node *np, unsigned int int_type)
 {
 	int ret;
 
+	of_property_read_u32(np, "local-timer-index", &mct_local_idx);
+
 	ret = exynos4_timer_resources(np);
 	if (ret)
 		return ret;
@@ -619,7 +634,7 @@ static int __init mct_init_dt(struct device_node *np, unsigned int int_type)
 	if (ret)
 		return ret;
 
-	return exynos4_clockevent_init();
+	return (mct_local_idx == 0) ? exynos4_clockevent_init() : ret;
 }
 
 
-- 
2.34.1

[PATCH v2 4/4] clocksource/drivers/exynos_mct: Enable building on ARTPEC

[Vincent Whitchurch]

This timer block is used on ARTPEC-8.

Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---

Notes:
    v2: No changes.

 drivers/clocksource/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index ae95d06a4a8f..2ea981ef23af 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -419,7 +419,7 @@ config ATMEL_TCB_CLKSRC
 config CLKSRC_EXYNOS_MCT
 	bool "Exynos multi core timer driver" if COMPILE_TEST
 	depends on ARM || ARM64
-	depends on ARCH_EXYNOS || COMPILE_TEST
+	depends on ARCH_ARTPEC || ARCH_EXYNOS || COMPILE_TEST
 	help
 	  Support for Multi Core Timer controller on Exynos SoCs.
 
-- 
2.34.1

Re: [PATCH v2 1/4] dt-bindings: timer: exynos4210-mct: Add ARTPEC-8 MCT

[Krzysztof Kozlowski]

On 08/03/2022 15:24, Vincent Whitchurch wrote:
> This SoC has an MCT with 4 global and 8 local timer interrupts, add a
> specific compatible to match it as is done for the other platforms with
> this hardware block.
> 
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
> ---
> 
> Notes:
>     v2: New.  Requires Krzysztof's "dt-bindings: timer: exynos4210-mct: describe
>         hardware and its interrupts".
> 


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Best regards,
Krzysztof

Re: [PATCH v2 2/4] dt-bindings: timer: exynos4210-mct: Support using only local timer

[Krzysztof Kozlowski]

On 08/03/2022 15:24, Vincent Whitchurch wrote:
> The ARTPEC-8 SoC has a quad-core Cortex-A53 and a single-core Cortex-A5
> which share one MCT with one global and eight local timers.  The
> Cortex-A53 and Cortex-A5 do not have cache-coherency between them, and
> therefore run two separate kernels.
> 
> The Cortex-A53 boots first and starts the global FRC and also registers
> a clock events device using the global timer.  (This global timer clock
> events is usually replaced by arch timer clock events for each of the
> cores.)
> 
> When the A5 boots, we should not use the global timer interrupts or
> write to the global timer registers.  This is because even if there are
> four global comparators, the control bits for all four are in the same
> registers, and we would need to synchronize between the cpus.  Instead,
> the global timer FRC (already started by the A53) should be used as the
> clock source, and one of the local timers which are not used by the A53
> can be used for clock events on the A5.
> 
> To support this usecase, add a property to the binding to specify the
> first local timer index to be used. If this parameter is non-zero, the
> global timer interrupts will also not be used.
> 
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
> ---
> 
> Notes:
>     v2: New.
> 
>  .../bindings/timer/samsung,exynos4210-mct.yaml           | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.yaml b/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.yaml
> index dce42f1f7574..46f466081836 100644
> --- a/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.yaml
> +++ b/Documentation/devicetree/bindings/timer/samsung,exynos4210-mct.yaml
> @@ -47,6 +47,15 @@ properties:
>    reg:
>      maxItems: 1
>  
> +  local-timer-index:

You need vendor prefix. Also this should describe the actual hardware,
not driver behavior, so rather:
"samsung,local-timers"
with a uint32-array type and list of timers to use.

You also need separate property to skip FRC, so something like:
"samsung,frc-shared"
of type boolean.

In the bindings please describe the hardware, not the result you want to
achieve from driver model point of view.

Also disallow this for all other compatibles:
allOf:
 - if:
     not:
       properties:
       ...
   then:
     properties:
       samsung,local-timers: false
       samsung,frc-shared: false

The property simply should not be used outside of Artpec8. It's not
valid in other configurations.

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    default: 0
> +    maximum: 15     # Last local timer index
> +    description: |
> +      If present, sets the first local timer index to use.  If this value is
> +      set to a non-default value, the global timer will not be used for
> +      interrupts.

Do not describe the driver, but the hardware. Instead explain which
local timers are allowed to be used.

> +
>    interrupts:
>      description: |
>        Interrupts should be put in specific order. This is, the local timer


Best regards,
Krzysztof

Re: [PATCH v2 3/4] clocksource/drivers/exynos_mct: Support local-timer-index property

[Krzysztof Kozlowski]

On 08/03/2022 15:24, Vincent Whitchurch wrote:
> Support the documented semantics of the local-timer-index property: Use
> it as the first index of the local timer, ensure that global timer clock
> events device is not registered, and don't write to the global FRC if it
> is already started.
> 
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
> ---
> 
> Notes:
>     v2: Use devicetree property instead of module parameter.
> 
>  drivers/clocksource/exynos_mct.c | 25 ++++++++++++++++++++-----
>  1 file changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
> index f29c812b70c9..5f8b516614eb 100644
> --- a/drivers/clocksource/exynos_mct.c
> +++ b/drivers/clocksource/exynos_mct.c
> @@ -33,7 +33,7 @@
>  #define EXYNOS4_MCT_G_INT_ENB		EXYNOS4_MCTREG(0x248)
>  #define EXYNOS4_MCT_G_WSTAT		EXYNOS4_MCTREG(0x24C)
>  #define _EXYNOS4_MCT_L_BASE		EXYNOS4_MCTREG(0x300)
> -#define EXYNOS4_MCT_L_BASE(x)		(_EXYNOS4_MCT_L_BASE + (0x100 * x))
> +#define EXYNOS4_MCT_L_BASE(x)		(_EXYNOS4_MCT_L_BASE + (0x100 * (x)))
>  #define EXYNOS4_MCT_L_MASK		(0xffffff00)
>  
>  #define MCT_L_TCNTB_OFFSET		(0x00)
> @@ -75,6 +75,7 @@ enum {
>  static void __iomem *reg_base;
>  static unsigned long clk_rate;
>  static unsigned int mct_int_type;
> +static unsigned int mct_local_idx;

No more static variables. This was wrong design, happens, but let's not
grow the list.

I propose to conditionally (depending on property samsung,frc-shared)
assign .resume callback to NULL or exynos4_frc_resume. The init can
receive an argument whether to call frc_start().


>  static int mct_irqs[MCT_NR_IRQS];
>  
>  struct mct_clock_event_device {
> @@ -157,6 +158,17 @@ static void exynos4_mct_frc_start(void)
>  	u32 reg;
>  
>  	reg = readl_relaxed(reg_base + EXYNOS4_MCT_G_TCON);
> +
> +	/*
> +	 * If the FRC is already running, we don't need to start it again.  We
> +	 * could probably just do this on all systems, but, to avoid any risk
> +	 * for regressions, we only do it on systems where it's absolutely
> +	 * necessary (i.e., on systems where writes to the global registers
> +	 * need to be avoided).
> +	 */
> +	if (mct_local_idx && (reg & MCT_G_TCON_START))

This contradicts your intentions in commit #2 msg, where you described
that A53 will be started first.

1. If A53 is always started first, is it possible to be here from A5?
2. If above is possible, how do you handle locking? For example:
a. A53 started with some delay, entered exynos4_mct_frc_start() pass
this check;
b. A5 gets to exynos4_mct_frc_start(), check is still false, so A5
enables the FRC,
c. A53 also enables the FRC.

Having here relaxed reads and writes makes it even worse, unfortunately.

Best regards,
Krzysztof

Re: [PATCH v2 4/4] clocksource/drivers/exynos_mct: Enable building on ARTPEC

[Krzysztof Kozlowski]

On 08/03/2022 15:24, Vincent Whitchurch wrote:
> This timer block is used on ARTPEC-8.
> 
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
> ---
> 


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Best regards,
Krzysztof

Re: [PATCH v2 3/4] clocksource/drivers/exynos_mct: Support local-timer-index property

[Vincent Whitchurch]

On Tue, Mar 08, 2022 at 03:57:55PM +0100, Krzysztof Kozlowski wrote:
> On 08/03/2022 15:24, Vincent Whitchurch wrote:
> > diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
> > index f29c812b70c9..5f8b516614eb 100644
> > --- a/drivers/clocksource/exynos_mct.c
> > +++ b/drivers/clocksource/exynos_mct.c
> > @@ -33,7 +33,7 @@
> >  #define EXYNOS4_MCT_G_INT_ENB		EXYNOS4_MCTREG(0x248)
> >  #define EXYNOS4_MCT_G_WSTAT		EXYNOS4_MCTREG(0x24C)
> >  #define _EXYNOS4_MCT_L_BASE		EXYNOS4_MCTREG(0x300)
> > -#define EXYNOS4_MCT_L_BASE(x)		(_EXYNOS4_MCT_L_BASE + (0x100 * x))
> > +#define EXYNOS4_MCT_L_BASE(x)		(_EXYNOS4_MCT_L_BASE + (0x100 * (x)))
> >  #define EXYNOS4_MCT_L_MASK		(0xffffff00)
> >  
> >  #define MCT_L_TCNTB_OFFSET		(0x00)
> > @@ -75,6 +75,7 @@ enum {
> >  static void __iomem *reg_base;
> >  static unsigned long clk_rate;
> >  static unsigned int mct_int_type;
> > +static unsigned int mct_local_idx;
> 
> No more static variables. This was wrong design, happens, but let's not
> grow the list.
> 
> I propose to conditionally (depending on property samsung,frc-shared)
> assign .resume callback to NULL or exynos4_frc_resume. The init can
> receive an argument whether to call frc_start().

Could we just add the skip-write-register-if-already-started change in
exynos4_mct_frc_start() uncondtionally?  Perhaps it could be in a
separate patch too?  I was probably being over-cautious when I did it
conditionally on mct_local_idx.  Doing it uncondtionally would make it
easier to remove the global variable.

On my system the FRC is actually started long before Linux, and I assume
it's similar on other chips.

> 
> >  static int mct_irqs[MCT_NR_IRQS];
> >  
> >  struct mct_clock_event_device {
> > @@ -157,6 +158,17 @@ static void exynos4_mct_frc_start(void)
> >  	u32 reg;
> >  
> >  	reg = readl_relaxed(reg_base + EXYNOS4_MCT_G_TCON);
> > +
> > +	/*
> > +	 * If the FRC is already running, we don't need to start it again.  We
> > +	 * could probably just do this on all systems, but, to avoid any risk
> > +	 * for regressions, we only do it on systems where it's absolutely
> > +	 * necessary (i.e., on systems where writes to the global registers
> > +	 * need to be avoided).
> > +	 */
> > +	if (mct_local_idx && (reg & MCT_G_TCON_START))
> 
> This contradicts your intentions in commit #2 msg, where you described
> that A53 will be started first.

Yes, you're right.  The case of the FRC not being running when the A5
starts up is only ever hit in our simulation environment where we are
able to start Linux on the A5 directly, without having to go via the
A53.

> 1. If A53 is always started first, is it possible to be here from A5?
> 2. If above is possible, how do you handle locking? For example:
> a. A53 started with some delay, entered exynos4_mct_frc_start() pass
> this check;
> b. A5 gets to exynos4_mct_frc_start(), check is still false, so A5
> enables the FRC,
> c. A53 also enables the FRC.

The A5 is normally started from Linux on the A53 (using the remoteproc
framework).  This is long after exynos4_mct_frc_start() has been called
on the A53.

Re: [PATCH v2 3/4] clocksource/drivers/exynos_mct: Support local-timer-index property

[Krzysztof Kozlowski]

On 11/03/2022 12:35, Vincent Whitchurch wrote:
> On Tue, Mar 08, 2022 at 03:57:55PM +0100, Krzysztof Kozlowski wrote:
>> On 08/03/2022 15:24, Vincent Whitchurch wrote:
>>> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
>>> index f29c812b70c9..5f8b516614eb 100644
>>> --- a/drivers/clocksource/exynos_mct.c
>>> +++ b/drivers/clocksource/exynos_mct.c
>>> @@ -33,7 +33,7 @@
>>>  #define EXYNOS4_MCT_G_INT_ENB		EXYNOS4_MCTREG(0x248)
>>>  #define EXYNOS4_MCT_G_WSTAT		EXYNOS4_MCTREG(0x24C)
>>>  #define _EXYNOS4_MCT_L_BASE		EXYNOS4_MCTREG(0x300)
>>> -#define EXYNOS4_MCT_L_BASE(x)		(_EXYNOS4_MCT_L_BASE + (0x100 * x))
>>> +#define EXYNOS4_MCT_L_BASE(x)		(_EXYNOS4_MCT_L_BASE + (0x100 * (x)))
>>>  #define EXYNOS4_MCT_L_MASK		(0xffffff00)
>>>  
>>>  #define MCT_L_TCNTB_OFFSET		(0x00)
>>> @@ -75,6 +75,7 @@ enum {
>>>  static void __iomem *reg_base;
>>>  static unsigned long clk_rate;
>>>  static unsigned int mct_int_type;
>>> +static unsigned int mct_local_idx;
>>
>> No more static variables. This was wrong design, happens, but let's not
>> grow the list.
>>
>> I propose to conditionally (depending on property samsung,frc-shared)
>> assign .resume callback to NULL or exynos4_frc_resume. The init can
>> receive an argument whether to call frc_start().
> 
> Could we just add the skip-write-register-if-already-started change in
> exynos4_mct_frc_start() uncondtionally?  Perhaps it could be in a
> separate patch too?  I was probably being over-cautious when I did it
> conditionally on mct_local_idx.  Doing it uncondtionally would make it
> easier to remove the global variable.
> 
> On my system the FRC is actually started long before Linux, and I assume
> it's similar on other chips.

+Cc Marek,

Maybe we could skip it, I don't know. It could be enabled by early boot
code or by trusted firmware. This would require more testing, on few
different platforms.

On my Exynos5422 HC1 board the MCT is not running upon boot. The
EXYNOS4_MCT_G_TCON starts with a reset value (0x0).

> 
>>
>>>  static int mct_irqs[MCT_NR_IRQS];
>>>  
>>>  struct mct_clock_event_device {
>>> @@ -157,6 +158,17 @@ static void exynos4_mct_frc_start(void)
>>>  	u32 reg;
>>>  
>>>  	reg = readl_relaxed(reg_base + EXYNOS4_MCT_G_TCON);
>>> +
>>> +	/*
>>> +	 * If the FRC is already running, we don't need to start it again.  We
>>> +	 * could probably just do this on all systems, but, to avoid any risk
>>> +	 * for regressions, we only do it on systems where it's absolutely
>>> +	 * necessary (i.e., on systems where writes to the global registers
>>> +	 * need to be avoided).
>>> +	 */
>>> +	if (mct_local_idx && (reg & MCT_G_TCON_START))
>>
>> This contradicts your intentions in commit #2 msg, where you described
>> that A53 will be started first.
> 
> Yes, you're right.  The case of the FRC not being running when the A5
> starts up is only ever hit in our simulation environment where we are
> able to start Linux on the A5 directly, without having to go via the
> A53.
> 
>> 1. If A53 is always started first, is it possible to be here from A5?
>> 2. If above is possible, how do you handle locking? For example:
>> a. A53 started with some delay, entered exynos4_mct_frc_start() pass
>> this check;
>> b. A5 gets to exynos4_mct_frc_start(), check is still false, so A5
>> enables the FRC,
>> c. A53 also enables the FRC.
> 
> The A5 is normally started from Linux on the A53 (using the remoteproc
> framework).  This is long after exynos4_mct_frc_start() has been called
> on the A53.

If it is 100% like this, let's make it explicit - if it is A53 (lack of
dedicated property), let's start it. If it A5 (property present), skip it.

Let's wait for Marek thoughts, he was digging the MCT a lot.


Best regards,
Krzysztof

Re: [PATCH v2 3/4] clocksource/drivers/exynos_mct: Support local-timer-index property

[Marek Szyprowski]

Hi Krzysztof,

On 11.03.2022 13:51, Krzysztof Kozlowski wrote:
> On 11/03/2022 12:35, Vincent Whitchurch wrote:
>> On Tue, Mar 08, 2022 at 03:57:55PM +0100, Krzysztof Kozlowski wrote:
>>> On 08/03/2022 15:24, Vincent Whitchurch wrote:
>>>> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
>>>> index f29c812b70c9..5f8b516614eb 100644
>>>> --- a/drivers/clocksource/exynos_mct.c
>>>> +++ b/drivers/clocksource/exynos_mct.c
>>>> @@ -33,7 +33,7 @@
>>>>   #define EXYNOS4_MCT_G_INT_ENB		EXYNOS4_MCTREG(0x248)
>>>>   #define EXYNOS4_MCT_G_WSTAT		EXYNOS4_MCTREG(0x24C)
>>>>   #define _EXYNOS4_MCT_L_BASE		EXYNOS4_MCTREG(0x300)
>>>> -#define EXYNOS4_MCT_L_BASE(x)		(_EXYNOS4_MCT_L_BASE + (0x100 * x))
>>>> +#define EXYNOS4_MCT_L_BASE(x)		(_EXYNOS4_MCT_L_BASE + (0x100 * (x)))
>>>>   #define EXYNOS4_MCT_L_MASK		(0xffffff00)
>>>>   
>>>>   #define MCT_L_TCNTB_OFFSET		(0x00)
>>>> @@ -75,6 +75,7 @@ enum {
>>>>   static void __iomem *reg_base;
>>>>   static unsigned long clk_rate;
>>>>   static unsigned int mct_int_type;
>>>> +static unsigned int mct_local_idx;
>>> No more static variables. This was wrong design, happens, but let's not
>>> grow the list.
>>>
>>> I propose to conditionally (depending on property samsung,frc-shared)
>>> assign .resume callback to NULL or exynos4_frc_resume. The init can
>>> receive an argument whether to call frc_start().
>> Could we just add the skip-write-register-if-already-started change in
>> exynos4_mct_frc_start() uncondtionally?  Perhaps it could be in a
>> separate patch too?  I was probably being over-cautious when I did it
>> conditionally on mct_local_idx.  Doing it uncondtionally would make it
>> easier to remove the global variable.
>>
>> On my system the FRC is actually started long before Linux, and I assume
>> it's similar on other chips.
> +Cc Marek,
>
> Maybe we could skip it, I don't know. It could be enabled by early boot
> code or by trusted firmware. This would require more testing, on few
> different platforms.
>
> On my Exynos5422 HC1 board the MCT is not running upon boot. The
> EXYNOS4_MCT_G_TCON starts with a reset value (0x0).
>
>>>>   static int mct_irqs[MCT_NR_IRQS];
>>>>   
>>>>   struct mct_clock_event_device {
>>>> @@ -157,6 +158,17 @@ static void exynos4_mct_frc_start(void)
>>>>   	u32 reg;
>>>>   
>>>>   	reg = readl_relaxed(reg_base + EXYNOS4_MCT_G_TCON);
>>>> +
>>>> +	/*
>>>> +	 * If the FRC is already running, we don't need to start it again.  We
>>>> +	 * could probably just do this on all systems, but, to avoid any risk
>>>> +	 * for regressions, we only do it on systems where it's absolutely
>>>> +	 * necessary (i.e., on systems where writes to the global registers
>>>> +	 * need to be avoided).
>>>> +	 */
>>>> +	if (mct_local_idx && (reg & MCT_G_TCON_START))
>>> This contradicts your intentions in commit #2 msg, where you described
>>> that A53 will be started first.
>> Yes, you're right.  The case of the FRC not being running when the A5
>> starts up is only ever hit in our simulation environment where we are
>> able to start Linux on the A5 directly, without having to go via the
>> A53.
>>
>>> 1. If A53 is always started first, is it possible to be here from A5?
>>> 2. If above is possible, how do you handle locking? For example:
>>> a. A53 started with some delay, entered exynos4_mct_frc_start() pass
>>> this check;
>>> b. A5 gets to exynos4_mct_frc_start(), check is still false, so A5
>>> enables the FRC,
>>> c. A53 also enables the FRC.
>> The A5 is normally started from Linux on the A53 (using the remoteproc
>> framework).  This is long after exynos4_mct_frc_start() has been called
>> on the A53.
> If it is 100% like this, let's make it explicit - if it is A53 (lack of
> dedicated property), let's start it. If it A5 (property present), skip it.
>
> Let's wait for Marek thoughts, he was digging the MCT a lot.


Right, I've played a bit with MCT on some older Exynos SoCs (ARM 32bit 
based and even Exynos5433) and it looked that none of it enabled MCT FRC 
timer in their proprietary firmware. I've even proposed a patch for this 
once ([1]), but such approach has been rejected. I think that calling 
exynos4_mct_frc_start() unconditionally won't hurt.

[1] https://lore.kernel.org/all/20181018095708.1527-5-m.szyprowski@samsung.com/

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v2 3/4] clocksource/drivers/exynos_mct: Support local-timer-index property

[Vincent Whitchurch]

On Mon, Mar 21, 2022 at 09:00:08AM +0100, Marek Szyprowski wrote:
> Right, I've played a bit with MCT on some older Exynos SoCs (ARM 32bit 
> based and even Exynos5433) and it looked that none of it enabled MCT FRC 
> timer in their proprietary firmware. I've even proposed a patch for this 
> once ([1]), but such approach has been rejected. I think that calling 
> exynos4_mct_frc_start() unconditionally won't hurt.

Thank you for looking into this.  The proposal was however not to avoid
changing when exynos4_mct_frc_start() is called, but to instead skip the
write to the Timer Enable bit of the G_TCON register if it is already
set, like in the below patch.  (This is needed to avoid races when the
FRC is shared between CPUs in an AMP configuration, since TCON can be
modified for other reasons from the CPU which is using the global
comparator.)

If I understand your comment correctly, such a change should not cause
any difference at least on the platforms you looked at since there
MCT_G_TCON_START will not have been set at startup.

diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c
index 6db3d5511b0f..ed462e0a77ff 100644
--- a/drivers/clocksource/exynos_mct.c
+++ b/drivers/clocksource/exynos_mct.c
@@ -162,6 +162,9 @@ static void exynos4_mct_frc_start(void)
 	u32 reg;
 
 	reg = readl_relaxed(reg_base + EXYNOS4_MCT_G_TCON);
+	if (reg & MCT_G_TCON_START)
+		return;
+
 	reg |= MCT_G_TCON_START;
 	exynos4_mct_write(reg, EXYNOS4_MCT_G_TCON);
 }

Re: [PATCH v2 3/4] clocksource/drivers/exynos_mct: Support local-timer-index property

[Vincent Whitchurch]

On Wed, Mar 30, 2022 at 10:21:37AM +0200, Vincent Whitchurch wrote:
> On Mon, Mar 21, 2022 at 09:00:08AM +0100, Marek Szyprowski wrote:
> > Right, I've played a bit with MCT on some older Exynos SoCs (ARM 32bit 
> > based and even Exynos5433) and it looked that none of it enabled MCT FRC 
> > timer in their proprietary firmware. I've even proposed a patch for this 
> > once ([1]), but such approach has been rejected. I think that calling 
> > exynos4_mct_frc_start() unconditionally won't hurt.
> 
> Thank you for looking into this.  The proposal was however not to avoid
> changing when exynos4_mct_frc_start() is called, but to instead skip the
> write to the Timer Enable bit of the G_TCON register if it is already
> set, like in the below patch.  (This is needed to avoid races when the
> FRC is shared between CPUs in an AMP configuration, since TCON can be
> modified for other reasons from the CPU which is using the global
> comparator.)
> 
> If I understand your comment correctly, such a change should not cause
> any difference at least on the platforms you looked at since there
> MCT_G_TCON_START will not have been set at startup.

I needed the frc-shared property anyway to prevent registration of the
clock events so I followed Krzysztof's suggestion of doing this
conditionally and also clearing the resume callback.