Re: [PATCH 7/8] uio: bind uio_dmem_genirq via OF

Re: [PATCH 7/8] uio: bind uio_dmem_genirq via OF

[Mark Rutland]

[adding devicetree list]

On Fri, Jul 15, 2016 at 02:34:02PM +0530, Anup Patel wrote:
> From: Jan Viktorin <viktorin@rehivetech.com>
> 
> The uio_dmem_genirq works in a similar ways as uio_pdrv_genirq now.
> 
> It accepts the of_id module parameter to specify UIO compatible
> string as module parameter. There are few other module parameters
> to specify DT property name for number bits in DMA mask and details
> about dynamic regions.
> 
> Following are the newly added module parameters:
> 1) of_id: The UIO compatible string to be used for DT probing
> 2) of_dma_bits_prot: This is name of OF property which will be
> used to specify number of DMA mask bits in UIO DT node.
> 3) of_dmem_count_prop: This is name of OF property which will be
> used to specify number of dynamic regions in UIO DT node.
> 4) of_dmem_sizes_prop: This is name of OF property which will be
> used to specify sizes of each dynamic regions in UIO DT node.
> 
> Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
> Signed-off-by: Anup Patel <anup.patel@broadcom.com>
> ---
>  drivers/uio/uio_dmem_genirq.c | 113 +++++++++++++++++++++++++++++++++---------
>  1 file changed, 89 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/uio/uio_dmem_genirq.c b/drivers/uio/uio_dmem_genirq.c
> index a4d6d81..0a0cc19 100644
> --- a/drivers/uio/uio_dmem_genirq.c
> +++ b/drivers/uio/uio_dmem_genirq.c
> @@ -144,46 +144,107 @@ static int uio_dmem_genirq_irqcontrol(struct uio_info *dev_info, s32 irq_on)
>  	return 0;
>  }
>  
> +static char uio_of_dma_bits_prop[128] = "uio,dma-bits";
> +static char uio_of_dmem_count_prop[128] = "uio,number-of-dynamic-regions";
> +static char uio_of_dmem_sizes_prop[128] = "uio,dynamic-regions-sizes";

What are these proeprties? What is a "dynamic region" in hardware terms?

Why must they be in the DT, and why are the *property names* arbitrarily
overridable as module parameters? What exactly do you expect there to be
in a DT?

DT bindings need documentation, but there was none as part of this series.

I do not think this makes sense from a DT perspective.

Thanks,
Mark.

