[v3,3/4] crypto: jz4780-rng: Add RNG node to jz4780.dtsi
diff mbox

Message ID 20170918140241.24003-4-prasannatsmkumar@gmail.com
State Deferred
Delegated to: Herbert Xu
Headers show

Commit Message

PrasannaKumar Muralidharan Sept. 18, 2017, 2:02 p.m. UTC
Add RNG node to jz4780 dtsi. This driver uses registers that are part of
the register set used by Ingenic CGU driver. Use regmap in RNG driver to
access its register. Create 'simple-bus' node, make CGU and RNG node as
child of it so that both the nodes are visible without changing CGU
driver code.

Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>
---
Changes in v3:
* Create a cgublock node with "simple-bus" compatible
* Make CGU and RNG node as children of cgublock node.

Changes in v2:                                                                   
* Add "syscon" in CGU node's compatible section                                  
* Make RNG child node of CGU.                                                    

 arch/mips/boot/dts/ingenic/jz4780.dtsi | 25 ++++++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

Comments

James Hogan March 6, 2018, 9:32 a.m. UTC | #1
On Mon, Sep 18, 2017 at 07:32:40PM +0530, PrasannaKumar Muralidharan wrote:
> Add RNG node to jz4780 dtsi. This driver uses registers that are part of
> the register set used by Ingenic CGU driver. Use regmap in RNG driver to
> access its register. Create 'simple-bus' node, make CGU and RNG node as
> child of it so that both the nodes are visible without changing CGU
> driver code.
> 
> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>

Better late than never:
Acked-by: James Hogan <jhogan@kernel.org>

(I presume its okay for the reg ranges to overlap, ISTR that being an
issue a few years ago, but maybe thats fixed now).

Cheers
James
Rob Herring March 6, 2018, 1:55 p.m. UTC | #2
On Tue, Mar 6, 2018 at 3:32 AM, James Hogan <jhogan@kernel.org> wrote:
> On Mon, Sep 18, 2017 at 07:32:40PM +0530, PrasannaKumar Muralidharan wrote:
>> Add RNG node to jz4780 dtsi. This driver uses registers that are part of
>> the register set used by Ingenic CGU driver. Use regmap in RNG driver to
>> access its register. Create 'simple-bus' node, make CGU and RNG node as
>> child of it so that both the nodes are visible without changing CGU
>> driver code.

The goal should be to avoid changing the DT (because the h/w hasn't
changed), not avoid changing a driver.

>>
>> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>
>
> Better late than never:
> Acked-by: James Hogan <jhogan@kernel.org>
>
> (I presume its okay for the reg ranges to overlap, ISTR that being an
> issue a few years ago, but maybe thats fixed now).

No, that should be avoided. It does work because we have existing
cases that have to be supported.

>
> Cheers
> James
Paul Cercueil March 6, 2018, 11:01 p.m. UTC | #3
Le 2018-03-06 10:32, James Hogan a écrit :
> On Mon, Sep 18, 2017 at 07:32:40PM +0530, PrasannaKumar Muralidharan 
> wrote:
>> Add RNG node to jz4780 dtsi. This driver uses registers that are part 
>> of
>> the register set used by Ingenic CGU driver. Use regmap in RNG driver 
>> to
>> access its register. Create 'simple-bus' node, make CGU and RNG node 
>> as
>> child of it so that both the nodes are visible without changing CGU
>> driver code.
>> 
>> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>
> 
> Better late than never:
> Acked-by: James Hogan <jhogan@kernel.org>
> 
> (I presume its okay for the reg ranges to overlap, ISTR that being an
> issue a few years ago, but maybe thats fixed now).
> 
> Cheers
> James

What bothers me is that the CGU code has not been modified to use 
regmap, so the
registers area is actually mapped twice (once in the CGU driver, once 
with regmap).

Besides, regmap would be useful if the RNG registers were actually 
located in the
middle of the register area used by the CGU driver, which is not the 
case here.
The CGU block does have some registers after the RNG ones on the X1000 
SoC, but
I don't think they will ever be used (and if they are it won't be by the 
CGU driver).

Regards,
-Paul
PrasannaKumar Muralidharan March 7, 2018, 2:51 p.m. UTC | #4
Hi Paul,

