From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-9.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3FA1AC2BBD0 for ; Tue, 8 Sep 2020 08:47:02 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id ECF1A215A4 for ; Tue, 8 Sep 2020 08:47:01 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=ffwll.ch header.i=@ffwll.ch header.b="fBtJVR0h" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730189AbgIHIq6 (ORCPT ); Tue, 8 Sep 2020 04:46:58 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43934 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730175AbgIHIqr (ORCPT ); Tue, 8 Sep 2020 04:46:47 -0400 Received: from mail-wr1-x442.google.com (mail-wr1-x442.google.com [IPv6:2a00:1450:4864:20::442]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2AA62C061573 for ; Tue, 8 Sep 2020 01:46:46 -0700 (PDT) Received: by mail-wr1-x442.google.com with SMTP id a17so18171658wrn.6 for ; Tue, 08 Sep 2020 01:46:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=date:from:to:cc:subject:message-id:mail-followup-to:references :mime-version:content-disposition:in-reply-to; bh=WsjbvmmZKMC8MceZdsV0yPxKISX87E/DDuYAfGmxSEY=; b=fBtJVR0hdGCRQN6pQejVwyqvj4WARlY7n3kR5Jq6DLPNpw/t/j7MeChvHRdzRZDPBs P3CS2hNHu4yzXcjzDQYEKra3JyH7GWLo8t1YJgPVWDWBVZM8fKghLGZYxNa5weIiCmC2 dHQD21mCUbCQIUpsW1v3omqZ+25OJkI/yQbMw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-disposition :in-reply-to; bh=WsjbvmmZKMC8MceZdsV0yPxKISX87E/DDuYAfGmxSEY=; b=jAphjSWBF70Rd26oNfZK2BiMA7LY5RmyekeJlnCDDoeGirluo4+A8Noqx04WeXr8Ng oeo/GNCqWd6YWPH9/7Nvu8fDjFaKKVX4sX4n/oH2qOZStpenLR4WuygX001ejJTyqrZT /An0Nor758v4MLzbRheYCmvX4Cb8dG5BXFc5YIO2zI3DYwa9601C2kHbpjFedRmg+sEn MJYpLODbi8FLmAM9W4ChQHRelxmyS3NhT59t+nMMDVwEt8La5IfJMF3tYR2juLCYzJIf cpMR+H6G8N2FZA2G0oK9HaQT2eohKiDxPTnJT9qa01juRem2Fu9luXXeZAnPpC3S0T0W QGhg== X-Gm-Message-State: AOAM530/OIziwlUswcGEL5aWT1V8P8zEjqhM/vOVHQfGEuoPiKh2DSab Zvyv7vIU/7vzjsNLYiImAetKxg== X-Google-Smtp-Source: ABdhPJwlRwBcD/dkDnmeNt/pmsZ/RICLEMEd9+YvOB934Q64ggiWpJyKdIPX011RteQqsUvs1SSr2Q== X-Received: by 2002:adf:81e6:: with SMTP id 93mr25732190wra.412.1599554804650; Tue, 08 Sep 2020 01:46:44 -0700 (PDT) Received: from phenom.ffwll.local ([2a02:168:57f4:0:efd0:b9e5:5ae6:c2fa]) by smtp.gmail.com with ESMTPSA id f19sm31169981wmh.44.2020.09.08.01.46.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 08 Sep 2020 01:46:43 -0700 (PDT) Date: Tue, 8 Sep 2020 10:46:42 +0200 From: Daniel Vetter To: Neil Armstrong Cc: dri-devel@lists.freedesktop.org, linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 6/6] drm/meson: add support for MIPI-DSI transceiver Message-ID: <20200908084642.GG2352366@phenom.ffwll.local> Mail-Followup-To: Neil Armstrong , dri-devel@lists.freedesktop.org, linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org References: <20200907081825.1654-1-narmstrong@baylibre.com> <20200907081825.1654-7-narmstrong@baylibre.com> <20200907084351.GV2352366@phenom.ffwll.local> <20200907084423.GW2352366@phenom.ffwll.local> <20200907180315.GY2352366@phenom.ffwll.local> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Operating-System: Linux phenom 5.7.0-1-amd64 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Sep 08, 2020 at 10:06:03AM +0200, Neil Armstrong wrote: > Hi, > > On 07/09/2020 20:03, Daniel Vetter wrote: > > On Mon, Sep 07, 2020 at 11:03:29AM +0200, Neil Armstrong wrote: > >> On 07/09/2020 10:44, Daniel Vetter wrote: > >>> On Mon, Sep 07, 2020 at 10:43:51AM +0200, Daniel Vetter wrote: > >>>> On Mon, Sep 07, 2020 at 10:18:25AM +0200, Neil Armstrong wrote: > >>>>> The Amlogic AXg SoCs embeds a Synopsys DW-MIPI-DSI transceiver (ver 1.21a), with a custom > >>>>> glue managing the IP resets, clock and data input similar to the DW-HDMI Glue on other > >>>>> Amlogic SoCs. > >>>>> > >>>>> This adds support for the Glue managing the transceiver, mimicing the init flow provided > >>>>> by Amlogic to setup the ENCl encoder, the glue, the transceiver, the digital D-PHY and the > >>>>> Analog PHY in the proper way. > >>>>> > >>>>> The DW-MIPI-DSI transceiver + D-PHY are directly clocked by the VCLK2 clock, which pixel clock > >>>>> is derived and feeds the ENCL encoder and the VIU pixel reader. > >>>>> > >>>>> An optional "MEAS" clock can be enabled to measure the delay between each vsync feeding the > >>>>> DW-MIPI-DSI transceiver. > >>>>> > >>>>> Signed-off-by: Neil Armstrong > >>>> > >>>> More dw-hdmi drivers which aren't bridges but components, and the thing is > >>>> still midlayer-y as heck :-/ > >>> > >>> *dw-dsi, but really they both work the same way and should both be fixed > >>> ... > >> > >> They are bridges but since they have platform-dependent code due to theirs's generic IP > >> nature, they need to be intanciated by components to sync with the platform code. > > > > Yeah that's how it currently works, but there's not much reason why it > > needs to work like that. That platform glue code can also be put behind > > the drm_bridge abstraction, and we could use the normal dt based bridge > > lookup like for everything else. > > > > Afaiui the only reason dw-* bridge drivers work like that is because for > > historical reasons we only had component.c at first, and none of the more > > fancy dynamic bridge lookup. So would be really good to switch this all > > over to a proper&clean bridge abstraction, and not leak everything around. > > Does it mean we should avoit using components, right ? Yup, at least when there's an established specific mechanism to handle cross-driver interactions/EPROBE_DEFER. So if you want a drm_panel or drm_bridge or a clock or i2c or anything else standardized like that, no component.c please. That's just for the driver-specific glue when you have entirely IP-specific way of building up a driver from components, which will never be reused outside of that overall driver. Hence why dw-* using components is rather uncool. > > I've typed up what I think should be the way forward a few times already, > > but thus far not even the todo.rst entry materialized: > > > > https://lore.kernel.org/dri-devel/20200430135841.GE10381@phenom.ffwll.local/ > > > > If everyone is always about "not in my patch series" it'll never happen. > > Today, the dw-mipi-dsi and dw-mipi-hdmi has mandatory callbacks to implement > vendor specific features, like : > - hdmi/dsi phy handling when mixed into the glue/custom/synopsys but with platform specific values > - platform specific mode validation > - hpd setup => could use laurent's work here with no_connector, but how do we handle rxsense ??? > - dsi timings/lane mbps ? these are platform phy specific > > For amlogic, I can split the "component" code handling venc/vclk into a primary bridge and add a > secondary driver only handling the glue around dw-mipi-dsi/dw-mipi-hdmi, would that be a good start ? I think what we should do here: - type up todo.rst with the overall structure we want to aim for, i.e. where is the code, who sets up what, how are the bridges bound into the overall driver. - demidlayer dw-* enough so that the callbacks are gone, and it becomes more a toolbox/library for building a dw-* driver. Maybe we're there already. - All new drivers then need to use the toolbox and have everything behind a drm_bridge driver in drm/bridge/, with just default binding exposed to drivers, no EXPORT_SYMBOL at all. Also no exported symbols used, this should all be directly linked into the dw-*.ko imo and treated as internals. - We might need to split the header files to make that clean enough. - Once all existing dw-* users are converted, we ditch the EXPORT_SYMBOL stuff completely (since I expect we'll just have one overall dw-dsi.ko module with all bridge driver variants). Cheers, Daniel > > Neil > > > > > Cheers, Daniel > > > > > >> > >> Neil > >> > >>> > >>>> > >>>> Can we try to fix this? There's a ton of this going on, and the more we > >>>> add the old fashioned way the harder this gets to fix up for real. > >>>> -Daniel > >>>> > >>>>> --- > >>>>> drivers/gpu/drm/meson/Kconfig | 7 + > >>>>> drivers/gpu/drm/meson/Makefile | 1 + > >>>>> drivers/gpu/drm/meson/meson_dw_mipi_dsi.c | 562 ++++++++++++++++++++++ > >>>>> 3 files changed, 570 insertions(+) > >>>>> create mode 100644 drivers/gpu/drm/meson/meson_dw_mipi_dsi.c > >>>>> > >>>>> diff --git a/drivers/gpu/drm/meson/Kconfig b/drivers/gpu/drm/meson/Kconfig > >>>>> index 9f9281dd49f8..385f6f23839b 100644 > >>>>> --- a/drivers/gpu/drm/meson/Kconfig > >>>>> +++ b/drivers/gpu/drm/meson/Kconfig > >>>>> @@ -16,3 +16,10 @@ config DRM_MESON_DW_HDMI > >>>>> default y if DRM_MESON > >>>>> select DRM_DW_HDMI > >>>>> imply DRM_DW_HDMI_I2S_AUDIO > >>>>> + > >>>>> +config DRM_MESON_DW_MIPI_DSI > >>>>> + tristate "MIPI DSI Synopsys Controller support for Amlogic Meson Display" > >>>>> + depends on DRM_MESON > >>>>> + default y if DRM_MESON > >>>>> + select DRM_DW_MIPI_DSI > >>>>> + select GENERIC_PHY_MIPI_DPHY > >>>>> diff --git a/drivers/gpu/drm/meson/Makefile b/drivers/gpu/drm/meson/Makefile > >>>>> index 28a519cdf66b..2cc870e91182 100644 > >>>>> --- a/drivers/gpu/drm/meson/Makefile > >>>>> +++ b/drivers/gpu/drm/meson/Makefile > >>>>> @@ -5,3 +5,4 @@ meson-drm-y += meson_rdma.o meson_osd_afbcd.o > >>>>> > >>>>> obj-$(CONFIG_DRM_MESON) += meson-drm.o > >>>>> obj-$(CONFIG_DRM_MESON_DW_HDMI) += meson_dw_hdmi.o > >>>>> +obj-$(CONFIG_DRM_MESON_DW_MIPI_DSI) += meson_dw_mipi_dsi.o > >>>>> diff --git a/drivers/gpu/drm/meson/meson_dw_mipi_dsi.c b/drivers/gpu/drm/meson/meson_dw_mipi_dsi.c > >>>>> new file mode 100644 > >>>>> index 000000000000..bbe1294fce7c > >>>>> --- /dev/null > >>>>> +++ b/drivers/gpu/drm/meson/meson_dw_mipi_dsi.c > >>>>> @@ -0,0 +1,562 @@ > >>>>> +// SPDX-License-Identifier: GPL-2.0-or-later > >>>>> +/* > >>>>> + * Copyright (C) 2016 BayLibre, SAS > >>>>> + * Author: Neil Armstrong > >>>>> + * Copyright (C) 2015 Amlogic, Inc. All rights reserved. > >>>>> + */ > >>>>> + > >>>>> +#include > >>>>> +#include > >>>>> +#include > >>>>> +#include > >>>>> +#include > >>>>> +#include > >>>>> +#include > >>>>> +#include > >>>>> + > >>>>> +#include