| Message ID | 1392985527-6260-7-git-send-email-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 21 Feb 2014 13:25:22 +0100, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > Use recently introduced of_reserved_mem_device_init() function to > automatically assign respective reserved memory region to the newly created > platform and amba device. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> I'm wary on this patch. It hides the assignment of regions into the core code and I worry that it is the wrong level of abstraction. I would think that drivers should know that they need a reserved memory region and should be calling the API to obtain the region directly rather than doing it for them. The reason being is that there may be some situations where the common code isn't quite right and the driver needs to override the behaviour. If it is called automatically then the driver cannot do that. Is it really a big burden to have the driver call the reserved memory init function? g. > --- > drivers/of/platform.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c > index 404d1daebefa..3df0b1826e8b 100644 > --- a/drivers/of/platform.c > +++ b/drivers/of/platform.c > @@ -21,6 +21,7 @@ > #include <linux/of_device.h> > #include <linux/of_irq.h> > #include <linux/of_platform.h> > +#include <linux/of_reserved_mem.h> > #include <linux/platform_device.h> > > const struct of_device_id of_default_bus_match_table[] = { > @@ -220,6 +221,8 @@ static struct platform_device *of_platform_device_create_pdata( > dev->dev.bus = &platform_bus_type; > dev->dev.platform_data = platform_data; > > + of_reserved_mem_device_init(&dev->dev); > + > /* We do not fill the DMA ops for platform devices by default. > * This is currently the responsibility of the platform code > * to do such, possibly using a device notifier > @@ -227,6 +230,7 @@ static struct platform_device *of_platform_device_create_pdata( > > if (of_device_add(dev) != 0) { > platform_device_put(dev); > + of_reserved_mem_device_release(&dev->dev); > return NULL; > } > > @@ -282,6 +286,8 @@ static struct amba_device *of_amba_device_create(struct device_node *node, > else > of_device_make_bus_id(&dev->dev); > > + of_reserved_mem_device_init(&dev->dev); > + > /* Allow the HW Peripheral ID to be overridden */ > prop = of_get_property(node, "arm,primecell-periphid", NULL); > if (prop) > @@ -308,6 +314,7 @@ static struct amba_device *of_amba_device_create(struct device_node *node, > return dev; > > err_free: > + of_reserved_mem_device_release(&dev->dev); > amba_device_put(dev); > return NULL; > } > -- > 1.7.9.5 >
Hello, On 2014-02-26 13:14, Grant Likely wrote: > On Fri, 21 Feb 2014 13:25:22 +0100, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > Use recently introduced of_reserved_mem_device_init() function to > > automatically assign respective reserved memory region to the newly created > > platform and amba device. > > > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > > I'm wary on this patch. It hides the assignment of regions into the core > code and I worry that it is the wrong level of abstraction. I would > think that drivers should know that they need a reserved memory region > and should be calling the API to obtain the region directly rather than > doing it for them. The reason being is that there may be some situations > where the common code isn't quite right and the driver needs to override > the behaviour. If it is called automatically then the driver cannot do > that. > > Is it really a big burden to have the driver call the reserved memory > init function? If a device requires very special handling of the reserved memory region, it may simply provide its own reserved memory region driver which will do the required early initialization. If we assume that driver needs to initialize reserved region manually, then why do we ever bother with adding support for custom reserved memory drivers and assigning regions to a device node? We can simply stick with just a set of reserved regions and tell drivers to use them. In my opinion for most typical use cases a board designer will assign 'dma-shared-pool' driver to the given set of devices, which in turn ensures that all memory allocations for dma purposes for those device will be served from that region. It is really not a driver role to initialize it in such case. Driver should focus on controlling hw operations, regardless the way the hardware module has been integrated to the system. I can perfectly imagine a generic driver which operates the same way in any of the following cases (depends mainly on the hw version): 1) restricted dma window, 2) iommu for all dma for the given device, 3) fully featured memory master for dma for the given device (no restrictions), if the respective kernel subsystems did the correct initialization and provide their own methods for managing DMA operation. I already have a working platform glue code for the above cases tested with Samsung multimedia drivers. No changes to the drivers were required. Best regards
On Thu, 27 Feb 2014 11:10:44 +0100, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > Hello, > > On 2014-02-26 13:14, Grant Likely wrote: > > On Fri, 21 Feb 2014 13:25:22 +0100, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > > Use recently introduced of_reserved_mem_device_init() function to > > > automatically assign respective reserved memory region to the newly created > > > platform and amba device. > > > > > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > > > > I'm wary on this patch. It hides the assignment of regions into the core > > code and I worry that it is the wrong level of abstraction. I would > > think that drivers should know that they need a reserved memory region > > and should be calling the API to obtain the region directly rather than > > doing it for them. The reason being is that there may be some situations > > where the common code isn't quite right and the driver needs to override > > the behaviour. If it is called automatically then the driver cannot do > > that. > > > > Is it really a big burden to have the driver call the reserved memory > > init function? > > If a device requires very special handling of the reserved memory region, > it may simply provide its own reserved memory region driver which will do > the required early initialization. > > If we assume that driver needs to initialize reserved region manually, then > why do we ever bother with adding support for custom reserved memory drivers > and assigning regions to a device node? We can simply stick with just a set > of reserved regions and tell drivers to use them. > > In my opinion for most typical use cases a board designer will assign > 'dma-shared-pool' driver to the given set of devices, which in turn ensures > that all memory allocations for dma purposes for those device will be > served from that region. It is really not a driver role to initialize it > in such case. Driver should focus on controlling hw operations, regardless > the way the hardware module has been integrated to the system. > > I can perfectly imagine a generic driver which operates the same way in any > of the following cases (depends mainly on the hw version): 1) restricted > dma window, 2) iommu for all dma for the given device, 3) fully featured > memory master for dma for the given device (no restrictions), if the > respective kernel subsystems did the correct initialization and provide > their own methods for managing DMA operation. I already have a working > platform glue code for the above cases tested with Samsung multimedia > drivers. No changes to the drivers were required. I don't have issue with the above. What I do have issue with is that by putting the call into core code, it means this binding gets applied to every platform devices, regardless of whether or not it makes sense. Yes of course there should be a common function for setting it up, and of_reserved_mem_device_init() looks about right. However, I strongly think that the setup call should be performed by the driver's .probe hook. I don't want it in of_platform_device_create_pdata(). g.
diff --git a/drivers/of/platform.c b/drivers/of/platform.c index 404d1daebefa..3df0b1826e8b 100644 --- a/drivers/of/platform.c +++ b/drivers/of/platform.c @@ -21,6 +21,7 @@ #include <linux/of_device.h> #include <linux/of_irq.h> #include <linux/of_platform.h> +#include <linux/of_reserved_mem.h> #include <linux/platform_device.h> const struct of_device_id of_default_bus_match_table[] = { @@ -220,6 +221,8 @@ static struct platform_device *of_platform_device_create_pdata( dev->dev.bus = &platform_bus_type; dev->dev.platform_data = platform_data; + of_reserved_mem_device_init(&dev->dev); + /* We do not fill the DMA ops for platform devices by default. * This is currently the responsibility of the platform code * to do such, possibly using a device notifier @@ -227,6 +230,7 @@ static struct platform_device *of_platform_device_create_pdata( if (of_device_add(dev) != 0) { platform_device_put(dev); + of_reserved_mem_device_release(&dev->dev); return NULL; } @@ -282,6 +286,8 @@ static struct amba_device *of_amba_device_create(struct device_node *node, else of_device_make_bus_id(&dev->dev); + of_reserved_mem_device_init(&dev->dev); + /* Allow the HW Peripheral ID to be overridden */ prop = of_get_property(node, "arm,primecell-periphid", NULL); if (prop) @@ -308,6 +314,7 @@ static struct amba_device *of_amba_device_create(struct device_node *node, return dev; err_free: + of_reserved_mem_device_release(&dev->dev); amba_device_put(dev); return NULL; }
Use recently introduced of_reserved_mem_device_init() function to automatically assign respective reserved memory region to the newly created platform and amba device. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- drivers/of/platform.c | 7 +++++++ 1 file changed, 7 insertions(+)