Hi Robin,
Thanks for your reply.
On 01/24/2018 09:49 PM, Robin Murphy wrote:
>>
>> +Optional properties:
>> +- clocks : A list of master clocks requires for the IOMMU to be
>> accessible
>
> s/requires/required/
ok
>
>> + by the host CPU. The number of clocks depends on the master
>> + block and might as well be zero. See [1] for generic clock
>
> Oops, some subtleties of English here :)
>
> To say "the number of clocks ... might as well be zero" effectively
> implies "there's no point ever specifying any clocks". I guess what you
> really mean here is "...might well be...", i.e. it is both valid and
> reasonably likely to require zero clocks.
ok
>
>> + bindings description.
>> +
>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
>> Optional properties:
>> - rockchip,disable-mmu-reset : Don't use the mmu reset operation.
>> @@ -27,5 +34,6 @@ Example:
>> reg = <0xff940300 0x100>;
>> interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
>> interrupt-names = "vopl_mmu";
>> + clocks = <&cru ACLK_VOP1>, <&cru DCLK_VOP1>, <&cru HCLK_VOP1>;
>> #iommu-cells = <0>;
>> };
>> diff --git a/drivers/iommu/rockchip-iommu.c
>> b/drivers/iommu/rockchip-iommu.c
>> index c4131ca792e0..8a5e2a659b67 100644
>> --- a/drivers/iommu/rockchip-iommu.c
>> +++ b/drivers/iommu/rockchip-iommu.c
>> @@ -4,6 +4,7 @@
>> * published by the Free Software Foundation.
>> */
>> +#include <linux/clk.h>
>> #include <linux/compiler.h>
>> #include <linux/delay.h>
>> #include <linux/device.h>
>> @@ -91,6 +92,8 @@ struct rk_iommu {
>> struct device *dev;
>> void __iomem **bases;
>> int num_mmu;
>> + struct clk_bulk_data *clocks;
>> + int num_clocks;
>> bool reset_disabled;
>> struct iommu_device iommu;
>> struct list_head node; /* entry in rk_iommu_domain.iommus */
>> @@ -450,6 +453,38 @@ static int rk_iommu_force_reset(struct rk_iommu
>> *iommu)
>> return 0;
>> }
>> +static int rk_iommu_of_get_clocks(struct rk_iommu *iommu)
>> +{
>> + struct device_node *np = iommu->dev->of_node;
>> + int ret;
>> + int i;
>> +
>> + ret = of_count_phandle_with_args(np, "clocks", "#clock-cells");
>> + if (ret == -ENOENT)
>> + return 0;
>> + else if (ret < 0)
>> + return ret;
>> +
>> + iommu->num_clocks = ret;
>> + iommu->clocks = devm_kcalloc(iommu->dev, iommu->num_clocks,
>> + sizeof(*iommu->clocks), GFP_KERNEL);
>> + if (!iommu->clocks)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < iommu->num_clocks; ++i) {
>> + iommu->clocks[i].clk = of_clk_get(np, i);
>> + if (IS_ERR(iommu->clocks[i].clk)) {
>> + ret = PTR_ERR(iommu->clocks[i].clk);
>> + goto err_clk_put;
>> + }
>> + }
>
> Just to confirm my understanding from a quick scan through the code, the
> reason we can't use clk_bulk_get() here is that currently, clocks[i].id
> being NULL means we'd end up just getting the first clock multiple
> times, right?
right, without a valid name, it would return the first clock.
/* Walk up the tree of devices looking for a clock that matches */
while (np) {
int index = 0;
/*
* For named clocks, first look up the name in the
* "clock-names" property. If it cannot be found, then
* index will be an error code, and of_clk_get() will fail.
*/
if (name)
index = of_property_match_string(np, "clock-names", name);
clk = __of_clk_get(np, index, dev_id, name);
>
> I guess there could be other users who also want "just get whatever
> clocks I have" functionality, so it might be worth proposing that for
> the core API as a separate/follow-up patch, but it definitely doesn't
> need to be part of this series.
right, i can try to do it later :)
>
> I really don't know enough about correct clk API usage, but modulo the
> binding comments it certainly looks nice and tidy now;
>
> Acked-by: Robin Murphy <robin.murphy at arm.com>
thanks.
>
> Thanks,
> Robin.
