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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c25bf384-a312-47a9-a27a-a943cbd33050@linux.intel.com>
Date: Wed, 10 Sep 2025 12:01:21 +0300
From: Jarkko Nikula <jarkko.nikula@...ux.intel.com>
To: Durai.ManickamKR@...rochip.com, Frank.li@....com
Cc: linux-i3c@...ts.infradead.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, alexandre.belloni@...tlin.com,
 robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 Balamanikandan.Gunasundar@...rochip.com, Nicolas.Ferre@...rochip.com
Subject: Re: [PATCH 2/4] i3c: master: add Microchip SAMA7D65 I3C HCI master
 driver

Hi

On 9/10/25 9:12 AM, Durai.ManickamKR@...rochip.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").