> +
> +static int uio_dmem_genirq_alloc_platdata(struct platform_device *pdev)
> +{
> +	struct uio_dmem_genirq_pdata pdata;
> +	u32 dma_bits, regions;
> +	u32 sizes[MAX_UIO_MAPS];
> +	int ret;
> +
> +	memset(&pdata, 0, sizeof(pdata));
> +
> +	ret = of_property_read_u32(pdev->dev.of_node,
> +				   uio_of_dma_bits_prop, &dma_bits);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"Missing property %s\n", uio_of_dma_bits_prop);
> +		return ret;
> +	}
> +	if (dma_bits > 64)
> +		dma_bits = 64;
> +
> +	dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(dma_bits));
> +
> +	ret = of_property_read_u32(pdev->dev.of_node,
> +				   uio_of_dmem_count_prop, &regions);
> +	if (ret) {
> +		dev_err(&pdev->dev,
> +			"Missing property %s\n", uio_of_dmem_count_prop);
> +		return ret;
> +	}
> +
> +	regions = min_t(u32, regions, MAX_UIO_MAPS);
> +
> +	ret = of_property_read_u32_array(pdev->dev.of_node,
> +					 uio_of_dmem_sizes_prop, sizes,
> +					 regions);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Missing or invalid property %s\n",
> +			uio_of_dmem_sizes_prop);
> +		return ret;
> +	}
> +
> +	pdata.dynamic_region_sizes = devm_kzalloc(&pdev->dev,
> +			sizeof(*pdata.dynamic_region_sizes) *
> +			pdata.num_dynamic_regions, GFP_KERNEL);
> +	if (!pdata.dynamic_region_sizes) {
> +		dev_err(&pdev->dev, "Unable to alloc dynamic_region_sizes\n");
> +		ret = -ENOMEM;
> +		return ret;
> +	}
> +
> +	pdata.num_dynamic_regions = regions;
> +	while (regions--)
> +		pdata.dynamic_region_sizes[regions] = sizes[regions];
> +
> +	pdata.uioinfo.name = pdev->dev.of_node->name;
> +	pdata.uioinfo.version = "devicetree";
> +
> +	return platform_device_add_data(pdev, &pdata, sizeof(pdata));
> +}
> +
>  static int uio_dmem_genirq_probe(struct platform_device *pdev)
>  {
> -	struct uio_dmem_genirq_pdata *pdata = dev_get_platdata(&pdev->dev);
> -	struct uio_info *uioinfo = &pdata->uioinfo;
> +	struct uio_dmem_genirq_pdata *pdata;
> +	struct uio_info *uioinfo;
>  	struct uio_dmem_genirq_platdata *priv;
>  	struct uio_mem *uiomem;
>  	int ret = -EINVAL;
>  	int i;
>  
>  	if (pdev->dev.of_node) {
> -		/* alloc uioinfo for one device */
> -		uioinfo = kzalloc(sizeof(*uioinfo), GFP_KERNEL);
> -		if (!uioinfo) {
> -			ret = -ENOMEM;
> -			dev_err(&pdev->dev, "unable to kmalloc\n");
> +		ret = uio_dmem_genirq_alloc_platdata(pdev);
> +		if (ret)
>  			goto bad2;
> -		}
> -		uioinfo->name = pdev->dev.of_node->name;
> -		uioinfo->version = "devicetree";
>  	}
>  
> +	pdata = dev_get_platdata(&pdev->dev);
> +	uioinfo = &pdata->uioinfo;
> +
>  	if (!uioinfo || !uioinfo->name || !uioinfo->version) {
>  		dev_err(&pdev->dev, "missing platform_data\n");
> -		goto bad0;
> +		goto bad2;
>  	}
>  
>  	if (uioinfo->handler || uioinfo->irqcontrol ||
>  	    uioinfo->irq_flags & IRQF_SHARED) {
>  		dev_err(&pdev->dev, "interrupt configuration error\n");
> -		goto bad0;
> +		goto bad2;
>  	}
>  
>  	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>  	if (!priv) {
>  		ret = -ENOMEM;
>  		dev_err(&pdev->dev, "unable to kmalloc\n");
> -		goto bad0;
> +		goto bad2;
>  	}
>  
> -	dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
> +	if (!pdev->dev.of_node)
> +		dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
>  
>  	priv->uioinfo = uioinfo;
>  	spin_lock_init(&priv->lock);
> @@ -278,10 +339,6 @@ static int uio_dmem_genirq_probe(struct platform_device *pdev)
>  	return 0;
>   bad1:
>  	kfree(priv);
> - bad0:
> -	/* kfree uioinfo for OF */
> -	if (pdev->dev.of_node)
> -		kfree(uioinfo);
>   bad2:
>  	return ret;
>  }
> @@ -296,10 +353,6 @@ static int uio_dmem_genirq_remove(struct platform_device *pdev)
>  	priv->uioinfo->handler = NULL;
>  	priv->uioinfo->irqcontrol = NULL;
>  
> -	/* kfree uioinfo for OF */
> -	if (pdev->dev.of_node)
> -		kfree(priv->uioinfo);
> -
>  	kfree(priv);
>  	return 0;
>  }
> @@ -327,10 +380,22 @@ static const struct dev_pm_ops uio_dmem_genirq_dev_pm_ops = {
>  };
>  
>  #ifdef CONFIG_OF
> -static const struct of_device_id uio_of_genirq_match[] = {
> -	{ /* empty for now */ },
> +static struct of_device_id uio_of_genirq_match[] = {
> +	{ /* This is filled with module_parm */ },
> +	{ /* end of list */ },
>  };
>  MODULE_DEVICE_TABLE(of, uio_of_genirq_match);
> +module_param_string(of_id, uio_of_genirq_match[0].compatible, 128, 0);
> +MODULE_PARM_DESC(of_id, "Openfirmware id of the device to be handled by uio");
> +module_param_string(of_dma_bits_prop, uio_of_dma_bits_prop, 128, 0);
> +MODULE_PARM_DESC(of_dma_bits_prop,
> +		 "Openfirmware property for number of bits in DMA mask");
> +module_param_string(of_dmem_count_prop, uio_of_dmem_count_prop, 128, 0);
> +MODULE_PARM_DESC(of_dmem_count_prop,
> +		 "Openfirmware property for dynamic region count");
> +module_param_string(of_dmem_sizes_prop, uio_of_dmem_sizes_prop, 128, 0);
> +MODULE_PARM_DESC(of_dmem_sizes_prop,
> +		 "Openfirmware property for dynamic region sizes");
>  #endif
>  
>  static struct platform_driver uio_dmem_genirq = {
> -- 
> 1.9.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH 7/8] uio: bind uio_dmem_genirq via OF

[Anup Patel]

On Fri, Jul 15, 2016 at 6:58 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> [adding devicetree list]
>
> On Fri, Jul 15, 2016 at 02:34:02PM +0530, Anup Patel wrote:
>> From: Jan Viktorin <viktorin@rehivetech.com>
>>
>> The uio_dmem_genirq works in a similar ways as uio_pdrv_genirq now.
>>
>> It accepts the of_id module parameter to specify UIO compatible
>> string as module parameter. There are few other module parameters
>> to specify DT property name for number bits in DMA mask and details
>> about dynamic regions.
>>
>> Following are the newly added module parameters:
>> 1) of_id: The UIO compatible string to be used for DT probing
>> 2) of_dma_bits_prot: This is name of OF property which will be
>> used to specify number of DMA mask bits in UIO DT node.
>> 3) of_dmem_count_prop: This is name of OF property which will be
>> used to specify number of dynamic regions in UIO DT node.
>> 4) of_dmem_sizes_prop: This is name of OF property which will be
>> used to specify sizes of each dynamic regions in UIO DT node.
>>
>> Signed-off-by: Jan Viktorin <viktorin@rehivetech.com>
>> Signed-off-by: Anup Patel <anup.patel@broadcom.com>
>> ---
>>  drivers/uio/uio_dmem_genirq.c | 113 +++++++++++++++++++++++++++++++++---------
>>  1 file changed, 89 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/uio/uio_dmem_genirq.c b/drivers/uio/uio_dmem_genirq.c
>> index a4d6d81..0a0cc19 100644
>> --- a/drivers/uio/uio_dmem_genirq.c
>> +++ b/drivers/uio/uio_dmem_genirq.c
>> @@ -144,46 +144,107 @@ static int uio_dmem_genirq_irqcontrol(struct uio_info *dev_info, s32 irq_on)
>>       return 0;
>>  }
>>
>> +static char uio_of_dma_bits_prop[128] = "uio,dma-bits";
>> +static char uio_of_dmem_count_prop[128] = "uio,number-of-dynamic-regions";
>> +static char uio_of_dmem_sizes_prop[128] = "uio,dynamic-regions-sizes";
>
> What are these proeprties? What is a "dynamic region" in hardware terms?

