Re: [PATCH v7 3/5] i3c: mipi-i3c-hci: add microchip sama7d65 SoC compatible with the required quirk

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Reusing AMD-specific hardcoded timing quirks for a Microchip SoC will result in incorrect I3C bus frequencies.
--

commit 8dab9b9edc49c0214800ec48372900ae0b5bbae4
Author: Manikandan Muralidharan <manikandan.m@microchip.com>

i3c: mipi-i3c-hci: add microchip sama7d65 SoC compatible with the required quirk

This commit adds support for the Microchip sama7d65 SoC I3C HCI master IP.
It introduces clock acquisition during probe and applies specific quirks
to the new compatible string.

> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
> index b781dbed21650..4cdf2abd42197 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
[ ... ]
> @@ -969,6 +970,7 @@ static int i3c_hci_init(struct i3c_hci *hci)
>  static int i3c_hci_probe(struct platform_device *pdev)
>  {
>  	const struct mipi_i3c_hci_platform_data *pdata = pdev->dev.platform_data;
> +	struct clk_bulk_data *clks;
>  	struct i3c_hci *hci;
>  	int irq, ret;
[ ... ]
> @@ -1001,6 +1003,11 @@ static int i3c_hci_probe(struct platform_device *pdev)
>  	if (!hci->quirks && platform_get_device_id(pdev))
>  		hci->quirks = platform_get_device_id(pdev)->driver_data;
>  
> +	ret = devm_clk_bulk_get_all_enabled(&pdev->dev, &clks);
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret,
> +				     "Failed to get clocks\n");
> +
>  	ret = i3c_hci_init(hci);
[ ... ]
> @@ -1031,6 +1038,9 @@ static void i3c_hci_remove(struct platform_device *pdev)
>  
>  static const __maybe_unused struct of_device_id i3c_hci_of_match[] = {
>  	{ .compatible = "mipi-i3c-hci", },
> +	{ .compatible = "microchip,sama7d65-i3c-hci",
> +	  .data = (void *)(ulong)(HCI_QUIRK_PIO_MODE | HCI_QUIRK_OD_PP_TIMING |
> +				  HCI_QUIRK_RESP_BUF_THLD) },

[Severity: High]
Will applying HCI_QUIRK_OD_PP_TIMING to this Microchip SoC result in
incorrect I3C bus frequencies?

This quirk typically invokes amd_set_od_pp_timing(), which writes hardcoded
timing divisors (0x00cf00cf for Open Drain and 0x00160016 for Push-Pull) to
the controller's vendor-specific registers. These fixed values were
calculated for a 9MHz I3C bus frequency assuming an AMD platform's specific
fixed input clock of roughly 400 MHz.

Since the Microchip SoC acquires its clocks dynamically via
devm_clk_bulk_get_all_enabled() as seen above, its input clock rate could
differ and vary based on the system's clock tree configuration.

Should these timing divisors be calculated dynamically against the actual
input clock rate obtained from the clock framework rather than blindly
reusing the fixed AMD divisors?

>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, i3c_hci_of_match);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525092405.1514213-1-manikandan.m@microchip.com?part=3