Re: [PATCH 2/4] i3c: master: add Microchip SAMA7D65 I3C HCI master driver

[<Durai.ManickamKR@microchip.com>]

Hi Jarkko, Conor, Frank & Wolfram,

 From the comments I received from you all, I got clarity like i should 
integrate the I3C changes specific to our SoC in the existing 
mipi-i3c-hci driver by using quirk or some board specific kernel config. 
I will resend the patch to mailing list with this approach in short for 
the review. Thanks for the support.

On 10/09/25 14:31, Jarkko Nikula wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
> the content is safe
>
> Hi
>
> On 9/10/25 9:12 AM, Durai.ManickamKR@microchip.com wrote:
>> On 10/09/25 02:48, Frank Li wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you 
>>> know the content is safe
>>>
>>> On Tue, Sep 09, 2025 at 04:43:31PM +0530, Durai Manickam KR wrote:
>>>> Add support for microchip SAMA7D65 I3C HCI master only IP. This
>>>> hardware is an instance of the MIPI I3C HCI Controller implementing
>>>> version 1.0 specification. This driver adds platform-specific
>>>> support for SAMA7D65 SoC.
>>
>> Hi Frank,
>>
>> We have integrated the I3C HCI IP from synopsys. But the IP which we
>> integrated into our SAMA7D65 SoC does not support DMA feature, Endianess
>> check and few other interrupt status. So we have to introduce a
>> microchip SoC specific macro to modify this driver. So we have below 2
>> approaches,
>>
> I'd try to avoid creating a copy driver for the same base IP as much as
> possible.
>
> MIPI I3C HCI driver supports both PIO and DMA. Not sure are there
> implementations where IP is synthezied to support both but for example
> AMD platform with ACPI ID AMDI5017 is PIO only and Intel is DMA only.
>
> I have two concrete examples below showing how difficult is to keep sync
> changes and one sidenote about unneeded code for your HW.
>
>>>> +static void mchp_hci_dat_v1_set_dynamic_addr(struct mchp_i3c_hci 
>>>> *hci,
>>>> +                                        unsigned int dat_idx, u8 
>>>> address)
>>>> +{
>>>> +        u32 dat_w0;
>>>> +
>>>> +        dat_w0 = dat_w0_read(dat_idx);
>>>> +        dat_w0 &= ~(DAT_0_DYNAMIC_ADDRESS | DAT_0_DYNADDR_PARITY);
>>>> +        dat_w0 |= FIELD_PREP(DAT_0_DYNAMIC_ADDRESS, address) |
>>>> +                  (dynaddr_parity(address) ? DAT_0_DYNADDR_PARITY 
>>>> : 0);
>>>> +        dat_w0_write(dat_idx, dat_w0);
>>>> +}
>
> This code calculates the parity wrong. See commit e55905a3f33c ("i3c:
> mipi-i3c-hci: use parity8 helper instead of open coding it").
>
> If I recall correctly this becomes visible with the 3rd or 4th device on
> the bus and therefore was for long unnoticed.
>
>>>> +static int mchp_i3c_hci_alloc_safe_xfer_buf(struct mchp_i3c_hci *hci,
>>>> +                                    struct mchp_hci_xfer *xfer)
>>>> +{
>>>> +     if (hci->io == &mchp_mipi_i3c_hci_pio ||
>>>> +         xfer->data == NULL || !is_vmalloc_addr(xfer->data))
>>>> +             return 0;
>>>> +     if (xfer->rnw)
>>>> +             xfer->bounce_buf = kzalloc(xfer->data_len, GFP_KERNEL);
>>>> +     else
>>>> +             xfer->bounce_buf = kmemdup(xfer->data,
>>>> +                                        xfer->data_len, GFP_KERNEL);
>>>> +
>>>> +     return xfer->bounce_buf == NULL ? -ENOMEM : 0;
>>>> +}
>>>> +
>>>> +static void mchp_i3c_hci_free_safe_xfer_buf(struct mchp_i3c_hci *hci,
>>>> +                                    struct mchp_hci_xfer *xfer)
>>>> +{
>>>> +     if (hci->io == &mchp_mipi_i3c_hci_pio || xfer->bounce_buf == 
>>>> NULL)
>>>> +     return;
>>>> +     if (xfer->rnw)
>>>> +             memcpy(xfer->data, xfer->bounce_buf, xfer->data_len);
>>>> +
>>>> +     kfree(xfer->bounce_buf);
>>>> +}
>
> Sidenote, these bounce buf stuff are complete unneeded if your HW
> supports only PIO transfers.
>
>>>> +static int mchp_i3c_hci_attach_i3c_dev(struct i3c_dev_desc *dev)
>>>> +{
>>>> +     struct i3c_master_controller *m = i3c_dev_get_master(dev);
>>>> +     struct mchp_i3c_hci *hci = to_i3c_hci(m);
>>>> +     struct mchp_i3c_hci_dev_data *dev_data;
>>>> +     int ret;
>>>> +
>>>> +     dev_data = kzalloc(sizeof(*dev_data), GFP_KERNEL);
>>>> +     if (!dev_data)
>>>> +             return -ENOMEM;
>>>> +     if (hci->cmd == &mchp_mipi_i3c_hci_cmd_v1) {
>>>> +             ret = mchp_mipi_i3c_hci_dat_v1.alloc_entry(hci);
>>>> +             if (ret < 0) {
>>>> +                     kfree(dev_data);
>>>> +                     return ret;
>>>> +             }
>>>> +             mchp_mipi_i3c_hci_dat_v1.set_dynamic_addr(hci, ret, 
>>>> dev->info.dyn_addr);
>>>> +             dev_data->dat_idx = ret;
>>>> +     }
>>>> +     i3c_dev_set_master_data(dev, dev_data);
>>>> +     return 0;
>>>> +}
>
> See commit 2b50719dd92f ("i3c: mipi-i3c-hci: Support SETDASA CCC").
>

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c