"Dynamic region" is DMAble memory alloced using dma_alloc_xxx() APIs
for the device.

For DMA-capable devices accessed from user-space, we need DMAble
memory available to user-space. Sometime such devices are also
cache-coherent (or IO-coherent) so in such case user-space will need
cacheable DMAble memory.

Number of "Dynamic regions" required by UIO dmem device depends
on the type of HW device hence we describe number of "Dynamic regions"
and size of "Dynamic regions" via DT attributes.

>
> Why must they be in the DT, and why are the *property names* arbitrarily
> overridable as module parameters? What exactly do you expect there to be
> in a DT?

They must be in DT for platform devices created using DT probing. The
number of "Dynamic regions" required by UIO device will depend on the
nature of device. We cannot have same number and sizes of "Dynamic
regions" for different UIO dmem devices.

The UIO dmem driver is a generic driver and not specific to any type of
HW. In fact, UIO dmem driver is generic enough to access variety of
platform devices from user-space using UIO framework.

Based on this Rob had previously suggested to not have any (or enforce
any) DT bindings for UIO dmem driver.

Refer, https://lkml.org/lkml/2016/5/18/457
Link to original Jan's patchset is https://lkml.org/lkml/2016/5/17/141

We had two options:
1) Get details of "Dynamic regions" via module parameter but this
would mean using same number and sizes of "Dynamic regions" for
all UIO dmem devices.
2) Pass names of DT attributes used by UIO dmem driver as
module parameters.

>
> DT bindings need documentation, but there was none as part of this series.
>
> I do not think this makes sense from a DT perspective.
>

As explained above, we are not fixing DT compatible string and
DT attributes names for UIO dmem driver instead we pass all
these as module parameters (preferable via kernel args or at
time of module insertion). Due to this we don't require DT bindings
document.

