Re: [PATCH v4 08/12] i3c: dw-i3c-master: Add a quirk to skip clock and reset

[Philipp Zabel <p.zabel@pengutronix.de>]

On Di, 2026-06-16 at 09:54 +0000, Akhil R wrote:
> Some ACPI-enumerated devices like Tegra410 do not have clock and reset
> resources exposed via the clk/reset frameworks. Unlike device tree,
> ACPI on Arm does not model such provider functions. The hardware is
> expected to be brought out of reset and have its clocks enabled by the
> firmware before the OS takes over. Any data to be shared with the OS is
> passed using the _DSD property.
> 
> Add match data for such devices to skip acquiring clock and reset controls
> during probe and read the clock rate from the "clock-frequency" _DSD
> property. Note that the "clock-frequency" here is the controller's core
> clock and not the bus speed. I3C specifies the bus speed separately using
> "i3c-scl-hz" and "i2c-scl-hz" and hence this should not cause any conflict.
> 
> Also, move match data parsing before clock/reset acquisition so the quirk
> is available early enough.
> 
> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
> ---
>  drivers/i3c/master/dw-i3c-master.c | 66 ++++++++++++++++++++----------
>  1 file changed, 44 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index 3e510fddf06c..a2a4b88c2017 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
> @@ -242,6 +242,7 @@
>  /* List of quirks */
>  #define AMD_I3C_OD_PP_TIMING		BIT(1)
>  #define DW_I3C_DISABLE_RUNTIME_PM_QUIRK	BIT(2)
> +#define DW_I3C_ACPI_SKIP_CLK_RST		BIT(3)
>  
>  struct dw_i3c_cmd {
>  	u32 cmd_lo;
> @@ -556,13 +557,28 @@ static void dw_i3c_master_set_intr_regs(struct dw_i3c_master *master)
>  	writel(IBI_REQ_REJECT_ALL, master->regs + IBI_MR_REQ_REJECT);
>  }
>  
> +static unsigned long dw_i3c_master_get_core_rate(struct dw_i3c_master *master)
> +{
> +	unsigned int core_rate_prop;
> +
> +	if (!(master->quirks & DW_I3C_ACPI_SKIP_CLK_RST))
> +		return clk_get_rate(master->core_clk);
> +
> +	if (device_property_read_u32(master->dev, "clock-frequency", &core_rate_prop)) {
> +		dev_err(master->dev, "missing clock-frequency property\n");
> +		return 0;
> +	}
> +
> +	return core_rate_prop;
> +}
> +
>  static int dw_i3c_clk_cfg(struct dw_i3c_master *master)
>  {
>  	unsigned long core_rate, core_period;
>  	u32 scl_timing;
>  	u8 hcnt, lcnt;
>  
> -	core_rate = clk_get_rate(master->core_clk);
> +	core_rate = dw_i3c_master_get_core_rate(master);
>  	if (!core_rate)
>  		return -EINVAL;
>  
> @@ -615,7 +631,7 @@ static int dw_i2c_clk_cfg(struct dw_i3c_master *master)
>  	u16 hcnt, lcnt;
>  	u32 scl_timing;
>  
> -	core_rate = clk_get_rate(master->core_clk);
> +	core_rate = dw_i3c_master_get_core_rate(master);
>  	if (!core_rate)
>  		return -EINVAL;
>  
> @@ -1577,18 +1593,33 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
>  	if (IS_ERR(master->regs))
>  		return PTR_ERR(master->regs);
>  
> -	master->core_clk = devm_clk_get_enabled(&pdev->dev, NULL);
> -	if (IS_ERR(master->core_clk))
> -		return PTR_ERR(master->core_clk);
> -
> -	master->pclk = devm_clk_get_optional_enabled(&pdev->dev, "pclk");
> -	if (IS_ERR(master->pclk))
> -		return PTR_ERR(master->pclk);
> +	if (has_acpi_companion(&pdev->dev)) {
> +		quirks = (unsigned long)device_get_match_data(&pdev->dev);
> +	} else if (pdev->dev.of_node) {
> +		drvdata = device_get_match_data(&pdev->dev);
> +		if (drvdata)
> +			quirks = drvdata->flags;
> +	}
> +	master->quirks = quirks;
>  
> -	master->core_rst = devm_reset_control_get_optional_exclusive_deasserted(&pdev->dev,
> -										"core_rst");
> -	if (IS_ERR(master->core_rst))
> -		return PTR_ERR(master->core_rst);
> +	if (master->quirks & DW_I3C_ACPI_SKIP_CLK_RST) {
> +		master->core_clk = NULL;
> +		master->pclk = NULL;
> +		master->core_rst = NULL;
> +	} else {
> +		master->core_clk = devm_clk_get_enabled(&pdev->dev, NULL);
> +		if (IS_ERR(master->core_clk))
> +			return PTR_ERR(master->core_clk);
> +
> +		master->pclk = devm_clk_get_optional_enabled(&pdev->dev, "pclk");
> +		if (IS_ERR(master->pclk))
> +			return PTR_ERR(master->pclk);
> +
> +		master->core_rst = devm_reset_control_get_optional_exclusive_deasserted(&pdev->dev,
> +											"core_rst");
> +		if (IS_ERR(master->core_rst))
> +			return PTR_ERR(master->core_rst);

Why does this need to move around at all?

The reset control is already optional, so I would expect this to set
core_rst to NULL on missing reset, no quirk necessary.

Same question for pclk. Wouldn't it be enough to make core_clk optional
as well?

regards
Philipp