mbox series

[00/17] Allwinner R40 HDMI refactoring

Message ID 20180706175113.26698-1-jernej.skrabec@siol.net
Headers show
Series Allwinner R40 HDMI refactoring | expand

Message

Jernej Škrabec July 6, 2018, 5:50 p.m. UTC
This series fixes several issues found in R40 HDMI patch series after
it was applied. Conversation can be found here:
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-June/586011.html

One issue raised in that thread is probably not solved (conversation
still on going), but I want to send these patches for review before
I'm going on vacation.

There is also additional DW HDMI driver fix not mentioned in mails.

Patches are based on latest linux-next (next-20180706) and are ordered
in such way that they don't break R40 HDMI at any time. Because of that
I suggest that whole series goes through drm-misc to preserve that order.

I also tested those patches on H3 to make sure it doesn't break other
platforms. However, it would be nice to test for regressions also on
older SoCs (with DE1).

Best regards,
Jernej

Jernej Skrabec (17):
  dt-bindings: display: sun4i-drm: Add R40 display engine compatible
  drm/sun4i: Add R40 display engine compatible
  ARM: dts: sun8i: r40: Remove fallback display engine compatible
  drm/sun4i: tcon-top: Cleanup clock handling
  drm/sun4i: tcon: Release node when traversing of graph
  dt-bindings: display: sun4i-drm: Add R40 TV TCON description
  drm/sun4i: DW HDMI: Release nodes if error happens during CRTC search
  ARM: dts: sun8i: r40: Add mixer ids to TCON TOP
  drm/sun4i: mixer: Read id from DT
  drm/sun4i: tcon-top: Add helpers for switching mux
  drm/sun4i: tcon: Add another way for matching mixers with tcon
  drm/sun4i: tcon: Add support for R40 TCON
  ARM: dts: sun8i: r40: Remove fallback compatible for TCON TV
  ARM: dts: sun8i: r40: Add missing TCON-TOP - TCON connections
  ARM: dts: sun8i: r40: Disable TCONs by default.
  drm/sun4i: tcon-top: Remove mux configuration at probe time
  dt-bindings: display: sun4i-drm: Fix order of DW HDMI PHY compatibles

 .../bindings/display/sunxi/sun4i-drm.txt      |   6 +-
 .../boot/dts/sun8i-r40-bananapi-m2-ultra.dts  |  20 +-
 arch/arm/boot/dts/sun8i-r40.dtsi              |  65 ++++++-
 drivers/gpu/drm/sun4i/sun4i_drv.c             |   1 +
 drivers/gpu/drm/sun4i/sun4i_tcon.c            |  92 ++++++++-
 drivers/gpu/drm/sun4i/sun8i_dw_hdmi.c         |  15 +-
 drivers/gpu/drm/sun4i/sun8i_mixer.c           |  35 +++-
 drivers/gpu/drm/sun4i/sun8i_tcon_top.c        | 183 ++++++++----------
 drivers/gpu/drm/sun4i/sun8i_tcon_top.h        |   4 +
 9 files changed, 278 insertions(+), 143 deletions(-)

Comments

Chen-Yu Tsai July 10, 2018, 3:20 p.m. UTC | #1
On Sat, Jul 7, 2018 at 1:50 AM, Jernej Skrabec <jernej.skrabec@siol.net> wrote:
> R40 has versatile display pipeline. It supports two simultanious outputs
> on various outputs (TVE, VGA, HDMI, MIPI DSI, LCD).
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>

Reviewed-by: Chen-Yu Tsai <wens@csie.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chen-Yu Tsai July 10, 2018, 3:21 p.m. UTC | #2
On Sat, Jul 7, 2018 at 1:50 AM, Jernej Skrabec <jernej.skrabec@siol.net> wrote:
> R40 has pretty unique display pipeline. Because of that, H3 display
> engine compatible fallback should be removed.
>
> Fixes: 05a43a262d03 ("ARM: dts: sun8i: r40: Add HDMI pipeline")
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>