On 7 March 2018 at 04:31, Paul Cercueil <paul@crapouillou.net> wrote:
> Le 2018-03-06 10:32, James Hogan a écrit :
>>
>> On Mon, Sep 18, 2017 at 07:32:40PM +0530, PrasannaKumar Muralidharan
>> wrote:
>>>
>>> Add RNG node to jz4780 dtsi. This driver uses registers that are part of
>>> the register set used by Ingenic CGU driver. Use regmap in RNG driver to
>>> access its register. Create 'simple-bus' node, make CGU and RNG node as
>>> child of it so that both the nodes are visible without changing CGU
>>> driver code.
>>>
>>> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>
>>
>>
>> Better late than never:
>> Acked-by: James Hogan <jhogan@kernel.org>
>>
>> (I presume its okay for the reg ranges to overlap, ISTR that being an
>> issue a few years ago, but maybe thats fixed now).
>>
>> Cheers
>> James
>
>
> What bothers me is that the CGU code has not been modified to use regmap, so
> the
> registers area is actually mapped twice (once in the CGU driver, once with
> regmap).

One of my previous versions changed CGU code to use regmap. I got a
review comment saying that is not required
(https://patchwork.kernel.org/patch/9906889/). The points in the
comment were valid so I reverted the change. Please have a look at the
discussion.

> Besides, regmap would be useful if the RNG registers were actually located
> in the
> middle of the register area used by the CGU driver, which is not the case
> here.
> The CGU block does have some registers after the RNG ones on the X1000 SoC,
> but
> I don't think they will ever be used (and if they are it won't be by the CGU
> driver).
>
> Regards,
> -Paul

