| Message ID | 20191115062326.16476-1-pgaj@cadence.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] i3c: master: cdns: add data hold delay support | expand |
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,
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, >
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,
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(-)