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

[Krzysztof Kozlowski <krzk@kernel.org>]

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