[PATCH v3 0/2] media: rcar-vin: Mask access to VNCSI_IFMD register

[PATCH v3 0/2] media: rcar-vin: Mask access to VNCSI_IFMD register

[Jacopo Mondi]

As reported in patch 2/2 commit message the the VNCSI_IFMD register
has the following limitations according to chip manual revision 2.20

- 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.

This patch upports the BSP change commit f54697394457
("media: rcar-vin: Fix VnCSI_IFMD register access for r8a77990") from
Koji Matsuoka

Tested on r-car E3 Ebisu.

v2 -> v3:
- Remove a few comments and add Niklas' tag to [2/2]

v1 -> v2:
- Inspect the channel routing table to deduce the availability of DES1/DES0
  bits as suggested by Niklas.

Jacopo Mondi (2):
  media: rcar-vin: Remove unused macro
  media: rcar-vin: Mask VNCSI_IFMD register

 drivers/media/platform/rcar-vin/rcar-dma.c | 26 ++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

--
2.29.1

[PATCH v3 1/2] media: rcar-vin: Remove unused macro

[Jacopo Mondi]

The VNCSI_IFMD_CSI_CHSEL_MASK is not used: remove it.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-dma.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index f6e84fa29bce..378514a75bc2 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -137,7 +137,6 @@
 #define VNCSI_IFMD_DES1		(1 << 26)
 #define VNCSI_IFMD_DES0		(1 << 25)
 #define VNCSI_IFMD_CSI_CHSEL(n) (((n) & 0xf) << 0)
-#define VNCSI_IFMD_CSI_CHSEL_MASK 0xf
 
 /* Video n scaling control register (Gen3) */
 #define VNUDS_CTRL_AMD		(1 << 30)
-- 
2.29.1

[PATCH v3 2/2] media: rcar-vin: Mask VNCSI_IFMD register

[Jacopo Mondi]

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")

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
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 | 25 +++++++++++++++++++---
 1 file changed, 22 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..6397eea665d1 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,26 @@ 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++) {
+		if (route->csi == RVIN_CSI20 || route->csi == RVIN_CSI21)
+			ifmd |= VNCSI_IFMD_DES1;
+		else
+			ifmd |= VNCSI_IFMD_DES0;
+
+		if (ifmd == (VNCSI_IFMD_DES0 | VNCSI_IFMD_DES1))
+			break;
+	}
 
-	rvin_write(vin, ifmd, VNCSI_IFMD_REG);
+	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

Re: [PATCH v3 0/2] media: rcar-vin: Mask access to VNCSI_IFMD register

[Mauro Carvalho Chehab]

Em Thu, 26 Nov 2020 08:47:55 +0100
Jacopo Mondi <jacopo+renesas@jmondi.org> escreveu:

> As reported in patch 2/2 commit message the the VNCSI_IFMD register
> has the following limitations according to chip manual revision 2.20
> 
> - 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.
> 
> This patch upports the BSP change commit f54697394457
> ("media: rcar-vin: Fix VnCSI_IFMD register access for r8a77990") from
> Koji Matsuoka

As checkpatch warned:

	-:22: WARNING: Unknown commit id 'f54697394457', maybe rebased or not pulled?
	
There's no such commit upstream. Are you referring to an OOT patch?

If so, you should provide an URL from where the patch is stored,
as otherwise this comment won't make any sense upstream.

If you can't provide such URL, please adjust the comment to provide
a description that won't be dependent of such OOT commit.

> 
> Tested on r-car E3 Ebisu.
> 
> v2 -> v3:
> - Remove a few comments and add Niklas' tag to [2/2]
> 
> v1 -> v2:
> - Inspect the channel routing table to deduce the availability of DES1/DES0
>   bits as suggested by Niklas.
> 
> Jacopo Mondi (2):
>   media: rcar-vin: Remove unused macro
>   media: rcar-vin: Mask VNCSI_IFMD register
> 
>  drivers/media/platform/rcar-vin/rcar-dma.c | 26 ++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
> 
> --
> 2.29.1
> 



Thanks,
Mauro

Re: [PATCH v3 0/2] media: rcar-vin: Mask access to VNCSI_IFMD register

[Hans Verkuil]

