| Message ID | 1596740494-19306-9-git-send-email-skomatineni@nvidia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v9,01/10] media: tegra-video: Fix channel format alignment | expand |
06.08.2020 22:01, Sowjanya Komatineni пишет: ... > +int tegra_mipi_start_calibration(struct tegra_mipi_device *device) > { > const struct tegra_mipi_soc *soc = device->mipi->soc; > unsigned int i; > @@ -381,12 +375,16 @@ int tegra_mipi_calibrate(struct tegra_mipi_device *device) > value |= MIPI_CAL_CTRL_START; > tegra_mipi_writel(device->mipi, value, MIPI_CAL_CTRL); > > - mutex_unlock(&device->mipi->lock); > - clk_disable(device->mipi->clk); > + /* > + * Wait for min 72uS to let calibration logic finish calibration > + * sequence codes before waiting for pads idle state to apply the > + * results. > + */ > + usleep_range(75, 80); Could you please explain why the ACTIVE bit can't be polled instead of using the fixed delay? Doesn't ACTIVE bit represents the state of the busy FSM?
On 8/6/20 7:31 PM, Dmitry Osipenko wrote: > 06.08.2020 22:01, Sowjanya Komatineni пишет: > ... >> +int tegra_mipi_start_calibration(struct tegra_mipi_device *device) >> { >> const struct tegra_mipi_soc *soc = device->mipi->soc; >> unsigned int i; >> @@ -381,12 +375,16 @@ int tegra_mipi_calibrate(struct tegra_mipi_device *device) >> value |= MIPI_CAL_CTRL_START; >> tegra_mipi_writel(device->mipi, value, MIPI_CAL_CTRL); >> >> - mutex_unlock(&device->mipi->lock); >> - clk_disable(device->mipi->clk); >> + /* >> + * Wait for min 72uS to let calibration logic finish calibration >> + * sequence codes before waiting for pads idle state to apply the >> + * results. >> + */ >> + usleep_range(75, 80); > Could you please explain why the ACTIVE bit can't be polled instead of > using the fixed delay? Doesn't ACTIVE bit represents the state of the > busy FSM? Based on internal discussion, ACTIVE bit gets cleared when all enabled pads calibration is done (same time as when DONE set to 1). Will request HW designer to look into design and confirm exactly when ACTIVE bit gets cleared. Will get back on this.
On 8/6/20 8:10 PM, Sowjanya Komatineni wrote: > > On 8/6/20 7:31 PM, Dmitry Osipenko wrote: >> 06.08.2020 22:01, Sowjanya Komatineni пишет: >> ... >>> +int tegra_mipi_start_calibration(struct tegra_mipi_device *device) >>> { >>> const struct tegra_mipi_soc *soc = device->mipi->soc; >>> unsigned int i; >>> @@ -381,12 +375,16 @@ int tegra_mipi_calibrate(struct >>> tegra_mipi_device *device) >>> value |= MIPI_CAL_CTRL_START; >>> tegra_mipi_writel(device->mipi, value, MIPI_CAL_CTRL); >>> - mutex_unlock(&device->mipi->lock); >>> - clk_disable(device->mipi->clk); >>> + /* >>> + * Wait for min 72uS to let calibration logic finish calibration >>> + * sequence codes before waiting for pads idle state to apply the >>> + * results. >>> + */ >>> + usleep_range(75, 80); >> Could you please explain why the ACTIVE bit can't be polled instead of >> using the fixed delay? Doesn't ACTIVE bit represents the state of the >> busy FSM? > > Based on internal discussion, ACTIVE bit gets cleared when all enabled > pads calibration is done (same time as when DONE set to 1). > > Will request HW designer to look into design and confirm exactly when > ACTIVE bit gets cleared. > > Will get back on this. > Verified with HW designer. above is correct. ACTIVE bit update happens same time as DONE bit. Active = !(DONE) In case of calibration logic waiting for LP-11 where done bit does not get set, ACTIVE will still be 1 and on next start trigger new calibration will start > >
On 8/6/20 8:14 PM, Sowjanya Komatineni wrote: > > On 8/6/20 8:10 PM, Sowjanya Komatineni wrote: >> >> On 8/6/20 7:31 PM, Dmitry Osipenko wrote: >>> 06.08.2020 22:01, Sowjanya Komatineni пишет: >>> ... >>>> +int tegra_mipi_start_calibration(struct tegra_mipi_device *device) >>>> { >>>> const struct tegra_mipi_soc *soc = device->mipi->soc; >>>> unsigned int i; >>>> @@ -381,12 +375,16 @@ int tegra_mipi_calibrate(struct >>>> tegra_mipi_device *device) >>>> value |= MIPI_CAL_CTRL_START; >>>> tegra_mipi_writel(device->mipi, value, MIPI_CAL_CTRL); >>>> - mutex_unlock(&device->mipi->lock); >>>> - clk_disable(device->mipi->clk); >>>> + /* >>>> + * Wait for min 72uS to let calibration logic finish calibration >>>> + * sequence codes before waiting for pads idle state to apply the >>>> + * results. >>>> + */ >>>> + usleep_range(75, 80); >>> Could you please explain why the ACTIVE bit can't be polled instead of >>> using the fixed delay? Doesn't ACTIVE bit represents the state of the >>> busy FSM? >> >> Based on internal discussion, ACTIVE bit gets cleared when all >> enabled pads calibration is done (same time as when DONE set to 1). >> >> Will request HW designer to look into design and confirm exactly when >> ACTIVE bit gets cleared. >> >> Will get back on this. >> > Verified with HW designer. above is correct. ACTIVE bit update happens > same time as DONE bit. > > Active = !(DONE) > > In case of calibration logic waiting for LP-11 where done bit does not > get set, ACTIVE will still be 1 and on next start trigger new > calibration will start > Based on internal design check from designer, as long as its in waiting for LP-11 stage, next calibration request can be triggered again but ACTIVE bit we will see it at 1. So we should check for DONE bits to confirm if calibration is done or not. To start next calibration, it can take effect as long as its in wait for LP-11 mode. >> >>
07.08.2020 06:18, Sowjanya Komatineni пишет: > > On 8/6/20 8:14 PM, Sowjanya Komatineni wrote: >> >> On 8/6/20 8:10 PM, Sowjanya Komatineni wrote: >>> >>> On 8/6/20 7:31 PM, Dmitry Osipenko wrote: >>>> 06.08.2020 22:01, Sowjanya Komatineni пишет: >>>> ... >>>>> +int tegra_mipi_start_calibration(struct tegra_mipi_device *device) >>>>> { >>>>> const struct tegra_mipi_soc *soc = device->mipi->soc; >>>>> unsigned int i; >>>>> @@ -381,12 +375,16 @@ int tegra_mipi_calibrate(struct >>>>> tegra_mipi_device *device) >>>>> value |= MIPI_CAL_CTRL_START; >>>>> tegra_mipi_writel(device->mipi, value, MIPI_CAL_CTRL); >>>>> - mutex_unlock(&device->mipi->lock); >>>>> - clk_disable(device->mipi->clk); >>>>> + /* >>>>> + * Wait for min 72uS to let calibration logic finish calibration >>>>> + * sequence codes before waiting for pads idle state to apply the >>>>> + * results. >>>>> + */ >>>>> + usleep_range(75, 80); >>>> Could you please explain why the ACTIVE bit can't be polled instead of >>>> using the fixed delay? Doesn't ACTIVE bit represents the state of the >>>> busy FSM? >>> >>> Based on internal discussion, ACTIVE bit gets cleared when all >>> enabled pads calibration is done (same time as when DONE set to 1). >>> >>> Will request HW designer to look into design and confirm exactly when >>> ACTIVE bit gets cleared. >>> >>> Will get back on this. >>> >> Verified with HW designer. above is correct. ACTIVE bit update happens >> same time as DONE bit. >> >> Active = !(DONE) >> >> In case of calibration logic waiting for LP-11 where done bit does not >> get set, ACTIVE will still be 1 and on next start trigger new >> calibration will start >> > Based on internal design check from designer, as long as its in waiting > for LP-11 stage, next calibration request can be triggered again but > ACTIVE bit we will see it at 1. So we should check for DONE bits to > confirm if calibration is done or not. > > To start next calibration, it can take effect as long as its in wait for > LP-11 mode. I meant the start_calibration() will poll the ACTIVE bit (calibration busy), while the finish_calibration() will poll the DONE bit (calibration applied).
On 8/6/20 9:01 PM, Dmitry Osipenko wrote: > 07.08.2020 06:18, Sowjanya Komatineni пишет: >> On 8/6/20 8:14 PM, Sowjanya Komatineni wrote: >>> On 8/6/20 8:10 PM, Sowjanya Komatineni wrote: >>>> On 8/6/20 7:31 PM, Dmitry Osipenko wrote: >>>>> 06.08.2020 22:01, Sowjanya Komatineni пишет: >>>>> ... >>>>>> +int tegra_mipi_start_calibration(struct tegra_mipi_device *device) >>>>>> { >>>>>> const struct tegra_mipi_soc *soc = device->mipi->soc; >>>>>> unsigned int i; >>>>>> @@ -381,12 +375,16 @@ int tegra_mipi_calibrate(struct >>>>>> tegra_mipi_device *device) >>>>>> value |= MIPI_CAL_CTRL_START; >>>>>> tegra_mipi_writel(device->mipi, value, MIPI_CAL_CTRL); >>>>>> - mutex_unlock(&device->mipi->lock); >>>>>> - clk_disable(device->mipi->clk); >>>>>> + /* >>>>>> + * Wait for min 72uS to let calibration logic finish calibration >>>>>> + * sequence codes before waiting for pads idle state to apply the >>>>>> + * results. >>>>>> + */ >>>>>> + usleep_range(75, 80); >>>>> Could you please explain why the ACTIVE bit can't be polled instead of >>>>> using the fixed delay? Doesn't ACTIVE bit represents the state of the >>>>> busy FSM? >>>> Based on internal discussion, ACTIVE bit gets cleared when all >>>> enabled pads calibration is done (same time as when DONE set to 1). >>>> >>>> Will request HW designer to look into design and confirm exactly when >>>> ACTIVE bit gets cleared. >>>> >>>> Will get back on this. >>>> >>> Verified with HW designer. above is correct. ACTIVE bit update happens >>> same time as DONE bit. >>> >>> Active = !(DONE) >>> >>> In case of calibration logic waiting for LP-11 where done bit does not >>> get set, ACTIVE will still be 1 and on next start trigger new >>> calibration will start >>> >> Based on internal design check from designer, as long as its in waiting >> for LP-11 stage, next calibration request can be triggered again but >> ACTIVE bit we will see it at 1. So we should check for DONE bits to >> confirm if calibration is done or not. >> >> To start next calibration, it can take effect as long as its in wait for >> LP-11 mode. > I meant the start_calibration() will poll the ACTIVE bit (calibration > busy), while the finish_calibration() will poll the DONE bit > (calibration applied). ACTIVE bit can be 1 when previous calibration process does not see LP-11. So there is no need to use ACTIVE bit during start of calibration. At HW level, both ACTIVE and DONE bits get set at same time. So waiting for ACTIVE to be 0 during start calibration instead of 7uS will not work as ACTIVE bit will not become 0 after calibration sequence codes and it will get updated along with DONE bits only after applying results to pads which happens after seeing LP-11 on pads.
On 8/6/20 9:05 PM, Sowjanya Komatineni wrote: > > On 8/6/20 9:01 PM, Dmitry Osipenko wrote: >> 07.08.2020 06:18, Sowjanya Komatineni пишет: >>> On 8/6/20 8:14 PM, Sowjanya Komatineni wrote: >>>> On 8/6/20 8:10 PM, Sowjanya Komatineni wrote: >>>>> On 8/6/20 7:31 PM, Dmitry Osipenko wrote: >>>>>> 06.08.2020 22:01, Sowjanya Komatineni пишет: >>>>>> ... >>>>>>> +int tegra_mipi_start_calibration(struct tegra_mipi_device *device) >>>>>>> { >>>>>>> const struct tegra_mipi_soc *soc = device->mipi->soc; >>>>>>> unsigned int i; >>>>>>> @@ -381,12 +375,16 @@ int tegra_mipi_calibrate(struct >>>>>>> tegra_mipi_device *device) >>>>>>> value |= MIPI_CAL_CTRL_START; >>>>>>> tegra_mipi_writel(device->mipi, value, MIPI_CAL_CTRL); >>>>>>> - mutex_unlock(&device->mipi->lock); >>>>>>> - clk_disable(device->mipi->clk); >>>>>>> + /* >>>>>>> + * Wait for min 72uS to let calibration logic finish >>>>>>> calibration >>>>>>> + * sequence codes before waiting for pads idle state to >>>>>>> apply the >>>>>>> + * results. >>>>>>> + */ >>>>>>> + usleep_range(75, 80); >>>>>> Could you please explain why the ACTIVE bit can't be polled >>>>>> instead of >>>>>> using the fixed delay? Doesn't ACTIVE bit represents the state of >>>>>> the >>>>>> busy FSM? >>>>> Based on internal discussion, ACTIVE bit gets cleared when all >>>>> enabled pads calibration is done (same time as when DONE set to 1). >>>>> >>>>> Will request HW designer to look into design and confirm exactly when >>>>> ACTIVE bit gets cleared. >>>>> >>>>> Will get back on this. >>>>> >>>> Verified with HW designer. above is correct. ACTIVE bit update happens >>>> same time as DONE bit. >>>> >>>> Active = !(DONE) >>>> >>>> In case of calibration logic waiting for LP-11 where done bit does not >>>> get set, ACTIVE will still be 1 and on next start trigger new >>>> calibration will start >>>> >>> Based on internal design check from designer, as long as its in waiting >>> for LP-11 stage, next calibration request can be triggered again but >>> ACTIVE bit we will see it at 1. So we should check for DONE bits to >>> confirm if calibration is done or not. >>> >>> To start next calibration, it can take effect as long as its in wait >>> for >>> LP-11 mode. >> I meant the start_calibration() will poll the ACTIVE bit (calibration >> busy), while the finish_calibration() will poll the DONE bit >> (calibration applied). > > ACTIVE bit can be 1 when previous calibration process does not see LP-11. > > So there is no need to use ACTIVE bit during start of calibration. > > At HW level, both ACTIVE and DONE bits get set at same time. > > So waiting for ACTIVE to be 0 during start calibration instead of > *75uS will not work as ACTIVE bit will not become 0 after calibration > sequence codes and it will get updated along with DONE bits only after > applying results to pads which happens after seeing LP-11 on pads. > *typo fixed
07.08.2020 07:06, Sowjanya Komatineni пишет: > > On 8/6/20 9:05 PM, Sowjanya Komatineni wrote: >> >> On 8/6/20 9:01 PM, Dmitry Osipenko wrote: >>> 07.08.2020 06:18, Sowjanya Komatineni пишет: >>>> On 8/6/20 8:14 PM, Sowjanya Komatineni wrote: >>>>> On 8/6/20 8:10 PM, Sowjanya Komatineni wrote: >>>>>> On 8/6/20 7:31 PM, Dmitry Osipenko wrote: >>>>>>> 06.08.2020 22:01, Sowjanya Komatineni пишет: >>>>>>> ... >>>>>>>> +int tegra_mipi_start_calibration(struct tegra_mipi_device *device) >>>>>>>> { >>>>>>>> const struct tegra_mipi_soc *soc = device->mipi->soc; >>>>>>>> unsigned int i; >>>>>>>> @@ -381,12 +375,16 @@ int tegra_mipi_calibrate(struct >>>>>>>> tegra_mipi_device *device) >>>>>>>> value |= MIPI_CAL_CTRL_START; >>>>>>>> tegra_mipi_writel(device->mipi, value, MIPI_CAL_CTRL); >>>>>>>> - mutex_unlock(&device->mipi->lock); >>>>>>>> - clk_disable(device->mipi->clk); >>>>>>>> + /* >>>>>>>> + * Wait for min 72uS to let calibration logic finish >>>>>>>> calibration >>>>>>>> + * sequence codes before waiting for pads idle state to >>>>>>>> apply the >>>>>>>> + * results. >>>>>>>> + */ >>>>>>>> + usleep_range(75, 80); >>>>>>> Could you please explain why the ACTIVE bit can't be polled >>>>>>> instead of >>>>>>> using the fixed delay? Doesn't ACTIVE bit represents the state of >>>>>>> the >>>>>>> busy FSM? >>>>>> Based on internal discussion, ACTIVE bit gets cleared when all >>>>>> enabled pads calibration is done (same time as when DONE set to 1). >>>>>> >>>>>> Will request HW designer to look into design and confirm exactly when >>>>>> ACTIVE bit gets cleared. >>>>>> >>>>>> Will get back on this. >>>>>> >>>>> Verified with HW designer. above is correct. ACTIVE bit update happens >>>>> same time as DONE bit. >>>>> >>>>> Active = !(DONE) >>>>> >>>>> In case of calibration logic waiting for LP-11 where done bit does not >>>>> get set, ACTIVE will still be 1 and on next start trigger new >>>>> calibration will start >>>>> >>>> Based on internal design check from designer, as long as its in waiting >>>> for LP-11 stage, next calibration request can be triggered again but >>>> ACTIVE bit we will see it at 1. So we should check for DONE bits to >>>> confirm if calibration is done or not. >>>> >>>> To start next calibration, it can take effect as long as its in wait >>>> for >>>> LP-11 mode. >>> I meant the start_calibration() will poll the ACTIVE bit (calibration >>> busy), while the finish_calibration() will poll the DONE bit >>> (calibration applied). >> >> ACTIVE bit can be 1 when previous calibration process does not see LP-11. >> >> So there is no need to use ACTIVE bit during start of calibration. >> >> At HW level, both ACTIVE and DONE bits get set at same time. >> >> So waiting for ACTIVE to be 0 during start calibration instead of >> *75uS will not work as ACTIVE bit will not become 0 after calibration >> sequence codes and it will get updated along with DONE bits only after >> applying results to pads which happens after seeing LP-11 on pads. >> > *typo fixed I see now, thank you.
diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c index 3820e8d..a7864e9 100644 --- a/drivers/gpu/drm/tegra/dsi.c +++ b/drivers/gpu/drm/tegra/dsi.c @@ -694,11 +694,11 @@ static int tegra_dsi_pad_calibrate(struct tegra_dsi *dsi) DSI_PAD_PREEMP_PD(0x03) | DSI_PAD_PREEMP_PU(0x3); tegra_dsi_writel(dsi, value, DSI_PAD_CONTROL_3); - err = tegra_mipi_calibrate(dsi->mipi); + err = tegra_mipi_start_calibration(dsi->mipi); if (err < 0) return err; - return tegra_mipi_wait(dsi->mipi); + return tegra_mipi_finish_calibration(dsi->mipi); } static void tegra_dsi_set_timeout(struct tegra_dsi *dsi, unsigned long bclk, diff --git a/drivers/gpu/host1x/mipi.c b/drivers/gpu/host1x/mipi.c index e606464..2efe12d 100644 --- a/drivers/gpu/host1x/mipi.c +++ b/drivers/gpu/host1x/mipi.c @@ -293,19 +293,13 @@ int tegra_mipi_disable(struct tegra_mipi_device *dev) } EXPORT_SYMBOL(tegra_mipi_disable); -int tegra_mipi_wait(struct tegra_mipi_device *device) +int tegra_mipi_finish_calibration(struct tegra_mipi_device *device) { struct tegra_mipi *mipi = device->mipi; void __iomem *status_reg = mipi->regs + (MIPI_CAL_STATUS << 2); u32 value; int err; - err = clk_enable(device->mipi->clk); - if (err < 0) - return err; - - mutex_lock(&device->mipi->lock); - err = readl_relaxed_poll_timeout(status_reg, value, !(value & MIPI_CAL_STATUS_ACTIVE) && (value & MIPI_CAL_STATUS_DONE), 50, @@ -315,9 +309,9 @@ int tegra_mipi_wait(struct tegra_mipi_device *device) return err; } -EXPORT_SYMBOL(tegra_mipi_wait); +EXPORT_SYMBOL(tegra_mipi_finish_calibration); -int tegra_mipi_calibrate(struct tegra_mipi_device *device) +int tegra_mipi_start_calibration(struct tegra_mipi_device *device) { const struct tegra_mipi_soc *soc = device->mipi->soc; unsigned int i; @@ -381,12 +375,16 @@ int tegra_mipi_calibrate(struct tegra_mipi_device *device) value |= MIPI_CAL_CTRL_START; tegra_mipi_writel(device->mipi, value, MIPI_CAL_CTRL); - mutex_unlock(&device->mipi->lock); - clk_disable(device->mipi->clk); + /* + * Wait for min 72uS to let calibration logic finish calibration + * sequence codes before waiting for pads idle state to apply the + * results. + */ + usleep_range(75, 80); return 0; } -EXPORT_SYMBOL(tegra_mipi_calibrate); +EXPORT_SYMBOL(tegra_mipi_start_calibration); static const struct tegra_mipi_pad tegra114_mipi_pads[] = { { .data = MIPI_CAL_CONFIG_CSIA }, diff --git a/include/linux/host1x.h b/include/linux/host1x.h index 20c885d..ce59a6a 100644 --- a/include/linux/host1x.h +++ b/include/linux/host1x.h @@ -333,7 +333,7 @@ struct tegra_mipi_device *tegra_mipi_request(struct device *device, void tegra_mipi_free(struct tegra_mipi_device *device); int tegra_mipi_enable(struct tegra_mipi_device *device); int tegra_mipi_disable(struct tegra_mipi_device *device); -int tegra_mipi_calibrate(struct tegra_mipi_device *device); -int tegra_mipi_wait(struct tegra_mipi_device *device); +int tegra_mipi_start_calibration(struct tegra_mipi_device *device); +int tegra_mipi_finish_calibration(struct tegra_mipi_device *device); #endif