| 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: <20251223-grumpy-daft-loon-6a6186@quoll>
Date: Tue, 23 Dec 2025 15:32:06 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Tzu-Hao Wei <twei@...ado.com>
Cc: SriNavmani A <srinavmani@...ado.com>,
Prasad Bolisetty <pbolisetty@...ado.com>, Vinod Koul <vkoul@...nel.org>,
Neil Armstrong <neil.armstrong@...aro.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Harshit Shah <hshah@...ado.com>, Ulf Hansson <ulf.hansson@...aro.org>,
Adrian Hunter <adrian.hunter@...el.com>, Michal Simek <michal.simek@....com>,
linux-phy@...ts.infradead.org, devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, linux-mmc@...r.kernel.org
Subject: Re: [PATCH 2/8] phy: axiado: add Arasan eMMC-PHY for Axiado
On Mon, Dec 22, 2025 at 04:45:01PM +0800, Tzu-Hao Wei wrote:
> @@ -15,6 +15,7 @@ obj-$(CONFIG_PHY_AIROHA_PCIE) += phy-airoha-pcie.o
> obj-$(CONFIG_PHY_NXP_PTN3222) += phy-nxp-ptn3222.o
Where is maintainers file update in this patch? Why shall we take
unmaintained code?
> obj-y += allwinner/ \
> amlogic/ \
> + axiado/ \
> broadcom/ \
> cadence/ \
> freescale/ \
> diff --git a/drivers/phy/axiado/Kconfig b/drivers/phy/axiado/Kconfig
> new file mode 100644
> index 0000000000000000000000000000000000000000..824114e6068da327308321b9884552ad33db9efc
> --- /dev/null
> +++ b/drivers/phy/axiado/Kconfig
> @@ -0,0 +1,15 @@
> +#
> +# PHY drivers for Axiado platforms
> +#
> +
Missing menuconfig or other if-block for groupping this with your ARCH
and COMPILE_TEST dependency.
Look how other NEW and MAINTAINED platforms did it.
> +config PHY_AX3000_EMMC
> + tristate "Axiado eMMC PHY driver"
> + select GENERIC_PHY
> + help
> + This enables support for the eMMC PHY block found on the
> + Axiado AX3000 SoCs. The PHY provides the physical layer
> + interface used by the Arasan SDHCI host controller for emmc
> + signaling and timing adjustment.
> +
> + If you are building a kernel for AX3000 platform with
> + eMMC storage, say Y or N.
...
> +static void arasan_emmc_phy_write(struct axiado_emmc_phy *ax_phy, u32 offset, u32 data)
> +{
> + writel(data, ax_phy->reg_base + offset);
> +}
> +
> +static int arasan_emmc_phy_read(struct axiado_emmc_phy *ax_phy, u32 offset)
Useless wrappers. Just use readl/writel directly. You are not making
code more readable.
> +{
> + u32 val = readl(ax_phy->reg_base + offset);
> +
> + return val;
> +}
> +
> +static int axiado_emmc_phy_init(struct phy *phy)
> +{
> + u32 val;
> + ktime_t timeout;
> +
> + struct axiado_emmc_phy *ax_phy = phy_get_drvdata(phy);
> +
> + val = arasan_emmc_phy_read(ax_phy, PHY_CTRL_1);
> + arasan_emmc_phy_write(ax_phy, PHY_CTRL_1, val | RETB_ENBL | RTRIM_EN);
> +
> + val = arasan_emmc_phy_read(ax_phy, PHY_CTRL_3);
> + arasan_emmc_phy_write(ax_phy, PHY_CTRL_3, val | PDB_ENBL);
> +
> + /* Wait max 3000 ms */
> + timeout = ktime_add_ms(ktime_get(), LOOP_TIMEOUT);
> +
> + while (1) {
> + bool timedout = ktime_after(ktime_get(), timeout);
> +
> + if (arasan_emmc_phy_read(ax_phy, STATUS) & CALDONE_MASK)
> + break;
> +
> + if (timedout) {
> + dev_err(&phy->dev, "CALDONE_MASK bit is not cleared.");
> + return -ETIMEDOUT;
> + }
> + udelay(TIMEOUT_DELAY);
> + }
> +
> + val = arasan_emmc_phy_read(ax_phy, PHY_CTRL_1);
> +
> + arasan_emmc_phy_write(ax_phy, PHY_CTRL_1, val | REN_CMD_EN | PU_CMD_EN);
> +
> + val = arasan_emmc_phy_read(ax_phy, PHY_CTRL_2);
> + arasan_emmc_phy_write(ax_phy, PHY_CTRL_2, val | REN_STRB);
> +
> + val = arasan_emmc_phy_read(ax_phy, PHY_CTRL_3);
> + arasan_emmc_phy_write(ax_phy, PHY_CTRL_3, val | MAX_CLK_BUF0 |
> + MAX_CLK_BUF1 | MAX_CLK_BUF2);
> +
> + val = arasan_emmc_phy_read(ax_phy, CAP_REG_IN_S1_MSB);
> + arasan_emmc_phy_write(ax_phy, CAP_REG_IN_S1_MSB, CLK_MULTIPLIER);
> +
> + val = arasan_emmc_phy_read(ax_phy, PHY_CTRL_3);
> + arasan_emmc_phy_write(ax_phy, PHY_CTRL_3, val | SEL_DLY_RXCLK |
> + SEL_DLY_TXCLK);
> +
> + return 0;
> +}
> +
> +static int axiado_emmc_phy_power_on(struct phy *phy)
> +{
> + struct axiado_emmc_phy *ax_phy = phy_get_drvdata(phy);
> +
> + u32 val;
> + ktime_t timeout;
> +
> + val = arasan_emmc_phy_read(ax_phy, PHY_CTRL_1);
> + arasan_emmc_phy_write(ax_phy, PHY_CTRL_1, val | RETB_ENBL);
> +
> + val = arasan_emmc_phy_read(ax_phy, PHY_CTRL_3);
> + arasan_emmc_phy_write(ax_phy, PHY_CTRL_3, val | PDB_ENBL);
> +
> + val = arasan_emmc_phy_read(ax_phy, PHY_CTRL_2);
> + arasan_emmc_phy_write(ax_phy, PHY_CTRL_2, val | OTAP_SEL(OTAP_DLY));
> +
> + arasan_emmc_phy_read(ax_phy, PHY_CTRL_2);
> +
> + val = arasan_emmc_phy_read(ax_phy, PHY_CTRL_1);
> + arasan_emmc_phy_write(ax_phy, PHY_CTRL_1, val | DLL_TRM(DLL_TRM_ICP));
> +
> + arasan_emmc_phy_write(ax_phy, STATUS, 0x00);
> +
> + val = arasan_emmc_phy_read(ax_phy, PHY_CTRL_3);
> + arasan_emmc_phy_write(ax_phy, PHY_CTRL_3, val | DLL_FRQSEL(FRQ_SEL));
> +
> + /* Wait max 3000 ms */
> + timeout = ktime_add_ms(ktime_get(), LOOP_TIMEOUT);
> +
> + while (1) {
You proper read_poll loop.
> + bool timedout = ktime_after(ktime_get(), timeout);
> +
> + if (arasan_emmc_phy_read(ax_phy, STATUS) & DLL_RDY_MASK)
> + break;
> +
> + if (timedout) {
> + dev_err(&phy->dev, "DLL_RDY_MASK bit is not cleared.");
> + return -ETIMEDOUT;
> + }
> + udelay(TIMEOUT_DELAY);
...
> +static int axiado_emmc_phy_probe(struct platform_device *pdev)
> +{
> + struct axiado_emmc_phy *ax_phy;
> + struct phy_provider *phy_provider;
> + struct device *dev = &pdev->dev;
> + const struct of_device_id *id;
> + struct phy *generic_phy;
> + struct resource *res;
> +
> + if (!dev->of_node)
> + return -ENODEV;
> +
> + ax_phy = devm_kzalloc(dev, sizeof(*ax_phy), GFP_KERNEL);
> + if (!ax_phy)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
Use proper wrapper to combine get resource and ioremap.
> + ax_phy->reg_base = devm_ioremap_resource(&pdev->dev, res);
> +
Drop blank line, there's never such.
> + if (IS_ERR(ax_phy->reg_base))
> + return PTR_ERR(ax_phy->reg_base);
> +
> + id = of_match_node(axiado_emmc_phy_of_match, pdev->dev.of_node);
> + if (!id) {
> + dev_err(dev, "failed to get match_node\n");
What is the point of this? You do not use this match at all, no other
devices. How can your device bind and still fail the match?
Drop
> + return -EINVAL;
> + }
> +
> + generic_phy = devm_phy_create(dev, dev->of_node, &axiado_emmc_phy_ops);
> + if (IS_ERR(generic_phy)) {
> + dev_err(dev, "failed to create PHY\n");
> + return PTR_ERR(generic_phy);
Syntax is - return dev_err_probe.
Didn't Axiado receive this feedback before? Are you sure that you have
procedures set inside to avoid repeating same mistakes?
Best regards,
Krzysztof
Powered by blists - more mailing lists