[PATCH v11 10/27] iommu/exynos: use managed device helper functions

[PATCH v11 10/27] iommu/exynos: use managed device helper functions

[Cho KyongHo]

This patch uses managed device helper functions in the probe().

Signed-off-by: Cho KyongHo <pullip.cho-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/iommu/exynos-iommu.c |   64 +++++++++++++++++-------------------------
 1 file changed, 26 insertions(+), 38 deletions(-)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 36e6b73..33b424d 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -499,51 +499,48 @@ void exynos_sysmmu_tlb_invalidate(struct device *dev)
 
 static int exynos_sysmmu_probe(struct platform_device *pdev)
 {
-	int ret;
+	int irq, ret;
 	struct device *dev = &pdev->dev;
 	struct sysmmu_drvdata *data;
 	struct resource *res;
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
-	if (!data) {
-		dev_dbg(dev, "Not enough memory\n");
-		ret = -ENOMEM;
-		goto err_alloc;
-	}
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
-		dev_dbg(dev, "Unable to find IOMEM region\n");
-		ret = -ENOENT;
-		goto err_init;
+		dev_err(dev, "Unable to find IOMEM region\n");
+		return -ENOENT;
 	}
 
-	data->sfrbase = ioremap(res->start, resource_size(res));
-	if (!data->sfrbase) {
-		dev_dbg(dev, "Unable to map IOMEM @ PA:%#x\n", res->start);
-		ret = -ENOENT;
-		goto err_res;
-	}
+	data->sfrbase = devm_ioremap_resource(dev, res);
+	if (IS_ERR(data->sfrbase))
+		return PTR_ERR(data->sfrbase);
 
