| Message ID | 20201116110428.27338-3-jacopo+renesas@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: rcar-vin: Mask access to VNCSI_IFMD register | expand |
Hi Jacopo, Thanks for your work. On 2020-11-16 12:04:28 +0100, Jacopo Mondi wrote: > The VNCSI_IFMD register controls the data expansion mode and the > channel routing between the CSI-2 receivers and VIN instances. > > According to the chip manual revision 2.20 not all fields are available > for all the SoCs: > - V3M, V3H and E3 do not support the DES1 field has they do not feature > a CSI20 receiver. > - D3 only supports parallel input, and the whole register shall always > be written as 0. > > Inspect the per-SoC channel routing table where the available CSI-2 > instances are reported and configure VNCSI_IFMD accordingly. > > This patch upports the BSP change commit f54697394457 > ("media: rcar-vin: Fix VnCSI_IFMD register access for r8a77990") I really like this approach, nice work. > > Suggested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > drivers/media/platform/rcar-vin/rcar-dma.c | 26 +++++++++++++++++++--- > 1 file changed, 23 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c > index 378514a75bc2..ab6818b34e5a 100644 > --- a/drivers/media/platform/rcar-vin/rcar-dma.c > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c > @@ -1570,7 +1570,9 @@ int rvin_dma_register(struct rvin_dev *vin, int irq) > */ > int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel) > { > - u32 ifmd, vnmc; > + const struct rvin_group_route *route; > + u32 ifmd = 0; > + u32 vnmc; > int ret; > > ret = pm_runtime_get_sync(vin->dev); > @@ -1583,9 +1585,27 @@ int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel) > vnmc = rvin_read(vin, VNMC_REG); > rvin_write(vin, vnmc & ~VNMC_VUP, VNMC_REG); > > - ifmd = VNCSI_IFMD_DES1 | VNCSI_IFMD_DES0 | VNCSI_IFMD_CSI_CHSEL(chsel); > + /* > + * Set data expansion mode to "pad with 0s" by inspecting the routes > + * table to find out which bit fields are available in the IFMD > + * register. IFMD_DES1 controls data expansion mode for CSI20/21, > + * IFMD_DES0 controls data expansion mode for CSI40/41. > + */ > + for (route = vin->info->routes; route->mask; route++) { > + /* CSI21 is only available in r8a7795es1 and not documented. */ I would drop this comment as we do not know what will happen in future SoCs. > + if (route->csi == RVIN_CSI20 || route->csi == RVIN_CSI21) > + ifmd |= VNCSI_IFMD_DES1; > + else > + ifmd |= VNCSI_IFMD_DES0; > > - rvin_write(vin, ifmd, VNCSI_IFMD_REG); > + /* If both have been set stop looping. */ > + if (ifmd == (VNCSI_IFMD_DES0 | VNCSI_IFMD_DES1)) > + break; I would remove the comment here as I think the code is quiet self explanatory and reading the comment made me think I was missing something obvious ;-) > + } Missing blank line. With these small nits fixed, Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > + if (ifmd) { > + ifmd |= VNCSI_IFMD_CSI_CHSEL(chsel); > + rvin_write(vin, ifmd, VNCSI_IFMD_REG); > + } > > vin_dbg(vin, "Set IFMD 0x%x\n", ifmd); > > -- > 2.29.1 > -- Regards, Niklas Söderlund
Hi Niklas, On Mon, Nov 23, 2020 at 04:31:24PM +0100, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your work. > > On 2020-11-16 12:04:28 +0100, Jacopo Mondi wrote: > > The VNCSI_IFMD register controls the data expansion mode and the > > channel routing between the CSI-2 receivers and VIN instances. > > > > According to the chip manual revision 2.20 not all fields are available > > for all the SoCs: > > - V3M, V3H and E3 do not support the DES1 field has they do not feature > > a CSI20 receiver. > > - D3 only supports parallel input, and the whole register shall always > > be written as 0. > > > > Inspect the per-SoC channel routing table where the available CSI-2 > > instances are reported and configure VNCSI_IFMD accordingly. > > > > This patch upports the BSP change commit f54697394457 > > ("media: rcar-vin: Fix VnCSI_IFMD register access for r8a77990") > > I really like this approach, nice work. > I'm happy you like it, almost like you suggested it in first place, right ? :) > > > > Suggested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > drivers/media/platform/rcar-vin/rcar-dma.c | 26 +++++++++++++++++++--- > > 1 file changed, 23 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c > > index 378514a75bc2..ab6818b34e5a 100644 > > --- a/drivers/media/platform/rcar-vin/rcar-dma.c > > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c > > @@ -1570,7 +1570,9 @@ int rvin_dma_register(struct rvin_dev *vin, int irq) > > */ > > int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel) > > { > > - u32 ifmd, vnmc; > > + const struct rvin_group_route *route; > > + u32 ifmd = 0; > > + u32 vnmc; > > int ret; > > > > ret = pm_runtime_get_sync(vin->dev); > > @@ -1583,9 +1585,27 @@ int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel) > > vnmc = rvin_read(vin, VNMC_REG); > > rvin_write(vin, vnmc & ~VNMC_VUP, VNMC_REG); > > > > - ifmd = VNCSI_IFMD_DES1 | VNCSI_IFMD_DES0 | VNCSI_IFMD_CSI_CHSEL(chsel); > > + /* > > + * Set data expansion mode to "pad with 0s" by inspecting the routes > > + * table to find out which bit fields are available in the IFMD > > + * register. IFMD_DES1 controls data expansion mode for CSI20/21, > > + * IFMD_DES0 controls data expansion mode for CSI40/41. > > + */ > > + for (route = vin->info->routes; route->mask; route++) { > > + /* CSI21 is only available in r8a7795es1 and not documented. */ > > I would drop this comment as we do not know what will happen in future > SoCs. > > > + if (route->csi == RVIN_CSI20 || route->csi == RVIN_CSI21) > > + ifmd |= VNCSI_IFMD_DES1; > > + else > > + ifmd |= VNCSI_IFMD_DES0; > > > > - rvin_write(vin, ifmd, VNCSI_IFMD_REG); > > + /* If both have been set stop looping. */ > > + if (ifmd == (VNCSI_IFMD_DES0 | VNCSI_IFMD_DES1)) > > + break; > > I would remove the comment here as I think the code is quiet self > explanatory and reading the comment made me think I was missing > something obvious ;-) > Correct, I'm breaking the first rule of avoiding superfluous comments. > > + } > > Missing blank line. Was kind of intentional, but I can add one > > With these small nits fixed, > > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> Thanks j > > > + if (ifmd) { > > + ifmd |= VNCSI_IFMD_CSI_CHSEL(chsel); > > + rvin_write(vin, ifmd, VNCSI_IFMD_REG); > > + } > > > > vin_dbg(vin, "Set IFMD 0x%x\n", ifmd); > > > > -- > > 2.29.1 > > > > -- > Regards, > Niklas Söderlund
diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c index 378514a75bc2..ab6818b34e5a 100644 --- a/drivers/media/platform/rcar-vin/rcar-dma.c +++ b/drivers/media/platform/rcar-vin/rcar-dma.c @@ -1570,7 +1570,9 @@ int rvin_dma_register(struct rvin_dev *vin, int irq) */ int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel) { - u32 ifmd, vnmc; + const struct rvin_group_route *route; + u32 ifmd = 0; + u32 vnmc; int ret; ret = pm_runtime_get_sync(vin->dev); @@ -1583,9 +1585,27 @@ int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel) vnmc = rvin_read(vin, VNMC_REG); rvin_write(vin, vnmc & ~VNMC_VUP, VNMC_REG); - ifmd = VNCSI_IFMD_DES1 | VNCSI_IFMD_DES0 | VNCSI_IFMD_CSI_CHSEL(chsel); + /* + * Set data expansion mode to "pad with 0s" by inspecting the routes + * table to find out which bit fields are available in the IFMD + * register. IFMD_DES1 controls data expansion mode for CSI20/21, + * IFMD_DES0 controls data expansion mode for CSI40/41. + */ + for (route = vin->info->routes; route->mask; route++) { + /* CSI21 is only available in r8a7795es1 and not documented. */ + if (route->csi == RVIN_CSI20 || route->csi == RVIN_CSI21) + ifmd |= VNCSI_IFMD_DES1; + else + ifmd |= VNCSI_IFMD_DES0; - rvin_write(vin, ifmd, VNCSI_IFMD_REG); + /* If both have been set stop looping. */ + if (ifmd == (VNCSI_IFMD_DES0 | VNCSI_IFMD_DES1)) + break; + } + if (ifmd) { + ifmd |= VNCSI_IFMD_CSI_CHSEL(chsel); + rvin_write(vin, ifmd, VNCSI_IFMD_REG); + } vin_dbg(vin, "Set IFMD 0x%x\n", ifmd);
The VNCSI_IFMD register controls the data expansion mode and the channel routing between the CSI-2 receivers and VIN instances. According to the chip manual revision 2.20 not all fields are available for all the SoCs: - V3M, V3H and E3 do not support the DES1 field has they do not feature a CSI20 receiver. - D3 only supports parallel input, and the whole register shall always be written as 0. Inspect the per-SoC channel routing table where the available CSI-2 instances are reported and configure VNCSI_IFMD accordingly. This patch upports the BSP change commit f54697394457 ("media: rcar-vin: Fix VnCSI_IFMD register access for r8a77990") Suggested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- drivers/media/platform/rcar-vin/rcar-dma.c | 26 +++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-)