Reviewed-by: Chen-Yu Tsai <wens@csie.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chen-Yu Tsai July 10, 2018, 3:23 p.m. UTC | #3
On Sat, Jul 7, 2018 at 1:51 AM, Jernej Skrabec <jernej.skrabec@siol.net> wrote:
> There is no need to acquire reference to clock just to get its name.
>
> This commit just cleans up the code. There is no functional change.
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>

Reviewed-by: Chen-Yu Tsai <wens@csie.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chen-Yu Tsai July 10, 2018, 3:23 p.m. UTC | #4
On Sat, Jul 7, 2018 at 1:51 AM, Jernej Skrabec <jernej.skrabec@siol.net> wrote:
> Function sun4i_tcon_find_engine_traverse() doesn't release node if it
> needs to traverse of graph deeper than 1 level.
>
> Fix this by calling of_node_put().
>
> Fixes: 49836b11fe71 ("drm/sun4i: tcon: Generalize engine search algorithm")
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>

Reviewed-by: Chen-Yu Tsai <wens@csie.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chen-Yu Tsai July 10, 2018, 3:31 p.m. UTC | #5
On Sat, Jul 7, 2018 at 1:51 AM, Jernej Skrabec <jernej.skrabec@siol.net> wrote:
> If error happens in sun8i_dw_hdmi_find_possible_crtcs(), nodes are not
> released with of_node_put() before returning.
>
> Fix that by calling of_node_put() when necessary. While on it, clean up
> the code by using of_graph_get_remote_node() which also lowers number of
> cases where error handling has to be performed.

Good job!

> Fixes: 57e23de02f48 ("drm/sun4i: DW HDMI: Expand algorithm for possible crtcs")
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>

Reviewed-by: Chen-Yu Tsai <wens@csie.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chen-Yu Tsai July 10, 2018, 3:35 p.m. UTC | #6
On Sat, Jul 7, 2018 at 1:51 AM, Jernej Skrabec <jernej.skrabec@siol.net> wrote:
> Mixer has to have a way to read its id, if needed.

You should mention that this is required by the device tree binding sun4i-drm,
in the second paragraph of the first section:

    For all connections between components up to the TCONs in the display
    pipeline, when there are multiple components of the same type at the
    same depth, the local endpoint ID must be the same as the remote
    component's index.

You might also want to update the diagram for TCON TOP in the binding doc.

ChenYu

