[v2,2/4] clk: renesas: rcar-gen3-cpg: add spinlock
diff mbox series

Message ID eef5ee23-311a-4367-43b5-b793357f06ef@cogentembedded.com
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series
  • Renesas R8A77980 CPG/MSSR RPC clock support
Related show

Commit Message

Sergei Shtylyov Dec. 4, 2018, 7:49 p.m. UTC
Protect the CPG register read-modify-write sequence with a spinlock.

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
Changes in version 2:
- new patch.

 drivers/clk/renesas/rcar-gen3-cpg.c |    8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Simon Horman Dec. 14, 2018, 3:33 p.m. UTC | #1
On Tue, Dec 04, 2018 at 10:49:44PM +0300, Sergei Shtylyov wrote:
> Protect the CPG register read-modify-write sequence with a spinlock.

Spinlocks are expensive, I think an explanation of why this is
necessary would be worthwhile.

> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> ---
> Changes in version 2:
> - new patch.
> 
>  drivers/clk/renesas/rcar-gen3-cpg.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> Index: renesas-drivers/drivers/clk/renesas/rcar-gen3-cpg.c
> ===================================================================
> --- renesas-drivers.orig/drivers/clk/renesas/rcar-gen3-cpg.c
> +++ renesas-drivers/drivers/clk/renesas/rcar-gen3-cpg.c
> @@ -30,14 +30,19 @@
>  
>  #define CPG_RCKCR_CKSEL	BIT(15)	/* RCLK Clock Source Select */
>  
> +static spinlock_t cpg_lock;
> +
>  static void cpg_reg_modify(void __iomem *reg, u32 clear, u32 set)
>  {
> +	unsigned long flags;
>  	u32 val;
>  
> +	spin_lock_irqsave(&cpg_lock, flags);
>  	val = readl(reg);
>  	val &= ~clear;
>  	val |= set;
>  	writel(val, reg);
> +	spin_unlock_irqrestore(&cpg_lock, flags);
>  };
>  
>  struct cpg_simple_notifier {
> @@ -604,5 +609,8 @@ int __init rcar_gen3_cpg_init(const stru
>  	if (attr)
>  		cpg_quirks = (uintptr_t)attr->data;
>  	pr_debug("%s: mode = 0x%x quirks = 0x%x\n", __func__, mode, cpg_quirks);
> +
> +	spin_lock_init(&cpg_lock);
> +
>  	return 0;
>  }
>
Geert Uytterhoeven Jan. 21, 2019, 1:40 p.m. UTC | #2
Hi Simon,

On Fri, Dec 14, 2018 at 4:33 PM Simon Horman <horms@verge.net.au> wrote:
> On Tue, Dec 04, 2018 at 10:49:44PM +0300, Sergei Shtylyov wrote:
> > Protect the CPG register read-modify-write sequence with a spinlock.
>
> Spinlocks are expensive, I think an explanation of why this is
> necessary would be worthwhile.

Some clock registers contain bits for multiple clocks.
Hence read-modify-write cycles on these registers should be protected
against concurrent accesses for multiple clocks.

Gr{oetje,eeting}s,

                        Geert
Geert Uytterhoeven Jan. 21, 2019, 2:08 p.m. UTC | #3
On Tue, Dec 4, 2018 at 8:49 PM Sergei Shtylyov
<sergei.shtylyov@cogentembedded.com> wrote:
> Protect the CPG register read-modify-write sequence with a spinlock.
>
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert
Sergei Shtylyov Jan. 21, 2019, 6:49 p.m. UTC | #4
On 12/14/2018 06:33 PM, Simon Horman wrote:
> On Tue, Dec 04, 2018 at 10:49:44PM +0300, Sergei Shtylyov wrote:
>> Protect the CPG register read-modify-write sequence with a spinlock.
> 
> Spinlocks are expensive, I think an explanation of why this is
> necessary would be worthwhile.

   The driver using a spinlock to protect the hardware registers is such a 
common scenario, I didn't think it was worth a special explanation. :-)

> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
[...]

MBR, Sergei

Patch
diff mbox series

Index: renesas-drivers/drivers/clk/renesas/rcar-gen3-cpg.c
===================================================================
--- renesas-drivers.orig/drivers/clk/renesas/rcar-gen3-cpg.c
+++ renesas-drivers/drivers/clk/renesas/rcar-gen3-cpg.c
@@ -30,14 +30,19 @@ 
 
 #define CPG_RCKCR_CKSEL	BIT(15)	/* RCLK Clock Source Select */
 
+static spinlock_t cpg_lock;
+
 static void cpg_reg_modify(void __iomem *reg, u32 clear, u32 set)
 {
+	unsigned long flags;
 	u32 val;
 
+	spin_lock_irqsave(&cpg_lock, flags);
 	val = readl(reg);
 	val &= ~clear;
 	val |= set;
 	writel(val, reg);
+	spin_unlock_irqrestore(&cpg_lock, flags);
 };
 
 struct cpg_simple_notifier {
@@ -604,5 +609,8 @@  int __init rcar_gen3_cpg_init(const stru
 	if (attr)
 		cpg_quirks = (uintptr_t)attr->data;
 	pr_debug("%s: mode = 0x%x quirks = 0x%x\n", __func__, mode, cpg_quirks);
+
+	spin_lock_init(&cpg_lock);
+
 	return 0;
 }