On 03/12/2020 07:58, Mauro Carvalho Chehab wrote:
> Em Thu, 26 Nov 2020 08:47:55 +0100
> Jacopo Mondi <jacopo+renesas@jmondi.org> escreveu:
> 
>> As reported in patch 2/2 commit message the the VNCSI_IFMD register
>> has the following limitations according to chip manual revision 2.20
>>
>> - 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.
>>
>> This patch upports the BSP change commit f54697394457
>> ("media: rcar-vin: Fix VnCSI_IFMD register access for r8a77990") from
>> Koji Matsuoka
> 
> As checkpatch warned:
> 
> 	-:22: WARNING: Unknown commit id 'f54697394457', maybe rebased or not pulled?
> 	
> There's no such commit upstream. Are you referring to an OOT patch?
> 
> If so, you should provide an URL from where the patch is stored,
> as otherwise this comment won't make any sense upstream.
> 
> If you can't provide such URL, please adjust the comment to provide
> a description that won't be dependent of such OOT commit.

Read the commit message:

>> This patch upports the BSP change commit f54697394457
>> ("media: rcar-vin: Fix VnCSI_IFMD register access for r8a77990") from
>> Koji Matsuoka

This commit is from the rcar BSP package, and does not refer to a kernel
commit. So it is correct that the commit ID is unknown.

Regards,

	Hans

> 
>>
>> Tested on r-car E3 Ebisu.
>>
>> v2 -> v3:
>> - Remove a few comments and add Niklas' tag to [2/2]
>>
>> v1 -> v2:
>> - Inspect the channel routing table to deduce the availability of DES1/DES0
>>   bits as suggested by Niklas.
>>
>> Jacopo Mondi (2):
>>   media: rcar-vin: Remove unused macro
>>   media: rcar-vin: Mask VNCSI_IFMD register
>>
>>  drivers/media/platform/rcar-vin/rcar-dma.c | 26 ++++++++++++++++++----
>>  1 file changed, 22 insertions(+), 4 deletions(-)
>>
>> --
>> 2.29.1
>>
> 
> 
> 
> Thanks,
> Mauro
> 

Re: [PATCH v3 0/2] media: rcar-vin: Mask access to VNCSI_IFMD register

[Hans Verkuil]

On 03/12/2020 08:06, Hans Verkuil wrote:
> On 03/12/2020 07:58, Mauro Carvalho Chehab wrote:
>> Em Thu, 26 Nov 2020 08:47:55 +0100
>> Jacopo Mondi <jacopo+renesas@jmondi.org> escreveu:
>>
>>> As reported in patch 2/2 commit message the the VNCSI_IFMD register
>>> has the following limitations according to chip manual revision 2.20
>>>
>>> - 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.
>>>
>>> This patch upports the BSP change commit f54697394457
>>> ("media: rcar-vin: Fix VnCSI_IFMD register access for r8a77990") from
>>> Koji Matsuoka
>>
>> As checkpatch warned:
>>
>> 	-:22: WARNING: Unknown commit id 'f54697394457', maybe rebased or not pulled?
>> 	
>> There's no such commit upstream. Are you referring to an OOT patch?
>>
>> If so, you should provide an URL from where the patch is stored,
>> as otherwise this comment won't make any sense upstream.
>>
>> If you can't provide such URL, please adjust the comment to provide
>> a description that won't be dependent of such OOT commit.

Ah, it's too early in the morning for me. I didn't read your email carefully
enough.

Just ignore what I wrote.

Jacopo, can you repost with a URL? Or perhaps just drop this bit of text.

Regards,

	Hans

> 
> Read the commit message:
> 
>>> This patch upports the BSP change commit f54697394457
>>> ("media: rcar-vin: Fix VnCSI_IFMD register access for r8a77990") from
>>> Koji Matsuoka
> 
> This commit is from the rcar BSP package, and does not refer to a kernel
> commit. So it is correct that the commit ID is unknown.
> 
> Regards,
> 
> 	Hans
> 
>>
>>>
>>> Tested on r-car E3 Ebisu.
>>>
>>> v2 -> v3:
>>> - Remove a few comments and add Niklas' tag to [2/2]
>>>
>>> v1 -> v2:
>>> - Inspect the channel routing table to deduce the availability of DES1/DES0
>>>   bits as suggested by Niklas.
>>>
>>> Jacopo Mondi (2):
>>>   media: rcar-vin: Remove unused macro
>>>   media: rcar-vin: Mask VNCSI_IFMD register
>>>
>>>  drivers/media/platform/rcar-vin/rcar-dma.c | 26 ++++++++++++++++++----
>>>  1 file changed, 22 insertions(+), 4 deletions(-)
>>>
>>> --
>>> 2.29.1
>>>
>>
>>
>>
>> Thanks,
>> Mauro
>>
> 

