From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751893AbeB0Q7T (ORCPT ); Tue, 27 Feb 2018 11:59:19 -0500 Received: from foss.arm.com ([217.140.101.70]:38614 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750938AbeB0Q7S (ORCPT ); Tue, 27 Feb 2018 11:59:18 -0500 From: Robin Murphy Subject: Re: [PATCH v5 08/13] iommu/rockchip: Control clocks needed to access the IOMMU To: JeffyChen , Rob Herring , Tomasz Figa Cc: "linux-kernel@vger.kernel.org" , Ricky Liang , simon xue , open@263.net, "263.netOPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS devicetree"@vger.kernel.org, Heiko Stuebner , "open list:ARM/Rockchip SoC..." , "list@263.net:IOMMU DRIVERS" , Joerg Roedel , Mark Rutland , linux-arm-kernel@lists.infradead.org References: <20180124103516.2571-1-jeffy.chen@rock-chips.com> <20180124103516.2571-9-jeffy.chen@rock-chips.com> <20180130170515.3g6wtadqgmehxh5b@rob-hp-laptop> <5A72F7D2.1050201@rock-chips.com> <5A8FEBC6.4000408@rock-chips.com> Message-ID: <33d1d6bd-3455-5cad-6990-a9ca94063f3a@arm.com> Date: Tue, 27 Feb 2018 16:59:13 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <5A8FEBC6.4000408@rock-chips.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Robin Murphy Subject: Re: [PATCH v5 08/13] iommu/rockchip: Control clocks needed to access the IOMMU Date: Tue, 27 Feb 2018 16:59:13 +0000 Message-ID: <33d1d6bd-3455-5cad-6990-a9ca94063f3a@arm.com> References: <20180124103516.2571-1-jeffy.chen@rock-chips.com> <20180124103516.2571-9-jeffy.chen@rock-chips.com> <20180130170515.3g6wtadqgmehxh5b@rob-hp-laptop> <5A72F7D2.1050201@rock-chips.com> <5A8FEBC6.4000408@rock-chips.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <5A8FEBC6.4000408-TNX95d0MmH7DzftRWevZcw@public.gmane.org> Content-Language: en-GB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: JeffyChen , Rob Herring , Tomasz Figa Cc: Mark Rutland , Heiko Stuebner , open-Y9sIeH5OGRo@public.gmane.org, "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Ricky Liang , "open list:ARM/Rockchip SoC..." , "list-Y9sIeH5OGRo@public.gmane.org:IOMMU DRIVERS" , "263.netOPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS devicetree"@vger.kernel.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org List-Id: linux-rockchip.vger.kernel.org T24gMjMvMDIvMTggMTA6MjQsIEplZmZ5Q2hlbiB3cm90ZToKPiBIaSBndXlzLAo+IAo+IE9uIDAy LzAxLzIwMTggMDc6MTkgUE0sIEplZmZ5Q2hlbiB3cm90ZToKPj4+Pj4+Cj4+Pj4+Pgo+Pj4+Pj4g ZGlmZiAtLWdpdAo+Pj4+Pj4gYS9Eb2N1bWVudGF0aW9uL2RldmljZXRyZWUvYmluZGluZ3MvaW9t bXUvcm9ja2NoaXAsaW9tbXUudHh0Cj4+Pj4+PiBiL0RvY3VtZW50YXRpb24vZGV2aWNldHJlZS9i aW5kaW5ncy9pb21tdS9yb2NrY2hpcCxpb21tdS50eHQKPj4+Pj4+IGluZGV4IDIwOThmNzczMjI2 NC4uMzNkZDg1MzM1OWZhIDEwMDY0NAo+Pj4+Pj4gLS0tIGEvRG9jdW1lbnRhdGlvbi9kZXZpY2V0 cmVlL2JpbmRpbmdzL2lvbW11L3JvY2tjaGlwLGlvbW11LnR4dAo+Pj4+Pj4gKysrIGIvRG9jdW1l bnRhdGlvbi9kZXZpY2V0cmVlL2JpbmRpbmdzL2lvbW11L3JvY2tjaGlwLGlvbW11LnR4dAo+Pj4+ Pj4gQEAgLTE0LDYgKzE0LDEzIEBAIFJlcXVpcmVkIHByb3BlcnRpZXM6Cj4+Pj4+PiDCoMKgwqDC oMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqAgInNpbmdsZS1tYXN0ZXIiIGRldmlj ZSwgYW5kIG5lZWRzIG5vCj4+Pj4+PiBhZGRpdGlvbmFsIGluZm9ybWF0aW9uCj4+Pj4+PiDCoMKg wqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqDCoMKgwqAgdG8gYXNzb2NpYXRlIHdpdGgg aXRzIG1hc3RlciBkZXZpY2UuwqAgU2VlOgo+Pj4+Pj4KPj4+Pj4+IERvY3VtZW50YXRpb24vZGV2 aWNldHJlZS9iaW5kaW5ncy9pb21tdS9pb21tdS50eHQKPj4+Pj4+ICtPcHRpb25hbCBwcm9wZXJ0 aWVzOgo+Pj4+Pj4gKy0gY2xvY2tzIDogQSBsaXN0IG9mIG1hc3RlciBjbG9ja3MgcmVxdWlyZXMg Zm9yIHRoZSBJT01NVSB0byBiZQo+Pj4+Pj4gYWNjZXNzaWJsZQo+Pj4+Pj4gK8KgwqDCoMKgwqDC oMKgwqDCoMKgIGJ5IHRoZSBob3N0IENQVS4gVGhlIG51bWJlciBvZiBjbG9ja3MgZGVwZW5kcyBv biB0aGUKPj4+Pj4+IG1hc3Rlcgo+Pj4+Pj4gK8KgwqDCoMKgwqDCoMKgwqDCoMKgIGJsb2NrIGFu ZCBtaWdodCBhcyB3ZWxsIGJlIHplcm8uIFNlZSBbMV0gZm9yIGdlbmVyaWMgCj4+Pj4+PiBjbG9j awo+Pj4+Pj4gK8KgwqDCoMKgwqDCoMKgwqDCoMKgIGJpbmRpbmdzIGRlc2NyaXB0aW9uLgo+Pj4+ Pgo+Pj4+PiBIYXJkd2FyZSBibG9ja3MgZG9uJ3QgaGF2ZSBhIHZhcmlhYmxlIG51bWJlciBvZiBj bG9jayBjb25uZWN0aW9ucy4KPj4+Pgo+Pj4+IEkgdGhpbmsgeW91IHVuZGVyZXN0aW1hdGUgdGhl IGltYWdpbmF0aW9uIG9mIGhhcmR3YXJlIGRlc2lnbmVycy4gOikKPj4+Cj4+PiBMZWFybmVkIGxv bmcgYWdvIHRvIG5ldmVyIGRvIHRoYXQuIElmIHRoZXJlIGFyZSAyIHdheXMgdG8gZG8KPj4+IHNv bWV0aGluZywgdGhleSB3aWxsIGZpbmQgYSAzcmQgd2F5Lgo+Pj4KPj4+PiBGb3IgUm9ja2NoaXAg SU9NTVUsIHRoZXJlIGlzIGEgc2V0IG9mIGNsb2Nrcywgd2hpY2ggYWxsIG5lZWQgdG8gYmUKPj4+ PiBlbmFibGVkIGZvciBJT01NVSByZWdpc3RlciBhY2Nlc3MgdG8gc3VjY2VlZC4gVGhlIGNsb2Nr cyBhcmUgbm90Cj4+Pj4gZGlyZWN0bHkgZmVkIHRvIHRoZSBJT01NVSwgYnV0IHRoZXkgYXJlIG5l ZWRlZCBmb3IgdGhlIHZhcmlvdXMgYnVzZXMKPj4+PiBhbmQgaW50ZXJtZWRpYXRlIGJsb2NrcyBv biB0aGUgd2F5IHRvIHRoZSBJT01NVSB0byB3b3JrLgo+Pj4KPj4+IFRoZSBiaW5kaW5nIHNob3Vs ZCBkZXNjcmliZSB0aGUgY2xvY2sgY29ubmVjdGlvbnMsIG5vdCB3aGF0IGNsb2NrcyBhCj4+PiBk cml2ZXIgbmVlZHMgKGN1cnJlbnRseSkuIEl0IHNvdW5kcyBsaWtlIGEgbGFjayBvZiBtYW5hZ2lu ZyBidXMgY2xvY2tzCj4+PiB0byBtZS4KPj4+Cj4+PiBJbiBhbnkgY2FzZSwgdGhlIGJpbmRpbmcg bXVzdCBiZSB3cml0dGVuIHNvIGl0IGNhbiBiZSB2ZXJpZmllZC4gSWYgeW91Cj4+PiBjYW4gaGF2 ZSBhbnkgbnVtYmVyIG9mIGNsb2NrcyB3aXRoIGFueSBuYW1lcywgdGhlcmUncyBubyBwb2ludCBp bgo+Pj4gZG9jdW1lbnRpbmcuCj4+Cj4+IHRoZSByb2NrY2hpcCBJT01NVSBpcyBwYXJ0IG9mIHRo ZSBtYXN0ZXIgYmxvY2sgaW4gaGFyZHdhcmUsIHNvIGl0IG5lZWRzCj4+IHRvIGNvbnRyb2wgdGhl IG1hc3RlcidzIHBvd2VyIGRvbWFpbiBhbmQgc29tZSBvZiB0aGUgbWFzdGVyJ3MgY2xvY2tzCj4+ IHdoZW4gYWNjZXNzIGl0J3MgcmVnaXN0ZXJzLgo+Pgo+PiBhbmQgdGhlIG51bWJlciBvZiBjbG9j a3MgbmVlZGVkIGhlcmUsIG1pZ2h0IGJlIGRpZmZlcmVudCBiZXR3ZWVuIGVhY2gKPj4gSU9NTVVz KGFjY29yZGluZyB0byB3aGljaCBtYXN0ZXIgYmxvY2sgaXQgYmVsb25ncyksIGl0J3MgYSBsaXR0 bGUgbGlrZQo+PiBvdXIgcG93ZXIgZG9tYWluOgo+PiBodHRwczovL2VsaXhpci5mcmVlLWVsZWN0 cm9ucy5jb20vbGludXgvbGF0ZXN0L3NvdXJjZS9hcmNoL2FybTY0L2Jvb3QvZHRzL3JvY2tjaGlw L3JrMzM5OS5kdHNpI0w5MzUgCj4+Cj4+Cj4+Cj4+IGknbSBub3Qgc3VyZSBob3cgdG8gZGVzY3Jp YmUgdGhpcyBjb3JyZWN0bHksIGlzIGl0IG9rIHVzZSBzb21ldGhpbmcgbGlrZQo+PiAidGhlIHNh bWUgYXMgaXQncyBtYXN0ZXIgYmxvY2siPwo+IAo+IHdvdWxkIGl0IG1ha2Ugc2Vuc2UgdG8gYWRk IGEgcHJvcGVydHkgdG8gc3BlY2lmeSB0aGUgbWFzdGVyIHdobyBvd25zIHRoZSAKPiBpb21tdSwg YW5kIHdlIGNhbiBnZXQgYWxsIGNsb2Nrcyhvbmx5IHNvbWUgb2YgdGhvc2UgY2xvY2tzIGFyZSBh Y3R1YWxseSAKPiBuZWVkZWQpIGZyb20gaXQgaW4gdGhlIG9mX3hsYXRlKCk/IGFuZCB3ZSBjYW4g YWxzbyByZXVzZSB0aGUgY2xvY2stbmFtZXMgCj4gb2YgdGhhdCBtYXN0ZXIgdG8gYnVpbGQgY2xr X2J1bGtfZGF0YSBhbmQgbG9nIGVycm9ycyBpbiBjbGtfYnVsa19nZXQuCgpJJ20gaW5jbGluZWQg dG8gYWdyZWUgd2l0aCBSb2IgaGVyZSAtIGlmIHdlJ3JlIHRvIGFkZCBhbnl0aGluZyB0byB0aGUg CmJpbmRpbmcsIGl0IHNob3VsZCBvbmx5IGJlIHdoYXRldmVyIGNsb2NrIGlucHV0cyBhcmUgZGVm aW5lZCBmb3IgdGhlIApJT01NVSBJUCBibG9jayBpdHNlbGYuIElmIExpbnV4IGRvZXNuJ3QgcHJv cGVybHkgaGFuZGxlIHRoZSBpbnRlcmNvbm5lY3QgCmNsb2NrIGhpZXJhcmNoeSBleHRlcm5hbCB0 byBhIHBhcnRpY3VsYXIgaW50ZWdyYXRpb24sIHRoYXQncyBhIHNlcGFyYXRlIAppc3N1ZSBhbmQg aXQncyBub3QgdGhlIGJpbmRpbmcncyBwcm9ibGVtLgoKSSBhY3R1YWxseSBxdWl0ZSBsaWtlIHRo ZSBoYWNrIG9mICJib3Jyb3dpbmciIHRoZSBjbG9ja3MgZnJvbSAKZGV2LT5vZl9ub2RlIGluIG9m X3hsYXRlKCkgLSB5b3Ugc2hvdWxkbid0IG5lZWQgYW55IERUIGNoYW5nZXMgZm9yIHRoYXQsIApi ZWNhdXNlIHlvdSBhbHJlYWR5IGtub3cgdGhhdCBlYWNoIElPTU1VIGluc3RhbmNlIG9ubHkgaGFz IHRoZSBvbmUgCm1hc3RlciBkZXZpY2UgYW55d2F5LgoKUm9iaW4uCl9fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmlvbW11IG1haWxpbmcgbGlzdAppb21tdUBs aXN0cy5saW51eC1mb3VuZGF0aW9uLm9yZwpodHRwczovL2xpc3RzLmxpbnV4Zm91bmRhdGlvbi5v cmcvbWFpbG1hbi9saXN0aW5mby9pb21tdQ== From mboxrd@z Thu Jan 1 00:00:00 1970 From: robin.murphy@arm.com (Robin Murphy) Date: Tue, 27 Feb 2018 16:59:13 +0000 Subject: [PATCH v5 08/13] iommu/rockchip: Control clocks needed to access the IOMMU In-Reply-To: <5A8FEBC6.4000408@rock-chips.com> References: <20180124103516.2571-1-jeffy.chen@rock-chips.com> <20180124103516.2571-9-jeffy.chen@rock-chips.com> <20180130170515.3g6wtadqgmehxh5b@rob-hp-laptop> <5A72F7D2.1050201@rock-chips.com> <5A8FEBC6.4000408@rock-chips.com> Message-ID: <33d1d6bd-3455-5cad-6990-a9ca94063f3a@arm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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.