| Message ID | 20210412113457.328012-16-tomi.valkeinen@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: ti-vpe: cal: prepare for multistream support | expand |
Hi Tomi, Thank you for the patch. On Mon, Apr 12, 2021 at 02:34:44PM +0300, Tomi Valkeinen wrote: > Asserting ComplexIO reset seems to affect the HW (ie. asserting reset > will break an active capture), but the RESET_DONE bit never changes to > "reset is ongoing" state. Thus we always get a timeout. > > Drop the wait, as it seems to achieve nothing. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > drivers/media/platform/ti-vpe/cal-camerarx.c | 15 ++------------- > 1 file changed, 2 insertions(+), 13 deletions(-) > > diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c > index 0354f311c5d2..245c601b992c 100644 > --- a/drivers/media/platform/ti-vpe/cal-camerarx.c > +++ b/drivers/media/platform/ti-vpe/cal-camerarx.c > @@ -405,7 +405,6 @@ static int cal_camerarx_start(struct cal_camerarx *phy) > > static void cal_camerarx_stop(struct cal_camerarx *phy) > { > - unsigned int i; > int ret; > > cal_camerarx_ppi_disable(phy); > @@ -419,19 +418,9 @@ static void cal_camerarx_stop(struct cal_camerarx *phy) > CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL, > CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_MASK); > > - /* Wait for power down completion */ > - for (i = 0; i < 10; i++) { > - if (cal_read_field(phy->cal, > - CAL_CSI2_COMPLEXIO_CFG(phy->instance), > - CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_MASK) == > - CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_RESETONGOING) Isn't this the wrong condition ? I would have expected CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_RESETCOMPLETED, not CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_RESETONGOING. That could explain why you always get a timeout. > - break; > - usleep_range(1000, 1100); > - } > - phy_dbg(3, phy, "CAL_CSI2_COMPLEXIO_CFG(%d) = 0x%08x Complex IO in Reset (%d) %s\n", > + phy_dbg(3, phy, "CAL_CSI2_COMPLEXIO_CFG(%d) = 0x%08x Complex IO in Reset\n", > phy->instance, > - cal_read(phy->cal, CAL_CSI2_COMPLEXIO_CFG(phy->instance)), i, > - (i >= 10) ? "(timeout)" : ""); > + cal_read(phy->cal, CAL_CSI2_COMPLEXIO_CFG(phy->instance))); > > /* Disable the phy */ > cal_camerarx_disable(phy); -- Regards, Laurent Pinchart
On 18/04/2021 15:46, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Mon, Apr 12, 2021 at 02:34:44PM +0300, Tomi Valkeinen wrote: >> Asserting ComplexIO reset seems to affect the HW (ie. asserting reset >> will break an active capture), but the RESET_DONE bit never changes to >> "reset is ongoing" state. Thus we always get a timeout. >> >> Drop the wait, as it seems to achieve nothing. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> drivers/media/platform/ti-vpe/cal-camerarx.c | 15 ++------------- >> 1 file changed, 2 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c >> index 0354f311c5d2..245c601b992c 100644 >> --- a/drivers/media/platform/ti-vpe/cal-camerarx.c >> +++ b/drivers/media/platform/ti-vpe/cal-camerarx.c >> @@ -405,7 +405,6 @@ static int cal_camerarx_start(struct cal_camerarx *phy) >> >> static void cal_camerarx_stop(struct cal_camerarx *phy) >> { >> - unsigned int i; >> int ret; >> >> cal_camerarx_ppi_disable(phy); >> @@ -419,19 +418,9 @@ static void cal_camerarx_stop(struct cal_camerarx *phy) >> CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL, >> CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_MASK); >> >> - /* Wait for power down completion */ >> - for (i = 0; i < 10; i++) { >> - if (cal_read_field(phy->cal, >> - CAL_CSI2_COMPLEXIO_CFG(phy->instance), >> - CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_MASK) == >> - CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_RESETONGOING) > > Isn't this the wrong condition ? I would have expected > CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_RESETCOMPLETED, not > CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_RESETONGOING. That could explain why > you always get a timeout. No, I don't think so. The complexio reset is set active just before the wait. So the reset status should show reset ongoing, until at some point we release the reset (we do that when starting the PHY again). The TRM doesn't talk about this, though. So, I guess the status might go to RESETONGOING for a very short time and back to RESETCOMPLETED before the code can see the RESETONGOING. But I suspect the status just stays at RESETCOMPLETED. Tomi
Hi Tomi, On Mon, Apr 19, 2021 at 02:29:20PM +0300, Tomi Valkeinen wrote: > On 18/04/2021 15:46, Laurent Pinchart wrote: > > On Mon, Apr 12, 2021 at 02:34:44PM +0300, Tomi Valkeinen wrote: > >> Asserting ComplexIO reset seems to affect the HW (ie. asserting reset > >> will break an active capture), but the RESET_DONE bit never changes to > >> "reset is ongoing" state. Thus we always get a timeout. > >> > >> Drop the wait, as it seems to achieve nothing. > >> > >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > >> --- > >> drivers/media/platform/ti-vpe/cal-camerarx.c | 15 ++------------- > >> 1 file changed, 2 insertions(+), 13 deletions(-) > >> > >> diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c > >> index 0354f311c5d2..245c601b992c 100644 > >> --- a/drivers/media/platform/ti-vpe/cal-camerarx.c > >> +++ b/drivers/media/platform/ti-vpe/cal-camerarx.c > >> @@ -405,7 +405,6 @@ static int cal_camerarx_start(struct cal_camerarx *phy) > >> > >> static void cal_camerarx_stop(struct cal_camerarx *phy) > >> { > >> - unsigned int i; > >> int ret; > >> > >> cal_camerarx_ppi_disable(phy); > >> @@ -419,19 +418,9 @@ static void cal_camerarx_stop(struct cal_camerarx *phy) > >> CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL, > >> CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_MASK); > >> > >> - /* Wait for power down completion */ > >> - for (i = 0; i < 10; i++) { > >> - if (cal_read_field(phy->cal, > >> - CAL_CSI2_COMPLEXIO_CFG(phy->instance), > >> - CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_MASK) == > >> - CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_RESETONGOING) > > > > Isn't this the wrong condition ? I would have expected > > CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_RESETCOMPLETED, not > > CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_RESETONGOING. That could explain why > > you always get a timeout. > > No, I don't think so. The complexio reset is set active just before the > wait. So the reset status should show reset ongoing, until at some point > we release the reset (we do that when starting the PHY again). > > The TRM doesn't talk about this, though. So, I guess the status might go > to RESETONGOING for a very short time and back to RESETCOMPLETED before > the code can see the RESETONGOING. But I suspect the status just stays > at RESETCOMPLETED. The TRM is indeed not very clear. My understanding was that asserting RESET_CTRL initiates the reset, and RESET_DONE switches to 1 once the reset completes. There's however a note in the initialization sequence that states a. Deassert the CAMERARX reset: i. Set CAL_CSI2_COMPLEXIO_CFG_j[30] RESET_CTRL to 0x1. CAUTION For the CAL_CSI2_COMPLEXIO_CFG_j[29] RESET_DONE bit to be set to 0x1 (reset completed), the external sensor must to be active and sending the MIPI HS BYTECLK (that is, RXBYTECLKHS clock). The RESET_DONE bit may thus only toggle when de-asserting the reset signal (by setting RESET_CTRL to 1). It would be useful to test that hypothesis by reading RESET_DONE just before setting RESET_CTRL to 1, and right after. I'd expect the values to be 0 and 1 respectively. If that's the case, then this patch is likely correct, so Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> The register macros are quite confusing by the way. We have #define CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_MASK BIT(30) #define CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL 0 #define CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_OPERATIONAL 1 When reading the code, I thought cal_write_field(phy->cal, CAL_CSI2_COMPLEXIO_CFG(phy->instance), CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL, CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_MASK) meant that we were setting the reset bit to 1. I would personally get rid of the _MASK suffixes for single bits, and use 0 and 1 in the code instead of CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL and CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_OPERATIONAL. -- Regards, Laurent Pinchart
On 29/04/2021 02:57, Laurent Pinchart wrote: > Hi Tomi, > > On Mon, Apr 19, 2021 at 02:29:20PM +0300, Tomi Valkeinen wrote: >> On 18/04/2021 15:46, Laurent Pinchart wrote: >>> On Mon, Apr 12, 2021 at 02:34:44PM +0300, Tomi Valkeinen wrote: >>>> Asserting ComplexIO reset seems to affect the HW (ie. asserting reset >>>> will break an active capture), but the RESET_DONE bit never changes to >>>> "reset is ongoing" state. Thus we always get a timeout. >>>> >>>> Drop the wait, as it seems to achieve nothing. >>>> >>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >>>> --- >>>> drivers/media/platform/ti-vpe/cal-camerarx.c | 15 ++------------- >>>> 1 file changed, 2 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c >>>> index 0354f311c5d2..245c601b992c 100644 >>>> --- a/drivers/media/platform/ti-vpe/cal-camerarx.c >>>> +++ b/drivers/media/platform/ti-vpe/cal-camerarx.c >>>> @@ -405,7 +405,6 @@ static int cal_camerarx_start(struct cal_camerarx *phy) >>>> >>>> static void cal_camerarx_stop(struct cal_camerarx *phy) >>>> { >>>> - unsigned int i; >>>> int ret; >>>> >>>> cal_camerarx_ppi_disable(phy); >>>> @@ -419,19 +418,9 @@ static void cal_camerarx_stop(struct cal_camerarx *phy) >>>> CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL, >>>> CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_MASK); >>>> >>>> - /* Wait for power down completion */ >>>> - for (i = 0; i < 10; i++) { >>>> - if (cal_read_field(phy->cal, >>>> - CAL_CSI2_COMPLEXIO_CFG(phy->instance), >>>> - CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_MASK) == >>>> - CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_RESETONGOING) >>> >>> Isn't this the wrong condition ? I would have expected >>> CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_RESETCOMPLETED, not >>> CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_RESETONGOING. That could explain why >>> you always get a timeout. >> >> No, I don't think so. The complexio reset is set active just before the >> wait. So the reset status should show reset ongoing, until at some point >> we release the reset (we do that when starting the PHY again). >> >> The TRM doesn't talk about this, though. So, I guess the status might go >> to RESETONGOING for a very short time and back to RESETCOMPLETED before >> the code can see the RESETONGOING. But I suspect the status just stays >> at RESETCOMPLETED. > > The TRM is indeed not very clear. My understanding was that asserting > RESET_CTRL initiates the reset, and RESET_DONE switches to 1 once the > reset completes. There's however a note in the initialization sequence > that states No, it's a bit to keep (or release) camerarx in reset. When the camerarx is being started, both reset ctrl and done are 0. The driver then releases the reset by setting ctrl to 1. Nothing happens at this time, as the camerarx needs the byteclk from the sensor to finish the reset. Later, when the sensor has been started, done changes to 1. This works fine. The problem is on the stop side. Setting ctrl back to 0 does something, as the capture stops, so presumably the camerarx goes into reset state. But the done bit does not change. The done bit is back to 0 when we start the camerarx again. My guess is that it's reset when the CAL module is turned off via runtime PM. This is not good, as there's no strict rule that says CAL will be turned off by runtime PM. However, I have not found any other way to reset the done bit. And in the case that we don't get a CAL reset, I guess we're still fine, as the camerarx is just already out of reset, and works. Tomi
Hi Tomi, On Thu, Apr 29, 2021 at 02:57:23AM +0300, Laurent Pinchart wrote: > On Mon, Apr 19, 2021 at 02:29:20PM +0300, Tomi Valkeinen wrote: > > On 18/04/2021 15:46, Laurent Pinchart wrote: > > > On Mon, Apr 12, 2021 at 02:34:44PM +0300, Tomi Valkeinen wrote: > > >> Asserting ComplexIO reset seems to affect the HW (ie. asserting reset > > >> will break an active capture), but the RESET_DONE bit never changes to > > >> "reset is ongoing" state. Thus we always get a timeout. > > >> > > >> Drop the wait, as it seems to achieve nothing. > > >> > > >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > >> --- > > >> drivers/media/platform/ti-vpe/cal-camerarx.c | 15 ++------------- > > >> 1 file changed, 2 insertions(+), 13 deletions(-) > > >> > > >> diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c > > >> index 0354f311c5d2..245c601b992c 100644 > > >> --- a/drivers/media/platform/ti-vpe/cal-camerarx.c > > >> +++ b/drivers/media/platform/ti-vpe/cal-camerarx.c > > >> @@ -405,7 +405,6 @@ static int cal_camerarx_start(struct cal_camerarx *phy) > > >> > > >> static void cal_camerarx_stop(struct cal_camerarx *phy) > > >> { > > >> - unsigned int i; > > >> int ret; > > >> > > >> cal_camerarx_ppi_disable(phy); > > >> @@ -419,19 +418,9 @@ static void cal_camerarx_stop(struct cal_camerarx *phy) > > >> CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL, > > >> CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_MASK); > > >> > > >> - /* Wait for power down completion */ > > >> - for (i = 0; i < 10; i++) { > > >> - if (cal_read_field(phy->cal, > > >> - CAL_CSI2_COMPLEXIO_CFG(phy->instance), > > >> - CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_MASK) == > > >> - CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_RESETONGOING) > > > > > > Isn't this the wrong condition ? I would have expected > > > CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_RESETCOMPLETED, not > > > CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_RESETONGOING. That could explain why > > > you always get a timeout. > > > > No, I don't think so. The complexio reset is set active just before the > > wait. So the reset status should show reset ongoing, until at some point > > we release the reset (we do that when starting the PHY again). > > > > The TRM doesn't talk about this, though. So, I guess the status might go > > to RESETONGOING for a very short time and back to RESETCOMPLETED before > > the code can see the RESETONGOING. But I suspect the status just stays > > at RESETCOMPLETED. > > The TRM is indeed not very clear. My understanding was that asserting > RESET_CTRL initiates the reset, and RESET_DONE switches to 1 once the > reset completes. There's however a note in the initialization sequence > that states > > a. Deassert the CAMERARX reset: > i. Set CAL_CSI2_COMPLEXIO_CFG_j[30] RESET_CTRL to 0x1. > CAUTION > For the CAL_CSI2_COMPLEXIO_CFG_j[29] RESET_DONE bit to be set to 0x1 > (reset completed), the external sensor must to be active and sending the > MIPI HS BYTECLK (that is, RXBYTECLKHS clock). > > The RESET_DONE bit may thus only toggle when de-asserting the reset > signal (by setting RESET_CTRL to 1). It would be useful to test that > hypothesis by reading RESET_DONE just before setting RESET_CTRL to 1, > and right after. I'd expect the values to be 0 and 1 respectively. If > that's the case, then this patch is likely correct, so > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > The register macros are quite confusing by the way. We have > > #define CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_MASK BIT(30) > #define CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL 0 > #define CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_OPERATIONAL 1 > > When reading the code, I thought > > cal_write_field(phy->cal, CAL_CSI2_COMPLEXIO_CFG(phy->instance), > CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL, > CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_MASK) > > meant that we were setting the reset bit to 1. I would personally get > rid of the _MASK suffixes for single bits, and use 0 and 1 in the code > instead of CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL and > CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_OPERATIONAL. Do you think this would be a good idea ? It can be done in a follow-up patch. -- Regards, Laurent Pinchart
On 04/06/2021 16:44, Laurent Pinchart wrote: > Hi Tomi, > > On Thu, Apr 29, 2021 at 02:57:23AM +0300, Laurent Pinchart wrote: >> On Mon, Apr 19, 2021 at 02:29:20PM +0300, Tomi Valkeinen wrote: >>> On 18/04/2021 15:46, Laurent Pinchart wrote: >>>> On Mon, Apr 12, 2021 at 02:34:44PM +0300, Tomi Valkeinen wrote: >>>>> Asserting ComplexIO reset seems to affect the HW (ie. asserting reset >>>>> will break an active capture), but the RESET_DONE bit never changes to >>>>> "reset is ongoing" state. Thus we always get a timeout. >>>>> >>>>> Drop the wait, as it seems to achieve nothing. >>>>> >>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >>>>> --- >>>>> drivers/media/platform/ti-vpe/cal-camerarx.c | 15 ++------------- >>>>> 1 file changed, 2 insertions(+), 13 deletions(-) >>>>> >>>>> diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c >>>>> index 0354f311c5d2..245c601b992c 100644 >>>>> --- a/drivers/media/platform/ti-vpe/cal-camerarx.c >>>>> +++ b/drivers/media/platform/ti-vpe/cal-camerarx.c >>>>> @@ -405,7 +405,6 @@ static int cal_camerarx_start(struct cal_camerarx *phy) >>>>> >>>>> static void cal_camerarx_stop(struct cal_camerarx *phy) >>>>> { >>>>> - unsigned int i; >>>>> int ret; >>>>> >>>>> cal_camerarx_ppi_disable(phy); >>>>> @@ -419,19 +418,9 @@ static void cal_camerarx_stop(struct cal_camerarx *phy) >>>>> CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL, >>>>> CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_MASK); >>>>> >>>>> - /* Wait for power down completion */ >>>>> - for (i = 0; i < 10; i++) { >>>>> - if (cal_read_field(phy->cal, >>>>> - CAL_CSI2_COMPLEXIO_CFG(phy->instance), >>>>> - CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_MASK) == >>>>> - CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_RESETONGOING) >>>> >>>> Isn't this the wrong condition ? I would have expected >>>> CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_RESETCOMPLETED, not >>>> CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_RESETONGOING. That could explain why >>>> you always get a timeout. >>> >>> No, I don't think so. The complexio reset is set active just before the >>> wait. So the reset status should show reset ongoing, until at some point >>> we release the reset (we do that when starting the PHY again). >>> >>> The TRM doesn't talk about this, though. So, I guess the status might go >>> to RESETONGOING for a very short time and back to RESETCOMPLETED before >>> the code can see the RESETONGOING. But I suspect the status just stays >>> at RESETCOMPLETED. >> >> The TRM is indeed not very clear. My understanding was that asserting >> RESET_CTRL initiates the reset, and RESET_DONE switches to 1 once the >> reset completes. There's however a note in the initialization sequence >> that states >> >> a. Deassert the CAMERARX reset: >> i. Set CAL_CSI2_COMPLEXIO_CFG_j[30] RESET_CTRL to 0x1. >> CAUTION >> For the CAL_CSI2_COMPLEXIO_CFG_j[29] RESET_DONE bit to be set to 0x1 >> (reset completed), the external sensor must to be active and sending the >> MIPI HS BYTECLK (that is, RXBYTECLKHS clock). >> >> The RESET_DONE bit may thus only toggle when de-asserting the reset >> signal (by setting RESET_CTRL to 1). It would be useful to test that >> hypothesis by reading RESET_DONE just before setting RESET_CTRL to 1, >> and right after. I'd expect the values to be 0 and 1 respectively. If >> that's the case, then this patch is likely correct, so >> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> >> >> The register macros are quite confusing by the way. We have >> >> #define CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_MASK BIT(30) >> #define CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL 0 >> #define CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_OPERATIONAL 1 >> >> When reading the code, I thought >> >> cal_write_field(phy->cal, CAL_CSI2_COMPLEXIO_CFG(phy->instance), >> CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL, >> CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_MASK) >> >> meant that we were setting the reset bit to 1. I would personally get >> rid of the _MASK suffixes for single bits, and use 0 and 1 in the code >> instead of CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL and >> CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_OPERATIONAL. > > Do you think this would be a good idea ? It can be done in a follow-up > patch. I'd rather keep the MASK prefix as it's used for multiple bit masks too. I think the problem here is the define for 0. The define should tell what the bit value does, but here it's just the field name. The value defines could perhaps be: #define CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_ASSERT 0 #define CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_DEASSERT 1 Using just 0 or 1 may work fine at times, but often you don't know what they mean. You set 1 to RESET_CTRL. Does it put the IP into reset? Or release the reset? Some other bits in cal_regs.h are fine, like: #define CAL_CTRL_PWRSCPCLK_MASK BIT(21) #define CAL_CTRL_PWRSCPCLK_AUTO 0 #define CAL_CTRL_PWRSCPCLK_FORCE 1 But some have this same issue: #define CAL_CTRL_POSTED_WRITES_MASK BIT(0) #define CAL_CTRL_POSTED_WRITES_NONPOSTED 0 #define CAL_CTRL_POSTED_WRITES 1 Tomi
Hi Tomi, On Mon, Jun 07, 2021 at 01:41:07PM +0300, Tomi Valkeinen wrote: > On 04/06/2021 16:44, Laurent Pinchart wrote: > > On Thu, Apr 29, 2021 at 02:57:23AM +0300, Laurent Pinchart wrote: > >> On Mon, Apr 19, 2021 at 02:29:20PM +0300, Tomi Valkeinen wrote: > >>> On 18/04/2021 15:46, Laurent Pinchart wrote: > >>>> On Mon, Apr 12, 2021 at 02:34:44PM +0300, Tomi Valkeinen wrote: > >>>>> Asserting ComplexIO reset seems to affect the HW (ie. asserting reset > >>>>> will break an active capture), but the RESET_DONE bit never changes to > >>>>> "reset is ongoing" state. Thus we always get a timeout. > >>>>> > >>>>> Drop the wait, as it seems to achieve nothing. > >>>>> > >>>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > >>>>> --- > >>>>> drivers/media/platform/ti-vpe/cal-camerarx.c | 15 ++------------- > >>>>> 1 file changed, 2 insertions(+), 13 deletions(-) > >>>>> > >>>>> diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c > >>>>> index 0354f311c5d2..245c601b992c 100644 > >>>>> --- a/drivers/media/platform/ti-vpe/cal-camerarx.c > >>>>> +++ b/drivers/media/platform/ti-vpe/cal-camerarx.c > >>>>> @@ -405,7 +405,6 @@ static int cal_camerarx_start(struct cal_camerarx *phy) > >>>>> > >>>>> static void cal_camerarx_stop(struct cal_camerarx *phy) > >>>>> { > >>>>> - unsigned int i; > >>>>> int ret; > >>>>> > >>>>> cal_camerarx_ppi_disable(phy); > >>>>> @@ -419,19 +418,9 @@ static void cal_camerarx_stop(struct cal_camerarx *phy) > >>>>> CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL, > >>>>> CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_MASK); > >>>>> > >>>>> - /* Wait for power down completion */ > >>>>> - for (i = 0; i < 10; i++) { > >>>>> - if (cal_read_field(phy->cal, > >>>>> - CAL_CSI2_COMPLEXIO_CFG(phy->instance), > >>>>> - CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_MASK) == > >>>>> - CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_RESETONGOING) > >>>> > >>>> Isn't this the wrong condition ? I would have expected > >>>> CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_RESETCOMPLETED, not > >>>> CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_RESETONGOING. That could explain why > >>>> you always get a timeout. > >>> > >>> No, I don't think so. The complexio reset is set active just before the > >>> wait. So the reset status should show reset ongoing, until at some point > >>> we release the reset (we do that when starting the PHY again). > >>> > >>> The TRM doesn't talk about this, though. So, I guess the status might go > >>> to RESETONGOING for a very short time and back to RESETCOMPLETED before > >>> the code can see the RESETONGOING. But I suspect the status just stays > >>> at RESETCOMPLETED. > >> > >> The TRM is indeed not very clear. My understanding was that asserting > >> RESET_CTRL initiates the reset, and RESET_DONE switches to 1 once the > >> reset completes. There's however a note in the initialization sequence > >> that states > >> > >> a. Deassert the CAMERARX reset: > >> i. Set CAL_CSI2_COMPLEXIO_CFG_j[30] RESET_CTRL to 0x1. > >> CAUTION > >> For the CAL_CSI2_COMPLEXIO_CFG_j[29] RESET_DONE bit to be set to 0x1 > >> (reset completed), the external sensor must to be active and sending the > >> MIPI HS BYTECLK (that is, RXBYTECLKHS clock). > >> > >> The RESET_DONE bit may thus only toggle when de-asserting the reset > >> signal (by setting RESET_CTRL to 1). It would be useful to test that > >> hypothesis by reading RESET_DONE just before setting RESET_CTRL to 1, > >> and right after. I'd expect the values to be 0 and 1 respectively. If > >> that's the case, then this patch is likely correct, so > >> > >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> > >> > >> The register macros are quite confusing by the way. We have > >> > >> #define CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_MASK BIT(30) > >> #define CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL 0 > >> #define CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_OPERATIONAL 1 > >> > >> When reading the code, I thought > >> > >> cal_write_field(phy->cal, CAL_CSI2_COMPLEXIO_CFG(phy->instance), > >> CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL, > >> CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_MASK) > >> > >> meant that we were setting the reset bit to 1. I would personally get > >> rid of the _MASK suffixes for single bits, and use 0 and 1 in the code > >> instead of CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL and > >> CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_OPERATIONAL. > > > > Do you think this would be a good idea ? It can be done in a follow-up > > patch. > > I'd rather keep the MASK prefix as it's used for multiple bit masks too. > > I think the problem here is the define for 0. The define should tell > what the bit value does, but here it's just the field name. The value > defines could perhaps be: > > #define CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_ASSERT 0 > #define CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_DEASSERT 1 > > Using just 0 or 1 may work fine at times, but often you don't know what > they mean. You set 1 to RESET_CTRL. Does it put the IP into reset? Or > release the reset? I have a small preference for dropping _MASK for single bits, but the above is fine with me too, it's entirely up to you. > Some other bits in cal_regs.h are fine, like: > > #define CAL_CTRL_PWRSCPCLK_MASK BIT(21) > #define CAL_CTRL_PWRSCPCLK_AUTO 0 > #define CAL_CTRL_PWRSCPCLK_FORCE 1 > > But some have this same issue: > > #define CAL_CTRL_POSTED_WRITES_MASK BIT(0) > #define CAL_CTRL_POSTED_WRITES_NONPOSTED 0 > #define CAL_CTRL_POSTED_WRITES 1 -- Regards, Laurent Pinchart
diff --git a/drivers/media/platform/ti-vpe/cal-camerarx.c b/drivers/media/platform/ti-vpe/cal-camerarx.c index 0354f311c5d2..245c601b992c 100644 --- a/drivers/media/platform/ti-vpe/cal-camerarx.c +++ b/drivers/media/platform/ti-vpe/cal-camerarx.c @@ -405,7 +405,6 @@ static int cal_camerarx_start(struct cal_camerarx *phy) static void cal_camerarx_stop(struct cal_camerarx *phy) { - unsigned int i; int ret; cal_camerarx_ppi_disable(phy); @@ -419,19 +418,9 @@ static void cal_camerarx_stop(struct cal_camerarx *phy) CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL, CAL_CSI2_COMPLEXIO_CFG_RESET_CTRL_MASK); - /* Wait for power down completion */ - for (i = 0; i < 10; i++) { - if (cal_read_field(phy->cal, - CAL_CSI2_COMPLEXIO_CFG(phy->instance), - CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_MASK) == - CAL_CSI2_COMPLEXIO_CFG_RESET_DONE_RESETONGOING) - break; - usleep_range(1000, 1100); - } - phy_dbg(3, phy, "CAL_CSI2_COMPLEXIO_CFG(%d) = 0x%08x Complex IO in Reset (%d) %s\n", + phy_dbg(3, phy, "CAL_CSI2_COMPLEXIO_CFG(%d) = 0x%08x Complex IO in Reset\n", phy->instance, - cal_read(phy->cal, CAL_CSI2_COMPLEXIO_CFG(phy->instance)), i, - (i >= 10) ? "(timeout)" : ""); + cal_read(phy->cal, CAL_CSI2_COMPLEXIO_CFG(phy->instance))); /* Disable the phy */ cal_camerarx_disable(phy);
Asserting ComplexIO reset seems to affect the HW (ie. asserting reset will break an active capture), but the RESET_DONE bit never changes to "reset is ongoing" state. Thus we always get a timeout. Drop the wait, as it seems to achieve nothing. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- drivers/media/platform/ti-vpe/cal-camerarx.c | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-)