Re: [PATCH v3 0/2] media: rcar-vin: Mask access to VNCSI_IFMD register

[Mauro Carvalho Chehab]

Em Thu, 3 Dec 2020 08:08:09 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 03/12/2020 08:06, Hans Verkuil wrote:
> > On 03/12/2020 07:58, Mauro Carvalho Chehab wrote:  
> >> Em Thu, 26 Nov 2020 08:47:55 +0100
> >> Jacopo Mondi <jacopo+renesas@jmondi.org> escreveu:
> >>  
> >>> As reported in patch 2/2 commit message the the VNCSI_IFMD register
> >>> has the following limitations according to chip manual revision 2.20
> >>>
> >>> - 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.
> >>>
> >>> This patch upports the BSP change commit f54697394457
> >>> ("media: rcar-vin: Fix VnCSI_IFMD register access for r8a77990") from
> >>> Koji Matsuoka  
> >>
> >> As checkpatch warned:
> >>
> >> 	-:22: WARNING: Unknown commit id 'f54697394457', maybe rebased or not pulled?
> >> 	
> >> There's no such commit upstream. Are you referring to an OOT patch?
> >>
> >> If so, you should provide an URL from where the patch is stored,
> >> as otherwise this comment won't make any sense upstream.
> >>
> >> If you can't provide such URL, please adjust the comment to provide
> >> a description that won't be dependent of such OOT commit.  
> 
> Ah, it's too early in the morning for me. I didn't read your email carefully
> enough.

Yeah, I realized that the commit is for an OOT patch ;-)

The main point is that Patch descriptions should be self-contained, bringing
everything that is needed to describe the patch on it. It is acceptable to have
URLs pointing to some external references that will be there as long as the
driver will remain at the Linux Kernel. The way the description currently is,
it doesn't fulfill such criteria.

-

Btw, I was unable to find such commit, even googling for it.

So, even now, people will find problems if they want to understand the
description. Things will become a lot worse after a couple of years.

So, such description has to be changed.

For now, I'm merging the other patches of the PR without this one.

> 
> Just ignore what I wrote.
> 
> Jacopo, can you repost with a URL? Or perhaps just drop this bit of text.
> 
> Regards,
> 
> 	Hans
> 
> > 
> > Read the commit message:
> >   
> >>> This patch upports the BSP change commit f54697394457
> >>> ("media: rcar-vin: Fix VnCSI_IFMD register access for r8a77990") from
> >>> Koji Matsuoka  
> > 
> > This commit is from the rcar BSP package, and does not refer to a kernel
> > commit. So it is correct that the commit ID is unknown.
> > 
> > Regards,
> > 
> > 	Hans
> >   
> >>  
> >>>
> >>> Tested on r-car E3 Ebisu.
> >>>
> >>> v2 -> v3:
> >>> - Remove a few comments and add Niklas' tag to [2/2]
> >>>
> >>> v1 -> v2:
> >>> - Inspect the channel routing table to deduce the availability of DES1/DES0
> >>>   bits as suggested by Niklas.
> >>>
> >>> Jacopo Mondi (2):
> >>>   media: rcar-vin: Remove unused macro
> >>>   media: rcar-vin: Mask VNCSI_IFMD register
> >>>
> >>>  drivers/media/platform/rcar-vin/rcar-dma.c | 26 ++++++++++++++++++----
> >>>  1 file changed, 22 insertions(+), 4 deletions(-)
> >>>
> >>> --
> >>> 2.29.1
> >>>  
> >>
> >>
> >>
> >> Thanks,
> >> Mauro
> >>  
> >   
> 



Thanks,
Mauro

Re: [PATCH v3 0/2] media: rcar-vin: Mask access to VNCSI_IFMD register

[Geert Uytterhoeven]

Hi Mauro,