Regards,
Anup

Re: [PATCH 7/8] uio: bind uio_dmem_genirq via OF

[Mark Rutland]

On Fri, Jul 15, 2016 at 09:57:28PM +0530, Anup Patel wrote:
> On Fri, Jul 15, 2016 at 6:58 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Fri, Jul 15, 2016 at 02:34:02PM +0530, Anup Patel wrote:
> >> +static char uio_of_dma_bits_prop[128] = "uio,dma-bits";
> >> +static char uio_of_dmem_count_prop[128] = "uio,number-of-dynamic-regions";
> >> +static char uio_of_dmem_sizes_prop[128] = "uio,dynamic-regions-sizes";
> >
> > What are these proeprties? What is a "dynamic region" in hardware terms?
> 
> "Dynamic region" is DMAble memory alloced using dma_alloc_xxx() APIs
> for the device.
> 
> For DMA-capable devices accessed from user-space, we need DMAble
> memory available to user-space. Sometime such devices are also
> cache-coherent (or IO-coherent) so in such case user-space will need
> cacheable DMAble memory.
> 
> Number of "Dynamic regions" required by UIO dmem device depends
> on the type of HW device hence we describe number of "Dynamic regions"
> and size of "Dynamic regions" via DT attributes.

So this is something akin to the number of DMA channels a device might
use.

Either that's implicit knowledge of the device (if fixed), which doesn't
need to be in the DT, or a real property that should be described
explicitly in the device binding, that has nothing to do with UIO.

In general, a device which you might want to use with UIO may not
require such properties, so this is imposing a requirement on DTBs
purely for the sake of userspace software.

> > Why must they be in the DT, and why are the *property names* arbitrarily
> > overridable as module parameters? What exactly do you expect there to be
> > in a DT?
> 
> They must be in DT for platform devices created using DT probing. The
> number of "Dynamic regions" required by UIO device will depend on the
> nature of device. We cannot have same number and sizes of "Dynamic
> regions" for different UIO dmem devices.

If the devices are so different, they should not be handled by the same
driver that has no knowledge of those differences. Platform VFIO has the
same problem, and requires device-specific reset drivers.

> The UIO dmem driver is a generic driver and not specific to any type of
> HW. In fact, UIO dmem driver is generic enough to access variety of
> platform devices from user-space using UIO framework.
> 
> Based on this Rob had previously suggested to not have any (or enforce
> any) DT bindings for UIO dmem driver.
> 
> Refer, https://lkml.org/lkml/2016/5/18/457

I don't follow. Here Rob stated:

  DT describes h/w. UIO is not a h/w block, so this does not belong in DT. A
  UIO vs. kernel driver is purely a kernel decision which shouldn't
  require a DT change.

  The properties should be part of match data for a compatible string that
  needs them set. Or if they can be defined in a way that is actually a
  property of the h/w, then it would be acceptible. You'd still need to
  define compatible strings that the properties apply to.

Above, you have come up with a default set of property names which you
go looking for, and you expect a class of property, even if you don't
expect specific names. That constitutes a binding, and a poorly-defined
one at that.

Per my reading, Rob's point in https://lkml.org/lkml/2016/5/23/539 was
that providing the *values* as module parameters was fine, not providing
the *property names*.

> Link to original Jan's patchset is https://lkml.org/lkml/2016/5/17/141
> 
> We had two options:
> 1) Get details of "Dynamic regions" via module parameter but this
> would mean using same number and sizes of "Dynamic regions" for
> all UIO dmem devices.
> 2) Pass names of DT attributes used by UIO dmem driver as
> module parameters.
> 
> > DT bindings need documentation, but there was none as part of this series.
> >
> > I do not think this makes sense from a DT perspective.
> 
> As explained above, we are not fixing DT compatible string and
> DT attributes names for UIO dmem driver instead we pass all
> these as module parameters (preferable via kernel args or at
> time of module insertion). Due to this we don't require DT bindings
> document.

If you're looking for properties in the DT, you're assuming a binding of
some sort, regardless of whether you expect particular names.

To be clear, NAK to this as it stands.

Thanks,
Mark.

Re: [PATCH 7/8] uio: bind uio_dmem_genirq via OF

[Anup Patel]

