Linux-i3c Archive on lore.kernel.org
 help / color / Atom feed
From: Przemyslaw Gaj <pgaj@cadence.com>
To: <vitor.soares@synopsys.com>
Cc: linux-i3c@lists.infradead.org, bbrezillon@kernel.org,
	rafalc@cadence.com, vitor.soares@synopsys.com
Subject: Re: [PATCH v5 2/7] i3c: split i3c_master_register into init - register pair
Date: Mon, 2 Dec 2019 11:31:26 +0100
Message-ID: <20191202103124.GA27882@global.cadence.com> (raw)
In-Reply-To: <1561236905-8901-3-git-send-email-pgaj@cadence.com>

Hi Vitor,

I know you didn't like this solution to split i3c_master_register. Could
you look into that once again?

Without that, I'm not able to satisfy the use case when secondary master
is registered after ENEC(MR) received already. That time registration path
is little bit different. It may be the case that we don't receive next
ENEC(MR) and won't be able to register all the devices / complete bus
initialization.

As it's different, it's good practice to split that routine and let HC drivers
register it differently. Without that, i3c_master_register becomes huge
and not readable, takes too many responsibilities.

Anyway, I updated DW HC driver also in that patch. I remember you and
Boris mentioned I didn't do that.

Can we discuss that aproach?

