[PATCH] fbdev: simplefb: add support for 'memory-region' property on DT node

[PATCH] fbdev: simplefb: add support for 'memory-region' property on DT node

[Kunihiko Hayashi]

Enables 'memory-region' property referring to the memory description on
the reserved-memory node in case of devicetree use.
If there is no 'reg' property that specifies the address and size of
the framebuffer, the address and size written in the memory description
on the reserved-memory node can be used for the framebuffer.

Furthermore, the reserved-memory node needs to have "no-map" attributes
because simplefb driver maps the region by ioremap_wc().

Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
 .../bindings/display/simple-framebuffer.txt        |  3 ++
 drivers/video/fbdev/simplefb.c                     | 32 ++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.txt b/Documentation/devicetree/bindings/display/simple-framebuffer.txt
index 5a9ce51..be5139f 100644
--- a/Documentation/devicetree/bindings/display/simple-framebuffer.txt
+++ b/Documentation/devicetree/bindings/display/simple-framebuffer.txt
@@ -56,6 +56,9 @@ Optional properties:
   framebuffer remains active.
 
 - display : phandle pointing to the primary display hardware node
+- memory-region: phandle to a node describing memory region as framebuffer
+		 memory instead of reg property. The node should include
+		 'no-map'.
 
 Example:
 
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index a3c44ec..aefc4b1 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -29,6 +29,7 @@
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/of_platform.h>
 #include <linux/parser.h>
 #include <linux/regulator/consumer.h>
