[v2] i3c: master: cdns: add data hold delay support
diff mbox series

Message ID 20191115062326.16476-1-pgaj@cadence.com
State New
Headers show
Series
  • [v2] i3c: master: cdns: add data hold delay support
Related show

Commit Message

Przemysław Gaj Nov. 15, 2019, 6:23 a.m. UTC
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(-)

Comments

Boris Brezillon Nov. 15, 2019, 11:05 a.m. UTC | #1
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,
Przemysław Gaj Nov. 15, 2019, 11:50 a.m. UTC | #2
The 11/15/2019 12:05, Boris Brezillon wrote:
> 
> 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().
>

Yes, I can change it. I took this name directly from register map. As
rest of the names :-)
 
> >  #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).
> 

Ok. I'll think about that. It is possible to change frequency so maybe
it's good to calculate that instead of hardcoding that value.

> > +};
> > +
> >  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).
> 

Ok. Thank you.

> > +
> >  	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,
>

Patch
diff mbox series

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))
 #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;
+};
+
 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);
 	struct resource *res;
 	int ret, irq;
 	u32 val;
 
+	if (!of_id)
+		return -ENODEV;
+
 	master = devm_kzalloc(&pdev->dev, sizeof(*master), GFP_KERNEL);
 	if (!master)
 		return -ENOMEM;
 
+	master->devdata = of_id->data;
+
 	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,