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

[JeffyChen <jeffy.chen-TNX95d0MmH7DzftRWevZcw@public.gmane.org>]

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.

>
>>
>> Rob