[PATCH v11 15/27] iommu/exynos: use convenient macro to handle gate clocks

[PATCH v11 15/27] iommu/exynos: use convenient macro to handle gate clocks

[Cho KyongHo]

exynos-iommu driver must care about master H/W's gate clock as well as
System MMU's gate clock. To enhance readability of the source code,
macros to gate/ungate those clocks are defined.

Signed-off-by: Cho KyongHo <pullip.cho@samsung.com>
---
 drivers/iommu/exynos-iommu.c |   34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 71e77f1..cef62d0 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -101,6 +101,16 @@
 #define REG_PB1_SADDR		0x054
 #define REG_PB1_EADDR		0x058
 
+#define __clk_gate_ctrl(data, clk, en)	do {		\
+		if (data->clk)				\
+			clk_##en##able(data->clk);	\
+	} while (0)
+
+#define __sysmmu_clk_enable(data)	__clk_gate_ctrl(data, clk, en)
+#define __sysmmu_clk_disable(data)	__clk_gate_ctrl(data, clk, dis)
+#define __master_clk_enable(data)	__clk_gate_ctrl(data, clk_master, en)
+#define __master_clk_disable(data)	__clk_gate_ctrl(data, clk_master, dis)
+
 static struct kmem_cache *lv2table_kmem_cache;
 
 static unsigned long *section_entry(unsigned long *pgtable, unsigned long iova)
@@ -302,7 +312,7 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void *dev_id)
 
 	WARN_ON(!is_sysmmu_active(data));
 
-	clk_enable(data->clk_master);
+	__master_clk_enable(data);
 	itype = (enum exynos_sysmmu_inttype)
 		__ffs(__raw_readl(data->sfrbase + REG_INT_STATUS));
 	if (WARN_ON(!((itype >= 0) && (itype < SYSMMU_FAULT_UNKNOWN))))
@@ -329,7 +339,7 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void *dev_id)
 	if (itype != SYSMMU_FAULT_UNKNOWN)
 		sysmmu_unblock(data->sfrbase);
 
-	clk_disable(data->clk_master);
+	__master_clk_disable(data);
 
 	read_unlock(&data->lock);
 
@@ -346,12 +356,12 @@ static bool __exynos_sysmmu_disable(struct sysmmu_drvdata *data)
 	if (!set_sysmmu_inactive(data))
 		goto finish;
 
-	clk_enable(data->clk_master);
+	__master_clk_enable(data);
 
 	__raw_writel(CTRL_DISABLE, data->sfrbase + REG_MMU_CTRL);
 
-	clk_disable(data->clk);
-	clk_disable(data->clk_master);
+	__sysmmu_clk_disable(data);
+	__master_clk_disable(data);
 
 	disabled = true;
 	data->pgtable = 0;
