* Re: [PATCH 1/7] ARM: dts: sunxi: Add regulator-boot-on property to ahci-5v regulator
From: Maxime Ripard @ 2014-11-13 16:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1415871109-28332-1-git-send-email-hdegoede@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 604 bytes --]
On Thu, Nov 13, 2014 at 10:31:43AM +0100, Hans de Goede wrote:
> This avoids it getting briefly turned off between when the regulator getting
> registered and the ahci driver turning it back on, thus avoiding the disk
> going into emergency head park mode.
>
> Cc: Bruno Prémont <bonbons@linux-vserver.org>
> Reported-and-tested-by: Bruno Prémont <bonbons@linux-vserver.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Applied, and split up the tags.
Thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 4/7] ARM: dts: sun6i: Add simplefb node
From: Hans de Goede @ 2014-11-13 17:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20141113164606.GM20972@lukather>
Hi,
On 11/13/2014 05:46 PM, Maxime Ripard wrote:
> Hi,
>
> On Thu, Nov 13, 2014 at 10:31:46AM +0100, Hans de Goede wrote:
>> Add a simplefb template node for u-boot to further fill and activate.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> arch/arm/boot/dts/sun6i-a31.dtsi | 10 ++++++++++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
>> index 543f895..438952e 100644
>> --- a/arch/arm/boot/dts/sun6i-a31.dtsi
>> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
>> @@ -62,6 +62,16 @@
>> ethernet0 = &gmac;
>> };
>>
>> + chosen {
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + ranges;
>> +
>> + framebuffer0 {
>> + compatible = "simple-framebuffer";
>> + status = "disabled";
>
> Aren't we suppose to have clocks in there too?
I deliberately left them out as they are not needed on sun6i, once the blocks
are setup ahb access is not necessary, but it seems that the ahb_gate bits
in sun4i / sun5i / sun7i double as a reset. Toggling them on / off / on breaks
the mode, while toggling module clocks on / off / on only causes a glitch.
Since the sun6i has a separate reset controller, the clock gates seem to really
only control ahb access which is not necessary once the mode has been set up.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH v5 0/5] simplefb: add clock handling code
From: Hans de Goede @ 2014-11-13 18:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5464CC5A.7010508@ti.com>
Hi,
On 11/13/2014 04:20 PM, Tomi Valkeinen wrote:
> On 12/11/14 18:15, Hans de Goede wrote:
>> Hi Tomi,
>>
>> Here is v5 of my simplefb patch-set, this time with an Ack from
>> Grant Likely for the dt-bindings bits, which should end all controversy
>> surrounding that patch.
>>
>> For changes since the original postings please see the per patch changelogs
>> in the patch commit messages.
>>
>> Please merge this into your tree for 3.19.
>
> I applied this and the "dt-bindings: simplefb: Specify node location and
> handoff related properties" v3 series to my test branch, compiled for
> x64 and:
>
> drivers/video/fbdev/simplefb.c: In function ‘simplefb_clocks_init’:
> drivers/video/fbdev/simplefb.c:204:2: error: implicit declaration of
> function ‘of_clk_get_parent_count’ [-Werror=implicit-function-declaration]
> par->clk_count = of_clk_get_parent_count(np);
> ^
> In file included from include/linux/i2c.h:33:0,
> from include/uapi/linux/fb.h:5,
> from include/linux/fb.h:5,
> from drivers/video/fbdev/simplefb.c:24:
> drivers/video/fbdev/simplefb.c: In function ‘simplefb_init’:
> drivers/video/fbdev/simplefb.c:399:25: error: ‘of_chosen’ undeclared
> (first use in this function)
> for_each_child_of_node(of_chosen, np) {
> ^
> include/linux/of.h:736:33: note: in definition of macro
> ‘for_each_child_of_node’
> for (child = of_get_next_child(parent, NULL); child != NULL; \
> ^
> drivers/video/fbdev/simplefb.c:399:25: note: each undeclared identifier
> is reported only once for each function it appears in
> for_each_child_of_node(of_chosen, np) {
> ^
> include/linux/of.h:736:33: note: in definition of macro
> ‘for_each_child_of_node’
> for (child = of_get_next_child(parent, NULL); child != NULL; \
> ^
>
> We need "depends on OF"?
Good catch!
So a quick-grep through the entire kernel sources has thought me that
apparently simplefb is used on non devicetree / of platforms through
platform-device instantiation, using the trick of naming the platform-device the
same as the driver to get the driver to bind.
So the answer to your compile breakage is to make the relevant code bits
be #ifdef CONFIG_OF
I'll respin the offending patch (5/5) in the "simplefb: add clock handling code"
series, and I'll respin the entire second series, as I need to also address
your comment on the example in the bindings there.
I hope to be able to post a re-spun version of both later tonight.
Regards,
Hans
>
> Tomi
>
>
^ permalink raw reply
* Re: [PATCH 4/4] arm: dts: omap3-gta04: Add static configuration for devconf1 register
From: Tony Lindgren @ 2014-11-13 18:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5464969F.30708@ti.com>
* Tomi Valkeinen <tomi.valkeinen@ti.com> [141113 03:33]:
> On 12/11/14 17:02, Tony Lindgren wrote:
>
> >> And, with a quick grep, I see CONTROL_DEVCONF1 touched in multiple
> >> places in the kernel. I wonder if adding a pinmux entry for it could
> >> cause some rather odd problems.
> >
> > They can all use pinctrl-single no problem.
>
> Can, but don't. That's my worry. If we touch the DEVCONF1 via pinmux,
> and we have code in mach-omap2 that also touch DEVCONF1, without any
> knowledge (and locking) between those...
Hmm yeah the McBSP clock mux could be racy as the mux register for
McBSP is treated as a clock. This register muxes the clock between
external pin and internal clock. Considering that this should be
selectable at board level as the external clock probably needs to be
used if level shifters are being used, it should be really handled by
pinctrl-single.
The other use for hsmmc.c and pdata-quirks.c for the one time mux for
MMC clock from the MMC clock pin. That can be done with pinctrl-single
from the MMC driver too for DT based booting.
Then we just have the save and restore of the registers for
off-idle.
> So _maybe_ that's not an issue, as the pinmux config we have here is
> fixed, and done once at boot time, and maybe the code in mach-omap2 that
> touch DEVCONF1 is also ran just once and not at the same time as the
> pinmux. But I don't know if that's so.
It seems we could just do a read-only check for McBSP in the clock
code for the mux register, or even completely drop that code from
cclock3xxx_data.c and start using the pinctrl for that mux.
Paul & Tero, got any comments here?
Regards,
Tony
^ permalink raw reply
* [PATCH v6] simplefb: add clock handling code
From: Hans de Goede @ 2014-11-13 19:22 UTC (permalink / raw)
To: linux-arm-kernel
From: Luc Verhaegen <libv@skynet.be>
This claims and enables clocks listed in the simple framebuffer dt node.
This is needed so that the display engine, in case the required clocks
are known by the kernel code and are described in the dt, will remain
properly enabled.
Signed-off-by: Luc Verhaegen <libv@skynet.be>
[hdegoede@redhat.com: Change clks from list to dynamic array]
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
Reviewed-by: Maxime Ripard <maxime.ripard@free-electrons.com>
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Acked-by: Grant Likely <grant.likely@linaro.org>
--
Changes in v4:
-change clks from linkedlist to dynamic allocated array
-propagate EPROBE_DEFER up from simplefb_clocks_init to simplefb_probe
changes in v6:
-Wrap the clock code in #ifdef CONFIG_OF, as simplefb is used on non
devicetree platforms too
---
drivers/video/fbdev/simplefb.c | 106 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 105 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index cdcf1fe..868099a 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -26,6 +26,7 @@
#include <linux/module.h>
#include <linux/platform_data/simplefb.h>
#include <linux/platform_device.h>
+#include <linux/clk-provider.h>
static struct fb_fix_screeninfo simplefb_fix = {
.id = "simple",
@@ -167,8 +168,103 @@ static int simplefb_parse_pd(struct platform_device *pdev,
struct simplefb_par {
u32 palette[PSEUDO_PALETTE_SIZE];
+#ifdef CONFIG_OF
+ int clk_count;
+ struct clk **clks;
+#endif
};
+/*
+ * Clock handling code.
+ *
+ * Here we handle the clocks property of our "simple-framebuffer" dt node.
+ * This is necessary so that we can make sure that any clocks needed by
+ * the display engine that the bootloader set up for us (and for which it
+ * provided a simplefb dt node), stay up, for the life of the simplefb
+ * driver.
+ *
+ * When the driver unloads, we cleanly disable, and then release the clocks.
+ *
+ * We only complain about errors here, no action is taken as the most likely
+ * error can only happen due to a mismatch between the bootloader which set
+ * up simplefb, and the clock definitions in the device tree. Chances are
+ * that there are no adverse effects, and if there are, a clean teardown of
+ * the fb probe will not help us much either. So just complain and carry on,
+ * and hope that the user actually gets a working fb at the end of things.
+ */
+static int
+simplefb_clocks_init(struct simplefb_par *par, struct platform_device *pdev)
+{
+#ifdef CONFIG_OF
+ struct device_node *np = pdev->dev.of_node;
+ struct clk *clock;
+ int i, ret;
+
+ if (dev_get_platdata(&pdev->dev) || !np)
+ return 0;
+
+ par->clk_count = of_clk_get_parent_count(np);
+ if (par->clk_count <= 0)
+ return 0;
+
+ par->clks = kcalloc(par->clk_count, sizeof(struct clk *), GFP_KERNEL);
+ if (!par->clks)
+ return -ENOMEM;
+
+ for (i = 0; i < par->clk_count; i++) {
+ clock = of_clk_get(np, i);
+ if (IS_ERR(clock)) {
+ if (PTR_ERR(clock) = -EPROBE_DEFER) {
+ while (--i >= 0) {
+ if (par->clks[i])
+ clk_put(par->clks[i]);
+ }
+ kfree(par->clks);
+ return -EPROBE_DEFER;
+ }
+ dev_err(&pdev->dev, "%s: clock %d not found: %ld\n",
+ __func__, i, PTR_ERR(clock));
+ continue;
+ }
+ par->clks[i] = clock;
+ }
+
+ for (i = 0; i < par->clk_count; i++) {
+ if (par->clks[i]) {
+ ret = clk_prepare_enable(par->clks[i]);
+ if (ret) {
+ dev_err(&pdev->dev,
+ "%s: failed to enable clock %d: %d\n",
+ __func__, i, ret);
+ clk_put(par->clks[i]);
+ par->clks[i] = NULL;
+ }
+ }
+ }
+#endif
+ return 0;
+}
+
+static void
+simplefb_clocks_destroy(struct simplefb_par *par)
+{
+#ifdef CONFIG_OF
+ int i;
+
+ if (!par->clks)
+ return;
+
+ for (i = 0; i < par->clk_count; i++) {
+ if (par->clks[i]) {
+ clk_disable_unprepare(par->clks[i]);
+ clk_put(par->clks[i]);
+ }
+ }
+
+ kfree(par->clks);
+#endif
+}
+
static int simplefb_probe(struct platform_device *pdev)
{
int ret;
@@ -236,6 +332,10 @@ static int simplefb_probe(struct platform_device *pdev)
}
info->pseudo_palette = par->palette;
+ ret = simplefb_clocks_init(par, pdev);
+ if (ret < 0)
+ goto error_unmap;
+
dev_info(&pdev->dev, "framebuffer at 0x%lx, 0x%x bytes, mapped to 0x%p\n",
info->fix.smem_start, info->fix.smem_len,
info->screen_base);
@@ -247,13 +347,15 @@ static int simplefb_probe(struct platform_device *pdev)
ret = register_framebuffer(info);
if (ret < 0) {
dev_err(&pdev->dev, "Unable to register simplefb: %d\n", ret);
- goto error_unmap;
+ goto error_clocks;
}
dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);
return 0;
+error_clocks:
+ simplefb_clocks_destroy(par);
error_unmap:
iounmap(info->screen_base);
error_fb_release:
@@ -264,8 +366,10 @@ error_fb_release:
static int simplefb_remove(struct platform_device *pdev)
{
struct fb_info *info = platform_get_drvdata(pdev);
+ struct simplefb_par *par = info->par;
unregister_framebuffer(info);
+ simplefb_clocks_destroy(par);
framebuffer_release(info);
return 0;
--
2.1.0
^ permalink raw reply related
* [PATCH v4 1/4] dt-bindings: simplefb: Specify node location and handoff related properties
From: Hans de Goede @ 2014-11-13 19:23 UTC (permalink / raw)
To: linux-arm-kernel
Since simplefb nodes do not relate directly to hw typically they have been
placed in the root of the devicetree. As the represent runtime information
having them as sub-nodes of /chosen is more logical, specify this.
Also specify when to set the chosen stdout-path property to a simplefb node.
For reliable handover to a hardware specific driver, that driver needs to
know which simplefb to unregister when taking over, specify how the hw driver
can find the matching simplefb node.
Last add some advice on how to fill and use simplefb nodes from a firmware
pov.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
--
Changes in v2:
-Add stdout-path to the example code
Changes in v3:
-Specify that the node name must be "framebuffer@<address>"
-Specify that the link to link simplefb node and display hw nodes to one
another for handover goes into the simplefb node, and must be called "display"
-Specify how aliases may be used to tell the OS how to number displays
-Update the example to reflect these changes
Changes in v4:
-Keep the size of the reg property in the example as it was before
---
.../bindings/video/simple-framebuffer.txt | 49 +++++++++++++++++++++-
1 file changed, 48 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
index 8f35718..c548f33 100644
--- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
+++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
@@ -4,6 +4,31 @@ A simple frame-buffer describes a frame-buffer setup by firmware or
the bootloader, with the assumption that the display hardware has already
been set up to scan out from the memory pointed to by the reg property.
+Since simplefb nodes represent runtime information they must be sub-nodes of
+the chosen node (*). Simplefb nodes must be named "framebuffer@<address>".
+
+If a simplefb node represents the preferred console for user interaction,
+then the chosen node's stdout-path property must point to it.
+
+If the devicetree contains nodes for the display hardware used by a simplefb,
+then the simplefb node must contain a property called "display", which
+contains a phandle pointing to the primary display hw node, so that the OS
+knows which simplefb to disable when handing over control to a driver for the
+real hardware. The bindings for the hw nodes must specify which node is
+considered the primary node.
+
+It is advised to add display# aliases to help the OS determine how to number
+things. If display# aliases are used, then if the simplefb node contains a
+"display" property then the /aliases/display# path must point to the display
+hw node the "display" property points to, otherwise it must point directly
+to the simplefb node.
+
+It is advised that devicetree files contain pre-filled, disabled framebuffer
+nodes, so that the firmware only needs to update the mode information and
+enable them. This way if e.g. later on support for more display clocks get
+added, the simplefb nodes will already contain this info and the firmware
+does not need to be updated.
+
Required properties:
- compatible: "simple-framebuffer"
- reg: Should contain the location and size of the framebuffer memory.
@@ -19,14 +44,36 @@ Optional properties:
are expected to already be configured correctly. The OS must
ensure these clocks are not modified or disabled while the
simple framebuffer remains active.
+- display : phandle pointing to the primary display hardware node
Example:
- framebuffer {
+aliases {
+ display0 = &lcdc0;
+}
+
+chosen {
+ framebuffer0: framebuffer@1d385000 {
compatible = "simple-framebuffer";
reg = <0x1d385000 (1600 * 1200 * 2)>;
width = <1600>;
height = <1200>;
stride = <(1600 * 2)>;
format = "r5g6b5";
+ clocks = <&ahb_gates 36>, <&ahb_gates 43>, <&ahb_gates 44>;
+ display = <&lcdc0>;
+ };
+ stdout-path = &framebuffer0;
+};
+
+soc@01c00000 {
+ lcdc0: lcdc@1c0c000 {
+ compatible = "allwinner,sun4i-a10-lcdc";
+ ...
};
+};
+
+
+*) Older devicetree files may have a compatible = "simple-framebuffer" node
+in a different place, operating systems must first enumerate any compatible
+nodes found under chosen and then check for other compatible nodes.
--
2.1.0
^ permalink raw reply related
* [PATCH v4 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen
From: Hans de Goede @ 2014-11-13 19:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1415906594-2461-1-git-send-email-hdegoede@redhat.com>
Update simplefb to support the new preferred location for simplefb dt nodes
under /chosen.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
--
Changes in v2:
-Make name array larger in case we ever encounter more then 10000 framebuffers
Changes in v3:
-Switch to for_each_child_of_node to loop over the nodes under chosen
Changes in v4:
-Wrap the chosen enumeration code in #ifdef CONFIG_OF, as simplefb is used on
non devicetree platforms too
-simplefb can only be built-in, so drop the module_exit function calling
platform_driver_unregister
---
drivers/video/fbdev/simplefb.c | 23 ++++++++++++++++++++++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index 868099a..e381a53 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -27,6 +27,7 @@
#include <linux/platform_data/simplefb.h>
#include <linux/platform_device.h>
#include <linux/clk-provider.h>
+#include <linux/of_platform.h>
static struct fb_fix_screeninfo simplefb_fix = {
.id = "simple",
@@ -390,7 +391,27 @@ static struct platform_driver simplefb_driver = {
.probe = simplefb_probe,
.remove = simplefb_remove,
};
-module_platform_driver(simplefb_driver);
+
+static int __init simplefb_init(void)
+{
+ int ret;
+ struct device_node __maybe_unused *np;
+
+ ret = platform_driver_register(&simplefb_driver);
+ if (ret)
+ return ret;
+
+#ifdef CONFIG_OF
+ for_each_child_of_node(of_chosen, np) {
+ if (of_device_is_compatible(np, "simple-framebuffer"))
+ of_platform_device_create(np, NULL, NULL);
+ }
+#endif
+
+ return 0;
+}
+
+module_init(simplefb_init);
MODULE_AUTHOR("Stephen Warren <swarren@wwwdotorg.org>");
MODULE_DESCRIPTION("Simple framebuffer driver");
--
2.1.0
^ permalink raw reply related
* [PATCH v4 3/4] fbcon: Change fbcon_init from module_init to fs_initcall
From: Hans de Goede @ 2014-11-13 19:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1415906594-2461-1-git-send-email-hdegoede@redhat.com>
Various fb drivers register themselves before module_init so as to have a
console as early as possible, this is of little use if fbcon does not
initialize early too.
Fbcon cannot initialize earlier then fs_initcall, because then the creation
of /sys/class/graphics/fbcon will fail.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/video/console/fbcon.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/console/fbcon.c b/drivers/video/console/fbcon.c
index eb976ee..ea43724 100644
--- a/drivers/video/console/fbcon.c
+++ b/drivers/video/console/fbcon.c
@@ -3624,7 +3624,7 @@ static int __init fb_console_init(void)
return 0;
}
-module_init(fb_console_init);
+fs_initcall(fb_console_init);
#ifdef MODULE
--
2.1.0
^ permalink raw reply related
* [PATCH v4 4/4] simplefb: Change simplefb_init from module_init to fs_initcall
From: Hans de Goede @ 2014-11-13 19:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1415906594-2461-1-git-send-email-hdegoede@redhat.com>
One of the reasons for having the simplefb nodes in /chosen, and doing
explicit enumeration of the nodes there, is too allow enumerating them sooner,
so that we get a console earlier on.
Doing this earlier then fs_initcall is not useful, since the fb only turns
into a console when fbcon intializes, which is a fs_initcall too.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
--
Changes in v4 (of the patch-set):
-Drop the comment about using fs_initcall vs module_exit, as we no longer have
a module_exit
---
drivers/video/fbdev/simplefb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
index e381a53..1fc973c 100644
--- a/drivers/video/fbdev/simplefb.c
+++ b/drivers/video/fbdev/simplefb.c
@@ -411,7 +411,7 @@ static int __init simplefb_init(void)
return 0;
}
-module_init(simplefb_init);
+fs_initcall(simplefb_init);
MODULE_AUTHOR("Stephen Warren <swarren@wwwdotorg.org>");
MODULE_DESCRIPTION("Simple framebuffer driver");
--
2.1.0
^ permalink raw reply related
* Re: [PATCH v6] simplefb: add clock handling code
From: Geert Uytterhoeven @ 2014-11-13 19:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1415906546-2408-1-git-send-email-hdegoede@redhat.com>
On Thu, Nov 13, 2014 at 8:22 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> changes in v6:
> -Wrap the clock code in #ifdef CONFIG_OF, as simplefb is used on non
> devicetree platforms too
With clocks?
If yes, "clk_get(&pdev->dev, NULL)" can return a clock...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH 4/4] arm: dts: omap3-gta04: Add static configuration for devconf1 register
From: Paul Walmsley @ 2014-11-13 22:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20141113182848.GQ26481@atomide.com>
Hi
On Thu, 13 Nov 2014, Tony Lindgren wrote:
> * Tomi Valkeinen <tomi.valkeinen@ti.com> [141113 03:33]:
> > On 12/11/14 17:02, Tony Lindgren wrote:
> >
> > >> And, with a quick grep, I see CONTROL_DEVCONF1 touched in multiple
> > >> places in the kernel. I wonder if adding a pinmux entry for it could
> > >> cause some rather odd problems.
> > >
> > > They can all use pinctrl-single no problem.
> >
> > Can, but don't. That's my worry. If we touch the DEVCONF1 via pinmux,
> > and we have code in mach-omap2 that also touch DEVCONF1, without any
> > knowledge (and locking) between those...
>
> Hmm yeah the McBSP clock mux could be racy as the mux register for
> McBSP is treated as a clock. This register muxes the clock between
> external pin and internal clock. Considering that this should be
> selectable at board level as the external clock probably needs to be
> used if level shifters are being used, it should be really handled by
> pinctrl-single.
>
> The other use for hsmmc.c and pdata-quirks.c for the one time mux for
> MMC clock from the MMC clock pin. That can be done with pinctrl-single
> from the MMC driver too for DT based booting.
>
> Then we just have the save and restore of the registers for
> off-idle.
>
> > So _maybe_ that's not an issue, as the pinmux config we have here is
> > fixed, and done once at boot time, and maybe the code in mach-omap2 that
> > touch DEVCONF1 is also ran just once and not at the same time as the
> > pinmux. But I don't know if that's so.
>
> It seems we could just do a read-only check for McBSP in the clock
> code for the mux register, or even completely drop that code from
> cclock3xxx_data.c and start using the pinctrl for that mux.
>
> Paul & Tero, got any comments here?
It's best to move all of the SCM register reads/writes to an SCM IP block
driver. This driver would be the only entity that would touch the SCM IP
block registers - no other code on the system would touch it (perhaps
aside from anything needed for early init). The SCM driver would enforce
mutual exclusion via a spinlock, so concurrent SCM register modifications
wouldn't flake out. Then the SCM driver would register clocks with the
CCF, register pins with the pinctrl subsystem, etc. etc.
- Paul
^ permalink raw reply
* Re: [PATCH 4/4] arm: dts: omap3-gta04: Add static configuration for devconf1 register
From: Tony Lindgren @ 2014-11-13 23:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <alpine.DEB.2.02.1411132251320.17030@utopia.booyaka.com>
* Paul Walmsley <paul@pwsan.com> [141113 15:01]:
> Hi
>
> On Thu, 13 Nov 2014, Tony Lindgren wrote:
>
> > * Tomi Valkeinen <tomi.valkeinen@ti.com> [141113 03:33]:
> > > On 12/11/14 17:02, Tony Lindgren wrote:
> > >
> > > >> And, with a quick grep, I see CONTROL_DEVCONF1 touched in multiple
> > > >> places in the kernel. I wonder if adding a pinmux entry for it could
> > > >> cause some rather odd problems.
> > > >
> > > > They can all use pinctrl-single no problem.
> > >
> > > Can, but don't. That's my worry. If we touch the DEVCONF1 via pinmux,
> > > and we have code in mach-omap2 that also touch DEVCONF1, without any
> > > knowledge (and locking) between those...
> >
> > Hmm yeah the McBSP clock mux could be racy as the mux register for
> > McBSP is treated as a clock. This register muxes the clock between
> > external pin and internal clock. Considering that this should be
> > selectable at board level as the external clock probably needs to be
> > used if level shifters are being used, it should be really handled by
> > pinctrl-single.
> >
> > The other use for hsmmc.c and pdata-quirks.c for the one time mux for
> > MMC clock from the MMC clock pin. That can be done with pinctrl-single
> > from the MMC driver too for DT based booting.
> >
> > Then we just have the save and restore of the registers for
> > off-idle.
> >
> > > So _maybe_ that's not an issue, as the pinmux config we have here is
> > > fixed, and done once at boot time, and maybe the code in mach-omap2 that
> > > touch DEVCONF1 is also ran just once and not at the same time as the
> > > pinmux. But I don't know if that's so.
> >
> > It seems we could just do a read-only check for McBSP in the clock
> > code for the mux register, or even completely drop that code from
> > cclock3xxx_data.c and start using the pinctrl for that mux.
> >
> > Paul & Tero, got any comments here?
>
> It's best to move all of the SCM register reads/writes to an SCM IP block
> driver. This driver would be the only entity that would touch the SCM IP
> block registers - no other code on the system would touch it (perhaps
> aside from anything needed for early init). The SCM driver would enforce
> mutual exclusion via a spinlock, so concurrent SCM register modifications
> wouldn't flake out. Then the SCM driver would register clocks with the
> CCF, register pins with the pinctrl subsystem, etc. etc.
We actually do have that with pinctrl-single + syscon. We certainly
need to implement more Linux framework drivers for the SCM registers.
Things like regulators, clocks, and PHYs, but they should use
pinctrl-single + syscon. See the the pbias-regulator.c for example.
Looking at the McBSP clock handling, threre's yet more handling of
the same DEVCONF1 mux register in omap2_mcbsp_set_clks_src that gets
alled from omap_mcbsp_dai_set_dai_sysclk.
To me it seems that if we handle the DEVCONF with pinctrl-single, we
don't need most of the McBSP fck code or the omap2_mcbsp_set_clks_src.
Having the mux register as the clock enable register is not nice..
Who knows what the clock coming from the external pin might be :)
Regards,
Tony
^ permalink raw reply
* Re: [PATCH 4/4] arm: dts: omap3-gta04: Add static configuration for devconf1 register
From: Tero Kristo @ 2014-11-14 7:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20141113235840.GS26481@atomide.com>
On 11/14/2014 01:58 AM, Tony Lindgren wrote:
> * Paul Walmsley <paul@pwsan.com> [141113 15:01]:
>> Hi
>>
>> On Thu, 13 Nov 2014, Tony Lindgren wrote:
>>
>>> * Tomi Valkeinen <tomi.valkeinen@ti.com> [141113 03:33]:
>>>> On 12/11/14 17:02, Tony Lindgren wrote:
>>>>
>>>>>> And, with a quick grep, I see CONTROL_DEVCONF1 touched in multiple
>>>>>> places in the kernel. I wonder if adding a pinmux entry for it could
>>>>>> cause some rather odd problems.
>>>>>
>>>>> They can all use pinctrl-single no problem.
>>>>
>>>> Can, but don't. That's my worry. If we touch the DEVCONF1 via pinmux,
>>>> and we have code in mach-omap2 that also touch DEVCONF1, without any
>>>> knowledge (and locking) between those...
>>>
>>> Hmm yeah the McBSP clock mux could be racy as the mux register for
>>> McBSP is treated as a clock. This register muxes the clock between
>>> external pin and internal clock. Considering that this should be
>>> selectable at board level as the external clock probably needs to be
>>> used if level shifters are being used, it should be really handled by
>>> pinctrl-single.
>>>
>>> The other use for hsmmc.c and pdata-quirks.c for the one time mux for
>>> MMC clock from the MMC clock pin. That can be done with pinctrl-single
>>> from the MMC driver too for DT based booting.
>>>
>>> Then we just have the save and restore of the registers for
>>> off-idle.
>>>
>>>> So _maybe_ that's not an issue, as the pinmux config we have here is
>>>> fixed, and done once at boot time, and maybe the code in mach-omap2 that
>>>> touch DEVCONF1 is also ran just once and not at the same time as the
>>>> pinmux. But I don't know if that's so.
>>>
>>> It seems we could just do a read-only check for McBSP in the clock
>>> code for the mux register, or even completely drop that code from
>>> cclock3xxx_data.c and start using the pinctrl for that mux.
>>>
>>> Paul & Tero, got any comments here?
>>
>> It's best to move all of the SCM register reads/writes to an SCM IP block
>> driver. This driver would be the only entity that would touch the SCM IP
>> block registers - no other code on the system would touch it (perhaps
>> aside from anything needed for early init). The SCM driver would enforce
>> mutual exclusion via a spinlock, so concurrent SCM register modifications
>> wouldn't flake out. Then the SCM driver would register clocks with the
>> CCF, register pins with the pinctrl subsystem, etc. etc.
>
> We actually do have that with pinctrl-single + syscon. We certainly
> need to implement more Linux framework drivers for the SCM registers.
> Things like regulators, clocks, and PHYs, but they should use
> pinctrl-single + syscon. See the the pbias-regulator.c for example.
>
> Looking at the McBSP clock handling, threre's yet more handling of
> the same DEVCONF1 mux register in omap2_mcbsp_set_clks_src that gets
> alled from omap_mcbsp_dai_set_dai_sysclk.
>
> To me it seems that if we handle the DEVCONF with pinctrl-single, we
> don't need most of the McBSP fck code or the omap2_mcbsp_set_clks_src.
> Having the mux register as the clock enable register is not nice..
> Who knows what the clock coming from the external pin might be :)
The PRCM/clock cleanups that I have under work basically splits the
clock inits under their respective IP blocks; currently everything is
registered under generic PRCM. System control module will be one of the
clock providers (and is going to look like a driver), which will be
registering its own clocks. This doesn't change the fact that pinctrl is
directly mapping its own register space atm though, it might be possible
to re-route this to use the generic system control module if need be though.
I guess its just a political decision which way we want to go, currently
we have lots of system control clocks under the clock data (for
AM33xx,AM43xx,OMAP3), but we can remove these easily if need be. In some
cases it is nicer to have the data in the clock tree though, the drivers
don't need to care if they are touching a clock or a pinctrl entity.
Some people have been converting additional stuff to CCF outside of
PRCM, like Archit did some work to try and get control module clock
support for DRA7, and Tomi has been talking to convert some of the DSS
internal clocks to CCF also.
-Tero
^ permalink raw reply
* Re: [PATCH v6] simplefb: add clock handling code
From: Hans de Goede @ 2014-11-14 7:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAMuHMdW-6Eb9DSE=JQm7xWMhJUpt1xxLqS=6KU+=ZDMsmtiBag@mail.gmail.com>
Hi,
On 11/13/2014 08:39 PM, Geert Uytterhoeven wrote:
> On Thu, Nov 13, 2014 at 8:22 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> changes in v6:
>> -Wrap the clock code in #ifdef CONFIG_OF, as simplefb is used on non
>> devicetree platforms too
>
> With clocks?
No, the existing users are in arch/x86/kernel/sysfb*.c so no clock support.
And any new users which need clocks should use devicetree.
Regards,
Hans
^ permalink raw reply
* [PATCH] dt-bindings: simplefb: Document naming scheme for pre-populated simplefb nodes
From: Hans de Goede @ 2014-11-14 9:02 UTC (permalink / raw)
To: linux-arm-kernel
Since we advice to use pre-populated simeplfb nodes in dt files, where the
firmware then only needs to fill in the mode info + address and enable it,
we should also specify how these pre-populated nodes should be named so
that the firmware can find and enable the right one.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Documentation/devicetree/bindings/video/simple-framebuffer.txt | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
index c548f33..01dc948 100644
--- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
+++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
@@ -29,6 +29,13 @@ enable them. This way if e.g. later on support for more display clocks get
added, the simplefb nodes will already contain this info and the firmware
does not need to be updated.
+If pre-filled framebuffer nodes are used, they should be named
+"framebuffer#-<output>", e.g. "framebuffer0-hdmi". The output should be
+included in the name since different outputs typically require different
+clocks and the clocks are part of the pre-populated nodes. The firmware must
+rename the nodes to the standard "framebuffer@<address>" using the runtime
+chosen address when enabling the nodes.
+
Required properties:
- compatible: "simple-framebuffer"
- reg: Should contain the location and size of the framebuffer memory.
--
2.1.0
^ permalink raw reply related
* Re: [PATCH] dt-bindings: simplefb: Document naming scheme for pre-populated simplefb nodes
From: Grant Likely @ 2014-11-14 9:12 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1415955751-6316-1-git-send-email-hdegoede@redhat.com>
On Fri, Nov 14, 2014 at 9:02 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Since we advice to use pre-populated simeplfb nodes in dt files, where the
> firmware then only needs to fill in the mode info + address and enable it,
> we should also specify how these pre-populated nodes should be named so
> that the firmware can find and enable the right one.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Documentation/devicetree/bindings/video/simple-framebuffer.txt | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> index c548f33..01dc948 100644
> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> @@ -29,6 +29,13 @@ enable them. This way if e.g. later on support for more display clocks get
> added, the simplefb nodes will already contain this info and the firmware
> does not need to be updated.
>
> +If pre-filled framebuffer nodes are used, they should be named
> +"framebuffer#-<output>", e.g. "framebuffer0-hdmi". The output should be
> +included in the name since different outputs typically require different
> +clocks and the clocks are part of the pre-populated nodes. The firmware must
> +rename the nodes to the standard "framebuffer@<address>" using the runtime
> +chosen address when enabling the nodes.
-<output> should be optional, an only used when there are multiple options.
Something we've not talked about is how to handle a framebuffer that
has been set up with mirrored displays. Does that case matter?
g.
^ permalink raw reply
* Re: [PATCH v4 1/4] dt-bindings: simplefb: Specify node location and handoff related properties
From: Grant Likely @ 2014-11-14 9:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1415906594-2461-1-git-send-email-hdegoede@redhat.com>
On Thu, Nov 13, 2014 at 7:23 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Since simplefb nodes do not relate directly to hw typically they have been
> placed in the root of the devicetree. As the represent runtime information
> having them as sub-nodes of /chosen is more logical, specify this.
>
> Also specify when to set the chosen stdout-path property to a simplefb node.
>
> For reliable handover to a hardware specific driver, that driver needs to
> know which simplefb to unregister when taking over, specify how the hw driver
> can find the matching simplefb node.
>
> Last add some advice on how to fill and use simplefb nodes from a firmware
> pov.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> --
> Changes in v2:
> -Add stdout-path to the example code
> Changes in v3:
> -Specify that the node name must be "framebuffer@<address>"
> -Specify that the link to link simplefb node and display hw nodes to one
> another for handover goes into the simplefb node, and must be called "display"
> -Specify how aliases may be used to tell the OS how to number displays
> -Update the example to reflect these changes
> Changes in v4:
> -Keep the size of the reg property in the example as it was before
> ---
> .../bindings/video/simple-framebuffer.txt | 49 +++++++++++++++++++++-
> 1 file changed, 48 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> index 8f35718..c548f33 100644
> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> @@ -4,6 +4,31 @@ A simple frame-buffer describes a frame-buffer setup by firmware or
> the bootloader, with the assumption that the display hardware has already
> been set up to scan out from the memory pointed to by the reg property.
>
> +Since simplefb nodes represent runtime information they must be sub-nodes of
> +the chosen node (*). Simplefb nodes must be named "framebuffer@<address>".
> +
> +If a simplefb node represents the preferred console for user interaction,
> +then the chosen node's stdout-path property must point to it.
Since we've decided to point the display aliases at the display node
instead of the framebuffer, the stdout-path property should also point
to the display node. The simplefb driver can still work out which
framebuffer to use for stdout path in the same way that it figures out
how to resolve the display alias. This way the stdout details aren't
lost when the framebuffer is disabled and It also means that
stdout-path can be merely set to the alias ("display0") instead of the
full path ("/full/path/to/node").
> +
> +If the devicetree contains nodes for the display hardware used by a simplefb,
> +then the simplefb node must contain a property called "display", which
> +contains a phandle pointing to the primary display hw node, so that the OS
> +knows which simplefb to disable when handing over control to a driver for the
> +real hardware. The bindings for the hw nodes must specify which node is
> +considered the primary node.
> +
> +It is advised to add display# aliases to help the OS determine how to number
> +things. If display# aliases are used, then if the simplefb node contains a
s/then if/and if/
> +"display" property then the /aliases/display# path must point to the display
> +hw node the "display" property points to, otherwise it must point directly
> +to the simplefb node.
> +
> +It is advised that devicetree files contain pre-filled, disabled framebuffer
> +nodes, so that the firmware only needs to update the mode information and
> +enable them. This way if e.g. later on support for more display clocks get
> +added, the simplefb nodes will already contain this info and the firmware
> +does not need to be updated.
You should merge your guidance of naming pre-filled framebuffer nodes
into this patch. There is no need to have it separate.
> +
> Required properties:
> - compatible: "simple-framebuffer"
> - reg: Should contain the location and size of the framebuffer memory.
> @@ -19,14 +44,36 @@ Optional properties:
> are expected to already be configured correctly. The OS must
> ensure these clocks are not modified or disabled while the
> simple framebuffer remains active.
> +- display : phandle pointing to the primary display hardware node
>
> Example:
>
> - framebuffer {
> +aliases {
> + display0 = &lcdc0;
> +}
> +
> +chosen {
> + framebuffer0: framebuffer@1d385000 {
> compatible = "simple-framebuffer";
> reg = <0x1d385000 (1600 * 1200 * 2)>;
> width = <1600>;
> height = <1200>;
> stride = <(1600 * 2)>;
> format = "r5g6b5";
> + clocks = <&ahb_gates 36>, <&ahb_gates 43>, <&ahb_gates 44>;
> + display = <&lcdc0>;
> + };
> + stdout-path = &framebuffer0;
> +};
> +
> +soc@01c00000 {
> + lcdc0: lcdc@1c0c000 {
> + compatible = "allwinner,sun4i-a10-lcdc";
> + ...
> };
> +};
> +
> +
> +*) Older devicetree files may have a compatible = "simple-framebuffer" node
> +in a different place, operating systems must first enumerate any compatible
> +nodes found under chosen and then check for other compatible nodes.
> --
> 2.1.0
>
^ permalink raw reply
* Re: [PATCH v4 2/4] simplefb: Add support for enumerating simplefb dt nodes in /chosen
From: Grant Likely @ 2014-11-14 9:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1415906594-2461-2-git-send-email-hdegoede@redhat.com>
On Thu, Nov 13, 2014 at 7:23 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Update simplefb to support the new preferred location for simplefb dt nodes
> under /chosen.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> --
> Changes in v2:
> -Make name array larger in case we ever encounter more then 10000 framebuffers
> Changes in v3:
> -Switch to for_each_child_of_node to loop over the nodes under chosen
> Changes in v4:
> -Wrap the chosen enumeration code in #ifdef CONFIG_OF, as simplefb is used on
> non devicetree platforms too
> -simplefb can only be built-in, so drop the module_exit function calling
> platform_driver_unregister
> ---
> drivers/video/fbdev/simplefb.c | 23 ++++++++++++++++++++++-
> 1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index 868099a..e381a53 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -27,6 +27,7 @@
> #include <linux/platform_data/simplefb.h>
> #include <linux/platform_device.h>
> #include <linux/clk-provider.h>
> +#include <linux/of_platform.h>
>
> static struct fb_fix_screeninfo simplefb_fix = {
> .id = "simple",
> @@ -390,7 +391,27 @@ static struct platform_driver simplefb_driver = {
> .probe = simplefb_probe,
> .remove = simplefb_remove,
> };
> -module_platform_driver(simplefb_driver);
> +
> +static int __init simplefb_init(void)
> +{
> + int ret;
> + struct device_node __maybe_unused *np;
> +
> + ret = platform_driver_register(&simplefb_driver);
> + if (ret)
> + return ret;
> +
> +#ifdef CONFIG_OF
> + for_each_child_of_node(of_chosen, np) {
> + if (of_device_is_compatible(np, "simple-framebuffer"))
> + of_platform_device_create(np, NULL, NULL);
> + }
> +#endif
Nit: Kind of ugly. This should work instead:
if (IS_ENABLED(CONFIG_OF_PLATFORM) && of_chosen) {
for_each_child_of_node(of_chosen, np)
if (of_device_is_compatible(np, "simple-framebuffer"))
of_platform_device_create(np, NULL, NULL);
}
And move the forward declaration of of_chosen in include/linux/of.h up
5 lines so that it is always declared. gcc will optimize it out, but
it will still get properly syntax checked on every build.
> +
> + return 0;
> +}
> +
> +module_init(simplefb_init);
>
> MODULE_AUTHOR("Stephen Warren <swarren@wwwdotorg.org>");
> MODULE_DESCRIPTION("Simple framebuffer driver");
> --
> 2.1.0
>
^ permalink raw reply
* Re: [PATCH v6] simplefb: add clock handling code
From: Grant Likely @ 2014-11-14 9:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1415906546-2408-1-git-send-email-hdegoede@redhat.com>
On Thu, Nov 13, 2014 at 7:22 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> From: Luc Verhaegen <libv@skynet.be>
>
> This claims and enables clocks listed in the simple framebuffer dt node.
> This is needed so that the display engine, in case the required clocks
> are known by the kernel code and are described in the dt, will remain
> properly enabled.
>
> Signed-off-by: Luc Verhaegen <libv@skynet.be>
> [hdegoede@redhat.com: Change clks from list to dynamic array]
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Reviewed-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
> Acked-by: Grant Likely <grant.likely@linaro.org>
> --
> Changes in v4:
> -change clks from linkedlist to dynamic allocated array
> -propagate EPROBE_DEFER up from simplefb_clocks_init to simplefb_probe
> changes in v6:
> -Wrap the clock code in #ifdef CONFIG_OF, as simplefb is used on non
> devicetree platforms too
> ---
> drivers/video/fbdev/simplefb.c | 106 ++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 105 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
> index cdcf1fe..868099a 100644
> --- a/drivers/video/fbdev/simplefb.c
> +++ b/drivers/video/fbdev/simplefb.c
> @@ -26,6 +26,7 @@
> #include <linux/module.h>
> #include <linux/platform_data/simplefb.h>
> #include <linux/platform_device.h>
> +#include <linux/clk-provider.h>
>
> static struct fb_fix_screeninfo simplefb_fix = {
> .id = "simple",
> @@ -167,8 +168,103 @@ static int simplefb_parse_pd(struct platform_device *pdev,
>
> struct simplefb_par {
> u32 palette[PSEUDO_PALETTE_SIZE];
> +#ifdef CONFIG_OF
> + int clk_count;
> + struct clk **clks;
> +#endif
> };
>
> +/*
> + * Clock handling code.
> + *
> + * Here we handle the clocks property of our "simple-framebuffer" dt node.
> + * This is necessary so that we can make sure that any clocks needed by
> + * the display engine that the bootloader set up for us (and for which it
> + * provided a simplefb dt node), stay up, for the life of the simplefb
> + * driver.
> + *
> + * When the driver unloads, we cleanly disable, and then release the clocks.
> + *
> + * We only complain about errors here, no action is taken as the most likely
> + * error can only happen due to a mismatch between the bootloader which set
> + * up simplefb, and the clock definitions in the device tree. Chances are
> + * that there are no adverse effects, and if there are, a clean teardown of
> + * the fb probe will not help us much either. So just complain and carry on,
> + * and hope that the user actually gets a working fb at the end of things.
> + */
> +static int
> +simplefb_clocks_init(struct simplefb_par *par, struct platform_device *pdev)
Going against convention here. Functions in the rest of the file have
the return parameters on the same line as the function name. Don't do
something different here.
Besides, it's just bad practice. Putting the return values on a
separate line means they don't show up in the results when grepping
for a function name.
> +{
> +#ifdef CONFIG_OF
Rather ugly. When #ifdefs are unavoidable, please wrap the entire
function including the prototype and have an #else block with a static
inline empty function so that the #ifdef/#endif directives don't
interfere with the indentation flow.
> + struct device_node *np = pdev->dev.of_node;
> + struct clk *clock;
> + int i, ret;
> +
> + if (dev_get_platdata(&pdev->dev) || !np)
> + return 0;
> +
> + par->clk_count = of_clk_get_parent_count(np);
> + if (par->clk_count <= 0)
> + return 0;
> +
> + par->clks = kcalloc(par->clk_count, sizeof(struct clk *), GFP_KERNEL);
> + if (!par->clks)
> + return -ENOMEM;
> +
> + for (i = 0; i < par->clk_count; i++) {
> + clock = of_clk_get(np, i);
Should check if there is an 'of' version of devm_clk_get(). Then the
clock destroy function wouldn't be needed at all.
> + if (IS_ERR(clock)) {
> + if (PTR_ERR(clock) = -EPROBE_DEFER) {
> + while (--i >= 0) {
> + if (par->clks[i])
> + clk_put(par->clks[i]);
> + }
> + kfree(par->clks);
> + return -EPROBE_DEFER;
> + }
> + dev_err(&pdev->dev, "%s: clock %d not found: %ld\n",
> + __func__, i, PTR_ERR(clock));
> + continue;
> + }
> + par->clks[i] = clock;
> + }
(comment for a followup patch) As discussed on IRC, I don't think we
want the driver to return -EPROBE_DEFER if it can't get a clock. It
should continue anyway. The kernel knows the framebuffer is already
set up, so there is no reason to defer using it. The driver should
keep track of the clocks that it failed to obtain and try again just
before unused clocks get disabled. Have a special initcall hook that
walks though the bound framebuffers and tries the unobtained clocks
again.
> +
> + for (i = 0; i < par->clk_count; i++) {
> + if (par->clks[i]) {
> + ret = clk_prepare_enable(par->clks[i]);
> + if (ret) {
> + dev_err(&pdev->dev,
> + "%s: failed to enable clock %d: %d\n",
> + __func__, i, ret);
> + clk_put(par->clks[i]);
> + par->clks[i] = NULL;
> + }
> + }
> + }
> +#endif
> + return 0;
> +}
> +
> +static void
> +simplefb_clocks_destroy(struct simplefb_par *par)
> +{
> +#ifdef CONFIG_OF
> + int i;
> +
> + if (!par->clks)
> + return;
> +
> + for (i = 0; i < par->clk_count; i++) {
> + if (par->clks[i]) {
> + clk_disable_unprepare(par->clks[i]);
> + clk_put(par->clks[i]);
> + }
> + }
> +
> + kfree(par->clks);
> +#endif
> +}
> +
> static int simplefb_probe(struct platform_device *pdev)
> {
> int ret;
> @@ -236,6 +332,10 @@ static int simplefb_probe(struct platform_device *pdev)
> }
> info->pseudo_palette = par->palette;
>
> + ret = simplefb_clocks_init(par, pdev);
> + if (ret < 0)
> + goto error_unmap;
> +
> dev_info(&pdev->dev, "framebuffer at 0x%lx, 0x%x bytes, mapped to 0x%p\n",
> info->fix.smem_start, info->fix.smem_len,
> info->screen_base);
> @@ -247,13 +347,15 @@ static int simplefb_probe(struct platform_device *pdev)
> ret = register_framebuffer(info);
> if (ret < 0) {
> dev_err(&pdev->dev, "Unable to register simplefb: %d\n", ret);
> - goto error_unmap;
> + goto error_clocks;
> }
>
> dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);
>
> return 0;
>
> +error_clocks:
> + simplefb_clocks_destroy(par);
> error_unmap:
> iounmap(info->screen_base);
> error_fb_release:
> @@ -264,8 +366,10 @@ error_fb_release:
> static int simplefb_remove(struct platform_device *pdev)
> {
> struct fb_info *info = platform_get_drvdata(pdev);
> + struct simplefb_par *par = info->par;
>
> unregister_framebuffer(info);
> + simplefb_clocks_destroy(par);
> framebuffer_release(info);
>
> return 0;
> --
> 2.1.0
>
^ permalink raw reply
* Re: [PATCH] dt-bindings: simplefb: Document naming scheme for pre-populated simplefb nodes
From: Hans de Goede @ 2014-11-14 9:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CACxGe6txgJKsybuSrCXzCn8sPJYZVwo-wm+pV5KuTmP=YHiA2g@mail.gmail.com>
Hi,
On 11/14/2014 10:12 AM, Grant Likely wrote:
> On Fri, Nov 14, 2014 at 9:02 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Since we advice to use pre-populated simeplfb nodes in dt files, where the
>> firmware then only needs to fill in the mode info + address and enable it,
>> we should also specify how these pre-populated nodes should be named so
>> that the firmware can find and enable the right one.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Documentation/devicetree/bindings/video/simple-framebuffer.txt | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> index c548f33..01dc948 100644
>> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> @@ -29,6 +29,13 @@ enable them. This way if e.g. later on support for more display clocks get
>> added, the simplefb nodes will already contain this info and the firmware
>> does not need to be updated.
>>
>> +If pre-filled framebuffer nodes are used, they should be named
>> +"framebuffer#-<output>", e.g. "framebuffer0-hdmi". The output should be
>> +included in the name since different outputs typically require different
>> +clocks and the clocks are part of the pre-populated nodes. The firmware must
>> +rename the nodes to the standard "framebuffer@<address>" using the runtime
>> +chosen address when enabling the nodes.
>
> -<output> should be optional, an only used when there are multiple options.
Right, notice the "should be named", I would rather leave it at that then
complicate the wording by adding something about only use -output when there
are multiple options.
> Something we've not talked about is how to handle a framebuffer that
> has been set up with mirrored displays. Does that case matter?
It is bound to happen sooner or later I guess, but it is still just a single
framebuffer, so from the simplefb kernel driver pov it does not matter, it really
is just another variant on the -<output> theme where there happen to be 2 outputs
active at the same time.
Regards,
Hans
>
> g.
>
^ permalink raw reply
* Re: [PATCH 02/15] GPIO: port LoCoMo gpio support from old driver
From: Linus Walleij @ 2014-11-14 10:11 UTC (permalink / raw)
To: Dmitry Eremin-Solenikov
Cc: Mark Brown,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Linux Input,
linux-leds-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org, Andrea Adami,
Russell King, Daniel Mack, Haojian Zhuang, Robert Jarzmik,
Alexandre Courbot, Dmitry Torokhov, Bryan Wu, Richard Purdie,
Samuel Ortiz, Lee Jones, Jingoo
In-Reply-To: <CALT56yPr42FV66USngocw=eWPt81d5R2MJxmzBnv02HOMmXAkA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Tue, Nov 11, 2014 at 2:16 PM, Dmitry Eremin-Solenikov
<dbaryshkov@gmail.com> wrote:
> Just to better understand your suggestions: do you want me to convert
> to regmap only gpio driver or do you suggest to convert all LoCoMo drivers?
Um... I was just thinking about this one usecase.
It's no big deal, the other review comments are more important.
> That is doable, of course, but the amount of changes is overwhelming.
> Also I'm concerned about the performance impact from going through
> regmap layers.
Is it on a critical path? The current locking isn't any less invasive
AFAICT.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v4 1/4] dt-bindings: simplefb: Specify node location and handoff related properties
From: Hans de Goede @ 2014-11-14 10:15 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CACxGe6uJhyyAfXDEk5a9fR+RtiRbAO4qYzOrStcanJxUuc1GiQ@mail.gmail.com>
Hi,
On 11/14/2014 10:19 AM, Grant Likely wrote:
> On Thu, Nov 13, 2014 at 7:23 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Since simplefb nodes do not relate directly to hw typically they have been
>> placed in the root of the devicetree. As the represent runtime information
>> having them as sub-nodes of /chosen is more logical, specify this.
>>
>> Also specify when to set the chosen stdout-path property to a simplefb node.
>>
>> For reliable handover to a hardware specific driver, that driver needs to
>> know which simplefb to unregister when taking over, specify how the hw driver
>> can find the matching simplefb node.
>>
>> Last add some advice on how to fill and use simplefb nodes from a firmware
>> pov.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
>> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>> --
>> Changes in v2:
>> -Add stdout-path to the example code
>> Changes in v3:
>> -Specify that the node name must be "framebuffer@<address>"
>> -Specify that the link to link simplefb node and display hw nodes to one
>> another for handover goes into the simplefb node, and must be called "display"
>> -Specify how aliases may be used to tell the OS how to number displays
>> -Update the example to reflect these changes
>> Changes in v4:
>> -Keep the size of the reg property in the example as it was before
>> ---
>> .../bindings/video/simple-framebuffer.txt | 49 +++++++++++++++++++++-
>> 1 file changed, 48 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> index 8f35718..c548f33 100644
>> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
>> @@ -4,6 +4,31 @@ A simple frame-buffer describes a frame-buffer setup by firmware or
>> the bootloader, with the assumption that the display hardware has already
>> been set up to scan out from the memory pointed to by the reg property.
>>
>> +Since simplefb nodes represent runtime information they must be sub-nodes of
>> +the chosen node (*). Simplefb nodes must be named "framebuffer@<address>".
>> +
>> +If a simplefb node represents the preferred console for user interaction,
>> +then the chosen node's stdout-path property must point to it.
>
> Since we've decided to point the display aliases at the display node
> instead of the framebuffer, the stdout-path property should also point
> to the display node. The simplefb driver can still work out which
> framebuffer to use for stdout path in the same way that it figures out
> how to resolve the display alias. This way the stdout details aren't
> lost when the framebuffer is disabled and It also means that
> stdout-path can be merely set to the alias ("display0") instead of the
> full path ("/full/path/to/node").
Ok, will fix for V5.
>
>> +
>> +If the devicetree contains nodes for the display hardware used by a simplefb,
>> +then the simplefb node must contain a property called "display", which
>> +contains a phandle pointing to the primary display hw node, so that the OS
>> +knows which simplefb to disable when handing over control to a driver for the
>> +real hardware. The bindings for the hw nodes must specify which node is
>> +considered the primary node.
>> +
>> +It is advised to add display# aliases to help the OS determine how to number
>> +things. If display# aliases are used, then if the simplefb node contains a
>
> s/then if/and if/
Erm, no the then applies to both the musts following that sentence, in pseudo
code it looks like this:
if (display# aliases are used) {
if (simplefb node contains a "display" property) {
/aliases/display# path must point to the display hw node the "display" property points to
} else {
/aliases/display# path must point directly to the simplefb node.
}
}
So no && between the (display# aliases are used) and (simplefb node contains a "display" property)
conditions.
>
>> +"display" property then the /aliases/display# path must point to the display
>> +hw node the "display" property points to, otherwise it must point directly
>> +to the simplefb node.
>> +
>> +It is advised that devicetree files contain pre-filled, disabled framebuffer
>> +nodes, so that the firmware only needs to update the mode information and
>> +enable them. This way if e.g. later on support for more display clocks get
>> +added, the simplefb nodes will already contain this info and the firmware
>> +does not need to be updated.
>
> You should merge your guidance of naming pre-filled framebuffer nodes
> into this patch. There is no need to have it separate.
Will do for v5.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH 4/7] ARM: dts: sun6i: Add simplefb node
From: Maxime Ripard @ 2014-11-14 10:24 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5464EFF9.9010203@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 1819 bytes --]
On Thu, Nov 13, 2014 at 06:52:57PM +0100, Hans de Goede wrote:
> Hi,
>
> On 11/13/2014 05:46 PM, Maxime Ripard wrote:
> > Hi,
> >
> > On Thu, Nov 13, 2014 at 10:31:46AM +0100, Hans de Goede wrote:
> >> Add a simplefb template node for u-boot to further fill and activate.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >> arch/arm/boot/dts/sun6i-a31.dtsi | 10 ++++++++++
> >> 1 file changed, 10 insertions(+)
> >>
> >> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
> >> index 543f895..438952e 100644
> >> --- a/arch/arm/boot/dts/sun6i-a31.dtsi
> >> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
> >> @@ -62,6 +62,16 @@
> >> ethernet0 = &gmac;
> >> };
> >>
> >> + chosen {
> >> + #address-cells = <1>;
> >> + #size-cells = <1>;
> >> + ranges;
> >> +
> >> + framebuffer0 {
> >> + compatible = "simple-framebuffer";
> >> + status = "disabled";
> >
> > Aren't we suppose to have clocks in there too?
>
> I deliberately left them out as they are not needed on sun6i, once the blocks
> are setup ahb access is not necessary, but it seems that the ahb_gate bits
> in sun4i / sun5i / sun7i double as a reset. Toggling them on / off / on breaks
> the mode, while toggling module clocks on / off / on only causes a glitch.
>
> Since the sun6i has a separate reset controller, the clock gates seem to really
> only control ahb access which is not necessary once the mode has been set up.
Ok. That can always be changed in the future if we ever find we need
them anyway.
I'll merge these four patches as soon as the bindings doc is
acked-by/merged by Grant or any other DT maintainer.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v6] simplefb: add clock handling code
From: Hans de Goede @ 2014-11-14 10:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CACxGe6uB-J8_uzNJqCOEjM5G8=Bq=KFf-O1tfXVwP0=cMY+7iw@mail.gmail.com>
Hi,
On 11/14/2014 10:42 AM, Grant Likely wrote:
> On Thu, Nov 13, 2014 at 7:22 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> From: Luc Verhaegen <libv@skynet.be>
>>
>> This claims and enables clocks listed in the simple framebuffer dt node.
>> This is needed so that the display engine, in case the required clocks
>> are known by the kernel code and are described in the dt, will remain
>> properly enabled.
>>
>> Signed-off-by: Luc Verhaegen <libv@skynet.be>
>> [hdegoede@redhat.com: Change clks from list to dynamic array]
>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Acked-by: Geert Uytterhoeven <geert@linux-m68k.org>
>> Reviewed-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>> Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
>> Acked-by: Grant Likely <grant.likely@linaro.org>
>> --
>> Changes in v4:
>> -change clks from linkedlist to dynamic allocated array
>> -propagate EPROBE_DEFER up from simplefb_clocks_init to simplefb_probe
>> changes in v6:
>> -Wrap the clock code in #ifdef CONFIG_OF, as simplefb is used on non
>> devicetree platforms too
>> ---
>> drivers/video/fbdev/simplefb.c | 106 ++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 105 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
>> index cdcf1fe..868099a 100644
>> --- a/drivers/video/fbdev/simplefb.c
>> +++ b/drivers/video/fbdev/simplefb.c
>> @@ -26,6 +26,7 @@
>> #include <linux/module.h>
>> #include <linux/platform_data/simplefb.h>
>> #include <linux/platform_device.h>
>> +#include <linux/clk-provider.h>
>>
>> static struct fb_fix_screeninfo simplefb_fix = {
>> .id = "simple",
>> @@ -167,8 +168,103 @@ static int simplefb_parse_pd(struct platform_device *pdev,
>>
>> struct simplefb_par {
>> u32 palette[PSEUDO_PALETTE_SIZE];
>> +#ifdef CONFIG_OF
>> + int clk_count;
>> + struct clk **clks;
>> +#endif
>> };
>>
>> +/*
>> + * Clock handling code.
>> + *
>> + * Here we handle the clocks property of our "simple-framebuffer" dt node.
>> + * This is necessary so that we can make sure that any clocks needed by
>> + * the display engine that the bootloader set up for us (and for which it
>> + * provided a simplefb dt node), stay up, for the life of the simplefb
>> + * driver.
>> + *
>> + * When the driver unloads, we cleanly disable, and then release the clocks.
>> + *
>> + * We only complain about errors here, no action is taken as the most likely
>> + * error can only happen due to a mismatch between the bootloader which set
>> + * up simplefb, and the clock definitions in the device tree. Chances are
>> + * that there are no adverse effects, and if there are, a clean teardown of
>> + * the fb probe will not help us much either. So just complain and carry on,
>> + * and hope that the user actually gets a working fb at the end of things.
>> + */
>> +static int
>> +simplefb_clocks_init(struct simplefb_par *par, struct platform_device *pdev)
>
> Going against convention here. Functions in the rest of the file have
> the return parameters on the same line as the function name. Don't do
> something different here.
>
> Besides, it's just bad practice. Putting the return values on a
> separate line means they don't show up in the results when grepping
> for a function name.
This was present in the original version of this patch (which I did not write),
will fix for v7.
>
>> +{
>> +#ifdef CONFIG_OF
>
> Rather ugly. When #ifdefs are unavoidable, please wrap the entire
> function including the prototype and have an #else block with a static
> inline empty function so that the #ifdef/#endif directives don't
> interfere with the indentation flow.
Will fix for v7.
>> + struct device_node *np = pdev->dev.of_node;
>> + struct clk *clock;
>> + int i, ret;
>> +
>> + if (dev_get_platdata(&pdev->dev) || !np)
>> + return 0;
>> +
>> + par->clk_count = of_clk_get_parent_count(np);
>> + if (par->clk_count <= 0)
>> + return 0;
>> +
>> + par->clks = kcalloc(par->clk_count, sizeof(struct clk *), GFP_KERNEL);
>> + if (!par->clks)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < par->clk_count; i++) {
>> + clock = of_clk_get(np, i);
>
> Should check if there is an 'of' version of devm_clk_get(). Then the
> clock destroy function wouldn't be needed at all.
There is no version of devm_clk_get which takes an index rather then a name.
>> + if (IS_ERR(clock)) {
>> + if (PTR_ERR(clock) = -EPROBE_DEFER) {
>> + while (--i >= 0) {
>> + if (par->clks[i])
>> + clk_put(par->clks[i]);
>> + }
>> + kfree(par->clks);
>> + return -EPROBE_DEFER;
>> + }
>> + dev_err(&pdev->dev, "%s: clock %d not found: %ld\n",
>> + __func__, i, PTR_ERR(clock));
>> + continue;
>> + }
>> + par->clks[i] = clock;
>> + }
>
> (comment for a followup patch) As discussed on IRC, I don't think we
> want the driver to return -EPROBE_DEFER if it can't get a clock. It
> should continue anyway. The kernel knows the framebuffer is already
> set up, so there is no reason to defer using it. The driver should
> keep track of the clocks that it failed to obtain and try again just
> before unused clocks get disabled. Have a special initcall hook that
> walks though the bound framebuffers and tries the unobtained clocks
> again.
>
>> +
>> + for (i = 0; i < par->clk_count; i++) {
>> + if (par->clks[i]) {
>> + ret = clk_prepare_enable(par->clks[i]);
>> + if (ret) {
>> + dev_err(&pdev->dev,
>> + "%s: failed to enable clock %d: %d\n",
>> + __func__, i, ret);
>> + clk_put(par->clks[i]);
>> + par->clks[i] = NULL;
>> + }
>> + }
>> + }
>> +#endif
>> + return 0;
>> +}
>> +
>> +static void
>> +simplefb_clocks_destroy(struct simplefb_par *par)
>> +{
>> +#ifdef CONFIG_OF
>> + int i;
>> +
>> + if (!par->clks)
>> + return;
>> +
>> + for (i = 0; i < par->clk_count; i++) {
>> + if (par->clks[i]) {
>> + clk_disable_unprepare(par->clks[i]);
>> + clk_put(par->clks[i]);
>> + }
>> + }
>> +
>> + kfree(par->clks);
>> +#endif
>> +}
>> +
>> static int simplefb_probe(struct platform_device *pdev)
>> {
>> int ret;
>> @@ -236,6 +332,10 @@ static int simplefb_probe(struct platform_device *pdev)
>> }
>> info->pseudo_palette = par->palette;
>>
>> + ret = simplefb_clocks_init(par, pdev);
>> + if (ret < 0)
>> + goto error_unmap;
>> +
>> dev_info(&pdev->dev, "framebuffer at 0x%lx, 0x%x bytes, mapped to 0x%p\n",
>> info->fix.smem_start, info->fix.smem_len,
>> info->screen_base);
>> @@ -247,13 +347,15 @@ static int simplefb_probe(struct platform_device *pdev)
>> ret = register_framebuffer(info);
>> if (ret < 0) {
>> dev_err(&pdev->dev, "Unable to register simplefb: %d\n", ret);
>> - goto error_unmap;
>> + goto error_clocks;
>> }
>>
>> dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);
>>
>> return 0;
>>
>> +error_clocks:
>> + simplefb_clocks_destroy(par);
>> error_unmap:
>> iounmap(info->screen_base);
>> error_fb_release:
>> @@ -264,8 +366,10 @@ error_fb_release:
>> static int simplefb_remove(struct platform_device *pdev)
>> {
>> struct fb_info *info = platform_get_drvdata(pdev);
>> + struct simplefb_par *par = info->par;
>>
>> unregister_framebuffer(info);
>> + simplefb_clocks_destroy(par);
>> framebuffer_release(info);
>>
>> return 0;
>> --
>> 2.1.0
>>
Regards,
Hans
^ permalink raw reply
* Re: [PATCH 4/7] ARM: dts: sun6i: Add simplefb node
From: Hans de Goede @ 2014-11-14 10:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20141114102417.GY20972@lukather>
Hi,
On 11/14/2014 11:24 AM, Maxime Ripard wrote:
> On Thu, Nov 13, 2014 at 06:52:57PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 11/13/2014 05:46 PM, Maxime Ripard wrote:
>>> Hi,
>>>
>>> On Thu, Nov 13, 2014 at 10:31:46AM +0100, Hans de Goede wrote:
>>>> Add a simplefb template node for u-boot to further fill and activate.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>> arch/arm/boot/dts/sun6i-a31.dtsi | 10 ++++++++++
>>>> 1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
>>>> index 543f895..438952e 100644
>>>> --- a/arch/arm/boot/dts/sun6i-a31.dtsi
>>>> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
>>>> @@ -62,6 +62,16 @@
>>>> ethernet0 = &gmac;
>>>> };
>>>>
>>>> + chosen {
>>>> + #address-cells = <1>;
>>>> + #size-cells = <1>;
>>>> + ranges;
>>>> +
>>>> + framebuffer0 {
>>>> + compatible = "simple-framebuffer";
>>>> + status = "disabled";
>>>
>>> Aren't we suppose to have clocks in there too?
>>
>> I deliberately left them out as they are not needed on sun6i, once the blocks
>> are setup ahb access is not necessary, but it seems that the ahb_gate bits
>> in sun4i / sun5i / sun7i double as a reset. Toggling them on / off / on breaks
>> the mode, while toggling module clocks on / off / on only causes a glitch.
>>
>> Since the sun6i has a separate reset controller, the clock gates seem to really
>> only control ahb access which is not necessary once the mode has been set up.
>
> Ok. That can always be changed in the future if we ever find we need
> them anyway.
>
> I'll merge these four patches as soon as the bindings doc is
> acked-by/merged by Grant or any other DT maintainer.
Note these patches are using framebuffer0 rather then framebuffer0-hdmi as node
name, which may cause issues if we later want to add lcd support. So I've a v2
coming which changes the name to framebuffer0-hdmi, please merge that instead.
Regards,
Hans
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox