Re: [PATCH v2 2/2] i3c/master: add the mipi-i3c-hci driver

[Nicolas Pitre <nico@fluxnic.net>]

[-- Attachment #1: Type: text/plain, Size: 11146 bytes --]

On Thu, 1 Oct 2020, Sakari Ailus wrote:

> Hi Nicolas,

Hello Sakari,

Thanks for your review.

> Thanks for the patchset. It's a big patch... it might be easier to get
> reviews by splitting it. Feel free to do that if it's not too hard.

This is hard.  The only thing that I could split out without too much 
pain that still can be tested is the DMA support. And there is no 
bisectability advantage in doing so either.

> Please see my comments below.
> 
> On Tue, Aug 18, 2020 at 11:17:23PM -0400, Nicolas Pitre wrote:
> > From: Nicolas Pitre <npitre@baylibre.com>
> > +/* Data Transfer Speed and Mode */
> > +enum hci_cmd_mode {
> > +	MODE_I3C_SDR0		= 0x0,
> > +	MODE_I3C_SDR1		= 0x1,
> > +	MODE_I3C_SDR2		= 0x2,
> > +	MODE_I3C_SDR3		= 0x3,
> > +	MODE_I3C_SDR4		= 0x4,
> > +	MODE_I3C_HDR_TSx	= 0x5,
> > +	MODE_I3C_HDR_DDR	= 0x6,
> > +	MODE_I3C_HDR_BT		= 0x7,
> > +	MODE_I3C_Fm_FmP		= 0x8,
> > +	MODE_I2C_Fm		= 0x0,
> > +	MODE_I2C_FmP		= 0x1,
> > +	MODE_I2C_UD1		= 0x2,
> > +	MODE_I2C_UD2		= 0x3,
> > +	MODE_I2C_UD3		= 0x4,
> 
> I wonder if there's something else that separates I3C and I²C modes. Should
> the two be in a different enum?

There is only that I2C bit in the Device Address Table. Still, those 
mode values are stored in the same register field so there is no real 
advantage from splitting them.

> > +/* TID generation */
> > +#define hci_get_tid(bits) \
> > +	(atomic_inc_return_relaxed(&hci->next_cmd_tid) % (1 << (bits)))
> 
> 1U. And you don't need a modulo here, simple bitwise and is more efficient.

Good point about 1U. However the compiler is smart enough to convert the 
modulus into a bitwise "and" in the generated assembly.

> > +/* Our various instances */
> > +extern const struct hci_cmd_ops i3c_hci_cmd_v1;
> > +extern const struct hci_cmd_ops i3c_hci_cmd_v2;
> 
> As these are global symbols, I'd add a prefix such as "mipi_" to these.

Won't make a difference in the modular case... but fair enough.

> > +// SPDX-License-Identifier: BSD-3-Clause
> 
> Please read Documentation/process/license-rules.rst . IOW, BSD 3-clause
> license alone is not one of the acceptable licenses. I also don't see a
> need for dual licensing.

Really?

I did read Documentation/process/license-rules.rst obviously.

Let's have a look again. From that document:

|The licenses currently used, as well as the licenses for code added to the
|kernel, can be broken down into:
|
|1. _`Preferred licenses`:
|
|   Whenever possible these licenses should be used as they are known to be
|   fully compatible and widely used.  These licenses are available from the
|   directory::
|
|      LICENSES/preferred/
|
|   in the kernel source tree.

Incidentally, the file LICENSES/preferred/BSD-3-Clause can be found 
there. And it contains:

|  To use the BSD 3-clause "New" or "Revised" License put the following SPDX
|  tag/value pair into a comment according to the placement guidelines in
|  the licensing rules documentation:
|    SPDX-License-Identifier: BSD-3-Clause

There is indeed a mention in license-rules.rst that suggests: 
"individual files can be provided under a dual license, e.g. one of the 
compatible GPL variants and alternatively under a permissive license 
like BSD, MIT etc." It says *can* not *must*. In fact, there is a 
section explicitly for licenses that may only be used in a dual-license 
setup:

|3. Dual Licensing Only
|
|   These licenses should only be used to dual license code with another
|   license in addition to a preferred license.  These licenses are available
|   from the directory::
|
|      LICENSES/dual/
|
|   in the kernel source tree.

And no BSD license is to be found there.

If still in doubt, let's see what exists in practice:

$ git grep "SPDX-License-Identifier: BSD-3-Clause\($\| \*/\)" drivers/
drivers/crypto/talitos.h:/* SPDX-License-Identifier: BSD-3-Clause */
drivers/firmware/ti_sci.h:/* SPDX-License-Identifier: BSD-3-Clause */
drivers/net/dsa/sja1105/sja1105_clocking.c:// SPDX-License-Identifier: BSD-3-Clause
drivers/net/dsa/sja1105/sja1105_sgmii.h:/* SPDX-License-Identifier: BSD-3-Clause */
drivers/net/dsa/sja1105/sja1105_spi.c:// SPDX-License-Identifier: BSD-3-Clause
drivers/net/dsa/sja1105/sja1105_static_config.c:// SPDX-License-Identifier: BSD-3-Clause
drivers/net/dsa/sja1105/sja1105_static_config.h:/* SPDX-License-Identifier: BSD-3-Clause */
drivers/remoteproc/omap_remoteproc.h:/* SPDX-License-Identifier: BSD-3-Clause */
drivers/staging/greybus/audio_apbridgea.h:/* SPDX-License-Identifier: BSD-3-Clause */
drivers/staging/greybus/tools/lbtest:# SPDX-License-Identifier: BSD-3-Clause
drivers/staging/greybus/tools/loopback_test.c:// SPDX-License-Identifier: BSD-3-Clause
drivers/usb/serial/keyspan_usa26msg.h:/* SPDX-License-Identifier: BSD-3-Clause */
drivers/usb/serial/keyspan_usa28msg.h:/* SPDX-License-Identifier: BSD-3-Clause */
drivers/usb/serial/keyspan_usa49msg.h:/* SPDX-License-Identifier: BSD-3-Clause */
drivers/usb/serial/keyspan_usa67msg.h:/* SPDX-License-Identifier: BSD-3-Clause */
drivers/usb/serial/keyspan_usa90msg.h:/* SPDX-License-Identifier: BSD-3-Clause */

So there are couple precedents already.

> > +#define CMD_A0_TOC			           W0_BIT_(31)
> > +#define CMD_A0_ROC			           W0_BIT_(30)
> 
> Please use tabs for alignment.

Fixed.

> > +#define CMD_I1_DATA_BYTE_4(v)		FIELD_PREP(W1_MASK(63, 56), v)
> 
> I'm not sure W*_MASK() macros are helpful here. I'd just use plain
> FIELD_PREP() and BIT(). The digit after I in the macro name already signals
> the position.

Those macros are very helpful. The spec is written in terms of bit 
numbers from 0 through 127. Manually converting those into the 32-bit 
word that the hardware interface uses is error prone.

> > +#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)
> 
> Please align the macro expansions.

OK.

> > +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;
> 
> Please use unsigned int instead. Same elsewhere.

Why?

> > +	int mode = hci_get_i3c_mode(hci);
> 
> This returns an enum. Why int here?

Good point.

> > +	u8 *data = xfer->data;
> > +	u_int data_len = xfer->data_len;
> > +	bool rnw = xfer->rnw;
> > +	int ret;
> > +
> > +	BUG_ON(raw);
> 
> Uh-oh. I think you really should return an error code instead. Also
> consider WARN_ON().

As mentioned previously, this may happen only if the code is buggy and 
should be caught right away. But I'm gaving in on this one.

> > +	xfer->cmd_tid = hci_get_tid(4);
> 
> Why 4 and e.g. not 7? A macro with a descriptive name would be nice.

The macro is:

#define hci_get_tid(bits)

but yeah... it could be in the macro name as well.

In fact, there is no longer 3-bits TID fields. They're all 4-bits wide 
now. So I simply removed the macro argument instead.

> > +		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;
> > +		}
> 
> I see the same pattern being repeated elsewhere. Could you move it into a
> function and call that here?

Indeed, good point.

> > +		DBG("next_addr = 0x%02x, DAA using DAT %d", next_addr, dat_idx);
> 
> dev_dbg() perhaps? Same elsewhere.

Nah... Given all the needed arguments and the function name prefix I 
want, the dev_dbg() ended up spanning 3 lines whereas the DBG() wrapper 
takes only one in most cases.

> > +	}
> > +
> > +	if (dat_idx >= 0)
> > +		i3c_hci_dat_free_entry(hci, dat_idx);
> > +	hci_free_xfer(xfer, 1);
> 
> A newline before a lone return would be nice.
> 
> > +	return ret;

