| Message ID | 20171226211424.870595086@cogentembedded.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi Sergei, Thank you for the patch. On Tuesday, 26 December 2017 23:14:12 EET Sergei Shtylyov wrote: > Laurent has added support for the VSP2-D found on R-Car V3M (R8A77970) but I'm not sure there's a need to state my name in the commit message. > the video output that VSP2-D sends to DU has a greenish garbage-like line Why does the text in your patches (commit message, comments, ...) sometime have double spaces between words ? > repeated every 8 or so screen rows. Is it every "8 or so" rows, or exactly every 8 rows ? > It turns out that V3M has a teeny LIF register (at least it's documented!) > that you need to set to some kind of a magic value for the LIF to work > correctly... > > Based on the original (and large) patch by Daisuke Matsushita > <daisuke.matsushita.ns@hitachi.com>. What else is in the big patch ? Is it available somewhere ? > Fixes: d455b45f8393 ("v4l: vsp1: Add support for new VSP2-BS, VSP2-DL and > VSP2-D instances") > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > --- > This patch is against the 'media_tree.git' repo's 'master' branch. > > Changes in version 2: > - added a comment before the V3M SoC check; > - fixed indetation in that check; > - reformatted the patch description. > > drivers/media/platform/vsp1/vsp1_lif.c | 12 ++++++++++++ > drivers/media/platform/vsp1/vsp1_regs.h | 5 +++++ > 2 files changed, 17 insertions(+) > > Index: media_tree/drivers/media/platform/vsp1/vsp1_lif.c > =================================================================== > --- media_tree.orig/drivers/media/platform/vsp1/vsp1_lif.c > +++ media_tree/drivers/media/platform/vsp1/vsp1_lif.c > @@ -155,6 +155,18 @@ static void lif_configure(struct vsp1_en > (obth << VI6_LIF_CTRL_OBTH_SHIFT) | > (format->code == 0 ? VI6_LIF_CTRL_CFMT : 0) | > VI6_LIF_CTRL_REQSEL | VI6_LIF_CTRL_LIF_EN); > + > + /* > + * R-Car V3M has the buffer attribute register you absolutely need > + * to write kinda magic value to for the LIF to work correctly... > + */ I'm not sure about the "kinda" magic value. 1536 is very likely a buffer size. How about the following text ? /* * On V3M the LBA register has to be set to a non-default value to * guarantee proper operation (otherwise artifacts may appear on the * output). The value required by the datasheet is not documented but * is likely a buffer size or threshold. */ The commit message should also be updated to feel a bit less magic. > + if ((entity->vsp1->version & > + (VI6_IP_VERSION_MODEL_MASK | VI6_IP_VERSION_SOC_MASK)) == > + (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M)) { > + vsp1_lif_write(lif, dl, VI6_LIF_LBA, > + VI6_LIF_LBA_LBA0 | > + (1536 << VI6_LIF_LBA_LBA1_SHIFT)); > + } > } The datasheet documents the register as being present on both V3M and M3-W (and the test I've just run on H3 shows that the register is present there as well). Should we program it on M3-W or leave it to the default value that should be what is recommended by the datasheet for that SoC ? > static const struct vsp1_entity_operations lif_entity_ops = { > Index: media_tree/drivers/media/platform/vsp1/vsp1_regs.h > =================================================================== > --- media_tree.orig/drivers/media/platform/vsp1/vsp1_regs.h > +++ media_tree/drivers/media/platform/vsp1/vsp1_regs.h > @@ -693,6 +693,11 @@ > #define VI6_LIF_CSBTH_LBTH_MASK (0x7ff << 0) > #define VI6_LIF_CSBTH_LBTH_SHIFT 0 > > +#define VI6_LIF_LBA 0x3b0c > +#define VI6_LIF_LBA_LBA0 (1 << 31) > +#define VI6_LIF_LBA_LBA1_MASK (0xfff << 16) > +#define VI6_LIF_LBA_LBA1_SHIFT 16 > + > /* ------------------------------------------------------------------------ > * Security Control Registers > */
Hello! On 01/15/2018 03:51 PM, Laurent Pinchart wrote: > On Tuesday, 26 December 2017 23:14:12 EET Sergei Shtylyov wrote: >> Laurent has added support for the VSP2-D found on R-Car V3M (R8A77970) but > > I'm not sure there's a need to state my name in the commit message. You were the author of the patch this one has in the Fixes: tag, so I thought that was appropriate. I can remove that if you don't want you name mentioned... >> the video output that VSP2-D sends to DU has a greenish garbage-like line > > Why does the text in your patches (commit message, comments, ...) sometime > have double spaces between words ? The text looks more pleasant (at least to me). Can remove them if you want... >> repeated every 8 or so screen rows. > > Is it every "8 or so" rows, or exactly every 8 rows ? You really want me to count the pixels? >> It turns out that V3M has a teeny LIF register (at least it's documented!) >> that you need to set to some kind of a magic value for the LIF to work >> correctly... >> >> Based on the original (and large) patch by Daisuke Matsushita >> <daisuke.matsushita.ns@hitachi.com>. > > What else is in the big patch ? Is it available somewhere ? Assorted changes gathered together only because they all bring the support for R8A77970. If you're really curious, here you are: https://github.com/CogentEmbedded/meta-rcar/blob/v2.12.0/meta-rcar-gen3/recipes-kernel/linux/linux-renesas/0030-arm64-renesas-r8a7797-Add-Renesas-R8A7797-SoC-suppor.patch >> Fixes: d455b45f8393 ("v4l: vsp1: Add support for new VSP2-BS, VSP2-DL and >> VSP2-D instances") >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >> >> --- >> This patch is against the 'media_tree.git' repo's 'master' branch. >> >> Changes in version 2: >> - added a comment before the V3M SoC check; >> - fixed indetation in that check; >> - reformatted the patch description. >> >> drivers/media/platform/vsp1/vsp1_lif.c | 12 ++++++++++++ >> drivers/media/platform/vsp1/vsp1_regs.h | 5 +++++ >> 2 files changed, 17 insertions(+) >> >> Index: media_tree/drivers/media/platform/vsp1/vsp1_lif.c >> =================================================================== >> --- media_tree.orig/drivers/media/platform/vsp1/vsp1_lif.c >> +++ media_tree/drivers/media/platform/vsp1/vsp1_lif.c >> @@ -155,6 +155,18 @@ static void lif_configure(struct vsp1_en >> (obth << VI6_LIF_CTRL_OBTH_SHIFT) | >> (format->code == 0 ? VI6_LIF_CTRL_CFMT : 0) | >> VI6_LIF_CTRL_REQSEL | VI6_LIF_CTRL_LIF_EN); >> + >> + /* >> + * R-Car V3M has the buffer attribute register you absolutely need >> + * to write kinda magic value to for the LIF to work correctly... >> + */ > > I'm not sure about the "kinda" magic value. 1536 is very likely a buffer size. Well, that's only guessing. The manual doesn't say anything about what the number is. > How about the following text ? > > /* > * On V3M the LBA register has to be set to a non-default value to I'd then spell its full name, LIF0 Buffer Attribute register. > * guarantee proper operation (otherwise artifacts may appear on the > * output). The value required by the datasheet is not documented but > * is likely a buffer size or threshold. > */ > OK, can change that (modulo the name). > The commit message should also be updated to feel a bit less magic. I'll see about it. >> + if ((entity->vsp1->version & >> + (VI6_IP_VERSION_MODEL_MASK | VI6_IP_VERSION_SOC_MASK)) == >> + (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M)) { >> + vsp1_lif_write(lif, dl, VI6_LIF_LBA, >> + VI6_LIF_LBA_LBA0 | >> + (1536 << VI6_LIF_LBA_LBA1_SHIFT)); >> + } >> } > > The datasheet documents the register as being present on both V3M and M3-W > (and the test I've just run on H3 shows that the register is present there as > well). Should we program it on M3-W or leave it to the default value that > should be what is recommended by the datasheet for that SoC ? If the default value matches what's recommended by the manual, then I'd leave the register alone. But my task was R8A77970 support only anyway... [..] MBR, Sergei
Hi Sergei, On Monday, 15 January 2018 18:06:48 EET Sergei Shtylyov wrote: > On 01/15/2018 03:51 PM, Laurent Pinchart wrote: > > On Tuesday, 26 December 2017 23:14:12 EET Sergei Shtylyov wrote: > >> Laurent has added support for the VSP2-D found on R-Car V3M (R8A77970) > >> but > > > > I'm not sure there's a need to state my name in the commit message. > > You were the author of the patch this one has in the Fixes: tag, so I > thought that was appropriate. I can remove that if you don't want you name > mentioned... It's just that I thought it was more important to refer to the patch than to my name, developers reading the log will be more interested in the technical details than in my person :-) > >> the video output that VSP2-D sends to DU has a greenish garbage-like > >> line > > > > Why does the text in your patches (commit message, comments, ...) sometime > > have double spaces between words ? > > The text looks more pleasant (at least to me). Can remove them if you > want... I don't think we try to do typesetting in kernel code or commit messages, so I'd stick to single spaces if you don't mind. > >> repeated every 8 or so screen rows. > > > > Is it every "8 or so" rows, or exactly every 8 rows ? > > You really want me to count the pixels? I'd like to know a bit more about the systems, whether the green lines appear at the exact same interval, whether they bounce up and down or are always at the same place, ... > >> It turns out that V3M has a teeny LIF register (at least it's > >> documented!) that you need to set to some kind of a magic value for the > >> LIF to work correctly... > >> > >> Based on the original (and large) patch by Daisuke Matsushita > >> <daisuke.matsushita.ns@hitachi.com>. > > > > What else is in the big patch ? Is it available somewhere ? > > Assorted changes gathered together only because they all bring the > support for R8A77970. If you're really curious, here you are: > > https://github.com/CogentEmbedded/meta-rcar/blob/v2.12.0/meta-rcar-gen3/reci > pes-kernel/linux/linux-renesas/0030-arm64-renesas-r8a7797-Add-Renesas-R8A779 > 7-SoC-suppor.patch Thank you. > >> Fixes: d455b45f8393 ("v4l: vsp1: Add support for new VSP2-BS, VSP2-DL and > >> VSP2-D instances") > >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > >> > >> --- > >> This patch is against the 'media_tree.git' repo's 'master' branch. > >> > >> Changes in version 2: > >> - added a comment before the V3M SoC check; > >> - fixed indetation in that check; > >> - reformatted the patch description. > >> > >> drivers/media/platform/vsp1/vsp1_lif.c | 12 ++++++++++++ > >> drivers/media/platform/vsp1/vsp1_regs.h | 5 +++++ > >> 2 files changed, 17 insertions(+) > >> > >> Index: media_tree/drivers/media/platform/vsp1/vsp1_lif.c > >> =================================================================== > >> --- media_tree.orig/drivers/media/platform/vsp1/vsp1_lif.c > >> +++ media_tree/drivers/media/platform/vsp1/vsp1_lif.c > >> @@ -155,6 +155,18 @@ static void lif_configure(struct vsp1_en > >> (obth << VI6_LIF_CTRL_OBTH_SHIFT) | > >> (format->code == 0 ? VI6_LIF_CTRL_CFMT : 0) | > >> VI6_LIF_CTRL_REQSEL | VI6_LIF_CTRL_LIF_EN); > >> + > >> + /* > >> + * R-Car V3M has the buffer attribute register you absolutely need > >> + * to write kinda magic value to for the LIF to work correctly... > >> + */ > > > > I'm not sure about the "kinda" magic value. 1536 is very likely a buffer > > size. > > Well, that's only guessing. The manual doesn't say anything about what the > number is. Yes it's just an educated guess. > > How about the following text ? > > > > /* > > > > * On V3M the LBA register has to be set to a non-default value to > > I'd then spell its full name, LIF0 Buffer Attribute register. Sounds good to me. > > * guarantee proper operation (otherwise artifacts may appear on the > > * output). The value required by the datasheet is not documented but > > * is likely a buffer size or threshold. > > */ > > OK, can change that (modulo the name). > > > The commit message should also be updated to feel a bit less magic. > > I'll see about it. > > >> + if ((entity->vsp1->version & > >> + (VI6_IP_VERSION_MODEL_MASK | VI6_IP_VERSION_SOC_MASK)) == > >> + (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M)) { > >> + vsp1_lif_write(lif, dl, VI6_LIF_LBA, > >> + VI6_LIF_LBA_LBA0 | > >> + (1536 << VI6_LIF_LBA_LBA1_SHIFT)); > >> + } > >> > >> } > > > > The datasheet documents the register as being present on both V3M and M3-W > > (and the test I've just run on H3 shows that the register is present there > > as well). Should we program it on M3-W or leave it to the default value > > that should be what is recommended by the datasheet for that SoC ? > > If the default value matches what's recommended by the manual, then I'd > leave the register alone. OK, I agree. > But my task was R8A77970 support only anyway... Sure, and my task is to push you to do more when it makes sense ;-)
Index: media_tree/drivers/media/platform/vsp1/vsp1_lif.c =================================================================== --- media_tree.orig/drivers/media/platform/vsp1/vsp1_lif.c +++ media_tree/drivers/media/platform/vsp1/vsp1_lif.c @@ -155,6 +155,18 @@ static void lif_configure(struct vsp1_en (obth << VI6_LIF_CTRL_OBTH_SHIFT) | (format->code == 0 ? VI6_LIF_CTRL_CFMT : 0) | VI6_LIF_CTRL_REQSEL | VI6_LIF_CTRL_LIF_EN); + + /* + * R-Car V3M has the buffer attribute register you absolutely need + * to write kinda magic value to for the LIF to work correctly... + */ + if ((entity->vsp1->version & + (VI6_IP_VERSION_MODEL_MASK | VI6_IP_VERSION_SOC_MASK)) == + (VI6_IP_VERSION_MODEL_VSPD_V3 | VI6_IP_VERSION_SOC_V3M)) { + vsp1_lif_write(lif, dl, VI6_LIF_LBA, + VI6_LIF_LBA_LBA0 | + (1536 << VI6_LIF_LBA_LBA1_SHIFT)); + } } static const struct vsp1_entity_operations lif_entity_ops = { Index: media_tree/drivers/media/platform/vsp1/vsp1_regs.h =================================================================== --- media_tree.orig/drivers/media/platform/vsp1/vsp1_regs.h +++ media_tree/drivers/media/platform/vsp1/vsp1_regs.h @@ -693,6 +693,11 @@ #define VI6_LIF_CSBTH_LBTH_MASK (0x7ff << 0) #define VI6_LIF_CSBTH_LBTH_SHIFT 0 +#define VI6_LIF_LBA 0x3b0c +#define VI6_LIF_LBA_LBA0 (1 << 31) +#define VI6_LIF_LBA_LBA1_MASK (0xfff << 16) +#define VI6_LIF_LBA_LBA1_SHIFT 16 + /* ----------------------------------------------------------------------------- * Security Control Registers */
Laurent has added support for the VSP2-D found on R-Car V3M (R8A77970) but the video output that VSP2-D sends to DU has a greenish garbage-like line repeated every 8 or so screen rows. It turns out that V3M has a teeny LIF register (at least it's documented!) that you need to set to some kind of a magic value for the LIF to work correctly... Based on the original (and large) patch by Daisuke Matsushita <daisuke.matsushita.ns@hitachi.com>. Fixes: d455b45f8393 ("v4l: vsp1: Add support for new VSP2-BS, VSP2-DL and VSP2-D instances") Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> --- This patch is against the 'media_tree.git' repo's 'master' branch. Changes in version 2: - added a comment before the V3M SoC check; - fixed indetation in that check; - reformatted the patch description. drivers/media/platform/vsp1/vsp1_lif.c | 12 ++++++++++++ drivers/media/platform/vsp1/vsp1_regs.h | 5 +++++ 2 files changed, 17 insertions(+)