于 2017年6月7日 GMT+08:00 下午5:35:12, Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx> 写到:
>On Mon, Jun 05, 2017 at 12:01:41AM +0800, Icenowy Zheng wrote:
>> Some SoC's DE2 has two mixers. Defaultly the mixer0 is connected to
>> tcon0 and mixer1 is connected to tcon1; however by setting a bit
>> the connection can be swapped.
>>
>> As we now hardcode the default connection, ignore the bonus endpoint
>for
>> the mixer's output and the TCON's input, as they stands for the
>swapped
>> connection.
>>
>> Signed-off-by: Icenowy Zheng <icenowy@xxxxxxx>
>
>So, I'm still not quite sure what this patch exactly is supposed to be
>doing.
>
>You mention that the routing between the mixers and tcons can be
>changed, and that we need to ignore the TCON input...
Yes, from the TCON perspective, the connection from
mixer0 to TCON1 and from mixer 1 to TCON0 should be
ignored. And from the mixer perspective, the connections should
also be omitted when binding in sun4i_drv.c.
>
>> ---
>> Changes in v2:
>> - Change to use new endpoint reg definition.
>>
>> drivers/gpu/drm/sun4i/sun4i_drv.c | 45 ++++++++++++++++++++++++++++
>> drivers/gpu/drm/sun4i/sun4i_tcon.c | 61
>++++++++++++++++++++++++++++++++------
>> drivers/gpu/drm/sun4i/sun4i_tcon.h | 2 ++
>> 3 files changed, 99 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c
>b/drivers/gpu/drm/sun4i/sun4i_drv.c
>> index f19100c91c2b..775eee82d8a9 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_drv.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
>> @@ -178,6 +178,13 @@ static bool sun4i_drv_node_is_frontend(struct
>device_node *node)
>> of_device_is_compatible(node,
>"allwinner,sun8i-a33-display-frontend");
>> }
>>
>> +static bool sun4i_drv_node_is_swappable_de2_mixer(struct device_node
>*node)
>> +{
>> + /* The V3s has only one mixer-tcon pair, so it's not listed here.
>*/
>> + return of_device_is_compatible(node,
>"allwinner,sun8i-h3-de2-mixer0") ||
>> + of_device_is_compatible(node, "allwinner,sun8i-h3-de2-mixer1");
>> +}
>> +
>> static bool sun4i_drv_node_is_tcon(struct device_node *node)
>> {
>> return of_device_is_compatible(node, "allwinner,sun5i-a13-tcon") ||
>> @@ -261,6 +268,44 @@ static int sun4i_drv_add_endpoints(struct device
>*dev,
>> }
>> }
>>
>> + /*
>> + * The second endpoint of the output of a swappable DE2 mixer
>> + * is the TCON after connection swapping.
>> + * Ignore it now, as we now hardcode mixer0->tcon0,
>> + * mixer1->tcon1 connection.
>> + */
>> + if (sun4i_drv_node_is_swappable_de2_mixer(node)) {
>> + struct device_node *remote_ep_node;
>> + struct of_endpoint local_endpoint, remote_endpoint;
>> +
>> + remote_ep_node = of_graph_get_remote_endpoint(ep);
>> + if (!remote_ep_node) {
>> + DRM_DEBUG_DRIVER("Couldn't get remote endpoint\n");
>> + continue;
>> + }
>> +
>> + if (of_graph_parse_endpoint(ep, &local_endpoint)) {
>> + DRM_DEBUG_DRIVER("Couldn't parse local endpoint\n");
>> + of_node_put(remote_ep_node);
>> + continue;
>> + }
>> +
>> + if (of_graph_parse_endpoint(remote_ep_node,
>> + &remote_endpoint)) {
>> + DRM_DEBUG_DRIVER("Couldn't parse remote endpoint\n");
>> + of_node_put(remote_ep_node);
>> + continue;
>> + }
>> +
>> + if (local_endpoint.id != remote_endpoint.id) {
>> + DRM_DEBUG_DRIVER("Endpoint is an unused connection for DE2
>mixer... skipping\n");
>> + of_node_put(remote_ep_node);
>> + continue;
>> + }
>> +
>> + of_node_put(remote_ep_node);
>> + }
>> +
>> /* Walk down our tree */
>> count += sun4i_drv_add_endpoints(dev, match, remote);
>
>... yet this is not parsing the input at all, but only the output
>nodes.
Yes.
Mixers will have two output endpoints, but we will only use
the same id one and ignore the swapped one.
So does the situation of TCON input.
>
>
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> index d9791292553e..568cea0e5f8f 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c
>> @@ -464,7 +464,8 @@ static int sun4i_tcon_init_regmap(struct device
>*dev,
>> * requested via the get_id function of the engine.
>> */
>> static struct sunxi_engine *sun4i_tcon_find_engine(struct sun4i_drv
>*drv,
>> - struct device_node *node)
>> + struct device_node *node,
>> + bool skip_bonus_ep)
>> {
>> struct device_node *port, *ep, *remote;
>> struct sunxi_engine *engine;
>> @@ -478,6 +479,42 @@ static struct sunxi_engine
>*sun4i_tcon_find_engine(struct sun4i_drv *drv,
>> if (!remote)
>> continue;
>>
>> + if (skip_bonus_ep) {
>> + struct device_node *remote_ep_node;
>> + struct of_endpoint local_endpoint, remote_endpoint;
>> +
>> + remote_ep_node = of_graph_get_remote_endpoint(ep);
>> + if (!remote_ep_node) {
>> + DRM_DEBUG_DRIVER("Couldn't get remote endpoint\n");
>> + of_node_put(remote);
>> + continue;
>> + }
>> +
>> + if (of_graph_parse_endpoint(ep, &local_endpoint)) {
>> + DRM_DEBUG_DRIVER("Couldn't parse local endpoint\n");
>> + of_node_put(remote);
>> + of_node_put(remote_ep_node);
>> + continue;
>> + }
>> +
>> + if (of_graph_parse_endpoint(remote_ep_node,
>> + &remote_endpoint)) {
>> + DRM_DEBUG_DRIVER("Couldn't parse remote endpoint\n");
>> + of_node_put(remote);
>> + of_node_put(remote_ep_node);
>> + continue;
>> + }
>> +
>> + if (local_endpoint.id != remote_endpoint.id) {
>> + DRM_DEBUG_DRIVER("Skipping bonus mixer->TCON connection when
>searching engine\n");
>> + of_node_put(remote);
>> + of_node_put(remote_ep_node);
>> + continue;
>> + }
>> +
>> + of_node_put(remote_ep_node);
>> + }
>> +
>
>I have no idea what this is supposed to be doing either.
>
>I might be wrong, but I really feel like there's a big mismatch
>between your commit log, and what you actually implement.
>
>In your commit log, you should state:
>
>A) What is the current behaviour
>B) Why that is a problem
>C) How do you address it
>
>And you don't.
>
>However, after discussing it with Chen-Yu, it seems like you're trying
>to have all the mixers probed before the TCONs. If that is so, there's
>nothing specific to the H3 here, and we also have the same issue on
>dual-pipeline DE1 (A10, A20, A31). Chen-Yu worked on that a bit, but
>the easiest solution would be to move from a DFS algorithm to walk
>down the graph to a BFS one.
>
>That way, we would add all mixers first, then the TCONs, then the
>encoders, and the component framework will probe them in order.
No. I said that they're swappable, however, I don't
want to implement the swap now, but hardcode 0-0 1-1
connection. However, as you and Chen-Yu said, device tree
should reflect the real hardware, there will be bonus endpoints
for the swapped connection.
What I want to do is to ignore the bonus connection, in order to
prevent them from confusing the code.
If you just change the bind sequence, I think it cannot be
prevented that wrong connections will be bound.
>
>Maxime
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