What do you mean? It is part of the cleanup and exit block which doesn't 
make it lone anyway.

> > +static int i3c_hci_bus_init(struct i3c_master_controller *m)
> > +{
> > +	struct i3c_hci *hci = to_i3c_hci(m);
> > +	struct i3c_device_info info;
> > +	int ret;
> > +
> > +	DBG("");
> 
> Really?

Sure!

> > +#if 0
> > +	if (ccc->rnw) {
> > +		HEXDUMP("got: ", ccc->dests[0].payload.data,
> > +				 ccc->dests[0].payload.len);
> > +	}
> 
> This should probably be removed.

Well, after Boris's suggestion, I turned it into this:

	if (ccc->rnw)
		DBG("got: %*ph",
			ccc->dests[0].payload.len, ccc->dests[0].payload.data);

But yes, this should go eventually. Not yet though.

> > +	size_limit = 1 << (16 + FIELD_GET(HC_CAP_MAX_DATA_LENGTH, hci->caps));
> 
> 1U

ACK.

> > +		xfer[i].rnw = (i2c_xfers[i].flags & I2C_M_RD);
> 
> No need for parentheses.

Right.

> > +	/* Validate HCI hardware version */
> > +	regval = reg_read(HCI_VERSION);
> > +	hci->version_major = (regval >> 8) & 0xf;
> > +	hci->version_minor = (regval >> 4) & 0xf;
> > +	hci->revision = regval & 0xf;
> > +	NOTE("HCI v%u.%u r%02u",
> > +	     hci->version_major, hci->version_minor, hci->revision);
> 
> dev_info()?

Same argument as for DBG().

> You might also want to say which HCI it is.

Good point.

> > +		ERR("unsupported HCI version");
> 
> dev_err().

Ditto.

> > +		return -EPROTONOSUPPORT;
> 
> I'd just use -EINVAL.

-EINVAL is widely used already. Let's try to report more precise errors 
when we can.

> > +int i3c_hci_dat_init(struct i3c_hci *hci)
> 
> Please add some descriptive prefix for the new global symbols.

Right. Moved those into object methods anyway.

> > +	for (dat_idx = find_first_bit(hci->DAT_data, hci->DAT_entries);
> > +	     dat_idx < hci->DAT_entries;
> > +	     dat_idx = find_next_bit(hci->DAT_data, hci->DAT_entries, dat_idx)) {
> > +		dat_w0 = dat_w0_read(dat_idx);
> > +		if (FIELD_GET(DAT_0_DYNAMIC_ADDRESS, dat_w0) == dev_addr)
> > +			return dat_idx;
> > +	}
> > +	/* what to do here? */
> > +	BUG();
> 
> Ouch. How about returning a negative error code?

Indeed.

> > + * Paul Kimelman's debug trace log facility.
> > + * This is completely vendor and hardware specific.
> 
> As useful this may have been for debugging while working with the driver
> implementation, I don't think it should be part of the MIPI I3C HCI driver.

Agreed. It won't be part of the next post.

> > +#define VENDOR_CAP_ENTRIES ARRAY_SIZE(vendor_ext_caps)
> 
> Please just use ARRAY_SIZE() where you need it.

OK.

> > +	if (pio->curr_tx &&
> > +	    pio->curr_tx->data_left != pio->curr_tx->data_len)
> 
> Fits on a single line.

Indeed.


Nicolas

[-- Attachment #2: Type: text/plain, Size: 111 bytes --]

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