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

[<Manikandan.M@microchip.com>]

Hi Frank,

On 02/06/26 10:27 pm, Frank Li wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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?
> 

During the I3C sensor slave integration, a similar OD and PP timing 
configuration was required, so the values had to be chosen with previous 
implementation as reference at the time. Going forward, a dynamic timing 
configuration approach would be the most reliable and robust solution.

I am currently working on integrating dynamic timing register updates 
based on the core clock and I3C/I2C bus clock frequencies with CLK 
quirks, and will share the patch series shortly.

Thank you for your cooperation.

> 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

-- 
Thanks and Regards,
Manikandan M.