Re: [RFC PATCH 1/2] drm: bridge: anx7688: Add anx7688 bridge driver support.

From: Nicolas Boichat
Date: Wed Jun 22 2016 - 02:39:12 EST

+Philipp

On Wed, Jun 22, 2016 at 11:54 AM, Archit Taneja <architt@xxxxxxxxxxxxxx> wrote:
>
>
> On 6/22/2016 8:14 AM, Nicolas Boichat wrote:
>>
>> Hi Archit,
>>
>> Thanks for your reply.
>>
>> On Tue, Jun 21, 2016 at 11:39 PM, Archit Taneja <architt@xxxxxxxxxxxxxx>
>> wrote:
>>>
>>> Hi,
>>>
>>> On 6/20/2016 12:44 PM, Nicolas Boichat wrote:
>>>>
>>>>
>>>> ANX7688 is a HDMI to DP converter (as well as USB-C port controller),
>>>> that has an internal microcontroller.
>>>>
>>>> The only reason a Linux kernel driver is necessary is to reject
>>>> resolutions that require more bandwidth than what is available on
>>>> the DP side. DP bandwidth and lane count are reported by the bridge
>>>> via 2 registers on I2C.
>>>
>>>
>>>
>>> How does the chip know when to enable/disable itself? Does it shutoff
>>> itself if there isn't anything on the HDMI link?
>>
>>
>> Not 100% sure of the internals (there is a closed source firmware in
>> the chip), but I believe the HDMI/DP part of the chip is switched on
>> whenever there is a DP over USB-C connector plugged in.
>>
>>>>
>>>> Signed-off-by: Nicolas Boichat <drinkcat@xxxxxxxxxxxx>
>>>> ---
>>>>
>>>> Hi,
>>>>
>>>> I tested this driver using the Mediatek HDMI controller (MT8173)
>>>> upstream
>>>> of the ANX bridge chip (Phillip sent a PULL request on June 13:
>>>> git://git.pengutronix.de/git/pza/linux.git tags/mediatek-drm-2016-06-13
>>>> ).
>>>>
>>>> I have 2 concerns, that I'm not sure how to address within the kernel
>>>> DRM
>>>> framework:
>>>> 1. All other bridge drivers also have a connector attached to it.
>>>> However, in
>>>> this case, we cannot read the monitor EDID directly, so I'm not
>>>> sure
>>>> what
>>>> I could put in a "get_modes" function. Instead, ANX7688 provides a
>>>> I2C
>>>> passthru/repeater, so the EDID is read on the Mediatek HDMI
>>>> controller side.
>>>>
>>>> That leads to a somewhat strange layout, where we have:
>>>> - MTK HDMI bridge/encoder
>>>> - MTK connector (HDMI)
>>>> - ANX7688 bridge
>>>> - No connector
>>>
>>>
>>>
>>>
>>> You should ideally have one DP connector at the end of the chain:
>>>
>>> - MTK HDMI bridge/encoder
>>> - ANX7688 bridge
>>> - Connector (DP)
>>>
>>> In the dt-bindings for this board, hdmi's output port shouldn't be
>>> connected to a hdmi connector, but the input port of the ANX7688
>>> DP bridge. The output port of the bridge should be the one that
>>> connects to the DP connector.
>>
>>
>> Yes that's what I do (in the device tree).
>>
>> Actually, experimenting a bit more with the code, I realized that the
>> connector is always attached to the encoder, not the bridge, so the 2
>> layouts above are actually identical (from the userspace point of
>> view). Except that the connector name should be HDMI in one case, and
>> DP in the other. But I think that's mostly a cosmetic difference?
>
>
> Yeah, probably. I don't know what exactly the userspace does with
> the connector type, but it would be nice to represent it as a DP
> connector in case it makes any decisions based on it.