-	ret = platform_get_irq(pdev, 0);
-	if (ret <= 0) {
+	irq = platform_get_irq(pdev, 0);
+	if (irq <= 0) {
 		dev_dbg(dev, "Unable to find IRQ resource\n");
-		goto err_irq;
+		return irq;
 	}
 
-	ret = request_irq(ret, exynos_sysmmu_irq, 0,
+	ret = devm_request_irq(dev, irq, exynos_sysmmu_irq, 0,
 				dev_name(dev), data);
 	if (ret) {
-		dev_dbg(dev, "Unabled to register interrupt handler\n");
-		goto err_irq;
+		dev_err(dev, "Unabled to register handler of irq %d\n", irq);
+		return ret;
 	}
 
-	if (dev_get_platdata(dev)) {
-		data->clk = clk_get(dev, "sysmmu");
-		if (IS_ERR(data->clk)) {
-			data->clk = NULL;
-			dev_dbg(dev, "No clock descriptor registered\n");
-		}
+	data->clk = devm_clk_get(dev, "sysmmu");
+	if (IS_ERR(data->clk)) {
+		dev_info(dev, "No gate clock found!\n");
+		data->clk = NULL;
+	}
+
+	ret = clk_prepare(data->clk);
+	if (ret) {
+		dev_err(dev, "Failed to prepare clk\n");
+		return ret;
 	}
 
 	data->sysmmu = dev;
@@ -556,17 +553,8 @@ static int exynos_sysmmu_probe(struct platform_device *pdev)
 
 	pm_runtime_enable(dev);
 
-	dev_dbg(dev, "Initialized\n");
+	dev_dbg(dev, "Probed and initialized\n");
 	return 0;
-err_irq:
-	free_irq(platform_get_irq(pdev, 0), data);
-err_res:
-	iounmap(data->sfrbase);
-err_init:
-	kfree(data);
-err_alloc:
-	dev_err(dev, "Failed to initialize\n");
-	return ret;
 }
 
 static struct platform_driver exynos_sysmmu_driver = {
-- 
1.7.9.5

Re: [PATCH v11 10/27] iommu/exynos: use managed device helper functions

[Tomasz Figa]

Hi KyongHo,

On 14.03.2014 06:05, Cho KyongHo wrote:
> This patch uses managed device helper functions in the probe().
>
> Signed-off-by: Cho KyongHo <pullip.cho-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>   drivers/iommu/exynos-iommu.c |   64 +++++++++++++++++-------------------------
>   1 file changed, 26 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index 36e6b73..33b424d 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -499,51 +499,48 @@ void exynos_sysmmu_tlb_invalidate(struct device *dev)
>
>   static int exynos_sysmmu_probe(struct platform_device *pdev)
>   {
> -	int ret;
> +	int irq, ret;
>   	struct device *dev = &pdev->dev;
>   	struct sysmmu_drvdata *data;
>   	struct resource *res;
>
> -	data = kzalloc(sizeof(*data), GFP_KERNEL);
> -	if (!data) {
> -		dev_dbg(dev, "Not enough memory\n");
> -		ret = -ENOMEM;
> -		goto err_alloc;
> -	}
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
>
>   	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>   	if (!res) {
> -		dev_dbg(dev, "Unable to find IOMEM region\n");
> -		ret = -ENOENT;
> -		goto err_init;
> +		dev_err(dev, "Unable to find IOMEM region\n");
> +		return -ENOENT;
>   	}

No need to check for error and print message, because 
devm_ioremap_resource() already checks the passed resource and handles 
error cases.

Best regards,
Tomasz

Re: [PATCH v11 10/27] iommu/exynos: use managed device helper functions

[Cho KyongHo]

On Fri, 14 Mar 2014 14:28:36 +0100, Tomasz Figa wrote:
> Hi KyongHo,
> 
> On 14.03.2014 06:05, Cho KyongHo wrote:
> > This patch uses managed device helper functions in the probe().
> >
> > Signed-off-by: Cho KyongHo <pullip.cho@samsung.com>
> > ---
> >   drivers/iommu/exynos-iommu.c |   64 +++++++++++++++++-------------------------
> >   1 file changed, 26 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> > index 36e6b73..33b424d 100644
> > --- a/drivers/iommu/exynos-iommu.c
> > +++ b/drivers/iommu/exynos-iommu.c
> > @@ -499,51 +499,48 @@ void exynos_sysmmu_tlb_invalidate(struct device *dev)
> >
> >   static int exynos_sysmmu_probe(struct platform_device *pdev)
> >   {
> > -	int ret;
> > +	int irq, ret;
> >   	struct device *dev = &pdev->dev;
> >   	struct sysmmu_drvdata *data;
> >   	struct resource *res;
> >
> > -	data = kzalloc(sizeof(*data), GFP_KERNEL);
> > -	if (!data) {
> > -		dev_dbg(dev, "Not enough memory\n");
> > -		ret = -ENOMEM;
> > -		goto err_alloc;
> > -	}
> > +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > +	if (!data)
> > +		return -ENOMEM;
> >
> >   	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >   	if (!res) {
> > -		dev_dbg(dev, "Unable to find IOMEM region\n");
> > -		ret = -ENOENT;
> > -		goto err_init;
> > +		dev_err(dev, "Unable to find IOMEM region\n");
> > +		return -ENOENT;
> >   	}
> 
> No need to check for error and print message, because 
> devm_ioremap_resource() already checks the passed resource and handles 
> error cases.
> 

Yes but devm_ioremap_resource() just tells that the given 'res' is not
correct. I think the message in the driver is more informative.

Thanks.

KyongHo

Re: [PATCH v11 10/27] iommu/exynos: use managed device helper functions

[Tomasz Figa]

On 18.03.2014 11:38, Cho KyongHo wrote:
> On Fri, 14 Mar 2014 14:28:36 +0100, Tomasz Figa wrote:
>> Hi KyongHo,
>>
>> On 14.03.2014 06:05, Cho KyongHo wrote:
>>> This patch uses managed device helper functions in the probe().
>>>
>>> Signed-off-by: Cho KyongHo <pullip.cho-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>>> ---
>>>    drivers/iommu/exynos-iommu.c |   64 +++++++++++++++++-------------------------
>>>    1 file changed, 26 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
>>> index 36e6b73..33b424d 100644
>>> --- a/drivers/iommu/exynos-iommu.c
>>> +++ b/drivers/iommu/exynos-iommu.c
>>> @@ -499,51 +499,48 @@ void exynos_sysmmu_tlb_invalidate(struct device *dev)
>>>
>>>    static int exynos_sysmmu_probe(struct platform_device *pdev)
>>>    {
>>> -	int ret;
>>> +	int irq, ret;
>>>    	struct device *dev = &pdev->dev;
>>>    	struct sysmmu_drvdata *data;
>>>    	struct resource *res;
>>>
>>> -	data = kzalloc(sizeof(*data), GFP_KERNEL);
>>> -	if (!data) {
>>> -		dev_dbg(dev, "Not enough memory\n");
>>> -		ret = -ENOMEM;
>>> -		goto err_alloc;
>>> -	}
>>> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>>> +	if (!data)
>>> +		return -ENOMEM;
>>>
>>>    	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>    	if (!res) {
>>> -		dev_dbg(dev, "Unable to find IOMEM region\n");
>>> -		ret = -ENOENT;
>>> -		goto err_init;
>>> +		dev_err(dev, "Unable to find IOMEM region\n");
>>> +		return -ENOENT;
>>>    	}
>>
>> No need to check for error and print message, because
>> devm_ioremap_resource() already checks the passed resource and handles
>> error cases.
>>
>
> Yes but devm_ioremap_resource() just tells that the given 'res' is not
> correct. I think the message in the driver is more informative.

The common practice used in Linux kernel is to not duplicate such 
messages. It is obvious that devm_ioremap_resource() printing such 
message is related to an IOMEM resource anyway, as you can't used it 
with other types of resources.

Best regards,
Tomasz

Re: [PATCH v11 10/27] iommu/exynos: use managed device helper functions

[Jingoo Han]

On Wednesday, March 19, 2014 12:12 AM, Tomasz Figa wrote:
> On 18.03.2014 11:38, Cho KyongHo wrote:
> > On Fri, 14 Mar 2014 14:28:36 +0100, Tomasz Figa wrote:
> >> On 14.03.2014 06:05, Cho KyongHo wrote:
> >>> This patch uses managed device helper functions in the probe().
> >>>
> >>> Signed-off-by: Cho KyongHo <pullip.cho@samsung.com>
> >>> ---
> >>>    drivers/iommu/exynos-iommu.c |   64 +++++++++++++++++-------------------------
> >>>    1 file changed, 26 insertions(+), 38 deletions(-)
> >>>
> >>> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> >>> index 36e6b73..33b424d 100644
> >>> --- a/drivers/iommu/exynos-iommu.c
> >>> +++ b/drivers/iommu/exynos-iommu.c

[.....]

> >>>    	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >>>    	if (!res) {
> >>> -		dev_dbg(dev, "Unable to find IOMEM region\n");
> >>> -		ret = -ENOENT;
> >>> -		goto err_init;
> >>> +		dev_err(dev, "Unable to find IOMEM region\n");
> >>> +		return -ENOENT;
> >>>    	}
> >>
> >> No need to check for error and print message, because
> >> devm_ioremap_resource() already checks the passed resource and handles
> >> error cases.
> >>
> >
> > Yes but devm_ioremap_resource() just tells that the given 'res' is not
> > correct. I think the message in the driver is more informative.
> 
> The common practice used in Linux kernel is to not duplicate such
> messages. It is obvious that devm_ioremap_resource() printing such
> message is related to an IOMEM resource anyway, as you can't used it
> with other types of resources.

+1

I agree with Tomasz Figa's opinion.
These messages have been being removed from Linux kernel.
Thank you.

Best regards,
Jingoo Han

Re: [PATCH v11 10/27] iommu/exynos: use managed device helper functions

[Cho KyongHo]

On Wed, 19 Mar 2014 08:59:09 +0900, Jingoo Han wrote:
> On Wednesday, March 19, 2014 12:12 AM, Tomasz Figa wrote:
> > On 18.03.2014 11:38, Cho KyongHo wrote:
> > > On Fri, 14 Mar 2014 14:28:36 +0100, Tomasz Figa wrote:
> > >> On 14.03.2014 06:05, Cho KyongHo wrote:
> > >>> This patch uses managed device helper functions in the probe().
> > >>>
> > >>> Signed-off-by: Cho KyongHo <pullip.cho-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> > >>> ---
> > >>>    drivers/iommu/exynos-iommu.c |   64 +++++++++++++++++-------------------------
> > >>>    1 file changed, 26 insertions(+), 38 deletions(-)
> > >>>
> > >>> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> > >>> index 36e6b73..33b424d 100644
> > >>> --- a/drivers/iommu/exynos-iommu.c
> > >>> +++ b/drivers/iommu/exynos-iommu.c
> 
> [.....]
> 
> > >>>    	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > >>>    	if (!res) {
> > >>> -		dev_dbg(dev, "Unable to find IOMEM region\n");
> > >>> -		ret = -ENOENT;
> > >>> -		goto err_init;
> > >>> +		dev_err(dev, "Unable to find IOMEM region\n");
> > >>> +		return -ENOENT;
> > >>>    	}
> > >>
> > >> No need to check for error and print message, because
> > >> devm_ioremap_resource() already checks the passed resource and handles
> > >> error cases.
> > >>
> > >
> > > Yes but devm_ioremap_resource() just tells that the given 'res' is not
> > > correct. I think the message in the driver is more informative.
> > 
> > The common practice used in Linux kernel is to not duplicate such
> > messages. It is obvious that devm_ioremap_resource() printing such
> > message is related to an IOMEM resource anyway, as you can't used it
> > with other types of resources.
> 
> +1
> 
> I agree with Tomasz Figa's opinion.
> These messages have been being removed from Linux kernel.
> Thank you.
> 
Ok.

Thank you for the advice.

Kyong Ho 

Re: [PATCH v11 10/27] iommu/exynos: use managed device helper functions

[Sachin Kamat]

Hi KyongHo,

On 14 March 2014 10:35, Cho KyongHo <pullip.cho-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> This patch uses managed device helper functions in the probe().
>
> Signed-off-by: Cho KyongHo <pullip.cho-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
[snip]

> +       data->clk = devm_clk_get(dev, "sysmmu");
> +       if (IS_ERR(data->clk)) {
> +               dev_info(dev, "No gate clock found!\n");
> +               data->clk = NULL;
> +       }

Why aren't you returning from here upon error?

> +
> +       ret = clk_prepare(data->clk);
> +       if (ret) {
> +               dev_err(dev, "Failed to prepare clk\n");
> +               return ret;
>         }
>
>         data->sysmmu = dev;
> @@ -556,17 +553,8 @@ static int exynos_sysmmu_probe(struct platform_device *pdev)
>
>         pm_runtime_enable(dev);
>
> -       dev_dbg(dev, "Initialized\n");
> +       dev_dbg(dev, "Probed and initialized\n");

This message looks redundant.

-- 
With warm regards,
Sachin

Re: [PATCH v11 10/27] iommu/exynos: use managed device helper functions

[Cho KyongHo]

On Fri, 14 Mar 2014 20:52:43 +0530, Sachin Kamat wrote:
> Hi KyongHo,
> 
> On 14 March 2014 10:35, Cho KyongHo <pullip.cho-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> > This patch uses managed device helper functions in the probe().
> >
> > Signed-off-by: Cho KyongHo <pullip.cho-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> > ---
> [snip]
> 
> > +       data->clk = devm_clk_get(dev, "sysmmu");
> > +       if (IS_ERR(data->clk)) {
> > +               dev_info(dev, "No gate clock found!\n");
> > +               data->clk = NULL;
> > +       }
> 
> Why aren't you returning from here upon error?

It is for the case of a System MMU which does not need clock gating.

> > +
> > +       ret = clk_prepare(data->clk);
> > +       if (ret) {
> > +               dev_err(dev, "Failed to prepare clk\n");
> > +               return ret;
> >         }
> >
> >         data->sysmmu = dev;
> > @@ -556,17 +553,8 @@ static int exynos_sysmmu_probe(struct platform_device *pdev)
> >
> >         pm_runtime_enable(dev);
> >
> > -       dev_dbg(dev, "Initialized\n");
> > +       dev_dbg(dev, "Probed and initialized\n");
> 
> This message looks redundant.

Ok.

Do you mean that checking sysfs does the same?

Thank you.

KyongHo.

Re: [PATCH v11 10/27] iommu/exynos: use managed device helper functions

[Tomasz Figa]

On 18.03.2014 12:09, Cho KyongHo wrote:
> On Fri, 14 Mar 2014 20:52:43 +0530, Sachin Kamat wrote:
>> Hi KyongHo,
>>
>> On 14 March 2014 10:35, Cho KyongHo <pullip.cho@samsung.com> wrote:
>>> This patch uses managed device helper functions in the probe().
>>>
>>> Signed-off-by: Cho KyongHo <pullip.cho@samsung.com>
>>> ---
>> [snip]
>>
>>> +       data->clk = devm_clk_get(dev, "sysmmu");
>>> +       if (IS_ERR(data->clk)) {
>>> +               dev_info(dev, "No gate clock found!\n");
>>> +               data->clk = NULL;
>>> +       }
>>
>> Why aren't you returning from here upon error?
>
> It is for the case of a System MMU which does not need clock gating.
>

Are there really such cases?

Best regards,
Tomasz

Re: [PATCH v11 10/27] iommu/exynos: use managed device helper functions

[Cho KyongHo]

On Tue, 18 Mar 2014 16:14:53 +0100, Tomasz Figa wrote:
> On 18.03.2014 12:09, Cho KyongHo wrote:
> > On Fri, 14 Mar 2014 20:52:43 +0530, Sachin Kamat wrote:
> >> Hi KyongHo,
> >>
> >> On 14 March 2014 10:35, Cho KyongHo <pullip.cho-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> >>> This patch uses managed device helper functions in the probe().
> >>>
> >>> Signed-off-by: Cho KyongHo <pullip.cho-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> >>> ---
> >> [snip]
> >>
> >>> +       data->clk = devm_clk_get(dev, "sysmmu");
> >>> +       if (IS_ERR(data->clk)) {
> >>> +               dev_info(dev, "No gate clock found!\n");
> >>> +               data->clk = NULL;
> >>> +       }
> >>
> >> Why aren't you returning from here upon error?
> >
> > It is for the case of a System MMU which does not need clock gating.
> >
> 
> Are there really such cases?
> 

Yes.
Especially in the case of initial stage of new SoC development.

I have experianced some software workaround for H/W restriction
needs prevention of clock gating for some devices.

Regards,

KyongHo

Re: [PATCH v11 10/27] iommu/exynos: use managed device helper functions

[Sachin Kamat]

On 19 March 2014 14:29, Cho KyongHo <pullip.cho-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> On Tue, 18 Mar 2014 16:14:53 +0100, Tomasz Figa wrote:
>> On 18.03.2014 12:09, Cho KyongHo wrote:
>> > On Fri, 14 Mar 2014 20:52:43 +0530, Sachin Kamat wrote:
>> >> Hi KyongHo,
>> >>
>> >> On 14 March 2014 10:35, Cho KyongHo <pullip.cho-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
>> >>> This patch uses managed device helper functions in the probe().
>> >>>
>> >>> Signed-off-by: Cho KyongHo <pullip.cho-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> >>> ---
>> >> [snip]
>> >>
>> >>> +       data->clk = devm_clk_get(dev, "sysmmu");
>> >>> +       if (IS_ERR(data->clk)) {
>> >>> +               dev_info(dev, "No gate clock found!\n");
>> >>> +               data->clk = NULL;
>> >>> +       }
>> >>
>> >> Why aren't you returning from here upon error?
>> >
>> > It is for the case of a System MMU which does not need clock gating.
>> >
>>
>> Are there really such cases?
>>
>
> Yes.
> Especially in the case of initial stage of new SoC development.
>
> I have experianced some software workaround for H/W restriction
> needs prevention of clock gating for some devices.

So aren't these basically some exceptions/hacks rather than the usual way
of functioning of the device?

-- 
With warm regards,
Sachin

Re: [PATCH v11 10/27] iommu/exynos: use managed device helper functions

[Tomasz Figa]

On 19.03.2014 10:01, Sachin Kamat wrote:
> On 19 March 2014 14:29, Cho KyongHo <pullip.cho@samsung.com> wrote:
>> On Tue, 18 Mar 2014 16:14:53 +0100, Tomasz Figa wrote:
>>> On 18.03.2014 12:09, Cho KyongHo wrote:
>>>> On Fri, 14 Mar 2014 20:52:43 +0530, Sachin Kamat wrote:
>>>>> Hi KyongHo,
>>>>>
>>>>> On 14 March 2014 10:35, Cho KyongHo <pullip.cho@samsung.com> wrote:
>>>>>> This patch uses managed device helper functions in the probe().
>>>>>>
>>>>>> Signed-off-by: Cho KyongHo <pullip.cho@samsung.com>
>>>>>> ---
>>>>> [snip]
>>>>>
>>>>>> +       data->clk = devm_clk_get(dev, "sysmmu");
>>>>>> +       if (IS_ERR(data->clk)) {
>>>>>> +               dev_info(dev, "No gate clock found!\n");
>>>>>> +               data->clk = NULL;
>>>>>> +       }
>>>>>
>>>>> Why aren't you returning from here upon error?
>>>>
>>>> It is for the case of a System MMU which does not need clock gating.
>>>>
>>>
>>> Are there really such cases?
>>>
>>
>> Yes.
>> Especially in the case of initial stage of new SoC development.
>>
>> I have experianced some software workaround for H/W restriction
>> needs prevention of clock gating for some devices.
>
> So aren't these basically some exceptions/hacks rather than the usual way
> of functioning of the device?
>

This actually raises a good question, whether we really need to support 
such early development SoC versions in mainline.

Another thing is that if you need to assure that a clock is ungated, you 
must acquire it and prepare_enable explicitly, so I don't think this 
kind of handling is correct.

Best regards,
Tomasz

Re: [PATCH v11 10/27] iommu/exynos: use managed device helper functions

[Cho KyongHo]

On Wed, 19 Mar 2014 13:08:42 +0100, Tomasz Figa wrote:
> On 19.03.2014 10:01, Sachin Kamat wrote:
> > On 19 March 2014 14:29, Cho KyongHo <pullip.cho-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> >> On Tue, 18 Mar 2014 16:14:53 +0100, Tomasz Figa wrote:
> >>> On 18.03.2014 12:09, Cho KyongHo wrote:
> >>>> On Fri, 14 Mar 2014 20:52:43 +0530, Sachin Kamat wrote:
> >>>>> Hi KyongHo,
> >>>>>
> >>>>> On 14 March 2014 10:35, Cho KyongHo <pullip.cho-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> >>>>>> This patch uses managed device helper functions in the probe().
> >>>>>>
> >>>>>> Signed-off-by: Cho KyongHo <pullip.cho-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> >>>>>> ---
> >>>>> [snip]
> >>>>>
> >>>>>> +       data->clk = devm_clk_get(dev, "sysmmu");
> >>>>>> +       if (IS_ERR(data->clk)) {
> >>>>>> +               dev_info(dev, "No gate clock found!\n");
> >>>>>> +               data->clk = NULL;
> >>>>>> +       }
> >>>>>
> >>>>> Why aren't you returning from here upon error?
> >>>>
> >>>> It is for the case of a System MMU which does not need clock gating.
> >>>>
> >>>
> >>> Are there really such cases?
> >>>
> >>
> >> Yes.
> >> Especially in the case of initial stage of new SoC development.
> >>
> >> I have experianced some software workaround for H/W restriction
> >> needs prevention of clock gating for some devices.
> >
> > So aren't these basically some exceptions/hacks rather than the usual way
> > of functioning of the device?
> >
> 
> This actually raises a good question, whether we really need to support 
> such early development SoC versions in mainline.
> 
> Another thing is that if you need to assure that a clock is ungated, you 
> must acquire it and prepare_enable explicitly, so I don't think this 
> kind of handling is correct.
> 
On early development step of a new SoC, clock related stuffs and
some device drivers like display controller are usually developed in parallel.

In that case, -ENOENT from clk_get() must not treated as an error.
"[PATCH v11 20/17] iommu/exynos: allow having multiple System MMUs for a master H/W"
patch distinguishes -ENOENT from other error values returned by devm_clk_get().

Regards,

KyongHo


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v11 10/27] iommu/exynos: use managed device helper functions

[Tomasz Figa]

On 20.03.2014 11:03, Cho KyongHo wrote:
> On Wed, 19 Mar 2014 13:08:42 +0100, Tomasz Figa wrote:
>> On 19.03.2014 10:01, Sachin Kamat wrote:
>>> On 19 March 2014 14:29, Cho KyongHo <pullip.cho@samsung.com> wrote:
>>>> On Tue, 18 Mar 2014 16:14:53 +0100, Tomasz Figa wrote:
>>>>> On 18.03.2014 12:09, Cho KyongHo wrote:
>>>>>> On Fri, 14 Mar 2014 20:52:43 +0530, Sachin Kamat wrote:
>>>>>>> Hi KyongHo,
>>>>>>>
>>>>>>> On 14 March 2014 10:35, Cho KyongHo <pullip.cho@samsung.com> wrote:
>>>>>>>> This patch uses managed device helper functions in the probe().
>>>>>>>>
>>>>>>>> Signed-off-by: Cho KyongHo <pullip.cho@samsung.com>
>>>>>>>> ---
>>>>>>> [snip]
>>>>>>>
>>>>>>>> +       data->clk = devm_clk_get(dev, "sysmmu");
>>>>>>>> +       if (IS_ERR(data->clk)) {
>>>>>>>> +               dev_info(dev, "No gate clock found!\n");
>>>>>>>> +               data->clk = NULL;
>>>>>>>> +       }
>>>>>>>
>>>>>>> Why aren't you returning from here upon error?
>>>>>>
>>>>>> It is for the case of a System MMU which does not need clock gating.
>>>>>>
>>>>>
>>>>> Are there really such cases?
>>>>>
>>>>
>>>> Yes.
>>>> Especially in the case of initial stage of new SoC development.
>>>>
>>>> I have experianced some software workaround for H/W restriction
>>>> needs prevention of clock gating for some devices.
>>>
>>> So aren't these basically some exceptions/hacks rather than the usual way
>>> of functioning of the device?
>>>
>>
>> This actually raises a good question, whether we really need to support
>> such early development SoC versions in mainline.
>>
>> Another thing is that if you need to assure that a clock is ungated, you
>> must acquire it and prepare_enable explicitly, so I don't think this
>> kind of handling is correct.
>>
> On early development step of a new SoC, clock related stuffs and
> some device drivers like display controller are usually developed in parallel.
>
> In that case, -ENOENT from clk_get() must not treated as an error.
> "[PATCH v11 20/17] iommu/exynos: allow having multiple System MMUs for a master H/W"
> patch distinguishes -ENOENT from other error values returned by devm_clk_get().

I still don't think upstream is right place for such development hacks 
and such assumption will mask potential errors caused by clocks 
unspecified in DT.

If such thing is needed for development, an extra patch might be kept in 
development tree, until clock driver is implemented or a dummy 
fixed-rate clock might be specified in DT.

Best regards,
Tomasz

Re: [PATCH v11 10/27] iommu/exynos: use managed device helper functions

[Cho KyongHo]

On Thu, 20 Mar 2014 11:44:50 +0100, Tomasz Figa wrote:
> On 20.03.2014 11:03, Cho KyongHo wrote:
> > On Wed, 19 Mar 2014 13:08:42 +0100, Tomasz Figa wrote:
> >> On 19.03.2014 10:01, Sachin Kamat wrote:
> >>> On 19 March 2014 14:29, Cho KyongHo <pullip.cho-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> >>>> On Tue, 18 Mar 2014 16:14:53 +0100, Tomasz Figa wrote:
> >>>>> On 18.03.2014 12:09, Cho KyongHo wrote:
> >>>>>> On Fri, 14 Mar 2014 20:52:43 +0530, Sachin Kamat wrote:
> >>>>>>> Hi KyongHo,
> >>>>>>>
> >>>>>>> On 14 March 2014 10:35, Cho KyongHo <pullip.cho-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> >>>>>>>> This patch uses managed device helper functions in the probe().
> >>>>>>>>
> >>>>>>>> Signed-off-by: Cho KyongHo <pullip.cho-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> >>>>>>>> ---
> >>>>>>> [snip]
> >>>>>>>
> >>>>>>>> +       data->clk = devm_clk_get(dev, "sysmmu");
> >>>>>>>> +       if (IS_ERR(data->clk)) {
> >>>>>>>> +               dev_info(dev, "No gate clock found!\n");
> >>>>>>>> +               data->clk = NULL;
> >>>>>>>> +       }
> >>>>>>>
> >>>>>>> Why aren't you returning from here upon error?
> >>>>>>
> >>>>>> It is for the case of a System MMU which does not need clock gating.
> >>>>>>
> >>>>>
> >>>>> Are there really such cases?
> >>>>>
> >>>>
> >>>> Yes.
> >>>> Especially in the case of initial stage of new SoC development.
> >>>>
> >>>> I have experianced some software workaround for H/W restriction
> >>>> needs prevention of clock gating for some devices.
> >>>
> >>> So aren't these basically some exceptions/hacks rather than the usual way
> >>> of functioning of the device?
> >>>
> >>
> >> This actually raises a good question, whether we really need to support
> >> such early development SoC versions in mainline.
> >>
> >> Another thing is that if you need to assure that a clock is ungated, you
> >> must acquire it and prepare_enable explicitly, so I don't think this
> >> kind of handling is correct.
> >>
> > On early development step of a new SoC, clock related stuffs and
> > some device drivers like display controller are usually developed in parallel.
> >
> > In that case, -ENOENT from clk_get() must not treated as an error.
> > "[PATCH v11 20/17] iommu/exynos: allow having multiple System MMUs for a master H/W"
> > patch distinguishes -ENOENT from other error values returned by devm_clk_get().
> 
> I still don't think upstream is right place for such development hacks 
> and such assumption will mask potential errors caused by clocks 
> unspecified in DT.
> 
> If such thing is needed for development, an extra patch might be kept in 
> development tree, until clock driver is implemented or a dummy 
> fixed-rate clock might be specified in DT.
> 

Ok.

Now I understand.

Error from clk_get() will be failure of probe in the next patch series.

Thanks.

KyongHo