On Thu, Dec 3, 2020 at 8:53 AM Mauro Carvalho Chehab
<mchehab+huawei@kernel.org> wrote:
> Em Thu, 3 Dec 2020 08:08:09 +0100
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > On 03/12/2020 08:06, Hans Verkuil wrote:
> > > On 03/12/2020 07:58, Mauro Carvalho Chehab wrote:
> > >> Em Thu, 26 Nov 2020 08:47:55 +0100
> > >> Jacopo Mondi <jacopo+renesas@jmondi.org> escreveu:
> > >>
> > >>> As reported in patch 2/2 commit message the the VNCSI_IFMD register
> > >>> has the following limitations according to chip manual revision 2.20
> > >>>
> > >>> - 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.
> > >>>
> > >>> This patch upports the BSP change commit f54697394457
> > >>> ("media: rcar-vin: Fix VnCSI_IFMD register access for r8a77990") from
> > >>> Koji Matsuoka
> > >>
> > >> As checkpatch warned:
> > >>
> > >>    -:22: WARNING: Unknown commit id 'f54697394457', maybe rebased or not pulled?
> > >>
> > >> There's no such commit upstream. Are you referring to an OOT patch?
> > >>
> > >> If so, you should provide an URL from where the patch is stored,
> > >> as otherwise this comment won't make any sense upstream.
> > >>
> > >> If you can't provide such URL, please adjust the comment to provide
> > >> a description that won't be dependent of such OOT commit.
> >
> > Ah, it's too early in the morning for me. I didn't read your email carefully
> > enough.
>
> Yeah, I realized that the commit is for an OOT patch ;-)
>
> The main point is that Patch descriptions should be self-contained, bringing
> everything that is needed to describe the patch on it. It is acceptable to have
> URLs pointing to some external references that will be there as long as the
> driver will remain at the Linux Kernel. The way the description currently is,
> it doesn't fulfill such criteria.
>
> -
>
> Btw, I was unable to find such commit, even googling for it.

https://github.com/renesas-rcar/linux-bsp/commit/f54697394457

It's indeed unfortunate that Google doesn't support searching by
git commit ID.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 0/2] media: rcar-vin: Mask access to VNCSI_IFMD register

[Jacopo Mondi]

Hi,

On Thu, Dec 03, 2020 at 09:33:34AM +0100, Geert Uytterhoeven wrote:
> Hi Mauro,
>
> On Thu, Dec 3, 2020 at 8:53 AM Mauro Carvalho Chehab
> <mchehab+huawei@kernel.org> wrote:
> > Em Thu, 3 Dec 2020 08:08:09 +0100
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > > On 03/12/2020 08:06, Hans Verkuil wrote:
> > > > On 03/12/2020 07:58, Mauro Carvalho Chehab wrote:
> > > >> Em Thu, 26 Nov 2020 08:47:55 +0100
> > > >> Jacopo Mondi <jacopo+renesas@jmondi.org> escreveu:
> > > >>
> > > >>> As reported in patch 2/2 commit message the the VNCSI_IFMD register
> > > >>> has the following limitations according to chip manual revision 2.20
> > > >>>
> > > >>> - 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.
> > > >>>
> > > >>> This patch upports the BSP change commit f54697394457
> > > >>> ("media: rcar-vin: Fix VnCSI_IFMD register access for r8a77990") from
> > > >>> Koji Matsuoka
> > > >>
> > > >> As checkpatch warned:
> > > >>
> > > >>    -:22: WARNING: Unknown commit id 'f54697394457', maybe rebased or not pulled?
> > > >>

Ups :/

> > > >> There's no such commit upstream. Are you referring to an OOT patch?
> > > >>
> > > >> If so, you should provide an URL from where the patch is stored,
> > > >> as otherwise this comment won't make any sense upstream.
> > > >>
> > > >> If you can't provide such URL, please adjust the comment to provide
> > > >> a description that won't be dependent of such OOT commit.
> > >
> > > Ah, it's too early in the morning for me. I didn't read your email carefully
> > > enough.
> >
> > Yeah, I realized that the commit is for an OOT patch ;-)
> >
> > The main point is that Patch descriptions should be self-contained, bringing
> > everything that is needed to describe the patch on it. It is acceptable to have
> > URLs pointing to some external references that will be there as long as the
> > driver will remain at the Linux Kernel. The way the description currently is,
> > it doesn't fulfill such criteria.
> >
> > -
> >
> > Btw, I was unable to find such commit, even googling for it.
>
> https://github.com/renesas-rcar/linux-bsp/commit/f54697394457
>
> It's indeed unfortunate that Google doesn't support searching by
> git commit ID.

Thanks Geert for the link and sorry Mauro and Hans.

Hans confirmed me he'll fix the commit message when making the PR,
thank you!

Cheers
   j

>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds