Re: [PATCH v4 5/5] usb: dwc3: rockchip: add devicetree bindings documentation

[William Wu]

Dear Heiko,

On 06/29/2016 12:41 AM, Heiko Stuebner wrote:
> Hi William,
>
> Am Dienstag, 28. Juni 2016, 11:18:04 schrieb William Wu:
> > > > So about the usb3 controller clk management, I think it should contain
> > > > the following clk:
> > > > 1.  aclk_usb3otg1
> > > > 2.  aclk_usb3otg0
> > > > 3.  aclk_usb3_grf
> > > correct, aclk_usb3otgX would then be the busclk for each controller if
> > > I'm not mistaken and the grf clock should also get enabled, like we
> > > also plan on doing for the vio_grf clock in the display area.
> > OK, so  aclk_usb3_grf should be marked as critical, right?
> nope ... the consensus for the vio_grf clock was that the driver accessing
> it should control it. So for the usb3_grf clock this would be your dwc3-
> based driver being responsible for the grf-clock.
Ah, I misunderstand before. I'll control usb3_grf in dwc3 driver.
> > I found that most of the grf clocks haven't been marked as critical,
> > apart from vio_grf. So may I keep the aclk_usb3_grf in usb3 dts, and
> > remove it after clock manager adds it to critical clocks?
> you should keep the usb3-grf clock in the dts all the time.
OK, I'll keep it in the dts.
> > > > 4.  aclk_usb3_noc
> > > > For "aclk_usb3_noc", I discuss with our clk manager, the clk is always
> > > > on before,
> > > > but according to upstream maintainer's suggestion, we need to manage
> > > > the
> > > > noc clk by each module.
> > > can you point me to this discussion? The bus-interconnect is a very
> > > separate component, which we don't model so far and thus we have all
> > > the noc clocks simply marked as critical.
> > >
> > > As this clock doesn't belong to the actual usb controller block, but
> > > said
> > > separate component, handling it in the controller seems somehow wrong to
> > > me.
> > >
> > > So my (current) opinion would again be to mark the noc clock as critical
> > > for the time being.
> > Sorry, it seems that I get the new information about clk management too
> > late.:-)
> >
> > There's no dedicated discussion about noc clk, but similar to grf clock,
> > please refer to "https://patchwork.kernel.org/patch/9171467/"; for add
> > pclk_vio_grf to critical clock on the RK3399, and you have agreed on
> > that mark vio grf clk as critical. So I agree with your opinion, thanks~
> The difference as I see it is, that the GRF parts are _part_ of the usb3
> controller, while the interconnect really is a separate component,
> connecting the controller to the rest of the system.
>
> ----------	
> |  USB3  |----[Interconnect]----[rest of the system]
> | [+GRF] |	
> ----------	
>
> So the controller binding + driver should handle all clocks it needs to
> operate. We currently don't model and handle the separate interconnect
> though, so that noc clock should be critical for the time being.
Thanks for your professional explanation. Now I think I have
more clearly understood the clocks. And I'll upload a new
patch later.
> Heiko