Re: [PATCH v5 08/13] iommu/rockchip: Control clocks needed to access the IOMMU

[JeffyChen]

Hi Robin,

Thanks for your reply.

On 02/28/2018 11:06 PM, Robin Murphy wrote:
> On 28/02/18 13:00, JeffyChen wrote:
> > Hi Robin,
> >
> > Thanks for your reply.
> >
> > On 02/28/2018 12:59 AM, Robin Murphy wrote:
> > > > > the rockchip IOMMU is part of the master block in hardware, so it
> > > > > needs
> > > > > to control the master's power domain and some of the master's clocks
> > > > > when access it's registers.
> > > > >
> > > > > and the number of clocks needed here, might be different between each
> > > > > IOMMUs(according to which master block it belongs), it's a little like
> > > > > our power domain:
> > > > > https://elixir.free-electrons.com/linux/latest/source/arch/arm64/boot/dts/rockchip/rk3399.dtsi#L935
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > i'm not sure how to describe this correctly, is it ok use something
> > > > > like
> > > > > "the same as it's master block"?
> > > >
> > > > would it make sense to add a property to specify the master who owns
> > > > the iommu, and we can get all clocks(only some of those clocks are
> > > > actually needed) from it in the of_xlate()? and we can also reuse the
> > > > clock-names of that master to build clk_bulk_data and log errors in
> > > > clk_bulk_get.
> > >
> > > I'm inclined to agree with Rob here - if we're to add anything to the
> > > binding, it should only be whatever clock inputs are defined for the
> > > IOMMU IP block itself. If Linux doesn't properly handle the interconnect
> > > clock hierarchy external to a particular integration, that's a separate
> > > issue and it's not the binding's problem.
> > >
> > > I actually quite like the hack of "borrowing" the clocks from
> > > dev->of_node in of_xlate() - you shouldn't need any DT changes for that,
> > > because you already know that each IOMMU instance only has the one
> > > master device anyway.
> >
> > Thanks:) but actually we are going to support sharing IOMMU between
> > multiple masters(one of them is the main master i think) in the newer
> > chips(not yet supported on upstream kernel)...
>
> Ha! OK, fair enough, back to the first point then...
>
> > So we might have to get all clocks from all masters, or find a way to
> > specify the main master...and for the multiple masters case, do it in
> > of_xlate() turns out to be a little racy...maybe we can add a property
> > to specify main master, and get it's clocks in probe()?
>
> I notice that the 4.4 BSP kernel consistently specifies "aclk" and
> "hclk" for the IOMMU instances - it feels unusual to say "why don't we
> follow the downstream binding?", but it does look a lot like what I
> would expect (I'd guess at one for the register slave interface and one
> for the master interface/general operation?)
huh, right.
i did noticed that, but there's a hevc_mmu with ("aclk", "hclk", 
"clk_core", "clk_cabac") confused me.

so confirmed with Simon, that hevc_mmu is wrong. currently all IOMMUs 
should only have 2 clks, either aclk+hclk or aclk+pclk (depends on the 
clk tree)

so it seems to be a good idea to do so, will send patches soon, thanks :)
> If we can implement conceptually-correct clock handling based on an
> accurate binding, which should cover most cases, and *then* look at
> hacking around those where it doesn't quite work in practice due to
> shortcomings elsewhere, that would be ideal, and of course a lot nicer
> than just jumping straight into piles of hacks.
>
> Robin.