| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <kvxt4hm3y5hsjiir2nnycr5kqgflswt2lra4b2akh5zju6ahqw@4w2xmkjgho22>
Date: Wed, 24 Dec 2025 19:06:50 +0800
From: Xu Yang <xu.yang_2@....com>
To: Vinod Koul <vkoul@...nel.org>
Cc: neil.armstrong@...aro.org, shawnguo@...nel.org, kernel@...gutronix.de,
festevam@...il.com, jun.li@....com, Frank.Li@....com, linux-phy@...ts.infradead.org,
imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] phy: fsl-imx8mq-usb: add debugfs to access control
register
On Tue, Dec 23, 2025 at 07:49:11PM +0530, Vinod Koul wrote:
> On 19-12-25, 15:54, Xu Yang wrote:
> > The CR port is a simple 16-bit data/address parallel port that is
> > provided for on-chip access to the control registers inside the
> > USB 3.0 femtoPHY. While access to these registers is not required
> > for normal PHY operation, this interface enables you to access
> > some of the PHY’s diagnostic features during normal operation or
> > to override some basic PHY control signals.
> >
> > 3 debugfs files are created to read and write control registers,
> > all use hexadecimal format:
> > ctrl_reg_base: the register offset to write, or the start offset
> > to read.
> > ctrl_reg_count: how many continuous registers to be read.
> > ctrl_reg_value: read to show the continuous registers value from
> > the offset in ctrl_reg_base, to ctrl_reg_base
> > + ctrl_reg_count - 1, one line for one register.
> > when write, override the register at ctrl_reg_base,
> > one time can only change one 16bits register.
> >
> > Signed-off-by: Li Jun <jun.li@....com>
> > Signed-off-by: Xu Yang <xu.yang_2@....com>
> > ---
> > drivers/phy/freescale/phy-fsl-imx8mq-usb.c | 221 ++++++++++++++++++++-
> > 1 file changed, 220 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> > index ad8a55012e42..cb2392008ad2 100644
> > --- a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> > +++ b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> > @@ -1,8 +1,9 @@
> > // SPDX-License-Identifier: GPL-2.0+
> > -/* Copyright (c) 2017 NXP. */
> > +/* Copyright (c) 2017-2025 NXP. */
> >
> > #include <linux/bitfield.h>
> > #include <linux/clk.h>
> > +#include <linux/debugfs.h>
> > #include <linux/delay.h>
> > #include <linux/io.h>
> > #include <linux/module.h>
> > @@ -52,6 +53,21 @@
> >
> > #define PHY_TUNE_DEFAULT 0xffffffff
> >
> > +/* PHY control register access */
> > +#define PHY_CTRL_REG_COUNT_MAX 0x42
> > +#define PHY_CTRL_REG_OFFSET_MAX 0x201f
> > +
> > +#define PHY_CRCTL 0x30
> > +#define PHY_CRCTL_DATA_IN_MASK GENMASK(15, 0)
> > +#define PHY_CRCTL_CAP_ADDR BIT(16)
> > +#define PHY_CRCTL_CAP_DATA BIT(17)
> > +#define PHY_CRCTL_CR_WRITE BIT(18)
> > +#define PHY_CRCTL_CR_READ BIT(19)
> > +
> > +#define PHY_CRSR 0x34
> > +#define PHY_CRSR_DATA_OUT_MASK GENMASK(15, 0)
> > +#define PHY_CRSR_CR_ACK BIT(16)
> > +
> > #define TCA_CLK_RST 0x00
> > #define TCA_CLK_RST_SW BIT(9)
> > #define TCA_CLK_RST_REF_CLK_EN BIT(1)
> > @@ -113,6 +129,9 @@ struct imx8mq_usb_phy {
> > void __iomem *base;
> > struct regulator *vbus;
> > struct tca_blk *tca;
> > + struct dentry *debugfs;
> > + u16 cr_access_base;
> > + u16 cr_read_count;
> > u32 pcs_tx_swing_full;
> > u32 pcs_tx_deemph_3p5db;
> > u32 tx_vref_tune;
> > @@ -420,6 +439,204 @@ static u32 phy_pcs_tx_swing_full_from_property(u32 percent)
> > percent = min(percent, 100U);
> >
> > return (percent * 127) / 100;
> > +};
> > +
> > +#define IMX8M_PHY_DEBUG_PORT_LOOP_TIMEOUT 500000
>
> empty line here, possibly move this to top of file
I'm going to remove it after I add a helper.
>
> > +static int imx8mq_phy_ctrl_reg_addr(struct imx8mq_usb_phy *imx_phy, u16 offset)
> > +{
> > + void __iomem *cr_ctrl = imx_phy->base + PHY_CRCTL;
> > + void __iomem *cr_sr = imx_phy->base + PHY_CRSR;
> > + struct device *dev = &imx_phy->phy->dev;
> > + int i;
> > +
> > + /* Address Phrase */
> > + writel(offset, cr_ctrl);
> > + writel(readl(cr_ctrl) | PHY_CRCTL_CAP_ADDR, cr_ctrl);
> > +
> > + /* Wait CRSR[16] == 1 */
> > + i = IMX8M_PHY_DEBUG_PORT_LOOP_TIMEOUT;
> > + while (!(readl(cr_sr) & PHY_CRSR_CR_ACK) && i > 0)
> > + i--;
> > + if (i == 0)
> > + goto cr_addr_err;
> > +
> > + writel(readl(cr_ctrl) & (~PHY_CRCTL_CAP_ADDR), cr_ctrl);
> > +
> > + /* Wait CRSR[16] == 0 */
> > + i = IMX8M_PHY_DEBUG_PORT_LOOP_TIMEOUT;
> > + while ((readl(cr_sr) & PHY_CRSR_CR_ACK) && i > 0)
> > + i--;
> > + if (i == 0)
> > + goto cr_addr_err;
> > +
> > + return 0;
> > +
> > +cr_addr_err:
> > + dev_err(dev, "Failed to address reg 0x%x.\n", offset);
> > + return -EIO;
> > +}
> > +
> > +static int imx8mq_phy_ctrl_reg_read(struct imx8mq_usb_phy *imx_phy, u16 offset)
> > +{
> > + void __iomem *cr_ctrl = imx_phy->base + PHY_CRCTL;
> > + void __iomem *cr_sr = imx_phy->base + PHY_CRSR;
> > + struct device *dev = &imx_phy->phy->dev;
> > + int val, i;
> > +
> > + /* Address Phrase */
> > + val = imx8mq_phy_ctrl_reg_addr(imx_phy, offset);
> > + if (val)
> > + return val;
> > +
> > + /* Read Phrase */
> > + writel(readl(cr_ctrl) | PHY_CRCTL_CR_READ, cr_ctrl);
> > +
> > + /* Wait CRSR[16] == 1 */
> > + i = IMX8M_PHY_DEBUG_PORT_LOOP_TIMEOUT;
> > + while (!(readl(cr_sr) & PHY_CRSR_CR_ACK) && i > 0)
> > + i--;
> > + if (i == 0)
> > + goto cr_read_err;
> > +
> > + val = readl(cr_sr);
> > + writel(val & ~PHY_CRCTL_CR_READ, cr_ctrl);
> > +
> > + /* Wait CRSR[16] == 0 */
> > + i = IMX8M_PHY_DEBUG_PORT_LOOP_TIMEOUT;
> > + while (!(readl(cr_sr) & PHY_CRSR_CR_ACK) && i > 0)
> > + i--;
> > + if (i == 0)
> > + goto cr_read_err;
> > +
> > + return val;
> > +
> > +cr_read_err:
> > + dev_err(dev, "Failed to read reg 0x%x.\n", offset);
> > + return -EIO;
> > +}
> > +
> > +static int imx8mq_phy_ctrl_reg_write(struct imx8mq_usb_phy *imx_phy,
> > + u16 offset, u16 val)
> > +{
> > + void __iomem *cr_ctrl = imx_phy->base + PHY_CRCTL;
> > + void __iomem *cr_sr = imx_phy->base + PHY_CRSR;
> > + struct device *dev = &imx_phy->phy->dev;
> > + int i, ret;
> > +
> > + /* Address Phrase */
> > + ret = imx8mq_phy_ctrl_reg_addr(imx_phy, offset);
> > + if (ret)
> > + return ret;
> > +
> > + writel(val, cr_ctrl);
> > +
> > + /* Set cr_cap_data to be 1'b1 */
> > + writel(readl(cr_ctrl) | PHY_CRCTL_CAP_DATA, cr_ctrl);
> > + /* Wait CRSR[16] == 1 */
> > + i = IMX8M_PHY_DEBUG_PORT_LOOP_TIMEOUT;
> > + while (!(readl(cr_sr) & PHY_CRSR_CR_ACK) && i > 0)
> > + i--;
> > + if (i == 0)
> > + goto cr_write_err;
>
> seems like a repeated pattern, maybe move to a helper. (if driver used
> regmap, it could have used helpers from regmap for this polling)
Yes, good suggestion! I'll add a helper for this.
>
> > +
> > + /* Clear cr_cap_data to be 1'br0 */
> > + writel(readl(cr_ctrl) & ~PHY_CRCTL_CAP_DATA, cr_ctrl);
> > + /* Wait CRSR[16] == 0 */
> > + i = IMX8M_PHY_DEBUG_PORT_LOOP_TIMEOUT;
> > + while ((readl(cr_sr) & PHY_CRSR_CR_ACK) && i > 0)
> > + i--;
> > + if (i == 0)
> > + goto cr_write_err;
> > +
> > + /* Set cr_write to be 1'b1 */
> > + writel(readl(cr_ctrl) | PHY_CRCTL_CR_WRITE, cr_ctrl);
> > + /* Wait CRSR[16] == 1 */
> > + i = IMX8M_PHY_DEBUG_PORT_LOOP_TIMEOUT;
> > + while (!(readl(cr_sr) & PHY_CRSR_CR_ACK) && i > 0)
> > + i--;
> > + if (i == 0)
> > + goto cr_write_err;
> > +
> > + /* Clear cr_write to be 1'br0 */
> > + writel(readl(cr_ctrl) & ~PHY_CRCTL_CR_WRITE, cr_ctrl);
> > + /* Wait CRSR[16] == 0 */
> > + i = IMX8M_PHY_DEBUG_PORT_LOOP_TIMEOUT;
> > + while ((readl(cr_sr) & PHY_CRSR_CR_ACK) && i > 0)
> > + i--;
> > + if (i == 0)
> > + goto cr_write_err;
> > +
> > + return 0;
> > +
> > +cr_write_err:
> > + dev_err(dev, "Failed to write reg 0x%x.\n", offset);
> > + return -EIO;
> > +}
> > +
> > +static int ctrl_reg_value_show(struct seq_file *s, void *unused)
> > +{
> > + struct imx8mq_usb_phy *imx_phy = s->private;
> > + u16 i, val, base = imx_phy->cr_access_base;
> > +
> > + for (i = 0; i < imx_phy->cr_read_count; i++) {
> > + val = imx8mq_phy_ctrl_reg_read(imx_phy, base + i);
> > + seq_printf(s, "Control Register 0x%x value is 0x%4x\n",
> > + base + i, val);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int ctrl_reg_value_open(struct inode *inode, struct file *file)
> > +{
> > + return single_open(file, ctrl_reg_value_show, inode->i_private);
> > +}
> > +
> > +static ssize_t ctrl_reg_value_write(struct file *file, const char __user *ubuf,
> > + size_t count, loff_t *ppos)
> > +{
> > + struct seq_file *s = file->private_data;
> > + struct imx8mq_usb_phy *imx_phy = s->private;
> > + u16 cr_value;
> > + int ret;
> > +
> > + ret = kstrtou16_from_user(ubuf, count, 16, &cr_value);
> > + if (ret)
> > + return ret;
> > +
> > + imx8mq_phy_ctrl_reg_write(imx_phy, imx_phy->cr_access_base, cr_value);
> > +
> > + return count;
> > +}
> > +
> > +static const struct file_operations ctrl_reg_value_fops = {
> > + .open = ctrl_reg_value_open,
> > + .write = ctrl_reg_value_write,
> > + .read = seq_read,
> > + .llseek = seq_lseek,
> > + .release = single_release,
> > +};
>
> please use DEFINE_DEBUGFS_ATTRIBUTE for this
This "ctrl_reg_value" file can output more than one register value. So I
can't use DEFINE_DEBUGFS_ATTRIBUTE because it can only support one register
one time. I'm going to use DEFINE_SHOW_STORE_ATTRIBUTE instead.
>
> > +
> > +static void debug_create_files(struct imx8mq_usb_phy *imx_phy)
> > +{
> > + struct device *dev = &imx_phy->phy->dev;
> > +
> > + imx_phy->debugfs = debugfs_create_dir(dev_name(dev), imx_phy->phy->debugfs);
> > +
> > + debugfs_create_x16("ctrl_reg_base", 0600, imx_phy->debugfs,
> > + &imx_phy->cr_access_base);
> > + debugfs_create_x16("ctrl_reg_count", 0600, imx_phy->debugfs,
> > + &imx_phy->cr_read_count);
> > + debugfs_create_file("ctrl_reg_value", 0600, imx_phy->debugfs,
> > + imx_phy, &ctrl_reg_value_fops);
> > +
> > + imx_phy->cr_access_base = 0;
> > + imx_phy->cr_read_count = 1;
> > +}
> > +
> > +static void debug_remove_files(struct imx8mq_usb_phy *imx_phy)
> > +{
> > + debugfs_remove_recursive(imx_phy->debugfs);
> > }
> >
> > static void imx8m_get_phy_tuning_data(struct imx8mq_usb_phy *imx_phy)
> > @@ -724,6 +941,7 @@ static int imx8mq_usb_phy_probe(struct platform_device *pdev)
> > "failed to get tca\n");
> >
> > imx8m_get_phy_tuning_data(imx_phy);
> > + debug_create_files(imx_phy);
>
> please use phy->debugfs as root node
Okay. Will do.
Thanks,
Xu Yang
Powered by blists - more mailing lists