@@ -294,6 +295,35 @@ static void simplefb_clocks_enable(struct simplefb_par *par,
 static void simplefb_clocks_destroy(struct simplefb_par *par) { }
 #endif
 
+#if defined CONFIG_OF
+static struct resource *simplefb_parse_dt_reserved_mem(struct device *dev)
+{
+	static struct resource res;
+	struct device_node *np;
+	int ret;
+
+	np = of_parse_phandle(dev->of_node, "memory-region", 0);
+	if (!np)
+		return NULL;
+
+	ret = of_address_to_resource(np, 0, &res);
+	if (ret < 0)
+		return NULL;
+
+	if (!of_find_property(np, "no-map", NULL)) {
+		dev_err(dev, "Can't apply mapped reserved-memory\n");
+		return NULL;
+	}
+
+	return &res;
+}
+#else
+static struct resource *simplefb_parse_dt_reserved_mem(struct device *dev)
+{
+	return NULL;
+}
+#endif
+
 #if defined CONFIG_OF && defined CONFIG_REGULATOR
 
 #define SUPPLY_SUFFIX "-supply"
@@ -428,6 +458,8 @@ static int simplefb_probe(struct platform_device *pdev)
 		return ret;
 
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!mem)
+		mem = simplefb_parse_dt_reserved_mem(&pdev->dev);
 	if (!mem) {
 		dev_err(&pdev->dev, "No memory resource\n");
 		return -EINVAL;
-- 
2.7.4

Re: [PATCH] fbdev: simplefb: add support for 'memory-region' property on DT node

[Hans de Goede]

Hi,

On 23-01-18 12:34, Kunihiko Hayashi wrote:
> Enables 'memory-region' property referring to the memory description on
> the reserved-memory node in case of devicetree use.
> If there is no 'reg' property that specifies the address and size of
> the framebuffer, the address and size written in the memory description
> on the reserved-memory node can be used for the framebuffer.
> 
> Furthermore, the reserved-memory node needs to have "no-map" attributes
> because simplefb driver maps the region by ioremap_wc().
> 
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>

Seems reasonable to me:

Acked-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

> ---
>   .../bindings/display/simple-framebuffer.txt        |  3 ++
>   drivers/video/fbdev/simplefb.c                     | 32 ++++++++++++++++++++++
>   2 files changed, 35 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.txt b/Documentation/devicetree/bindings/display/simple-framebuffer.txt
> index 5a9ce51..be5139f 100644
> --- a/Documentation/devicetree/bindings/display/simple-framebuffer.txt
> +++ b/Documentation/devicetree/bindings/display/simple-framebuffer.txt
> @@ -56,6 +56,9 @@ Optional properties:
>     framebuffer remains active.
>   
>   - display : phandle pointing to the primary display hardware node
> +- memory-region: phandle to a node describing memory region as framebuffer
> +		 memory instead of reg property. The node should include
> +		 'no-map'.
>   
>   Example:
>   
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index a3c44ec..aefc4b1 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -29,6 +29,7 @@
>   #include <linux/clk.h>
>   #include <linux/clk-provider.h>
>   #include <linux/of.h>
> +#include <linux/of_address.h>
>   #include <linux/of_platform.h>
>   #include <linux/parser.h>
>   #include <linux/regulator/consumer.h>
> @@ -294,6 +295,35 @@ static void simplefb_clocks_enable(struct simplefb_par *par,
>   static void simplefb_clocks_destroy(struct simplefb_par *par) { }
>   #endif
>   
> +#if defined CONFIG_OF
> +static struct resource *simplefb_parse_dt_reserved_mem(struct device *dev)
> +{
> +	static struct resource res;
> +	struct device_node *np;
> +	int ret;
> +
> +	np = of_parse_phandle(dev->of_node, "memory-region", 0);
> +	if (!np)
> +		return NULL;
> +
> +	ret = of_address_to_resource(np, 0, &res);
> +	if (ret < 0)
> +		return NULL;
> +
> +	if (!of_find_property(np, "no-map", NULL)) {
> +		dev_err(dev, "Can't apply mapped reserved-memory\n");
> +		return NULL;
> +	}
> +
> +	return &res;
> +}
> +#else
> +static struct resource *simplefb_parse_dt_reserved_mem(struct device *dev)
> +{
> +	return NULL;
> +}
> +#endif
> +
>   #if defined CONFIG_OF && defined CONFIG_REGULATOR
>   
>   #define SUPPLY_SUFFIX "-supply"
> @@ -428,6 +458,8 @@ static int simplefb_probe(struct platform_device *pdev)
>   		return ret;
>   
>   	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!mem)
> +		mem = simplefb_parse_dt_reserved_mem(&pdev->dev);
>   	if (!mem) {
>   		dev_err(&pdev->dev, "No memory resource\n");
>   		return -EINVAL;
> 

Re: [PATCH] fbdev: simplefb: add support for 'memory-region' property on DT node

[Bartlomiej Zolnierkiewicz]

Hi,

On Tuesday, January 23, 2018 08:34:56 PM Kunihiko Hayashi wrote:
> Enables 'memory-region' property referring to the memory description on
> the reserved-memory node in case of devicetree use.
> If there is no 'reg' property that specifies the address and size of
> the framebuffer, the address and size written in the memory description
> on the reserved-memory node can be used for the framebuffer.
> 
> Furthermore, the reserved-memory node needs to have "no-map" attributes
> because simplefb driver maps the region by ioremap_wc().
> 
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>

This needs an ACK from Rob or Mark (DT bindings Maintainers).

> ---
>  .../bindings/display/simple-framebuffer.txt        |  3 ++
>  drivers/video/fbdev/simplefb.c                     | 32 ++++++++++++++++++++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.txt b/Documentation/devicetree/bindings/display/simple-framebuffer.txt
> index 5a9ce51..be5139f 100644
> --- a/Documentation/devicetree/bindings/display/simple-framebuffer.txt
> +++ b/Documentation/devicetree/bindings/display/simple-framebuffer.txt
> @@ -56,6 +56,9 @@ Optional properties:
>    framebuffer remains active.
>  
>  - display : phandle pointing to the primary display hardware node
> +- memory-region: phandle to a node describing memory region as framebuffer
> +		 memory instead of reg property. The node should include
> +		 'no-map'.
>  
>  Example:
>  
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index a3c44ec..aefc4b1 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -29,6 +29,7 @@
>  #include <linux/clk.h>
>  #include <linux/clk-provider.h>
>  #include <linux/of.h>
> +#include <linux/of_address.h>
>  #include <linux/of_platform.h>
>  #include <linux/parser.h>
>  #include <linux/regulator/consumer.h>
> @@ -294,6 +295,35 @@ static void simplefb_clocks_enable(struct simplefb_par *par,
>  static void simplefb_clocks_destroy(struct simplefb_par *par) { }
>  #endif
>  
> +#if defined CONFIG_OF
> +static struct resource *simplefb_parse_dt_reserved_mem(struct device *dev)
> +{
> +	static struct resource res;
> +	struct device_node *np;
> +	int ret;
> +
> +	np = of_parse_phandle(dev->of_node, "memory-region", 0);
> +	if (!np)
> +		return NULL;
> +
> +	ret = of_address_to_resource(np, 0, &res);
> +	if (ret < 0)
> +		return NULL;
> +
> +	if (!of_find_property(np, "no-map", NULL)) {
> +		dev_err(dev, "Can't apply mapped reserved-memory\n");
> +		return NULL;
> +	}
> +
> +	return &res;
> +}
> +#else
> +static struct resource *simplefb_parse_dt_reserved_mem(struct device *dev)
> +{
> +	return NULL;
> +}
> +#endif
> +
>  #if defined CONFIG_OF && defined CONFIG_REGULATOR
>  
>  #define SUPPLY_SUFFIX "-supply"
> @@ -428,6 +458,8 @@ static int simplefb_probe(struct platform_device *pdev)
>  		return ret;
>  
>  	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!mem)
> +		mem = simplefb_parse_dt_reserved_mem(&pdev->dev);
>  	if (!mem) {
>  		dev_err(&pdev->dev, "No memory resource\n");
>  		return -EINVAL;

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH] fbdev: simplefb: add support for 'memory-region' property on DT node

[Andy Shevchenko]

On Thu, Feb 1, 2018 at 5:56 PM, Bartlomiej Zolnierkiewicz
<b.zolnierkie@samsung.com> wrote:
> On Tuesday, January 23, 2018 08:34:56 PM Kunihiko Hayashi wrote:
>> Enables 'memory-region' property referring to the memory description on
>> the reserved-memory node in case of devicetree use.
>> If there is no 'reg' property that specifies the address and size of
>> the framebuffer, the address and size written in the memory description
>> on the reserved-memory node can be used for the framebuffer.
>>
>> Furthermore, the reserved-memory node needs to have "no-map" attributes
>> because simplefb driver maps the region by ioremap_wc().
>>
>> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>

>> +- memory-region: phandle to a node describing memory region as framebuffer
>> +              memory instead of reg property. The node should include
>> +              'no-map'.

>>       mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +     if (!mem)
>> +             mem = simplefb_parse_dt_reserved_mem(&pdev->dev);

I'm not sure I understood why you need this entire function?

Put your memory resource ('reg' property) as part of reserved memory
with necessary flags.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] fbdev: simplefb: add support for 'memory-region' property on DT node

[Kunihiko Hayashi]

Hi Andy,

On Thu, 1 Feb 2018 21:03:30 +0200 <andy.shevchenko@gmail.com> wrote:

> On Thu, Feb 1, 2018 at 5:56 PM, Bartlomiej Zolnierkiewicz
> <b.zolnierkie@samsung.com> wrote:
> > On Tuesday, January 23, 2018 08:34:56 PM Kunihiko Hayashi wrote:
> >> Enables 'memory-region' property referring to the memory description on
> >> the reserved-memory node in case of devicetree use.
> >> If there is no 'reg' property that specifies the address and size of
> >> the framebuffer, the address and size written in the memory description
> >> on the reserved-memory node can be used for the framebuffer.
> >>
> >> Furthermore, the reserved-memory node needs to have "no-map" attributes
> >> because simplefb driver maps the region by ioremap_wc().
> >>
> >> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> 
> >> +- memory-region: phandle to a node describing memory region as framebuffer
> >> +              memory instead of reg property. The node should include
> >> +              'no-map'.
> 
> >>       mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> +     if (!mem)
> >> +             mem = simplefb_parse_dt_reserved_mem(&pdev->dev);
> 
> I'm not sure I understood why you need this entire function?
> 
> Put your memory resource ('reg' property) as part of reserved memory
> with necessary flags.

Surely we prepare a memory resource as a part of reserved memory,
for example:

	reserved-memory {
		fb_area: memory@0xa0000000 {
			reg = <0xa0000000 0x400000>;
			no-map;
		};
	};

And we need to specify the address and size as a reg property in
the framebuffer node.

	framebuffer {
		compatible = "simple-framebuffer";
		reg = <0xa0000000 0x400000>;
	};

This function allows us to specify the area with phandle to
the reserved memory instead of same address and size.

	framebuffer {
		compatible = "simple-framebuffer";
		memory-region = <&fb_area>;
	};

If both reg and memory-region properties are specified
in the framebuffer node, the reg propery will be applied.

---
Best Regards,
Kunihiko Hayashi

Re: [PATCH] fbdev: simplefb: add support for 'memory-region' property on DT node

[Kunihiko Hayashi]

Hi Bartlomiej, Rob, Mark,

On Thu, 1 Feb 2018 16:56:08 +0100 <b.zolnierkie@samsung.com> wrote:
> 
> Hi,
> 
> On Tuesday, January 23, 2018 08:34:56 PM Kunihiko Hayashi wrote:
> > Enables 'memory-region' property referring to the memory description on
> > the reserved-memory node in case of devicetree use.
> > If there is no 'reg' property that specifies the address and size of
> > the framebuffer, the address and size written in the memory description
> > on the reserved-memory node can be used for the framebuffer.
> > 
> > Furthermore, the reserved-memory node needs to have "no-map" attributes
> > because simplefb driver maps the region by ioremap_wc().
> > 
> > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> 
> This needs an ACK from Rob or Mark (DT bindings Maintainers).

Thanks for pointing out.

Rob, Mark, would you please confirm the patch?
This patch contains the addition to dt-bindings.

---
Best Regards,
Kunihiko Hayashi

Re: [PATCH] fbdev: simplefb: add support for 'memory-region' property on DT node

[Rob Herring]

On Thu, Feb 01, 2018 at 04:56:08PM +0100, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Tuesday, January 23, 2018 08:34:56 PM Kunihiko Hayashi wrote:
> > Enables 'memory-region' property referring to the memory description on
> > the reserved-memory node in case of devicetree use.
> > If there is no 'reg' property that specifies the address and size of
> > the framebuffer, the address and size written in the memory description
> > on the reserved-memory node can be used for the framebuffer.
> > 
> > Furthermore, the reserved-memory node needs to have "no-map" attributes
> > because simplefb driver maps the region by ioremap_wc().
> > 
> > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> 
> This needs an ACK from Rob or Mark (DT bindings Maintainers).
> 
> > ---
> >  .../bindings/display/simple-framebuffer.txt        |  3 ++
> >  drivers/video/fbdev/simplefb.c                     | 32 ++++++++++++++++++++++
> >  2 files changed, 35 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.txt b/Documentation/devicetree/bindings/display/simple-framebuffer.txt
> > index 5a9ce51..be5139f 100644
> > --- a/Documentation/devicetree/bindings/display/simple-framebuffer.txt
> > +++ b/Documentation/devicetree/bindings/display/simple-framebuffer.txt
> > @@ -56,6 +56,9 @@ Optional properties:
> >    framebuffer remains active.
> >  
> >  - display : phandle pointing to the primary display hardware node
> > +- memory-region: phandle to a node describing memory region as framebuffer
> > +		 memory instead of reg property. The node should include
> > +		 'no-map'.

This should also state when it's appropriate to use this instead of reg. 
The memory would only be reclaimed if reg is used.

Though I'm wondering what keeps the simple fb memory from getting used 
by the OS if reserved memory is not always used.

Rob

Re: [PATCH] fbdev: simplefb: add support for 'memory-region' property on DT node

[Kunihiko Hayashi]

Hi Rob,

On Mon, 5 Feb 2018 00:09:14 -0600 <robh@kernel.org> wrote:

> On Thu, Feb 01, 2018 at 04:56:08PM +0100, Bartlomiej Zolnierkiewicz wrote:
> > 
> > Hi,
> > 
> > On Tuesday, January 23, 2018 08:34:56 PM Kunihiko Hayashi wrote:
> > > Enables 'memory-region' property referring to the memory description on
> > > the reserved-memory node in case of devicetree use.
> > > If there is no 'reg' property that specifies the address and size of
> > > the framebuffer, the address and size written in the memory description
> > > on the reserved-memory node can be used for the framebuffer.
> > > 
> > > Furthermore, the reserved-memory node needs to have "no-map" attributes
> > > because simplefb driver maps the region by ioremap_wc().
> > > 
> > > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> > 
> > This needs an ACK from Rob or Mark (DT bindings Maintainers).
> > 
> > > ---
> > >  .../bindings/display/simple-framebuffer.txt        |  3 ++
> > >  drivers/video/fbdev/simplefb.c                     | 32 ++++++++++++++++++++++
> > >  2 files changed, 35 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.txt b/Documentation/devicetree/bindings/display/simple-framebuffer.txt
> > > index 5a9ce51..be5139f 100644
> > > --- a/Documentation/devicetree/bindings/display/simple-framebuffer.txt
> > > +++ b/Documentation/devicetree/bindings/display/simple-framebuffer.txt
> > > @@ -56,6 +56,9 @@ Optional properties:
> > >    framebuffer remains active.
> > >  
> > >  - display : phandle pointing to the primary display hardware node
> > > +- memory-region: phandle to a node describing memory region as framebuffer
> > > +		 memory instead of reg property. The node should include
> > > +		 'no-map'.
> 
> This should also state when it's appropriate to use this instead of reg. 
> The memory would only be reclaimed if reg is used.
> 
> Though I'm wondering what keeps the simple fb memory from getting used 
> by the OS if reserved memory is not always used.

Sorry that my understanding might not be correct.

Since a framebuffer defined by "reserved-memory" can't be reclaimed,
I've assumed such the framebuffer was dedicated and had some restrictions
by device or firmware.

Even in that case, You mean that we describe only "reg" directly,
not go through "reserved-memory". Is that correct?

It might be better to define new driver handling the dedicated framebuffer
rather than using simple-fb, though, I'm not sure.

---
Best Regards,
Kunihiko Hayashi