AFAICT does not matter... And, in any case, USB-C to HDMI adapters are
far more common (so we'd do HDMI->(DP over USB-C)->HDMI....), so there
is a high change that advertising as DP would be wrong (and
advertising as HDMI would be correct by chance...).

>>
>>> One way I can think of fixing this is to make make the MTK hdmi
>>> encoder driver a bit smarter by observing the DT connections. If
>>> it's output port is connected to just a hdmi-connector, then
>>> things should be as before. If the output is connected to the DP
>>> bridge, then it should create a DP connector. The connector ops
>>> for the DP connector can still be the same as that of the HDMI
>>> connector before, but the phandle to the DDC bus would be in the
>>> DP device node in this case.
>>
>>
>> I think it'd be a bit weird to have the DDC bus phandle on the DP
>> connector, as we're not reading the EDID on the DP side of the bridge,
>> but on the HDMI side (and the bridge can do all sort of things to the
>> EDID: At the very least, I think it caches it).
>
>
> On the board, do the DDC lines join directly from the SoC pins to the
> DP connector, or does it go via the ANX7688 chip?

The DDC/I2C lines go to the ANX7688 chip. (DP has AUX channel instead)

> Even with the current bindings (referred from the link below), the
> hdmi connector has the DDC node. Shouldn't it be the same in the DP
> case too? The DP connector, like before, is still manged by the HDMI
> driver, the only difference being the name and that it's two hops away
> in the DT bindings.
>
> https://patchwork.kernel.org/patch/9137089/

True, the bindings say so, but the current code actually looks for
ddc-i2c-bus property in whatever is connected on the endpoint (be it a
bridge or a connector).

And again, a bit odd as DP connector does not have true I2C lines...

Phillip? Any opinion?

>>
>>> This way, you can get around having the correct layout.
>>>
>>> Ideally, a bridge driver shouldn't be the one that creates a
>>> connector. It may contain some of the connector functionality, but
>>> the connector creation should be managed by the kms driver.
>>> Almost all bridge drivers creating a connector in their .attach
>>> callbacks since they own some of the connector functionality (like
>>> reading EDID). That's something we're trying to fix by providing
>>> some more bridge api that kms drivers can use.
>>>
>>> Since this bridge driver doesn't have any connector functionality
>>> anyway, you should be okay.
>>
>>
>> Great, thanks for clarifying.
>>
>>>>
>>>> Resolution filtering works fine though, thanks to mode_fixup
>>>> callback
>>>> on the
>>>> bridge. It also helps that Mediatek HDMI bridge calls mode_fixup
>>>> from
>>>> its
>>>> connector mode_valid callback, so that invalid modes are not even
>>>> presented
>>>> to userspace.
>>>>
>>>> 2. In the bandwidth computation, I hard-code 8-bit per channel (bpc).
>>>> bpc does
>>>> not seem to be included in the mode setting itself. We could
>>>> possibly
>>>> iterate
>>>> over connectors on the DRM device, but then, IIUC,
>>>> connector->display_info.bpc
>>>> indicates the _maximum_ bpc supported by the monitor.
>>>
>>>
>>>
>>> I'm not clear about this either. Some drivers set a bus format
>>> on the connector via drm_display_info_set_bus_formats in their
>>> get_modes connector op, and then retrieve it later.
>>
>>
>> Ah, interesting... Seems like we'd need a big switch/case to convert
>> from bus_format to bpp, though.
>>
>>>>
>>>> Any pointers? Thanks!
>>>>
>>>> Best,
>>>>
>>>> Nicolas
>>>>
>>>> drivers/gpu/drm/bridge/Kconfig | 9 ++
>>>> drivers/gpu/drm/bridge/Makefile | 1 +
>>>> drivers/gpu/drm/bridge/analogix-anx7688.c | 227
>>>> ++++++++++++++++++++++++++++++
>>>> 3 files changed, 237 insertions(+)
>>>> create mode 100644 drivers/gpu/drm/bridge/analogix-anx7688.c
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/Kconfig
>>>> b/drivers/gpu/drm/bridge/Kconfig
>>>> index 8f7423f..0c1eb41 100644
>>>> --- a/drivers/gpu/drm/bridge/Kconfig
>>>> +++ b/drivers/gpu/drm/bridge/Kconfig
>>>> @@ -7,6 +7,15 @@ config DRM_BRIDGE
>>>> menu "Display Interface Bridges"
>>>> depends on DRM && DRM_BRIDGE
>>>>
>>>> +config DRM_ANALOGIX_ANX7688
>>>> + tristate "Analogix ANX7688 bridge"
>>>> + depends on DRM
>>>> + select DRM_KMS_HELPER
>>>> + ---help---
>>>> + ANX7688 is a transmitter to support DisplayPort over USB-C for
>>>> + smartphone and tablets.
>>>> + This driver only supports the HDMI to DP component of the
>>>> chip.
>>>> +
>>>> config DRM_ANALOGIX_ANX78XX
>>>> tristate "Analogix ANX78XX bridge"
>>>> select DRM_KMS_HELPER
>>>> diff --git a/drivers/gpu/drm/bridge/Makefile
>>>> b/drivers/gpu/drm/bridge/Makefile
>>>> index 96b13b3..d744c6c 100644
>>>> --- a/drivers/gpu/drm/bridge/Makefile
>>>> +++ b/drivers/gpu/drm/bridge/Makefile
>>>> @@ -1,5 +1,6 @@
>>>> ccflags-y := -Iinclude/drm
>>>>
>>>> +obj-$(CONFIG_DRM_ANALOGIX_ANX7688) += analogix-anx7688.o
>>>> obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
>>>> obj-$(CONFIG_DRM_DW_HDMI) += dw-hdmi.o
>>>> obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw-hdmi-ahb-audio.o
>>>> diff --git a/drivers/gpu/drm/bridge/analogix-anx7688.c
>>>> b/drivers/gpu/drm/bridge/analogix-anx7688.c
>>>> new file mode 100644
>>>> index 0000000..2c34029
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/bridge/analogix-anx7688.c
>>>> @@ -0,0 +1,227 @@
>>>> +/*
>>>> + * ANX7688 HDMI->DP bridge driver
>>>> + *
>>>> + * Copyright (C) 2016 Google, Inc.
>>>> + *
>>>> + * This software is licensed under the terms of the GNU General Public
>>>> + * License version 2, as published by the Free Software Foundation, and
>>>> + * may be copied, distributed, and modified under those terms.
>>>> + *
>>>> + * This program is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>>> + * GNU General Public License for more details.
>>>> + */
>>>> +
>>>> +#include <linux/delay.h>
>>>> +#include <linux/gpio.h>
>>>> +#include <linux/i2c.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_gpio.h>
>>>> +#include <drm/drmP.h>
>>>
>>>
>>>
>>> The 3 headers above aren't needed.
>>
>>
>> Nor gpio.h. Removed.
>>
>>>
>>>> +#include <drm/drm_crtc.h>
>>>> +
>>>> +/* Register addresses */
>>>> +#define VENDOR_ID_REG 0x00
>>>> +#define DEVICE_ID_REG 0x02
>>>> +
>>>> +#define FW_VERSION_REG 0x80
>>>> +
>>>> +#define DP_BANDWIDTH_REG 0x85
>>>> +#define DP_LANE_COUNT_REG 0x86
>>>> +
>>>> +#define VENDOR_ID 0x1f29
>>>> +#define DEVICE_ID 0x7688
>>>> +
>>>> +/* First supported firmware version (0.85) */
>>>> +#define MINIMUM_FW_VERSION 0x0085
>>>> +
>>>> +struct anx7688 {
>>>> + struct drm_bridge bridge;
>>>> + struct i2c_client *client;
>>>> +
>>>> + bool filter;
>>>> +};
>>>> +
>>>> +static int anx7688_read(struct i2c_client *client, u8 reg, u8 *data,
>>>> + u16 data_len)
>>>> +{
>>>> + int ret;
>>>> + struct i2c_msg msgs[] = {
>>>> + {
>>>> + .addr = client->addr,
>>>> + .flags = 0,
>>>> + .len = 1,
>>>> + .buf = &reg,
>>>> + },
>>>> + {
>>>> + .addr = client->addr,
>>>> + .flags = I2C_M_RD,
>>>> + .len = data_len,
>>>> + .buf = data,
>>>> + }
>>>> + };
>>>> +
>>>> + ret = i2c_transfer(client->adapter, msgs, 2);
>>>> +
>>>> + if (ret == 2)
>>>> + return 0;
>>>> + if (ret < 0)
>>>> + return ret;
>>>> + else
>>>> + return -EIO;
>>>> +}
>>>> +
>>>> +static inline struct anx7688 *bridge_to_anx7688(struct drm_bridge
>>>> *bridge)
>>>> +{
>>>> + return container_of(bridge, struct anx7688, bridge);
>>>> +}
>>>> +
>>>> +static bool anx7688_bridge_mode_fixup(struct drm_bridge *bridge,
>>>> + const struct drm_display_mode
>>>> *mode,
>>>> + struct drm_display_mode
>>>> *adjusted_mode)
>>>> +{
>>>> + struct anx7688 *anx7688 = bridge_to_anx7688(bridge);
>>>> + u8 regs[2];
>>>> + int totalbw, requiredbw;
>>>
>>>
>>>
>>> It might make sense to use a u32 or long or something here to prevent
>>> risk of overflow.
>>
>>
>> Well, mode->clock is already an int (and there won't be any overflow
>> in requiredbw unless we go for THz clocks ,-))
>
>
> Ah okay, missed that.
>
>>
>> totalbw could do with a bit more of sanity checking (e.g. check that
>> regs[0] * regs[1] is not absurd). Like, we know regs[1] <= 2 on this
>> chip. Will fix.
>>
>>>> + int ret;
>>>> +
>>>> + if (!anx7688->filter)
>>>> + return true;
>>>> +
>>>> + /* Read both regs 0x85 (bandwidth) and 0x86 (lane count). */
>>>> + ret = anx7688_read(anx7688->client, DP_BANDWIDTH_REG, regs, 2);
>>>
>>>
>>>
>>> Who programmed these registers in the first place? Is the lane count
>>> some sort of reset value? Or is it something that changes dynamically?
>>
>>
>> The ANX7688 OCM (on-chip microcontroller) sets it. It changes
>> depending on the downstream (DP) bandwidth/lane count, so it changes
>> on each plug event (after DP link training presumably).
>
>
> Okay. Makes sense, then.
>
> Thanks,
> Archit
>
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project