[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 10:55:16PM +0200, Marek Vasut wrote:
> On 05/07/2018 10:11 PM, Maxime Ripard wrote:
> > 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.
> 
> So uh, how do I make this NOT sound like a request to you ?
> Can you phrase it for me ?

You are in a situation of power here. Asking the exact same question
when you are the one in power or is a peer doesn't have the same
impact, unless you make it clear that it is an actual question and not
some way to have it fixed.

Something like "Would switching to a proper clock driver be an
option?" for example would have carried the message better imo, if
this was a genuine question and not a request.

> >> 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.
> 
> Which is where exactly ?

Having to deal with code from 2012 everywhere.

> >>> 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.
> 
> If I said "yes" to every single patch adding just a minor additional bit
> of crap to the codebase, we'd be in the state in which we were in 2012,
> sinking under the boatload of ifdeffery and ad-hoc solutions. So I think
> some push is needed to avoid that situation.

I don't have any issue with the end goal, and your willingness to have
the code ported over to new APIs. But if from one day to another every
maintainer goes like this, this will simply not fly. This is not just
about having just a simple clock driver, but also a pinctrl one, and
converting all the consumer drivers to the device model, oh, and btw,
the DM doesn't fit in the SPL anymore, so we would probably need to do
an SPL driver as well. Probably with some painful Kconfig conversions
all over the tree even.

This is no longer a simple request, but some huge spaghetti changes
that need to be done, mostly by volunteers. And at some point, it just
becomes easier to give up, fork, and just maintain our stuff like we
were doing before. Or just stop maintaining it entirely. And I'm not
sure either situation is something we want.

tl; dr: I'd like some moderation.

> >>> 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.
> 
> Er, no, I am always picky and hard.
> 
> > 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 think I pointed out pretty much all of it ? If I missed something,
> it's because it was hidden and didn't surface until the patchset got
> into some better shape.

And yet, this is the first time you bring up the phy and clock
support.

> >> 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 :)
> 
> Is this going to happen at some point ?

At some point, yes, but I can't give you a deadline.

Maxime

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