Re: [PATCH v2] i3c: master: cdns: add data hold delay support

[Boris Brezillon <boris.brezillon@collabora.com>]

On Fri, 15 Nov 2019 07:23:26 +0100
Przemyslaw Gaj <pgaj@cadence.com> wrote:

> This patch adds support for THD_DEL (Data Hold Delay) to Cadence
> I3C master constoller driver.
> 
> As per MIPI I3C Specification 1.0, Table 75 (page 142) defines
> non-zero minimal tHD_PP timing on master output (Fig 65). This
> setting allows to meet this timing on master's soc outputs,
> regardless of PCB balancing.
> 
> Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com>
> 
> ---
> 
> Main changes between v1 and v2:
> - Add device-specific data which holds the thd_del value
> - Remove device tree property
> ---
>  drivers/i3c/master/i3c-master-cdns.c | 37 +++++++++++++++++++++++++++++++-----
>  1 file changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i3c/master/i3c-master-cdns.c b/drivers/i3c/master/i3c-master-cdns.c
> index 10db0bf0655a..3531396f04fd 100644
> --- a/drivers/i3c/master/i3c-master-cdns.c
> +++ b/drivers/i3c/master/i3c-master-cdns.c
> @@ -22,6 +22,7 @@
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
>  #include <linux/workqueue.h>
> +#include <linux/of_device.h>
>  
>  #define DEV_ID				0x0
>  #define DEV_ID_I3C_MASTER		0x5034
> @@ -60,6 +61,7 @@
>  #define CTRL_HALT_EN			BIT(30)
>  #define CTRL_MCS			BIT(29)
>  #define CTRL_MCS_EN			BIT(28)
> +#define CTRL_THD_DEL(x)			(((x) << 24) & GENMASK(25, 24))

DEL is a bit ambiguous, how about _DELAY().

>  #define CTRL_HJ_DISEC			BIT(8)
>  #define CTRL_MST_ACK			BIT(7)
>  #define CTRL_HJ_ACK			BIT(6)
> @@ -388,6 +390,10 @@ struct cdns_i3c_xfer {
>  	struct cdns_i3c_cmd cmds[0];
>  };
>  
> +struct cdns_i3c_data {
> +	u8 thd_del;

Same here, thd_delay. BTW, is the clk driving the I3C master clk
expected to be fixed? If not, I'd suggest expressing this delay is
seconds (nano or micro depending on the precision you need) so the
driver can calculate the proper thd_delay value based on
clk_get_rate(master->sysclk).

> +};
> +
>  struct cdns_i3c_master {
>  	struct work_struct hj_work;
>  	struct i3c_master_controller base;
> @@ -408,6 +414,7 @@ struct cdns_i3c_master {
>  	struct clk *pclk;
>  	struct cdns_i3c_master_caps caps;
>  	unsigned long i3c_scl_lim;
> +	const struct cdns_i3c_data *devdata;
>  };
>  
>  static inline struct cdns_i3c_master *
> @@ -1264,6 +1271,15 @@ static int cdns_i3c_master_bus_init(struct i3c_master_controller *m)
>  	 * We will issue ENTDAA afterwards from the threaded IRQ handler.
>  	 */
>  	ctrl |= CTRL_HJ_ACK | CTRL_HJ_DISEC | CTRL_HALT_EN | CTRL_MCS_EN;
> +
> +	/*
> +	 * Configure data hold delay based on device-specific data.
> +	 *
> +	 * MIPI I3C Specification 1.0 defines non-zero minimal tHD_PP timing on
> +	 * master output. This setting allows to meet this timing on master's
> +	 * SoC outputs, regardless of PCB balancing.
> +	 */
> +	ctrl |= CTRL_THD_DEL(master->devdata->thd_del);
>  	writel(ctrl, master->regs + CTRL);
>  
>  	cdns_i3c_master_enable(master);
> @@ -1521,17 +1537,33 @@ static void cdns_i3c_master_hj(struct work_struct *work)
>  	i3c_master_do_daa(&master->base);
>  }
>  
> +static struct cdns_i3c_data cdns_i3c_devdata = {
> +	.thd_del = 0x3,
> +};
> +
> +static const struct of_device_id cdns_i3c_master_of_ids[] = {
> +	{ .compatible = "cdns,i3c-master", .data = &cdns_i3c_devdata },
> +	{ /* sentinel */ },
> +};
> +
>  static int cdns_i3c_master_probe(struct platform_device *pdev)
>  {
>  	struct cdns_i3c_master *master;
> +	const struct of_device_id *of_id = of_match_device(
> +		of_match_ptr(cdns_i3c_master_of_ids), &pdev->dev);

The of_match_ptr() is not needed since cdns_i3c_master_of_ids is always
defined.


>  	struct resource *res;
>  	int ret, irq;
>  	u32 val;
>  
> +	if (!of_id)
> +		return -ENODEV;
> +

Not sure ENODEV is the right error code here. If of_id == NULL that
means cdns_i3c_master_of_ids[] is not filled correctly, so I'd suggest
returning EINVAL.

>  	master = devm_kzalloc(&pdev->dev, sizeof(*master), GFP_KERNEL);
>  	if (!master)
>  		return -ENOMEM;
>  
> +	master->devdata = of_id->data;

If you do:

	master->devdata = of_device_get_match_data(&pdev->dev);
	if (!master->devdata)
		return -EINVAL;

You can get rid of of_id (and the associate assignment/test).

> +
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	master->regs = devm_ioremap_resource(&pdev->dev, res);
>  	if (IS_ERR(master->regs))
> @@ -1631,11 +1663,6 @@ static int cdns_i3c_master_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -static const struct of_device_id cdns_i3c_master_of_ids[] = {
> -	{ .compatible = "cdns,i3c-master" },
> -	{ /* sentinel */ },
> -};
> -
>  static struct platform_driver cdns_i3c_master = {
>  	.probe = cdns_i3c_master_probe,
>  	.remove = cdns_i3c_master_remove,


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