[U-Boot] [PATCH v7 06/35] musb: sunxi: Add OTG device clkgate and reset for H3/H5

[Maxime Ripard]

On Mon, May 07, 2018 at 05:32:34PM +0200, Marek Vasut wrote:
> On 05/07/2018 04:52 PM, Maxime Ripard wrote:
> > On Mon, May 07, 2018 at 01:47:43PM +0200, Marek Vasut wrote:
> >> On 05/07/2018 09:33 AM, Jagan Teki wrote:
> >>> Add OTG device clkgate and reset for H3/H5 through driver_data.
> >>>
> >>> Signed-off-by: Jagan Teki <jagan at amarulasolutions.com>
> >>
> >> Why don't you implement a clock driver for this SoC instead ?
> > 
> > Aren't you asking a bit too much?
> 
> I am not asking for anything, this is a question, not a request.

My bad then, this definitely sounded like a request to me.

> I asked why not implement a clock driver and use it just like any other
> civilized modern driver would instead of digging in the clock controller
> registers from a USB framework driver (which is icky).

From an absolute point of view, I agree. But we are where we are.

> I think that if we are doing some sort of conversion, we should do it
> completely and properly instead of leaving in hacks like this. A clock
> driver which allows enabling/disabling clock is probably like what, 2
> hour work ? So maybe it's worth investing that time up front to save
> maintenance burden in the future.

This is definitely not a 2 hours job. More like 2 weeks if you want to
do it properly, and by which I mean creating the clock driver,
converting all the users to it, and then making sure you don't have
any regressions.

This is on our radar, but this won't happen overnight.

> > Since the first post of these patches, you've asked to rework in a
> > significant manner the driver already, including doing a new PHY
> > driver to use the device model, and making other substantial changes
> > to it.
> 
> Well yes, because it was crap at the beginning and I don't want to see
> the crap accumulating. It has become much better since, as you can see I
> only had a few minor comments.

And that's totally your role, but at the same time, the point of this
series is not to fix the whole world, but rather add support for one
particular SoC that is using pretty much the same design than any of
our other SoCs' USB phy before. And here we are, 35 patches and
counting.

> > Jagan complied to all your requests so far, but this one is going to
> > create yet another ton of patches on top of an (already) 35 patches
> > series. And this request comes out of nowhere at the 7th version.
> 
> I disagree, one clock driver patch and a tweak to the series, unless I
> missed something obvious.

This won't be one clock driver patch. Seriously.

> > Creating a new clock driver will take a lot of effort, and this really
> > surprise me given that we've had strictly no feedback from you on this
> > considering all the previous SoCs bringups we've done so far.
> 
> What do you mean by "this" ? I think i did review the previous
> iterations of this series ? If not, was I on CC ?

You did, and thanks a lot for that. The only thing I'm noting is that
it's the first time you're being so picky about a series. I appreciate
that you have to draw the line somewhere, and when things you want in
your subsystem aren't moving as fast as you'd like them to be you have
to enforce new rules. But if you were unhappy about something, you
never told us, which doesn't seem like a good path forward
either. Even in your previous reviews of that particular series.

> I have to admit, I don't really care about the rest of the Allwinner SoC
> code or what you do there, I only care about the USB part and this
> poking of clock controller registers seems wrong in a DM/DT driver.

And I do agree on that. But we also have some history to carry.

> I also don't mind if the clock driver comes later, but I would like to
> see it happen at some point (soon) to remove this register poking.

Awesome then :)

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com