| Message ID | 20191018153437.20614-1-bparrot@ti.com |
|---|---|
| Headers | show |
| Series | media: ti-vpe: cal: maintenance | expand |
On 10/18/19 5:34 PM, Benoit Parrot wrote: > Apply Errata i913 every time the functional clock is enabled. > This should take care of suspend/resume case as well. > > Signed-off-by: Benoit Parrot <bparrot@ti.com> > Signed-off-by: Jyri Sarha <jsarha@ti.com> > --- > drivers/media/platform/ti-vpe/cal.c | 56 +++++++++++++++++++++++- > drivers/media/platform/ti-vpe/cal_regs.h | 27 ++++++++++++ > 2 files changed, 82 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c > index 62aeedb705e9..3cbc4dca6de8 100644 > --- a/drivers/media/platform/ti-vpe/cal.c > +++ b/drivers/media/platform/ti-vpe/cal.c > @@ -284,6 +284,13 @@ static struct cal_data dra72x_cal_data = { > .flags = 0, > }; > > +static struct cal_data dra72x_es1_cal_data = { > + .csi2_phy_core = dra72x_cal_csi_phy, > + .num_csi2_phy = ARRAY_SIZE(dra72x_cal_csi_phy), > + > + .flags = DRA72_CAL_PRE_ES2_LDO_DISABLE, > +}; > + > /* > * there is one cal_dev structure in the driver, it is shared by > * all instances. > @@ -569,9 +576,52 @@ static void cal_get_hwinfo(struct cal_dev *dev) > hwinfo); > } > > +/* > + * Errata i913: CSI2 LDO Needs to be disabled when module is powered on > + * > + * Enabling CSI2 LDO shorts it to core supply. It is crucial the 2 CSI2 > + * LDOs on the device are disabled if CSI-2 module is powered on > + * (0x4845 B304 | 0x4845 B384 [28:27] = 0x1) or in ULPS (0x4845 B304 > + * | 0x4845 B384 [28:27] = 0x2) mode. Common concerns include: high > + * current draw on the module supply in active mode. > + * > + * Errata does not apply when CSI-2 module is powered off > + * (0x4845 B304 | 0x4845 B384 [28:27] = 0x0). > + * > + * SW Workaround: > + * Set the following register bits to disable the LDO, > + * which is essentially CSI2 REG10 bit 6: > + * > + * Core 0: 0x4845 B828 = 0x0000 0040 > + * Core 1: 0x4845 B928 = 0x0000 0040 > + */ > +static void i913_errata(struct cal_dev *dev, unsigned int port) > +{ > + u32 reg10 = reg_read(dev->cc[port], CAL_CSI2_PHY_REG10); > + > + set_field(®10, CAL_CSI2_PHY_REG0_HSCLOCKCONFIG_DISABLE, > + CAL_CSI2_PHY_REG10_I933_LDO_DISABLE_MASK); > + > + cal_dbg(1, dev, "CSI2_%d_REG10 = 0x%08x\n", port, reg10); > + reg_write(dev->cc[port], CAL_CSI2_PHY_REG10, reg10); > +} > + > static inline int cal_runtime_get(struct cal_dev *dev) I'd drop the 'inline' here. It doesn't seem appropriate anymore since this function is now more complex. Regards, Hans > { > - return pm_runtime_get_sync(&dev->pdev->dev); > + int r; > + > + r = pm_runtime_get_sync(&dev->pdev->dev); > + > + if (dev->flags & DRA72_CAL_PRE_ES2_LDO_DISABLE) { > + /* > + * Apply errata on both port eveytime we (re-)enable > + * the clock > + */ > + i913_errata(dev, 0); > + i913_errata(dev, 1); > + } > + > + return r; > } > > static inline void cal_runtime_put(struct cal_dev *dev) > @@ -2071,6 +2121,10 @@ static const struct of_device_id cal_of_match[] = { > .compatible = "ti,dra72-cal", > .data = (void *)&dra72x_cal_data, > }, > + { > + .compatible = "ti,dra72-pre-es2-cal", > + .data = (void *)&dra72x_es1_cal_data, > + }, > {}, > }; > MODULE_DEVICE_TABLE(of, cal_of_match); > diff --git a/drivers/media/platform/ti-vpe/cal_regs.h b/drivers/media/platform/ti-vpe/cal_regs.h > index 68cfc922b422..78d6f015c9ea 100644 > --- a/drivers/media/platform/ti-vpe/cal_regs.h > +++ b/drivers/media/platform/ti-vpe/cal_regs.h > @@ -10,6 +10,30 @@ > #ifndef __TI_CAL_REGS_H > #define __TI_CAL_REGS_H > > +/* > + * struct cal_dev.flags possibilities > + * > + * DRA72_CAL_PRE_ES2_LDO_DISABLE: > + * Errata i913: CSI2 LDO Needs to be disabled when module is powered on > + * > + * Enabling CSI2 LDO shorts it to core supply. It is crucial the 2 CSI2 > + * LDOs on the device are disabled if CSI-2 module is powered on > + * (0x4845 B304 | 0x4845 B384 [28:27] = 0x1) or in ULPS (0x4845 B304 > + * | 0x4845 B384 [28:27] = 0x2) mode. Common concerns include: high > + * current draw on the module supply in active mode. > + * > + * Errata does not apply when CSI-2 module is powered off > + * (0x4845 B304 | 0x4845 B384 [28:27] = 0x0). > + * > + * SW Workaround: > + * Set the following register bits to disable the LDO, > + * which is essentially CSI2 REG10 bit 6: > + * > + * Core 0: 0x4845 B828 = 0x0000 0040 > + * Core 1: 0x4845 B928 = 0x0000 0040 > + */ > +#define DRA72_CAL_PRE_ES2_LDO_DISABLE BIT(0) > + > #define CAL_NUM_CSI2_PORTS 2 > > /* CAL register offsets */ > @@ -71,6 +95,7 @@ > #define CAL_CSI2_PHY_REG0 0x000 > #define CAL_CSI2_PHY_REG1 0x004 > #define CAL_CSI2_PHY_REG2 0x008 > +#define CAL_CSI2_PHY_REG10 0x028 > > /* CAL Control Module Core Camerrx Control register offsets */ > #define CM_CTRL_CORE_CAMERRX_CONTROL 0x000 > @@ -458,6 +483,8 @@ > #define CAL_CSI2_PHY_REG1_CLOCK_MISS_DETECTOR_STATUS_SUCCESS 0 > #define CAL_CSI2_PHY_REG1_RESET_DONE_STATUS_MASK GENMASK(29, 28) > > +#define CAL_CSI2_PHY_REG10_I933_LDO_DISABLE_MASK BIT_MASK(6) > + > #define CAL_CSI2_PHY_REG2_CCP2_SYNC_PATTERN_MASK GENMASK(23, 0) > #define CAL_CSI2_PHY_REG2_TRIGGER_CMD_RXTRIGESC3_MASK GENMASK(25, 24) > #define CAL_CSI2_PHY_REG2_TRIGGER_CMD_RXTRIGESC2_MASK GENMASK(27, 26) >
Hi Benoit, This series looks good to me. I had just one small comment on patch 7 and a comment on patch 19. Just post a v2 for just patch 7/19 and post a patch 20/19 for the requested MAINTAINERS change. Once I have Rob's Acks I can merge this. Regards, Hans On 10/18/19 5:34 PM, Benoit Parrot wrote: > This a collection of backlog patches I have been carrying for the CAL > driver. > > - Add support for SoC variants. > > - Switches to syscon/regmap to access a system controller register for > the DPHY configuration. This register has different bit layout depending > on the SoC version. > > - It adds supports for pre ES2.0 silicon errata. > > - Reworked the DPHY initialization sequence to match the technical > reference manual and provide a more robust restartability. > > - Adds the missing ability to power subdevice. > > - Update the devicetree binding and then converts it to dt-schema > > Benoit Parrot (18): > dt-bindings: media: cal: update binding to use syscon > dt-bindings: media: cal: update binding example > media: ti-vpe: cal: Add per platform data support > media: ti-vpe: cal: Enable DMABUF export > dt-bindings: media: cal: update binding to add PHY LDO errata support > media: ti-vpe: cal: add CSI2 PHY LDO errata support > media: ti-vpe: cal: Fix ths_term/ths_settle parameters > media: ti-vpe: cal: Fix pixel processing parameters > media: ti-vpe: cal: Align DPHY init sequence with docs > dt-bindings: media: cal: update binding to add DRA76x support > media: ti-vpe: cal: Add DRA76x support > dt-bindings: media: cal: update binding to add AM654 support > media: ti-vpe: cal: Add AM654 support > media: ti-vpe: cal: Add subdev s_power hooks > media: ti-vpe: cal: Properly calculate max resolution boundary > media: ti-vpe: cal: Fix a WARN issued when start streaming fails > media: ti-vpe: cal: fix enum_mbus_code/frame_size subdev arguments > dt-bindings: media: cal: convert binding to yaml > > Nikhil Devshatwar (1): > media: ti-vpe: cal: Restrict DMA to avoid memory corruption > > .../devicetree/bindings/media/ti,cal.yaml | 186 +++++ > .../devicetree/bindings/media/ti-cal.txt | 72 -- > drivers/media/platform/Kconfig | 2 +- > drivers/media/platform/ti-vpe/cal.c | 775 ++++++++++++++---- > drivers/media/platform/ti-vpe/cal_regs.h | 27 + > 5 files changed, 830 insertions(+), 232 deletions(-) > create mode 100644 Documentation/devicetree/bindings/media/ti,cal.yaml > delete mode 100644 Documentation/devicetree/bindings/media/ti-cal.txt >
Hans Verkuil <hverkuil@xs4all.nl> wrote on Mon [2019-Oct-21 12:38:03 +0200]: > On 10/18/19 5:34 PM, Benoit Parrot wrote: > > Apply Errata i913 every time the functional clock is enabled. > > This should take care of suspend/resume case as well. > > > > Signed-off-by: Benoit Parrot <bparrot@ti.com> > > Signed-off-by: Jyri Sarha <jsarha@ti.com> > > --- > > drivers/media/platform/ti-vpe/cal.c | 56 +++++++++++++++++++++++- > > drivers/media/platform/ti-vpe/cal_regs.h | 27 ++++++++++++ > > 2 files changed, 82 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c > > index 62aeedb705e9..3cbc4dca6de8 100644 > > --- a/drivers/media/platform/ti-vpe/cal.c > > +++ b/drivers/media/platform/ti-vpe/cal.c > > @@ -284,6 +284,13 @@ static struct cal_data dra72x_cal_data = { > > .flags = 0, > > }; > > > > +static struct cal_data dra72x_es1_cal_data = { > > + .csi2_phy_core = dra72x_cal_csi_phy, > > + .num_csi2_phy = ARRAY_SIZE(dra72x_cal_csi_phy), > > + > > + .flags = DRA72_CAL_PRE_ES2_LDO_DISABLE, > > +}; > > + > > /* > > * there is one cal_dev structure in the driver, it is shared by > > * all instances. > > @@ -569,9 +576,52 @@ static void cal_get_hwinfo(struct cal_dev *dev) > > hwinfo); > > } > > > > +/* > > + * Errata i913: CSI2 LDO Needs to be disabled when module is powered on > > + * > > + * Enabling CSI2 LDO shorts it to core supply. It is crucial the 2 CSI2 > > + * LDOs on the device are disabled if CSI-2 module is powered on > > + * (0x4845 B304 | 0x4845 B384 [28:27] = 0x1) or in ULPS (0x4845 B304 > > + * | 0x4845 B384 [28:27] = 0x2) mode. Common concerns include: high > > + * current draw on the module supply in active mode. > > + * > > + * Errata does not apply when CSI-2 module is powered off > > + * (0x4845 B304 | 0x4845 B384 [28:27] = 0x0). > > + * > > + * SW Workaround: > > + * Set the following register bits to disable the LDO, > > + * which is essentially CSI2 REG10 bit 6: > > + * > > + * Core 0: 0x4845 B828 = 0x0000 0040 > > + * Core 1: 0x4845 B928 = 0x0000 0040 > > + */ > > +static void i913_errata(struct cal_dev *dev, unsigned int port) > > +{ > > + u32 reg10 = reg_read(dev->cc[port], CAL_CSI2_PHY_REG10); > > + > > + set_field(®10, CAL_CSI2_PHY_REG0_HSCLOCKCONFIG_DISABLE, > > + CAL_CSI2_PHY_REG10_I933_LDO_DISABLE_MASK); > > + > > + cal_dbg(1, dev, "CSI2_%d_REG10 = 0x%08x\n", port, reg10); > > + reg_write(dev->cc[port], CAL_CSI2_PHY_REG10, reg10); > > +} > > + > > static inline int cal_runtime_get(struct cal_dev *dev) > > I'd drop the 'inline' here. It doesn't seem appropriate anymore since this > function is now more complex. Ok I'll fix that Benoit > > Regards, > > Hans > > > { > > - return pm_runtime_get_sync(&dev->pdev->dev); > > + int r; > > + > > + r = pm_runtime_get_sync(&dev->pdev->dev); > > + > > + if (dev->flags & DRA72_CAL_PRE_ES2_LDO_DISABLE) { > > + /* > > + * Apply errata on both port eveytime we (re-)enable > > + * the clock > > + */ > > + i913_errata(dev, 0); > > + i913_errata(dev, 1); > > + } > > + > > + return r; > > } > > > > static inline void cal_runtime_put(struct cal_dev *dev) > > @@ -2071,6 +2121,10 @@ static const struct of_device_id cal_of_match[] = { > > .compatible = "ti,dra72-cal", > > .data = (void *)&dra72x_cal_data, > > }, > > + { > > + .compatible = "ti,dra72-pre-es2-cal", > > + .data = (void *)&dra72x_es1_cal_data, > > + }, > > {}, > > }; > > MODULE_DEVICE_TABLE(of, cal_of_match); > > diff --git a/drivers/media/platform/ti-vpe/cal_regs.h b/drivers/media/platform/ti-vpe/cal_regs.h > > index 68cfc922b422..78d6f015c9ea 100644 > > --- a/drivers/media/platform/ti-vpe/cal_regs.h > > +++ b/drivers/media/platform/ti-vpe/cal_regs.h > > @@ -10,6 +10,30 @@ > > #ifndef __TI_CAL_REGS_H > > #define __TI_CAL_REGS_H > > > > +/* > > + * struct cal_dev.flags possibilities > > + * > > + * DRA72_CAL_PRE_ES2_LDO_DISABLE: > > + * Errata i913: CSI2 LDO Needs to be disabled when module is powered on > > + * > > + * Enabling CSI2 LDO shorts it to core supply. It is crucial the 2 CSI2 > > + * LDOs on the device are disabled if CSI-2 module is powered on > > + * (0x4845 B304 | 0x4845 B384 [28:27] = 0x1) or in ULPS (0x4845 B304 > > + * | 0x4845 B384 [28:27] = 0x2) mode. Common concerns include: high > > + * current draw on the module supply in active mode. > > + * > > + * Errata does not apply when CSI-2 module is powered off > > + * (0x4845 B304 | 0x4845 B384 [28:27] = 0x0). > > + * > > + * SW Workaround: > > + * Set the following register bits to disable the LDO, > > + * which is essentially CSI2 REG10 bit 6: > > + * > > + * Core 0: 0x4845 B828 = 0x0000 0040 > > + * Core 1: 0x4845 B928 = 0x0000 0040 > > + */ > > +#define DRA72_CAL_PRE_ES2_LDO_DISABLE BIT(0) > > + > > #define CAL_NUM_CSI2_PORTS 2 > > > > /* CAL register offsets */ > > @@ -71,6 +95,7 @@ > > #define CAL_CSI2_PHY_REG0 0x000 > > #define CAL_CSI2_PHY_REG1 0x004 > > #define CAL_CSI2_PHY_REG2 0x008 > > +#define CAL_CSI2_PHY_REG10 0x028 > > > > /* CAL Control Module Core Camerrx Control register offsets */ > > #define CM_CTRL_CORE_CAMERRX_CONTROL 0x000 > > @@ -458,6 +483,8 @@ > > #define CAL_CSI2_PHY_REG1_CLOCK_MISS_DETECTOR_STATUS_SUCCESS 0 > > #define CAL_CSI2_PHY_REG1_RESET_DONE_STATUS_MASK GENMASK(29, 28) > > > > +#define CAL_CSI2_PHY_REG10_I933_LDO_DISABLE_MASK BIT_MASK(6) > > + > > #define CAL_CSI2_PHY_REG2_CCP2_SYNC_PATTERN_MASK GENMASK(23, 0) > > #define CAL_CSI2_PHY_REG2_TRIGGER_CMD_RXTRIGESC3_MASK GENMASK(25, 24) > > #define CAL_CSI2_PHY_REG2_TRIGGER_CMD_RXTRIGESC2_MASK GENMASK(27, 26) > > >
Hans Verkuil <hverkuil@xs4all.nl> wrote on Mon [2019-Oct-21 12:50:02 +0200]: > Hi Benoit, > > This series looks good to me. I had just one small comment on patch 7 > and a comment on patch 19. > > Just post a v2 for just patch 7/19 and post a patch 20/19 for the requested > MAINTAINERS change. > > Once I have Rob's Acks I can merge this. Ok, thanks. I'll send a v2 probably later this week or early next. Benoit > > Regards, > > Hans > > On 10/18/19 5:34 PM, Benoit Parrot wrote:
On Fri, Oct 18, 2019 at 10:34:21AM -0500, Benoit Parrot wrote: > First this patch adds a method to access the CTRL_CORE_CAMERRX_CONTROL > register to use the syscon mechanism. For backward compatibility we also > handle using the existing camerrx_control "reg" entry if a syscon node > is not found. > > In addition the register bit layout for the CTRL_CORE_CAMERRX_CONTROL > changes depending on the device. In order to support this we need to use > a register access scheme based on data configuration instead of using > static macro. > > In this case we make use of the regmap facility and create data set > based on the various device and phy available. > > Signed-off-by: Benoit Parrot <bparrot@ti.com> > --- > drivers/media/platform/ti-vpe/cal.c | 281 +++++++++++++++++++++------- > 1 file changed, 212 insertions(+), 69 deletions(-) > @@ -1816,6 +1911,18 @@ static int cal_probe(struct platform_device *pdev) > if (!dev) > return -ENOMEM; > > + match = of_match_device(of_match_ptr(cal_of_match), &pdev->dev); Use of_device_get_match_data() instead. > + if (!match) > + return -ENODEV; > + > + if (match->data) { > + dev->data = (struct cal_data *)match->data; > + dev->flags = dev->data->flags; > + } else { > + dev_err(&pdev->dev, "Could not get feature data based on compatible version\n"); > + return -ENODEV; > + } > + > /* set pseudo v4l2 device name so we can use v4l2_printk */ > strscpy(dev->v4l2_dev.name, CAL_MODULE_NAME, > sizeof(dev->v4l2_dev.name)); > @@ -1823,6 +1930,43 @@ static int cal_probe(struct platform_device *pdev) > /* save pdev pointer */ > dev->pdev = pdev; > > + if (parent && of_property_read_bool(parent, "syscon-camerrx")) { > + syscon_camerrx = > + syscon_regmap_lookup_by_phandle(parent, > + "syscon-camerrx"); > + if (IS_ERR(syscon_camerrx)) { > + dev_err(&pdev->dev, "failed to get syscon-camerrx regmap\n"); > + return PTR_ERR(syscon_camerrx); > + } > + > + if (of_property_read_u32_index(parent, "syscon-camerrx", 1, > + &syscon_camerrx_offset)) { Kind of odd to read the property twice and using functions that don't match the type. We have functions to retrieve phandle and args. > + dev_err(&pdev->dev, "failed to get syscon-camerrx offset\n"); > + return -EINVAL; > + } > + } else { > + /* > + * Backward DTS compatibility. > + * If syscon entry is not present then check if the > + * camerrx_control resource is present. > + */ > + syscon_camerrx = cal_get_camerarx_regmap(dev); > + if (IS_ERR(syscon_camerrx)) { > + dev_err(&pdev->dev, "failed to get camerrx_control regmap\n"); > + return PTR_ERR(syscon_camerrx); > + } > + /* In this case the base already point to the direct > + * CM register so no need for an offset > + */ > + syscon_camerrx_offset = 0; > + } > + > + dev->syscon_camerrx = syscon_camerrx; > + dev->syscon_camerrx_offset = syscon_camerrx_offset; > + ret = cal_camerarx_regmap_init(dev); > + if (ret) > + return ret; > + > dev->res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > "cal_top"); > dev->base = devm_ioremap_resource(&pdev->dev, dev->res);
Rob Herring <robh@kernel.org> wrote on Tue [2019-Oct-29 08:18:55 -0500]: > On Fri, Oct 18, 2019 at 10:34:21AM -0500, Benoit Parrot wrote: > > First this patch adds a method to access the CTRL_CORE_CAMERRX_CONTROL > > register to use the syscon mechanism. For backward compatibility we also > > handle using the existing camerrx_control "reg" entry if a syscon node > > is not found. > > > > In addition the register bit layout for the CTRL_CORE_CAMERRX_CONTROL > > changes depending on the device. In order to support this we need to use > > a register access scheme based on data configuration instead of using > > static macro. > > > > In this case we make use of the regmap facility and create data set > > based on the various device and phy available. > > > > Signed-off-by: Benoit Parrot <bparrot@ti.com> > > --- > > drivers/media/platform/ti-vpe/cal.c | 281 +++++++++++++++++++++------- > > 1 file changed, 212 insertions(+), 69 deletions(-) > > > > @@ -1816,6 +1911,18 @@ static int cal_probe(struct platform_device *pdev) > > if (!dev) > > return -ENOMEM; > > > > + match = of_match_device(of_match_ptr(cal_of_match), &pdev->dev); > > Use of_device_get_match_data() instead. Ok I'll change that. > > > + if (!match) > > + return -ENODEV; > > + > > + if (match->data) { > > + dev->data = (struct cal_data *)match->data; > > + dev->flags = dev->data->flags; > > + } else { > > + dev_err(&pdev->dev, "Could not get feature data based on compatible version\n"); > > + return -ENODEV; > > + } > > + > > /* set pseudo v4l2 device name so we can use v4l2_printk */ > > strscpy(dev->v4l2_dev.name, CAL_MODULE_NAME, > > sizeof(dev->v4l2_dev.name)); > > @@ -1823,6 +1930,43 @@ static int cal_probe(struct platform_device *pdev) > > /* save pdev pointer */ > > dev->pdev = pdev; > > > > + if (parent && of_property_read_bool(parent, "syscon-camerrx")) { > > + syscon_camerrx = > > + syscon_regmap_lookup_by_phandle(parent, > > + "syscon-camerrx"); > > + if (IS_ERR(syscon_camerrx)) { > > + dev_err(&pdev->dev, "failed to get syscon-camerrx regmap\n"); > > + return PTR_ERR(syscon_camerrx); > > + } > > + > > + if (of_property_read_u32_index(parent, "syscon-camerrx", 1, > > + &syscon_camerrx_offset)) { > > Kind of odd to read the property twice and using functions that don't > match the type. We have functions to retrieve phandle and args. Yeah, I wanted to make a distinction between the node being present and any other kind of errors, so we can have a little more precise error message. > > > + dev_err(&pdev->dev, "failed to get syscon-camerrx offset\n"); > > + return -EINVAL; > > + } > > + } else { > > + /* > > + * Backward DTS compatibility. > > + * If syscon entry is not present then check if the > > + * camerrx_control resource is present. > > + */ > > + syscon_camerrx = cal_get_camerarx_regmap(dev); > > + if (IS_ERR(syscon_camerrx)) { > > + dev_err(&pdev->dev, "failed to get camerrx_control regmap\n"); > > + return PTR_ERR(syscon_camerrx); > > + } > > + /* In this case the base already point to the direct > > + * CM register so no need for an offset > > + */ > > + syscon_camerrx_offset = 0; > > + } > > + > > + dev->syscon_camerrx = syscon_camerrx; > > + dev->syscon_camerrx_offset = syscon_camerrx_offset; > > + ret = cal_camerarx_regmap_init(dev); > > + if (ret) > > + return ret; > > + > > dev->res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > > "cal_top"); > > dev->base = devm_ioremap_resource(&pdev->dev, dev->res); >