Mixel MIPI DPHY support for NXPs i.MX8 SOCs
Subsystems (3)
v9 cover Mixel MIPI DPHY support for NXPs i.MX8 SOCs
This is basically a resend of v8 with one minor debug printk fixed and Rob's Reviewed-by for binding docs collteted. Thanks Rob!
This adds initial support for the Mixel IP based mipi dphy as found on i.MX8 processors. It has support for the i.MX8MQ, support for other variants can be added - once the platform specific parts are in - via the provided devdata. The driver is somewhat based on what's found in NXPs BSP.
Public documentation on the DPHY's registers is currently thin in the i.MX8 reference manuals (even on the i.MX8QXP form 11/18) so most of the values were taken from existing drivers. Newer NXP drivers have a bit more details so where possible the timings are calculated and validated.
This was tested with an initial version of a NWL MIPI DSI host controller driver
https://lists.freedesktop.org/archives/dri-devel/2019-March/209685.html
and a forward ported DCSS driver on linux-next 20190408.
Robert Chiras (the author of the corresponding driver in NXPs vendor tree) got this driver to work in his tree as well using mxsfb:
https://www.spinics.net/lists/arm-kernel/msg711950.html
Changes from v8
* Collect Reviewed-by from Rob Herring
* Fix {hs,clk}_prepare vs {hs,clk}_zero debug print out
Changes from v7
* As per review comments from Rob Herring
* Use fsl, as vendor prefix
* Drop changes to vendor-prefixes.txt due to that
* Shorten mixel_dphy to dphy in the example
* Fix an indentation error noticed by checkpatch that got introduced in v6
* Use lowercase letters in hex addresses in DT bindings example
Changes from v6
* Depend on HAS_IOMEM (fixes a build problem on UM spotted by kbuild)
Changes from v5
* Fix build problems on mips (spotted by the kbuild test robot) by using u32 consistently and long long for lp_t.
Changes from v4
* Build by default on ARCH_MXC && ARM64
Changes form v3
* Check correct variable after devm_ioremap_resource
* Add Robert Chiras as Co-authored-by since he's the author of the driver in NXPs BSP.
Changes from v2
* As per review comments from Fabio Estevam
* KConfig: select REGMAP_MMIO
* Drop phy_read
* Don't make phy_write inline
* Remove duplicate debugging output
* Comment style and typo fixes
* Add #defines's for PLL lock timing values
* Return correct error value when PLL fails to lock
* Check error when enabling clock
* Use devm_ioremap_resource
* As per review comments from Robert Chiras
* Deassert PD_DPHY after PLL lock (as per mixel ref manual)
* Assert PD_{DPHY,PLL} before power on (as per mixel ref manual)manual
* Add exit phy_op to reset CN/CM/CO
Changes from v1
* As per review comments from Fabio Estevam
* Kconfig: tristate mixel dphy support.
* Drop unused 'ret' in mixel_dphy_ref_power_off.
* Match values of DPHY_RXL{PRP,DRP} to those of
https://source.codeaurora.org/external/imx/linux-imx/log/?h=imx_4.14.78_1.0.0_ga
The previous values were based on 4.9.
* Use resource size on devm_ioremap, we have that in dt already.
* Use regmap so it's simple to dump the registers.
* Use regmap_read_poll_timeout instead of open coded loop.
* Add undocumented rxhs_settle register
* As per review comments from Sam Ravnborg
* Move driver to d/phy/freescale/
* Move SPDX-License-Identifier to top of file.
* Drop '/* #define DEBUG 1 */'.
* Use GPL-2.0+ since the vendor driver uses that as well.
* Drop the mutex, register access is now protected by regmap.
* Fix various style / indentation issues.
* Check for register read, write and ioremap errors
* Improve phy timing calculations
* Use LP clock rate where sensible, check for errors
* Use ad hoc forumulas for timings involving hs clock
* Switch from dphy_ops to devdata. Other i.MX8 variants differ in register layout too
* Add Mixel Inc to vendor-prefixes.txt
To: Kishon Vijay Abraham I <[email protected]>,Rob Herring <[email protected]>,Mark Rutland <[email protected]>,Shawn Guo <[email protected]>,Sascha Hauer <[email protected]>,Pengutronix Kernel Team <[email protected]>,Fabio Estevam <[email protected]>,NXP Linux Team <[email protected]>,Thierry Reding <[email protected]>,"Andreas Färber" <[email protected]>,Martin Blumenstingl <[email protected]>,Heiko Stuebner <[email protected]>,Johan Hovold <[email protected]>,Lucas Stach <[email protected]>,Abel Vesa <[email protected]>,Li Jun <[email protected]>,[email protected],[email protected],[email protected],[email protected],Robert Chiras <[email protected]>,Sam Ravnborg <[email protected]>,Maxime Ripard <maxi
[email protected]>
Guido Günther (2):
dt-bindings: phy: Add documentation for mixel dphy
phy: Add driver for mixel mipi dphy found on NXP's i.MX8 SoCs
.../bindings/phy/mixel,mipi-dsi-phy.txt | 29 + drivers/phy/freescale/Kconfig | 11 + drivers/phy/freescale/Makefile | 1 + .../phy/freescale/phy-fsl-imx8-mipi-dphy.c | 506 ++++++++++++++++++ 4 files changed, 547 insertions(+) create mode 100644 Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.txt create mode 100644 drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
Signature · 1 line
2.20.1
PATCH v9 1/2 dt-bindings: phy: Add documentation for mixel dphy
Add support for the MIXEL DPHY IP as found on NXP's i.MX8MQ SoCs.
--- .../bindings/phy/mixel,mipi-dsi-phy.txt | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.txt
Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.txt new file +29 -0
@@ -0,0 +1,29 @@ +Mixel DSI PHY for i.MX8 + +The Mixel MIPI-DSI PHY IP block is e.g. found on i.MX8 platforms (along the +MIPI-DSI IP from Northwest Logic). It represents the physical layer for the +electrical signals for DSI. + +Required properties: +- compatible: Must be: + - "fsl,imx8mq-mipi-dphy" +- clocks: Must contain an entry for each entry in clock-names. +- clock-names: Must contain the following entries: + - "phy_ref": phandle and specifier referring to the DPHY ref clock +- reg: the register range of the PHY controller +- #phy-cells: number of cells in PHY, as defined in + Documentation/devicetree/bindings/phy/phy-bindings.txt + this must be <0> + +Optional properties: +- power-domains: phandle to power domain + +Example: + dphy: dphy@30a0030 { + compatible = "fsl,imx8mq-mipi-dphy"; + clocks = <&clk IMX8MQ_CLK_DSI_PHY_REF>; + clock-names = "phy_ref"; + reg = <0x30a00300 0x100>; + power-domains = <&pd_mipi0>; + #phy-cells = <0>; + };
Signature · 1 line
2.20.1
PATCH v9 2/2 phy: Add driver for mixel mipi dphy found on NXP's i.MX8 SoCs
In reply to Guido GüntherThis adds support for the Mixel DPHY as found on i.MX8 CPUs but since this is an IP core it will likely be found on others in the future. So instead of adding this to the nwl host driver make it a generic PHY driver.
The driver supports the i.MX8MQ. Support for i.MX8QM and i.MX8QXP can be added once the necessary system controller bits are in via mixel_dphy_devdata.
--- drivers/phy/freescale/Kconfig | 11 + drivers/phy/freescale/Makefile | 1 + .../phy/freescale/phy-fsl-imx8-mipi-dphy.c | 506 ++++++++++++++++++ 3 files changed, 518 insertions(+) create mode 100644 drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c
drivers/phy/freescale/Kconfig +11 -0
@@ -3,3 +3,14 @@ config PHY_FSL_IMX8MQ_USB depends on OF && HAS_IOMEM select GENERIC_PHY default ARCH_MXC && ARM64 + +config PHY_MIXEL_MIPI_DPHY + tristate "Mixel MIPI DSI PHY support" + depends on OF && HAS_IOMEM + select GENERIC_PHY + select GENERIC_PHY_MIPI_DPHY + select REGMAP_MMIO + default ARCH_MXC && ARM64 + help + Enable this to add support for the Mixel DSI PHY as found + on NXP's i.MX8 family of SOCs.
drivers/phy/freescale/Makefile +1 -0
@@ -1 +1,2 @@ obj-$(CONFIG_PHY_FSL_IMX8MQ_USB) += phy-fsl-imx8mq-usb.o +obj-$(CONFIG_PHY_MIXEL_MIPI_DPHY) += phy-fsl-imx8-mipi-dphy.o
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c new file +506 -0
506 lines not on this page · raw message
Signature · 6 lines
2.20.1 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Fabio Estevam
reviewerRe: [PATCH v9 2/2] phy: Add driver for mixel mipi dphy found on NXP's i.MX8 SoCs
Hi Guido,
Quoted 6 lines · Guido Günther
This adds support for the Mixel DPHY as found on i.MX8 CPUs but since this is an IP core it will likely be found on others in the future. So instead of adding this to the nwl host driver make it a generic PHY driver.
Quoted 6 lines
The driver supports the i.MX8MQ. Support for i.MX8QM and i.MX8QXP can be added once the necessary system controller bits are in via
mixel_dphy_devdata.Co-authored-by Robert Chiras <[email protected]>Signed-off-by Guido Günther <[email protected]>
I wish I could test it on a imx8m-evk , but there are some other pieces needed such as Northwest Logic driver, mxsfb changes for supporting mx8m, OLED panel driver, etc
Anyway, it looks good to me and I have only a few minor comments:
Quoted 13 lines
--- drivers/phy/freescale/Kconfig | 11 + drivers/phy/freescale/Makefile | 1 + .../phy/freescale/phy-fsl-imx8-mipi-dphy.c | 506 ++++++++++++++++++ 3 files changed, 518 insertions(+) create mode 100644 drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.cdrivers/phy/freescale/Kconfig +0
-0@@ -3,3 +3,14 @@ config PHY_FSL_IMX8MQ_USB depends on OF && HAS_IOMEM
drivers/phy/freescale/Kconfig config PHY_FSL_IMX8MQ_USB · line 13
Quoted 10 lines
drivers/phy/freescale/Kconfig +8
-0select GENERIC_PHY default ARCH_MXC && ARM64 + +config PHY_MIXEL_MIPI_DPHY + tristate "Mixel MIPI DSI PHY support" + depends on OF && HAS_IOMEM + select GENERIC_PHY + select GENERIC_PHY_MIPI_DPHY + select REGMAP_MMIO + default ARCH_MXC && ARM64
I don't think that this default is a good idea.
There are imx8m systems that do not have display, so in this case it does not make sense to always force the build of this driver.
Quoted 22 lines
drivers/phy/freescale/Kconfig +3
-0+ help + Enable this to add support for the Mixel DSI PHY as found + on NXP's i.MX8 family of SOCs.drivers/phy/freescale/Makefile +1
-0@@ -1 +1,2 @@ obj-$(CONFIG_PHY_FSL_IMX8MQ_USB) += phy-fsl-imx8mq-usb.o +obj-$(CONFIG_PHY_MIXEL_MIPI_DPHY) += phy-fsl-imx8-mipi-dphy.odrivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c new file +6
-0@@ -0,0 +1,506 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* +* Copyright 2017,2018 NXP +* Copyright 2019 Purism SPC +*/ +
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c line 16
Quoted 10 lines
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c new file +10
-0+#include <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/delay.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_platform.h> +#include <linux/regmap.h> +#include <linux/phy/phy.h>
Please keep the headers sorted.
Quoted 5 lines
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c new file +3
-0+#include <linux/platform_device.h> +static int mixel_dphy_validate (struct phy *phy,enum phy_mode mode,int submode, +union phy_configure_opts *opts)
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c line 362
Quoted 10 lines
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c new file +10
-0+{ +struct mixel_dphy_cfg cfg = {0 }; + +if (mode !=PHY_MODE_MIPI_DPHY ) +return -EINVAL ; + +return mixel_dphy_config_from_opts (phy, &opts->mipi_dphy, &cfg); +} + +
A single blank line is enough.
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c line 371
Quoted 9 lines
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c new file +9
-0+static int mixel_dphy_init (struct phy *phy) +{ +phy_write (phy,PWR_OFF ,DPHY_PD_PLL ); +phy_write (phy,PWR_OFF ,DPHY_PD_DPHY ); + +return 0 ; +} + +
Ditto.
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c line 381
Quoted 10 lines
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c new file +10
-0+static int mixel_dphy_exit (struct phy *phy) +{ +phy_write (phy,0 ,DPHY_CM ); +phy_write (phy,0 ,DPHY_CN ); +phy_write (phy,0 ,DPHY_CO ); + +return 0 ; +} + +
Ditto.
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c line 420
Quoted 13 lines
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c new file +13
-0+static int mixel_dphy_power_off (struct phy *phy) +{ +struct mixel_dphy_priv *priv =phy_get_drvdata (phy); + +phy_write (phy,PWR_OFF ,DPHY_PD_PLL ); +phy_write (phy,PWR_OFF ,DPHY_PD_DPHY ); + +clk_disable_unprepare (priv->phy_ref_clk); + +return 0 ; +} + +
Ditto.
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c line 462
Quoted 4 lines
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c new file +4
-0+ res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); + regs =devm_ioremap_resource (dev, res); +if (IS_ERR (regs)) { +dev_err (dev,"Couldn't map the DPHY registers\n" );
You can skip this error message, because the core already complains on ioremap failures.
Re: [PATCH v9 2/2] phy: Add driver for mixel mipi dphy found on NXP's i.MX8 SoCs
Hi Fabio,
Quoted 48 lines · Fabio Estevam
Hi Guido,
Quoted 6 lines · Guido Günther
This adds support for the Mixel DPHY as found on i.MX8 CPUs but since this is an IP core it will likely be found on others in the future. So instead of adding this to the nwl host driver make it a generic PHY driver.
Quoted 6 lines
The driver supports the i.MX8MQ. Support for i.MX8QM and i.MX8QXP can be added once the necessary system controller bits are in via
mixel_dphy_devdata.Co-authored-by Robert Chiras <[email protected]>Signed-off-by Guido Günther <[email protected]>I wish I could test it on a imx8m-evk , but there are some other pieces needed such as Northwest Logic driver, mxsfb changes for supporting mx8m, OLED panel driver, etc
Anyway, it looks good to me and I have only a few minor comments:
Quoted 13 lines
--- drivers/phy/freescale/Kconfig | 11 + drivers/phy/freescale/Makefile | 1 + .../phy/freescale/phy-fsl-imx8-mipi-dphy.c | 506 ++++++++++++++++++ 3 files changed, 518 insertions(+) create mode 100644 drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.cdrivers/phy/freescale/Kconfig +0
-0@@ -3,3 +3,14 @@ config PHY_FSL_IMX8MQ_USB depends on OF && HAS_IOMEMdrivers/phy/freescale/Kconfig config PHY_FSL_IMX8MQ_USB · line 13
Quoted 10 lines
drivers/phy/freescale/Kconfig +8
-0select GENERIC_PHY default ARCH_MXC && ARM64 + +config PHY_MIXEL_MIPI_DPHY + tristate "Mixel MIPI DSI PHY support" + depends on OF && HAS_IOMEM + select GENERIC_PHY + select GENERIC_PHY_MIPI_DPHY + select REGMAP_MMIO + default ARCH_MXC && ARM64I don't think that this default is a good idea.
Quoted 2 lines
There are imx8m systems that do not have display, so in this case it does not make sense to always force the build of this driver.
O.k. - we can enable this based on imx-nwl later on. I've also addressed your other comments. Will wait for a v10 for a couple of days in case there's more feedback.
Thanks,
-- Guido
Re: [PATCH v9 2/2] phy: Add driver for mixel mipi dphy found on NXP's i.MX8 SoCs
In reply to Guido GüntherHi Guido.
Took a look at this, but feedback is trivial as
I have no experience with PHYs so use only
the feedback you consider relevant.
Sam
Quoted 82 lines
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c new file +76
-0@@ -0,0 +1,506 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* +* Copyright 2017,2018 NXP +* Copyright 2019 Purism SPC +*/ + + +/* DPHY registers */ +#define DPHY_PD_DPHY 0x00 +#define DPHY_M_PRG_HS_PREPARE 0x04 +#define DPHY_MC_PRG_HS_PREPARE 0x08 +#define DPHY_M_PRG_HS_ZERO 0x0c +#define DPHY_MC_PRG_HS_ZERO 0x10 +#define DPHY_M_PRG_HS_TRAIL 0x14 +#define DPHY_MC_PRG_HS_TRAIL 0x18 +#define DPHY_PD_PLL 0x1c +#define DPHY_TST 0x20 +#define DPHY_CN 0x24 +#define DPHY_CM 0x28 +#define DPHY_CO 0x2c +#define DPHY_LOCK 0x30 +#define DPHY_LOCK_BYP 0x34 +#define DPHY_REG_BYPASS_PLL 0x4C + +#define MBPS (x) ((x) * 1000000) + +#define DATA_RATE_MAX_SPEED MBPS(1500) +#define DATA_RATE_MIN_SPEED MBPS(80) + +#define PLL_LOCK_SLEEP 10 +#define PLL_LOCK_TIMEOUT 1000 + +#define CN_BUF 0xcb7a89c0 +#define CO_BUF 0x63 +#define CM (x) ( \ + ((x) < 32)?0xe0|((x)-16) : \ + ((x) < 64)?0xc0|((x)-32) : \ + ((x) < 128)?0x80|((x)-64) : \ + ((x) - 128)) +#define CN (x) (((x) == 1)?0x1f : (((CN_BUF)>>((x)-1))&0x1f)) +#define CO (x) ((CO_BUF)>>(8-(x))&0x3) + +/* PHY power on is active low */ +#define PWR_ON 0 +#define PWR_OFF 1 + +enum mixel_dphy_devtype { +MIXEL_IMX8MQ , +}; + +struct mixel_dphy_devdata { +u8 reg_tx_rcal; +u8 reg_auto_pd_en; +u8 reg_rxlprp; +u8 reg_rxcdrp; +u8 reg_rxhs_settle; +}; + +static const struct mixel_dphy_devdata mixel_dphy_devdata[] = { + [MIXEL_IMX8MQ ] = { + .reg_tx_rcal =0x38 , + .reg_auto_pd_en =0x3c , + .reg_rxlprp =0x40 , + .reg_rxcdrp =0x44 , + .reg_rxhs_settle =0x48 , + }, +}; + +struct mixel_dphy_cfg { +/* DPHY PLL parameters */ +u32 cm; +u32 cn; +u32 co; +/* DPHY register values */ +u8 mc_prg_hs_prepare; +u8 m_prg_hs_prepare;
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c line 86
Quoted 10 lines
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c new file +10
-0+u8 mc_prg_hs_zero; +u8 m_prg_hs_zero; +u8 mc_prg_hs_trail; +u8 m_prg_hs_trail; +u8 rxhs_settle; +}; + +struct mixel_dphy_priv { +struct mixel_dphy_cfg cfg; +struct regmap *regs;
It is a little confusing that the regmap is named regs. As regs in many other cases is used as name for the variable with pointer to registers.
See for example call to devm_regmap_init_mmio()
Quoted 21 lines
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c new file +21
-0+struct clk *phy_ref_clk; +const struct mixel_dphy_devdata *devdata; +}; + +static const struct regmap_config mixel_dphy_regmap_config = { + .reg_bits =8 , + .val_bits =32 , + .reg_stride =4 , + .max_register =DPHY_REG_BYPASS_PLL , + .name ="mipi-dphy" , +}; + +static int phy_write (struct phy *phy,u32 value,unsigned int reg) +{ +struct mixel_dphy_priv *priv =phy_get_drvdata (phy); +int ret; + + ret =regmap_write (priv->regs, reg, value); +if (ret <0 ) +dev_err (&phy->dev,"Failed to write DPHY reg %d: %d" , reg, ret); +return ret;
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c line 128
Quoted 10 lines
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c new file +10
-0+} + +/* +* Find a ratio close to the desired one using continued fraction +* approximation ending either at exact match or maximum allowed +* nominator, denominator. +*/ +static void get_best_ratio (u32 *pnum,u32 *pdenom,unsigned int max_n, +unsigned int max_d) +{
Maybe use u32 for all parameters?
That would also match usage below.
Quoted 62 lines
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c new file +62
-0+u32 a = *pnum; +u32 b = *pdenom; +u32 c; +u32 n[] = {0 ,1 }; +u32 d[] = {1 ,0 }; +u32 whole; +unsigned int i =1 ; + +while (b) { + i ^=1 ; + whole = a / b; + n[i] += (n[i ^1 ] * whole); + d[i] += (d[i ^1 ] * whole); +if ((n[i] > max_n) || (d[i] > max_d)) { + i ^=1 ; +break ; + } + c = a - (b * whole); + a = b; + b = c; + } + *pnum = n[i]; + *pdenom = d[i]; +} + +static int mixel_dphy_config_from_opts (struct phy *phy, +struct phy_configure_opts_mipi_dphy *dphy_opts, +struct mixel_dphy_cfg *cfg) +{ +struct mixel_dphy_priv *priv =dev_get_drvdata (phy->dev.parent); +unsigned long ref_clk =clk_get_rate (priv->phy_ref_clk); +u32 lp_t, numerator, denominator; +unsigned long long tmp; +u32 n; +int i; + +if (dphy_opts->hs_clk_rate >DATA_RATE_MAX_SPEED || + dphy_opts->hs_clk_rate <DATA_RATE_MIN_SPEED ) +return -EINVAL ; + + numerator = dphy_opts->hs_clk_rate; + denominator = ref_clk; +get_best_ratio (&numerator, &denominator,255 ,256 ); +if (!numerator || !denominator) { +dev_err (&phy->dev,"Invalid %d/%d for %ld/%ld\n" , + numerator, denominator, + dphy_opts->hs_clk_rate, ref_clk); +return -EINVAL ; + } + +while ((numerator <16 ) && (denominator <=128 )) { + numerator <<=1 ; + denominator <<=1 ; + } +/* +* CM ranges between 16 and 255 +* CN ranges between 1 and 32 +* CO is power of 2: 1, 2, 4, 8 +*/ + i =__ffs (denominator); +if (i >3 ) + i =3 ;
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c line 200
Quoted 10 lines
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c new file +10
-0+ cfg->cn = denominator >> i; + cfg->co =1 << i; + cfg->cm = numerator; + +if (cfg->cm <16 || cfg->cm >255 || + cfg->cn <1 || cfg->cn >32 || + cfg->co <1 || cfg->co >8 ) { +dev_err (&phy->dev,"Invalid CM/CN/CO values: %u/%u/%u\n" , + cfg->cm, cfg->cn, cfg->co); + dev_err(&phy->dev, "for hs_clk/ref_clk=%ld/%ld ⩰ %d/%d\n",
Hmm, my mutt does not like this symbol? ⩰
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c line 400
Quoted 10 lines
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c new file +10
-0+ +phy_write (phy,PWR_ON ,DPHY_PD_PLL ); + ret =regmap_read_poll_timeout (priv->regs,DPHY_LOCK , locked, + locked,PLL_LOCK_SLEEP , +PLL_LOCK_TIMEOUT ); +if (ret <0 ) { +dev_err (&phy->dev,"Could not get DPHY lock (%d)!\n" , ret); + goto clock_disable; + } +phy_write (phy,PWR_ON ,DPHY_PD_DPHY );
Are then no other timing constrains than the poll of DPHY_LOCK? Some panel (I know this is not a panel, but amyway) have some ridgid timing constrains around power ON/OFF.
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c line 506
Quoted 5 lines
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c new file +4
-0+ +MODULE_AUTHOR ("NXP Semiconductor" ); +MODULE_DESCRIPTION ("Mixel MIPI-DSI PHY driver" ); +MODULE_LICENSE ("GPL v2" );
Does this match the SPDX tag?
"
Quoted 1 line
+// SPDX-License-Identifier: GPL-2.0+
"
Re: [PATCH v9 2/2] phy: Add driver for mixel mipi dphy found on NXP's i.MX8 SoCs
Hi,
Quoted 5 lines · Sam Ravnborg
Hi Guido.
Took a look at this, but feedback is trivial as
I have no experience with PHYs so use only
the feedback you consider relevant.
They all made sense so I've incorporated them for v10.
Quoted 91 lines
Sam
Quoted 88 lines
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c new file +82
-0@@ -0,0 +1,506 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* +* Copyright 2017,2018 NXP +* Copyright 2019 Purism SPC +*/ + + +/* DPHY registers */ +#define DPHY_PD_DPHY 0x00 +#define DPHY_M_PRG_HS_PREPARE 0x04 +#define DPHY_MC_PRG_HS_PREPARE 0x08 +#define DPHY_M_PRG_HS_ZERO 0x0c +#define DPHY_MC_PRG_HS_ZERO 0x10 +#define DPHY_M_PRG_HS_TRAIL 0x14 +#define DPHY_MC_PRG_HS_TRAIL 0x18 +#define DPHY_PD_PLL 0x1c +#define DPHY_TST 0x20 +#define DPHY_CN 0x24 +#define DPHY_CM 0x28 +#define DPHY_CO 0x2c +#define DPHY_LOCK 0x30 +#define DPHY_LOCK_BYP 0x34 +#define DPHY_REG_BYPASS_PLL 0x4C + +#define MBPS (x) ((x) * 1000000) + +#define DATA_RATE_MAX_SPEED MBPS(1500) +#define DATA_RATE_MIN_SPEED MBPS(80) + +#define PLL_LOCK_SLEEP 10 +#define PLL_LOCK_TIMEOUT 1000 + +#define CN_BUF 0xcb7a89c0 +#define CO_BUF 0x63 +#define CM (x) ( \ + ((x) < 32)?0xe0|((x)-16) : \ + ((x) < 64)?0xc0|((x)-32) : \ + ((x) < 128)?0x80|((x)-64) : \ + ((x) - 128)) +#define CN (x) (((x) == 1)?0x1f : (((CN_BUF)>>((x)-1))&0x1f)) +#define CO (x) ((CO_BUF)>>(8-(x))&0x3) + +/* PHY power on is active low */ +#define PWR_ON 0 +#define PWR_OFF 1 + +enum mixel_dphy_devtype { +MIXEL_IMX8MQ , +}; + +struct mixel_dphy_devdata { +u8 reg_tx_rcal; +u8 reg_auto_pd_en; +u8 reg_rxlprp; +u8 reg_rxcdrp; +u8 reg_rxhs_settle; +}; + +static const struct mixel_dphy_devdata mixel_dphy_devdata[] = { + [MIXEL_IMX8MQ ] = { + .reg_tx_rcal =0x38 , + .reg_auto_pd_en =0x3c , + .reg_rxlprp =0x40 , + .reg_rxcdrp =0x44 , + .reg_rxhs_settle =0x48 , + }, +}; + +struct mixel_dphy_cfg { +/* DPHY PLL parameters */ +u32 cm; +u32 cn; +u32 co; +/* DPHY register values */ +u8 mc_prg_hs_prepare; +u8 m_prg_hs_prepare; +u8 mc_prg_hs_zero; +u8 m_prg_hs_zero; +u8 mc_prg_hs_trail; +u8 m_prg_hs_trail; +u8 rxhs_settle; +};
Quoted 8 lines
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c line 97
Quoted 4 lines
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c new file +4
-0+ +struct mixel_dphy_priv { +struct mixel_dphy_cfg cfg; +struct regmap *regs;It is a little confusing that the regmap is named regs. As regs in many other cases is used as name for the variable with pointer to registers.
See for example call todevm_regmap_init_mmio()
Changed to regmap and base.
Quoted 26 lines
Quoted 26 lines
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c new file +25
-0+struct clk *phy_ref_clk; +const struct mixel_dphy_devdata *devdata; +}; + +static const struct regmap_config mixel_dphy_regmap_config = { + .reg_bits =8 , + .val_bits =32 , + .reg_stride =4 , + .max_register =DPHY_REG_BYPASS_PLL , + .name ="mipi-dphy" , +}; + +static int phy_write (struct phy *phy,u32 value,unsigned int reg) +{ +struct mixel_dphy_priv *priv =phy_get_drvdata (phy); +int ret; + + ret =regmap_write (priv->regs, reg, value); +if (ret <0 ) +dev_err (&phy->dev,"Failed to write DPHY reg %d: %d" , reg, ret); +return ret; +} + +/* +* Find a ratio close to the desired one using continued fraction
Quoted 8 lines
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c line 128
Quoted 6 lines
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c new file +6
-0+* approximation ending either at exact match or maximum allowed +* nominator, denominator. +*/ +static void get_best_ratio (u32 *pnum,u32 *pdenom,unsigned int max_n, +unsigned int max_d) +{Maybe use u32 for all parameters?
That would also match usage below.
Less confusing, done.
Quoted 66 lines
Quoted 66 lines
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c new file +65
-0+u32 a = *pnum; +u32 b = *pdenom; +u32 c; +u32 n[] = {0 ,1 }; +u32 d[] = {1 ,0 }; +u32 whole; +unsigned int i =1 ; + +while (b) { + i ^=1 ; + whole = a / b; + n[i] += (n[i ^1 ] * whole); + d[i] += (d[i ^1 ] * whole); +if ((n[i] > max_n) || (d[i] > max_d)) { + i ^=1 ; +break ; + } + c = a - (b * whole); + a = b; + b = c; + } + *pnum = n[i]; + *pdenom = d[i]; +} + +static int mixel_dphy_config_from_opts (struct phy *phy, +struct phy_configure_opts_mipi_dphy *dphy_opts, +struct mixel_dphy_cfg *cfg) +{ +struct mixel_dphy_priv *priv =dev_get_drvdata (phy->dev.parent); +unsigned long ref_clk =clk_get_rate (priv->phy_ref_clk); +u32 lp_t, numerator, denominator; +unsigned long long tmp; +u32 n; +int i; + +if (dphy_opts->hs_clk_rate >DATA_RATE_MAX_SPEED || + dphy_opts->hs_clk_rate <DATA_RATE_MIN_SPEED ) +return -EINVAL ; + + numerator = dphy_opts->hs_clk_rate; + denominator = ref_clk; +get_best_ratio (&numerator, &denominator,255 ,256 ); +if (!numerator || !denominator) { +dev_err (&phy->dev,"Invalid %d/%d for %ld/%ld\n" , + numerator, denominator, + dphy_opts->hs_clk_rate, ref_clk); +return -EINVAL ; + } + +while ((numerator <16 ) && (denominator <=128 )) { + numerator <<=1 ; + denominator <<=1 ; + } +/* +* CM ranges between 16 and 255 +* CN ranges between 1 and 32 +* CO is power of 2: 1, 2, 4, 8 +*/ + i =__ffs (denominator); +if (i >3 ) + i =3 ; + cfg->cn = denominator >> i; + cfg->co =1 << i; + cfg->cm = numerator;
Quoted 8 lines
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c line 200
Quoted 7 lines
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c new file +7
-0+ +if (cfg->cm <16 || cfg->cm >255 || + cfg->cn <1 || cfg->cn >32 || + cfg->co <1 || cfg->co >8 ) { +dev_err (&phy->dev,"Invalid CM/CN/CO values: %u/%u/%u\n" , + cfg->cm, cfg->cn, cfg->co); + dev_err(&phy->dev, "for hs_clk/ref_clk=%ld/%ld ⩰ %d/%d\n",Hmm, my mutt does not like this symbol? ⩰
Changed to the ascii '~'.
Quoted 15 lines
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c line 400
Quoted 11 lines
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c new file +10
-0+ +phy_write (phy,PWR_ON ,DPHY_PD_PLL ); + ret =regmap_read_poll_timeout (priv->regs,DPHY_LOCK , locked, + locked,PLL_LOCK_SLEEP , +PLL_LOCK_TIMEOUT ); +if (ret <0 ) { +dev_err (&phy->dev,"Could not get DPHY lock (%d)!\n" , ret); + goto clock_disable; + } +phy_write (phy,PWR_ON ,DPHY_PD_DPHY );Are then no other timing constrains than the poll of
DPHY_LOCK? Some panel (I know this is not a panel, but amyway) have some ridgid timing constrains around power ON/OFF.
NXPs BSP doesn't have any additional timing constraints and that's the best we have in that area. Even the new imx8mq reference manual published last week isn't any more precise regarding DPHY power up.
Quoted 7 lines
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c line 506
Quoted 6 lines
drivers/phy/freescale/phy-fsl-imx8-mipi-dphy.c new file +4
-0+ +MODULE_AUTHOR ("NXP Semiconductor" ); +MODULE_DESCRIPTION ("Mixel MIPI-DSI PHY driver" ); +MODULE_LICENSE ("GPL v2" );Does this match the SPDX tag?
I've changed this to MODULE_LICENSE("GPL") since this seems to be used by driver that are v2+.
Quoted 4 lines
"
Quoted 1 line
+// SPDX-License-Identifier: GPL-2.0+
"
Thanks!
-- Guido
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v9 2/2] phy: Add driver for mixel mipi dphy found on NXP's i.MX8 SoCs
In reply to Guido GüntherAm 30.04.19 um 16:40 schrieb Guido Günther:
Quoted 10 lines
This adds support for the Mixel DPHY as found on i.MX8 CPUs but since this is an IP core it will likely be found on others in the future. So instead of adding this to the nwl host driver make it a generic PHY driver.
The driver supports the i.MX8MQ. Support for i.MX8QM and i.MX8QXP can be added once the necessary system controller bits are in via
mixel_dphy_devdata.Co-authored-by Robert Chiras <[email protected]>
This should be Co-developed-by and is lacking a Signed-off-by from that author. Robert, can you please provide one?
Quoted 1 line
Signed-off-by Guido Günther <[email protected]>
Thanks,
Andreas
Signature · 3 lines
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 21284 (AG Nürnberg)
Re: [EXT] Re: [PATCH v9 2/2] phy: Add driver for mixel mipi dphy found on NXP's i.MX8 SoCs
Quoted 15 lines · Andreas Färber
Am 30.04.19 um 16:40 schrieb Guido Günther:
Quoted 13 lines
This adds support for the Mixel DPHY as found on i.MX8 CPUs but since
this is an IP core it will likely be found on others in the future. So
instead of adding this to the nwl host driver make it a generic PHY driver.The driver supports the i.MX8MQ. Support for i.MX8QM and i.MX8QXP can be
added once the necessary system controller bits are in viamixel_dphy_devdata.
Quoted 4 lines
Quoted 1 line
Co-authored-by Robert Chiras <[email protected]>This should be Co-developed-by and is lacking a Signed-off-by from that
author. Robert, can you please provide one?
Sure. Added below.
Quoted 11 lines
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww
.kernel.org%2Fdoc%2Fhtml%2Flatest%2Fprocess%2Fsubmitting- patches.html%23when-to-use-acked-by-cc-and-co-developed- by&data=02%7C01%7Crobert.chiras%40nxp.com%7C012f6577aab440f62e160 8d6cf13f2a0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636924081917 754411&sdata=%2FrFNrAaL7MwPXuaYOlxfa8O4JLNILq5eFEq92qV2JZg%3D& ;reserved=0Quoted 2 lines
Signed-off-by Guido Günther <[email protected]>
Quoted 7 lines
Thanks,
Andreas--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 21284 (AG Nürnberg)