Re: [PATCH 2/8] phy: axiado: add Arasan eMMC-PHY for Axiado

Krzysztof Kozlowski
2025-12-23T14:32:09Z

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