| Message ID | 20190316154801.20460-3-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | media: Implement negotiation of CSI-2 data lanes | expand |
Hi Jacopo, On 16/03/2019 15:47, Jacopo Mondi wrote: > Post-pone the write of the ADV748X_IO_10 register that controls the active > routing between the CP and AFE cores to the 4-lanes CSI-2 TXA at TX > power-up time. "by caching its configuration in the driver state." > > While at there, use the 'csi4_in_sel' name, which matches the register 'While at it, use the...' Except I'm not sure csi4_in_sel is the right name for the cached values as below... > field description in the manual, in place of 'io_10' which is the full > register name. > This has a fixes tag, but doesn't state what the actual problem is? Can I assume that the problem is that the configuration here is being written to the hardware before it is powered up or such? Or perhaps reading through the patch again, is it that the call to link_setup can affect running streams? > Fixes: 9423ca350df7 ("media: adv748x: Implement TX link_setup callback") > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > drivers/media/i2c/adv748x/adv748x-core.c | 53 ++++++++++++++---------- > drivers/media/i2c/adv748x/adv748x.h | 2 + > 2 files changed, 33 insertions(+), 22 deletions(-) > > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c > index 0e5a75eb6d75..02135741b1a6 100644 > --- a/drivers/media/i2c/adv748x/adv748x-core.c > +++ b/drivers/media/i2c/adv748x/adv748x-core.c > @@ -305,23 +305,35 @@ static int adv748x_power_down_tx(struct adv748x_csi2 *tx) > > int adv748x_tx_power(struct adv748x_csi2 *tx, bool on) > { > - int val; > + u8 io10_mask = ADV748X_IO_10_CSI1_EN | ADV748X_IO_10_CSI4_EN | > + ADV748X_IO_10_CSI4_IN_SEL_AFE; > + struct adv748x_state *state = tx->state; > + int ret; > > if (!is_tx_enabled(tx)) > return 0; > > - val = tx_read(tx, ADV748X_CSI_FS_AS_LS); > - if (val < 0) > - return val; > + ret = tx_read(tx, ADV748X_CSI_FS_AS_LS); > + if (ret < 0) > + return ret; > > /* > * This test against BIT(6) is not documented by the datasheet, but was > * specified in the downstream driver. > * Track with a WARN_ONCE to determine if it is ever set by HW. > */ > - WARN_ONCE((on && val & ADV748X_CSI_FS_AS_LS_UNKNOWN), > + WARN_ONCE((on && ret & ADV748X_CSI_FS_AS_LS_UNKNOWN), > "Enabling with unknown bit set"); > > + /* Configure TXA routing. */ Should TXA routing be configured even on TXB power up? This function handles both TX code paths. (Edit: possibly yes) Can the logic that determines state->csi4_in_sel value simply be moved here (or to an independent adv748x_configure_routing() function)? I think this patch means that changes to routing will now only take effect when starting or stopping a stream, is that right? (If so - could that go into the commit message please?) > + if (on) { > + ret = io_clrset(state, ADV748X_IO_10, io10_mask, > + state->csi4_in_sel); > + if (ret) > + return ret; > + } > + > + > return on ? adv748x_power_up_tx(tx) : adv748x_power_down_tx(tx); > } > > @@ -337,10 +349,7 @@ static int adv748x_link_setup(struct media_entity *entity, > struct adv748x_state *state = v4l2_get_subdevdata(sd); > struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd); > bool enable = flags & MEDIA_LNK_FL_ENABLED; > - u8 io10_mask = ADV748X_IO_10_CSI1_EN | > - ADV748X_IO_10_CSI4_EN | > - ADV748X_IO_10_CSI4_IN_SEL_AFE; > - u8 io10 = 0; > + u8 csi4_in_sel = 0; > > /* Refuse to enable multiple links to the same TX at the same time. */ > if (enable && tx->src) > @@ -359,17 +368,19 @@ static int adv748x_link_setup(struct media_entity *entity, > > if (state->afe.tx) { > /* AFE Requires TXA enabled, even when output to TXB */ > - io10 |= ADV748X_IO_10_CSI4_EN; > + csi4_in_sel |= ADV748X_IO_10_CSI4_EN; > if (is_txa(tx)) > - io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE; > + csi4_in_sel |= ADV748X_IO_10_CSI4_IN_SEL_AFE; Hrm ... this is the one part that I think can't be determined without caching some sort of value to state the routing. > else > - io10 |= ADV748X_IO_10_CSI1_EN; > + csi4_in_sel |= ADV748X_IO_10_CSI1_EN; > } > > if (state->hdmi.tx) > - io10 |= ADV748X_IO_10_CSI4_EN; > + csi4_in_sel |= ADV748X_IO_10_CSI4_EN; > > - return io_clrset(state, ADV748X_IO_10, io10_mask, io10); > + state->csi4_in_sel = csi4_in_sel; > + > + return 0; > } > > static const struct media_entity_operations adv748x_tx_media_ops = { > @@ -485,7 +496,6 @@ static int adv748x_sw_reset(struct adv748x_state *state) > static int adv748x_reset(struct adv748x_state *state) > { > int ret; > - u8 regval = 0; > > ret = adv748x_sw_reset(state); > if (ret < 0) > @@ -504,6 +514,12 @@ static int adv748x_reset(struct adv748x_state *state) > if (ret) > return ret; > Should adv748x_reset() reset the state->csi4_in_sel cached value to 0 before setting it? > + /* Conditionally enable TXa and TXb. */ > + if (is_tx_enabled(&state->txa)) > + state->csi4_in_sel |= ADV748X_IO_10_CSI4_EN; > + if (is_tx_enabled(&state->txb)) > + state->csi4_in_sel |= ADV748X_IO_10_CSI1_EN; This makes it looks like the naming "csi4_in_sel" is less appropriate, as it covers both CSI4 and CSI1... Also, this is confusing two terms, between the 'enable' and the 'select' The _EN bits looks like they control the activation of the CSI transmitter, where as the 'select' bits control the routing. As the is_tx_enabled($TX) state is constant, perhaps that bit could be inferred later when the register is written, and doesn't need to be cached here? > + > /* Reset TXA and TXB */ > adv748x_tx_power(&state->txa, 1); > adv748x_tx_power(&state->txa, 0); > @@ -513,13 +529,6 @@ static int adv748x_reset(struct adv748x_state *state) > /* Disable chip powerdown & Enable HDMI Rx block */ > io_write(state, ADV748X_IO_PD, ADV748X_IO_PD_RX_EN); > > - /* Conditionally enable TXa and TXb. */ > - if (is_tx_enabled(&state->txa)) > - regval |= ADV748X_IO_10_CSI4_EN; > - if (is_tx_enabled(&state->txb)) > - regval |= ADV748X_IO_10_CSI1_EN; > - io_write(state, ADV748X_IO_10, regval); > - > /* Use vid_std and v_freq as freerun resolution for CP */ > cp_clrset(state, ADV748X_CP_CLMP_POS, ADV748X_CP_CLMP_POS_DIS_AUTO, > ADV748X_CP_CLMP_POS_DIS_AUTO); > diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h > index 4a5a6445604f..27c116d09284 100644 > --- a/drivers/media/i2c/adv748x/adv748x.h > +++ b/drivers/media/i2c/adv748x/adv748x.h > @@ -196,6 +196,8 @@ struct adv748x_state { > struct adv748x_afe afe; > struct adv748x_csi2 txa; > struct adv748x_csi2 txb; > + > + unsigned int csi4_in_sel; > }; > > #define adv748x_hdmi_to_state(h) container_of(h, struct adv748x_state, hdmi) > -- > 2.21.0 >
Hi Kieran, On Fri, Apr 12, 2019 at 03:15:46PM +0100, Kieran Bingham wrote: > Hi Jacopo, > > On 16/03/2019 15:47, Jacopo Mondi wrote: > > Post-pone the write of the ADV748X_IO_10 register that controls the active > > routing between the CP and AFE cores to the 4-lanes CSI-2 TXA at TX > > power-up time. > > "by caching its configuration in the driver state." > > > > > While at there, use the 'csi4_in_sel' name, which matches the register > > 'While at it, use the...' > > Except I'm not sure csi4_in_sel is the right name for the cached values > as below... > > > > field description in the manual, in place of 'io_10' which is the full > > register name. > > > > This has a fixes tag, but doesn't state what the actual problem is? > As reported in the cover letter, please see: "[PATCH] media: adv748x: Don't disable CSI-2 on link_setup" > Can I assume that the problem is that the configuration here is being > written to the hardware before it is powered up or such? > > Or perhaps reading through the patch again, is it that the call to > link_setup can affect running streams? The issue I was trying to solve, even with the first patch is that going through a link disable (eg. at media graph reset time) wrote to the csi4_in_sel register a 0 value, when both TXA and TXB routing where disabled and this causes issues on some HDMI transmiters, for reason Ian and Hans tried to expand but about which I'm not yet sure about. The idea here is to cache the routing settings at link_setup time (upon activation or deactivation of a link) and apply them to hardware at tx power up time, which happens at s_stream(1). In this way, if streaming is started, we know at least one link is enabled and we can write to csi4_in_sel. > > > > > Fixes: 9423ca350df7 ("media: adv748x: Implement TX link_setup callback") > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > drivers/media/i2c/adv748x/adv748x-core.c | 53 ++++++++++++++---------- > > drivers/media/i2c/adv748x/adv748x.h | 2 + > > 2 files changed, 33 insertions(+), 22 deletions(-) > > > > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c > > index 0e5a75eb6d75..02135741b1a6 100644 > > --- a/drivers/media/i2c/adv748x/adv748x-core.c > > +++ b/drivers/media/i2c/adv748x/adv748x-core.c > > @@ -305,23 +305,35 @@ static int adv748x_power_down_tx(struct adv748x_csi2 *tx) > > > > int adv748x_tx_power(struct adv748x_csi2 *tx, bool on) > > { > > - int val; > > + u8 io10_mask = ADV748X_IO_10_CSI1_EN | ADV748X_IO_10_CSI4_EN | > > + ADV748X_IO_10_CSI4_IN_SEL_AFE; > > + struct adv748x_state *state = tx->state; > > + int ret; > > > > if (!is_tx_enabled(tx)) > > return 0; > > > > - val = tx_read(tx, ADV748X_CSI_FS_AS_LS); > > - if (val < 0) > > - return val; > > + ret = tx_read(tx, ADV748X_CSI_FS_AS_LS); > > + if (ret < 0) > > + return ret; > > > > /* > > * This test against BIT(6) is not documented by the datasheet, but was > > * specified in the downstream driver. > > * Track with a WARN_ONCE to determine if it is ever set by HW. > > */ > > - WARN_ONCE((on && val & ADV748X_CSI_FS_AS_LS_UNKNOWN), > > + WARN_ONCE((on && ret & ADV748X_CSI_FS_AS_LS_UNKNOWN), > > "Enabling with unknown bit set"); > > > > + /* Configure TXA routing. */ > > Should TXA routing be configured even on TXB power up? This function > handles both TX code paths. (Edit: possibly yes) > The comment is wrong, thanks for noticing. > Can the logic that determines state->csi4_in_sel value simply be moved > here (or to an independent adv748x_configure_routing() function)? > It has to be changed at link_setup time in rensponse to a media link activation or deactivation. > I think this patch means that changes to routing will now only take > effect when starting or stopping a stream, is that right? (If so - could > that go into the commit message please?) > Well... Post-pone the write of the ADV748X_IO_10 register that controls the active routing between the CP and AFE cores to the 4-lanes CSI-2 TXA at TX power-up time. Doesn't it say that? Or what confused you is that s_stream->s_power(1) and I should mention streaming instead of power up? > > > > > + if (on) { > > + ret = io_clrset(state, ADV748X_IO_10, io10_mask, > > + state->csi4_in_sel); > > + if (ret) > > + return ret; > > + } > > + > > + > > return on ? adv748x_power_up_tx(tx) : adv748x_power_down_tx(tx); > > } > > > > @@ -337,10 +349,7 @@ static int adv748x_link_setup(struct media_entity *entity, > > struct adv748x_state *state = v4l2_get_subdevdata(sd); > > struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd); > > bool enable = flags & MEDIA_LNK_FL_ENABLED; > > - u8 io10_mask = ADV748X_IO_10_CSI1_EN | > > - ADV748X_IO_10_CSI4_EN | > > - ADV748X_IO_10_CSI4_IN_SEL_AFE; > > - u8 io10 = 0; > > + u8 csi4_in_sel = 0; > > > > /* Refuse to enable multiple links to the same TX at the same time. */ > > if (enable && tx->src) > > @@ -359,17 +368,19 @@ static int adv748x_link_setup(struct media_entity *entity, > > > > if (state->afe.tx) { > > /* AFE Requires TXA enabled, even when output to TXB */ > > - io10 |= ADV748X_IO_10_CSI4_EN; > > + csi4_in_sel |= ADV748X_IO_10_CSI4_EN; > > if (is_txa(tx)) > > - io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE; > > + csi4_in_sel |= ADV748X_IO_10_CSI4_IN_SEL_AFE; > > Hrm ... this is the one part that I think can't be determined without > caching some sort of value to state the routing. > Yes > > else > > - io10 |= ADV748X_IO_10_CSI1_EN; > > + csi4_in_sel |= ADV748X_IO_10_CSI1_EN; > > } > > > > if (state->hdmi.tx) > > - io10 |= ADV748X_IO_10_CSI4_EN; > > + csi4_in_sel |= ADV748X_IO_10_CSI4_EN; > > > > - return io_clrset(state, ADV748X_IO_10, io10_mask, io10); > > + state->csi4_in_sel = csi4_in_sel; > > + > > + return 0; > > } > > > > static const struct media_entity_operations adv748x_tx_media_ops = { > > @@ -485,7 +496,6 @@ static int adv748x_sw_reset(struct adv748x_state *state) > > static int adv748x_reset(struct adv748x_state *state) > > { > > int ret; > > - u8 regval = 0; > > > > ret = adv748x_sw_reset(state); > > if (ret < 0) > > @@ -504,6 +514,12 @@ static int adv748x_reset(struct adv748x_state *state) > > if (ret) > > return ret; > > > > Should adv748x_reset() reset the state->csi4_in_sel cached value to 0 > before setting it? I should better check when reset happens, and if it happens only when links have been disabled or not. If links are disabled, the variable gets zeroed by the link_setup callback. If reset happens with links active, we should not zero it otherwise we lose the configuration > > > > + /* Conditionally enable TXa and TXb. */ > > + if (is_tx_enabled(&state->txa)) > > + state->csi4_in_sel |= ADV748X_IO_10_CSI4_EN; > > + if (is_tx_enabled(&state->txb)) > > + state->csi4_in_sel |= ADV748X_IO_10_CSI1_EN; > > This makes it looks like the naming "csi4_in_sel" is less appropriate, > as it covers both CSI4 and CSI1... > Blame the hw designers :) > Also, this is confusing two terms, between the 'enable' and the 'select' > > The _EN bits looks like they control the activation of the CSI > transmitter, where as the 'select' bits control the routing. > You are righ. csi4_in_sel should only control the 3 bits used for routing, while enabling and disabling of the TXes is controlled by other bits of the io_10 register. I'll look into changing the name back > As the is_tx_enabled($TX) state is constant, perhaps that bit could be > inferred later when the register is written, and doesn't need to be > cached here? I'll consider that, thanks Thanks j > > > > + > > /* Reset TXA and TXB */ > > adv748x_tx_power(&state->txa, 1); > > adv748x_tx_power(&state->txa, 0); > > @@ -513,13 +529,6 @@ static int adv748x_reset(struct adv748x_state *state) > > /* Disable chip powerdown & Enable HDMI Rx block */ > > io_write(state, ADV748X_IO_PD, ADV748X_IO_PD_RX_EN); > > > > - /* Conditionally enable TXa and TXb. */ > > - if (is_tx_enabled(&state->txa)) > > - regval |= ADV748X_IO_10_CSI4_EN; > > - if (is_tx_enabled(&state->txb)) > > - regval |= ADV748X_IO_10_CSI1_EN; > > - io_write(state, ADV748X_IO_10, regval); > > - > > /* Use vid_std and v_freq as freerun resolution for CP */ > > cp_clrset(state, ADV748X_CP_CLMP_POS, ADV748X_CP_CLMP_POS_DIS_AUTO, > > ADV748X_CP_CLMP_POS_DIS_AUTO); > > diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h > > index 4a5a6445604f..27c116d09284 100644 > > --- a/drivers/media/i2c/adv748x/adv748x.h > > +++ b/drivers/media/i2c/adv748x/adv748x.h > > @@ -196,6 +196,8 @@ struct adv748x_state { > > struct adv748x_afe afe; > > struct adv748x_csi2 txa; > > struct adv748x_csi2 txb; > > + > > + unsigned int csi4_in_sel; > > }; > > > > #define adv748x_hdmi_to_state(h) container_of(h, struct adv748x_state, hdmi) > > -- > > 2.21.0 > > > > -- > Regards > -- > Kieran
Hi Jacopo, On 12/04/2019 15:45, Jacopo Mondi wrote: > Hi Kieran, > > On Fri, Apr 12, 2019 at 03:15:46PM +0100, Kieran Bingham wrote: >> Hi Jacopo, >> >> On 16/03/2019 15:47, Jacopo Mondi wrote: >>> Post-pone the write of the ADV748X_IO_10 register that controls the active >>> routing between the CP and AFE cores to the 4-lanes CSI-2 TXA at TX >>> power-up time. >> >> "by caching its configuration in the driver state." >> >>> >>> While at there, use the 'csi4_in_sel' name, which matches the register >> >> 'While at it, use the...' >> >> Except I'm not sure csi4_in_sel is the right name for the cached values >> as below... >> >> >>> field description in the manual, in place of 'io_10' which is the full >>> register name. >>> >> >> This has a fixes tag, but doesn't state what the actual problem is? >> > > As reported in the cover letter, please see: > "[PATCH] media: adv748x: Don't disable CSI-2 on link_setup" Ah I see, I had missed that. The patch itself should still describe the problem if it's fixing something. The cover letter will not be available in the git-log. Ok, so this patch supersedes "[PATCH] media: adv748x: Don't disable CSI-2 on link_setup" ? >> Can I assume that the problem is that the configuration here is being >> written to the hardware before it is powered up or such? >> >> Or perhaps reading through the patch again, is it that the call to >> link_setup can affect running streams? > > The issue I was trying to solve, even with the first patch is that > going through a link disable (eg. at media graph reset time) wrote to > the csi4_in_sel register a 0 value, when both TXA and TXB routing > where disabled and this causes issues on some HDMI transmiters, for > reason Ian and Hans tried to expand but about which I'm not yet sure > about. Ok, I found that patch and read their comments. So disabling the CSI2 might trigger a hot-plug event to the transmitter which makes them think they have been (physically) disconnected in some way, or triggers them to re-read the EDID which will fail. But we don't really know what the fault is yet. > The idea here is to cache the routing settings at link_setup time > (upon activation or deactivation of a link) and apply them to hardware > at tx power up time, which happens at s_stream(1). > > In this way, if streaming is started, we know at least one link is > enabled and we can write to csi4_in_sel. Overall this seems reasonable, only making a change to io10 when either of the stream states are changed. >>> Fixes: 9423ca350df7 ("media: adv748x: Implement TX link_setup callback") >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >>> --- >>> drivers/media/i2c/adv748x/adv748x-core.c | 53 ++++++++++++++---------- >>> drivers/media/i2c/adv748x/adv748x.h | 2 + >>> 2 files changed, 33 insertions(+), 22 deletions(-) >>> >>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c >>> index 0e5a75eb6d75..02135741b1a6 100644 >>> --- a/drivers/media/i2c/adv748x/adv748x-core.c >>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c >>> @@ -305,23 +305,35 @@ static int adv748x_power_down_tx(struct adv748x_csi2 *tx) >>> >>> int adv748x_tx_power(struct adv748x_csi2 *tx, bool on) >>> { >>> - int val; >>> + u8 io10_mask = ADV748X_IO_10_CSI1_EN | ADV748X_IO_10_CSI4_EN | >>> + ADV748X_IO_10_CSI4_IN_SEL_AFE; >>> + struct adv748x_state *state = tx->state; >>> + int ret; >>> >>> if (!is_tx_enabled(tx)) >>> return 0; >>> >>> - val = tx_read(tx, ADV748X_CSI_FS_AS_LS); >>> - if (val < 0) >>> - return val; >>> + ret = tx_read(tx, ADV748X_CSI_FS_AS_LS); >>> + if (ret < 0) >>> + return ret; >>> >>> /* >>> * This test against BIT(6) is not documented by the datasheet, but was >>> * specified in the downstream driver. >>> * Track with a WARN_ONCE to determine if it is ever set by HW. >>> */ >>> - WARN_ONCE((on && val & ADV748X_CSI_FS_AS_LS_UNKNOWN), >>> + WARN_ONCE((on && ret & ADV748X_CSI_FS_AS_LS_UNKNOWN), >>> "Enabling with unknown bit set"); >>> >>> + /* Configure TXA routing. */ >> >> Should TXA routing be configured even on TXB power up? This function >> handles both TX code paths. (Edit: possibly yes) >> > > The comment is wrong, thanks for noticing. > >> Can the logic that determines state->csi4_in_sel value simply be moved >> here (or to an independent adv748x_configure_routing() function)? >> > > It has to be changed at link_setup time in rensponse to a media link > activation or deactivation. > >> I think this patch means that changes to routing will now only take >> effect when starting or stopping a stream, is that right? (If so - could >> that go into the commit message please?) >> > > Well... > > Post-pone the write of the ADV748X_IO_10 register that controls the active > routing between the CP and AFE cores to the 4-lanes CSI-2 TXA at TX > power-up time. > > Doesn't it say that? Or what confused you is that s_stream->s_power(1) > and I should mention streaming instead of power up? I think of "Power up" meaning at device probe time (or possibly some runtime-pm event time). But yes, I think the distinction that it now happens at stream_on/stream_off is important. > >> >> >> >>> + if (on) { >>> + ret = io_clrset(state, ADV748X_IO_10, io10_mask, >>> + state->csi4_in_sel); >>> + if (ret) >>> + return ret; >>> + } >>> + >>> + >>> return on ? adv748x_power_up_tx(tx) : adv748x_power_down_tx(tx); >>> } >>> >>> @@ -337,10 +349,7 @@ static int adv748x_link_setup(struct media_entity *entity, >>> struct adv748x_state *state = v4l2_get_subdevdata(sd); >>> struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd); >>> bool enable = flags & MEDIA_LNK_FL_ENABLED; >>> - u8 io10_mask = ADV748X_IO_10_CSI1_EN | >>> - ADV748X_IO_10_CSI4_EN | >>> - ADV748X_IO_10_CSI4_IN_SEL_AFE; >>> - u8 io10 = 0; >>> + u8 csi4_in_sel = 0; >>> >>> /* Refuse to enable multiple links to the same TX at the same time. */ >>> if (enable && tx->src) >>> @@ -359,17 +368,19 @@ static int adv748x_link_setup(struct media_entity *entity, >>> >>> if (state->afe.tx) { >>> /* AFE Requires TXA enabled, even when output to TXB */ >>> - io10 |= ADV748X_IO_10_CSI4_EN; >>> + csi4_in_sel |= ADV748X_IO_10_CSI4_EN; >>> if (is_txa(tx)) >>> - io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE; >>> + csi4_in_sel |= ADV748X_IO_10_CSI4_IN_SEL_AFE; >> >> Hrm ... this is the one part that I think can't be determined without >> caching some sort of value to state the routing. >> > > Yes But the actual hardware shouldn't be updated if the stream is running right? (I wonder if the media-controller would prevent that anyway). > >>> else >>> - io10 |= ADV748X_IO_10_CSI1_EN; >>> + csi4_in_sel |= ADV748X_IO_10_CSI1_EN; >>> } >>> >>> if (state->hdmi.tx) >>> - io10 |= ADV748X_IO_10_CSI4_EN; >>> + csi4_in_sel |= ADV748X_IO_10_CSI4_EN; >>> >>> - return io_clrset(state, ADV748X_IO_10, io10_mask, io10); >>> + state->csi4_in_sel = csi4_in_sel; >>> + >>> + return 0; >>> } >>> >>> static const struct media_entity_operations adv748x_tx_media_ops = { >>> @@ -485,7 +496,6 @@ static int adv748x_sw_reset(struct adv748x_state *state) >>> static int adv748x_reset(struct adv748x_state *state) >>> { >>> int ret; >>> - u8 regval = 0; >>> >>> ret = adv748x_sw_reset(state); >>> if (ret < 0) >>> @@ -504,6 +514,12 @@ static int adv748x_reset(struct adv748x_state *state) >>> if (ret) >>> return ret; >>> >> >> Should adv748x_reset() reset the state->csi4_in_sel cached value to 0 >> before setting it? > > I should better check when reset happens, and if it happens only when > links have been disabled or not. > If links are disabled, the variable gets zeroed by the link_setup > callback. If reset happens with links active, we should not zero it > otherwise we lose the configuration Ah yes, I missed that the local variable is initialised to zero, and this state variable is set to the result at the end of the call... It does mean that the function ordering will be important here. It becomes irrelevant if these conditionals move to the same point anyway though. > >> >> >>> + /* Conditionally enable TXa and TXb. */ >>> + if (is_tx_enabled(&state->txa)) >>> + state->csi4_in_sel |= ADV748X_IO_10_CSI4_EN; >>> + if (is_tx_enabled(&state->txb)) >>> + state->csi4_in_sel |= ADV748X_IO_10_CSI1_EN; >> >> This makes it looks like the naming "csi4_in_sel" is less appropriate, >> as it covers both CSI4 and CSI1... >> > > Blame the hw designers :) Always. ... of course they keep blaming us back :D > >> Also, this is confusing two terms, between the 'enable' and the 'select' >> >> The _EN bits looks like they control the activation of the CSI >> transmitter, where as the 'select' bits control the routing. >> > > You are righ. csi4_in_sel should only control the 3 bits used for > routing, while enabling and disabling of the TXes is controlled by > other bits of the io_10 register. > I'll look into changing the name back > >> As the is_tx_enabled($TX) state is constant, perhaps that bit could be >> inferred later when the register is written, and doesn't need to be >> cached here? > > I'll consider that, thanks I think it's only the routing choice that needs to be stored in the state. That would minimise being stored 'globally', and the values could be determined at the time of programming the register. > > Thanks > j > >> >> >>> + >>> /* Reset TXA and TXB */ >>> adv748x_tx_power(&state->txa, 1); >>> adv748x_tx_power(&state->txa, 0); >>> @@ -513,13 +529,6 @@ static int adv748x_reset(struct adv748x_state *state) >>> /* Disable chip powerdown & Enable HDMI Rx block */ >>> io_write(state, ADV748X_IO_PD, ADV748X_IO_PD_RX_EN); >>> >>> - /* Conditionally enable TXa and TXb. */ >>> - if (is_tx_enabled(&state->txa)) >>> - regval |= ADV748X_IO_10_CSI4_EN; >>> - if (is_tx_enabled(&state->txb)) >>> - regval |= ADV748X_IO_10_CSI1_EN; >>> - io_write(state, ADV748X_IO_10, regval); >>> - >>> /* Use vid_std and v_freq as freerun resolution for CP */ >>> cp_clrset(state, ADV748X_CP_CLMP_POS, ADV748X_CP_CLMP_POS_DIS_AUTO, >>> ADV748X_CP_CLMP_POS_DIS_AUTO); >>> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h >>> index 4a5a6445604f..27c116d09284 100644 >>> --- a/drivers/media/i2c/adv748x/adv748x.h >>> +++ b/drivers/media/i2c/adv748x/adv748x.h >>> @@ -196,6 +196,8 @@ struct adv748x_state { >>> struct adv748x_afe afe; >>> struct adv748x_csi2 txa; >>> struct adv748x_csi2 txb; >>> + >>> + unsigned int csi4_in_sel; >>> }; >>> >>> #define adv748x_hdmi_to_state(h) container_of(h, struct adv748x_state, hdmi) >>> -- >>> 2.21.0 >>>
Hi Kieran, On Fri, Apr 12, 2019 at 04:57:45PM +0100, Kieran Bingham wrote: > Hi Jacopo, > > On 12/04/2019 15:45, Jacopo Mondi wrote: > > Hi Kieran, > > > > On Fri, Apr 12, 2019 at 03:15:46PM +0100, Kieran Bingham wrote: > >> Hi Jacopo, > >> > >> On 16/03/2019 15:47, Jacopo Mondi wrote: > >>> Post-pone the write of the ADV748X_IO_10 register that controls the active > >>> routing between the CP and AFE cores to the 4-lanes CSI-2 TXA at TX > >>> power-up time. > >> > >> "by caching its configuration in the driver state." > >> > >>> > >>> While at there, use the 'csi4_in_sel' name, which matches the register > >> > >> 'While at it, use the...' > >> > >> Except I'm not sure csi4_in_sel is the right name for the cached values > >> as below... > >> > >> > >>> field description in the manual, in place of 'io_10' which is the full > >>> register name. > >>> > >> > >> This has a fixes tag, but doesn't state what the actual problem is? > >> > > > > As reported in the cover letter, please see: > > "[PATCH] media: adv748x: Don't disable CSI-2 on link_setup" > > Ah I see, I had missed that. > > The patch itself should still describe the problem if it's fixing > something. The cover letter will not be available in the git-log. > You're right, I'll expand the commit message. > Ok, so this patch supersedes "[PATCH] media: adv748x: Don't disable > CSI-2 on link_setup" ? > Yes > >> Can I assume that the problem is that the configuration here is being > >> written to the hardware before it is powered up or such? > >> > >> Or perhaps reading through the patch again, is it that the call to > >> link_setup can affect running streams? > > > > The issue I was trying to solve, even with the first patch is that > > going through a link disable (eg. at media graph reset time) wrote to > > the csi4_in_sel register a 0 value, when both TXA and TXB routing > > where disabled and this causes issues on some HDMI transmiters, for > > reason Ian and Hans tried to expand but about which I'm not yet sure > > about. > > Ok, I found that patch and read their comments. So disabling the CSI2 > might trigger a hot-plug event to the transmitter which makes them think > they have been (physically) disconnected in some way, or triggers them > to re-read the EDID which will fail. But we don't really know what the > fault is yet. > Correct. Please note that I've seen this happening with one HDMI transmitter (a chromecast device), while if HDMI source is a laptop everything's fine... > > > The idea here is to cache the routing settings at link_setup time > > (upon activation or deactivation of a link) and apply them to hardware > > at tx power up time, which happens at s_stream(1). > > > > In this way, if streaming is started, we know at least one link is > > enabled and we can write to csi4_in_sel. > > Overall this seems reasonable, only making a change to io10 when either > of the stream states are changed. > Thanks, I'll re-send (maybe even out of this series if it gets stale). Could I have your tag on the next iteration? Thanks j > > >>> Fixes: 9423ca350df7 ("media: adv748x: Implement TX link_setup callback") > >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > >>> --- > >>> drivers/media/i2c/adv748x/adv748x-core.c | 53 ++++++++++++++---------- > >>> drivers/media/i2c/adv748x/adv748x.h | 2 + > >>> 2 files changed, 33 insertions(+), 22 deletions(-) > >>> > >>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c > >>> index 0e5a75eb6d75..02135741b1a6 100644 > >>> --- a/drivers/media/i2c/adv748x/adv748x-core.c > >>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c > >>> @@ -305,23 +305,35 @@ static int adv748x_power_down_tx(struct adv748x_csi2 *tx) > >>> > >>> int adv748x_tx_power(struct adv748x_csi2 *tx, bool on) > >>> { > >>> - int val; > >>> + u8 io10_mask = ADV748X_IO_10_CSI1_EN | ADV748X_IO_10_CSI4_EN | > >>> + ADV748X_IO_10_CSI4_IN_SEL_AFE; > >>> + struct adv748x_state *state = tx->state; > >>> + int ret; > >>> > >>> if (!is_tx_enabled(tx)) > >>> return 0; > >>> > >>> - val = tx_read(tx, ADV748X_CSI_FS_AS_LS); > >>> - if (val < 0) > >>> - return val; > >>> + ret = tx_read(tx, ADV748X_CSI_FS_AS_LS); > >>> + if (ret < 0) > >>> + return ret; > >>> > >>> /* > >>> * This test against BIT(6) is not documented by the datasheet, but was > >>> * specified in the downstream driver. > >>> * Track with a WARN_ONCE to determine if it is ever set by HW. > >>> */ > >>> - WARN_ONCE((on && val & ADV748X_CSI_FS_AS_LS_UNKNOWN), > >>> + WARN_ONCE((on && ret & ADV748X_CSI_FS_AS_LS_UNKNOWN), > >>> "Enabling with unknown bit set"); > >>> > >>> + /* Configure TXA routing. */ > >> > >> Should TXA routing be configured even on TXB power up? This function > >> handles both TX code paths. (Edit: possibly yes) > >> > > > > The comment is wrong, thanks for noticing. > > > >> Can the logic that determines state->csi4_in_sel value simply be moved > >> here (or to an independent adv748x_configure_routing() function)? > >> > > > > It has to be changed at link_setup time in rensponse to a media link > > activation or deactivation. > > > >> I think this patch means that changes to routing will now only take > >> effect when starting or stopping a stream, is that right? (If so - could > >> that go into the commit message please?) > >> > > > > Well... > > > > Post-pone the write of the ADV748X_IO_10 register that controls the active > > routing between the CP and AFE cores to the 4-lanes CSI-2 TXA at TX > > power-up time. > > > > Doesn't it say that? Or what confused you is that s_stream->s_power(1) > > and I should mention streaming instead of power up? > > I think of "Power up" meaning at device probe time (or possibly some > runtime-pm event time). But yes, I think the distinction that it now > happens at stream_on/stream_off is important. > > > > > > >> > >> > >> > >>> + if (on) { > >>> + ret = io_clrset(state, ADV748X_IO_10, io10_mask, > >>> + state->csi4_in_sel); > >>> + if (ret) > >>> + return ret; > >>> + } > >>> + > >>> + > >>> return on ? adv748x_power_up_tx(tx) : adv748x_power_down_tx(tx); > >>> } > >>> > >>> @@ -337,10 +349,7 @@ static int adv748x_link_setup(struct media_entity *entity, > >>> struct adv748x_state *state = v4l2_get_subdevdata(sd); > >>> struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd); > >>> bool enable = flags & MEDIA_LNK_FL_ENABLED; > >>> - u8 io10_mask = ADV748X_IO_10_CSI1_EN | > >>> - ADV748X_IO_10_CSI4_EN | > >>> - ADV748X_IO_10_CSI4_IN_SEL_AFE; > >>> - u8 io10 = 0; > >>> + u8 csi4_in_sel = 0; > >>> > >>> /* Refuse to enable multiple links to the same TX at the same time. */ > >>> if (enable && tx->src) > >>> @@ -359,17 +368,19 @@ static int adv748x_link_setup(struct media_entity *entity, > >>> > >>> if (state->afe.tx) { > >>> /* AFE Requires TXA enabled, even when output to TXB */ > >>> - io10 |= ADV748X_IO_10_CSI4_EN; > >>> + csi4_in_sel |= ADV748X_IO_10_CSI4_EN; > >>> if (is_txa(tx)) > >>> - io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE; > >>> + csi4_in_sel |= ADV748X_IO_10_CSI4_IN_SEL_AFE; > >> > >> Hrm ... this is the one part that I think can't be determined without > >> caching some sort of value to state the routing. > >> > > > > Yes > > But the actual hardware shouldn't be updated if the stream is running > right? (I wonder if the media-controller would prevent that anyway). > > > > > > >>> else > >>> - io10 |= ADV748X_IO_10_CSI1_EN; > >>> + csi4_in_sel |= ADV748X_IO_10_CSI1_EN; > >>> } > >>> > >>> if (state->hdmi.tx) > >>> - io10 |= ADV748X_IO_10_CSI4_EN; > >>> + csi4_in_sel |= ADV748X_IO_10_CSI4_EN; > >>> > >>> - return io_clrset(state, ADV748X_IO_10, io10_mask, io10); > >>> + state->csi4_in_sel = csi4_in_sel; > >>> + > >>> + return 0; > >>> } > >>> > >>> static const struct media_entity_operations adv748x_tx_media_ops = { > >>> @@ -485,7 +496,6 @@ static int adv748x_sw_reset(struct adv748x_state *state) > >>> static int adv748x_reset(struct adv748x_state *state) > >>> { > >>> int ret; > >>> - u8 regval = 0; > >>> > >>> ret = adv748x_sw_reset(state); > >>> if (ret < 0) > >>> @@ -504,6 +514,12 @@ static int adv748x_reset(struct adv748x_state *state) > >>> if (ret) > >>> return ret; > >>> > >> > >> Should adv748x_reset() reset the state->csi4_in_sel cached value to 0 > >> before setting it? > > > > I should better check when reset happens, and if it happens only when > > links have been disabled or not. > > If links are disabled, the variable gets zeroed by the link_setup > > callback. If reset happens with links active, we should not zero it > > otherwise we lose the configuration > > > Ah yes, I missed that the local variable is initialised to zero, and > this state variable is set to the result at the end of the call... > > It does mean that the function ordering will be important here. > > It becomes irrelevant if these conditionals move to the same point > anyway though. > > > > > > >> > >> > >>> + /* Conditionally enable TXa and TXb. */ > >>> + if (is_tx_enabled(&state->txa)) > >>> + state->csi4_in_sel |= ADV748X_IO_10_CSI4_EN; > >>> + if (is_tx_enabled(&state->txb)) > >>> + state->csi4_in_sel |= ADV748X_IO_10_CSI1_EN; > >> > >> This makes it looks like the naming "csi4_in_sel" is less appropriate, > >> as it covers both CSI4 and CSI1... > >> > > > > Blame the hw designers :) > > Always. ... of course they keep blaming us back :D > > > > >> Also, this is confusing two terms, between the 'enable' and the 'select' > >> > >> The _EN bits looks like they control the activation of the CSI > >> transmitter, where as the 'select' bits control the routing. > >> > > > > You are righ. csi4_in_sel should only control the 3 bits used for > > routing, while enabling and disabling of the TXes is controlled by > > other bits of the io_10 register. > > I'll look into changing the name back > > > >> As the is_tx_enabled($TX) state is constant, perhaps that bit could be > >> inferred later when the register is written, and doesn't need to be > >> cached here? > > > > I'll consider that, thanks > > I think it's only the routing choice that needs to be stored in the > state. That would minimise being stored 'globally', and the values could > be determined at the time of programming the register. > > > > > Thanks > > j > > > >> > >> > >>> + > >>> /* Reset TXA and TXB */ > >>> adv748x_tx_power(&state->txa, 1); > >>> adv748x_tx_power(&state->txa, 0); > >>> @@ -513,13 +529,6 @@ static int adv748x_reset(struct adv748x_state *state) > >>> /* Disable chip powerdown & Enable HDMI Rx block */ > >>> io_write(state, ADV748X_IO_PD, ADV748X_IO_PD_RX_EN); > >>> > >>> - /* Conditionally enable TXa and TXb. */ > >>> - if (is_tx_enabled(&state->txa)) > >>> - regval |= ADV748X_IO_10_CSI4_EN; > >>> - if (is_tx_enabled(&state->txb)) > >>> - regval |= ADV748X_IO_10_CSI1_EN; > >>> - io_write(state, ADV748X_IO_10, regval); > >>> - > >>> /* Use vid_std and v_freq as freerun resolution for CP */ > >>> cp_clrset(state, ADV748X_CP_CLMP_POS, ADV748X_CP_CLMP_POS_DIS_AUTO, > >>> ADV748X_CP_CLMP_POS_DIS_AUTO); > >>> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h > >>> index 4a5a6445604f..27c116d09284 100644 > >>> --- a/drivers/media/i2c/adv748x/adv748x.h > >>> +++ b/drivers/media/i2c/adv748x/adv748x.h > >>> @@ -196,6 +196,8 @@ struct adv748x_state { > >>> struct adv748x_afe afe; > >>> struct adv748x_csi2 txa; > >>> struct adv748x_csi2 txb; > >>> + > >>> + unsigned int csi4_in_sel; > >>> }; > >>> > >>> #define adv748x_hdmi_to_state(h) container_of(h, struct adv748x_state, hdmi) > >>> -- > >>> 2.21.0 > >>> > > -- > Regards > -- > Kieran
Hi Jacopo, Sorry for dragging up an old thread... On 15/04/2019 08:00, Jacopo Mondi wrote: > Hi Kieran, > > On Fri, Apr 12, 2019 at 04:57:45PM +0100, Kieran Bingham wrote: >> Hi Jacopo, >> >> On 12/04/2019 15:45, Jacopo Mondi wrote: >>> Hi Kieran, >>> >>> On Fri, Apr 12, 2019 at 03:15:46PM +0100, Kieran Bingham wrote: >>>> Hi Jacopo, >>>> >>>> On 16/03/2019 15:47, Jacopo Mondi wrote: >>>>> Post-pone the write of the ADV748X_IO_10 register that controls the active >>>>> routing between the CP and AFE cores to the 4-lanes CSI-2 TXA at TX >>>>> power-up time. >>>> >>>> "by caching its configuration in the driver state." >>>> >>>>> >>>>> While at there, use the 'csi4_in_sel' name, which matches the register >>>> >>>> 'While at it, use the...' >>>> >>>> Except I'm not sure csi4_in_sel is the right name for the cached values >>>> as below... >>>> >>>> >>>>> field description in the manual, in place of 'io_10' which is the full >>>>> register name. >>>>> >>>> >>>> This has a fixes tag, but doesn't state what the actual problem is? >>>> >>> >>> As reported in the cover letter, please see: >>> "[PATCH] media: adv748x: Don't disable CSI-2 on link_setup" >> >> Ah I see, I had missed that. >> >> The patch itself should still describe the problem if it's fixing >> something. The cover letter will not be available in the git-log. >> > > You're right, I'll expand the commit message. > >> Ok, so this patch supersedes "[PATCH] media: adv748x: Don't disable >> CSI-2 on link_setup" ? >> > > Yes > >>>> Can I assume that the problem is that the configuration here is being >>>> written to the hardware before it is powered up or such? >>>> >>>> Or perhaps reading through the patch again, is it that the call to >>>> link_setup can affect running streams? >>> >>> The issue I was trying to solve, even with the first patch is that >>> going through a link disable (eg. at media graph reset time) wrote to >>> the csi4_in_sel register a 0 value, when both TXA and TXB routing >>> where disabled and this causes issues on some HDMI transmiters, for >>> reason Ian and Hans tried to expand but about which I'm not yet sure >>> about. >> >> Ok, I found that patch and read their comments. So disabling the CSI2 >> might trigger a hot-plug event to the transmitter which makes them think >> they have been (physically) disconnected in some way, or triggers them >> to re-read the EDID which will fail. But we don't really know what the >> fault is yet. >> > > Correct. Please note that I've seen this happening with one HDMI > transmitter (a chromecast device), while if HDMI source is a laptop > everything's fine... > >> >>> The idea here is to cache the routing settings at link_setup time >>> (upon activation or deactivation of a link) and apply them to hardware >>> at tx power up time, which happens at s_stream(1). >>> >>> In this way, if streaming is started, we know at least one link is >>> enabled and we can write to csi4_in_sel. >> >> Overall this seems reasonable, only making a change to io10 when either >> of the stream states are changed. >> > > Thanks, I'll re-send (maybe even out of this series if it gets stale). > Could I have your tag on the next iteration? I found this patch on a branch while rebasing old patches to latest. It still applies, and seems to make sense - and it looks like I agreed with you back when you posted it. It seems it only needed me to say: "Yes, Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> " To unblock this particular patch (separate from the rest of the series it currently resides in. So there you have it ;-) With the only action remaining being to try to briefly describe the problem it fixes in the commit message. Thanks, -- Kieran > > Thanks > j > >> >>>>> Fixes: 9423ca350df7 ("media: adv748x: Implement TX link_setup callback") >>>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >>>>> --- >>>>> drivers/media/i2c/adv748x/adv748x-core.c | 53 ++++++++++++++---------- >>>>> drivers/media/i2c/adv748x/adv748x.h | 2 + >>>>> 2 files changed, 33 insertions(+), 22 deletions(-) >>>>> >>>>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c >>>>> index 0e5a75eb6d75..02135741b1a6 100644 >>>>> --- a/drivers/media/i2c/adv748x/adv748x-core.c >>>>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c >>>>> @@ -305,23 +305,35 @@ static int adv748x_power_down_tx(struct adv748x_csi2 *tx) >>>>> >>>>> int adv748x_tx_power(struct adv748x_csi2 *tx, bool on) >>>>> { >>>>> - int val; >>>>> + u8 io10_mask = ADV748X_IO_10_CSI1_EN | ADV748X_IO_10_CSI4_EN | >>>>> + ADV748X_IO_10_CSI4_IN_SEL_AFE; >>>>> + struct adv748x_state *state = tx->state; >>>>> + int ret; >>>>> >>>>> if (!is_tx_enabled(tx)) >>>>> return 0; >>>>> >>>>> - val = tx_read(tx, ADV748X_CSI_FS_AS_LS); >>>>> - if (val < 0) >>>>> - return val; >>>>> + ret = tx_read(tx, ADV748X_CSI_FS_AS_LS); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> >>>>> /* >>>>> * This test against BIT(6) is not documented by the datasheet, but was >>>>> * specified in the downstream driver. >>>>> * Track with a WARN_ONCE to determine if it is ever set by HW. >>>>> */ >>>>> - WARN_ONCE((on && val & ADV748X_CSI_FS_AS_LS_UNKNOWN), >>>>> + WARN_ONCE((on && ret & ADV748X_CSI_FS_AS_LS_UNKNOWN), >>>>> "Enabling with unknown bit set"); >>>>> >>>>> + /* Configure TXA routing. */ >>>> >>>> Should TXA routing be configured even on TXB power up? This function >>>> handles both TX code paths. (Edit: possibly yes) >>>> >>> >>> The comment is wrong, thanks for noticing. >>> >>>> Can the logic that determines state->csi4_in_sel value simply be moved >>>> here (or to an independent adv748x_configure_routing() function)? >>>> >>> >>> It has to be changed at link_setup time in rensponse to a media link >>> activation or deactivation. >>> >>>> I think this patch means that changes to routing will now only take >>>> effect when starting or stopping a stream, is that right? (If so - could >>>> that go into the commit message please?) >>>> >>> >>> Well... >>> >>> Post-pone the write of the ADV748X_IO_10 register that controls the active >>> routing between the CP and AFE cores to the 4-lanes CSI-2 TXA at TX >>> power-up time. >>> >>> Doesn't it say that? Or what confused you is that s_stream->s_power(1) >>> and I should mention streaming instead of power up? >> >> I think of "Power up" meaning at device probe time (or possibly some >> runtime-pm event time). But yes, I think the distinction that it now >> happens at stream_on/stream_off is important. >> >> >> >>> >>>> >>>> >>>> >>>>> + if (on) { >>>>> + ret = io_clrset(state, ADV748X_IO_10, io10_mask, >>>>> + state->csi4_in_sel); >>>>> + if (ret) >>>>> + return ret; >>>>> + } >>>>> + >>>>> + >>>>> return on ? adv748x_power_up_tx(tx) : adv748x_power_down_tx(tx); >>>>> } >>>>> >>>>> @@ -337,10 +349,7 @@ static int adv748x_link_setup(struct media_entity *entity, >>>>> struct adv748x_state *state = v4l2_get_subdevdata(sd); >>>>> struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd); >>>>> bool enable = flags & MEDIA_LNK_FL_ENABLED; >>>>> - u8 io10_mask = ADV748X_IO_10_CSI1_EN | >>>>> - ADV748X_IO_10_CSI4_EN | >>>>> - ADV748X_IO_10_CSI4_IN_SEL_AFE; >>>>> - u8 io10 = 0; >>>>> + u8 csi4_in_sel = 0; >>>>> >>>>> /* Refuse to enable multiple links to the same TX at the same time. */ >>>>> if (enable && tx->src) >>>>> @@ -359,17 +368,19 @@ static int adv748x_link_setup(struct media_entity *entity, >>>>> >>>>> if (state->afe.tx) { >>>>> /* AFE Requires TXA enabled, even when output to TXB */ >>>>> - io10 |= ADV748X_IO_10_CSI4_EN; >>>>> + csi4_in_sel |= ADV748X_IO_10_CSI4_EN; >>>>> if (is_txa(tx)) >>>>> - io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE; >>>>> + csi4_in_sel |= ADV748X_IO_10_CSI4_IN_SEL_AFE; >>>> >>>> Hrm ... this is the one part that I think can't be determined without >>>> caching some sort of value to state the routing. >>>> >>> >>> Yes >> >> But the actual hardware shouldn't be updated if the stream is running >> right? (I wonder if the media-controller would prevent that anyway). >> >> >> >>> >>>>> else >>>>> - io10 |= ADV748X_IO_10_CSI1_EN; >>>>> + csi4_in_sel |= ADV748X_IO_10_CSI1_EN; >>>>> } >>>>> >>>>> if (state->hdmi.tx) >>>>> - io10 |= ADV748X_IO_10_CSI4_EN; >>>>> + csi4_in_sel |= ADV748X_IO_10_CSI4_EN; >>>>> >>>>> - return io_clrset(state, ADV748X_IO_10, io10_mask, io10); >>>>> + state->csi4_in_sel = csi4_in_sel; >>>>> + >>>>> + return 0; >>>>> } >>>>> >>>>> static const struct media_entity_operations adv748x_tx_media_ops = { >>>>> @@ -485,7 +496,6 @@ static int adv748x_sw_reset(struct adv748x_state *state) >>>>> static int adv748x_reset(struct adv748x_state *state) >>>>> { >>>>> int ret; >>>>> - u8 regval = 0; >>>>> >>>>> ret = adv748x_sw_reset(state); >>>>> if (ret < 0) >>>>> @@ -504,6 +514,12 @@ static int adv748x_reset(struct adv748x_state *state) >>>>> if (ret) >>>>> return ret; >>>>> >>>> >>>> Should adv748x_reset() reset the state->csi4_in_sel cached value to 0 >>>> before setting it? >>> >>> I should better check when reset happens, and if it happens only when >>> links have been disabled or not. >>> If links are disabled, the variable gets zeroed by the link_setup >>> callback. If reset happens with links active, we should not zero it >>> otherwise we lose the configuration >> >> >> Ah yes, I missed that the local variable is initialised to zero, and >> this state variable is set to the result at the end of the call... >> >> It does mean that the function ordering will be important here. >> >> It becomes irrelevant if these conditionals move to the same point >> anyway though. >> >> >> >>> >>>> >>>> >>>>> + /* Conditionally enable TXa and TXb. */ >>>>> + if (is_tx_enabled(&state->txa)) >>>>> + state->csi4_in_sel |= ADV748X_IO_10_CSI4_EN; >>>>> + if (is_tx_enabled(&state->txb)) >>>>> + state->csi4_in_sel |= ADV748X_IO_10_CSI1_EN; >>>> >>>> This makes it looks like the naming "csi4_in_sel" is less appropriate, >>>> as it covers both CSI4 and CSI1... >>>> >>> >>> Blame the hw designers :) >> >> Always. ... of course they keep blaming us back :D >> >>> >>>> Also, this is confusing two terms, between the 'enable' and the 'select' >>>> >>>> The _EN bits looks like they control the activation of the CSI >>>> transmitter, where as the 'select' bits control the routing. >>>> >>> >>> You are righ. csi4_in_sel should only control the 3 bits used for >>> routing, while enabling and disabling of the TXes is controlled by >>> other bits of the io_10 register. >>> I'll look into changing the name back >>> >>>> As the is_tx_enabled($TX) state is constant, perhaps that bit could be >>>> inferred later when the register is written, and doesn't need to be >>>> cached here? >>> >>> I'll consider that, thanks >> >> I think it's only the routing choice that needs to be stored in the >> state. That would minimise being stored 'globally', and the values could >> be determined at the time of programming the register. >> >>> >>> Thanks >>> j >>> >>>> >>>> >>>>> + >>>>> /* Reset TXA and TXB */ >>>>> adv748x_tx_power(&state->txa, 1); >>>>> adv748x_tx_power(&state->txa, 0); >>>>> @@ -513,13 +529,6 @@ static int adv748x_reset(struct adv748x_state *state) >>>>> /* Disable chip powerdown & Enable HDMI Rx block */ >>>>> io_write(state, ADV748X_IO_PD, ADV748X_IO_PD_RX_EN); >>>>> >>>>> - /* Conditionally enable TXa and TXb. */ >>>>> - if (is_tx_enabled(&state->txa)) >>>>> - regval |= ADV748X_IO_10_CSI4_EN; >>>>> - if (is_tx_enabled(&state->txb)) >>>>> - regval |= ADV748X_IO_10_CSI1_EN; >>>>> - io_write(state, ADV748X_IO_10, regval); >>>>> - >>>>> /* Use vid_std and v_freq as freerun resolution for CP */ >>>>> cp_clrset(state, ADV748X_CP_CLMP_POS, ADV748X_CP_CLMP_POS_DIS_AUTO, >>>>> ADV748X_CP_CLMP_POS_DIS_AUTO); >>>>> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h >>>>> index 4a5a6445604f..27c116d09284 100644 >>>>> --- a/drivers/media/i2c/adv748x/adv748x.h >>>>> +++ b/drivers/media/i2c/adv748x/adv748x.h >>>>> @@ -196,6 +196,8 @@ struct adv748x_state { >>>>> struct adv748x_afe afe; >>>>> struct adv748x_csi2 txa; >>>>> struct adv748x_csi2 txb; >>>>> + >>>>> + unsigned int csi4_in_sel; >>>>> }; >>>>> >>>>> #define adv748x_hdmi_to_state(h) container_of(h, struct adv748x_state, hdmi) >>>>> -- >>>>> 2.21.0 >>>>> >> >> -- >> Regards >> -- >> Kieran
diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c index 0e5a75eb6d75..02135741b1a6 100644 --- a/drivers/media/i2c/adv748x/adv748x-core.c +++ b/drivers/media/i2c/adv748x/adv748x-core.c @@ -305,23 +305,35 @@ static int adv748x_power_down_tx(struct adv748x_csi2 *tx) int adv748x_tx_power(struct adv748x_csi2 *tx, bool on) { - int val; + u8 io10_mask = ADV748X_IO_10_CSI1_EN | ADV748X_IO_10_CSI4_EN | + ADV748X_IO_10_CSI4_IN_SEL_AFE; + struct adv748x_state *state = tx->state; + int ret; if (!is_tx_enabled(tx)) return 0; - val = tx_read(tx, ADV748X_CSI_FS_AS_LS); - if (val < 0) - return val; + ret = tx_read(tx, ADV748X_CSI_FS_AS_LS); + if (ret < 0) + return ret; /* * This test against BIT(6) is not documented by the datasheet, but was * specified in the downstream driver. * Track with a WARN_ONCE to determine if it is ever set by HW. */ - WARN_ONCE((on && val & ADV748X_CSI_FS_AS_LS_UNKNOWN), + WARN_ONCE((on && ret & ADV748X_CSI_FS_AS_LS_UNKNOWN), "Enabling with unknown bit set"); + /* Configure TXA routing. */ + if (on) { + ret = io_clrset(state, ADV748X_IO_10, io10_mask, + state->csi4_in_sel); + if (ret) + return ret; + } + + return on ? adv748x_power_up_tx(tx) : adv748x_power_down_tx(tx); } @@ -337,10 +349,7 @@ static int adv748x_link_setup(struct media_entity *entity, struct adv748x_state *state = v4l2_get_subdevdata(sd); struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd); bool enable = flags & MEDIA_LNK_FL_ENABLED; - u8 io10_mask = ADV748X_IO_10_CSI1_EN | - ADV748X_IO_10_CSI4_EN | - ADV748X_IO_10_CSI4_IN_SEL_AFE; - u8 io10 = 0; + u8 csi4_in_sel = 0; /* Refuse to enable multiple links to the same TX at the same time. */ if (enable && tx->src) @@ -359,17 +368,19 @@ static int adv748x_link_setup(struct media_entity *entity, if (state->afe.tx) { /* AFE Requires TXA enabled, even when output to TXB */ - io10 |= ADV748X_IO_10_CSI4_EN; + csi4_in_sel |= ADV748X_IO_10_CSI4_EN; if (is_txa(tx)) - io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE; + csi4_in_sel |= ADV748X_IO_10_CSI4_IN_SEL_AFE; else - io10 |= ADV748X_IO_10_CSI1_EN; + csi4_in_sel |= ADV748X_IO_10_CSI1_EN; } if (state->hdmi.tx) - io10 |= ADV748X_IO_10_CSI4_EN; + csi4_in_sel |= ADV748X_IO_10_CSI4_EN; - return io_clrset(state, ADV748X_IO_10, io10_mask, io10); + state->csi4_in_sel = csi4_in_sel; + + return 0; } static const struct media_entity_operations adv748x_tx_media_ops = { @@ -485,7 +496,6 @@ static int adv748x_sw_reset(struct adv748x_state *state) static int adv748x_reset(struct adv748x_state *state) { int ret; - u8 regval = 0; ret = adv748x_sw_reset(state); if (ret < 0) @@ -504,6 +514,12 @@ static int adv748x_reset(struct adv748x_state *state) if (ret) return ret; + /* Conditionally enable TXa and TXb. */ + if (is_tx_enabled(&state->txa)) + state->csi4_in_sel |= ADV748X_IO_10_CSI4_EN; + if (is_tx_enabled(&state->txb)) + state->csi4_in_sel |= ADV748X_IO_10_CSI1_EN; + /* Reset TXA and TXB */ adv748x_tx_power(&state->txa, 1); adv748x_tx_power(&state->txa, 0); @@ -513,13 +529,6 @@ static int adv748x_reset(struct adv748x_state *state) /* Disable chip powerdown & Enable HDMI Rx block */ io_write(state, ADV748X_IO_PD, ADV748X_IO_PD_RX_EN); - /* Conditionally enable TXa and TXb. */ - if (is_tx_enabled(&state->txa)) - regval |= ADV748X_IO_10_CSI4_EN; - if (is_tx_enabled(&state->txb)) - regval |= ADV748X_IO_10_CSI1_EN; - io_write(state, ADV748X_IO_10, regval); - /* Use vid_std and v_freq as freerun resolution for CP */ cp_clrset(state, ADV748X_CP_CLMP_POS, ADV748X_CP_CLMP_POS_DIS_AUTO, ADV748X_CP_CLMP_POS_DIS_AUTO); diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h index 4a5a6445604f..27c116d09284 100644 --- a/drivers/media/i2c/adv748x/adv748x.h +++ b/drivers/media/i2c/adv748x/adv748x.h @@ -196,6 +196,8 @@ struct adv748x_state { struct adv748x_afe afe; struct adv748x_csi2 txa; struct adv748x_csi2 txb; + + unsigned int csi4_in_sel; }; #define adv748x_hdmi_to_state(h) container_of(h, struct adv748x_state, hdmi)
Post-pone the write of the ADV748X_IO_10 register that controls the active routing between the CP and AFE cores to the 4-lanes CSI-2 TXA at TX power-up time. While at there, use the 'csi4_in_sel' name, which matches the register field description in the manual, in place of 'io_10' which is the full register name. Fixes: 9423ca350df7 ("media: adv748x: Implement TX link_setup callback") Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- drivers/media/i2c/adv748x/adv748x-core.c | 53 ++++++++++++++---------- drivers/media/i2c/adv748x/adv748x.h | 2 + 2 files changed, 33 insertions(+), 22 deletions(-) -- 2.21.0