* [PATCH] media: rcar-csi2: Fix PHTW table values for E3/V3M
@ 2018-12-10 14:53 Jacopo Mondi
2018-12-10 15:24 ` Niklas Söderlund
2018-12-10 20:16 ` Laurent Pinchart
0 siblings, 2 replies; 6+ messages in thread
From: Jacopo Mondi @ 2018-12-10 14:53 UTC (permalink / raw)
To: niklas.soderlund, laurent.pinchart, kieran.bingham
Cc: Jacopo Mondi, linux-media, linux-renesas-soc
The PHTW selection algorithm implemented in rcsi2_phtw_write_mbps() checks for
lower bound of the interval used to match the desired bandwidth. Use that
in place of the currently used upport bound.
Fixes: 10c08812fe60 ("media: rcar: rcar-csi2: Update V3M/E3 PHTW tables")
Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
drivers/media/platform/rcar-vin/rcar-csi2.c | 62 ++++++++++++++---------------
1 file changed, 31 insertions(+), 31 deletions(-)
diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index 80ad906d1136..7e9cb8bcfe70 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -152,37 +152,37 @@ static const struct rcsi2_mbps_reg phtw_mbps_h3_v3h_m3n[] = {
};
static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
- { .mbps = 89, .reg = 0x00 },
- { .mbps = 99, .reg = 0x20 },
- { .mbps = 109, .reg = 0x40 },
- { .mbps = 129, .reg = 0x02 },
- { .mbps = 139, .reg = 0x22 },
- { .mbps = 149, .reg = 0x42 },
- { .mbps = 169, .reg = 0x04 },
- { .mbps = 179, .reg = 0x24 },
- { .mbps = 199, .reg = 0x44 },
- { .mbps = 219, .reg = 0x06 },
- { .mbps = 239, .reg = 0x26 },
- { .mbps = 249, .reg = 0x46 },
- { .mbps = 269, .reg = 0x08 },
- { .mbps = 299, .reg = 0x28 },
- { .mbps = 329, .reg = 0x0a },
- { .mbps = 359, .reg = 0x2a },
- { .mbps = 399, .reg = 0x4a },
- { .mbps = 449, .reg = 0x0c },
- { .mbps = 499, .reg = 0x2c },
- { .mbps = 549, .reg = 0x0e },
- { .mbps = 599, .reg = 0x2e },
- { .mbps = 649, .reg = 0x10 },
- { .mbps = 699, .reg = 0x30 },
- { .mbps = 749, .reg = 0x12 },
- { .mbps = 799, .reg = 0x32 },
- { .mbps = 849, .reg = 0x52 },
- { .mbps = 899, .reg = 0x72 },
- { .mbps = 949, .reg = 0x14 },
- { .mbps = 999, .reg = 0x34 },
- { .mbps = 1049, .reg = 0x54 },
- { .mbps = 1099, .reg = 0x74 },
+ { .mbps = 80, .reg = 0x00 },
+ { .mbps = 90, .reg = 0x20 },
+ { .mbps = 100, .reg = 0x40 },
+ { .mbps = 110, .reg = 0x02 },
+ { .mbps = 130, .reg = 0x22 },
+ { .mbps = 140, .reg = 0x42 },
+ { .mbps = 150, .reg = 0x04 },
+ { .mbps = 170, .reg = 0x24 },
+ { .mbps = 180, .reg = 0x44 },
+ { .mbps = 200, .reg = 0x06 },
+ { .mbps = 220, .reg = 0x26 },
+ { .mbps = 240, .reg = 0x46 },
+ { .mbps = 250, .reg = 0x08 },
+ { .mbps = 270, .reg = 0x28 },
+ { .mbps = 300, .reg = 0x0a },
+ { .mbps = 330, .reg = 0x2a },
+ { .mbps = 360, .reg = 0x4a },
+ { .mbps = 400, .reg = 0x0c },
+ { .mbps = 450, .reg = 0x2c },
+ { .mbps = 500, .reg = 0x0e },
+ { .mbps = 550, .reg = 0x2e },
+ { .mbps = 600, .reg = 0x10 },
+ { .mbps = 650, .reg = 0x30 },
+ { .mbps = 700, .reg = 0x12 },
+ { .mbps = 750, .reg = 0x32 },
+ { .mbps = 800, .reg = 0x52 },
+ { .mbps = 850, .reg = 0x72 },
+ { .mbps = 900, .reg = 0x14 },
+ { .mbps = 950, .reg = 0x34 },
+ { .mbps = 1000, .reg = 0x54 },
+ { .mbps = 1050, .reg = 0x74 },
{ .mbps = 1125, .reg = 0x16 },
{ /* sentinel */ },
};
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] media: rcar-csi2: Fix PHTW table values for E3/V3M 2018-12-10 14:53 [PATCH] media: rcar-csi2: Fix PHTW table values for E3/V3M Jacopo Mondi @ 2018-12-10 15:24 ` Niklas Söderlund 2018-12-10 20:16 ` Laurent Pinchart 1 sibling, 0 replies; 6+ messages in thread From: Niklas Söderlund @ 2018-12-10 15:24 UTC (permalink / raw) To: Jacopo Mondi Cc: laurent.pinchart, kieran.bingham, linux-media, linux-renesas-soc Hi Jacopo, Thanks for fixing this. On 2018-12-10 15:53:55 +0100, Jacopo Mondi wrote: > The PHTW selection algorithm implemented in rcsi2_phtw_write_mbps() checks for > lower bound of the interval used to match the desired bandwidth. Use that > in place of the currently used upport bound. > > Fixes: 10c08812fe60 ("media: rcar: rcar-csi2: Update V3M/E3 PHTW tables") > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/media/platform/rcar-vin/rcar-csi2.c | 62 ++++++++++++++--------------- > 1 file changed, 31 insertions(+), 31 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c > index 80ad906d1136..7e9cb8bcfe70 100644 > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c > @@ -152,37 +152,37 @@ static const struct rcsi2_mbps_reg phtw_mbps_h3_v3h_m3n[] = { > }; > > static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = { > - { .mbps = 89, .reg = 0x00 }, > - { .mbps = 99, .reg = 0x20 }, > - { .mbps = 109, .reg = 0x40 }, > - { .mbps = 129, .reg = 0x02 }, > - { .mbps = 139, .reg = 0x22 }, > - { .mbps = 149, .reg = 0x42 }, > - { .mbps = 169, .reg = 0x04 }, > - { .mbps = 179, .reg = 0x24 }, > - { .mbps = 199, .reg = 0x44 }, > - { .mbps = 219, .reg = 0x06 }, > - { .mbps = 239, .reg = 0x26 }, > - { .mbps = 249, .reg = 0x46 }, > - { .mbps = 269, .reg = 0x08 }, > - { .mbps = 299, .reg = 0x28 }, > - { .mbps = 329, .reg = 0x0a }, > - { .mbps = 359, .reg = 0x2a }, > - { .mbps = 399, .reg = 0x4a }, > - { .mbps = 449, .reg = 0x0c }, > - { .mbps = 499, .reg = 0x2c }, > - { .mbps = 549, .reg = 0x0e }, > - { .mbps = 599, .reg = 0x2e }, > - { .mbps = 649, .reg = 0x10 }, > - { .mbps = 699, .reg = 0x30 }, > - { .mbps = 749, .reg = 0x12 }, > - { .mbps = 799, .reg = 0x32 }, > - { .mbps = 849, .reg = 0x52 }, > - { .mbps = 899, .reg = 0x72 }, > - { .mbps = 949, .reg = 0x14 }, > - { .mbps = 999, .reg = 0x34 }, > - { .mbps = 1049, .reg = 0x54 }, > - { .mbps = 1099, .reg = 0x74 }, > + { .mbps = 80, .reg = 0x00 }, > + { .mbps = 90, .reg = 0x20 }, > + { .mbps = 100, .reg = 0x40 }, > + { .mbps = 110, .reg = 0x02 }, > + { .mbps = 130, .reg = 0x22 }, > + { .mbps = 140, .reg = 0x42 }, > + { .mbps = 150, .reg = 0x04 }, > + { .mbps = 170, .reg = 0x24 }, > + { .mbps = 180, .reg = 0x44 }, > + { .mbps = 200, .reg = 0x06 }, > + { .mbps = 220, .reg = 0x26 }, > + { .mbps = 240, .reg = 0x46 }, > + { .mbps = 250, .reg = 0x08 }, > + { .mbps = 270, .reg = 0x28 }, > + { .mbps = 300, .reg = 0x0a }, > + { .mbps = 330, .reg = 0x2a }, > + { .mbps = 360, .reg = 0x4a }, > + { .mbps = 400, .reg = 0x0c }, > + { .mbps = 450, .reg = 0x2c }, > + { .mbps = 500, .reg = 0x0e }, > + { .mbps = 550, .reg = 0x2e }, > + { .mbps = 600, .reg = 0x10 }, > + { .mbps = 650, .reg = 0x30 }, > + { .mbps = 700, .reg = 0x12 }, > + { .mbps = 750, .reg = 0x32 }, > + { .mbps = 800, .reg = 0x52 }, > + { .mbps = 850, .reg = 0x72 }, > + { .mbps = 900, .reg = 0x14 }, > + { .mbps = 950, .reg = 0x34 }, > + { .mbps = 1000, .reg = 0x54 }, > + { .mbps = 1050, .reg = 0x74 }, > { .mbps = 1125, .reg = 0x16 }, > { /* sentinel */ }, > }; > -- > 2.7.4 > -- Regards, Niklas Söderlund ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: rcar-csi2: Fix PHTW table values for E3/V3M 2018-12-10 14:53 [PATCH] media: rcar-csi2: Fix PHTW table values for E3/V3M Jacopo Mondi 2018-12-10 15:24 ` Niklas Söderlund @ 2018-12-10 20:16 ` Laurent Pinchart 2018-12-11 2:01 ` Niklas Söderlund 1 sibling, 1 reply; 6+ messages in thread From: Laurent Pinchart @ 2018-12-10 20:16 UTC (permalink / raw) To: Jacopo Mondi Cc: niklas.soderlund, kieran.bingham, linux-media, linux-renesas-soc Hi Jacopo, Thank you for the patch. On Monday, 10 December 2018 16:53:55 EET Jacopo Mondi wrote: > The PHTW selection algorithm implemented in rcsi2_phtw_write_mbps() checks > for lower bound of the interval used to match the desired bandwidth. Use > that in place of the currently used upport bound. The rcsi2_phtw_write_mbps() function performs the following (error handling removed): const struct rcsi2_mbps_reg *value; for (value = values; value->mbps; value++) if (value->mbps >= mbps) break; return rcsi2_phtw_write(priv, value->reg, code); With this patch, an mbps value of 85 will match the second entry in the phtw_mbps_v3m_e3 table: [0] { .mbps = 80, .reg = 0x00 }, [1] { .mbps = 90, .reg = 0x20 }, ... The datasheet however documents the range 80-89 to map to 0x00. What am I missing ? > Fixes: 10c08812fe60 ("media: rcar: rcar-csi2: Update V3M/E3 PHTW tables") > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > drivers/media/platform/rcar-vin/rcar-csi2.c | 62 ++++++++++++------------- > 1 file changed, 31 insertions(+), 31 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c > b/drivers/media/platform/rcar-vin/rcar-csi2.c index > 80ad906d1136..7e9cb8bcfe70 100644 > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c > @@ -152,37 +152,37 @@ static const struct rcsi2_mbps_reg > phtw_mbps_h3_v3h_m3n[] = { }; > > static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = { > - { .mbps = 89, .reg = 0x00 }, > - { .mbps = 99, .reg = 0x20 }, > - { .mbps = 109, .reg = 0x40 }, > - { .mbps = 129, .reg = 0x02 }, > - { .mbps = 139, .reg = 0x22 }, > - { .mbps = 149, .reg = 0x42 }, > - { .mbps = 169, .reg = 0x04 }, > - { .mbps = 179, .reg = 0x24 }, > - { .mbps = 199, .reg = 0x44 }, > - { .mbps = 219, .reg = 0x06 }, > - { .mbps = 239, .reg = 0x26 }, > - { .mbps = 249, .reg = 0x46 }, > - { .mbps = 269, .reg = 0x08 }, > - { .mbps = 299, .reg = 0x28 }, > - { .mbps = 329, .reg = 0x0a }, > - { .mbps = 359, .reg = 0x2a }, > - { .mbps = 399, .reg = 0x4a }, > - { .mbps = 449, .reg = 0x0c }, > - { .mbps = 499, .reg = 0x2c }, > - { .mbps = 549, .reg = 0x0e }, > - { .mbps = 599, .reg = 0x2e }, > - { .mbps = 649, .reg = 0x10 }, > - { .mbps = 699, .reg = 0x30 }, > - { .mbps = 749, .reg = 0x12 }, > - { .mbps = 799, .reg = 0x32 }, > - { .mbps = 849, .reg = 0x52 }, > - { .mbps = 899, .reg = 0x72 }, > - { .mbps = 949, .reg = 0x14 }, > - { .mbps = 999, .reg = 0x34 }, > - { .mbps = 1049, .reg = 0x54 }, > - { .mbps = 1099, .reg = 0x74 }, > + { .mbps = 80, .reg = 0x00 }, > + { .mbps = 90, .reg = 0x20 }, > + { .mbps = 100, .reg = 0x40 }, > + { .mbps = 110, .reg = 0x02 }, > + { .mbps = 130, .reg = 0x22 }, > + { .mbps = 140, .reg = 0x42 }, > + { .mbps = 150, .reg = 0x04 }, > + { .mbps = 170, .reg = 0x24 }, > + { .mbps = 180, .reg = 0x44 }, > + { .mbps = 200, .reg = 0x06 }, > + { .mbps = 220, .reg = 0x26 }, > + { .mbps = 240, .reg = 0x46 }, > + { .mbps = 250, .reg = 0x08 }, > + { .mbps = 270, .reg = 0x28 }, > + { .mbps = 300, .reg = 0x0a }, > + { .mbps = 330, .reg = 0x2a }, > + { .mbps = 360, .reg = 0x4a }, > + { .mbps = 400, .reg = 0x0c }, > + { .mbps = 450, .reg = 0x2c }, > + { .mbps = 500, .reg = 0x0e }, > + { .mbps = 550, .reg = 0x2e }, > + { .mbps = 600, .reg = 0x10 }, > + { .mbps = 650, .reg = 0x30 }, > + { .mbps = 700, .reg = 0x12 }, > + { .mbps = 750, .reg = 0x32 }, > + { .mbps = 800, .reg = 0x52 }, > + { .mbps = 850, .reg = 0x72 }, > + { .mbps = 900, .reg = 0x14 }, > + { .mbps = 950, .reg = 0x34 }, > + { .mbps = 1000, .reg = 0x54 }, > + { .mbps = 1050, .reg = 0x74 }, > { .mbps = 1125, .reg = 0x16 }, > { /* sentinel */ }, > }; -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: rcar-csi2: Fix PHTW table values for E3/V3M 2018-12-10 20:16 ` Laurent Pinchart @ 2018-12-11 2:01 ` Niklas Söderlund 2018-12-11 7:49 ` Laurent Pinchart 2018-12-11 11:33 ` jacopo mondi 0 siblings, 2 replies; 6+ messages in thread From: Niklas Söderlund @ 2018-12-11 2:01 UTC (permalink / raw) To: Laurent Pinchart Cc: Jacopo Mondi, kieran.bingham, linux-media, linux-renesas-soc Hi Laurent, Thanks for your feedback, On 2018-12-10 22:16:52 +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Monday, 10 December 2018 16:53:55 EET Jacopo Mondi wrote: > > The PHTW selection algorithm implemented in rcsi2_phtw_write_mbps() checks > > for lower bound of the interval used to match the desired bandwidth. Use > > that in place of the currently used upport bound. > > The rcsi2_phtw_write_mbps() function performs the following (error handling > removed): > > const struct rcsi2_mbps_reg *value; > > for (value = values; value->mbps; value++) > if (value->mbps >= mbps) > break; > > return rcsi2_phtw_write(priv, value->reg, code); > > With this patch, an mbps value of 85 will match the second entry in the > phtw_mbps_v3m_e3 table: > > [0] { .mbps = 80, .reg = 0x00 }, > [1] { .mbps = 90, .reg = 0x20 }, > ... > > The datasheet however documents the range 80-89 to map to 0x00. > > What am I missing ? I'm afraid you are missing a issue with the original implementation of the rcar-csi2 driver (my fault). The issue you point out is a problem with the current freq selection logic not the tables themself which needs to be corrected. This patch aligns the table with the other tables in the driver and is sound. A patch (Jacopo care to submit it?) is needed to resolve the faulty logic in the driver. It should select the range according to Laurents findings and not the range above it as the current code does. > > > Fixes: 10c08812fe60 ("media: rcar: rcar-csi2: Update V3M/E3 PHTW tables") > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > drivers/media/platform/rcar-vin/rcar-csi2.c | 62 ++++++++++++------------- > > 1 file changed, 31 insertions(+), 31 deletions(-) > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c > > b/drivers/media/platform/rcar-vin/rcar-csi2.c index > > 80ad906d1136..7e9cb8bcfe70 100644 > > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c > > @@ -152,37 +152,37 @@ static const struct rcsi2_mbps_reg > > phtw_mbps_h3_v3h_m3n[] = { }; > > > > static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = { > > - { .mbps = 89, .reg = 0x00 }, > > - { .mbps = 99, .reg = 0x20 }, > > - { .mbps = 109, .reg = 0x40 }, > > - { .mbps = 129, .reg = 0x02 }, > > - { .mbps = 139, .reg = 0x22 }, > > - { .mbps = 149, .reg = 0x42 }, > > - { .mbps = 169, .reg = 0x04 }, > > - { .mbps = 179, .reg = 0x24 }, > > - { .mbps = 199, .reg = 0x44 }, > > - { .mbps = 219, .reg = 0x06 }, > > - { .mbps = 239, .reg = 0x26 }, > > - { .mbps = 249, .reg = 0x46 }, > > - { .mbps = 269, .reg = 0x08 }, > > - { .mbps = 299, .reg = 0x28 }, > > - { .mbps = 329, .reg = 0x0a }, > > - { .mbps = 359, .reg = 0x2a }, > > - { .mbps = 399, .reg = 0x4a }, > > - { .mbps = 449, .reg = 0x0c }, > > - { .mbps = 499, .reg = 0x2c }, > > - { .mbps = 549, .reg = 0x0e }, > > - { .mbps = 599, .reg = 0x2e }, > > - { .mbps = 649, .reg = 0x10 }, > > - { .mbps = 699, .reg = 0x30 }, > > - { .mbps = 749, .reg = 0x12 }, > > - { .mbps = 799, .reg = 0x32 }, > > - { .mbps = 849, .reg = 0x52 }, > > - { .mbps = 899, .reg = 0x72 }, > > - { .mbps = 949, .reg = 0x14 }, > > - { .mbps = 999, .reg = 0x34 }, > > - { .mbps = 1049, .reg = 0x54 }, > > - { .mbps = 1099, .reg = 0x74 }, > > + { .mbps = 80, .reg = 0x00 }, > > + { .mbps = 90, .reg = 0x20 }, > > + { .mbps = 100, .reg = 0x40 }, > > + { .mbps = 110, .reg = 0x02 }, > > + { .mbps = 130, .reg = 0x22 }, > > + { .mbps = 140, .reg = 0x42 }, > > + { .mbps = 150, .reg = 0x04 }, > > + { .mbps = 170, .reg = 0x24 }, > > + { .mbps = 180, .reg = 0x44 }, > > + { .mbps = 200, .reg = 0x06 }, > > + { .mbps = 220, .reg = 0x26 }, > > + { .mbps = 240, .reg = 0x46 }, > > + { .mbps = 250, .reg = 0x08 }, > > + { .mbps = 270, .reg = 0x28 }, > > + { .mbps = 300, .reg = 0x0a }, > > + { .mbps = 330, .reg = 0x2a }, > > + { .mbps = 360, .reg = 0x4a }, > > + { .mbps = 400, .reg = 0x0c }, > > + { .mbps = 450, .reg = 0x2c }, > > + { .mbps = 500, .reg = 0x0e }, > > + { .mbps = 550, .reg = 0x2e }, > > + { .mbps = 600, .reg = 0x10 }, > > + { .mbps = 650, .reg = 0x30 }, > > + { .mbps = 700, .reg = 0x12 }, > > + { .mbps = 750, .reg = 0x32 }, > > + { .mbps = 800, .reg = 0x52 }, > > + { .mbps = 850, .reg = 0x72 }, > > + { .mbps = 900, .reg = 0x14 }, > > + { .mbps = 950, .reg = 0x34 }, > > + { .mbps = 1000, .reg = 0x54 }, > > + { .mbps = 1050, .reg = 0x74 }, > > { .mbps = 1125, .reg = 0x16 }, > > { /* sentinel */ }, > > }; > > -- > Regards, > > Laurent Pinchart > > > -- Regards, Niklas Söderlund ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: rcar-csi2: Fix PHTW table values for E3/V3M 2018-12-11 2:01 ` Niklas Söderlund @ 2018-12-11 7:49 ` Laurent Pinchart 2018-12-11 11:33 ` jacopo mondi 1 sibling, 0 replies; 6+ messages in thread From: Laurent Pinchart @ 2018-12-11 7:49 UTC (permalink / raw) To: Niklas Söderlund Cc: Jacopo Mondi, kieran.bingham, linux-media, linux-renesas-soc Hi Niklas, On Tuesday, 11 December 2018 04:01:15 EET Niklas Söderlund wrote: > On 2018-12-10 22:16:52 +0200, Laurent Pinchart wrote: > > On Monday, 10 December 2018 16:53:55 EET Jacopo Mondi wrote: > >> The PHTW selection algorithm implemented in rcsi2_phtw_write_mbps() > >> checks for lower bound of the interval used to match the desired > >> bandwidth. Use that in place of the currently used upport bound. > > > > The rcsi2_phtw_write_mbps() function performs the following (error > > handling removed): > > > > const struct rcsi2_mbps_reg *value; > > > > for (value = values; value->mbps; value++) > > if (value->mbps >= mbps) > > break; > > > > return rcsi2_phtw_write(priv, value->reg, code); > > > > With this patch, an mbps value of 85 will match the second entry in the > > phtw_mbps_v3m_e3 table: > > > > [0] { .mbps = 80, .reg = 0x00 }, > > [1] { .mbps = 90, .reg = 0x20 }, > > ... > > > > The datasheet however documents the range 80-89 to map to 0x00. > > > > What am I missing ? > > I'm afraid you are missing a issue with the original implementation of > the rcar-csi2 driver (my fault). The issue you point out is a problem > with the current freq selection logic not the tables themself which > needs to be corrected. > > This patch aligns the table with the other tables in the driver and is > sound. A patch (Jacopo care to submit it?) is needed to resolve the > faulty logic in the driver. It should select the range according to > Laurents findings and not the range above it as the current code does. I wonder whether we should instead modify the tables, to avoid making the selection logic more complicated and less efficient CPU-wise. Speaking of which, the tables are interestingly specified in three different ways in the datasheet: - The TESTDIN_DATA table specifies non-overlapping ranges e.g. [80, 89]: 0x00, [90, 99]: 0x20, [100, 109]: 0x40, ... In this regard this patch is an improvement (that is if the faulty selection logic gets fixed), as otherwise a frequency of 109.5 would be classified in the [110, 129] range, while I think it is meant to be in the [100, 109] range. Another option would be to set the table mbps value to the high bound of the range: { .mbps = 90, .reg = 0x00 }, { .mbps = 100, .reg = 0x20 }, { .mbps = 110, .reg = 0x40 }, ... and use strict lower comparison logic: const struct rcsi2_mbps_reg *value; for (value = values; value->mbps; value++) if (mbps < value->mbps) break - The PHTW table specifies individual bit rates e.g. 80: 0x86, 90: 0x86, 100: 0x87, .. I'm not sure how to interpret this. If I had to guess, I would say it means [80, 90[: 0x86, [90, 100[: 0x86, [100, 110[: 0x87, .. We could thus use the same logic than for the TESTDIN_DATA table (and while at it merge adjacent ranges that share the same PHTW value). - The HSFREQRANGE tables specify overlapping ranges e.g. [80, 131.25]: 0x20, [80.75, 141.75]: 0x30, [90.25, 152.25]: 0x01, ... This has to be converted to non-overlapping ranges. I would advice centering each non-overlapping range to the center of the corresponding overlapping range. Today we instead have non-overlapping ranges in the driver whose bounds are set to the center of the overlapping ranges, and I don't think this is right. And we could then use the same logic as above here too. The downside is that the tables would need to be carefully reviewed as they would derive from the values in the datasheet instead of computing them blindly, and the upside would be simpler code. If we want to instead copy the tables blindly for ease of review, then I think we'll need more complex selection logic, with different logics for the different tables. > >> Fixes: 10c08812fe60 ("media: rcar: rcar-csi2: Update V3M/E3 PHTW > >> tables") > >> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > >> --- > >> > >> drivers/media/platform/rcar-vin/rcar-csi2.c | 62 +++++++++++------------ > >> 1 file changed, 31 insertions(+), 31 deletions(-) > >> > >> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c > >> b/drivers/media/platform/rcar-vin/rcar-csi2.c index > >> 80ad906d1136..7e9cb8bcfe70 100644 > >> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c > >> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c > >> @@ -152,37 +152,37 @@ static const struct rcsi2_mbps_reg > >> phtw_mbps_h3_v3h_m3n[] = { }; > >> > >> static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = { > >> - { .mbps = 89, .reg = 0x00 }, > >> - { .mbps = 99, .reg = 0x20 }, > >> - { .mbps = 109, .reg = 0x40 }, > >> - { .mbps = 129, .reg = 0x02 }, > >> - { .mbps = 139, .reg = 0x22 }, > >> - { .mbps = 149, .reg = 0x42 }, > >> - { .mbps = 169, .reg = 0x04 }, > >> - { .mbps = 179, .reg = 0x24 }, > >> - { .mbps = 199, .reg = 0x44 }, > >> - { .mbps = 219, .reg = 0x06 }, > >> - { .mbps = 239, .reg = 0x26 }, > >> - { .mbps = 249, .reg = 0x46 }, > >> - { .mbps = 269, .reg = 0x08 }, > >> - { .mbps = 299, .reg = 0x28 }, > >> - { .mbps = 329, .reg = 0x0a }, > >> - { .mbps = 359, .reg = 0x2a }, > >> - { .mbps = 399, .reg = 0x4a }, > >> - { .mbps = 449, .reg = 0x0c }, > >> - { .mbps = 499, .reg = 0x2c }, > >> - { .mbps = 549, .reg = 0x0e }, > >> - { .mbps = 599, .reg = 0x2e }, > >> - { .mbps = 649, .reg = 0x10 }, > >> - { .mbps = 699, .reg = 0x30 }, > >> - { .mbps = 749, .reg = 0x12 }, > >> - { .mbps = 799, .reg = 0x32 }, > >> - { .mbps = 849, .reg = 0x52 }, > >> - { .mbps = 899, .reg = 0x72 }, > >> - { .mbps = 949, .reg = 0x14 }, > >> - { .mbps = 999, .reg = 0x34 }, > >> - { .mbps = 1049, .reg = 0x54 }, > >> - { .mbps = 1099, .reg = 0x74 }, > >> + { .mbps = 80, .reg = 0x00 }, > >> + { .mbps = 90, .reg = 0x20 }, > >> + { .mbps = 100, .reg = 0x40 }, > >> + { .mbps = 110, .reg = 0x02 }, > >> + { .mbps = 130, .reg = 0x22 }, > >> + { .mbps = 140, .reg = 0x42 }, > >> + { .mbps = 150, .reg = 0x04 }, > >> + { .mbps = 170, .reg = 0x24 }, > >> + { .mbps = 180, .reg = 0x44 }, > >> + { .mbps = 200, .reg = 0x06 }, > >> + { .mbps = 220, .reg = 0x26 }, > >> + { .mbps = 240, .reg = 0x46 }, > >> + { .mbps = 250, .reg = 0x08 }, > >> + { .mbps = 270, .reg = 0x28 }, > >> + { .mbps = 300, .reg = 0x0a }, > >> + { .mbps = 330, .reg = 0x2a }, > >> + { .mbps = 360, .reg = 0x4a }, > >> + { .mbps = 400, .reg = 0x0c }, > >> + { .mbps = 450, .reg = 0x2c }, > >> + { .mbps = 500, .reg = 0x0e }, > >> + { .mbps = 550, .reg = 0x2e }, > >> + { .mbps = 600, .reg = 0x10 }, > >> + { .mbps = 650, .reg = 0x30 }, > >> + { .mbps = 700, .reg = 0x12 }, > >> + { .mbps = 750, .reg = 0x32 }, > >> + { .mbps = 800, .reg = 0x52 }, > >> + { .mbps = 850, .reg = 0x72 }, > >> + { .mbps = 900, .reg = 0x14 }, > >> + { .mbps = 950, .reg = 0x34 }, > >> + { .mbps = 1000, .reg = 0x54 }, > >> + { .mbps = 1050, .reg = 0x74 }, > >> { .mbps = 1125, .reg = 0x16 }, > >> { /* sentinel */ }, > >> > >> }; -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: rcar-csi2: Fix PHTW table values for E3/V3M 2018-12-11 2:01 ` Niklas Söderlund 2018-12-11 7:49 ` Laurent Pinchart @ 2018-12-11 11:33 ` jacopo mondi 1 sibling, 0 replies; 6+ messages in thread From: jacopo mondi @ 2018-12-11 11:33 UTC (permalink / raw) To: Niklas Söderlund Cc: Laurent Pinchart, Jacopo Mondi, kieran.bingham, linux-media, linux-renesas-soc [-- Attachment #1: Type: text/plain, Size: 5705 bytes --] Hi Laurent, Niklas, On Tue, Dec 11, 2018 at 03:01:15AM +0100, Niklas Söderlund wrote: > Hi Laurent, > > Thanks for your feedback, > > On 2018-12-10 22:16:52 +0200, Laurent Pinchart wrote: > > Hi Jacopo, > > > > Thank you for the patch. > > > > On Monday, 10 December 2018 16:53:55 EET Jacopo Mondi wrote: > > > The PHTW selection algorithm implemented in rcsi2_phtw_write_mbps() checks > > > for lower bound of the interval used to match the desired bandwidth. Use > > > that in place of the currently used upport bound. > > > > The rcsi2_phtw_write_mbps() function performs the following (error handling > > removed): > > > > const struct rcsi2_mbps_reg *value; > > > > for (value = values; value->mbps; value++) > > if (value->mbps >= mbps) > > break; > > > > return rcsi2_phtw_write(priv, value->reg, code); > > > > With this patch, an mbps value of 85 will match the second entry in the > > phtw_mbps_v3m_e3 table: > > > > [0] { .mbps = 80, .reg = 0x00 }, > > [1] { .mbps = 90, .reg = 0x20 }, > > ... > > > > The datasheet however documents the range 80-89 to map to 0x00. > > > > What am I missing ? > > I'm afraid you are missing a issue with the original implementation of > the rcar-csi2 driver (my fault). The issue you point out is a problem > with the current freq selection logic not the tables themself which > needs to be corrected. > > This patch aligns the table with the other tables in the driver and is > sound. A patch (Jacopo care to submit it?) is needed to resolve the > faulty logic in the driver. It should select the range according to > Laurents findings and not the range above it as the current code does. > > > I've been notified my previous patch broke the matching logic in the CSI-2 driver, and so to keep it consistent, I basically reverted what I've done. Anyway, a re-work of those matching loop and tables is welcome, and as you suggested in your last email, it should be done not only for PHTW but for other tables as well. In the meantime, I think this patch should go in (afaik there have been no functional changes on E3/V3M) so that we start from a (slightly suboptimal) fixed point. Thanks j > > > Fixes: 10c08812fe60 ("media: rcar: rcar-csi2: Update V3M/E3 PHTW tables") > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > > --- > > > drivers/media/platform/rcar-vin/rcar-csi2.c | 62 ++++++++++++------------- > > > 1 file changed, 31 insertions(+), 31 deletions(-) > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c > > > b/drivers/media/platform/rcar-vin/rcar-csi2.c index > > > 80ad906d1136..7e9cb8bcfe70 100644 > > > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c > > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c > > > @@ -152,37 +152,37 @@ static const struct rcsi2_mbps_reg > > > phtw_mbps_h3_v3h_m3n[] = { }; > > > > > > static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = { > > > - { .mbps = 89, .reg = 0x00 }, > > > - { .mbps = 99, .reg = 0x20 }, > > > - { .mbps = 109, .reg = 0x40 }, > > > - { .mbps = 129, .reg = 0x02 }, > > > - { .mbps = 139, .reg = 0x22 }, > > > - { .mbps = 149, .reg = 0x42 }, > > > - { .mbps = 169, .reg = 0x04 }, > > > - { .mbps = 179, .reg = 0x24 }, > > > - { .mbps = 199, .reg = 0x44 }, > > > - { .mbps = 219, .reg = 0x06 }, > > > - { .mbps = 239, .reg = 0x26 }, > > > - { .mbps = 249, .reg = 0x46 }, > > > - { .mbps = 269, .reg = 0x08 }, > > > - { .mbps = 299, .reg = 0x28 }, > > > - { .mbps = 329, .reg = 0x0a }, > > > - { .mbps = 359, .reg = 0x2a }, > > > - { .mbps = 399, .reg = 0x4a }, > > > - { .mbps = 449, .reg = 0x0c }, > > > - { .mbps = 499, .reg = 0x2c }, > > > - { .mbps = 549, .reg = 0x0e }, > > > - { .mbps = 599, .reg = 0x2e }, > > > - { .mbps = 649, .reg = 0x10 }, > > > - { .mbps = 699, .reg = 0x30 }, > > > - { .mbps = 749, .reg = 0x12 }, > > > - { .mbps = 799, .reg = 0x32 }, > > > - { .mbps = 849, .reg = 0x52 }, > > > - { .mbps = 899, .reg = 0x72 }, > > > - { .mbps = 949, .reg = 0x14 }, > > > - { .mbps = 999, .reg = 0x34 }, > > > - { .mbps = 1049, .reg = 0x54 }, > > > - { .mbps = 1099, .reg = 0x74 }, > > > + { .mbps = 80, .reg = 0x00 }, > > > + { .mbps = 90, .reg = 0x20 }, > > > + { .mbps = 100, .reg = 0x40 }, > > > + { .mbps = 110, .reg = 0x02 }, > > > + { .mbps = 130, .reg = 0x22 }, > > > + { .mbps = 140, .reg = 0x42 }, > > > + { .mbps = 150, .reg = 0x04 }, > > > + { .mbps = 170, .reg = 0x24 }, > > > + { .mbps = 180, .reg = 0x44 }, > > > + { .mbps = 200, .reg = 0x06 }, > > > + { .mbps = 220, .reg = 0x26 }, > > > + { .mbps = 240, .reg = 0x46 }, > > > + { .mbps = 250, .reg = 0x08 }, > > > + { .mbps = 270, .reg = 0x28 }, > > > + { .mbps = 300, .reg = 0x0a }, > > > + { .mbps = 330, .reg = 0x2a }, > > > + { .mbps = 360, .reg = 0x4a }, > > > + { .mbps = 400, .reg = 0x0c }, > > > + { .mbps = 450, .reg = 0x2c }, > > > + { .mbps = 500, .reg = 0x0e }, > > > + { .mbps = 550, .reg = 0x2e }, > > > + { .mbps = 600, .reg = 0x10 }, > > > + { .mbps = 650, .reg = 0x30 }, > > > + { .mbps = 700, .reg = 0x12 }, > > > + { .mbps = 750, .reg = 0x32 }, > > > + { .mbps = 800, .reg = 0x52 }, > > > + { .mbps = 850, .reg = 0x72 }, > > > + { .mbps = 900, .reg = 0x14 }, > > > + { .mbps = 950, .reg = 0x34 }, > > > + { .mbps = 1000, .reg = 0x54 }, > > > + { .mbps = 1050, .reg = 0x74 }, > > > { .mbps = 1125, .reg = 0x16 }, > > > { /* sentinel */ }, > > > }; > > > > -- > > Regards, > > > > Laurent Pinchart > > > > > > > > -- > Regards, > Niklas Söderlund [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-12-11 11:33 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-12-10 14:53 [PATCH] media: rcar-csi2: Fix PHTW table values for E3/V3M Jacopo Mondi 2018-12-10 15:24 ` Niklas Söderlund 2018-12-10 20:16 ` Laurent Pinchart 2018-12-11 2:01 ` Niklas Söderlund 2018-12-11 7:49 ` Laurent Pinchart 2018-12-11 11:33 ` jacopo mondi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).