| Message ID | 20200814034854.460830-1-nico@fluxnic.net |
|---|---|
| Headers | show |
| Series | MIPI I3c HCI (Host Controller Interface) driver | expand |
Hi Nicolas, Nicolas Pitre <nico@fluxnic.net> wrote on Thu, 13 Aug 2020 23:48:54 -0400: > From: Nicolas Pitre <npitre@baylibre.com> > > This adds basic support for hardware implementing the MIPI I3C HCI > specification. This driver is currently limited by the capabilities > of the I3C subsystem, meaning things like scheduled commands, > auto-commands and NCM are not yet supported. > > This supports version 1.0 of the MIPI I3C HCI spec, as well as the > imminent release of version 1.1. Support for draft version 2.0 of the > spec is also partially included but is guaranteed to change as the spec > is still a work in progress. > > This is also lightly tested as actual hardware is still very scarse, > even for HCI v1.0. Further contributions to this driver are expected > once vendor implementations and new I3C devices become available. Nice work! I honnestly do not know a lot about HCIs and I basically had only minor nits to point out, see below. > > Signed-off-by: Nicolas Pitre <npitre@baylibre.com> > --- > drivers/i3c/master/Kconfig | 13 + > drivers/i3c/master/Makefile | 1 + > drivers/i3c/master/mipi-i3c-hci/Makefile | 9 + > drivers/i3c/master/mipi-i3c-hci/cmd.h | 106 +++ > drivers/i3c/master/mipi-i3c-hci/cmd_v1.c | 362 ++++++++ > drivers/i3c/master/mipi-i3c-hci/cmd_v2.c | 280 ++++++ > drivers/i3c/master/mipi-i3c-hci/core.c | 801 +++++++++++++++++ > drivers/i3c/master/mipi-i3c-hci/dat.h | 28 + > drivers/i3c/master/mipi-i3c-hci/dat_v1.c | 170 ++++ > drivers/i3c/master/mipi-i3c-hci/dct.h | 16 + > drivers/i3c/master/mipi-i3c-hci/dct_v1.c | 36 + > drivers/i3c/master/mipi-i3c-hci/debug.c | 79 ++ > drivers/i3c/master/mipi-i3c-hci/debug.h | 17 + > drivers/i3c/master/mipi-i3c-hci/dma.c | 767 ++++++++++++++++ > drivers/i3c/master/mipi-i3c-hci/ext_caps.c | 249 ++++++ > drivers/i3c/master/mipi-i3c-hci/ext_caps.h | 19 + > drivers/i3c/master/mipi-i3c-hci/hci.h | 150 ++++ > drivers/i3c/master/mipi-i3c-hci/ibi.h | 42 + > drivers/i3c/master/mipi-i3c-hci/pio.c | 961 +++++++++++++++++++++ > 19 files changed, 4106 insertions(+) > create mode 100644 drivers/i3c/master/mipi-i3c-hci/Makefile > create mode 100644 drivers/i3c/master/mipi-i3c-hci/cmd.h > create mode 100644 drivers/i3c/master/mipi-i3c-hci/cmd_v1.c > create mode 100644 drivers/i3c/master/mipi-i3c-hci/cmd_v2.c > create mode 100644 drivers/i3c/master/mipi-i3c-hci/core.c > create mode 100644 drivers/i3c/master/mipi-i3c-hci/dat.h > create mode 100644 drivers/i3c/master/mipi-i3c-hci/dat_v1.c > create mode 100644 drivers/i3c/master/mipi-i3c-hci/dct.h > create mode 100644 drivers/i3c/master/mipi-i3c-hci/dct_v1.c > create mode 100644 drivers/i3c/master/mipi-i3c-hci/debug.c > create mode 100644 drivers/i3c/master/mipi-i3c-hci/debug.h > create mode 100644 drivers/i3c/master/mipi-i3c-hci/dma.c > create mode 100644 drivers/i3c/master/mipi-i3c-hci/ext_caps.c > create mode 100644 drivers/i3c/master/mipi-i3c-hci/ext_caps.h > create mode 100644 drivers/i3c/master/mipi-i3c-hci/hci.h > create mode 100644 drivers/i3c/master/mipi-i3c-hci/ibi.h > create mode 100644 drivers/i3c/master/mipi-i3c-hci/pio.c > > diff --git a/drivers/i3c/master/Kconfig b/drivers/i3c/master/Kconfig > index 4e80a1fcbf..eb5830923f 100644 > --- a/drivers/i3c/master/Kconfig > +++ b/drivers/i3c/master/Kconfig > @@ -21,3 +21,16 @@ config DW_I3C_MASTER > > This driver can also be built as a module. If so, the module > will be called dw-i3c-master. > + > +config MIPI_I3C_HCI > + tristate "MIPI I3C Host Controller Interface driver" > + depends on I3C > + help > + Support for hardware following the MIPI Aliance's I3C Host Controller > + Interface specification. > + > + For details please see: > + https://www.mipi.org/specifications/i3c-hci > + > + This driver can also be built as a module. If so, the module will be > + called mipi-i3c-hci. > diff --git a/drivers/i3c/master/Makefile b/drivers/i3c/master/Makefile > index 7eea9e0861..b892fd4caf 100644 > --- a/drivers/i3c/master/Makefile > +++ b/drivers/i3c/master/Makefile > @@ -1,3 +1,4 @@ > # SPDX-License-Identifier: GPL-2.0-only > obj-$(CONFIG_CDNS_I3C_MASTER) += i3c-master-cdns.o > obj-$(CONFIG_DW_I3C_MASTER) += dw-i3c-master.o > +obj-$(CONFIG_MIPI_I3C_HCI) += mipi-i3c-hci/ > diff --git a/drivers/i3c/master/mipi-i3c-hci/Makefile b/drivers/i3c/master/mipi-i3c-hci/Makefile > new file mode 100644 > index 0000000000..8349960c5b > --- /dev/null > +++ b/drivers/i3c/master/mipi-i3c-hci/Makefile > @@ -0,0 +1,9 @@ > +# SPDX-License-Identifier: BSD-3-Clause Just out of curiosity, why this license? > + > +#ccflags-y := -DDEBUG Probably a leftover? > + > +obj-$(CONFIG_MIPI_I3C_HCI) += mipi-i3c-hci.o > +mipi-i3c-hci-y := core.o ext_caps.o pio.o dma.o \ > + cmd_v1.o cmd_v2.o \ > + dat_v1.o dct_v1.o \ > + debug.o [...] > +#define CMD_C1_DATA_LENGTH(v) FIELD_PREP(W1_MASK(63, 48), v) > +#define CMD_C1_OFFSET(v) FIELD_PREP(W1_MASK(47, 32), v) > +#define CMD_C0_TOC W0_BIT_(31) > +#define CMD_C0_ROC W0_BIT_(30) > +#define CMD_C0_RNW W0_BIT_(29) > +#define CMD_C0_MODE(v) FIELD_PREP(W0_MASK(28, 26), v) > +#define CMD_C0_16_BIT_SUBOFFSET W0_bit(25) > +#define CMD_C0_FIRST_PHASE_MODE W0_BIT_(24) > +#define CMD_C0_DATA_LENGTH_POSITION(v) FIELD_PREP(W0_MASK(23, 22), v) > +#define CMD_C0_DEV_INDEX(v) FIELD_PREP(W0_MASK(20, 16), v) > +#define CMD_C0_CP W0_BIT_(15) > +#define CMD_C0_CMD(v) FIELD_PREP(W0_MASK(14, 7), v) > +#define CMD_C0_TID(v) FIELD_PREP(W0_MASK(6, 3), v) > + > +/* > + * Internal Control Command > + */ > + > +#define CMD_0_ATTR_M FIELD_PREP(CMD_0_ATTR, 0x7) > + > +#define CMD_M1_VENDOR_SPECIFIC W1_MASK(63, 32) > +#define CMD_M0_MIPI_RESERVED W0_MASK(31, 12) > +#define CMD_M0_MIPI_CMD W0_MASK(11, 8) > +#define CMD_M0_VENDOR_INFO_PRESENT W0_BIT_(7) > +#define CMD_M0_TID(v) FIELD_PREP(W0_MASK(6, 3), v) > + > + > +static int hci_cmd_v1_prep_ccc(struct i3c_hci *hci, > + struct hci_xfer *xfer, > + u8 ccc_addr, u8 ccc_cmd, bool raw) > +{ > + u_int dat_idx = 0; I guess u_int her and below is not the preferred way to declare an unsigned int? > + int mode = hci_get_i3c_mode(hci); > + u8 *data = xfer->data; > + u_int data_len = xfer->data_len; > + bool rnw = xfer->rnw; > + int ret; > + > + BUG_ON(raw); It looks like 'raw' cannot be used with v1 (at least you seem to take care of it in v2), so maybe BUG_ON is a bit radical here and you can simply return an error? I think the use of BUG() is not appreciated in general. > + > + if (ccc_addr != I3C_BROADCAST_ADDR) { > + ret = i3c_hci_dat_get_index(hci, ccc_addr); > + if (ret < 0) > + return -ENODEV; > + dat_idx = ret; > + } > + > + xfer->cmd_tid = hci_get_tid(4); > + > + if (!rnw && data_len <= 4) { > + /* we use an Immediate Data Transfer Command */ > + xfer->cmd_desc[0] = > + CMD_0_ATTR_I | > + CMD_I0_TID(xfer->cmd_tid) | > + CMD_I0_CMD(ccc_cmd) | CMD_I0_CP | > + CMD_I0_DEV_INDEX(dat_idx) | > + CMD_I0_DTT(data_len) | > + CMD_I0_MODE(mode); > + xfer->cmd_desc[1] = 0; > + switch (data_len) { > + case 4: > + xfer->cmd_desc[1] |= CMD_I1_DATA_BYTE_4(data[3]); > + fallthrough; > + case 3: > + xfer->cmd_desc[1] |= CMD_I1_DATA_BYTE_3(data[2]); > + fallthrough; > + case 2: > + xfer->cmd_desc[1] |= CMD_I1_DATA_BYTE_2(data[1]); > + fallthrough; > + case 1: > + xfer->cmd_desc[1] |= CMD_I1_DATA_BYTE_1(data[0]); > + fallthrough; > + case 0: > + break; > + } > + /* we consumed all the data in the cmd descriptor */ > + xfer->data = NULL; > + } else { > + /* we use a Regular Data Transfer Command */ > + xfer->cmd_desc[0] = > + CMD_0_ATTR_R | > + CMD_R0_TID(xfer->cmd_tid) | > + CMD_R0_CMD(ccc_cmd) | CMD_R0_CP | > + CMD_R0_DEV_INDEX(dat_idx) | > + CMD_R0_MODE(mode) | > + (rnw ? CMD_R0_RNW : 0); > + xfer->cmd_desc[1] = > + CMD_R1_DATA_LENGTH(data_len); > + } > + > + return 0; > +} [...] > +static int hci_cmd_v1_daa(struct i3c_hci *hci) > +{ > + struct hci_xfer *xfer; > + int ret, dat_idx = -1; > + u8 next_addr; > + u64 pid; > + u_int dcr, bcr; > + DECLARE_COMPLETION_ONSTACK(done); > + > + xfer = hci_alloc_xfer(2); > + if (!xfer) > + return -ENOMEM; > + > + /* > + * Simple for now: we allocate a temporary DAT entry, do a single > + * DAA, register the device which will allocate its own DAT entry > + * via the core callback, then free the temporary DAT entry. > + * Loop until there is no more devices to assign an address to. > + * Yes, there is room for improvements. > + */ > + for (;;) { > + ret = i3c_hci_dat_alloc_entry(hci); > + if (ret < 0) > + break; > + dat_idx = ret; > + ret = i3c_master_get_free_addr(&hci->master, next_addr); > + if (ret < 0) > + break; > + next_addr = ret; > + > + DBG("next_addr = 0x%02x, DAA using DAT %d", next_addr, dat_idx); > + i3c_hci_dat_set_dynamic_addr(hci, dat_idx, next_addr); > + i3c_hci_dct_index_reset(hci); > + > + xfer->cmd_tid = hci_get_tid(4); > + xfer->cmd_desc[0] = > + CMD_0_ATTR_A | > + CMD_A0_TID(xfer->cmd_tid) | > + CMD_A0_CMD(I3C_CCC_ENTDAA) | > + CMD_A0_DEV_INDEX(dat_idx) | > + CMD_A0_DEV_COUNT(1) | > + CMD_A0_ROC | CMD_A0_TOC; > + xfer->cmd_desc[1] = 0; > + hci->io->queue_xfer(hci, xfer, 1); > + if (!wait_for_completion_timeout(&done, HZ) && > + hci->io->dequeue_xfer(hci, xfer, 1)) { > + ret = -ETIME; > + break; > + } > + if (RESP_STATUS(xfer[0].response) == RESP_ERR_NACK && > + RESP_STATUS(xfer[0].response) == 1) { > + ret = 0; /* no more devices to be assigned */ > + break; > + } > + if (RESP_STATUS(xfer[0].response) != RESP_SUCCESS) { > + ret = -EIO; > + break; > + } > + > + i3c_hci_dct_get_val(hci, 0, &pid, &dcr, &bcr); > + DBG("assigned address %#x to device PID=0x%llx DCR=%#x BCR=%#x", > + next_addr, pid, dcr, bcr); > + > + i3c_hci_dat_free_entry(hci, dat_idx); > + dat_idx = -1; > + > + /* > + * TODO: Extend the subsystem layer to allow for registering > + * new device and provide BCR/DCR/PID at the same time. Not sure this is needed if you don't use it directly as the core will anyway (in its current form) send the relevant CCC to read these registers. > + */ > + ret = i3c_master_add_i3c_dev_locked(&hci->master, next_addr); > + if (ret) > + break; > + } > + > + if (dat_idx >= 0) > + i3c_hci_dat_free_entry(hci, dat_idx); > + hci_free_xfer(xfer, 1); > + return ret; > +} > + > +const struct hci_cmd_ops i3c_hci_cmd_v1 = { > + .prep_ccc = hci_cmd_v1_prep_ccc, > + .prep_i3c_xfer = hci_cmd_v1_prep_i3c_xfer, > + .prep_i2c_xfer = hci_cmd_v1_prep_i2c_xfer, > + .perform_daa = hci_cmd_v1_daa, I know Boris does not like such space alignment :) > +}; [...] > +void i3c_hci_resume(struct i3c_hci *hci) > +{ > + /* the HC_CONTROL_RESUME bit is R/W1C so just read and write back */ > + reg_write(HC_CONTROL, reg_read(HC_CONTROL)); > +} > + > +/* located here rather than pio.c because needed bits are in core reg space */ > +void i3c_hci_pio_reset(struct i3c_hci *hci) > +{ > + reg_write(RESET_CONTROL, RX_FIFO_RST|TX_FIFO_RST|RESP_QUEUE_RST); Style with missing spaces ^ ^ > +} > + > +/* located here rather than dct.c because needed bits are in core reg space */ > +void i3c_hci_dct_index_reset(struct i3c_hci *hci) > +{ > + reg_write(DCT_SECTION_OFFSET, FIELD_PREP(DCT_TABLE_INDEX, 0)); > +} > + > +static int i3c_hci_send_ccc_cmd(struct i3c_master_controller *m, > + struct i3c_ccc_cmd *ccc) > +{ > + struct i3c_hci *hci = to_i3c_hci(m); > + struct hci_xfer *xfer; > + bool raw = !!(hci->quirks & HCI_QUIRK_RAW_CCC); > + bool prefixed = raw && !!(ccc->id & I3C_CCC_DIRECT); > + u_int nxfers = ccc->ndests + prefixed; > + DECLARE_COMPLETION_ONSTACK(done); > + int i, last, ret = 0; > + > + DBG("cmd=%#x rnw=%d ndests=%d data[0].len=%d", > + ccc->id, ccc->rnw, ccc->ndests, ccc->dests[0].payload.len); > + > + xfer = hci_alloc_xfer(nxfers); > + if (!xfer) > + return -ENOMEM; > + > + if (prefixed) { > + xfer->data = NULL; > + xfer->data_len = 0; > + xfer->rnw = false; > + hci->cmd->prep_ccc(hci, xfer, I3C_BROADCAST_ADDR, > + ccc->id, true); > + xfer++; > + } > + > + for (i = 0; i < nxfers - prefixed; i++) { > + xfer[i].data = ccc->dests[i].payload.data; > + xfer[i].data_len = ccc->dests[i].payload.len; > + xfer[i].rnw = ccc->rnw; > + ret = hci->cmd->prep_ccc(hci, &xfer[i], ccc->dests[i].addr, > + ccc->id, raw); > + if (ret) > + goto out; > + xfer[i].cmd_desc[0] |= CMD_0_ROC; > + } > + last = i - 1; > + xfer[last].cmd_desc[0] |= CMD_0_TOC; > + xfer[last].completion = &done; > + > + if (prefixed) > + xfer--; > + > + ret = hci->io->queue_xfer(hci, xfer, nxfers); > + if (ret) > + goto out; > + if (!wait_for_completion_timeout(&done, HZ) && > + hci->io->dequeue_xfer(hci, xfer, nxfers)) { > + ret = -ETIME; > + goto out; > + } > + for (i = prefixed; i < nxfers; i++) { > + if (ccc->rnw) > + ccc->dests[i - prefixed].payload.len = > + RESP_DATA_LENGTH(xfer[i].response); > + if (RESP_STATUS(xfer[i].response) != RESP_SUCCESS) { > + ret = -EIO; > + goto out; > + } > + } > + > +#if 0 > + if (ccc->rnw) { > + HEXDUMP("got: ", ccc->dests[0].payload.data, > + ccc->dests[0].payload.len); > + } > +#endif I guess this debug block can be dropped too (there are many debug information the should probably be dropped or turned into dev_info() or similar). > + > +out: > + hci_free_xfer(xfer, nxfers); > + return ret; > +} > + > +static int i3c_hci_daa(struct i3c_master_controller *m) > +{ > + struct i3c_hci *hci = to_i3c_hci(m); > + > + DBG(""); > + > + return hci->cmd->perform_daa(hci); > +} [...] > diff --git a/drivers/i3c/master/mipi-i3c-hci/debug.c b/drivers/i3c/master/mipi-i3c-hci/debug.c > new file mode 100644 > index 0000000000..3885499119 > --- /dev/null > +++ b/drivers/i3c/master/mipi-i3c-hci/debug.c > @@ -0,0 +1,79 @@ > +// SPDX-License-Identifier: BSD-3-Clause > +/* > + * Copyright (c) 2020, MIPI Alliance, Inc. > + * > + * Author: Nicolas Pitre <npitre@baylibre.com> > + */ > + > +#include <linux/device.h> > +#include <linux/io.h> > +#include <linux/i3c/master.h> > +#include <linux/printk.h> > + > +#include "hci.h" > +#include "debug.h" > + > +/* > + * Paul Kimelman's debug trace log facility. > + * This is completely vendor and hardware specific. > + */ > +void __PK_debug_trace(struct i3c_hci *hci, const char *func) > +{ > + void __iomem *trcp = (void __iomem *)hci->vendor_data + 7*4; Maybe you need to define what is 7*4 , 0*4 below, v >> 27, etc Also there are many missing spaces between operators (7 * 4,w & (1 <<9). > + uint32_t idx, cycles, match; > + uint32_t trace_full = 1; > + > + if (!hci->vendor_data) > + return; > + > + idx = readl(trcp + 0*4); > + cycles = idx>>16; > + idx &= 0xFFFF; > + if (idx > 0xFFF) { > + pr_info("Trace count=%d, stored=%d; cycles=%d\n", idx, 0xFFF, cycles); > + match = (idx-1) & 0xFFF; > + } else { > + pr_info("Trace count=%d; cycles=%d\n", idx, cycles); > + match = idx; > + idx = 0; > + } > + > + writel(idx | (trace_full << 31), trcp + 1*4); // 1st to read > + for (; idx != match; idx = (idx+1)&0xFFF) { // ring through it > + uint32_t v = readl(trcp + 1*4); // get trace word > + uint32_t up = trace_full ? readl(trcp + 1*4) : 0; > + char *e2 = (v&(1<<9))?"/NACK":""; > + char *e3 = (v&(1<<10))?"/WRABT":""; > + char *e4 = (v&(1<<11))?"/TERM":""; > + char *e5 = (v & (1<<7)) ? "/INVREQ" : ""; > + char *e6 = (v & (1<<8)) ? "/TIMEOUT" : ""; > + if (trace_full) { > + uint32_t cmd = up >> 16; > + pr_info("%-5d: ", up & 0xFFFF); > + if (cmd) > + pr_info("cmd=%s tid=%02X, TOC=%d, ROC=%d, %s %s addr=%X\n---", > + (cmd & (1<<2)) ? "Cmd" : "DAA", > + (cmd >> 3) & 0xF, cmd >> 31, (cmd >> 30) & 1, > + ((cmd >> 7) & 1) ? "I2C" : "I3C", > + (cmd & (1<<8))?"Read ":"Write", > + (cmd >> 9) & 0x7F); > + else > + pr_cont("\n----"); > + } else > + pr_info("%08X: ", v); > + pr_cont("#%3X: st=%03X,status=%02X%s%s%s%s%s%s%s%s%s%s%s%s\n", idx, > + v & 0x7F, (v >> 12) & 0xFFF, ((v >> 24) & 1) ? ", clrs" : "", > + ((v >> 25) & 1) ? ", ERROR" : "", e2, e3, e4, e5, e6, > + ((v >> 26) & 1) ? ",clre" : "", ((v >> 27) & 1) ? ", NACK" : "", > + ((v >> 28) & 1) ? ", IN_DAA" : "", > + ((v >> 29) & 1) ? ", IRQ" : "", ((v >> 30) & 1) ? ", cRSP" : ""); > + } > + // clear trace index so can tell when we have new ones > + writel(0, trcp + 0*4); > +} [...] > +static void hci_dma_cleanup(struct i3c_hci *hci) > +{ > + struct hci_rings_data *rings = hci->io_data; > + struct hci_rh_data *rh; > + u_int i; > + > + if (!rings) > + return; > + > + for (i = 0; i < rings->total; i++) { > + rh = &rings->headers[i]; > + > + rh_reg_write(RING_CONTROL, 0); > + rh_reg_write(CR_SETUP, 0); > + rh_reg_write(IBI_SETUP, 0); > + rh_reg_write(INTR_SIGNAL_ENABLE, 0); > + > + if (rh->xfer) > + dma_free_coherent(&hci->master.dev, > + rh->xfer_struct_sz * rh->xfer_entries, > + rh->xfer, rh->xfer_phys); > + if (rh->resp) > + dma_free_coherent(&hci->master.dev, > + rh->resp_struct_sz * rh->xfer_entries, > + rh->resp, rh->resp_phys); > + kfree(rh->src_xfers); > + if (rh->ibi_status) > + dma_free_coherent(&hci->master.dev, > + rh->ibi_status_sz * rh->ibi_status_entries, > + rh->ibi_status, rh->ibi_status_phys); > + if (rh->ibi_data_phys) I was told that _phys was a very bad suffix for something which is a DMA address an not focibly a physical address. > + dma_unmap_single(&hci->master.dev, rh->ibi_data_phys, > + rh->ibi_chunk_sz * rh->ibi_chunks_total, > + DMA_FROM_DEVICE); > + kfree(rh->ibi_data); > + } > + > + rhs_reg_write(CONTROL, 0); > + > + kfree(rings); > + hci->io_data = NULL; > +} [...] > +static bool hci_dma_dequeue_xfer(struct i3c_hci *hci, > + struct hci_xfer *xfer_list, int n) > +{ > + struct hci_rings_data *rings = hci->io_data; > + struct hci_rh_data *rh = &rings->headers[xfer_list[0].ring]; > + u_int i; > + bool did_unqueue = false; > + > + /* stop the ring */ > + rh_reg_write(RING_CONTROL, RING_CTRL_ABORT); > + if (wait_for_completion_timeout(&rh->op_done, HZ) == 0) { > + /* > + * We're deep in it if ever this condition is ever met. > + * Hardware might still be writing to memory, etc. > + */ > + ERR("unable to abort the ring"); > + BUG(); Why not just treating the error as always? > + } > + > + for (i = 0; i < n; i++) { > + struct hci_xfer *xfer = xfer_list + i; > + int idx = xfer->ring_entry; > + > + /* > + * At the time the abort happened, the xfer might have > + * completed already. If not then replace corresponding > + * descriptor entries with a no-op. > + */ > + if (idx >= 0) { > + u32 *ring_data = rh->xfer + rh->xfer_struct_sz * idx; > + > + /* store no-op cmd descriptor */ > + *ring_data++ = FIELD_PREP(CMD_0_ATTR, 0x7); > + *ring_data++ = 0; > + if (hci->cmd == &i3c_hci_cmd_v2) { > + *ring_data++ = 0; > + *ring_data++ = 0; > + } > + > + /* disassociate this xfer struct */ > + rh->src_xfers[idx] = NULL; > + > + /* and unmap it */ > + hci_dma_unmap_xfer(hci, xfer, 1); > + > + did_unqueue = true; > + } > + } > + > + /* restart the ring */ > + rh_reg_write(RING_CONTROL, RING_CTRL_ENABLE); > + > + return did_unqueue; > +} [...] > +/* > + * Structure to represent a master initiated transfer. > + * The rnw, data and data_len fields must be initialized before calling any > + * hci->cmd->*() method. The cmd method will initialize cmd_desc[] and > + * possibly modify (clear) the data field. Then xfer->cmd_desc[0] can > + * be augmented with CMD_0_ROC and/or CMD_0_TOC. > + * The completion field needs to be initialized before queueing with > + * hci->io->queue_xfer(), and requires CMD_0_ROC to be set. > + */ > +struct hci_xfer { > + u32 cmd_desc[4]; > + u32 response; > + bool rnw; > + void *data; > + u_int data_len; > + u_int cmd_tid; > + struct completion *completion; > + union { > + struct { > + /* PIO specific */ > + struct hci_xfer *next_xfer; > + struct hci_xfer *next_data; > + struct hci_xfer *next_resp; > + u_int data_left; > + u32 data_word_before_partial; > + }; I think anonymous unions are prohibited because the kernel is supposed to be built with old gcc versions which do not support it. > + struct { > + /* DMA specific */ > + dma_addr_t data_phys; > + u_int ring; > + int ring_entry; > + }; > + }; > +}; > + > +static inline struct hci_xfer *hci_alloc_xfer(u_int n) > +{ > + return kzalloc(sizeof(struct hci_xfer) * n, GFP_KERNEL); > +} > + > +static inline void hci_free_xfer(struct hci_xfer *xfer, u_int n) > +{ > + kfree(xfer); > +} > + > + Thanks, Miquèl
On Thu, 20 Aug 2020 10:08:29 +0200 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > + /* > > + * TODO: Extend the subsystem layer to allow for registering > > + * new device and provide BCR/DCR/PID at the same time. > > Not sure this is needed if you don't use it directly as the core will > anyway (in its current form) send the relevant CCC to read these > registers. We considered optimizing that in the past but that means making the DAA and SETDASA registration different. I'm not sure it's worth it to be honest, PID/DCR/BCR only happens when initializing devices and I suspect the overhead of querying those DATA twice in case of DAA is negligible anyway.
On Thu, 20 Aug 2020, Miquel Raynal wrote: > Hi Nicolas, > > Nicolas Pitre <nico@fluxnic.net> wrote on Thu, 13 Aug 2020 23:48:54 -0400: > > > From: Nicolas Pitre <npitre@baylibre.com> > > > > This adds basic support for hardware implementing the MIPI I3C HCI > > specification. This driver is currently limited by the capabilities > > of the I3C subsystem, meaning things like scheduled commands, > > auto-commands and NCM are not yet supported. > > > > This supports version 1.0 of the MIPI I3C HCI spec, as well as the > > imminent release of version 1.1. Support for draft version 2.0 of the > > spec is also partially included but is guaranteed to change as the spec > > is still a work in progress. > > > > This is also lightly tested as actual hardware is still very scarse, > > even for HCI v1.0. Further contributions to this driver are expected > > once vendor implementations and new I3C devices become available. > > Nice work! I honnestly do not know a lot about HCIs and I basically > had only minor nits to point out, see below. Thanks for your comments. Please find my replies inline. > > diff --git a/drivers/i3c/master/mipi-i3c-hci/Makefile b/drivers/i3c/master/mipi-i3c-hci/Makefile > > new file mode 100644 > > index 0000000000..8349960c5b > > --- /dev/null > > +++ b/drivers/i3c/master/mipi-i3c-hci/Makefile > > @@ -0,0 +1,9 @@ > > +# SPDX-License-Identifier: BSD-3-Clause > > Just out of curiosity, why this license? MIPI is sponsoring this work and they own the copyright. It is their policy to license all their Open Source code as BSD. > > + > > +#ccflags-y := -DDEBUG > > Probably a leftover? Well, I left it there intentionally as the code is still actively being developed, so full debugging can quickly be reactivated by anyone. I can remove it if deemed too distracting. > [...] > > > +#define CMD_C1_DATA_LENGTH(v) FIELD_PREP(W1_MASK(63, 48), v) > > +#define CMD_C1_OFFSET(v) FIELD_PREP(W1_MASK(47, 32), v) > > +#define CMD_C0_TOC W0_BIT_(31) > > +#define CMD_C0_ROC W0_BIT_(30) > > +#define CMD_C0_RNW W0_BIT_(29) > > +#define CMD_C0_MODE(v) FIELD_PREP(W0_MASK(28, 26), v) > > +#define CMD_C0_16_BIT_SUBOFFSET W0_bit(25) > > +#define CMD_C0_FIRST_PHASE_MODE W0_BIT_(24) > > +#define CMD_C0_DATA_LENGTH_POSITION(v) FIELD_PREP(W0_MASK(23, 22), v) > > +#define CMD_C0_DEV_INDEX(v) FIELD_PREP(W0_MASK(20, 16), v) > > +#define CMD_C0_CP W0_BIT_(15) > > +#define CMD_C0_CMD(v) FIELD_PREP(W0_MASK(14, 7), v) > > +#define CMD_C0_TID(v) FIELD_PREP(W0_MASK(6, 3), v) > > + > > +/* > > + * Internal Control Command > > + */ > > + > > +#define CMD_0_ATTR_M FIELD_PREP(CMD_0_ATTR, 0x7) > > + > > +#define CMD_M1_VENDOR_SPECIFIC W1_MASK(63, 32) > > +#define CMD_M0_MIPI_RESERVED W0_MASK(31, 12) > > +#define CMD_M0_MIPI_CMD W0_MASK(11, 8) > > +#define CMD_M0_VENDOR_INFO_PRESENT W0_BIT_(7) > > +#define CMD_M0_TID(v) FIELD_PREP(W0_MASK(6, 3), v) > > + > > + > > +static int hci_cmd_v1_prep_ccc(struct i3c_hci *hci, > > + struct hci_xfer *xfer, > > + u8 ccc_addr, u8 ccc_cmd, bool raw) > > +{ > > + u_int dat_idx = 0; > > I guess u_int her and below is not the preferred way to declare an unsigned int? Why not? > > + int mode = hci_get_i3c_mode(hci); > > + u8 *data = xfer->data; > > + u_int data_len = xfer->data_len; > > + bool rnw = xfer->rnw; > > + int ret; > > + > > + BUG_ON(raw); > > It looks like 'raw' cannot be used with v1 (at least you seem to take > care of it in v2), so maybe BUG_ON is a bit radical here and you can > simply return an error? I think the use of BUG() is not appreciated in > general. That depends. Judgement is needed for BUG() usage. Here raw is absolutely impossible with v1 hardware and if ever this happens this is definitely a software bug that needs fixing right away. There is no point returning a runtime error code in that case as the upper layer won't know what to do about it. On the other hand, you absolutely don't want to BUG() on a condition that could _eventually_ happen at run time during normal usage. But that's not the case here. > > +const struct hci_cmd_ops i3c_hci_cmd_v1 = { > > + .prep_ccc = hci_cmd_v1_prep_ccc, > > + .prep_i3c_xfer = hci_cmd_v1_prep_i3c_xfer, > > + .prep_i2c_xfer = hci_cmd_v1_prep_i2c_xfer, > > + .perform_daa = hci_cmd_v1_daa, > > I know Boris does not like such space alignment :) Well... unfortunately for Boris, this is overwhelmingly prevalent in the kernel code: $ git grep "^"$'\t'"\.[^ ]*"$'\t'"*= " And I do like it. ;-P > > +void i3c_hci_pio_reset(struct i3c_hci *hci) > > +{ > > + reg_write(RESET_CONTROL, RX_FIFO_RST|TX_FIFO_RST|RESP_QUEUE_RST); > > Style with missing spaces ^ ^ True. Will fix. > > +static int i3c_hci_send_ccc_cmd(struct i3c_master_controller *m, > > + struct i3c_ccc_cmd *ccc) > > +{ > > + struct i3c_hci *hci = to_i3c_hci(m); > > + struct hci_xfer *xfer; > > + bool raw = !!(hci->quirks & HCI_QUIRK_RAW_CCC); > > + bool prefixed = raw && !!(ccc->id & I3C_CCC_DIRECT); > > + u_int nxfers = ccc->ndests + prefixed; > > + DECLARE_COMPLETION_ONSTACK(done); > > + int i, last, ret = 0; > > + > > + DBG("cmd=%#x rnw=%d ndests=%d data[0].len=%d", > > + ccc->id, ccc->rnw, ccc->ndests, ccc->dests[0].payload.len); > > + > > + xfer = hci_alloc_xfer(nxfers); > > + if (!xfer) > > + return -ENOMEM; > > + > > + if (prefixed) { > > + xfer->data = NULL; > > + xfer->data_len = 0; > > + xfer->rnw = false; > > + hci->cmd->prep_ccc(hci, xfer, I3C_BROADCAST_ADDR, > > + ccc->id, true); > > + xfer++; > > + } > > + > > + for (i = 0; i < nxfers - prefixed; i++) { > > + xfer[i].data = ccc->dests[i].payload.data; > > + xfer[i].data_len = ccc->dests[i].payload.len; > > + xfer[i].rnw = ccc->rnw; > > + ret = hci->cmd->prep_ccc(hci, &xfer[i], ccc->dests[i].addr, > > + ccc->id, raw); > > + if (ret) > > + goto out; > > + xfer[i].cmd_desc[0] |= CMD_0_ROC; > > + } > > + last = i - 1; > > + xfer[last].cmd_desc[0] |= CMD_0_TOC; > > + xfer[last].completion = &done; > > + > > + if (prefixed) > > + xfer--; > > + > > + ret = hci->io->queue_xfer(hci, xfer, nxfers); > > + if (ret) > > + goto out; > > + if (!wait_for_completion_timeout(&done, HZ) && > > + hci->io->dequeue_xfer(hci, xfer, nxfers)) { > > + ret = -ETIME; > > + goto out; > > + } > > + for (i = prefixed; i < nxfers; i++) { > > + if (ccc->rnw) > > + ccc->dests[i - prefixed].payload.len = > > + RESP_DATA_LENGTH(xfer[i].response); > > + if (RESP_STATUS(xfer[i].response) != RESP_SUCCESS) { > > + ret = -EIO; > > + goto out; > > + } > > + } > > + > > +#if 0 > > + if (ccc->rnw) { > > + HEXDUMP("got: ", ccc->dests[0].payload.data, > > + ccc->dests[0].payload.len); > > + } > > +#endif > > I guess this debug block can be dropped too (there are many debug > information the should probably be dropped or turned into dev_info() > or similar). Again, hardware bringup from different vendors and other developments are still ongoing. I'd wish for those to stay for the time being unless people feel strongly enough about these to become a merge show stopper. > > +/* > > + * Paul Kimelman's debug trace log facility. > > + * This is completely vendor and hardware specific. > > + */ > > +void __PK_debug_trace(struct i3c_hci *hci, const char *func) > > +{ > > + void __iomem *trcp = (void __iomem *)hci->vendor_data + 7*4; > > Maybe you need to define what is 7*4 , 0*4 below, v >> 27, etc > > Also there are many missing spaces between operators (7 * 4,w & (1 <<9). I think in this case this is really crossing the distraction threshold. This is used to extract debugging traces out of a specific FPGA implementation and is of no use to anyone else. I'll just rip that out from the next submission. > > + if (rh->ibi_data_phys) > > I was told that _phys was a very bad suffix for something which is a > DMA address an not focibly a physical address. Fair enough. The HCI spec refers to these as "physical memory" hence the suffix. What were you told to use instead? > > +static bool hci_dma_dequeue_xfer(struct i3c_hci *hci, > > + struct hci_xfer *xfer_list, int n) > > +{ > > + struct hci_rings_data *rings = hci->io_data; > > + struct hci_rh_data *rh = &rings->headers[xfer_list[0].ring]; > > + u_int i; > > + bool did_unqueue = false; > > + > > + /* stop the ring */ > > + rh_reg_write(RING_CONTROL, RING_CTRL_ABORT); > > + if (wait_for_completion_timeout(&rh->op_done, HZ) == 0) { > > + /* > > + * We're deep in it if ever this condition is ever met. > > + * Hardware might still be writing to memory, etc. > > + */ > > + ERR("unable to abort the ring"); > > + BUG(); > > Why not just treating the error as always? Again, if this ever happens, you're screwed. That means potential DMA engines could still be alive and about to scribble over memory that is about to be freed which may cause all sorts of impossible-to-find bugs in unrelated parts of the kernel. There is no point going on reporting such error condition to upper layers until the software, or possibly the hardware, is fixed > > +struct hci_xfer { > > + u32 cmd_desc[4]; > > + u32 response; > > + bool rnw; > > + void *data; > > + u_int data_len; > > + u_int cmd_tid; > > + struct completion *completion; > > + union { > > + struct { > > + /* PIO specific */ > > + struct hci_xfer *next_xfer; > > + struct hci_xfer *next_data; > > + struct hci_xfer *next_resp; > > + u_int data_left; > > + u32 data_word_before_partial; > > + }; > > I think anonymous unions are prohibited because the kernel is supposed > to be built with old gcc versions which do not support it. Hmmm... let's see: According to Documentation/process/changes.rst the current minimal required gcc version is 4.9. The oldest gcc I have lying around is 4.5.1 and it supports anonymous unions just fine. So I think we're clear. Nicolas
On Thu, 20 Aug 2020, Boris Brezillon wrote: > On Thu, 20 Aug 2020 10:08:29 +0200 > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > + /* > > > + * TODO: Extend the subsystem layer to allow for registering > > > + * new device and provide BCR/DCR/PID at the same time. > > > > Not sure this is needed if you don't use it directly as the core will > > anyway (in its current form) send the relevant CCC to read these > > registers. > > We considered optimizing that in the past but that means making the DAA > and SETDASA registration different. I'm not sure it's worth it to be > honest, PID/DCR/BCR only happens when initializing devices and I > suspect the overhead of querying those DATA twice in case of DAA is > negligible anyway. Wellllll... I know some people who do feel strongly about this particular issue for some reasons. So I'd prefer giving them some hope and leave the door open to some i3c_master_add_i3c_dev_and_info() interface. In the end, it's just a matter of pre-filling the info struct and skipping the PID retrieval in i3c_master_getpid_locked() if already available, etc. Nicolas
On Thu, 20 Aug 2020 12:34:13 -0400 (EDT) Nicolas Pitre <nico@fluxnic.net> wrote: > On Thu, 20 Aug 2020, Miquel Raynal wrote: > > > + > > > +#ccflags-y := -DDEBUG > > > > Probably a leftover? > > Well, I left it there intentionally as the code is still actively being > developed, so full debugging can quickly be reactivated by anyone. > I can remove it if deemed too distracting. How about using dynamic printk instead? I'm pretty sure you don't need to debug I3C stuff early enough to warrant usage of DDEBUG. > > > [...] > > > > > +#define CMD_C1_DATA_LENGTH(v) FIELD_PREP(W1_MASK(63, 48), v) > > > +#define CMD_C1_OFFSET(v) FIELD_PREP(W1_MASK(47, 32), v) > > > +#define CMD_C0_TOC W0_BIT_(31) > > > +#define CMD_C0_ROC W0_BIT_(30) > > > +#define CMD_C0_RNW W0_BIT_(29) > > > +#define CMD_C0_MODE(v) FIELD_PREP(W0_MASK(28, 26), v) > > > +#define CMD_C0_16_BIT_SUBOFFSET W0_bit(25) > > > +#define CMD_C0_FIRST_PHASE_MODE W0_BIT_(24) > > > +#define CMD_C0_DATA_LENGTH_POSITION(v) FIELD_PREP(W0_MASK(23, 22), v) > > > +#define CMD_C0_DEV_INDEX(v) FIELD_PREP(W0_MASK(20, 16), v) > > > +#define CMD_C0_CP W0_BIT_(15) > > > +#define CMD_C0_CMD(v) FIELD_PREP(W0_MASK(14, 7), v) > > > +#define CMD_C0_TID(v) FIELD_PREP(W0_MASK(6, 3), v) > > > + > > > +/* > > > + * Internal Control Command > > > + */ > > > + > > > +#define CMD_0_ATTR_M FIELD_PREP(CMD_0_ATTR, 0x7) > > > + > > > +#define CMD_M1_VENDOR_SPECIFIC W1_MASK(63, 32) > > > +#define CMD_M0_MIPI_RESERVED W0_MASK(31, 12) > > > +#define CMD_M0_MIPI_CMD W0_MASK(11, 8) > > > +#define CMD_M0_VENDOR_INFO_PRESENT W0_BIT_(7) > > > +#define CMD_M0_TID(v) FIELD_PREP(W0_MASK(6, 3), v) > > > + > > > + > > > +static int hci_cmd_v1_prep_ccc(struct i3c_hci *hci, > > > + struct hci_xfer *xfer, > > > + u8 ccc_addr, u8 ccc_cmd, bool raw) > > > +{ > > > + u_int dat_idx = 0; > > > > I guess u_int her and below is not the preferred way to declare an unsigned int? > > Why not? > > > > + int mode = hci_get_i3c_mode(hci); > > > + u8 *data = xfer->data; > > > + u_int data_len = xfer->data_len; > > > + bool rnw = xfer->rnw; > > > + int ret; > > > + > > > + BUG_ON(raw); > > > > It looks like 'raw' cannot be used with v1 (at least you seem to take > > care of it in v2), so maybe BUG_ON is a bit radical here and you can > > simply return an error? I think the use of BUG() is not appreciated in > > general. > > That depends. Judgement is needed for BUG() usage. > > Here raw is absolutely impossible with v1 hardware and if ever this > happens this is definitely a software bug that needs fixing right away. > There is no point returning a runtime error code in that case as the > upper layer won't know what to do about it. > > On the other hand, you absolutely don't want to BUG() on a condition > that could _eventually_ happen at run time during normal usage. But > that's not the case here. Well, people have tried to eradicate BUG() occurrences, so let's not add new ones if we can avoid it. How about a WARN_ON()+error: if (WARN_ON(raw)) return -EINVAL; > > > > +const struct hci_cmd_ops i3c_hci_cmd_v1 = { > > > + .prep_ccc = hci_cmd_v1_prep_ccc, > > > + .prep_i3c_xfer = hci_cmd_v1_prep_i3c_xfer, > > > + .prep_i2c_xfer = hci_cmd_v1_prep_i2c_xfer, > > > + .perform_daa = hci_cmd_v1_daa, > > > > I know Boris does not like such space alignment :) > > Well... unfortunately for Boris, this is overwhelmingly prevalent in the > kernel code: > > $ git grep "^"$'\t'"\.[^ ]*"$'\t'"*= " > > And I do like it. ;-P The rational being this preference is that sooner or later someone will add a field to hci_cmd_ops that messes up your nice formatting :P. Anyway, that's definitely not a blocker. > > > > +void i3c_hci_pio_reset(struct i3c_hci *hci) > > > +{ > > > + reg_write(RESET_CONTROL, RX_FIFO_RST|TX_FIFO_RST|RESP_QUEUE_RST); > > > > Style with missing spaces ^ ^ > > True. Will fix. > > > > +static int i3c_hci_send_ccc_cmd(struct i3c_master_controller *m, > > > + struct i3c_ccc_cmd *ccc) > > > +{ > > > + struct i3c_hci *hci = to_i3c_hci(m); > > > + struct hci_xfer *xfer; > > > + bool raw = !!(hci->quirks & HCI_QUIRK_RAW_CCC); > > > + bool prefixed = raw && !!(ccc->id & I3C_CCC_DIRECT); > > > + u_int nxfers = ccc->ndests + prefixed; > > > + DECLARE_COMPLETION_ONSTACK(done); > > > + int i, last, ret = 0; > > > + > > > + DBG("cmd=%#x rnw=%d ndests=%d data[0].len=%d", > > > + ccc->id, ccc->rnw, ccc->ndests, ccc->dests[0].payload.len); > > > + > > > + xfer = hci_alloc_xfer(nxfers); > > > + if (!xfer) > > > + return -ENOMEM; > > > + > > > + if (prefixed) { > > > + xfer->data = NULL; > > > + xfer->data_len = 0; > > > + xfer->rnw = false; > > > + hci->cmd->prep_ccc(hci, xfer, I3C_BROADCAST_ADDR, > > > + ccc->id, true); > > > + xfer++; > > > + } > > > + > > > + for (i = 0; i < nxfers - prefixed; i++) { > > > + xfer[i].data = ccc->dests[i].payload.data; > > > + xfer[i].data_len = ccc->dests[i].payload.len; > > > + xfer[i].rnw = ccc->rnw; > > > + ret = hci->cmd->prep_ccc(hci, &xfer[i], ccc->dests[i].addr, > > > + ccc->id, raw); > > > + if (ret) > > > + goto out; > > > + xfer[i].cmd_desc[0] |= CMD_0_ROC; > > > + } > > > + last = i - 1; > > > + xfer[last].cmd_desc[0] |= CMD_0_TOC; > > > + xfer[last].completion = &done; > > > + > > > + if (prefixed) > > > + xfer--; > > > + > > > + ret = hci->io->queue_xfer(hci, xfer, nxfers); > > > + if (ret) > > > + goto out; > > > + if (!wait_for_completion_timeout(&done, HZ) && > > > + hci->io->dequeue_xfer(hci, xfer, nxfers)) { > > > + ret = -ETIME; > > > + goto out; > > > + } > > > + for (i = prefixed; i < nxfers; i++) { > > > + if (ccc->rnw) > > > + ccc->dests[i - prefixed].payload.len = > > > + RESP_DATA_LENGTH(xfer[i].response); > > > + if (RESP_STATUS(xfer[i].response) != RESP_SUCCESS) { > > > + ret = -EIO; > > > + goto out; > > > + } > > > + } > > > + > > > +#if 0 > > > + if (ccc->rnw) { > > > + HEXDUMP("got: ", ccc->dests[0].payload.data, > > > + ccc->dests[0].payload.len); > > > + } > > > +#endif > > > > I guess this debug block can be dropped too (there are many debug > > information the should probably be dropped or turned into dev_info() > > or similar). > > Again, hardware bringup from different vendors and other developments > are still ongoing. I'd wish for those to stay for the time being unless > people feel strongly enough about these to become a merge show stopper. Can't we replace that by a dev_dbg() using the %*pE formater? > > > > +/* > > > + * Paul Kimelman's debug trace log facility. > > > + * This is completely vendor and hardware specific. > > > + */ > > > +void __PK_debug_trace(struct i3c_hci *hci, const char *func) > > > +{ > > > + void __iomem *trcp = (void __iomem *)hci->vendor_data + 7*4; > > > > Maybe you need to define what is 7*4 , 0*4 below, v >> 27, etc > > > > Also there are many missing spaces between operators (7 * 4,w & (1 <<9). > > I think in this case this is really crossing the distraction threshold. > This is used to extract debugging traces out of a specific FPGA > implementation and is of no use to anyone else. I'll just rip that out > from the next submission. > > > > + if (rh->ibi_data_phys) > > > > I was told that _phys was a very bad suffix for something which is a > > DMA address an not focibly a physical address. > > Fair enough. The HCI spec refers to these as "physical memory" hence the > suffix. What were you told to use instead? Maybe _dma instead of _phys? > > > > +static bool hci_dma_dequeue_xfer(struct i3c_hci *hci, > > > + struct hci_xfer *xfer_list, int n) > > > +{ > > > + struct hci_rings_data *rings = hci->io_data; > > > + struct hci_rh_data *rh = &rings->headers[xfer_list[0].ring]; > > > + u_int i; > > > + bool did_unqueue = false; > > > + > > > + /* stop the ring */ > > > + rh_reg_write(RING_CONTROL, RING_CTRL_ABORT); > > > + if (wait_for_completion_timeout(&rh->op_done, HZ) == 0) { > > > + /* > > > + * We're deep in it if ever this condition is ever met. > > > + * Hardware might still be writing to memory, etc. > > > + */ > > > + ERR("unable to abort the ring"); > > > + BUG(); > > > > Why not just treating the error as always? > > Again, if this ever happens, you're screwed. That means potential DMA > engines could still be alive and about to scribble over memory that is > about to be freed which may cause all sorts of impossible-to-find bugs > in unrelated parts of the kernel. There is no point going on reporting > such error condition to upper layers until the software, or possibly the > hardware, is fixed Again, I think adding a WARN_ON() and letting hci_dma_dequeue_xfer() return an error code is a good compromise. > > > > +struct hci_xfer { > > > + u32 cmd_desc[4]; > > > + u32 response; > > > + bool rnw; > > > + void *data; > > > + u_int data_len; > > > + u_int cmd_tid; > > > + struct completion *completion; > > > + union { > > > + struct { > > > + /* PIO specific */ > > > + struct hci_xfer *next_xfer; > > > + struct hci_xfer *next_data; > > > + struct hci_xfer *next_resp; > > > + u_int data_left; > > > + u32 data_word_before_partial; > > > + }; > > > > I think anonymous unions are prohibited because the kernel is supposed > > to be built with old gcc versions which do not support it. > > Hmmm... let's see: > > According to Documentation/process/changes.rst the current minimal > required gcc version is 4.9. > > The oldest gcc I have lying around is 4.5.1 and it supports anonymous > unions just fine. > > So I think we're clear. We had problems with a buggy version of gcc 4.<something> at some point, and that was only when initializing anonymous unions, but I think it's all behind us now.
On Thu, 20 Aug 2020 12:47:49 -0400 (EDT) Nicolas Pitre <nico@fluxnic.net> wrote: > On Thu, 20 Aug 2020, Boris Brezillon wrote: > > > On Thu, 20 Aug 2020 10:08:29 +0200 > > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > + /* > > > > + * TODO: Extend the subsystem layer to allow for registering > > > > + * new device and provide BCR/DCR/PID at the same time. > > > > > > Not sure this is needed if you don't use it directly as the core will > > > anyway (in its current form) send the relevant CCC to read these > > > registers. > > > > We considered optimizing that in the past but that means making the DAA > > and SETDASA registration different. I'm not sure it's worth it to be > > honest, PID/DCR/BCR only happens when initializing devices and I > > suspect the overhead of querying those DATA twice in case of DAA is > > negligible anyway. > > Wellllll... I know some people who do feel strongly about this > particular issue for some reasons. Mind developing a bit why? Boot-time maybe? > So I'd prefer giving them some hope > and leave the door open to some i3c_master_add_i3c_dev_and_info() > interface. In the end, it's just a matter of pre-filling the info struct > and skipping the PID retrieval in i3c_master_getpid_locked() if already > available, etc. I'm definitely not closing the door, but I'd like to understand why this is so important to them :-). Anyway, if the changes are not invasive, I don't have a good reason to refuse it.
On Thu, 20 Aug 2020, Boris Brezillon wrote: > On Thu, 20 Aug 2020 12:34:13 -0400 (EDT) > Nicolas Pitre <nico@fluxnic.net> wrote: > > > On Thu, 20 Aug 2020, Miquel Raynal wrote: > > > > > + > > > > +#ccflags-y := -DDEBUG > > > > > > Probably a leftover? > > > > Well, I left it there intentionally as the code is still actively being > > developed, so full debugging can quickly be reactivated by anyone. > > I can remove it if deemed too distracting. > > How about using dynamic printk instead? I'm pretty sure you don't need > to debug I3C stuff early enough to warrant usage of DDEBUG. Actually I do have DDEBUG set all the time when testing this code. The entire log is captured into a file that I can search for issues when they come. I'm too lazy to bother about dynamic printk for now. That is nice when debugging a deployed solution where you don't want to skew runtime timings too much, but I'm not at the point of doing performance measurements yet. Anyway, this one has crossed the distraction threshold too at this point so I'll remove it. > > > > + BUG_ON(raw); > > > > > > It looks like 'raw' cannot be used with v1 (at least you seem to take > > > care of it in v2), so maybe BUG_ON is a bit radical here and you can > > > simply return an error? I think the use of BUG() is not appreciated in > > > general. > > > > That depends. Judgement is needed for BUG() usage. > > > > Here raw is absolutely impossible with v1 hardware and if ever this > > happens this is definitely a software bug that needs fixing right away. > > There is no point returning a runtime error code in that case as the > > upper layer won't know what to do about it. > > > > On the other hand, you absolutely don't want to BUG() on a condition > > that could _eventually_ happen at run time during normal usage. But > > that's not the case here. > > Well, people have tried to eradicate BUG() occurrences, so let's not add > new ones if we can avoid it. How about a WARN_ON()+error: > > if (WARN_ON(raw)) > return -EINVAL; In this case I can agree to that. Will do. However... > > > > + /* > > > > + * We're deep in it if ever this condition is ever met. > > > > + * Hardware might still be writing to memory, etc. > > > > + */ > > > > + ERR("unable to abort the ring"); > > > > + BUG(); > > > > > > Why not just treating the error as always? > > > > Again, if this ever happens, you're screwed. That means potential DMA > > engines could still be alive and about to scribble over memory that is > > about to be freed which may cause all sorts of impossible-to-find bugs > > in unrelated parts of the kernel. There is no point going on reporting > > such error condition to upper layers until the software, or possibly the > > hardware, is fixed > > Again, I think adding a WARN_ON() and letting hci_dma_dequeue_xfer() > return an error code is a good compromise. Here I disagree. You just can't return and let the system go any longer if some stray DMA is not stopped, period. No compromize is possible here. > > > > +const struct hci_cmd_ops i3c_hci_cmd_v1 = { > > > > + .prep_ccc = hci_cmd_v1_prep_ccc, > > > > + .prep_i3c_xfer = hci_cmd_v1_prep_i3c_xfer, > > > > + .prep_i2c_xfer = hci_cmd_v1_prep_i2c_xfer, > > > > + .perform_daa = hci_cmd_v1_daa, > > > > > > I know Boris does not like such space alignment :) > > > > Well... unfortunately for Boris, this is overwhelmingly prevalent in the > > kernel code: > > > > $ git grep "^"$'\t'"\.[^ ]*"$'\t'"*= " > > > > And I do like it. ;-P > > The rational being this preference is that sooner or later someone will > add a field to hci_cmd_ops that messes up your nice formatting :P. When/if that happens, it is not very difficult to realign the other assignments. I don't think it is likely that adding/removing fields here will ever become an area of conflicting patch contention. OTOH this makes for easier code reading whose occurence is more likely. > Anyway, that's definitely not a blocker. Good! > > > > +#if 0 > > > > + if (ccc->rnw) { > > > > + HEXDUMP("got: ", ccc->dests[0].payload.data, > > > > + ccc->dests[0].payload.len); > > > > + } > > > > +#endif > > > > > > I guess this debug block can be dropped too (there are many debug > > > information the should probably be dropped or turned into dev_info() > > > or similar). > > > > Again, hardware bringup from different vendors and other developments > > are still ongoing. I'd wish for those to stay for the time being unless > > people feel strongly enough about these to become a merge show stopper. > > Can't we replace that by a dev_dbg() using the %*pE formater? Oh nice! Didn't know about that. Looks like %*ph is what I want here. > > > > + if (rh->ibi_data_phys) > > > > > > I was told that _phys was a very bad suffix for something which is a > > > DMA address an not focibly a physical address. > > > > Fair enough. The HCI spec refers to these as "physical memory" hence the > > suffix. What were you told to use instead? > > Maybe _dma instead of _phys? No problem, will do. Nicolas
On Thu, 20 Aug 2020, Boris Brezillon wrote: > On Thu, 20 Aug 2020 12:47:49 -0400 (EDT) > Nicolas Pitre <nico@fluxnic.net> wrote: > > > On Thu, 20 Aug 2020, Boris Brezillon wrote: > > > > > On Thu, 20 Aug 2020 10:08:29 +0200 > > > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > > > + /* > > > > > + * TODO: Extend the subsystem layer to allow for registering > > > > > + * new device and provide BCR/DCR/PID at the same time. > > > > > > > > Not sure this is needed if you don't use it directly as the core will > > > > anyway (in its current form) send the relevant CCC to read these > > > > registers. > > > > > > We considered optimizing that in the past but that means making the DAA > > > and SETDASA registration different. I'm not sure it's worth it to be > > > honest, PID/DCR/BCR only happens when initializing devices and I > > > suspect the overhead of querying those DATA twice in case of DAA is > > > negligible anyway. > > > > Wellllll... I know some people who do feel strongly about this > > particular issue for some reasons. > > Mind developing a bit why? Boot-time maybe? Mind you, I'd prefer for those people to argue their use case themselves as I'm still not fully convinced myself. But yeah, this has something to do with latency. And v2 of the spec goes a step further by making the DAA procedure give you the PID/DCR/BCR of the winning device before you provide it the address to be assigned, so you could skip SETDASA/SETNEWDA altogether by giving it the final address up front. But in order for this to work the subsystem would have to provide a query method that would go like: here's some PID/DCR/BCR: please give me the best address to assign this device... oh and do so within a 150ms delay. > > So I'd prefer giving them some hope > > and leave the door open to some i3c_master_add_i3c_dev_and_info() > > interface. In the end, it's just a matter of pre-filling the info struct > > and skipping the PID retrieval in i3c_master_getpid_locked() if already > > available, etc. > > I'm definitely not closing the door, but I'd like to understand why this > is so important to them :-). Anyway, if the changes are not invasive, I > don't have a good reason to refuse it. Right. At least now you've been warned this might be coming. Nicolas