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

[Robin Murphy <robin.murphy@arm.com>]

On 23/02/18 10:24, JeffyChen wrote:
> Hi guys,
> 
> On 02/01/2018 07:19 PM, JeffyChen wrote:
>>>>>>
>>>>>>
>>>>>> diff --git
>>>>>> a/Documentation/devicetree/bindings/iommu/rockchip,iommu.txt
>>>>>> b/Documentation/devicetree/bindings/iommu/rockchip,iommu.txt
>>>>>> index 2098f7732264..33dd853359fa 100644
>>>>>> --- a/Documentation/devicetree/bindings/iommu/rockchip,iommu.txt
>>>>>> +++ b/Documentation/devicetree/bindings/iommu/rockchip,iommu.txt
>>>>>> @@ -14,6 +14,13 @@ Required properties:
>>>>>>                       "single-master" device, and needs no
>>>>>> additional information
>>>>>>                       to associate with its master device.  See:
>>>>>>
>>>>>> Documentation/devicetree/bindings/iommu/iommu.txt
>>>>>> +Optional properties:
>>>>>> +- clocks : A list of master clocks requires for the IOMMU to be
>>>>>> accessible
>>>>>> +           by the host CPU. The number of clocks depends on the
>>>>>> master
>>>>>> +           block and might as well be zero. See [1] for generic 
>>>>>> clock
>>>>>> +           bindings description.
>>>>>
>>>>> Hardware blocks don't have a variable number of clock connections.
>>>>
>>>> I think you underestimate the imagination of hardware designers. :)
>>>
>>> Learned long ago to never do that. If there are 2 ways to do
>>> something, they will find a 3rd way.
>>>
>>>> For Rockchip IOMMU, there is a set of clocks, which all need to be
>>>> enabled for IOMMU register access to succeed. The clocks are not
>>>> directly fed to the IOMMU, but they are needed for the various buses
>>>> and intermediate blocks on the way to the IOMMU to work.
>>>
>>> The binding should describe the clock connections, not what clocks a
>>> driver needs (currently). It sounds like a lack of managing bus clocks
>>> to me.
>>>
>>> In any case, the binding must be written so it can be verified. If you
>>> can have any number of clocks with any names, there's no point in
>>> documenting.
>>
>> 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.

Robin.