* [PATCH v2] i3c: master: cdns: add data hold delay support
@ 2019-11-15 6:23 Przemyslaw Gaj
2019-11-15 11:05 ` Boris Brezillon
0 siblings, 1 reply; 3+ messages in thread
From: Przemyslaw Gaj @ 2019-11-15 6:23 UTC (permalink / raw)
To: bbrezillon; +Cc: linux-i3c, agolec, Przemyslaw Gaj, rafalc, vitor.soares
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))
#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,
--
2.14.0
_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v2] i3c: master: cdns: add data hold delay support
2019-11-15 6:23 [PATCH v2] i3c: master: cdns: add data hold delay support Przemyslaw Gaj
@ 2019-11-15 11:05 ` Boris Brezillon
2019-11-15 11:50 ` Przemyslaw Gaj
0 siblings, 1 reply; 3+ messages in thread
From: Boris Brezillon @ 2019-11-15 11:05 UTC (permalink / raw)
To: Przemyslaw Gaj; +Cc: linux-i3c, agolec, vitor.soares, rafalc, bbrezillon
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v2] i3c: master: cdns: add data hold delay support
2019-11-15 11:05 ` Boris Brezillon
@ 2019-11-15 11:50 ` Przemyslaw Gaj
0 siblings, 0 replies; 3+ messages in thread
From: Przemyslaw Gaj @ 2019-11-15 11:50 UTC (permalink / raw)
To: Boris Brezillon; +Cc: linux-i3c, agolec, vitor.soares, rafalc, bbrezillon
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,
>
--
--
Przemyslaw Gaj
_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-11-15 11:51 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-15 6:23 [PATCH v2] i3c: master: cdns: add data hold delay support Przemyslaw Gaj
2019-11-15 11:05 ` Boris Brezillon
2019-11-15 11:50 ` Przemyslaw Gaj
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).