> Add them in R40 DT.
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
>  arch/arm/boot/dts/sun8i-r40.dtsi | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/boot/dts/sun8i-r40.dtsi b/arch/arm/boot/dts/sun8i-r40.dtsi
> index 2afb079a3776..1dd088d82773 100644
> --- a/arch/arm/boot/dts/sun8i-r40.dtsi
> +++ b/arch/arm/boot/dts/sun8i-r40.dtsi
> @@ -576,9 +576,12 @@
>                                 #size-cells = <0>;
>
>                                 tcon_top_mixer0_in: port@0 {
> +                                       #address-cells = <1>;
> +                                       #size-cells = <0>;
>                                         reg = <0>;
>
> -                                       tcon_top_mixer0_in_mixer0: endpoint {
> +                                       tcon_top_mixer0_in_mixer0: endpoint@0 {
> +                                               reg = <0>;
>                                                 remote-endpoint = <&mixer0_out_tcon_top>;
>                                         };
>                                 };
> @@ -606,9 +609,12 @@
>                                 };
>
>                                 tcon_top_mixer1_in: port@2 {
> +                                       #address-cells = <1>;
> +                                       #size-cells = <0>;
>                                         reg = <2>;
>
> -                                       tcon_top_mixer1_in_mixer1: endpoint {
> +                                       tcon_top_mixer1_in_mixer1: endpoint@1 {
> +                                               reg = <1>;
>                                                 remote-endpoint = <&mixer1_out_tcon_top>;
>                                         };
>                                 };
> --
> 2.18.0
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chen-Yu Tsai July 10, 2018, 3:40 p.m. UTC | #7
On Sat, Jul 7, 2018 at 1:51 AM, Jernej Skrabec <jernej.skrabec@siol.net> wrote:
> Currently, TCON supports 2 ways to match TCON with engine (mixer in this
> case). Old way is to just traverse of graph backwards and compare node
> pointer. New way is to match TCON and engine by their respective ids.
> All SoCs with DE2 enabled till now used the old way, which means mixer
> id was never used and thus never implemented.
>
> However, for R40, only the new way will be used. To prepare for that,
> implemend fetching mixer id from DT.
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
>  drivers/gpu/drm/sun4i/sun8i_mixer.c | 35 +++++++++++++++++++++++++++--
>  1 file changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> index ee8febb25903..11221f96746d 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> @@ -23,6 +23,7 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/reset.h>
>  #include <linux/of_device.h>
> +#include <linux/of_graph.h>

These should be alphabetically ordered.

>
>  #include "sun4i_drv.h"
>  #include "sun8i_mixer.h"
> @@ -322,6 +323,37 @@ static struct regmap_config sun8i_mixer_regmap_config = {
>         .max_register   = 0xbfffc, /* guessed */
>  };
>
> +static int sun8i_mixer_of_get_id(struct device_node *node)
> +{
> +       struct device_node *port, *ep;
> +       int ret = -EINVAL;
> +
> +       /* output is port 1 */
> +       port = of_graph_get_port_by_id(node, 1);
> +       if (!port)
> +               return -EINVAL;
> +
> +       /* try finding an upstream endpoint */

You mean downstream.

> +       for_each_available_child_of_node(port, ep) {
> +               struct device_node *remote;
> +               u32 reg;
> +
> +               remote = of_graph_get_remote_endpoint(ep);
> +               if (!remote)
> +                       continue;
> +
> +               ret = of_property_read_u32(remote, "reg", &reg);
> +               if (ret)
> +                       continue;

This is somewhat redundant, given the current code structure will loop
over all available child nodes. What you want is probably an early
termination condition instead, i.e. if (!ret).

Also, remember to call of_node_put on the remote node.

ChenYu

> +
> +               ret = reg;
> +       }
> +
> +       of_node_put(port);
> +
> +       return ret;
> +}
> +
>  static int sun8i_mixer_bind(struct device *dev, struct device *master,
>                               void *data)
>  {
> @@ -353,8 +385,7 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
>         dev_set_drvdata(dev, mixer);
>         mixer->engine.ops = &sun8i_engine_ops;
>         mixer->engine.node = dev->of_node;
> -       /* The ID of the mixer currently doesn't matter */
> -       mixer->engine.id = -1;
> +       mixer->engine.id = sun8i_mixer_of_get_id(dev->of_node);
>
>         mixer->cfg = of_device_get_match_data(dev);
>         if (!mixer->cfg)
> --
> 2.18.0
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chen-Yu Tsai July 10, 2018, 3:49 p.m. UTC | #8
On Sat, Jul 7, 2018 at 1:51 AM, Jernej Skrabec <jernej.skrabec@siol.net> wrote:
> While registers between A83T and R40 TV TCON are the same, R40 TCON TV
> driver has to set additional muxes in TCON TOP. Because of that, remove
> fallback A83T TCON TV compatible.

Nit: device tree doesn't care about the implementation. You can however
mention that hardware interaction makes it not so compatible. :)

> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>

Otherwise,

Reviewed-by: Chen-Yu Tsai <wens@csie.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chen-Yu Tsai July 10, 2018, 3:53 p.m. UTC | #9
On Sat, Jul 7, 2018 at 1:51 AM, Jernej Skrabec <jernej.skrabec@siol.net> wrote:
> Current R40 is missing some graph connections between TCON TOP and
> TCONs.
>
> Add them.
>
> Fixes: 05a43a262d03 ("ARM: dts: sun8i: r40: Add HDMI pipeline")
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>

Reviewed-by: Chen-Yu Tsai <wens@csie.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chen-Yu Tsai July 10, 2018, 3:56 p.m. UTC | #10
On Sat, Jul 7, 2018 at 1:51 AM, Jernej Skrabec <jernej.skrabec@siol.net> wrote:
> We want to be able to set TCON TOP muxes at runtime. Add helpers for
> that.
>
> Old, static configuration of muxes at probe time is preserved for now.
> It will be removed when R40 TCON starts using them.
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>

Reviewed-by: Chen-Yu Tsai <wens@csie.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chen-Yu Tsai July 10, 2018, 4:09 p.m. UTC | #11
On Sat, Jul 7, 2018 at 1:51 AM, Jernej Skrabec <jernej.skrabec@siol.net> wrote:
> Now that R40 TCON migrated to runtime mux configuration, old code can be
> removed.
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
>  drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 81 +++-----------------------
>  1 file changed, 7 insertions(+), 74 deletions(-)
>
> diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> index c09b15b64192..78795d6cb174 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> @@ -87,34 +87,6 @@ int sun8i_tcon_top_de_config(struct device *dev, int mixer, int tcon)
>  }
>  EXPORT_SYMBOL(sun8i_tcon_top_de_config);
>
> -static int sun8i_tcon_top_get_connected_ep_id(struct device_node *node,
> -                                             int port_id)
> -{
> -       struct device_node *ep, *remote, *port;
> -       struct of_endpoint endpoint;
> -
> -       port = of_graph_get_port_by_id(node, port_id);
> -       if (!port)
> -               return -ENOENT;
> -
> -       for_each_available_child_of_node(port, ep) {
> -               remote = of_graph_get_remote_port_parent(ep);
> -               if (!remote)
> -                       continue;
> -
> -               if (of_device_is_available(remote)) {
> -                       of_graph_parse_endpoint(ep, &endpoint);
> -
> -                       of_node_put(remote);
> -
> -                       return endpoint.id;
> -               }
> -
> -               of_node_put(remote);
> -       }
> -
> -       return -ENOENT;
> -}
>
>  static struct clk_hw *sun8i_tcon_top_register_gate(struct device *dev,
>                                                    const char *parent,
> @@ -149,11 +121,9 @@ static int sun8i_tcon_top_bind(struct device *dev, struct device *master,
>         struct platform_device *pdev = to_platform_device(dev);
>         struct clk_hw_onecell_data *clk_data;
>         struct sun8i_tcon_top *tcon_top;
> -       bool mixer0_unused = false;
>         struct resource *res;
>         void __iomem *regs;
> -       int ret, i, id;
> -       u32 val;
> +       int ret, i;
>
>         tcon_top = devm_kzalloc(dev, sizeof(*tcon_top), GFP_KERNEL);
>         if (!tcon_top)
> @@ -198,49 +168,12 @@ static int sun8i_tcon_top_bind(struct device *dev, struct device *master,
>                 goto err_assert_reset;
>         }
>
> -       val = 0;
> -
> -       /* check if HDMI mux output is connected */
> -       if (sun8i_tcon_top_get_connected_ep_id(dev->of_node, 5) >= 0) {
> -               /* find HDMI input endpoint id, if it is connected at all*/
> -               id = sun8i_tcon_top_get_connected_ep_id(dev->of_node, 4);
> -               if (id >= 0)
> -                       val = FIELD_PREP(TCON_TOP_HDMI_SRC_MSK, id + 1);
> -               else
> -                       DRM_DEBUG_DRIVER("TCON TOP HDMI input is not connected\n");
> -       } else {
> -               DRM_DEBUG_DRIVER("TCON TOP HDMI output is not connected\n");
> -       }
> -
> -       writel(val, regs + TCON_TOP_GATE_SRC_REG);
> -
> -       val = 0;
> -
> -       /* process mixer0 mux output */
> -       id = sun8i_tcon_top_get_connected_ep_id(dev->of_node, 1);
> -       if (id >= 0) {
> -               val = FIELD_PREP(TCON_TOP_PORT_DE0_MSK, id);
> -       } else {
> -               DRM_DEBUG_DRIVER("TCON TOP mixer0 output is not connected\n");
> -               mixer0_unused = true;
> -       }
> -
> -       /* process mixer1 mux output */
> -       id = sun8i_tcon_top_get_connected_ep_id(dev->of_node, 3);
> -       if (id >= 0) {
> -               val |= FIELD_PREP(TCON_TOP_PORT_DE1_MSK, id);
> -
> -               /*
> -                * mixer0 mux has priority over mixer1 mux. We have to
> -                * make sure mixer0 doesn't overtake TCON from mixer1.
> -                */
> -               if (mixer0_unused && id == 0)
> -                       val |= FIELD_PREP(TCON_TOP_PORT_DE0_MSK, 1);
> -       } else {
> -               DRM_DEBUG_DRIVER("TCON TOP mixer1 output is not connected\n");
> -       }
> -
> -       writel(val, regs + TCON_TOP_PORT_SEL_REG);
> +       /*
> +        * Default register values might have some reserved bits set, which
> +        * prevents TCON TOP from working properly. Set them to 0 here.
> +        */
> +       writel(0, regs + TCON_TOP_GATE_SRC_REG);
> +       writel(0, regs + TCON_TOP_PORT_SEL_REG);

Would it make sense to just force a reset using the reset control?

ChenYu
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chen-Yu Tsai July 10, 2018, 4:12 p.m. UTC | #12
On Sat, Jul 7, 2018 at 1:51 AM, Jernej Skrabec <jernej.skrabec@siol.net> wrote:
> Till now, new way of matching engines with TCONs was reading their
> respective ids and match them by those ids. However, with introduction
> of TCON TOP, that might not be so straightforward anymore.
> - there might be more TCONs that engines (mixers)
> - TCON ids might have non-consecutive ids
>
> Workaround that by matching mixer id with TCON index from TCON list.
>
> For example, R40 has 2 mixers and 4 TCONs. Board designer can choose
> 2 outputs, which are connected to any of those 4 TCONs. As long as there
> are only 2 TCONs enabled in DT, using index in list as alternative id,
> will allow to match them with mixer 0 and 1.
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>

Reviewed-by: Chen-Yu Tsai <wens@csie.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chen-Yu Tsai July 10, 2018, 4:14 p.m. UTC | #13
On Sat, Jul 7, 2018 at 1:51 AM, Jernej Skrabec <jernej.skrabec@siol.net> wrote:
> R40 TV TCON is basically the same as on A83T. However, it needs special
> handling, because it has to set up TCON TOP muxes at runtime.
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>

Reviewed-by: Chen-Yu Tsai <wens@csie.org>

The runtime muxing of mixer <-> TCON is quite clever.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chen-Yu Tsai July 10, 2018, 4:16 p.m. UTC | #14
On Sat, Jul 7, 2018 at 1:51 AM, Jernej Skrabec <jernej.skrabec@siol.net> wrote:
> R40 has 4 TCONs, but only 2 of them can receive some kind of output at
> the same time. Let's disable them by default, so only those which are
> really connected on board can be enabled in board dts file.
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>

Originally I had wanted all TCONs enabled by default. But that would
require some more work for dynamic assignment of TCONs to CRTCs, which
is way out of scope for this series. So,

Reviewed-by: Chen-Yu Tsai <wens@csie.org>

I'll think about if and how this could be done.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jernej Škrabec July 10, 2018, 4:18 p.m. UTC | #15
Dne torek, 10. julij 2018 ob 18:09:26 CEST je Chen-Yu Tsai napisal(a):
> On Sat, Jul 7, 2018 at 1:51 AM, Jernej Skrabec <jernej.skrabec@siol.net> 
wrote:
> > Now that R40 TCON migrated to runtime mux configuration, old code can be
> > removed.
> > 
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > ---
> > 
> >  drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 81 +++-----------------------
> >  1 file changed, 7 insertions(+), 74 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c index c09b15b64192..78795d6cb174
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > @@ -87,34 +87,6 @@ int sun8i_tcon_top_de_config(struct device *dev, int
> > mixer, int tcon)> 
> >  }
> >  EXPORT_SYMBOL(sun8i_tcon_top_de_config);
> > 
> > -static int sun8i_tcon_top_get_connected_ep_id(struct device_node *node,
> > -                                             int port_id)
> > -{
> > -       struct device_node *ep, *remote, *port;
> > -       struct of_endpoint endpoint;
> > -
> > -       port = of_graph_get_port_by_id(node, port_id);
> > -       if (!port)
> > -               return -ENOENT;
> > -
> > -       for_each_available_child_of_node(port, ep) {
> > -               remote = of_graph_get_remote_port_parent(ep);
> > -               if (!remote)
> > -                       continue;
> > -
> > -               if (of_device_is_available(remote)) {
> > -                       of_graph_parse_endpoint(ep, &endpoint);
> > -
> > -                       of_node_put(remote);
> > -
> > -                       return endpoint.id;
> > -               }
> > -
> > -               of_node_put(remote);
> > -       }
> > -
> > -       return -ENOENT;
> > -}
> > 
> >  static struct clk_hw *sun8i_tcon_top_register_gate(struct device *dev,
> >  
> >                                                    const char *parent,
> > 
> > @@ -149,11 +121,9 @@ static int sun8i_tcon_top_bind(struct device *dev,
> > struct device *master,> 
> >         struct platform_device *pdev = to_platform_device(dev);
> >         struct clk_hw_onecell_data *clk_data;
> >         struct sun8i_tcon_top *tcon_top;
> > 
> > -       bool mixer0_unused = false;
> > 
> >         struct resource *res;
> >         void __iomem *regs;
> > 
> > -       int ret, i, id;
> > -       u32 val;
> > +       int ret, i;
> > 
> >         tcon_top = devm_kzalloc(dev, sizeof(*tcon_top), GFP_KERNEL);
> >         if (!tcon_top)
> > 
> > @@ -198,49 +168,12 @@ static int sun8i_tcon_top_bind(struct device *dev,
> > struct device *master,> 
> >                 goto err_assert_reset;
> >         
> >         }
> > 
> > -       val = 0;
> > -
> > -       /* check if HDMI mux output is connected */
> > -       if (sun8i_tcon_top_get_connected_ep_id(dev->of_node, 5) >= 0) {
> > -               /* find HDMI input endpoint id, if it is connected at
> > all*/
> > -               id = sun8i_tcon_top_get_connected_ep_id(dev->of_node, 4);
> > -               if (id >= 0)
> > -                       val = FIELD_PREP(TCON_TOP_HDMI_SRC_MSK, id + 1);
> > -               else
> > -                       DRM_DEBUG_DRIVER("TCON TOP HDMI input is not
> > connected\n"); -       } else {
> > -               DRM_DEBUG_DRIVER("TCON TOP HDMI output is not
> > connected\n"); -       }
> > -
> > -       writel(val, regs + TCON_TOP_GATE_SRC_REG);
> > -
> > -       val = 0;
> > -
> > -       /* process mixer0 mux output */
> > -       id = sun8i_tcon_top_get_connected_ep_id(dev->of_node, 1);
> > -       if (id >= 0) {
> > -               val = FIELD_PREP(TCON_TOP_PORT_DE0_MSK, id);
> > -       } else {
> > -               DRM_DEBUG_DRIVER("TCON TOP mixer0 output is not
> > connected\n"); -               mixer0_unused = true;
> > -       }
> > -
> > -       /* process mixer1 mux output */
> > -       id = sun8i_tcon_top_get_connected_ep_id(dev->of_node, 3);
> > -       if (id >= 0) {
> > -               val |= FIELD_PREP(TCON_TOP_PORT_DE1_MSK, id);
> > -
> > -               /*
> > -                * mixer0 mux has priority over mixer1 mux. We have to
> > -                * make sure mixer0 doesn't overtake TCON from mixer1.
> > -                */
> > -               if (mixer0_unused && id == 0)
> > -                       val |= FIELD_PREP(TCON_TOP_PORT_DE0_MSK, 1);
> > -       } else {
> > -               DRM_DEBUG_DRIVER("TCON TOP mixer1 output is not
> > connected\n"); -       }
> > -
> > -       writel(val, regs + TCON_TOP_PORT_SEL_REG);
> > +       /*
> > +        * Default register values might have some reserved bits set,
> > which
> > +        * prevents TCON TOP from working properly. Set them to 0 here.
> > +        */
> > +       writel(0, regs + TCON_TOP_GATE_SRC_REG);
> > +       writel(0, regs + TCON_TOP_PORT_SEL_REG);
> 
> Would it make sense to just force a reset using the reset control?

I wrote TCON TOP driver for H6 at first. For some reason, some registers had 
reserved bits set after reset line was released. TCON TOP was not used in U-
Boot, so I guess this counts as full reset.

I never tried to find out how it behaves on R40 because setting those 
registers to 0 seems like simple, good enough solution, which works 
everywhere.

Best regards,
Jernej



--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jernej Škrabec July 10, 2018, 7:41 p.m. UTC | #16
Dne torek, 10. julij 2018 ob 18:18:43 CEST je Jernej Škrabec napisal(a):
> Dne torek, 10. julij 2018 ob 18:09:26 CEST je Chen-Yu Tsai napisal(a):
> > On Sat, Jul 7, 2018 at 1:51 AM, Jernej Skrabec <jernej.skrabec@siol.net>
> 
> wrote:
> > > Now that R40 TCON migrated to runtime mux configuration, old code can be
> > > removed.
> > > 
> > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > ---
> > > 
> > >  drivers/gpu/drm/sun4i/sun8i_tcon_top.c | 81 +++-----------------------
> > >  1 file changed, 7 insertions(+), 74 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > > b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c index
> > > c09b15b64192..78795d6cb174
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_tcon_top.c
> > > @@ -87,34 +87,6 @@ int sun8i_tcon_top_de_config(struct device *dev, int
> > > mixer, int tcon)>
> > > 
> > >  }
> > >  EXPORT_SYMBOL(sun8i_tcon_top_de_config);
> > > 
> > > -static int sun8i_tcon_top_get_connected_ep_id(struct device_node *node,
> > > -                                             int port_id)
> > > -{
> > > -       struct device_node *ep, *remote, *port;
> > > -       struct of_endpoint endpoint;
> > > -
> > > -       port = of_graph_get_port_by_id(node, port_id);
> > > -       if (!port)
> > > -               return -ENOENT;
> > > -
> > > -       for_each_available_child_of_node(port, ep) {
> > > -               remote = of_graph_get_remote_port_parent(ep);
> > > -               if (!remote)
> > > -                       continue;
> > > -
> > > -               if (of_device_is_available(remote)) {
> > > -                       of_graph_parse_endpoint(ep, &endpoint);
> > > -
> > > -                       of_node_put(remote);
> > > -
> > > -                       return endpoint.id;
> > > -               }
> > > -
> > > -               of_node_put(remote);
> > > -       }
> > > -
> > > -       return -ENOENT;
> > > -}
> > > 
> > >  static struct clk_hw *sun8i_tcon_top_register_gate(struct device *dev,
> > >  
> > >                                                    const char *parent,
> > > 
> > > @@ -149,11 +121,9 @@ static int sun8i_tcon_top_bind(struct device *dev,
> > > struct device *master,>
> > > 
> > >         struct platform_device *pdev = to_platform_device(dev);
> > >         struct clk_hw_onecell_data *clk_data;
> > >         struct sun8i_tcon_top *tcon_top;
> > > 
> > > -       bool mixer0_unused = false;
> > > 
> > >         struct resource *res;
> > >         void __iomem *regs;
> > > 
> > > -       int ret, i, id;
> > > -       u32 val;
> > > +       int ret, i;
> > > 
> > >         tcon_top = devm_kzalloc(dev, sizeof(*tcon_top), GFP_KERNEL);
> > >         if (!tcon_top)
> > > 
> > > @@ -198,49 +168,12 @@ static int sun8i_tcon_top_bind(struct device *dev,
> > > struct device *master,>
> > > 
> > >                 goto err_assert_reset;
> > >         
> > >         }
> > > 
> > > -       val = 0;
> > > -
> > > -       /* check if HDMI mux output is connected */
> > > -       if (sun8i_tcon_top_get_connected_ep_id(dev->of_node, 5) >= 0) {
> > > -               /* find HDMI input endpoint id, if it is connected at
> > > all*/
> > > -               id = sun8i_tcon_top_get_connected_ep_id(dev->of_node,
> > > 4);
> > > -               if (id >= 0)
> > > -                       val = FIELD_PREP(TCON_TOP_HDMI_SRC_MSK, id + 1);
> > > -               else
> > > -                       DRM_DEBUG_DRIVER("TCON TOP HDMI input is not
> > > connected\n"); -       } else {
> > > -               DRM_DEBUG_DRIVER("TCON TOP HDMI output is not
> > > connected\n"); -       }
> > > -
> > > -       writel(val, regs + TCON_TOP_GATE_SRC_REG);
> > > -
> > > -       val = 0;
> > > -
> > > -       /* process mixer0 mux output */
> > > -       id = sun8i_tcon_top_get_connected_ep_id(dev->of_node, 1);
> > > -       if (id >= 0) {
> > > -               val = FIELD_PREP(TCON_TOP_PORT_DE0_MSK, id);
> > > -       } else {
> > > -               DRM_DEBUG_DRIVER("TCON TOP mixer0 output is not
> > > connected\n"); -               mixer0_unused = true;
> > > -       }
> > > -
> > > -       /* process mixer1 mux output */
> > > -       id = sun8i_tcon_top_get_connected_ep_id(dev->of_node, 3);
> > > -       if (id >= 0) {
> > > -               val |= FIELD_PREP(TCON_TOP_PORT_DE1_MSK, id);
> > > -
> > > -               /*
> > > -                * mixer0 mux has priority over mixer1 mux. We have to
> > > -                * make sure mixer0 doesn't overtake TCON from mixer1.
> > > -                */
> > > -               if (mixer0_unused && id == 0)
> > > -                       val |= FIELD_PREP(TCON_TOP_PORT_DE0_MSK, 1);
> > > -       } else {
> > > -               DRM_DEBUG_DRIVER("TCON TOP mixer1 output is not
> > > connected\n"); -       }
> > > -
> > > -       writel(val, regs + TCON_TOP_PORT_SEL_REG);
> > > +       /*
> > > +        * Default register values might have some reserved bits set,
> > > which
> > > +        * prevents TCON TOP from working properly. Set them to 0 here.
> > > +        */
> > > +       writel(0, regs + TCON_TOP_GATE_SRC_REG);
> > > +       writel(0, regs + TCON_TOP_PORT_SEL_REG);
> > 
> > Would it make sense to just force a reset using the reset control?
> 
> I wrote TCON TOP driver for H6 at first. For some reason, some registers had
> reserved bits set after reset line was released. TCON TOP was not used in
> U- Boot, so I guess this counts as full reset.
> 
> I never tried to find out how it behaves on R40 because setting those
> registers to 0 seems like simple, good enough solution, which works
> everywhere.

It seems those bits are not needed for R40. I'll leave them out for now. TCON 
TOP will need some adjustments for H6 anyway and I'll take a look at that 
then.
 
Best regards,
Jernej




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