From patchwork Fri Jan 20 15:12:36 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Neil Armstrong X-Patchwork-Id: 9528725 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 294B560113 for ; Fri, 20 Jan 2017 15:12:44 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 1995C2869E for ; Fri, 20 Jan 2017 15:12:44 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 0D995286A0; Fri, 20 Jan 2017 15:12:44 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.1 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 4371E2869E for ; Fri, 20 Jan 2017 15:12:43 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 3A6C96EBC3; Fri, 20 Jan 2017 15:12:42 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-wm0-x22b.google.com (mail-wm0-x22b.google.com [IPv6:2a00:1450:400c:c09::22b]) by gabe.freedesktop.org (Postfix) with ESMTPS id 169426EBC3 for ; Fri, 20 Jan 2017 15:12:41 +0000 (UTC) Received: by mail-wm0-x22b.google.com with SMTP id c206so48264911wme.0 for ; Fri, 20 Jan 2017 07:12:41 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=baylibre-com.20150623.gappssmtp.com; s=20150623; h=subject:to:references:cc:from:organization:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=SV2MWFig0uQX6rxMOcc2+k0Ag33O/imhRJJ1B9XhI5E=; b=R9zr3shiXKq3XbOA86UsJivH+bCAnzC/jL+cN2aNax5A7YBYLLzZvqWIgrdGcHUFSK 7LnSQemILLa6rFc3cZNj/lbbFhY6n4sTHROmLcFXfhP3kAdC2MxByvYnHSrL6ZwkG84D dATfiht+ui1rVi4xmQQEHMuMYOnGnQCRer/yMUZrY53hSFywVjW65k+wgHuVQfDLebZC 61dWvn0lYYe3GWqmvT0sDsY1Z0znTyfV6AVZoYdwDqj5k1gOdsgj1q3fXG4sXtyflmlo MJtxrOLdEODMRcR5QGQ5NWohurqC/5zcxKH0QR4uLDYAQBwnxQDc12+OBsUDLYGt2ZZ2 +uuQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:organization :message-id:date:user-agent:mime-version:in-reply-to :content-transfer-encoding; bh=SV2MWFig0uQX6rxMOcc2+k0Ag33O/imhRJJ1B9XhI5E=; b=OO509cnPIsYvc+f6dTbuUCyTdT9Ii/E0M+zkVNQP2Tu1sCw44qZq30/Fn29qcacXn4 Q13eU+sEiLryI5njkLFN0xY35r9NVrE5sXOhNDyTI7lkmRXD3wCdcGQMWp+nl5ucWVJj eQ77ZDDZwVoqOFxqI6zbQViuvjlhM+5MaX0Ree/HvGjMTHrwNj3IyVVkdVAGX9k1miB/ z+f2ohxUbEpMbGyzT+J3C2HwjJTg6yoWRYdMqHfxBpyNK70obbMvddwSr4flslNqkwYb zxMSubvKQrECg56Fx/OZ449BkuRX1f7YUi32VikLjzyK7V4VmjAsF4hsm3QLrD3xnDaN IoJg== X-Gm-Message-State: AIkVDXKUvwOX+PTAkd/X1R2Zs1Yx+x+ynMHdq39IheY5Tl+VnI2qLOGNJ66OtCY25PqDAMNx X-Received: by 10.28.228.87 with SMTP id b84mr3721855wmh.0.1484925159452; Fri, 20 Jan 2017 07:12:39 -0800 (PST) Received: from [192.168.1.21] ([90.63.244.31]) by smtp.gmail.com with ESMTPSA id y97sm6581433wmh.24.2017.01.20.07.12.38 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 20 Jan 2017 07:12:38 -0800 (PST) Subject: Re: [RFC/RFT PATCH 1/4] drm/bridge: dw-hdmi: Switch to regmap for register access To: Laurent Pinchart References: <1484656294-6140-1-git-send-email-narmstrong@baylibre.com> <1484656294-6140-2-git-send-email-narmstrong@baylibre.com> <5238949.NqUKITq39o@avalon> From: Neil Armstrong Organization: Baylibre Message-ID: <3d3593cc-ce8d-c8da-6f41-ae7cba688a35@baylibre.com> Date: Fri, 20 Jan 2017 16:12:36 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: <5238949.NqUKITq39o@avalon> Cc: Jose.Abreu@synopsys.com, laurent.pinchart+renesas@ideasonboard.com, kieran.bingham@ideasonboard.com, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Mark Brown , linux-amlogic@lists.infradead.org X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Virus-Scanned: ClamAV using ClamSMTP On 01/17/2017 03:39 PM, Laurent Pinchart wrote: > Hi Neil, > > Thank you for the patch. > > On Tuesday 17 Jan 2017 13:31:31 Neil Armstrong wrote: >> The Synopsys Designware HDMI TX Controller does not enforce register access >> on platforms instanciating it. >> The current driver supports two different types of memory-mapped flat >> register access, but in order to support the Amlogic Meson SoCs integration, >> and provide a more generic way to handle all sorts of register mapping, >> switch the register access to use the regmap infrastructure. >> >> In the case of the registers are not flat memory-mapped or does not conform > > s/does/do/ > >> at the actual driver implementation, a regmap struct can be given in the > > s/at the actual/to the current/ ? > >> plat_data and be used at probe or bind. >> >> Since the AHB audio driver only uses direct memory access, using regmap only >> allows the I2S audio driver to be registered. > > This sounds a bit unclear to me, how about "[...], only allow the I2C audio > driver to be registered if the device is directly memory-mapped." ? > >> Signed-off-by: Neil Armstrong >> --- >> drivers/gpu/drm/bridge/dw-hdmi.c | 105 +++++++++++++++++++----------------- >> include/drm/bridge/dw_hdmi.h | 1 + >> 2 files changed, 57 insertions(+), 49 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c >> b/drivers/gpu/drm/bridge/dw-hdmi.c index ca9d0ce..13747fe 100644 >> --- a/drivers/gpu/drm/bridge/dw-hdmi.c >> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c >> @@ -20,6 +20,7 @@ >> #include >> #include >> #include >> +#include > > Could you please keep the headers alphabetically sorted ? > >> #include >> #include >> @@ -167,8 +168,7 @@ struct dw_hdmi { >> unsigned int audio_n; >> bool audio_enable; >> >> - void (*write)(struct dw_hdmi *hdmi, u8 val, int offset); >> - u8 (*read)(struct dw_hdmi *hdmi, int offset); >> + struct regmap *regm; >> }; >> >> #define HDMI_IH_PHY_STAT0_RX_SENSE \ >> @@ -179,42 +179,23 @@ struct dw_hdmi { >> (HDMI_PHY_RX_SENSE0 | HDMI_PHY_RX_SENSE1 | \ >> HDMI_PHY_RX_SENSE2 | HDMI_PHY_RX_SENSE3) >> >> -static void dw_hdmi_writel(struct dw_hdmi *hdmi, u8 val, int offset) >> -{ >> - writel(val, hdmi->regs + (offset << 2)); >> -} >> - >> -static u8 dw_hdmi_readl(struct dw_hdmi *hdmi, int offset) >> -{ >> - return readl(hdmi->regs + (offset << 2)); >> -} >> - >> -static void dw_hdmi_writeb(struct dw_hdmi *hdmi, u8 val, int offset) >> -{ >> - writeb(val, hdmi->regs + offset); >> -} >> - >> -static u8 dw_hdmi_readb(struct dw_hdmi *hdmi, int offset) >> -{ >> - return readb(hdmi->regs + offset); >> -} >> - >> static inline void hdmi_writeb(struct dw_hdmi *hdmi, u8 val, int offset) > > Not related to this patch, but the value, offset order of arguments to the > write function has been making me cringe since the very first time I read the > code. I wonder if modifying this would be accepted. > >> { >> - hdmi->write(hdmi, val, offset); >> + regmap_write(hdmi->regm, offset, val); >> } >> >> static inline u8 hdmi_readb(struct dw_hdmi *hdmi, int offset) >> { >> - return hdmi->read(hdmi, offset); >> + unsigned int val = 0; >> + >> + regmap_read(hdmi->regm, offset, &val); >> + >> + return val; >> } >> >> static void hdmi_modb(struct dw_hdmi *hdmi, u8 data, u8 mask, unsigned reg) >> { >> - u8 val = hdmi_readb(hdmi, reg) & ~mask; >> - >> - val |= data & mask; >> - hdmi_writeb(hdmi, val, reg); >> + regmap_update_bits(hdmi->regm, reg, mask, data); >> } >> >> static void hdmi_mask_writeb(struct dw_hdmi *hdmi, u8 data, unsigned int >> reg, @@ -1949,6 +1930,21 @@ static int dw_hdmi_detect_phy(struct dw_hdmi >> *hdmi) return 0; >> } >> >> +static struct regmap_config hdmi_regmap_8bit_config = { >> + .reg_bits = 32, >> + .val_bits = 8, >> + .reg_stride = 1, >> + .max_register = HDMI_I2CM_FS_SCL_LCNT_0_ADDR, >> +}; >> + >> +static struct regmap_config hdmi_regmap_32bit_config = { >> + .reg_bits = 8, >> + .pad_bits = 24, >> + .val_bits = 32, >> + .reg_stride = 4, >> + .max_register = HDMI_I2CM_FS_SCL_LCNT_0_ADDR, >> +}; > > I believe you can make these const. > >> static struct dw_hdmi * >> __dw_hdmi_probe(struct platform_device *pdev, >> const struct dw_hdmi_plat_data *plat_data) >> @@ -1958,7 +1954,8 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) >> struct platform_device_info pdevinfo; >> struct device_node *ddc_node; >> struct dw_hdmi *hdmi; >> - struct resource *iores; >> + struct regmap_config *reg_config = &hdmi_regmap_8bit_config; > > No need to assign a value at declaration time (unless the compiler is not > smart enough and complains, or is smarter than me and finds a problem where I > don't). > >> + struct resource *iores = NULL; >> int irq; >> int ret; >> u32 val = 1; >> @@ -1982,20 +1979,21 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) >> mutex_init(&hdmi->audio_mutex); >> spin_lock_init(&hdmi->audio_lock); >> >> - of_property_read_u32(np, "reg-io-width", &val); >> - >> - switch (val) { >> - case 4: >> - hdmi->write = dw_hdmi_writel; >> - hdmi->read = dw_hdmi_readl; >> - break; >> - case 1: >> - hdmi->write = dw_hdmi_writeb; >> - hdmi->read = dw_hdmi_readb; >> - break; >> - default: >> - dev_err(dev, "reg-io-width must be 1 or 4\n"); >> - return ERR_PTR(-EINVAL); >> + if (plat_data->regm) >> + hdmi->regm = plat_data->regm; > > You need curly braces around this statement. > >> + else { >> + of_property_read_u32(np, "reg-io-width", &val); >> + switch (val) { >> + case 4: >> + reg_config = &hdmi_regmap_32bit_config; >> + break; >> + case 1: >> + reg_config = &hdmi_regmap_8bit_config; >> + break; >> + default: >> + dev_err(dev, "reg-io-width must be 1 or 4\n"); >> + return ERR_PTR(-EINVAL); >> + } >> } >> >> ddc_node = of_parse_phandle(np, "ddc-i2c-bus", 0); >> @@ -2011,11 +2009,20 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) >> dev_dbg(hdmi->dev, "no ddc property found\n"); >> } >> >> - iores = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> - hdmi->regs = devm_ioremap_resource(dev, iores); >> - if (IS_ERR(hdmi->regs)) { >> - ret = PTR_ERR(hdmi->regs); >> - goto err_res; >> + if (!plat_data->regm) { >> + iores = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + hdmi->regs = devm_ioremap_resource(dev, iores); >> + if (IS_ERR(hdmi->regs)) { >> + ret = PTR_ERR(hdmi->regs); >> + goto err_res; >> + } >> + >> + hdmi->regm = devm_regmap_init_mmio(dev, hdmi->regs, > reg_config); >> + if (IS_ERR(hdmi->regm)) { >> + dev_err(dev, "Failed to configure regmap\n"); >> + ret = PTR_ERR(hdmi->regm); >> + goto err_res; >> + } >> } >> >> hdmi->isfr_clk = devm_clk_get(hdmi->dev, "isfr"); >> @@ -2123,7 +2130,7 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) >> config0 = hdmi_readb(hdmi, HDMI_CONFIG0_ID); >> config3 = hdmi_readb(hdmi, HDMI_CONFIG3_ID); >> >> - if (config3 & HDMI_CONFIG3_AHBAUDDMA) { >> + if (iores && config3 & HDMI_CONFIG3_AHBAUDDMA) { > > You test !plat->regm above, and iores here. How about standardizing that ? If > you test for !plat->regm here, you won't have to initialize iores to NULL. > > Apart from these small issues the patch looks good to me. > >> struct dw_hdmi_audio_data audio; >> >> audio.phys = iores->start; >> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h >> index 735a8ab..163842d 100644 >> --- a/include/drm/bridge/dw_hdmi.h >> +++ b/include/drm/bridge/dw_hdmi.h >> @@ -60,6 +60,7 @@ struct dw_hdmi_plat_data { >> unsigned long mpixelclock); >> enum drm_mode_status (*mode_valid)(struct drm_connector *connector, >> struct drm_display_mode *mode); >> + struct regmap *regm; >> }; >> >> int dw_hdmi_probe(struct platform_device *pdev, > Hi All, The actual 4bytes regmap config actually fails on rk3288 because regmap bypasses all modification of the reg address because of the regmap_mmio implementation. The only remaining way is to keep a reg_shift in in the hdmi context, see the patch below. With such change, on rk3288 : Tested-by: Neil Armstrong " [ 10.400294] dwhdmi-rockchip ff980000.hdmi: Detected HDMI TX controller v2.00a with HDCP (DWC MHL PHY) " Neil -><------------------ diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c index 8b35df5..563647f 100644 --- a/drivers/gpu/drm/bridge/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/dw-hdmi.c @@ -168,6 +168,7 @@ struct dw_hdmi { unsigned int audio_n; bool audio_enable; + unsigned int reg_shift; struct regmap *regm; }; @@ -181,21 +182,21 @@ struct dw_hdmi { static inline void hdmi_writeb(struct dw_hdmi *hdmi, u8 val, int offset) { - regmap_write(hdmi->regm, offset, val); + regmap_write(hdmi->regm, offset << hdmi->reg_shift, val); } static inline u8 hdmi_readb(struct dw_hdmi *hdmi, int offset) { unsigned int val = 0; - regmap_read(hdmi->regm, offset, &val); + regmap_read(hdmi->regm, offset << hdmi->reg_shift, &val); return val; } static void hdmi_modb(struct dw_hdmi *hdmi, u8 data, u8 mask, unsigned reg) { - regmap_update_bits(hdmi->regm, reg, mask, data); + regmap_update_bits(hdmi->regm, reg << hdmi->reg_shift, mask, data); } static void hdmi_mask_writeb(struct dw_hdmi *hdmi, u8 data, unsigned int reg, @@ -1984,11 +1985,10 @@ static const struct regmap_config hdmi_regmap_8bit_config = { }; static const struct regmap_config hdmi_regmap_32bit_config = { - .reg_bits = 8, - .pad_bits = 24, + .reg_bits = 32, .val_bits = 32, .reg_stride = 4, - .max_register = HDMI_I2CM_FS_SCL_LCNT_0_ADDR, + .max_register = HDMI_I2CM_FS_SCL_LCNT_0_ADDR << 2, }; static struct dw_hdmi * @@ -2044,6 +2044,7 @@ __dw_hdmi_probe(struct platform_device *pdev, switch (val) { case 4: reg_config = &hdmi_regmap_32bit_config; + hdmi->reg_shift = 2; break; case 1: reg_config = &hdmi_regmap_8bit_config;