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

[Philipp Zabel]

Hi Martin,

On Mon, 2019-10-07 at 21:53 +0200, Martin Blumenstingl wrote:
> Hi Philipp,
> 
> On Thu, Oct 3, 2019 at 4:19 PM Philipp Zabel <pza@xxxxxxxxxxxxxx> wrote:
> [...]
> > > 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.
> > 
> > Just to be sure, you are suggesting to add support for the current
> > lantiq,reset binding to the reset-intel driver at a later point? I
> > see no reason not to do that, but I'm also not quite sure what the
> > benefit will be over just keeping reset-lantiq as is?
> 
> according to Chuan and Dilip the current reset-lantiq implementation
> is wrong [0].

The only issue seems to be the .reset callback, which doesn't have any
users anway.

> my understanding is that the Lantiq and Intel LGM reset controllers
> are identical except:
> - the Lantiq variant uses a weird register layout (reset and status
> registers not at consecutive offsets)
> - the bits of the reset and status registers sometimes don't match on
> the Lantiq variant

Thank you, so these are a good explanation for why the DT bindings
should be different.

> - the Intel variant has a dedicated registers area for the reset
> controller registers, while the Lantiq variant mixes them with various
> other functionality (for example: USB2 PHYs)

I'm not quite sure I understand why the intel driver is using syscon,
then. Either way, it shouldn't make a big difference if regmap is used
anyway. 

> > > 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).
> > 
> > More work than what alternative?
> 
> compared to "fixing" the existing reset-lantiq driver (reset callback)

That is still something you could do,Âor just drop the .reset callback
because there are no reset consumers using it anyway.

One correct thing to do would be to identify those self-clearing reset
bits and to disallow calling assert/deassert on them.

> and then (instead of adding a new driver) integrating Intel LGM
> support into reset-lantiq

Since at this point I'm not even sure whether merging the two at all is
better than keeping them separate, I have no opinion on whether merging
intel support into the lantiq driver or the other way around is
preferable.

> > > 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.
> > 
> > Reset drivers are typically so simple, I'm not quite sure whether it is
> > worth to integrate multiple drivers if it complicates matters too much.
> > In this case though I expect it would just be adding support for a
> > custom .of_xlate and lantiq specific register property parsing?
> 
> yes, that's how I understand the Lantiq and Intel reset controllers:
> - reset/status/assert/deassert callbacks would be shared across all variants
> - register parsing and of_xlate are SoC specific

Ok. If that turns out to be less rather than more boilerplate than two
separate drivers, that should be fine.

regards
Philipp