@@ -396,14 +406,14 @@ static int __exynos_sysmmu_enable(struct sysmmu_drvdata *data,
 
 	data->pgtable = pgtable;
 
-	clk_enable(data->clk_master);
-	clk_enable(data->clk);
+	__master_clk_enable(data);
+	__sysmmu_clk_enable(data);
 
 	__sysmmu_set_ptbase(data->sfrbase, pgtable);
 
 	__raw_writel(CTRL_ENABLE, data->sfrbase + REG_MMU_CTRL);
 
-	clk_disable(data->clk_master);
+	__master_clk_disable(data);
 
 	data->domain = domain;
 
@@ -462,7 +472,7 @@ static void sysmmu_tlb_invalidate_entry(struct device *dev, unsigned long iova,
 		unsigned int maj;
 		unsigned int num_inv = 1;
 
-		clk_enable(data->clk_master);
+		__master_clk_enable(data);
 
 		maj = __raw_readl(data->sfrbase + REG_MMU_VERSION);
 		/*
@@ -483,7 +493,7 @@ static void sysmmu_tlb_invalidate_entry(struct device *dev, unsigned long iova,
 							num_inv);
 			sysmmu_unblock(data->sfrbase);
 		}
-		clk_disable(data->clk_master);
+		__master_clk_disable(data);
 	} else {
 		dev_dbg(data->sysmmu, "Disabled. Skipping invalidating TLB.\n");
 	}
@@ -499,12 +509,12 @@ void exynos_sysmmu_tlb_invalidate(struct device *dev)
 	read_lock_irqsave(&data->lock, flags);
 
 	if (is_sysmmu_active(data)) {
-		clk_enable(data->clk_master);
+		__master_clk_enable(data);
 		if (sysmmu_block(data->sfrbase)) {
 			__sysmmu_tlb_invalidate(data->sfrbase);
 			sysmmu_unblock(data->sfrbase);
 		}
-		clk_disable(data->clk_master);
+		__master_clk_disable(data);
 	} else {
 		dev_dbg(data->sysmmu, "Disabled. Skipping invalidating TLB.\n");
 	}
-- 
1.7.9.5

Re: [PATCH v11 15/27] iommu/exynos: use convenient macro to handle gate clocks

[Tomasz Figa]

Hi KyongHo,

On 14.03.2014 06:09, Cho KyongHo wrote:
> exynos-iommu driver must care about master H/W's gate clock as well as
> System MMU's gate clock. To enhance readability of the source code,
> macros to gate/ungate those clocks are defined.
>
> Signed-off-by: Cho KyongHo <pullip.cho@samsung.com>
> ---
>   drivers/iommu/exynos-iommu.c |   34 ++++++++++++++++++++++------------
>   1 file changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index 71e77f1..cef62d0 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -101,6 +101,16 @@
>   #define REG_PB1_SADDR		0x054
>   #define REG_PB1_EADDR		0x058
>
> +#define __clk_gate_ctrl(data, clk, en)	do {		\
> +		if (data->clk)				\
> +			clk_##en##able(data->clk);	\
> +	} while (0)
> +
> +#define __sysmmu_clk_enable(data)	__clk_gate_ctrl(data, clk, en)
> +#define __sysmmu_clk_disable(data)	__clk_gate_ctrl(data, clk, dis)
> +#define __master_clk_enable(data)	__clk_gate_ctrl(data, clk_master, en)
> +#define __master_clk_disable(data)	__clk_gate_ctrl(data, clk_master, dis)
> +

I'd say that such macros only obfuscate code, without any gains, as you 
can see in diffstat - this patch adds more lines than it removes.

Please drop this change.

Best regards,
Tomasz

Re: [PATCH v11 15/27] iommu/exynos: use convenient macro to handle gate clocks

[Sachin Kamat]

 Hi KyongHo,

On 14 March 2014 19:13, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi KyongHo,
>
>
> On 14.03.2014 06:09, Cho KyongHo wrote:
>>
>> exynos-iommu driver must care about master H/W's gate clock as well as
>> System MMU's gate clock. To enhance readability of the source code,
>> macros to gate/ungate those clocks are defined.
>>
>> Signed-off-by: Cho KyongHo <pullip.cho@samsung.com>
>> ---
>>   drivers/iommu/exynos-iommu.c |   34 ++++++++++++++++++++++------------
>>   1 file changed, 22 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
>> index 71e77f1..cef62d0 100644
>> --- a/drivers/iommu/exynos-iommu.c
>> +++ b/drivers/iommu/exynos-iommu.c
>> @@ -101,6 +101,16 @@
>>   #define REG_PB1_SADDR         0x054
>>   #define REG_PB1_EADDR         0x058
>>
>> +#define __clk_gate_ctrl(data, clk, en) do {            \
>> +               if (data->clk)                          \
>> +                       clk_##en##able(data->clk);      \
>> +       } while (0)
>> +
>> +#define __sysmmu_clk_enable(data)      __clk_gate_ctrl(data, clk, en)
>> +#define __sysmmu_clk_disable(data)     __clk_gate_ctrl(data, clk, dis)
>> +#define __master_clk_enable(data)      __clk_gate_ctrl(data, clk_master,
>> en)
>> +#define __master_clk_disable(data)     __clk_gate_ctrl(data, clk_master,
>> dis)
>> +
>
>
> I'd say that such macros only obfuscate code, without any gains, as you can
> see in diffstat - this patch adds more lines than it removes.
>
> Please drop this change.

 I agree with Tomasz here.


-- 
With warm regards,
Sachin

Re: [PATCH v11 15/27] iommu/exynos: use convenient macro to handle gate clocks

[Cho KyongHo]

On Fri, 14 Mar 2014 22:27:59 +0530, Sachin Kamat wrote:
>  Hi KyongHo,
> 
> On 14 March 2014 19:13, Tomasz Figa <t.figa@samsung.com> wrote:
> > Hi KyongHo,
> >
> >
> > On 14.03.2014 06:09, Cho KyongHo wrote:
> >>
> >> exynos-iommu driver must care about master H/W's gate clock as well as
> >> System MMU's gate clock. To enhance readability of the source code,
> >> macros to gate/ungate those clocks are defined.
> >>
> >> Signed-off-by: Cho KyongHo <pullip.cho@samsung.com>
> >> ---
> >>   drivers/iommu/exynos-iommu.c |   34 ++++++++++++++++++++++------------
> >>   1 file changed, 22 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> >> index 71e77f1..cef62d0 100644
> >> --- a/drivers/iommu/exynos-iommu.c
> >> +++ b/drivers/iommu/exynos-iommu.c
> >> @@ -101,6 +101,16 @@
> >>   #define REG_PB1_SADDR         0x054
> >>   #define REG_PB1_EADDR         0x058
> >>
> >> +#define __clk_gate_ctrl(data, clk, en) do {            \
> >> +               if (data->clk)                          \
> >> +                       clk_##en##able(data->clk);      \
> >> +       } while (0)
> >> +
> >> +#define __sysmmu_clk_enable(data)      __clk_gate_ctrl(data, clk, en)
> >> +#define __sysmmu_clk_disable(data)     __clk_gate_ctrl(data, clk, dis)
> >> +#define __master_clk_enable(data)      __clk_gate_ctrl(data, clk_master,
> >> en)
> >> +#define __master_clk_disable(data)     __clk_gate_ctrl(data, clk_master,
> >> dis)
> >> +
> >
> >
> > I'd say that such macros only obfuscate code, without any gains, as you can
> > see in diffstat - this patch adds more lines than it removes.
> >
> > Please drop this change.
> 
>  I agree with Tomasz here.
> 

Are you concerning about using macros or more insertions than deletions?

The deletions in this patch are only clk_enable() and clk_disable()
but they must be
"if (!IS_ERR(clk)) clk_enable(clk)" and "if (!IS_ERR(clk)) clk_disable(clk)".

I think use of macro is fancier in that case.

Thank you.

KyongHo.

Re: [PATCH v11 15/27] iommu/exynos: use convenient macro to handle gate clocks

[Sachin Kamat]

On 18 March 2014 16:33, Cho KyongHo <pullip.cho@samsung.com> wrote:
> On Fri, 14 Mar 2014 22:27:59 +0530, Sachin Kamat wrote:
>>  Hi KyongHo,
>>
>> On 14 March 2014 19:13, Tomasz Figa <t.figa@samsung.com> wrote:
>> > Hi KyongHo,
>> >
>> >
>> > On 14.03.2014 06:09, Cho KyongHo wrote:
>> >>
>> >> exynos-iommu driver must care about master H/W's gate clock as well as
>> >> System MMU's gate clock. To enhance readability of the source code,
>> >> macros to gate/ungate those clocks are defined.
>> >>
>> >> Signed-off-by: Cho KyongHo <pullip.cho@samsung.com>
>> >> ---
>> >>   drivers/iommu/exynos-iommu.c |   34 ++++++++++++++++++++++------------
>> >>   1 file changed, 22 insertions(+), 12 deletions(-)
>> >>
>> >> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
>> >> index 71e77f1..cef62d0 100644
>> >> --- a/drivers/iommu/exynos-iommu.c
>> >> +++ b/drivers/iommu/exynos-iommu.c
>> >> @@ -101,6 +101,16 @@
>> >>   #define REG_PB1_SADDR         0x054
>> >>   #define REG_PB1_EADDR         0x058
>> >>
>> >> +#define __clk_gate_ctrl(data, clk, en) do {            \
>> >> +               if (data->clk)                          \
>> >> +                       clk_##en##able(data->clk);      \
>> >> +       } while (0)
>> >> +
>> >> +#define __sysmmu_clk_enable(data)      __clk_gate_ctrl(data, clk, en)
>> >> +#define __sysmmu_clk_disable(data)     __clk_gate_ctrl(data, clk, dis)
>> >> +#define __master_clk_enable(data)      __clk_gate_ctrl(data, clk_master,
>> >> en)
>> >> +#define __master_clk_disable(data)     __clk_gate_ctrl(data, clk_master,
>> >> dis)
>> >> +
>> >
>> >
>> > I'd say that such macros only obfuscate code, without any gains, as you can
>> > see in diffstat - this patch adds more lines than it removes.
>> >
>> > Please drop this change.
>>
>>  I agree with Tomasz here.
>>
>
> Are you concerning about using macros or more insertions than deletions?

It is just making the code more difficult to read and understand.

-- 
With warm regards,
Sachin

Re: [PATCH v11 15/27] iommu/exynos: use convenient macro to handle gate clocks

[Tomasz Figa]

On 18.03.2014 12:18, Sachin Kamat wrote:
> On 18 March 2014 16:33, Cho KyongHo <pullip.cho@samsung.com> wrote:
>> On Fri, 14 Mar 2014 22:27:59 +0530, Sachin Kamat wrote:
>>>   Hi KyongHo,
>>>
>>> On 14 March 2014 19:13, Tomasz Figa <t.figa@samsung.com> wrote:
>>>> Hi KyongHo,
>>>>
>>>>
>>>> On 14.03.2014 06:09, Cho KyongHo wrote:
>>>>>
>>>>> exynos-iommu driver must care about master H/W's gate clock as well as
>>>>> System MMU's gate clock. To enhance readability of the source code,
>>>>> macros to gate/ungate those clocks are defined.
>>>>>
>>>>> Signed-off-by: Cho KyongHo <pullip.cho@samsung.com>
>>>>> ---
>>>>>    drivers/iommu/exynos-iommu.c |   34 ++++++++++++++++++++++------------
>>>>>    1 file changed, 22 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
>>>>> index 71e77f1..cef62d0 100644
>>>>> --- a/drivers/iommu/exynos-iommu.c
>>>>> +++ b/drivers/iommu/exynos-iommu.c
>>>>> @@ -101,6 +101,16 @@
>>>>>    #define REG_PB1_SADDR         0x054
>>>>>    #define REG_PB1_EADDR         0x058
>>>>>
>>>>> +#define __clk_gate_ctrl(data, clk, en) do {            \
>>>>> +               if (data->clk)                          \
>>>>> +                       clk_##en##able(data->clk);      \
>>>>> +       } while (0)
>>>>> +
>>>>> +#define __sysmmu_clk_enable(data)      __clk_gate_ctrl(data, clk, en)
>>>>> +#define __sysmmu_clk_disable(data)     __clk_gate_ctrl(data, clk, dis)
>>>>> +#define __master_clk_enable(data)      __clk_gate_ctrl(data, clk_master,
>>>>> en)
>>>>> +#define __master_clk_disable(data)     __clk_gate_ctrl(data, clk_master,
>>>>> dis)
>>>>> +
>>>>
>>>>
>>>> I'd say that such macros only obfuscate code, without any gains, as you can
>>>> see in diffstat - this patch adds more lines than it removes.
>>>>
>>>> Please drop this change.
>>>
>>>   I agree with Tomasz here.
>>>
>>
>> Are you concerning about using macros or more insertions than deletions?
>
> It is just making the code more difficult to read and understand.

Especially when hiding accesses to struct fields inside and doing fancy 
stuff like concatenations.

Best regards,
Tomasz