Re: [PATCH 2/6] crypto: jz4780-rng: Make ingenic CGU driver use syscon

2017-08-21 Thread Paul Burton 
Hi PrasannaKumar,

On Sunday, 20 August 2017 09:12:12 PDT PrasannaKumar Muralidharan wrote:
> > Could you instead perhaps:
> >  - Just add "syscon" as a second compatible string to the CGU node in the
> >  
> >device tree, but otherwise leave it as-is without the extra
> >cgu_registers
> >node.
> >  
> >  - Have your RNG device node as a child of the CGU node, which should let
> >  it
> >  
> >pick up the regmap via syscon_node_to_regmap() as it already does.
> >  
> >  - Leave the CGU driver as-is, so it can continue accessing memory
> >  directly
> >  
> >rather than via regmap.
> 
> As per my understanding, CGU driver and syscon will map the same
> register set. Is that fine?

That should work fine.

It's only risky if you have 2 drivers using the same registers in ways that 
might race with one another or clobber one another's data. In this case each 
of the 2 drivers is using a different subset of registers that just happen to 
be in the same address range, so there shouldn't be an issue.

Thanks,
Paul

signature.asc
Description: This is a digitally signed message part.

Re: [PATCH 2/6] crypto: jz4780-rng: Make ingenic CGU driver use syscon

2017-08-20 Thread PrasannaKumar Muralidharan 
Hi Paul,

Thanks for your review.

