Received: by 2002:a05:6a10:f3d0:0:0:0:0 with SMTP id a16csp24148pxv; Thu, 24 Jun 2021 01:46:58 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwNc6LawPS0XwKz9s6nt7qYdbskafgSPoZLNyi71IIdwsYkaIFL3H9R51cUb1wFZKhMCnp3 X-Received: by 2002:a17:907:8319:: with SMTP id mq25mr4224486ejc.279.1624524418068; Thu, 24 Jun 2021 01:46:58 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1624524418; cv=none; d=google.com; s=arc-20160816; b=xVsOmRPUche4WZ1531AUDfjSrqzfngYV0epQtzu2eBQ/ovW2cCoYJUdsIeRh5SrvgE X62uF1ZFf17l1NG/cdMJB/YSokaILnOYiSySTG0JQwbdTBlkryebJVzN+fIqvCEHwCqz sfxUVRkOBmDMCQe3bz+YRrGoCjzst7CdMU4FwUsOItshKwXph8bguY3GMJ63P65cS8Yn o8kxlkkDp4E0wcPcZsJtq4k11i0H/hHyPBKNEwytssjGAjbAtBD9gd4yrbAxSeeVTNzw Efk58U9orqrgjXIVs+Du/mqvT8VLns+rU5Do5tHQdJ27RYRJnH4+4TcXrBJlhsPDGMBY o5ZQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:content-language :in-reply-to:mime-version:user-agent:date:message-id:from:references :cc:to:subject; bh=/H5wTbSkDb5EG/Plv5JL5DZ/eP+KuDuMv9Q+6+2CdC4=; b=UusZa2i4sc3TJY2FWHexfeX6AfH5gbsQKeUAqMDzwGFH2yZmcm+zmISUoUuPB+bKeS FKBB1wUDTODqy777R7CsMMhZF4kgMV2gtGab8jRRQkj/vokcEvzNHxyLQsu6jiecXJ0s oADC51Wn2YZOZ9p5Dl14Fza3ioTUe7YhHKCo6aweRYetS6GJZCkJI5mBK9B6jZYGbix/ z3Z7tgwS7KUc4IMOVjHdd8P92O59CcYTMXETn41GInbZXQdtrATsU1AAHJHns5xdx81W eeaAB7Cawm61QdKtSoajO1ic99Fh2DlDI9rWgkWtqKls2uAxHHfm1qmk/XLZUjmeEsA+ kUsA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=canonical.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id c16si2392578ejj.702.2021.06.24.01.46.35; Thu, 24 Jun 2021 01:46:58 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=canonical.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229379AbhFXIrg (ORCPT + 99 others); Thu, 24 Jun 2021 04:47:36 -0400 Received: from youngberry.canonical.com ([91.189.89.112]:57977 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229630AbhFXIrf (ORCPT ); Thu, 24 Jun 2021 04:47:35 -0400 Received: from mail-ed1-f70.google.com ([209.85.208.70]) by youngberry.canonical.com with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.93) (envelope-from ) id 1lwKz5-0006dd-Al for linux-kernel@vger.kernel.org; Thu, 24 Jun 2021 08:45:15 +0000 Received: by mail-ed1-f70.google.com with SMTP id y18-20020a0564022712b029038ffac1995eso2981314edd.12 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=gDrQzcUxAIX+UnJ59jfqAiidsCrt1hfG9xjT+MD70bBRXtlQIQagQRUpPAPa226dYH CHvJv3K/VDzfF9KGjYSYeXaqlOP4qrU2Zilpaofh+2Iyz7SXELOs5up6YznVEnt3in0A Pld6/DuZyEL5cC4YgGKR9rYzeLdbzzGsN0anRwSgWRPurl8BsOYmXKYhonLCQ6w5ssgF BfGKcSjFtfSIT6VYKw6XSj93lieVa4CAX1tyhuGXMoewAcpeZz6dbPMSex8Gzlpymo/Q 3slGFsQEm93Z69HQvhLA7uvMj95Po1vp6u8NwBB5uuOr7+OmYot3Gzyb8VYVzd9I6EGE DMIg== X-Gm-Message-State: AOAM530a4m3Q1JmhmvzPcmMsFtTchmlZAsQxcSQBe6CvqR/I+c0F/knh 14B+9RGJvoCoA9nol/taON1vvnn+p6V4jjY43pCJJy5IrUu5FC5ZWpBe50bSS9zNGUYlAmQP+SB 2BwSJOfHp4cZOLROb/luVIKAMoQ5kowUrhLGYU3Gw3g== X-Received: by 2002:a05:6402:397:: with SMTP id o23mr2732324edv.217.1624524315080; Thu, 24 Jun 2021 01:45:15 -0700 (PDT) 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: linux-kernel@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