Hi Boris and Vitor Find below my comment on DAA procedure. > -----Original Message----- > From: Vitor Soares [mailto:Vitor.Soares@xxxxxxxxxxxx] > Sent: Tuesday, February 27, 2018 5:04 PM > To: Boris Brezillon <boris.brezillon@xxxxxxxxxxx>; Vitor Soares > <Vitor.Soares@xxxxxxxxxxxx> > Cc: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>; Wolfram Sang > <wsa@xxxxxxxxxxxxx>; linux-i2c@xxxxxxxxxxxxxxx; Jonathan Corbet > <corbet@xxxxxxx>; linux-doc@xxxxxxxxxxxxxxx; Greg Kroah-Hartman > <gregkh@xxxxxxxxxxxxxxxxxxx>; Arnd Bergmann <arnd@xxxxxxxx>; > Przemyslaw Sroka <psroka@xxxxxxxxxxx>; Arkadiusz Golec > <agolec@xxxxxxxxxxx>; Alan Douglas <adouglas@xxxxxxxxxxx>; Bartosz > Folta <bfolta@xxxxxxxxxxx>; Damian Kos <dkos@xxxxxxxxxxx>; Alicja > Jurasik-Urbaniak <alicja@xxxxxxxxxxx>; Cyprian Wronka > <cwronka@xxxxxxxxxxx>; Suresh Punnoose <sureshp@xxxxxxxxxxx>; > Thomas Petazzoni <thomas.petazzoni@xxxxxxxxxxxxxxxxxx>; Nishanth > Menon <nm@xxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>; Pawel Moll > <pawel.moll@xxxxxxx>; Mark Rutland <mark.rutland@xxxxxxx>; Ian > Campbell <ijc+devicetree@xxxxxxxxxxxxxx>; Kumar Gala > <galak@xxxxxxxxxxxxxx>; devicetree@xxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>; Linus > Walleij <linus.walleij@xxxxxxxxxx> > Subject: Re: [PATCH v2 2/7] i3c: Add core I3C infrastructure > > EXTERNAL MAIL > > > Hi Boris > > > Às 8:36 PM de 2/26/2018, Boris Brezillon escreveu: > > Hi Vitor, > > > > On Mon, 26 Feb 2018 18:58:15 +0000 > > Vitor Soares <Vitor.Soares@xxxxxxxxxxxx> wrote: > > > >>>>> +/** > >>>>> + * i3c_device_do_priv_xfers() - do I3C SDR private transfers directed > to a > >>>>> + * specific device > >>>>> + * > >>>>> + * @dev: device with which the transfers should be done > >>>>> + * @xfers: array of transfers > >>>>> + * @nxfers: number of transfers > >>>>> + * > >>>>> + * Initiate one or several private SDR transfers with @dev. > >>>>> + * > >>>>> + * This function can sleep and thus cannot be called in atomic > context. > >>>>> + * > >>>>> + * Return: 0 in case of success, a negative error core otherwise. > >>>>> + */ > >>>>> +int i3c_device_do_priv_xfers(struct i3c_device *dev, > >>>>> + struct i3c_priv_xfer *xfers, > >>>>> + int nxfers) > >>>>> +{ > >>>>> + struct i3c_master_controller *master; > >>>>> + int i, ret; > >>>>> + > >>>>> + master = i3c_device_get_master(dev); > >>>>> + if (!master) > >>>>> + return -EINVAL; > >>>>> + > >>>>> + i3c_bus_normaluse_lock(master->bus); > >>>>> + for (i = 0; i < nxfers; i++) > >>>>> + xfers[i].addr = dev->info.dyn_addr; > >>>>> + > >>>>> + ret = i3c_master_do_priv_xfers_locked(master, xfers, > nxfers); > >>>>> + i3c_bus_normaluse_unlock(master->bus); > >>>>> + > >>>>> + return ret; > >>>>> +} > >>>>> +EXPORT_SYMBOL_GPL(i3c_device_do_priv_xfers); > >>>> The controller should know the speed mode for each xfer. The SDR0 > >>>> mode is used by default but if any device have read or write speed > >>>> limitations the controller can use SDRx. > >>> I might be wrong, but that's not my understanding of the spec. A > >>> device can express a speed limitation for SDR priv transfers, but > >>> this limitation applies to all SDR transfers. > >>> > >>> The speed R/W speed limitation is encoded in the device object, so, > >>> if the controller has to configure that on a per-transfer basis, one > >>> solution would be to pass the device to the ->priv_xfers(). > >> The speed R/W limitation is only for private transfers. Also the > >> device can have a limitation to write and not for read data. > >> This information is obtained with the command GETMXDS which returns > >> the Maximum Sustained Data Rate for non-CCC messages. > > And that's exactly what I expose in i3c_device_info, which is embedded > > in i3c_device, so you should have all the information you need to > > determine the speed in the controller driver if ->priv_xfer() is > > passed the device attached to those transfers. Would that be okay if > > we pass an i3c_device object to ->priv_xfers()? > > If you pass the i3c_device to ->priv_xfer(), then you won't need the address > too. > > Maybe someone else can give other point of view. > > >>> > >>>> This could be also applied to i2c transfers. > >>> Not really. The max SCL frequency is something that applies to the > >>> whole bus, because all I2C devices have to decode the address when > >>> messages are sent on the bus to determine if they should ignore or > >>> process the message. > >>> > >>>>> +#endif /* I3C_INTERNAL_H */ > >>>>> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c new file > >>>>> mode 100644 index 000000000000..1c85abac08d5 > >>>>> --- /dev/null > >>>>> +++ b/drivers/i3c/master.c > >>>>> @@ -0,0 +1,1433 @@ > >>>>> +// SPDX-License-Identifier: GPL-2.0 > >>>>> +/* > >>>>> + * Copyright (C) 2017 Cadence Design Systems Inc. > >>>>> + * > >>>>> + * Author: Boris Brezillon<boris.brezillon@xxxxxxxxxxxxxxxxxx> > >>>>> + */ > >>>>> + > >>>>> +#include <linux/slab.h> > >>>>> + > >>>>> +#include "internals.h" > >>>>> + > >>>>> +/** > >>>>> + * i3c_master_entdaa_locked() - start a DAA (Dynamic Address > Assignment) > >>>>> + * procedure > >>>>> + * @master: master used to send frames on the bus > >>>>> + * > >>>>> + * Send a ENTDAA CCC command to start a DAA procedure. > >>>>> + * > >>>>> + * Note that this function only sends the ENTDAA CCC command, all > >>>>> +the logic > >>>>> + * behind dynamic address assignment has to be handled in the I3C > >>>>> +master > >>>>> + * driver. > >>>>> + * > >>>>> + * This function must be called with the bus lock held in write > mode. > >>>>> + * > >>>>> + * Return: 0 in case of success, a negative error code otherwise. > >>>>> + */ > >>>>> +int i3c_master_entdaa_locked(struct i3c_master_controller > >>>>> +*master) { > >>>>> + struct i3c_ccc_cmd_dest dest = { }; > >>>>> + struct i3c_ccc_cmd cmd = { }; > >>>>> + int ret; > >>>>> + > >>>>> + dest.addr = I3C_BROADCAST_ADDR; > >>>>> + cmd.dests = &dest; > >>>>> + cmd.ndests = 1; > >>>>> + cmd.rnw = false; > >>>>> + cmd.id = I3C_CCC_ENTDAA; > >>>>> + > >>>>> + ret = i3c_master_send_ccc_cmd_locked(master, &cmd); > >>>>> + if (ret) > >>>>> + return ret; > >>>>> + > >>>>> + return 0; > >>>>> +} > >>>>> +EXPORT_SYMBOL_GPL(i3c_master_entdaa_locked); > >>>> can you explain the process? > >>> Not sure what you mean. The ENTDAA is just a CCC command that is > >>> used to trigger a DAA procedure. What the master controller does > >>> when it sends such a command is likely to be controller dependent, > >>> and it might even be possible that you don't need to call this > >>> function in your controller driver to trigger a DAA. If you want > >>> more details about the bus initialization steps and how the ENTDAA > >>> CCC command fits into it I recommend reading section "5.1.4 Bus > >>> Initialization and Dynamic Address Assignment Mode" > >>> > >>>> the command is only execute once, what if there is more devices on > >>>> the bus? > >>> Again, I'm not sure what you mean. The ENTDAA command is sent > every > >>> time a controller wants to discover new devices on the bus, that can > >>> be when initializing the bus, after a Hot Join event or simply > >>> triggered by the user (the last case is not supported yet though). > >>> > >>> Now, if you're interested in what happens after an ENTDAA CCC is > >>> sent, the controller will keep sending RepeatedStart until there's > >>> no more devices acking the request. You can have a look at "B.3 > >>> Error Types in Dynamic Address Arbitration" for more details. > >> My understanding is this command shall be executed once, this mean > >> that only one slave will assign the dynamic address (cmd.ndests = 1) > >> and not trigger the whole process of DAA. > > No, that's not what happens. The master controller is supposed to > > continue until no one replies with an Ack on the bus, and by continue > > I don't mean resend an ENTDAA, but issue a RepeatedStart followed by > > the broadcast address (0x7e). > > I am sorry, you have point here. I misunderstood that the cmd.ndests = 1 is > for broadcast commands. > > >> Either important is the SETDASA for declared I3C devices. So the DAA > >> process should start by send an SETDASA and them ENTDAA CCC > command. > > My understanding was that SETDASA was not mandatory, and was only > > useful when one wants to assign a specific dynamic address to a slave > > that has a static address (which is again not mandatory). > > I've tested it, and even devices with a static address participate to > > the DAA procedure if they've not been assigned a dynamic address yet, > > so I don't see the need for this SETDASA step if you don't need to > > assign a particular dynamic address to the device. > > > > Could you tell me why you think SETDASA is required? > > Yes, you are right... But in my opinion it is required as it does part of DAA > process. SETDASA is simply faster than ENTDAA, but only if there is no need to collect BCR/DCR/PID of such devices. I think most applications would like to have them as an status information so after all ENTDAA can be regarded as an generic approach (unless I'm mistaken). > > >>>>> +static u32 i3c_master_i2c_functionalities(struct i2c_adapter > >>>>> +*adap) { > >>>>> + return I2C_FUNC_SMBUS_EMUL | I2C_FUNC_I2C | > I2C_FUNC_10BIT_ADDR; > >>>>> +} > >>>> Is I2C_FUNC_10BIT_ADDR allowed ? > >>> According to "Table 4 I 2 C Features Allowed in I3C Slaves", yes (at > >>> least that my understanding). And the Cadence controller supports it. > >> The table say the oposite. The I2C extended address feature is not > >> used on I3C bus, thus this feature shall be disable. > > Actually, I was wrong when initially mentioning this table: it's about > > I2C features supported on I3C slaves, so not really what we're looking > > for. Here, we're wondering if I2C-only devices can have 10-bit > > addresses. The Cadence controller supports that, so there's probably > > nothing preventing use of 10-bit addresses for I2C transfers, but > > maybe not all I3C master controllers support that, so we should > > probably let the I3C master driver implement this method. > > The spec says that is "not used" so it will not interface with I3C bus. > > >> BTW it is optional on I2C devices. > > You mean I2C controllers? When an I2C device has a 10bit address, the > > controller has to support this mode to communicate with the device, at > > least that's my understanding. But we're digressing a bit. The > > question is not whether I2C devices can optionally use a 10 bit > > address, but whether I3C master controller can support this mode for > > I2C transfers to I2C-only devices. > > By the i2c spec the 10-bit address is optional, however the 7-bit address is > mandatory. > > >>> > >>>> > diff --git a/drivers/i3c/master/Kconfig > >>>> b/drivers/i3c/master/Kconfig > >>>>> new file mode 100644 > >>>>> index 000000000000..e69de29bb2d1 > >>>>> diff --git a/drivers/i3c/master/Makefile > >>>>> b/drivers/i3c/master/Makefile new file mode 100644 index > >>>>> 000000000000..e69de29bb2d1 diff --git a/include/linux/i3c/ccc.h > >>>>> b/include/linux/i3c/ccc.h new file mode 100644 index > >>>>> 000000000000..ff3e1a3e2c4c > >>>>> --- /dev/null > >>>>> +++ b/include/linux/i3c/ccc.h > >>>>> @@ -0,0 +1,380 @@ > >>>>> +/* SPDX-License-Identifier: GPL-2.0 */ > >>>>> +/* > >>>>> + * Copyright (C) 2017 Cadence Design Systems Inc. > >>>>> + * > >>>>> + * Author: Boris Brezillon<boris.brezillon@xxxxxxxxxxxxxxxxxx> > >>>>> + */ > >>>>> + > >>>>> + > >>>>> +/** > >>>>> + * enum i3c_ccc_test_mode - enum listing all available test modes > >>>>> + * > >>>>> + * @I3C_CCC_EXIT_TEST_MODE: exit test mode > >>>>> + * @I3C_CCC_VENDOR_TEST_MODE: enter vendor test mode */ > enum > >>>>> +i3c_ccc_test_mode { > >>>>> + I3C_CCC_EXIT_TEST_MODE, > >>>>> + I3C_CCC_VENDOR_TEST_MODE, > >>>>> +}; > >>>>> + > >>>>> +/** > >>>>> + * struct i3c_ccc_enttm - payload passed to ENTTM CCC > >>>>> + * > >>>>> + * @mode: one of the &enum i3c_ccc_test_mode modes > >>>>> + * > >>>>> + * Information passed to the ENTTM CCC to instruct an I3C device > >>>>> +to enter a > >>>>> + * specific test mode. > >>>>> + */ > >>>>> +struct i3c_ccc_enttm { > >>>>> + u8 mode; > >>>>> +} __packed; > >>>>> + > >>>>> +/** > >>>>> + * struct i3c_ccc_setda - payload passed to ENTTM CCC > >>>>> + * > >>>>> + * @mode: one of the &enum i3c_ccc_test_mode modes > >>>>> + * > >>>>> + * Information passed to the ENTTM CCC to instruct an I3C device > >>>>> +to enter a > >>>>> + * specific test mode. > >>>>> + */ > >>>>> +struct i3c_ccc_setda { > >>>>> + u8 addr; > >>>>> +} __packed; > >>>> what do you mean with struct? Maybe setdasa? if so, what is the > addr? > >> Do you have the function to use this structure? Because one command > >> use the static address and the other use the dynamic address. > >> > >>>> > >>>> > +/** > >>>>> + * enum i3c_sdr_max_data_rate - max data rate values for private > >>>>> +SDR transfers */ enum i3c_sdr_max_data_rate { > >>>>> + I3C_SDR_DR_FSCL_MAX, > >>>>> + I3C_SDR_DR_FSCL_8MHZ, > >>>>> + I3C_SDR_DR_FSCL_6MHZ, > >>>>> + I3C_SDR_DR_FSCL_4MHZ, > >>>>> + I3C_SDR_DR_FSCL_2MHZ, > >>>>> +}; > >>>> Can you change the names to: > >>>> > >>>> I3C_SDR0_FSCL_MAX, > >>>> I3C_SDR1_FSCL_8MHZ, > >>>> I3C_SDR2_FSCL_6MHZ, > >>>> I3C_SDR3_FSCL_4MHZ, > >>>> I3C_SDR4_FSCL_2MHZ, > >>>> > >>>> thus the data rate isn't repeated. > >>> What's the problem with the name I use? Moreover, I see no mention > >>> to the SDR0,1,2,3,4 modes in the public spec. > >> When you get the GETMXDS information, the maxWr and maxRd came > from 0 > >> to 4, so in my opinion I think in this way is easier to have a relationship. > > It's an emum, and there's no mention of the SDRX modes you're using > > here in the I3C spec. I guess it's named this way in your I3C > > controller datasheet. I personally don't see a good reason to add SDRX > > in the name, but if you insist... > > The idea is to say that it is SDR mode and the value that GETMXDS > command return. It is what makes sense to me. > > >>>>> + > >>>>> +#endif /* I3C_CCC_H */ > >>>>> diff --git a/include/linux/i3c/device.h > >>>>> b/include/linux/i3c/device.h new file mode 100644 index > >>>>> 000000000000..83958d3a02e2 > >>>>> --- /dev/null > >>>>> +++ b/include/linux/i3c/device.h > >>>>> @@ -0,0 +1,321 @@ > >>>>> +/* SPDX-License-Identifier: GPL-2.0 */ > >>>>> +/* > >>>>> + * Copyright (C) 2017 Cadence Design Systems Inc. > >>>>> + * > >>>>> + * Author: Boris Brezillon<boris.brezillon@xxxxxxxxxxxxxxxxxx> > >>>>> + */ > >>>>> + > >>>>> +#ifndef I3C_DEV_H > >>>>> +#define I3C_DEV_H > >>>>> + > >>>>> +#include <linux/device.h> > >>>>> +#include <linux/i2c.h> > >>>>> +#include <linux/mod_devicetable.h> #include <linux/module.h> > >>>>> + > >>>>> +/** > >>>>> + * enum i3c_hdr_mode - HDR mode ids > >>>>> + * @I3C_HDR_DDR: DDR mode > >>>>> + * @I3C_HDR_TSP: TSP mode > >>>>> + * @I3C_HDR_TSL: TSL mode > >>>>> + */ > >>>>> +enum i3c_hdr_mode { > >>>>> + I3C_HDR_DDR, > >>>>> + I3C_HDR_TSP, > >>>>> + I3C_HDR_TSL, > >>>>> +}; > >>>>> + > >>>>> +/** > >>>>> + * struct i3c_hdr_cmd - I3C HDR command > >>>>> + * @mode: HDR mode selected for this command > >>>>> + * @code: command opcode > >>>>> + * @addr: I3C dynamic address > >>>>> + * @ndatawords: number of data words (a word is 16bits wide) > >>>>> + * @data: input/output buffer > >>>>> + */ > >>>>> +struct i3c_hdr_cmd { > >>>>> + enum i3c_hdr_mode mode; > >>>>> + u8 code; > >>>>> + u8 addr; > >>>>> + int ndatawords; > >>>>> + union { > >>>>> + u16 *in; > >>>>> + const u16 *out; > >>>>> + } data; > >>>>> +}; > >> Please mention that the @code is what will define if the transfer is read > or write. > > Well, I think it's pretty clear in the definition you'll find in the > > ccc.h file, but I can add a comment here too if you like. > > I don't find it in ccc.h file. Anyway is not good to put this definition there > because is not related. > > >>>>> + > >>>>> +/* Private SDR read transfer */ > >>>>> +#define I3C_PRIV_XFER_READ BIT(0) > >>>>> +/* > >>>>> + * Instruct the controller to issue a STOP after a specific > >>>>> +transfer instead > >>>>> + * of a REPEATED START. > >>>>> + */ > >>>>> +#define I3C_PRIV_XFER_STOP BIT(1) > >>>>> + > >>>>> +/** > >>>>> + * struct i3c_priv_xfer - I3C SDR private transfer > >>>>> + * @addr: I3C dynamic address > >>>>> + * @len: transfer length in bytes of the transfer > >>>>> + * @flags: combination of I3C_PRIV_XFER_xxx flags > >>>>> + * @data: input/output buffer > >>>>> + */ > >>>>> +struct i3c_priv_xfer { > >>>>> + u8 addr; > >>>>> + u16 len; > >>>>> + u32 flags; > >>>>> + struct { > >>>>> + void *in; > >>>>> + const void *out; > >>>>> + } data; > >>>>> +}; > >>>> Same as above, i3c_sdr_max_data_rate to change the bus scl. > >>> If I'm understanding the spec correctly, that's not something you > >>> want to change on a per-transfer basis. The constraint is on the > >>> device itself and should IMO not be part of the i3c_priv_xfer struct. > >> As mention before this is important. > >> You can do the same as for struct i3c_hdr_cmd and add a enum > i3c_sdr_max_data_rate. > >> > >> The @flag only have 2 bits of load, is the rest opened? > > If by open you mean that we can add more flags if we need to, then yes. > > > >>> > >>>> > + > >>>>> +/** > >>>>> + * enum i3c_dcr - I3C DCR values > >>>>> + * @I3C_DCR_GENERIC_DEVICE: generic I3C device */ enum i3c_dcr > { > >>>>> + I3C_DCR_GENERIC_DEVICE = 0, > >>>>> +}; > >>>>> + > >>>>> +#define I3C_PID_MANUF_ID(pid) (((pid) & > GENMASK_ULL(47, 33)) >> 33) > >>>>> +#define I3C_PID_RND_LOWER_32BITS(pid) (!!((pid) & > BIT_ULL(32))) > >>>>> +#define I3C_PID_RND_VAL(pid) ((pid) & GENMASK_ULL(31, > 0)) > >>>>> +#define I3C_PID_PART_ID(pid) (((pid) & GENMASK_ULL(31, > 16)) >> 16) > >>>>> +#define I3C_PID_INSTANCE_ID(pid) (((pid) & GENMASK_ULL(15, > 12)) >> 12) > >>>>> +#define I3C_PID_EXTRA_INFO(pid) ((pid) & > GENMASK_ULL(11, 0)) > >>>>> + > >>>>> +#define I3C_BCR_DEVICE_ROLE(bcr) ((bcr) & GENMASK(7, 6)) > >>>>> +#define I3C_BCR_I3C_SLAVE (0 << 6) > >>>>> +#define I3C_BCR_I3C_MASTER (1 << 6) > >>>>> +#define I3C_BCR_HDR_CAP BIT(5) > >>>>> +#define I3C_BCR_BRIDGE BIT(4) > >>>>> +#define I3C_BCR_OFFLINE_CAP BIT(3) > >>>>> +#define I3C_BCR_IBI_PAYLOAD BIT(2) > >>>>> +#define I3C_BCR_IBI_REQ_CAP BIT(1) > >>>>> +#define I3C_BCR_MAX_DATA_SPEED_LIM BIT(0) > >>>>> + > >>>>> +/** > >>>>> + * struct i3c_device_info - I3C device information > >>>>> + * @pid: Provisional ID > >>>>> + * @bcr: Bus Characteristic Register > >>>>> + * @dcr: Device Characteristic Register > >>>>> + * @static_addr: static/I2C address > >>>>> + * @dyn_addr: dynamic address > >>>>> + * @hdr_cap: supported HDR modes > >>>>> + * @max_read_ds: max read speed information > >>>>> + * @max_write_ds: max write speed information > >>>>> + * @max_ibi_len: max IBI payload length > >>>>> + * @max_read_turnaround: max read turn-around time in > >>>>> +micro-seconds > >>>>> + * @max_read_len: max private SDR read length in bytes > >>>>> + * @max_write_len: max private SDR write length in bytes > >>>>> + * > >>>>> + * These are all basic information that should be advertised by an > I3C device. > >>>>> + * Some of them are optional depending on the device type and > >>>>> +device > >>>>> + * capabilities. > >>>>> + * For each I3C slave attached to a master with > >>>>> + * i3c_master_add_i3c_dev_locked(), the core will send the > >>>>> +relevant CCC command > >>>>> + * to retrieve these data. > >>>>> + */ > >>>>> +struct i3c_device_info { > >>>>> + u64 pid; > >>>>> + u8 bcr; > >>>>> + u8 dcr; > >>>>> + u8 static_addr; > >>>>> + u8 dyn_addr; > >>>>> + u8 hdr_cap; > >>>>> + u8 max_read_ds; > >>>>> + u8 max_write_ds; > >>>>> + u8 max_ibi_len; > >>>>> + u32 max_read_turnaround; > >>>>> + u16 max_read_len; > >>>>> + u16 max_write_len; > >>>>> +}; > >>>>> + > >>>> is this information filled with data provided from CCC commands? > >>> Yes, they are. > >> Ok, them the intention is to do this on bus_init(), right? > > Not only, it can be after a Hot-Join, or after the user has triggered > > a new DAA. Anyway, these information are retrieved anytime you add a > > device with i3c_master_add_i3c_dev_locked(), and the core uses CCC > > commands to do that. > > > >>>>> + > >>>>> +/** > >>>>> + * struct i3c_master_controller_ops - I3C master methods > >>>>> + * @bus_init: hook responsible for the I3C bus initialization. This > >>>>> + * initialization should follow the steps described in the I3C > >>>>> + * specification. This hook is called with the bus lock held > in > >>>>> + * write mode, which means all _locked() helpers can safely > be > >>>>> + * called from there > >>>>> + * @bus_cleanup: cleanup everything done in > >>>>> + * &i3c_master_controller_ops->bus_init(). This > function is > >>>>> + * optional and should only be implemented if > >>>>> + * &i3c_master_controller_ops->bus_init() attached > private data > >>>>> + * to I3C/I2C devices. This hook is called with the bus > lock > >>>>> + * held in write mode, which means all _locked() > helpers can > >>>>> + * safely be called from there > >>>>> + * @supports_ccc_cmd: should return true if the CCC command is > supported, false > >>>>> + * otherwise > >>>>> + * @send_ccc_cmd: send a CCC command > >>>>> + * @send_hdr_cmds: send one or several HDR commands. If there is > more than one > >>>>> + * command, they should ideally be sent in the same > HDR > >>>>> + * transaction > >>>>> + * @priv_xfers: do one or several private I3C SDR transfers > >>>>> + * @i2c_xfers: do one or several I2C transfers > >>>>> + * @request_ibi: attach an IBI handler to an I3C device. This implies > defining > >>>>> + * an IBI handler and the constraints of the IBI > (maximum payload > >>>>> + * length and number of pre-allocated slots). > >>>>> + * Some controllers support less IBI-capable devices > than regular > >>>>> + * devices, so this method might return -%EBUSY if > there's no > >>>>> + * more space for an extra IBI registration > >>>>> + * @free_ibi: free an IBI previously requested with ->request_ibi(). > The IBI > >>>>> + * should have been disabled with ->disable_irq() prior to > that > >>>>> + * @enable_ibi: enable the IBI. Only valid if ->request_ibi() has been > called > >>>>> + * prior to ->enable_ibi(). The controller should first > enable > >>>>> + * the IBI on the controller end (for example, unmask > the hardware > >>>>> + * IRQ) and then send the ENEC CCC command (with > the IBI flag set) > >>>>> + * to the I3C device > >>>>> + * @disable_ibi: disable an IBI. First send the DISEC CCC command > with the IBI > >>>>> + * flag set and then deactivate the hardware IRQ on > the > >>>>> + * controller end > >>>>> + * @recycle_ibi_slot: recycle an IBI slot. Called every time an IBI has > been > >>>>> + * processed by its handler. The IBI slot should be > put back > >>>>> + * in the IBI slot pool so that the controller can re- > use it > >>>>> + * for a future IBI > >>>>> + * > >>>>> + * One of the most important hooks in these ops is > >>>>> + * &i3c_master_controller_ops->bus_init(). Here is a > >>>>> +non-exhaustive list of > >>>>> + * things that should be done in &i3c_master_controller_ops- > >bus_init(): > >>>>> + * > >>>>> + * 1) call i3c_master_set_info() with all information describing > >>>>> +the master > >>>>> + * 2) ask all slaves to drop their dynamic address by sending the > RSTDAA CCC > >>>>> + * with i3c_master_rstdaa_locked() > >>>>> + * 3) ask all slaves to disable IBIs using > >>>>> +i3c_master_disec_locked() > >>>>> + * 4) start a DDA procedure by sending the ENTDAA CCC with > >>>>> + * i3c_master_entdaa_locked(), or using the internal DAA logic > provided by > >>>>> + * your controller > >>>> You mean SETDASA CCC command? > >>> No, I really mean ENTDAA and DAA. By internal DAA logic I mean that > >>> some controllers are probably automating the whole DAA procedure, > >>> while others may let the SW control every step. > >> My understanding is that i3c_master_entdaa_locked() will trigger the > >> DAA process and DAA can be done by SETDASA, ENTDAA and later after > >> the bus initialization with SETNEWDA. > > No. Only ENTDAA can trigger a DAA procedure. SETDASA is here to assign > > a single dynamic address to a device that already has a static address > > but no dynamic address yet, and SETNEWDA is here to modify the > dynamic > > address of a device that already has one. > > >> I think the DAA process should be more generic, right now is only > >> made through the ENTDAA command with (cmd.ndests = 1). > >> I mean, shouldn't this be made by the core? First doing DAA for the > >> devices declared and them try do discover the rest of devices on the bus. > > Can you detail a bit more? If the only part you're complaining about > > is pre-assignment of dynamic addresses with SETDASA when a device is > > declared in the DT with a reg and dynamic-address property, then yes, > > I think I can provide an helper for that. But this helper would still > > have to be called from the master controller driver (from ->bus_init() > > or after a Hot-Join). > > > > Now, if the question is, is there a way we can automate things even > > more and completely implement DAA from the core? I doubt it, because > > the way the core will trigger DAA, expose discovered devices or allow > > you to declare manually assigned addresses is likely to be > > controller-dependent. > > Please refer to figure 90 of public specification. As you can see the DAA > process should start with SETDASA command. > > With the current flow of this patch the DAA process is limited to ENTDAA > command only. > > > When I designed the framework I took the decision to base my work on > > the spec rather than focusing on the I3C master controller I had to > > support (Cadence). This is the reason I decided to keep the interface > > as simple as possible at the risk of encouraging code-duplication (at > > first) rather than coming up with an interface that is designed with a > > single controller in mind and having to break things every time a new > > controller comes out. > > > > Thank you for you comments, but I'd like to know if some of my design > > choices are blocking you to support your controller. What I've seen so > > far is a collection of things that might be relevant to fix (though > > most of them are subject to interpretation and/or a matter of taste), > > but nothing that should really block you. > > > > Can you clarify that, and maybe come back with a list of things that > > you think are preventing you from properly supporting the Synopsys > > controller? > > > > Thanks, > > > > Boris > > As you can check from my comments my concerns are about the i3c > specification without the controller in mind. > > Best regards, > Vitor Soares >
