[PATCH V2 1/3] iommu/arm-smmu: Add pm_runtime/sleep ops

[Robin Murphy]

On 08/02/17 12:30, Sricharan wrote:
> Hi Mark,
> 
>> On Wed, Feb 08, 2017 at 04:23:17PM +0530, Sricharan wrote:
>>>> On Thu, Feb 02, 2017 at 10:40:18PM +0530, Sricharan R wrote:
>>>>> +- clock-names:    Should be a pair of "smmu_iface_clk" and "smmu_bus_clk"
>>>>> +                  required for smmu's register group access and interface
>>>>> +                  clk for the smmu's underlying bus access.
>>>>> +
>>>>> +- clocks:         Phandles for respective clocks described by clock-names.
>>>>
>>>> Which SMMU implementations are those clock-names valid for?
>>>>
>>>> The SMMU architecture specifications do not architect the clocks, which
>>>> are implemementation-specific.
>>>>
>>>> AFAICT, this doesn't match MMU-400 or MMU-500.
>>>
>>> Ok, should be more specific. Infact QCOM has MMU-500 and also
>>> a smmu v2 implementation which is fully compatible with
>>> "arm,smmu-v2", with the clocks being controlled by the soc's
>>> clock controller. i was trying to define these clock bindings
>>> so that its works across socs.
>>
>> I don't think we can do that, if we don't know precisely what those
>> clocks are used for.
>>
>> i.e. we'd need a compatible string for the QCOM SMMUv2 variant, which
>> would imply the set of clocks.
>>
> 
> Ok, this was what i was trying to do for V3 and will actually put it
> this way.

Clocks are not architectural, so it only makes sense to associate them
with an implementation-specific compatible string. There's also no
guarantee that different microarchitectures have equivalent internal
clock domains - I'm not sure if "the SMMU's underlying bus access" is
meant to refer to accesses *by* the SMMU, i.e. page table walks,
accesses *through* the SMMU by upstream masters, or both, and the
differences are rather significant. I'd also note that an MMU-500
configuration may have up to *33* clocks.

>>> So there are one or more interface clocks which are required for the
>>> smmu's interface or the configuration access and one or more clocks
>>> required for smmu's downstream bus access. That was the reason i was
>>> trying to iterate over the list of clocks down below.  But agree that
>>> the bindings should define each of the clocks required separately.
>>
>> As above, I don't think the code should do this. It should only touch
>> the clocks it knows about.
>>
> 
> ok, after defining QCOM specific SMMU bindings, this would be become 
> handling clocks specific to QCOM implementation as per its clock
> bindings, which as i understand is what you suggest.
> 
>>> So one way here is, define a separate compatible for QCOM's SMMU
>>> implementation and define all the clock bindings as a part of it
>>> and handle it in the same way in the driver.
>>
>> That would be my preference.
>>
> 
> ok.

Either way, the QCOM implementation deserves its own compatible if only
for the sake of the imp-def gaps in the architecture (e.g. FSR.SS
behaviour WRT to IRQs as touched upon in the other thread).

Robin.

>>> But just thinking if it would scale well for any other soc that is
>>> compatible with arm,smmu-v2 driver and wants to handle clocks in the
>>> future ?
>>
>> I don't think we can have our cake and eat it here. Either we handle the
>> clock management for each variant, or we don't do it at all. We have no
>> idea what requirements a future variant might have w.r.t. the management
>> of clocks we don't know about yet.
>>
> 
> Right, at this point, this is first soc which adds the clocks in to the driver.
> Feels if the clocks are initialized and enabled/disabled as a part of some
> implementation specific callbacks, that would help always because that is
> the part which is going to different for each implementation and patches 2,3
> would remain common. Finally, as you have suggested will introduce new
> SMMU binding in the case of QCOM and will try to handle clocks specifically for that
> implementation and see how it looks.
> 
> Regards,
>  Sricharan
>