On Fri, Jul 15, 2016 at 10:22 PM, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> On Fri, Jul 15, 2016 at 09:57:28PM +0530, Anup Patel wrote:
>> On Fri, Jul 15, 2016 at 6:58 PM, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
>> > On Fri, Jul 15, 2016 at 02:34:02PM +0530, Anup Patel wrote:
>> >> +static char uio_of_dma_bits_prop[128] = "uio,dma-bits";
>> >> +static char uio_of_dmem_count_prop[128] = "uio,number-of-dynamic-regions";
>> >> +static char uio_of_dmem_sizes_prop[128] = "uio,dynamic-regions-sizes";
>> >
>> > What are these proeprties? What is a "dynamic region" in hardware terms?
>>
>> "Dynamic region" is DMAble memory alloced using dma_alloc_xxx() APIs
>> for the device.
>>
>> For DMA-capable devices accessed from user-space, we need DMAble
>> memory available to user-space. Sometime such devices are also
>> cache-coherent (or IO-coherent) so in such case user-space will need
>> cacheable DMAble memory.
>>
>> Number of "Dynamic regions" required by UIO dmem device depends
>> on the type of HW device hence we describe number of "Dynamic regions"
>> and size of "Dynamic regions" via DT attributes.
>
> So this is something akin to the number of DMA channels a device might
> use.
>
> Either that's implicit knowledge of the device (if fixed), which doesn't
> need to be in the DT, or a real property that should be described
> explicitly in the device binding, that has nothing to do with UIO.
>
> In general, a device which you might want to use with UIO may not
> require such properties, so this is imposing a requirement on DTBs
> purely for the sake of userspace software.
>
>> > Why must they be in the DT, and why are the *property names* arbitrarily
>> > overridable as module parameters? What exactly do you expect there to be
>> > in a DT?
>>
>> They must be in DT for platform devices created using DT probing. The
>> number of "Dynamic regions" required by UIO device will depend on the
>> nature of device. We cannot have same number and sizes of "Dynamic
>> regions" for different UIO dmem devices.
>
> If the devices are so different, they should not be handled by the same
> driver that has no knowledge of those differences. Platform VFIO has the
> same problem, and requires device-specific reset drivers.
>
>> The UIO dmem driver is a generic driver and not specific to any type of
>> HW. In fact, UIO dmem driver is generic enough to access variety of
>> platform devices from user-space using UIO framework.
>>
>> Based on this Rob had previously suggested to not have any (or enforce
>> any) DT bindings for UIO dmem driver.
>>
>> Refer, https://lkml.org/lkml/2016/5/18/457
>
> I don't follow. Here Rob stated:
>
>   DT describes h/w. UIO is not a h/w block, so this does not belong in DT. A
>   UIO vs. kernel driver is purely a kernel decision which shouldn't
>   require a DT change.
>
>   The properties should be part of match data for a compatible string that
>   needs them set. Or if they can be defined in a way that is actually a
>   property of the h/w, then it would be acceptible. You'd still need to
>   define compatible strings that the properties apply to.
>
> Above, you have come up with a default set of property names which you
> go looking for, and you expect a class of property, even if you don't
> expect specific names. That constitutes a binding, and a poorly-defined
> one at that.
>
> Per my reading, Rob's point in https://lkml.org/lkml/2016/5/23/539 was
> that providing the *values* as module parameters was fine, not providing
> the *property names*.
>
>> Link to original Jan's patchset is https://lkml.org/lkml/2016/5/17/141
>>
>> We had two options:
>> 1) Get details of "Dynamic regions" via module parameter but this
>> would mean using same number and sizes of "Dynamic regions" for
>> all UIO dmem devices.
>> 2) Pass names of DT attributes used by UIO dmem driver as
>> module parameters.
>>
>> > DT bindings need documentation, but there was none as part of this series.
>> >
>> > I do not think this makes sense from a DT perspective.
>>
>> As explained above, we are not fixing DT compatible string and
>> DT attributes names for UIO dmem driver instead we pass all
>> these as module parameters (preferable via kernel args or at
>> time of module insertion). Due to this we don't require DT bindings
>> document.
>
> If you're looking for properties in the DT, you're assuming a binding of
> some sort, regardless of whether you expect particular names.
>
> To be clear, NAK to this as it stands.

I think if we are not enforcing any DT bindings by passing compatible
and property names as module parameters then we don't need any
DT bindings document. We can certainly remove default property
names and enforce UIO dmem users to always pass this as module
parameters.

Anyways, you have already NAKed this so I will just get all dynamic
region info from module parameters only.

Regards,
Anup
--
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