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=-15.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=ham 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 9AC7CC49EA6 for ; Thu, 24 Jun 2021 08:45:18 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 76CF2613D8 for ; Thu, 24 Jun 2021 08:45:18 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229889AbhFXIrg (ORCPT ); Thu, 24 Jun 2021 04:47:36 -0400 Received: from youngberry.canonical.com ([91.189.89.112]:57982 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229379AbhFXIrf (ORCPT ); Thu, 24 Jun 2021 04:47:35 -0400 Received: from mail-ed1-f72.google.com ([209.85.208.72]) by youngberry.canonical.com with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.93) (envelope-from ) id 1lwKz5-0006dj-Ef for devicetree@vger.kernel.org; Thu, 24 Jun 2021 08:45:15 +0000 Received: by mail-ed1-f72.google.com with SMTP id r9-20020a05640251c9b0290394f9be24d9so2418456edd.19 for ; Thu, 24 Jun 2021 01:45:15 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=/H5wTbSkDb5EG/Plv5JL5DZ/eP+KuDuMv9Q+6+2CdC4=; b=NQsqXdfhNgCbZ6TIfok7F3QJqTblnZr7c6eDOrZ9DIXk74iT2UJK5pf5yPE0TxFjoK aUVQO0/Q5U+Zxq2p9SImU6ZvtHMfxuRrc/xahOxa9HAsOBQ7HtyiMKzfSRaEqylJ++kD 2LSZLi9VWKF8QaxH9WW3hESNEZIf9fVEf3tuKiKP8Z6licnCtbSFHjEq+QVN9UIlpSaS 0q7qdt0/aLjXH0H3kErIZkt7fGIZLwken8cAR+21M594zFVz59NATzgbQUCkueomhA0y FLuP2+CkaLKDAW7hV8VfLvv931hCs+l6cAjpHMLy4E26Gbe8rCZVxbWMP5tllEiZ3Td8 Dwbg== X-Gm-Message-State: AOAM533efEHPJveUQeDwtXJgbJgIN8Y1Lgn26EsyTFmxg6Hw/stFaziN Z+4MJgWdRewM/C4+LwG1jFutUqHqkC69egJ3n1buipCkgDcYcW9c+D9r2aTrMomuYr8jxH4TnMH KB+6c8NxChnnWayR8kuoVSZAFp83SDG+QFsdU9ds= X-Received: by 2002:a05:6402:397:: with SMTP id o23mr2732318edv.217.1624524315079; Thu, 24 Jun 2021 01:45:15 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxBszcTjCXG3oCAfyucufMKcECl+4nvSj4Djz126QLLoge/Hq3VB1rseLQvg0Nks1FoQysqPg== X-Received: by 2002:a05:6402:397:: with SMTP id o23mr2732302edv.217.1624524314897; Thu, 24 Jun 2021 01:45:14 -0700 (PDT) Received: from [192.168.1.115] (xdsl-188-155-177-222.adslplus.ch. [188.155.177.222]) by smtp.gmail.com with ESMTPSA id u12sm881642eje.40.2021.06.24.01.45.13 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 24 Jun 2021 01:45:14 -0700 (PDT) Subject: Re: [RFC PATCH 4/9] phy: samsung: Add SEC DSIM DPHY driver To: Jagan Teki , Peng Fan , Shawn Guo , Sascha Hauer , Tomasz Figa , Fancy Fang Cc: devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, dri-devel@lists.freedesktop.org, linux-phy@lists.infradead.org, linux-kernel@vger.kernel.org, NXP Linux Team , linux-amarula@amarulasolutions.com, Anthony Brandon , Francis Laniel , Matteo Lisi , Milco Pratesi , Kishon Vijay Abraham I , Vinod Koul References: <20210621072424.111733-1-jagan@amarulasolutions.com> <20210621072424.111733-5-jagan@amarulasolutions.com> From: Krzysztof Kozlowski Message-ID: Date: Thu, 24 Jun 2021 10:45:13 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 MIME-Version: 1.0 In-Reply-To: <20210621072424.111733-5-jagan@amarulasolutions.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org On 21/06/2021 09:24, Jagan Teki wrote: > Samsung SEC MIPI DSIM DPHY controller is part of registers > available in SEC MIPI DSIM bridge for NXP's i.MX8M Mini and > Nano Processors. > > Add phy driver for it. > > Cc: Kishon Vijay Abraham I > Cc: Vinod Koul > Signed-off-by: Jagan Teki > --- > drivers/phy/samsung/Kconfig | 9 + > drivers/phy/samsung/Makefile | 1 + > drivers/phy/samsung/phy-sec-dsim-dphy.c | 236 ++++++++++++++++++++++++ > 3 files changed, 246 insertions(+) > create mode 100644 drivers/phy/samsung/phy-sec-dsim-dphy.c You add a driver for a Samsung's component, so please Cc respective folks. They might help you with it or provide comments to avoid duplication of drivers/code: Around phy: Marek Szyprowski Jaehoon Chung Around MIPI/DRM: Inki Dae Seung-Woo Kim Andrzej Hajda and: linux-samsung-soc@vger.kernel.org > > diff --git a/drivers/phy/samsung/Kconfig b/drivers/phy/samsung/Kconfig > index e20d2fcc9fe7..e80d40d1278c 100644 > --- a/drivers/phy/samsung/Kconfig > +++ b/drivers/phy/samsung/Kconfig > @@ -103,3 +103,12 @@ config PHY_EXYNOS5250_SATA > Exynos5250 based SoCs.This SerDes/Phy supports SATA 1.5 Gb/s, > SATA 3.0 Gb/s, SATA 6.0 Gb/s speeds. It supports one SATA host > port to accept one SATA device. > + > +config PHY_SEC_DSIM_DPHY SEC is not a codename of a project/product/silicon. It is a company name, so basically you called this "PHY_SAMSUNG_DSIM_DPHY" which is too generic. > + tristate "Samsung SEC MIPI DSIM DPHY driver" What is Samsung SEC? Either Samsung or SEC, not both. > + depends on OF && HAS_IOMEM > + select GENERIC_PHY > + select REGMAP_MMIO > + help > + Enable this to add support for the SEC MIPI DSIM DPHY as found > + on NXP's i.MX8M Mini and Nano family of SOCs. > diff --git a/drivers/phy/samsung/Makefile b/drivers/phy/samsung/Makefile > index 3959100fe8a2..4d46c7ec0072 100644 > --- a/drivers/phy/samsung/Makefile > +++ b/drivers/phy/samsung/Makefile > @@ -11,3 +11,4 @@ phy-exynos-usb2-$(CONFIG_PHY_EXYNOS5250_USB2) += phy-exynos5250-usb2.o > phy-exynos-usb2-$(CONFIG_PHY_S5PV210_USB2) += phy-s5pv210-usb2.o > obj-$(CONFIG_PHY_EXYNOS5_USBDRD) += phy-exynos5-usbdrd.o > obj-$(CONFIG_PHY_EXYNOS5250_SATA) += phy-exynos5250-sata.o > +obj-$(CONFIG_PHY_SEC_DSIM_DPHY) += phy-sec-dsim-dphy.o > diff --git a/drivers/phy/samsung/phy-sec-dsim-dphy.c b/drivers/phy/samsung/phy-sec-dsim-dphy.c > new file mode 100644 > index 000000000000..31de4a774b5f > --- /dev/null > +++ b/drivers/phy/samsung/phy-sec-dsim-dphy.c > @@ -0,0 +1,236 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (C) 2018 NXP > + * Copyright (C) 2021 Amarula Solutions(India) > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define DSI_PHYCTRL_B1 0x00 > +#define DSI_PHYCTRL_B2 0x04 > +#define DSI_PHYCTRL_M1 0x08 > +#define DSI_PHYCTRL_M2 0x0c > +#define DSI_PHYTIMING 0x10 > +#define DSI_PHYTIMING1 0x14 > +#define DSI_PHYTIMING2 0x18 > + > +/* phytiming */ > +#define M_TLPXCTL_MASK GENMASK(15, 8) What is the "M_" prefix for? > +#define M_TLPXCTL(x) FIELD_PREP(M_TLPXCTL_MASK, (x)) > +#define M_THSEXITCTL_MASK GENMASK(7, 0) > +#define M_THSEXITCTL(x) FIELD_PREP(M_THSEXITCTL_MASK, (x)) > + > +/* phytiming1 */ > +#define M_TCLKPRPRCTL_MASK GENMASK(31, 24) > +#define M_TCLKPRPRCTL(x) FIELD_PREP(M_TCLKPRPRCTL_MASK, (x)) > +#define M_TCLKZEROCTL_MASK GENMASK(23, 16) > +#define M_TCLKZEROCTL(x) FIELD_PREP(M_TCLKZEROCTL_MASK, (x)) > +#define M_TCLKPOSTCTL_MASK GENMASK(15, 8) > +#define M_TCLKPOSTCTL(x) FIELD_PREP(M_TCLKPOSTCTL_MASK, (x)) > +#define M_TCLKTRAILCTL_MASK GENMASK(7, 0) > +#define M_TCLKTRAILCTL(x) FIELD_PREP(M_TCLKTRAILCTL_MASK, (x)) > + > +/* phytiming2 */ > +#define M_THSPRPRCTL_MASK GENMASK(23, 16) > +#define M_THSPRPRCTL(x) FIELD_PREP(M_THSPRPRCTL_MASK, (x)) > +#define M_THSZEROCTL_MASK GENMASK(15, 8) > +#define M_THSZEROCTL(x) FIELD_PREP(M_THSZEROCTL_MASK, (x)) > +#define M_THSTRAILCTL_MASK GENMASK(7, 0) > +#define M_THSTRAILCTL(x) FIELD_PREP(M_THSTRAILCTL_MASK, (x)) > + > +struct dsim_dphy_plat_data { Remove the m_ prefix. Please document all fields. > + unsigned int m_tlpxctl; > + unsigned int m_thsexitctl; > + unsigned int m_tclkprprctl; > + unsigned int m_tclkzeroctl; > + unsigned int m_tclkpostctl; > + unsigned int m_tclktrailctl; > + unsigned int m_thsprprctl; > + unsigned int m_thszeroctl; > + unsigned int m_thstrailctl; > +}; > + > +struct dsim_dphy { > + struct regmap *regmap; > + struct clk *phy_ref_clk; > + const struct dsim_dphy_plat_data *pdata; > +}; > + > +static const struct regmap_config dsim_dphy_regmap_config = { > + .reg_bits = 8, > + .val_bits = 32, > + .reg_stride = 4, > + .max_register = DSI_PHYTIMING2, > + .name = "mipi-dphy", > +}; > + > +static int dsim_dphy_init(struct phy *phy) > +{ > + struct dsim_dphy *dphy = phy_get_drvdata(phy); > + const struct dsim_dphy_plat_data *pdata = dphy->pdata; > + u32 reg; > + > + /* phytiming */ > + regmap_read(dphy->regmap, DSI_PHYTIMING, ®); > + > + reg &= ~M_TLPXCTL_MASK; > + reg |= M_TLPXCTL(pdata->m_tlpxctl); > + reg &= ~M_THSEXITCTL_MASK; > + reg |= M_THSEXITCTL(pdata->m_thsexitctl); > + regmap_write(dphy->regmap, DSI_PHYTIMING, reg); > + > + /* phytiming1 */ > + regmap_read(dphy->regmap, DSI_PHYTIMING1, ®); > + > + reg &= ~M_TCLKPRPRCTL_MASK; > + reg |= M_TCLKPRPRCTL(pdata->m_tclkprprctl); > + reg &= ~M_TCLKZEROCTL_MASK; > + reg |= M_TCLKZEROCTL(pdata->m_tclkzeroctl); > + reg &= ~M_TCLKPOSTCTL_MASK; > + reg |= M_TCLKPOSTCTL(pdata->m_tclkpostctl); > + reg &= ~M_TCLKTRAILCTL_MASK; > + reg |= M_TCLKTRAILCTL(pdata->m_tclktrailctl); > + regmap_write(dphy->regmap, DSI_PHYTIMING1, reg); > + > + /* phytiming2 */ > + regmap_read(dphy->regmap, DSI_PHYTIMING2, ®); > + > + reg &= ~M_THSPRPRCTL_MASK; > + reg |= M_THSPRPRCTL(pdata->m_thsprprctl); > + reg &= ~M_THSZEROCTL_MASK; > + reg |= M_THSZEROCTL(pdata->m_thszeroctl); > + reg &= ~M_THSTRAILCTL_MASK; > + reg |= M_THSTRAILCTL(pdata->m_thstrailctl); > + regmap_write(dphy->regmap, DSI_PHYTIMING2, reg); > + > + return 0; > +} > + > +static int dsim_dphy_exit(struct phy *phy) > +{ > + return 0; > +} > + > +static int dsim_dphy_power_on(struct phy *phy) > +{ > + struct dsim_dphy *dphy = phy_get_drvdata(phy); > + int ret; > + > + ret = clk_prepare_enable(dphy->phy_ref_clk); > + if (ret < 0) > + return ret; > + > + return ret; > +} > + > +static int dsim_dphy_power_off(struct phy *phy) > +{ > + struct dsim_dphy *dphy = phy_get_drvdata(phy); > + > + clk_disable_unprepare(dphy->phy_ref_clk); > + > + return 0; > +} > + > +static const struct phy_ops dsim_dphy_phy_ops = { > + .init = dsim_dphy_init, > + .exit = dsim_dphy_exit, > + .power_on = dsim_dphy_power_on, > + .power_off = dsim_dphy_power_off, > + .owner = THIS_MODULE, > +}; > + > +static const struct dsim_dphy_plat_data imx8mm_dphy_plat_data = { > + /* phytiming */ > + .m_tlpxctl = 0x06, > + .m_thsexitctl = 0x0b, > + /* phytiming1 */ > + .m_tclkprprctl = 0x07, > + .m_tclkzeroctl = 0x26, > + .m_tclkpostctl = 0x0d, > + .m_tclktrailctl = 0x08, > + /* phytimings2 */ > + .m_thsprprctl = 0x08, > + .m_thszeroctl = 0x0d, > + .m_thstrailctl = 0x0b, > +}; > + > +static const struct of_device_id dsim_dphy_of_match[] = { > + { > + .compatible = "fsl,imx8mm-sec-dsim-dphy", > + .data = &imx8mm_dphy_plat_data, > + }, > + { /* sentinel */ }, > +}; > +MODULE_DEVICE_TABLE(of, dsim_dphy_of_match); > + > +static int dsim_dphy_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + struct phy_provider *phy_provider; > + struct dsim_dphy *dphy; > + struct phy *phy; > + void __iomem *base; > + > + if (!np) > + return -ENODEV; How can this driver bind without 'np'? How is it possible? > + > + dphy = devm_kzalloc(dev, sizeof(*dphy), GFP_KERNEL); > + if (!dphy) > + return -ENOMEM; > + > + dphy->pdata = of_device_get_match_data(&pdev->dev); > + if (!dphy->pdata) > + return -EINVAL; > + > + base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + dphy->regmap = devm_regmap_init_mmio(&pdev->dev, base, > + &dsim_dphy_regmap_config); > + if (IS_ERR(dphy->regmap)) { > + dev_err(dev, "failed create the DPHY regmap\n"); > + return PTR_ERR(dphy->regmap); > + } > + > + dphy->phy_ref_clk = devm_clk_get(&pdev->dev, "phy_ref"); > + if (IS_ERR(dphy->phy_ref_clk)) { > + dev_err(dev, "failed to get phy_ref clock\n"); > + return PTR_ERR(dphy->phy_ref_clk); > + } > + > + dev_set_drvdata(dev, dphy); > + > + phy = devm_phy_create(dev, np, &dsim_dphy_phy_ops); > + if (IS_ERR(phy)) { > + dev_err(dev, "failed to create phy %ld\n", PTR_ERR(phy)); > + return PTR_ERR(phy); > + } > + phy_set_drvdata(phy, dphy); > + > + phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate); > + > + return PTR_ERR_OR_ZERO(phy_provider); > +} > + > +static struct platform_driver dsim_dphy_driver = { > + .probe = dsim_dphy_probe, > + .driver = { > + .name = "sec-dsim-dphy", "sec" it's too generic, what if next time we have another one from Samsung? sec2? Please choose a name matching the actual component. > + .of_match_table = dsim_dphy_of_match, > + } > +}; > +module_platform_driver(dsim_dphy_driver); > + > +MODULE_AUTHOR("Jagan Teki "); > +MODULE_DESCRIPTION("Samsung SEC MIPI DSIM DPHY driver");> +MODULE_LICENSE("GPL"); > Best regards, Krzysztof