The 06/22/2019 21:55, Przemyslaw Gaj wrote:
> This patch is base for mastership takeover where secondary master is
> initialized at probe time but register may be postponed till dynamic address is
> assigned to our device.
> 
> Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>
> ---
>  drivers/i3c/master.c                 | 86 ++++++++++++++++++++----------------
>  drivers/i3c/master/dw-i3c-master.c   | 34 +++++++-------
>  drivers/i3c/master/i3c-master-cdns.c | 45 ++++++++++---------
>  include/linux/i3c/master.h           | 12 ++---
>  4 files changed, 94 insertions(+), 83 deletions(-)
> 
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 0f7c31e..759078f 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -1528,32 +1528,9 @@ int i3c_master_do_daa(struct i3c_master_controller *master)
>  }
>  EXPORT_SYMBOL_GPL(i3c_master_do_daa);
>  
> -/**
> - * i3c_master_set_info() - set master device information
> - * @master: master used to send frames on the bus
> - * @info: I3C device information
> - *
> - * Set master device info. This should be called from
> - * &i3c_master_controller_ops->bus_init().
> - *
> - * Not all &i3c_device_info fields are meaningful for a master device.
> - * Here is a list of fields that should be properly filled:
> - *
> - * - &i3c_device_info->dyn_addr
> - * - &i3c_device_info->bcr
> - * - &i3c_device_info->dcr
> - * - &i3c_device_info->pid
> - * - &i3c_device_info->hdr_cap if %I3C_BCR_HDR_CAP bit is set in
> - *   &i3c_device_info->bcr
> - *
> - * This function must be called with the bus lock held in maintenance mode.
> - *
> - * Return: 0 if @info contains valid information (not every piece of
> - * information can be checked, but we can at least make sure @info->dyn_addr
> - * and @info->bcr are correct), -EINVAL otherwise.
> - */
> -int i3c_master_set_info(struct i3c_master_controller *master,
> -			const struct i3c_device_info *info)
> +static int i3c_master_set_info(struct i3c_master_controller *master,
> +			       const struct i3c_device_info *info,
> +			       bool secondary)
>  {
>  	struct i3c_dev_desc *i3cdev;
>  	int ret;
> @@ -1586,7 +1563,6 @@ int i3c_master_set_info(struct i3c_master_controller *master,
>  
>  	return ret;
>  }
> -EXPORT_SYMBOL_GPL(i3c_master_set_info);
>  
>  static void i3c_master_detach_free_devs(struct i3c_master_controller *master)
>  {
> @@ -2403,7 +2379,7 @@ static int i3c_master_check_ops(const struct i3c_master_controller_ops *ops)
>  }
>  
>  /**
> - * i3c_master_register() - register an I3C master
> + * i3c_master_init() - initializes all the structures required by I3C master
>   * @master: master used to send frames on the bus
>   * @parent: the parent device (the one that provides this I3C master
>   *	    controller)
> @@ -2417,16 +2393,14 @@ static int i3c_master_check_ops(const struct i3c_master_controller_ops *ops)
>   * - creates and initializes the I3C bus
>   * - populates the bus with static I2C devs if @parent->of_node is not
>   *   NULL
> - * - registers all I3C devices added by the controller during bus
> - *   initialization
> - * - registers the I2C adapter and all I2C devices
> + * - set bus mode when registering I2C devices.
>   *
>   * Return: 0 in case of success, a negative error code otherwise.
>   */
> -int i3c_master_register(struct i3c_master_controller *master,
> -			struct device *parent,
> -			const struct i3c_master_controller_ops *ops,
> -			bool secondary)
> +int i3c_master_init(struct i3c_master_controller *master,
> +		    struct device *parent,
> +		    const struct i3c_master_controller_ops *ops,
> +		    bool secondary)
>  {
>  	struct i3c_bus *i3cbus = i3c_master_get_bus(master);
>  	enum i3c_bus_mode mode = I3C_BUS_MODE_PURE;
> @@ -2488,10 +2462,47 @@ int i3c_master_register(struct i3c_master_controller *master,
>  		ret = -ENOMEM;
>  		goto err_put_dev;
>  	}
> +	return 0;
> +
> +err_put_dev:
> +	put_device(&master->dev);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_init);
> +
> +void i3c_master_cleanup(struct i3c_master_controller *master)
> +{
> +	put_device(&master->dev);
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_cleanup);
> +
> +/**
> + * i3c_master_register() - register an primary I3C master
> + * @master: master used to send frames on the bus
> + * @info: master info, describes this device
> + *
> + * This function takes care of everything for you:
> + *
> + * - updates this master info
> + * - registers all I3C devices added by the controller during bus
> + *   initialization
> + * - registers the I2C adapter and all I2C devices
> + *
> + * Return: 0 in case of success, a negative error code otherwise.
> + */
> +int i3c_master_register(struct i3c_master_controller *master,
> +			struct i3c_device_info *info)
> +{
> +	int ret;
> +
> +	ret = i3c_master_set_info(master, info, master->secondary);
> +	if (ret)
> +		return ret;
>  
>  	ret = i3c_master_bus_init(master);
>  	if (ret)
> -		goto err_put_dev;
> +		return ret;
>  
>  	ret = device_add(&master->dev);
>  	if (ret)
> @@ -2522,9 +2533,6 @@ int i3c_master_register(struct i3c_master_controller *master,
>  err_cleanup_bus:
>  	i3c_master_bus_cleanup(master);
>  
> -err_put_dev:
> -	put_device(&master->dev);
> -
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(i3c_master_register);
> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index 22ac305..8e91364 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
> @@ -593,7 +593,6 @@ static int dw_i3c_master_bus_init(struct i3c_master_controller *m)
>  {
>  	struct dw_i3c_master *master = to_dw_i3c_master(m);
>  	struct i3c_bus *bus = i3c_master_get_bus(m);
> -	struct i3c_device_info info = { };
>  	u32 thld_ctrl;
>  	int ret;
>  
> @@ -623,20 +622,6 @@ static int dw_i3c_master_bus_init(struct i3c_master_controller *m)
>  	writel(INTR_MASTER_MASK, master->regs + INTR_STATUS_EN);
>  	writel(INTR_MASTER_MASK, master->regs + INTR_SIGNAL_EN);
>  
> -	ret = i3c_master_get_free_addr(m, 0);
> -	if (ret < 0)
> -		return ret;
> -
> -	writel(DEV_ADDR_DYNAMIC_ADDR_VALID | DEV_ADDR_DYNAMIC(ret),
> -	       master->regs + DEVICE_ADDR);
> -
> -	memset(&info, 0, sizeof(info));
> -	info.dyn_addr = ret;
> -
> -	ret = i3c_master_set_info(&master->base, &info);
> -	if (ret)
> -		return ret;
> -
>  	writel(IBI_REQ_REJECT_ALL, master->regs + IBI_SIR_REQ_REJECT);
>  	writel(IBI_REQ_REJECT_ALL, master->regs + IBI_MR_REQ_REJECT);
>  
> @@ -1109,6 +1094,7 @@ static int dw_i3c_probe(struct platform_device *pdev)
>  {
>  	struct dw_i3c_master *master;
>  	struct resource *res;
> +	struct i3c_device_info info = { };
>  	int ret, irq;
>  
>  	master = devm_kzalloc(&pdev->dev, sizeof(*master), GFP_KERNEL);
> @@ -1160,8 +1146,22 @@ static int dw_i3c_probe(struct platform_device *pdev)
>  	master->maxdevs = ret >> 16;
>  	master->free_pos = GENMASK(master->maxdevs - 1, 0);
>  
> -	ret = i3c_master_register(&master->base, &pdev->dev,
> -				  &dw_mipi_i3c_ops, false);
> +	ret = i3c_master_init(&master->base, &pdev->dev,
> +			      &dw_mipi_i3c_ops, false);
> +	if (ret)
> +		goto err_assert_rst;
> +
> +	ret = i3c_master_get_free_addr(&master->base, 0);
> +	if (ret < 0)
> +		goto err_assert_rst;
> +
> +	writel(DEV_ADDR_DYNAMIC_ADDR_VALID | DEV_ADDR_DYNAMIC(ret),
> +	       master->regs + DEVICE_ADDR);
> +
> +	memset(&info, 0, sizeof(info));
> +	info.dyn_addr = ret;
> +
> +	ret = i3c_master_register(&master->base, &info);
>  	if (ret)
>  		goto err_assert_rst;
>  
> diff --git a/drivers/i3c/master/i3c-master-cdns.c b/drivers/i3c/master/i3c-master-cdns.c
> index 5aee315..9706426 100644
> --- a/drivers/i3c/master/i3c-master-cdns.c
> +++ b/drivers/i3c/master/i3c-master-cdns.c
> @@ -1193,8 +1193,7 @@ static int cdns_i3c_master_bus_init(struct i3c_master_controller *m)
>  	unsigned long pres_step, sysclk_rate, max_i2cfreq;
>  	struct i3c_bus *bus = i3c_master_get_bus(m);
>  	u32 ctrl, prescl0, prescl1, pres, low;
> -	struct i3c_device_info info = { };
> -	int ret, ncycles;
> +	int ncycles;
>  
>  	switch (bus->mode) {
>  	case I3C_BUS_MODE_PURE:
> @@ -1247,22 +1246,6 @@ static int cdns_i3c_master_bus_init(struct i3c_master_controller *m)
>  	prescl1 = PRESCL_CTRL1_OD_LOW(ncycles);
>  	writel(prescl1, master->regs + PRESCL_CTRL1);
>  
> -	/* Get an address for the master. */
> -	ret = i3c_master_get_free_addr(m, 0);
> -	if (ret < 0)
> -		return ret;
> -
> -	writel(prepare_rr0_dev_address(ret) | DEV_ID_RR0_IS_I3C,
> -	       master->regs + DEV_ID_RR0(0));
> -
> -	cdns_i3c_master_dev_rr_to_info(master, 0, &info);
> -	if (info.bcr & I3C_BCR_HDR_CAP)
> -		info.hdr_cap = I3C_CCC_HDR_MODE(I3C_HDR_DDR);
> -
> -	ret = i3c_master_set_info(&master->base, &info);
> -	if (ret)
> -		return ret;
> -
>  	/*
>  	 * Enable Hot-Join, and, when a Hot-Join request happens, disable all
>  	 * events coming from this device.
> @@ -1531,6 +1514,7 @@ static int cdns_i3c_master_probe(struct platform_device *pdev)
>  {
>  	struct cdns_i3c_master *master;
>  	struct resource *res;
> +	struct i3c_device_info info = { };
>  	int ret, irq;
>  	u32 val;
>  
> @@ -1606,13 +1590,32 @@ static int cdns_i3c_master_probe(struct platform_device *pdev)
>  	writel(MST_INT_IBIR_THR, master->regs + MST_IER);
>  	writel(DEVS_CTRL_DEV_CLR_ALL, master->regs + DEVS_CTRL);
>  
> -	ret = i3c_master_register(&master->base, &pdev->dev,
> -				  &cdns_i3c_master_ops, false);
> +	ret = i3c_master_init(&master->base, &pdev->dev, &cdns_i3c_master_ops, false);
>  	if (ret)
> -		goto err_disable_sysclk;
> +	goto err_disable_sysclk;
> +
> +	/* Get an address for the master. */
> +	ret = i3c_master_get_free_addr(&master->base, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	writel(prepare_rr0_dev_address(ret) | DEV_ID_RR0_IS_I3C,
> +	       master->regs + DEV_ID_RR0(0));
> +
> +	cdns_i3c_master_dev_rr_to_info(master, 0, &info);
> +	if (info.bcr & I3C_BCR_HDR_CAP)
> +		info.hdr_cap = I3C_CCC_HDR_MODE(I3C_HDR_DDR);
> +
> +	ret = i3c_master_register(&master->base, &info);
> +
> +	if (ret)
> +		goto err_cleanup;
>  
>  	return 0;
>  
> +err_cleanup:
> +	i3c_master_cleanup(&master->base);
> +
>  err_disable_sysclk:
>  	clk_disable_unprepare(master->sysclk);
>  
> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> index 42bb215..df3d769 100644
> --- a/include/linux/i3c/master.h
> +++ b/include/linux/i3c/master.h
> @@ -530,13 +530,13 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
>  				  u8 addr);
>  int i3c_master_do_daa(struct i3c_master_controller *master);
>  
> -int i3c_master_set_info(struct i3c_master_controller *master,
> -			const struct i3c_device_info *info);
> -
> +int i3c_master_init(struct i3c_master_controller *master,
> +		    struct device *parent,
> +		    const struct i3c_master_controller_ops *ops,
> +		    bool secondary);
> +void i3c_master_cleanup(struct i3c_master_controller *master);
>  int i3c_master_register(struct i3c_master_controller *master,
> -			struct device *parent,
> -			const struct i3c_master_controller_ops *ops,
> -			bool secondary);
> +			struct i3c_device_info *info);
>  int i3c_master_unregister(struct i3c_master_controller *master);
>  
>  /**
> -- 
> 2.4.5
> 

-- 
--
Regards,
Przemek

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

  parent reply index

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-22 20:54 [PATCH v5 0/7] Add the I3C mastership request Przemyslaw Gaj
2019-06-22 20:54 ` [PATCH v5 1/7] i3c: add addr and lvr to i2c_dev_desc structure Przemyslaw Gaj
2019-06-22 20:55 ` [PATCH v5 2/7] i3c: split i3c_master_register into init - register pair Przemyslaw Gaj
2019-07-06  8:48   ` Boris Brezillon
2019-12-02 10:31   ` Przemyslaw Gaj [this message]
2019-06-22 20:55 ` [PATCH v5 3/7] i3c: export i3c_bus_set_mode function Przemyslaw Gaj
2019-07-06  7:39   ` Boris Brezillon
2019-06-22 20:55 ` [PATCH v5 4/7] i3c: Add support for mastership request to I3C subsystem Przemyslaw Gaj
2019-07-06  9:00   ` Boris Brezillon
2019-07-10 18:04   ` Vitor Soares
2019-07-11  5:28     ` Przemyslaw Gaj
2019-07-11 10:11       ` Vitor Soares
2019-07-12 10:10         ` Przemyslaw Gaj
2019-07-12 11:28           ` Vitor Soares
2019-08-11 10:17             ` Boris Brezillon
2019-08-12 13:55               ` Vitor Soares
2019-08-12 14:55                 ` Boris Brezillon
2019-08-22 11:08                   ` Vitor Soares
2019-06-22 20:55 ` [PATCH v5 5/7] i3c: master: cdns: add support for mastership request to Cadence I3C master driver Przemyslaw Gaj
2019-06-22 20:55 ` [PATCH v5 6/7] i3c: master: Add module author Przemyslaw Gaj
2019-06-22 20:55 ` [PATCH v5 7/7] MAINTAINERS: add myself as co-maintainer of i3c subsystem Przemyslaw Gaj

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191202103124.GA27882@global.cadence.com \
    --to=pgaj@cadence.com \
    --cc=bbrezillon@kernel.org \
    --cc=linux-i3c@lists.infradead.org \
    --cc=rafalc@cadence.com \
    --cc=vitor.soares@synopsys.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-i3c Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-i3c/0 linux-i3c/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-i3c linux-i3c/ https://lore.kernel.org/linux-i3c \
		linux-i3c@lists.infradead.org
	public-inbox-index linux-i3c

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-i3c


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git