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

[Frank Li <Frank.li@nxp.com>]

On Mon, May 25, 2026 at 10:23:31AM +0000, sashiko-bot@kernel.org wrote:
> 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.

Look like make sense. Is it true?

Frank
>
> 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