On 19 August 2017 at 02:18, Paul Burton  wrote:
> Hi PrasannaKumar,
>
> On Thursday, 17 August 2017 11:25:16 PDT PrasannaKumar Muralidharan wrote:
>> Ingenic PRNG registers are a part of the same hardware block as clock
>> and power stuff. It is handled by CGU driver. Ingenic M200 SoC has power
>> related registers that are after the PRNG registers. So instead of
>> shortening the register range use syscon interface to expose regmap.
>> This regmap is used by the PRNG driver.
>>
>> Signed-off-by: PrasannaKumar Muralidharan 
>> ---
>>  arch/mips/boot/dts/ingenic/jz4740.dtsi | 14 +++
>>  arch/mips/boot/dts/ingenic/jz4780.dtsi | 14 +++
>>  drivers/clk/ingenic/cgu.c  | 46
>> +- drivers/clk/ingenic/cgu.h  |
>>  9 +--
>>  drivers/clk/ingenic/jz4740-cgu.c   | 30 +++---
>>  drivers/clk/ingenic/jz4780-cgu.c   | 10 
>>  6 files changed, 73 insertions(+), 50 deletions(-)
>>
>> diff --git a/arch/mips/boot/dts/ingenic/jz4740.dtsi
>> b/arch/mips/boot/dts/ingenic/jz4740.dtsi index 2ca7ce7..ada5e67 100644
>> --- a/arch/mips/boot/dts/ingenic/jz4740.dtsi
>> +++ b/arch/mips/boot/dts/ingenic/jz4740.dtsi
>> @@ -34,14 +34,18 @@
>>   clock-frequency = <32768>;
>>   };
>>
>> - cgu: jz4740-cgu@1000 {
>> - compatible = "ingenic,jz4740-cgu";
>> + cgu_registers {
>> + compatible = "simple-mfd", "syscon";
>>   reg = <0x1000 0x100>;
>>
>> - clocks = <>, <>;
>> - clock-names = "ext", "rtc";
>> + cgu: jz4780-cgu@1000 {
>> + compatible = "ingenic,jz4740-cgu";
>>
>> - #clock-cells = <1>;
>> + clocks = <>, <>;
>> + clock-names = "ext", "rtc";
>> +
>> + #clock-cells = <1>;
>> + };
>>   };
>>
>>   rtc_dev: rtc@10003000 {
>> diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi
>> b/arch/mips/boot/dts/ingenic/jz4780.dtsi index 4853ef6..1301694 100644
>> --- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
>> +++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
>> @@ -34,14 +34,18 @@
>>   clock-frequency = <32768>;
>>   };
>>
>> - cgu: jz4780-cgu@1000 {
>> - compatible = "ingenic,jz4780-cgu";
>> + cgu_registers {
>> + compatible = "simple-mfd", "syscon";
>>   reg = <0x1000 0x100>;
>>
>> - clocks = <>, <>;
>> - clock-names = "ext", "rtc";
>> + cgu: jz4780-cgu@1000 {
>> + compatible = "ingenic,jz4780-cgu";
>>
>> - #clock-cells = <1>;
>> + clocks = <>, <>;
>> + clock-names = "ext", "rtc";
>> +
>> + #clock-cells = <1>;
>> + };
>>   };
>>
>>   pinctrl: pin-controller@1001 {
>> diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
>> index e8248f9..2f12c7c 100644
>> --- a/drivers/clk/ingenic/cgu.c
>> +++ b/drivers/clk/ingenic/cgu.c
>> @@ -29,6 +29,18 @@
>>
>>  #define MHZ (1000 * 1000)
>>
>> +unsigned int cgu_readl(struct ingenic_cgu *cgu, unsigned int reg)
>> +{
>> + unsigned int val = 0;
>> + regmap_read(cgu->regmap, reg, );
>> + return val;
>> +}
>> +
>> +int cgu_writel(struct ingenic_cgu *cgu, unsigned int val, unsigned int reg)
>> +{
>> + return regmap_write(cgu->regmap, reg, val);
>> +}
>
> This is going to introduce a lot of overhead to the CGU driver - each
> regmap_read() or regmap_write() call will not only add overhead from the
> indirection but also acquire a lock which we really don't need.
>

Indeed.

> Could you instead perhaps:
>
>  - Just add "syscon" as a second compatible string to the CGU node in the
>device tree, but otherwise leave it as-is without the extra cgu_registers
>node.
>
>  - Have your RNG device node as a child of the CGU node, which should let it
>pick up the regmap via syscon_node_to_regmap() as it already does.
>
>  - Leave the CGU driver as-is, so it can continue accessing memory directly
>rather than via regmap.
>

As per my understanding, CGU driver and syscon will map the same
register set. Is that fine?

Thanks,
PrasannaKumar

Re: [PATCH 2/6] crypto: jz4780-rng: Make ingenic CGU driver use syscon

2017-08-18 Thread Paul Burton 
Hi PrasannaKumar,

On Thursday, 17 August 2017 11:25:16 PDT PrasannaKumar Muralidharan wrote:
> Ingenic PRNG registers are a part of the same hardware block as clock
> and power stuff. It is handled by CGU driver. Ingenic M200 SoC has power
> related registers that are after the PRNG registers. So instead of
> shortening the register range use syscon interface to expose regmap.
> This regmap is used by the PRNG driver.
> 
> Signed-off-by: PrasannaKumar Muralidharan 
> ---
>  arch/mips/boot/dts/ingenic/jz4740.dtsi | 14 +++
>  arch/mips/boot/dts/ingenic/jz4780.dtsi | 14 +++
>  drivers/clk/ingenic/cgu.c  | 46
> +- drivers/clk/ingenic/cgu.h  |
>  9 +--
>  drivers/clk/ingenic/jz4740-cgu.c   | 30 +++---
>  drivers/clk/ingenic/jz4780-cgu.c   | 10 
>  6 files changed, 73 insertions(+), 50 deletions(-)
> 
> diff --git a/arch/mips/boot/dts/ingenic/jz4740.dtsi
> b/arch/mips/boot/dts/ingenic/jz4740.dtsi index 2ca7ce7..ada5e67 100644
> --- a/arch/mips/boot/dts/ingenic/jz4740.dtsi
> +++ b/arch/mips/boot/dts/ingenic/jz4740.dtsi
> @@ -34,14 +34,18 @@
>   clock-frequency = <32768>;
>   };
> 
> - cgu: jz4740-cgu@1000 {
> - compatible = "ingenic,jz4740-cgu";
> + cgu_registers {
> + compatible = "simple-mfd", "syscon";
>   reg = <0x1000 0x100>;
> 
> - clocks = <>, <>;
> - clock-names = "ext", "rtc";
> + cgu: jz4780-cgu@1000 {
> + compatible = "ingenic,jz4740-cgu";
> 
> - #clock-cells = <1>;
> + clocks = <>, <>;
> + clock-names = "ext", "rtc";
> +
> + #clock-cells = <1>;
> + };
>   };
> 
>   rtc_dev: rtc@10003000 {
> diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi
> b/arch/mips/boot/dts/ingenic/jz4780.dtsi index 4853ef6..1301694 100644
> --- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
> +++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
> @@ -34,14 +34,18 @@
>   clock-frequency = <32768>;
>   };
> 
> - cgu: jz4780-cgu@1000 {
> - compatible = "ingenic,jz4780-cgu";
> + cgu_registers {
> + compatible = "simple-mfd", "syscon";
>   reg = <0x1000 0x100>;
> 
> - clocks = <>, <>;
> - clock-names = "ext", "rtc";
> + cgu: jz4780-cgu@1000 {
> + compatible = "ingenic,jz4780-cgu";
> 
> - #clock-cells = <1>;
> + clocks = <>, <>;
> + clock-names = "ext", "rtc";
> +
> + #clock-cells = <1>;
> + };
>   };
> 
>   pinctrl: pin-controller@1001 {
> diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
> index e8248f9..2f12c7c 100644
> --- a/drivers/clk/ingenic/cgu.c
> +++ b/drivers/clk/ingenic/cgu.c
> @@ -29,6 +29,18 @@
> 
>  #define MHZ (1000 * 1000)
> 
> +unsigned int cgu_readl(struct ingenic_cgu *cgu, unsigned int reg)
> +{
> + unsigned int val = 0;
> + regmap_read(cgu->regmap, reg, );
> + return val;
> +}
> +
> +int cgu_writel(struct ingenic_cgu *cgu, unsigned int val, unsigned int reg)
> +{
> + return regmap_write(cgu->regmap, reg, val);
> +}

This is going to introduce a lot of overhead to the CGU driver - each 
regmap_read() or regmap_write() call will not only add overhead from the 
indirection but also acquire a lock which we really don't need.

Could you instead perhaps:

 - Just add "syscon" as a second compatible string to the CGU node in the
   device tree, but otherwise leave it as-is without the extra cgu_registers
   node.

 - Have your RNG device node as a child of the CGU node, which should let it
   pick up the regmap via syscon_node_to_regmap() as it already does.

 - Leave the CGU driver as-is, so it can continue accessing memory directly
   rather than via regmap.

Thanks,
Paul

> +
>  /**
>   * ingenic_cgu_gate_get() - get the value of clock gate register bit
>   * @cgu: reference to the CGU whose registers should be read
> @@ -43,7 +55,7 @@ static inline bool
>  ingenic_cgu_gate_get(struct ingenic_cgu *cgu,
>const struct ingenic_cgu_gate_info *info)
>  {
> - return readl(cgu->base + info->reg) & BIT(info->bit);
> + return cgu_readl(cgu, info->reg) & BIT(info->bit);
>  }
> 
>  /**
> @@ -60,14 +72,14 @@ static inline void
>  ingenic_cgu_gate_set(struct ingenic_cgu *cgu,
>const struct ingenic_cgu_gate_info *info, bool val)
>  {
> - u32 clkgr = readl(cgu->base + info->reg);
> + u32 clkgr = cgu_readl(cgu, info->reg);
> 
>   if (val)
>   clkgr |= BIT(info->bit);
>   else
>   clkgr &= ~BIT(info->bit);
> 
> - writel(clkgr, cgu->base + info->reg);
> + cgu_writel(cgu, clkgr, info->reg);
>  }
> 
>  /*
> @@ -91,7 +103,7 @@

[PATCH 2/6] crypto: jz4780-rng: Make ingenic CGU driver use syscon

2017-08-17 Thread PrasannaKumar Muralidharan 
Ingenic PRNG registers are a part of the same hardware block as clock
and power stuff. It is handled by CGU driver. Ingenic M200 SoC has power
related registers that are after the PRNG registers. So instead of
shortening the register range use syscon interface to expose regmap.
This regmap is used by the PRNG driver.

Signed-off-by: PrasannaKumar Muralidharan 
---
 arch/mips/boot/dts/ingenic/jz4740.dtsi | 14 +++
 arch/mips/boot/dts/ingenic/jz4780.dtsi | 14 +++
 drivers/clk/ingenic/cgu.c  | 46 +-
 drivers/clk/ingenic/cgu.h  |  9 +--
 drivers/clk/ingenic/jz4740-cgu.c   | 30 +++---
 drivers/clk/ingenic/jz4780-cgu.c   | 10 
 6 files changed, 73 insertions(+), 50 deletions(-)

diff --git a/arch/mips/boot/dts/ingenic/jz4740.dtsi 
b/arch/mips/boot/dts/ingenic/jz4740.dtsi
index 2ca7ce7..ada5e67 100644
--- a/arch/mips/boot/dts/ingenic/jz4740.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4740.dtsi
@@ -34,14 +34,18 @@
clock-frequency = <32768>;
};
 
-   cgu: jz4740-cgu@1000 {
-   compatible = "ingenic,jz4740-cgu";
+   cgu_registers {
+   compatible = "simple-mfd", "syscon";
reg = <0x1000 0x100>;
 
-   clocks = <>, <>;
-   clock-names = "ext", "rtc";
+   cgu: jz4780-cgu@1000 {
+   compatible = "ingenic,jz4740-cgu";
 
-   #clock-cells = <1>;
+   clocks = <>, <>;
+   clock-names = "ext", "rtc";
+
+   #clock-cells = <1>;
+   };
};
 
rtc_dev: rtc@10003000 {
diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi 
b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index 4853ef6..1301694 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -34,14 +34,18 @@
clock-frequency = <32768>;
};
 
-   cgu: jz4780-cgu@1000 {
-   compatible = "ingenic,jz4780-cgu";
+   cgu_registers {
+   compatible = "simple-mfd", "syscon";
reg = <0x1000 0x100>;
 
-   clocks = <>, <>;
-   clock-names = "ext", "rtc";
+   cgu: jz4780-cgu@1000 {
+   compatible = "ingenic,jz4780-cgu";
 
-   #clock-cells = <1>;
+   clocks = <>, <>;
+   clock-names = "ext", "rtc";
+
+   #clock-cells = <1>;
+   };
};
 
pinctrl: pin-controller@1001 {
diff --git a/drivers/clk/ingenic/cgu.c b/drivers/clk/ingenic/cgu.c
index e8248f9..2f12c7c 100644
--- a/drivers/clk/ingenic/cgu.c
+++ b/drivers/clk/ingenic/cgu.c
@@ -29,6 +29,18 @@
 
 #define MHZ (1000 * 1000)
 
+unsigned int cgu_readl(struct ingenic_cgu *cgu, unsigned int reg)
+{
+   unsigned int val = 0;
+   regmap_read(cgu->regmap, reg, );
+   return val;
+}
+
+int cgu_writel(struct ingenic_cgu *cgu, unsigned int val, unsigned int reg)
+{
+   return regmap_write(cgu->regmap, reg, val);
+}
+
 /**
  * ingenic_cgu_gate_get() - get the value of clock gate register bit
  * @cgu: reference to the CGU whose registers should be read
@@ -43,7 +55,7 @@ static inline bool
 ingenic_cgu_gate_get(struct ingenic_cgu *cgu,
 const struct ingenic_cgu_gate_info *info)
 {
-   return readl(cgu->base + info->reg) & BIT(info->bit);
+   return cgu_readl(cgu, info->reg) & BIT(info->bit);
 }
 
 /**
@@ -60,14 +72,14 @@ static inline void
 ingenic_cgu_gate_set(struct ingenic_cgu *cgu,
 const struct ingenic_cgu_gate_info *info, bool val)
 {
-   u32 clkgr = readl(cgu->base + info->reg);
+   u32 clkgr = cgu_readl(cgu, info->reg);
 
if (val)
clkgr |= BIT(info->bit);
else
clkgr &= ~BIT(info->bit);
 
-   writel(clkgr, cgu->base + info->reg);
+   cgu_writel(cgu, clkgr, info->reg);
 }
 
 /*
@@ -91,7 +103,7 @@ ingenic_pll_recalc_rate(struct clk_hw *hw, unsigned long 
parent_rate)
pll_info = _info->pll;
 
spin_lock_irqsave(>lock, flags);
-   ctl = readl(cgu->base + pll_info->reg);
+   ctl = cgu_readl(cgu, pll_info->reg);
spin_unlock_irqrestore(>lock, flags);
 
m = (ctl >> pll_info->m_shift) & GENMASK(pll_info->m_bits - 1, 0);
@@ -190,7 +202,7 @@ ingenic_pll_set_rate(struct clk_hw *hw, unsigned long 
req_rate,
clk_info->name, req_rate, rate);
 
spin_lock_irqsave(>lock, flags);
-   ctl = readl(cgu->base + pll_info->reg);
+   ctl = cgu_readl(cgu, pll_info->reg);
 
ctl &= ~(GENMASK(pll_info->m_bits - 1, 0) << pll_info->m_shift);
ctl |= (m - pll_info->m_offset) << pll_info->m_shift;
@@ -204,11 +216,11 @@ ingenic_pll_set_rate(struct clk_hw *hw, unsigned long 
req_rate,
ctl &= ~BIT(pll_info->bypass_bit);