Ingenic M200 SoC's CGU has clock and power related registers after the
RNG registers. Paul Burton suggested using regmap to expose registers
to CGU and RNG drivers
(https://patchwork.linux-mips.org/patch/14094/).

Regards,
PrasannaKumar
PrasannaKumar Muralidharan March 7, 2018, 2:54 p.m. UTC | #5
Hi Rob,

On 6 March 2018 at 19:25, Rob Herring <robh+dt@kernel.org> wrote:
> On Tue, Mar 6, 2018 at 3:32 AM, James Hogan <jhogan@kernel.org> wrote:
>> On Mon, Sep 18, 2017 at 07:32:40PM +0530, PrasannaKumar Muralidharan wrote:
>>> Add RNG node to jz4780 dtsi. This driver uses registers that are part of
>>> the register set used by Ingenic CGU driver. Use regmap in RNG driver to
>>> access its register. Create 'simple-bus' node, make CGU and RNG node as
>>> child of it so that both the nodes are visible without changing CGU
>>> driver code.
>
> The goal should be to avoid changing the DT (because the h/w hasn't
> changed), not avoid changing a driver.

Please have a look at the discussion happened at
https://patchwork.linux-mips.org/patch/14094/. Looks like there is a
difference in though process between you and mips folks. I am not an
expert in DT so please suggest me the correct way to go about this.

>>>
>>> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>
>>
>> Better late than never:
>> Acked-by: James Hogan <jhogan@kernel.org>
>>
>> (I presume its okay for the reg ranges to overlap, ISTR that being an
>> issue a few years ago, but maybe thats fixed now).
>
> No, that should be avoided. It does work because we have existing
> cases that have to be supported.

I am sorry but I require guidance here. Do you have any suggestion on
how this should be.

>>
>> Cheers
>> James

Thanks and regards,
PrasannaKumar
Paul Cercueil March 7, 2018, 8:23 p.m. UTC | #6
Hi PrasannaKumar,

Le 2018-03-07 15:51, PrasannaKumar Muralidharan a écrit :
> Hi Paul,
> 
> On 7 March 2018 at 04:31, Paul Cercueil <paul@crapouillou.net> wrote:
>> Le 2018-03-06 10:32, James Hogan a écrit :
>>> 
>>> On Mon, Sep 18, 2017 at 07:32:40PM +0530, PrasannaKumar Muralidharan
>>> wrote:
>>>> 
>>>> Add RNG node to jz4780 dtsi. This driver uses registers that are 
>>>> part of
>>>> the register set used by Ingenic CGU driver. Use regmap in RNG 
>>>> driver to
>>>> access its register. Create 'simple-bus' node, make CGU and RNG node 
>>>> as
>>>> child of it so that both the nodes are visible without changing CGU
>>>> driver code.
>>>> 
>>>> Signed-off-by: PrasannaKumar Muralidharan 
>>>> <prasannatsmkumar@gmail.com>
>>> 
>>> 
>>> Better late than never:
>>> Acked-by: James Hogan <jhogan@kernel.org>
>>> 
>>> (I presume its okay for the reg ranges to overlap, ISTR that being an
>>> issue a few years ago, but maybe thats fixed now).
>>> 
>>> Cheers
>>> James
>> 
>> 
>> What bothers me is that the CGU code has not been modified to use 
>> regmap, so
>> the
>> registers area is actually mapped twice (once in the CGU driver, once 
>> with
>> regmap).
> 
> One of my previous versions changed CGU code to use regmap. I got a
> review comment saying that is not required
> (https://patchwork.kernel.org/patch/9906889/). The points in the
> comment were valid so I reverted the change. Please have a look at the
> discussion.

I don't know, the point of regmap is for when a register area is shared. 
It
does not make sense to me to have one driver use regmap and not the 
other one.

>> Besides, regmap would be useful if the RNG registers were actually 
>> located
>> in the
>> middle of the register area used by the CGU driver, which is not the 
>> case
>> here.
>> The CGU block does have some registers after the RNG ones on the X1000 
>> SoC,
>> but
>> I don't think they will ever be used (and if they are it won't be by 
>> the CGU
>> driver).
>> 
>> Regards,
>> -Paul
> 
> Ingenic M200 SoC's CGU has clock and power related registers after the
> RNG registers. Paul Burton suggested using regmap to expose registers
> to CGU and RNG drivers
> (https://patchwork.linux-mips.org/patch/14094/).

Where can I find the M200 programming manual? The M200's CGU might have 
some
registers located after the RNG ones, but that does not mean that they 
will
be used by the clocks driver.

Thanks,
-Paul
Rob Herring March 7, 2018, 8:25 p.m. UTC | #7
On Wed, Mar 7, 2018 at 8:54 AM, PrasannaKumar Muralidharan
<prasannatsmkumar@gmail.com> wrote:
> Hi Rob,
>
> On 6 March 2018 at 19:25, Rob Herring <robh+dt@kernel.org> wrote:
>> On Tue, Mar 6, 2018 at 3:32 AM, James Hogan <jhogan@kernel.org> wrote:
>>> On Mon, Sep 18, 2017 at 07:32:40PM +0530, PrasannaKumar Muralidharan wrote:
>>>> Add RNG node to jz4780 dtsi. This driver uses registers that are part of
>>>> the register set used by Ingenic CGU driver. Use regmap in RNG driver to
>>>> access its register. Create 'simple-bus' node, make CGU and RNG node as
>>>> child of it so that both the nodes are visible without changing CGU
>>>> driver code.
>>
>> The goal should be to avoid changing the DT (because the h/w hasn't
>> changed), not avoid changing a driver.
>
> Please have a look at the discussion happened at
> https://patchwork.linux-mips.org/patch/14094/. Looks like there is a
> difference in though process between you and mips folks. I am not an
> expert in DT so please suggest me the correct way to go about this.

Those comments are correct too.

Simply, there is no reason you have to add a rng node to add an RNG
driver. The driver that probes the existing node can register both
clocks and as an RNG provider.

Rob

Patch
diff mbox

diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index 4853ef6..5953b97 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -34,14 +34,29 @@ 
 		clock-frequency = <32768>;
 	};
 
-	cgu: jz4780-cgu@10000000 {
-		compatible = "ingenic,jz4780-cgu";
+	cgublock {
+		compatible = "simple-bus";
+
+		#address-cells = <1>;
+		#size-cells = <1>;
+
 		reg = <0x10000000 0x100>;
+		ranges;
 
-		clocks = <&ext>, <&rtc>;
-		clock-names = "ext", "rtc";
+		cgu: jz4780-cgu@0 {
+			compatible = "ingenic,jz4780-cgu";
+			reg = <0x10000000 0x100>;
 
-		#clock-cells = <1>;
+			clocks = <&ext>, <&rtc>;
+			clock-names = "ext", "rtc";
+
+			#clock-cells = <1>;
+		};
+
+		rng: rng@d8 {
+			compatible = "ingenic,jz4780-rng";
+			reg = <0x100000d8 0x8>;
+		};
 	};
 
 	pinctrl: pin-controller@10010000 {