Re: [PATCH v2 2/2] reset: Reset controller driver for Intel LGM SoC

[Martin Blumenstingl]

Hi Dilip,

(sorry for the late reply)

On Thu, Sep 12, 2019 at 8:38 AM Dilip Kota <eswara.kota@xxxxxxxxxxxxxxx> wrote:
[...]
> The major difference between the vrx200 and lgm is:
> 1.) RCU in vrx200 is having multiple register regions wheres RCU in lgm
> has one single register region.
> 2.) Register offsets and bit offsets are different.
>
> So enhancing the intel-reset-syscon.c to provide compatibility/support
> for vrx200.
> Please check the below dtsi binding proposal and let me know your view.
>
> rcu0:reset-controller@00000000 {
>      compatible= "intel,rcu-lgm";
>      reg = <0x0000000 0x80000>, <reg_set2 size>, <reg_set3 size>,
> <reg_set4 size>;
I'm not sure that I understand what are reg_set2/3/4 for
the first resource (0x80000 at 0x0) already covers the whole LGM RCU,
so what is the purpose of the other register resources

>     intel,global-reset = <0x10 30>;
>     #reset-cells = <3>;
> };
>
> "#reset-cells":
>    const:3
>    description: |
>      The 1st cell is the reset register offset.
>      The 2nd cell is the reset set bit offset.
>      The 3rd cell is the reset status bit offset.
I think this will work fine for VRX200 (and even older SoCs)
as you have described in your previous emails we can determine the
status offset from the reset offset using a simple if/else

for LGM I like your initial suggestion with #reset-cells = <2> because
it's easier to read and write.

> Reset driver takes care of parsing the register address "reg" as per the
> ".data" structure in struct of_device_id.
> Reset driver takes care of traversing the status register offset.
the differentiation between two and three #reset-cells can also happen
based on the struct of_device_id:
- the LGM implementation would simply also use the reset bit as status
bit (only two cells are needed)
- the implementation for earlier SoCs would parse the third cell and
use that as status bit

Philipp, can you please share your opinion on how to move forward with
the reset-intel driver from this series?
The reset_control_ops from the reset-intel driver are (in my opinion)
a bug-fixed and improved version of what we already have in
drivers/reset/reset-lantiq.c. The driver is NOT simply copy and paste
because the register layout was greatly simplified for the newer SoCs
(for which there is reset-intel) compared to the older ones
(reset-lantiq).
Dilip's suggestion (in my own words) is that you take his new
reset-intel driver, then we will work on porting reset-lantiq over to
that so in the end we can drop the reset-lantiq driver. This approach
means more work for me (as I am probably the one who then has to do
the work to port reset-lantiq over to reset-intel). I'm happy to do
that work if you think that it's worth following this approach.
So I want your opinion on this before I spend any effort on porting
reset-lantiq over to reset-intel.


Martin