Hi Paul,
Thanks for your review.
On 19 August 2017 at 02:18, Paul Burton <paul.burton@xxxxxxxxxx> 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 <prasannatsmkumar@xxxxxxxxx>
>> ---
>> 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@10000000 {
>> - compatible = "ingenic,jz4740-cgu";
>> + cgu_registers {
>> + compatible = "simple-mfd", "syscon";
>> reg = <0x10000000 0x100>;
>>
>> - clocks = <&ext>, <&rtc>;
>> - clock-names = "ext", "rtc";
>> + cgu: jz4780-cgu@10000000 {
>> + compatible = "ingenic,jz4740-cgu";
>>
>> - #clock-cells = <1>;
>> + clocks = <&ext>, <&rtc>;
>> + 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@10000000 {
>> - compatible = "ingenic,jz4780-cgu";
>> + cgu_registers {
>> + compatible = "simple-mfd", "syscon";
>> reg = <0x10000000 0x100>;
>>
>> - clocks = <&ext>, <&rtc>;
>> - clock-names = "ext", "rtc";
>> + cgu: jz4780-cgu@10000000 {
>> + compatible = "ingenic,jz4780-cgu";
>>
>> - #clock-cells = <1>;
>> + clocks = <&ext>, <&rtc>;
>> + clock-names = "ext", "rtc";
>> +
>> + #clock-cells = <1>;
>> + };
>> };
>>
>> pinctrl: pin-controller@10010000 {
>> 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, &val);
>> + 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
