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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
