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

[Marek Vasut]

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 ?

>> 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 ?

>> 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.

Fine

>>> 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.

>>> 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.

Fine

>>> 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.

>> 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 ?

-- 
Best regards,
Marek Vasut