[PATCH] [RFC] media: rcar-csi2: Corrects the selection of hsfreqrange

[PATCH] [RFC] media: rcar-csi2: Corrects the selection of hsfreqrange

[Suresh Udipi]

hsfreqrange should be chosen based on the calculated mbps which
is closer to the default bit rate  and within the range as per
table[1]. But current calculation always selects fist value which
greater than the calcuated mbps which may lead to chosing a
wrong range in some cases.

For example for 360 mbps for H3/M3N
Existing logic selects
Calculated value 360Mbps : Default 400Mbps Range [368.125 -433.125 mbps]

This hsfreqrange is buggy and also out of range.

The logic is changed to get the default value which is closest to the
calculated value [1]

Calculate value 360Mbps : Default 350Mbps  Range [320.625 -380.625 mpbs]

 [1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]

Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
Signed-off-by: Kazuyoshi Akiyama <akiyama@nds-osk.co.jp>
---
 drivers/media/platform/rcar-vin/rcar-csi2.c | 185 +++++++++++++++-------------
 1 file changed, 97 insertions(+), 88 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index faa9fb2..1966ece 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -134,6 +134,8 @@ struct phtw_value {
 struct rcsi2_mbps_reg {
 	u16 mbps;
 	u16 reg;
+	u16 min;
+	u16 max;
 };
 
 static const struct rcsi2_mbps_reg phtw_mbps_h3_v3h_m3n[] = {
@@ -201,96 +203,96 @@ static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
 #define PHYPLL_HSFREQRANGE(n)		((n) << 16)
 
 static const struct rcsi2_mbps_reg hsfreqrange_h3_v3h_m3n[] = {
-	{ .mbps =   80, .reg = 0x00 },
-	{ .mbps =   90, .reg = 0x10 },
-	{ .mbps =  100, .reg = 0x20 },
-	{ .mbps =  110, .reg = 0x30 },
-	{ .mbps =  120, .reg = 0x01 },
-	{ .mbps =  130, .reg = 0x11 },
-	{ .mbps =  140, .reg = 0x21 },
-	{ .mbps =  150, .reg = 0x31 },
-	{ .mbps =  160, .reg = 0x02 },
-	{ .mbps =  170, .reg = 0x12 },
-	{ .mbps =  180, .reg = 0x22 },
-	{ .mbps =  190, .reg = 0x32 },
-	{ .mbps =  205, .reg = 0x03 },
-	{ .mbps =  220, .reg = 0x13 },
-	{ .mbps =  235, .reg = 0x23 },
-	{ .mbps =  250, .reg = 0x33 },
-	{ .mbps =  275, .reg = 0x04 },
-	{ .mbps =  300, .reg = 0x14 },
-	{ .mbps =  325, .reg = 0x25 },
-	{ .mbps =  350, .reg = 0x35 },
-	{ .mbps =  400, .reg = 0x05 },
-	{ .mbps =  450, .reg = 0x16 },
-	{ .mbps =  500, .reg = 0x26 },
-	{ .mbps =  550, .reg = 0x37 },
-	{ .mbps =  600, .reg = 0x07 },
-	{ .mbps =  650, .reg = 0x18 },
-	{ .mbps =  700, .reg = 0x28 },
-	{ .mbps =  750, .reg = 0x39 },
-	{ .mbps =  800, .reg = 0x09 },
-	{ .mbps =  850, .reg = 0x19 },
-	{ .mbps =  900, .reg = 0x29 },
-	{ .mbps =  950, .reg = 0x3a },
-	{ .mbps = 1000, .reg = 0x0a },
-	{ .mbps = 1050, .reg = 0x1a },
-	{ .mbps = 1100, .reg = 0x2a },
-	{ .mbps = 1150, .reg = 0x3b },
-	{ .mbps = 1200, .reg = 0x0b },
-	{ .mbps = 1250, .reg = 0x1b },
-	{ .mbps = 1300, .reg = 0x2b },
-	{ .mbps = 1350, .reg = 0x3c },
-	{ .mbps = 1400, .reg = 0x0c },
-	{ .mbps = 1450, .reg = 0x1c },
-	{ .mbps = 1500, .reg = 0x2c },
+	{ .mbps =   80, .reg = 0x00, .min =   80, .max =   97 },
+	{ .mbps =   90, .reg = 0x10, .min =   80, .max =  107 },
+	{ .mbps =  100, .reg = 0x20, .min =   83, .max =  118 },
+	{ .mbps =  110, .reg = 0x30, .min =   93, .max =  128 },
+	{ .mbps =  120, .reg = 0x01, .min =  103, .max =  139 },
+	{ .mbps =  130, .reg = 0x11, .min =  112, .max =  149 },
+	{ .mbps =  140, .reg = 0x21, .min =  122, .max =  160 },
+	{ .mbps =  150, .reg = 0x31, .min =  131, .max =  170 },
+	{ .mbps =  160, .reg = 0x02, .min =  141, .max =  181 },
+	{ .mbps =  170, .reg = 0x12, .min =  150, .max =  191 },
+	{ .mbps =  180, .reg = 0x22, .min =  160, .max =  202 },
+	{ .mbps =  190, .reg = 0x32, .min =  169, .max =  212 },
+	{ .mbps =  205, .reg = 0x03, .min =  183, .max =  228 },
+	{ .mbps =  220, .reg = 0x13, .min =  198, .max =  224 },
+	{ .mbps =  235, .reg = 0x23, .min =  212, .max =  259 },
+	{ .mbps =  250, .reg = 0x33, .min =  226, .max =  275 },
+	{ .mbps =  275, .reg = 0x04, .min =  250, .max =  301 },
+	{ .mbps =  300, .reg = 0x14, .min =  274, .max =  328 },
+	{ .mbps =  325, .reg = 0x25, .min =  297, .max =  354 },
+	{ .mbps =  350, .reg = 0x35, .min =  321, .max =  380 },
+	{ .mbps =  400, .reg = 0x05, .min =  369, .max =  433 },
+	{ .mbps =  450, .reg = 0x16, .min =  416, .max =  485 },
+	{ .mbps =  500, .reg = 0x26, .min =  464, .max =  538 },
+	{ .mbps =  550, .reg = 0x37, .min =  499, .max =  590 },
+	{ .mbps =  600, .reg = 0x07, .min =  559, .max =  643 },
+	{ .mbps =  650, .reg = 0x18, .min =  606, .max =  695 },
+	{ .mbps =  700, .reg = 0x28, .min =  654, .max =  748 },
+	{ .mbps =  750, .reg = 0x39, .min =  701, .max =  800 },
+	{ .mbps =  800, .reg = 0x09, .min =  749, .max =  853 },
+	{ .mbps =  850, .reg = 0x19, .min =  796, .max =  905 },
+	{ .mbps =  900, .reg = 0x29, .min =  844, .max =  958 },
+	{ .mbps =  950, .reg = 0x3a, .min =  891, .max = 1010 },
+	{ .mbps = 1000, .reg = 0x0a, .min =  939, .max = 1063 },
+	{ .mbps = 1050, .reg = 0x1a, .min =  986, .max = 1115 },
+	{ .mbps = 1100, .reg = 0x2a, .min = 1034, .max = 1168 },
+	{ .mbps = 1150, .reg = 0x3b, .min = 1081, .max = 1220 },
+	{ .mbps = 1200, .reg = 0x0b, .min = 1129, .max = 1273 },
+	{ .mbps = 1250, .reg = 0x1b, .min = 1176, .max = 1325 },
+	{ .mbps = 1300, .reg = 0x2b, .min = 1212, .max = 1378 },
+	{ .mbps = 1350, .reg = 0x3c, .min = 1271, .max = 1430 },
+	{ .mbps = 1400, .reg = 0x0c, .min = 1319, .max = 1483 },
+	{ .mbps = 1450, .reg = 0x1c, .min = 1366, .max = 1500 },
+	{ .mbps = 1500, .reg = 0x2c, .min = 1414, .max = 1500 },
 	{ /* sentinel */ },
 };
 
 static const struct rcsi2_mbps_reg hsfreqrange_m3w_h3es1[] = {
-	{ .mbps =   80,	.reg = 0x00 },
-	{ .mbps =   90,	.reg = 0x10 },
-	{ .mbps =  100,	.reg = 0x20 },
-	{ .mbps =  110,	.reg = 0x30 },
-	{ .mbps =  120,	.reg = 0x01 },
-	{ .mbps =  130,	.reg = 0x11 },
-	{ .mbps =  140,	.reg = 0x21 },
-	{ .mbps =  150,	.reg = 0x31 },
-	{ .mbps =  160,	.reg = 0x02 },
-	{ .mbps =  170,	.reg = 0x12 },
-	{ .mbps =  180,	.reg = 0x22 },
-	{ .mbps =  190,	.reg = 0x32 },
-	{ .mbps =  205,	.reg = 0x03 },
-	{ .mbps =  220,	.reg = 0x13 },
-	{ .mbps =  235,	.reg = 0x23 },
-	{ .mbps =  250,	.reg = 0x33 },
-	{ .mbps =  275,	.reg = 0x04 },
-	{ .mbps =  300,	.reg = 0x14 },
-	{ .mbps =  325,	.reg = 0x05 },
-	{ .mbps =  350,	.reg = 0x15 },
-	{ .mbps =  400,	.reg = 0x25 },
-	{ .mbps =  450,	.reg = 0x06 },
-	{ .mbps =  500,	.reg = 0x16 },
-	{ .mbps =  550,	.reg = 0x07 },
-	{ .mbps =  600,	.reg = 0x17 },
-	{ .mbps =  650,	.reg = 0x08 },
-	{ .mbps =  700,	.reg = 0x18 },
-	{ .mbps =  750,	.reg = 0x09 },
-	{ .mbps =  800,	.reg = 0x19 },
-	{ .mbps =  850,	.reg = 0x29 },
-	{ .mbps =  900,	.reg = 0x39 },
-	{ .mbps =  950,	.reg = 0x0a },
-	{ .mbps = 1000,	.reg = 0x1a },
-	{ .mbps = 1050,	.reg = 0x2a },
-	{ .mbps = 1100,	.reg = 0x3a },
-	{ .mbps = 1150,	.reg = 0x0b },
-	{ .mbps = 1200,	.reg = 0x1b },
-	{ .mbps = 1250,	.reg = 0x2b },
-	{ .mbps = 1300,	.reg = 0x3b },
-	{ .mbps = 1350,	.reg = 0x0c },
-	{ .mbps = 1400,	.reg = 0x1c },
-	{ .mbps = 1450,	.reg = 0x2c },
-	{ .mbps = 1500,	.reg = 0x3c },
+	{ .mbps =   80,	.reg = 0x00, .min =   80,  .max =   110 },
+	{ .mbps =   90,	.reg = 0x10, .min =   80,  .max =   120 },
+	{ .mbps =  100,	.reg = 0x20, .min =   80,  .max =   131 },
+	{ .mbps =  110,	.reg = 0x30, .min =   81,  .max =   141 },
+	{ .mbps =  120,	.reg = 0x01, .min =   91,  .max =   152 },
+	{ .mbps =  130,	.reg = 0x11, .min =  100,  .max =   162 },
+	{ .mbps =  140,	.reg = 0x21, .min =  110,  .max =   173 },
+	{ .mbps =  150,	.reg = 0x31, .min =  119,  .max =   183 },
+	{ .mbps =  160,	.reg = 0x02, .min =  129,  .max =   194 },
+	{ .mbps =  170,	.reg = 0x12, .min =  138,  .max =   204 },
+	{ .mbps =  180,	.reg = 0x22, .min =  148,  .max =   215 },
+	{ .mbps =  190,	.reg = 0x32, .min =  157,  .max =   225 },
+	{ .mbps =  205,	.reg = 0x03, .min =  171,  .max =   241 },
+	{ .mbps =  220,	.reg = 0x13, .min =  186,  .max =   257 },
+	{ .mbps =  235,	.reg = 0x23, .min =  200,  .max =   273 },
+	{ .mbps =  250,	.reg = 0x33, .min =  238,  .max =   276 },
+	{ .mbps =  275,	.reg = 0x04, .min =  250,  .max =   301 },
+	{ .mbps =  300,	.reg = 0x14, .min =  274,  .max =   328 },
+	{ .mbps =  325,	.reg = 0x05, .min =  297,  .max =   354 },
+	{ .mbps =  350,	.reg = 0x15, .min =  321,  .max =   393 },
+	{ .mbps =  400,	.reg = 0x25, .min =  357,  .max =   446 },
+	{ .mbps =  450,	.reg = 0x06, .min =  404,  .max =   498 },
+	{ .mbps =  500,	.reg = 0x16, .min =  452,  .max =   551 },
+	{ .mbps =  550,	.reg = 0x07, .min =  499,  .max =   603 },
+	{ .mbps =  600,	.reg = 0x17, .min =  547,  .max =   656 },
+	{ .mbps =  650,	.reg = 0x08, .min =  594,  .max =   708 },
+	{ .mbps =  700,	.reg = 0x18, .min =  642,  .max =   761 },
+	{ .mbps =  750,	.reg = 0x09, .min =  689,  .max =   813 },
+	{ .mbps =  800,	.reg = 0x19, .min =  737,  .max =   866 },
+	{ .mbps =  850,	.reg = 0x29, .min =  784,  .max =   918 },
+	{ .mbps =  900,	.reg = 0x39, .min =  832,  .max =   971 },
+	{ .mbps =  950,	.reg = 0x0a, .min =  879,  .max =  1023 },
+	{ .mbps = 1000,	.reg = 0x1a, .min =  927,  .max =  1076 },
+	{ .mbps = 1050,	.reg = 0x2a, .min =  974,  .max =  1128 },
+	{ .mbps = 1100,	.reg = 0x3a, .min = 1022,  .max =  1181 },
+	{ .mbps = 1150,	.reg = 0x0b, .min = 1069,  .max =  1233 },
+	{ .mbps = 1200,	.reg = 0x1b, .min = 1117,  .max =  1286 },
+	{ .mbps = 1250,	.reg = 0x2b, .min = 1164,  .max =  1338 },
+	{ .mbps = 1300,	.reg = 0x3b, .min = 1212,  .max =  1391 },
+	{ .mbps = 1350,	.reg = 0x0c, .min = 1259,  .max =  1443 },
+	{ .mbps = 1400,	.reg = 0x1c, .min = 1307,  .max =  1496 },
+	{ .mbps = 1450,	.reg = 0x2c, .min = 1354,  .max =  1500 },
+	{ .mbps = 1500,	.reg = 0x3c, .min = 1402,  .max =  1500 },
 	{ /* sentinel */ },
 };
 
@@ -430,11 +432,18 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
 
 static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
 {
-	const struct rcsi2_mbps_reg *hsfreq;
+	const struct rcsi2_mbps_reg *hsfreq, *hsfreq_prev = NULL;
 
-	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
+	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) {
 		if (hsfreq->mbps >= mbps)
 			break;
+		hsfreq_prev = hsfreq;
+	}
+
+	if (hsfreq_prev &&
+	    ((mbps - hsfreq_prev->mbps) <= (hsfreq->mbps - mbps)) &&
+	    (hsfreq_prev->max >= mbps && hsfreq_prev->min <= mbps))
+		hsfreq = hsfreq_prev;
 
 	if (!hsfreq->mbps) {
 		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
-- 
2.7.4

Re: [PATCH] [RFC] media: rcar-csi2: Corrects the selection of hsfreqrange

[Niklas Söderlund]

Hi Suresh,

Thanks for your work!

On 2020-02-20 13:49:09 +0900, Suresh Udipi wrote:
> hsfreqrange should be chosen based on the calculated mbps which
> is closer to the default bit rate  and within the range as per
> table[1]. But current calculation always selects fist value which
> greater than the calcuated mbps which may lead to chosing a
> wrong range in some cases.
> 
> For example for 360 mbps for H3/M3N
> Existing logic selects
> Calculated value 360Mbps : Default 400Mbps Range [368.125 -433.125 mbps]
> 
> This hsfreqrange is buggy and also out of range.
> 
> The logic is changed to get the default value which is closest to the
> calculated value [1]
> 
> Calculate value 360Mbps : Default 350Mbps  Range [320.625 -380.625 mpbs]
> 
>  [1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]

It's great that you are addressing this issue. Out of curiosity what 
hardware setup and configuration have you used to trigger the issue (if 
any)?

> 
> Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
> Signed-off-by: Kazuyoshi Akiyama <akiyama@nds-osk.co.jp>
> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 185 +++++++++++++++-------------
>  1 file changed, 97 insertions(+), 88 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index faa9fb2..1966ece 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -134,6 +134,8 @@ struct phtw_value {
>  struct rcsi2_mbps_reg {
>  	u16 mbps;
>  	u16 reg;
> +	u16 min;
> +	u16 max;
>  };
>  
>  static const struct rcsi2_mbps_reg phtw_mbps_h3_v3h_m3n[] = {
> @@ -201,96 +203,96 @@ static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
>  #define PHYPLL_HSFREQRANGE(n)		((n) << 16)
>  
>  static const struct rcsi2_mbps_reg hsfreqrange_h3_v3h_m3n[] = {
> -	{ .mbps =   80, .reg = 0x00 },
> -	{ .mbps =   90, .reg = 0x10 },
> -	{ .mbps =  100, .reg = 0x20 },
> -	{ .mbps =  110, .reg = 0x30 },
> -	{ .mbps =  120, .reg = 0x01 },
> -	{ .mbps =  130, .reg = 0x11 },
> -	{ .mbps =  140, .reg = 0x21 },
> -	{ .mbps =  150, .reg = 0x31 },
> -	{ .mbps =  160, .reg = 0x02 },
> -	{ .mbps =  170, .reg = 0x12 },
> -	{ .mbps =  180, .reg = 0x22 },
> -	{ .mbps =  190, .reg = 0x32 },
> -	{ .mbps =  205, .reg = 0x03 },
> -	{ .mbps =  220, .reg = 0x13 },
> -	{ .mbps =  235, .reg = 0x23 },
> -	{ .mbps =  250, .reg = 0x33 },
> -	{ .mbps =  275, .reg = 0x04 },
> -	{ .mbps =  300, .reg = 0x14 },
> -	{ .mbps =  325, .reg = 0x25 },
> -	{ .mbps =  350, .reg = 0x35 },
> -	{ .mbps =  400, .reg = 0x05 },
> -	{ .mbps =  450, .reg = 0x16 },
> -	{ .mbps =  500, .reg = 0x26 },
> -	{ .mbps =  550, .reg = 0x37 },
> -	{ .mbps =  600, .reg = 0x07 },
> -	{ .mbps =  650, .reg = 0x18 },
> -	{ .mbps =  700, .reg = 0x28 },
> -	{ .mbps =  750, .reg = 0x39 },
> -	{ .mbps =  800, .reg = 0x09 },
> -	{ .mbps =  850, .reg = 0x19 },
> -	{ .mbps =  900, .reg = 0x29 },
> -	{ .mbps =  950, .reg = 0x3a },
> -	{ .mbps = 1000, .reg = 0x0a },
> -	{ .mbps = 1050, .reg = 0x1a },
> -	{ .mbps = 1100, .reg = 0x2a },
> -	{ .mbps = 1150, .reg = 0x3b },
> -	{ .mbps = 1200, .reg = 0x0b },
> -	{ .mbps = 1250, .reg = 0x1b },
> -	{ .mbps = 1300, .reg = 0x2b },
> -	{ .mbps = 1350, .reg = 0x3c },
> -	{ .mbps = 1400, .reg = 0x0c },
> -	{ .mbps = 1450, .reg = 0x1c },
> -	{ .mbps = 1500, .reg = 0x2c },
> +	{ .mbps =   80, .reg = 0x00, .min =   80, .max =   97 },
> +	{ .mbps =   90, .reg = 0x10, .min =   80, .max =  107 },
> +	{ .mbps =  100, .reg = 0x20, .min =   83, .max =  118 },
> +	{ .mbps =  110, .reg = 0x30, .min =   93, .max =  128 },
> +	{ .mbps =  120, .reg = 0x01, .min =  103, .max =  139 },
> +	{ .mbps =  130, .reg = 0x11, .min =  112, .max =  149 },
> +	{ .mbps =  140, .reg = 0x21, .min =  122, .max =  160 },
> +	{ .mbps =  150, .reg = 0x31, .min =  131, .max =  170 },
> +	{ .mbps =  160, .reg = 0x02, .min =  141, .max =  181 },
> +	{ .mbps =  170, .reg = 0x12, .min =  150, .max =  191 },
> +	{ .mbps =  180, .reg = 0x22, .min =  160, .max =  202 },
> +	{ .mbps =  190, .reg = 0x32, .min =  169, .max =  212 },
> +	{ .mbps =  205, .reg = 0x03, .min =  183, .max =  228 },
> +	{ .mbps =  220, .reg = 0x13, .min =  198, .max =  224 },
> +	{ .mbps =  235, .reg = 0x23, .min =  212, .max =  259 },
> +	{ .mbps =  250, .reg = 0x33, .min =  226, .max =  275 },
> +	{ .mbps =  275, .reg = 0x04, .min =  250, .max =  301 },
> +	{ .mbps =  300, .reg = 0x14, .min =  274, .max =  328 },
> +	{ .mbps =  325, .reg = 0x25, .min =  297, .max =  354 },
> +	{ .mbps =  350, .reg = 0x35, .min =  321, .max =  380 },
> +	{ .mbps =  400, .reg = 0x05, .min =  369, .max =  433 },
> +	{ .mbps =  450, .reg = 0x16, .min =  416, .max =  485 },
> +	{ .mbps =  500, .reg = 0x26, .min =  464, .max =  538 },
> +	{ .mbps =  550, .reg = 0x37, .min =  499, .max =  590 },
> +	{ .mbps =  600, .reg = 0x07, .min =  559, .max =  643 },
> +	{ .mbps =  650, .reg = 0x18, .min =  606, .max =  695 },
> +	{ .mbps =  700, .reg = 0x28, .min =  654, .max =  748 },
> +	{ .mbps =  750, .reg = 0x39, .min =  701, .max =  800 },
> +	{ .mbps =  800, .reg = 0x09, .min =  749, .max =  853 },
> +	{ .mbps =  850, .reg = 0x19, .min =  796, .max =  905 },
> +	{ .mbps =  900, .reg = 0x29, .min =  844, .max =  958 },
> +	{ .mbps =  950, .reg = 0x3a, .min =  891, .max = 1010 },
> +	{ .mbps = 1000, .reg = 0x0a, .min =  939, .max = 1063 },
> +	{ .mbps = 1050, .reg = 0x1a, .min =  986, .max = 1115 },
> +	{ .mbps = 1100, .reg = 0x2a, .min = 1034, .max = 1168 },
> +	{ .mbps = 1150, .reg = 0x3b, .min = 1081, .max = 1220 },
> +	{ .mbps = 1200, .reg = 0x0b, .min = 1129, .max = 1273 },
> +	{ .mbps = 1250, .reg = 0x1b, .min = 1176, .max = 1325 },
> +	{ .mbps = 1300, .reg = 0x2b, .min = 1212, .max = 1378 },
> +	{ .mbps = 1350, .reg = 0x3c, .min = 1271, .max = 1430 },
> +	{ .mbps = 1400, .reg = 0x0c, .min = 1319, .max = 1483 },
> +	{ .mbps = 1450, .reg = 0x1c, .min = 1366, .max = 1500 },
> +	{ .mbps = 1500, .reg = 0x2c, .min = 1414, .max = 1500 },
>  	{ /* sentinel */ },
>  };
>  
>  static const struct rcsi2_mbps_reg hsfreqrange_m3w_h3es1[] = {
> -	{ .mbps =   80,	.reg = 0x00 },
> -	{ .mbps =   90,	.reg = 0x10 },
> -	{ .mbps =  100,	.reg = 0x20 },
> -	{ .mbps =  110,	.reg = 0x30 },
> -	{ .mbps =  120,	.reg = 0x01 },
> -	{ .mbps =  130,	.reg = 0x11 },
> -	{ .mbps =  140,	.reg = 0x21 },
> -	{ .mbps =  150,	.reg = 0x31 },
> -	{ .mbps =  160,	.reg = 0x02 },
> -	{ .mbps =  170,	.reg = 0x12 },
> -	{ .mbps =  180,	.reg = 0x22 },
> -	{ .mbps =  190,	.reg = 0x32 },
> -	{ .mbps =  205,	.reg = 0x03 },
> -	{ .mbps =  220,	.reg = 0x13 },
> -	{ .mbps =  235,	.reg = 0x23 },
> -	{ .mbps =  250,	.reg = 0x33 },
> -	{ .mbps =  275,	.reg = 0x04 },
> -	{ .mbps =  300,	.reg = 0x14 },
> -	{ .mbps =  325,	.reg = 0x05 },
> -	{ .mbps =  350,	.reg = 0x15 },
> -	{ .mbps =  400,	.reg = 0x25 },
> -	{ .mbps =  450,	.reg = 0x06 },
> -	{ .mbps =  500,	.reg = 0x16 },
> -	{ .mbps =  550,	.reg = 0x07 },
> -	{ .mbps =  600,	.reg = 0x17 },
> -	{ .mbps =  650,	.reg = 0x08 },
> -	{ .mbps =  700,	.reg = 0x18 },
> -	{ .mbps =  750,	.reg = 0x09 },
> -	{ .mbps =  800,	.reg = 0x19 },
> -	{ .mbps =  850,	.reg = 0x29 },
> -	{ .mbps =  900,	.reg = 0x39 },
> -	{ .mbps =  950,	.reg = 0x0a },
> -	{ .mbps = 1000,	.reg = 0x1a },
> -	{ .mbps = 1050,	.reg = 0x2a },
> -	{ .mbps = 1100,	.reg = 0x3a },
> -	{ .mbps = 1150,	.reg = 0x0b },
> -	{ .mbps = 1200,	.reg = 0x1b },
> -	{ .mbps = 1250,	.reg = 0x2b },
> -	{ .mbps = 1300,	.reg = 0x3b },
> -	{ .mbps = 1350,	.reg = 0x0c },
> -	{ .mbps = 1400,	.reg = 0x1c },
> -	{ .mbps = 1450,	.reg = 0x2c },
> -	{ .mbps = 1500,	.reg = 0x3c },
> +	{ .mbps =   80,	.reg = 0x00, .min =   80,  .max =   110 },
> +	{ .mbps =   90,	.reg = 0x10, .min =   80,  .max =   120 },
> +	{ .mbps =  100,	.reg = 0x20, .min =   80,  .max =   131 },
> +	{ .mbps =  110,	.reg = 0x30, .min =   81,  .max =   141 },
> +	{ .mbps =  120,	.reg = 0x01, .min =   91,  .max =   152 },
> +	{ .mbps =  130,	.reg = 0x11, .min =  100,  .max =   162 },
> +	{ .mbps =  140,	.reg = 0x21, .min =  110,  .max =   173 },
> +	{ .mbps =  150,	.reg = 0x31, .min =  119,  .max =   183 },
> +	{ .mbps =  160,	.reg = 0x02, .min =  129,  .max =   194 },
> +	{ .mbps =  170,	.reg = 0x12, .min =  138,  .max =   204 },
> +	{ .mbps =  180,	.reg = 0x22, .min =  148,  .max =   215 },
> +	{ .mbps =  190,	.reg = 0x32, .min =  157,  .max =   225 },
> +	{ .mbps =  205,	.reg = 0x03, .min =  171,  .max =   241 },
> +	{ .mbps =  220,	.reg = 0x13, .min =  186,  .max =   257 },
> +	{ .mbps =  235,	.reg = 0x23, .min =  200,  .max =   273 },
> +	{ .mbps =  250,	.reg = 0x33, .min =  238,  .max =   276 },
> +	{ .mbps =  275,	.reg = 0x04, .min =  250,  .max =   301 },
> +	{ .mbps =  300,	.reg = 0x14, .min =  274,  .max =   328 },
> +	{ .mbps =  325,	.reg = 0x05, .min =  297,  .max =   354 },
> +	{ .mbps =  350,	.reg = 0x15, .min =  321,  .max =   393 },
> +	{ .mbps =  400,	.reg = 0x25, .min =  357,  .max =   446 },
> +	{ .mbps =  450,	.reg = 0x06, .min =  404,  .max =   498 },
> +	{ .mbps =  500,	.reg = 0x16, .min =  452,  .max =   551 },
> +	{ .mbps =  550,	.reg = 0x07, .min =  499,  .max =   603 },
> +	{ .mbps =  600,	.reg = 0x17, .min =  547,  .max =   656 },
> +	{ .mbps =  650,	.reg = 0x08, .min =  594,  .max =   708 },
> +	{ .mbps =  700,	.reg = 0x18, .min =  642,  .max =   761 },
> +	{ .mbps =  750,	.reg = 0x09, .min =  689,  .max =   813 },
> +	{ .mbps =  800,	.reg = 0x19, .min =  737,  .max =   866 },
> +	{ .mbps =  850,	.reg = 0x29, .min =  784,  .max =   918 },
> +	{ .mbps =  900,	.reg = 0x39, .min =  832,  .max =   971 },
> +	{ .mbps =  950,	.reg = 0x0a, .min =  879,  .max =  1023 },
> +	{ .mbps = 1000,	.reg = 0x1a, .min =  927,  .max =  1076 },
> +	{ .mbps = 1050,	.reg = 0x2a, .min =  974,  .max =  1128 },
> +	{ .mbps = 1100,	.reg = 0x3a, .min = 1022,  .max =  1181 },
> +	{ .mbps = 1150,	.reg = 0x0b, .min = 1069,  .max =  1233 },
> +	{ .mbps = 1200,	.reg = 0x1b, .min = 1117,  .max =  1286 },
> +	{ .mbps = 1250,	.reg = 0x2b, .min = 1164,  .max =  1338 },
> +	{ .mbps = 1300,	.reg = 0x3b, .min = 1212,  .max =  1391 },
> +	{ .mbps = 1350,	.reg = 0x0c, .min = 1259,  .max =  1443 },
> +	{ .mbps = 1400,	.reg = 0x1c, .min = 1307,  .max =  1496 },
> +	{ .mbps = 1450,	.reg = 0x2c, .min = 1354,  .max =  1500 },
> +	{ .mbps = 1500,	.reg = 0x3c, .min = 1402,  .max =  1500 },
>  	{ /* sentinel */ },
>  };
>  
> @@ -430,11 +432,18 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
>  
>  static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
>  {
> -	const struct rcsi2_mbps_reg *hsfreq;
> +	const struct rcsi2_mbps_reg *hsfreq, *hsfreq_prev = NULL;
>  
> -	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
> +	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) {
>  		if (hsfreq->mbps >= mbps)
>  			break;
> +		hsfreq_prev = hsfreq;
> +	}
> +
> +	if (hsfreq_prev &&
> +	    ((mbps - hsfreq_prev->mbps) <= (hsfreq->mbps - mbps)) &&
> +	    (hsfreq_prev->max >= mbps && hsfreq_prev->min <= mbps))
> +		hsfreq = hsfreq_prev;

Would it be possible to rework this to get rid of the mbps variable from 
strcut rcsi2_mbps_reg? I'm thinking of making the logic selecting the 
first entry in the table where mbps argument falls between the min and 
max values introduced in this patch.

>  
>  	if (!hsfreq->mbps) {
>  		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH] [RFC] media: rcar-csi2: Corrects the selection of hsfreqrange

[Niklas Söderlund]

Hi Suresh,

Gentle ping on this. Do you plan to send a new version of this patch?

On 2020-03-04 20:46:02 +0100, Niklas Söderlund wrote:
> Hi Suresh,
> 
> Thanks for your work!
> 
> On 2020-02-20 13:49:09 +0900, Suresh Udipi wrote:
> > hsfreqrange should be chosen based on the calculated mbps which
> > is closer to the default bit rate  and within the range as per
> > table[1]. But current calculation always selects fist value which
> > greater than the calcuated mbps which may lead to chosing a
> > wrong range in some cases.
> > 
> > For example for 360 mbps for H3/M3N
> > Existing logic selects
> > Calculated value 360Mbps : Default 400Mbps Range [368.125 -433.125 mbps]
> > 
> > This hsfreqrange is buggy and also out of range.
> > 
> > The logic is changed to get the default value which is closest to the
> > calculated value [1]
> > 
> > Calculate value 360Mbps : Default 350Mbps  Range [320.625 -380.625 mpbs]
> > 
> >  [1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]
> 
> It's great that you are addressing this issue. Out of curiosity what 
> hardware setup and configuration have you used to trigger the issue (if 
> any)?
> 
> > 
> > Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
> > Signed-off-by: Kazuyoshi Akiyama <akiyama@nds-osk.co.jp>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 185 +++++++++++++++-------------
> >  1 file changed, 97 insertions(+), 88 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index faa9fb2..1966ece 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -134,6 +134,8 @@ struct phtw_value {
> >  struct rcsi2_mbps_reg {
> >  	u16 mbps;
> >  	u16 reg;
> > +	u16 min;
> > +	u16 max;
> >  };
> >  
> >  static const struct rcsi2_mbps_reg phtw_mbps_h3_v3h_m3n[] = {
> > @@ -201,96 +203,96 @@ static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
> >  #define PHYPLL_HSFREQRANGE(n)		((n) << 16)
> >  
> >  static const struct rcsi2_mbps_reg hsfreqrange_h3_v3h_m3n[] = {
> > -	{ .mbps =   80, .reg = 0x00 },
> > -	{ .mbps =   90, .reg = 0x10 },
> > -	{ .mbps =  100, .reg = 0x20 },
> > -	{ .mbps =  110, .reg = 0x30 },
> > -	{ .mbps =  120, .reg = 0x01 },
> > -	{ .mbps =  130, .reg = 0x11 },
> > -	{ .mbps =  140, .reg = 0x21 },
> > -	{ .mbps =  150, .reg = 0x31 },
> > -	{ .mbps =  160, .reg = 0x02 },
> > -	{ .mbps =  170, .reg = 0x12 },
> > -	{ .mbps =  180, .reg = 0x22 },
> > -	{ .mbps =  190, .reg = 0x32 },
> > -	{ .mbps =  205, .reg = 0x03 },
> > -	{ .mbps =  220, .reg = 0x13 },
> > -	{ .mbps =  235, .reg = 0x23 },
> > -	{ .mbps =  250, .reg = 0x33 },
> > -	{ .mbps =  275, .reg = 0x04 },
> > -	{ .mbps =  300, .reg = 0x14 },
> > -	{ .mbps =  325, .reg = 0x25 },
> > -	{ .mbps =  350, .reg = 0x35 },
> > -	{ .mbps =  400, .reg = 0x05 },
> > -	{ .mbps =  450, .reg = 0x16 },
> > -	{ .mbps =  500, .reg = 0x26 },
> > -	{ .mbps =  550, .reg = 0x37 },
> > -	{ .mbps =  600, .reg = 0x07 },
> > -	{ .mbps =  650, .reg = 0x18 },
> > -	{ .mbps =  700, .reg = 0x28 },
> > -	{ .mbps =  750, .reg = 0x39 },
> > -	{ .mbps =  800, .reg = 0x09 },
> > -	{ .mbps =  850, .reg = 0x19 },
> > -	{ .mbps =  900, .reg = 0x29 },
> > -	{ .mbps =  950, .reg = 0x3a },
> > -	{ .mbps = 1000, .reg = 0x0a },
> > -	{ .mbps = 1050, .reg = 0x1a },
> > -	{ .mbps = 1100, .reg = 0x2a },
> > -	{ .mbps = 1150, .reg = 0x3b },
> > -	{ .mbps = 1200, .reg = 0x0b },
> > -	{ .mbps = 1250, .reg = 0x1b },
> > -	{ .mbps = 1300, .reg = 0x2b },
> > -	{ .mbps = 1350, .reg = 0x3c },
> > -	{ .mbps = 1400, .reg = 0x0c },
> > -	{ .mbps = 1450, .reg = 0x1c },
> > -	{ .mbps = 1500, .reg = 0x2c },
> > +	{ .mbps =   80, .reg = 0x00, .min =   80, .max =   97 },
> > +	{ .mbps =   90, .reg = 0x10, .min =   80, .max =  107 },
> > +	{ .mbps =  100, .reg = 0x20, .min =   83, .max =  118 },
> > +	{ .mbps =  110, .reg = 0x30, .min =   93, .max =  128 },
> > +	{ .mbps =  120, .reg = 0x01, .min =  103, .max =  139 },
> > +	{ .mbps =  130, .reg = 0x11, .min =  112, .max =  149 },
> > +	{ .mbps =  140, .reg = 0x21, .min =  122, .max =  160 },
> > +	{ .mbps =  150, .reg = 0x31, .min =  131, .max =  170 },
> > +	{ .mbps =  160, .reg = 0x02, .min =  141, .max =  181 },
> > +	{ .mbps =  170, .reg = 0x12, .min =  150, .max =  191 },
> > +	{ .mbps =  180, .reg = 0x22, .min =  160, .max =  202 },
> > +	{ .mbps =  190, .reg = 0x32, .min =  169, .max =  212 },
> > +	{ .mbps =  205, .reg = 0x03, .min =  183, .max =  228 },
> > +	{ .mbps =  220, .reg = 0x13, .min =  198, .max =  224 },
> > +	{ .mbps =  235, .reg = 0x23, .min =  212, .max =  259 },
> > +	{ .mbps =  250, .reg = 0x33, .min =  226, .max =  275 },
> > +	{ .mbps =  275, .reg = 0x04, .min =  250, .max =  301 },
> > +	{ .mbps =  300, .reg = 0x14, .min =  274, .max =  328 },
> > +	{ .mbps =  325, .reg = 0x25, .min =  297, .max =  354 },
> > +	{ .mbps =  350, .reg = 0x35, .min =  321, .max =  380 },
> > +	{ .mbps =  400, .reg = 0x05, .min =  369, .max =  433 },
> > +	{ .mbps =  450, .reg = 0x16, .min =  416, .max =  485 },
> > +	{ .mbps =  500, .reg = 0x26, .min =  464, .max =  538 },
> > +	{ .mbps =  550, .reg = 0x37, .min =  499, .max =  590 },
> > +	{ .mbps =  600, .reg = 0x07, .min =  559, .max =  643 },
> > +	{ .mbps =  650, .reg = 0x18, .min =  606, .max =  695 },
> > +	{ .mbps =  700, .reg = 0x28, .min =  654, .max =  748 },
> > +	{ .mbps =  750, .reg = 0x39, .min =  701, .max =  800 },
> > +	{ .mbps =  800, .reg = 0x09, .min =  749, .max =  853 },
> > +	{ .mbps =  850, .reg = 0x19, .min =  796, .max =  905 },
> > +	{ .mbps =  900, .reg = 0x29, .min =  844, .max =  958 },
> > +	{ .mbps =  950, .reg = 0x3a, .min =  891, .max = 1010 },
> > +	{ .mbps = 1000, .reg = 0x0a, .min =  939, .max = 1063 },
> > +	{ .mbps = 1050, .reg = 0x1a, .min =  986, .max = 1115 },
> > +	{ .mbps = 1100, .reg = 0x2a, .min = 1034, .max = 1168 },
> > +	{ .mbps = 1150, .reg = 0x3b, .min = 1081, .max = 1220 },
> > +	{ .mbps = 1200, .reg = 0x0b, .min = 1129, .max = 1273 },
> > +	{ .mbps = 1250, .reg = 0x1b, .min = 1176, .max = 1325 },
> > +	{ .mbps = 1300, .reg = 0x2b, .min = 1212, .max = 1378 },
> > +	{ .mbps = 1350, .reg = 0x3c, .min = 1271, .max = 1430 },
> > +	{ .mbps = 1400, .reg = 0x0c, .min = 1319, .max = 1483 },
> > +	{ .mbps = 1450, .reg = 0x1c, .min = 1366, .max = 1500 },
> > +	{ .mbps = 1500, .reg = 0x2c, .min = 1414, .max = 1500 },
> >  	{ /* sentinel */ },
> >  };
> >  
> >  static const struct rcsi2_mbps_reg hsfreqrange_m3w_h3es1[] = {
> > -	{ .mbps =   80,	.reg = 0x00 },
> > -	{ .mbps =   90,	.reg = 0x10 },
> > -	{ .mbps =  100,	.reg = 0x20 },
> > -	{ .mbps =  110,	.reg = 0x30 },
> > -	{ .mbps =  120,	.reg = 0x01 },
> > -	{ .mbps =  130,	.reg = 0x11 },
> > -	{ .mbps =  140,	.reg = 0x21 },
> > -	{ .mbps =  150,	.reg = 0x31 },
> > -	{ .mbps =  160,	.reg = 0x02 },
> > -	{ .mbps =  170,	.reg = 0x12 },
> > -	{ .mbps =  180,	.reg = 0x22 },
> > -	{ .mbps =  190,	.reg = 0x32 },
> > -	{ .mbps =  205,	.reg = 0x03 },
> > -	{ .mbps =  220,	.reg = 0x13 },
> > -	{ .mbps =  235,	.reg = 0x23 },
> > -	{ .mbps =  250,	.reg = 0x33 },
> > -	{ .mbps =  275,	.reg = 0x04 },
> > -	{ .mbps =  300,	.reg = 0x14 },
> > -	{ .mbps =  325,	.reg = 0x05 },
> > -	{ .mbps =  350,	.reg = 0x15 },
> > -	{ .mbps =  400,	.reg = 0x25 },
> > -	{ .mbps =  450,	.reg = 0x06 },
> > -	{ .mbps =  500,	.reg = 0x16 },
> > -	{ .mbps =  550,	.reg = 0x07 },
> > -	{ .mbps =  600,	.reg = 0x17 },
> > -	{ .mbps =  650,	.reg = 0x08 },
> > -	{ .mbps =  700,	.reg = 0x18 },
> > -	{ .mbps =  750,	.reg = 0x09 },
> > -	{ .mbps =  800,	.reg = 0x19 },
> > -	{ .mbps =  850,	.reg = 0x29 },
> > -	{ .mbps =  900,	.reg = 0x39 },
> > -	{ .mbps =  950,	.reg = 0x0a },
> > -	{ .mbps = 1000,	.reg = 0x1a },
> > -	{ .mbps = 1050,	.reg = 0x2a },
> > -	{ .mbps = 1100,	.reg = 0x3a },
> > -	{ .mbps = 1150,	.reg = 0x0b },
> > -	{ .mbps = 1200,	.reg = 0x1b },
> > -	{ .mbps = 1250,	.reg = 0x2b },
> > -	{ .mbps = 1300,	.reg = 0x3b },
> > -	{ .mbps = 1350,	.reg = 0x0c },
> > -	{ .mbps = 1400,	.reg = 0x1c },
> > -	{ .mbps = 1450,	.reg = 0x2c },
> > -	{ .mbps = 1500,	.reg = 0x3c },
> > +	{ .mbps =   80,	.reg = 0x00, .min =   80,  .max =   110 },
> > +	{ .mbps =   90,	.reg = 0x10, .min =   80,  .max =   120 },
> > +	{ .mbps =  100,	.reg = 0x20, .min =   80,  .max =   131 },
> > +	{ .mbps =  110,	.reg = 0x30, .min =   81,  .max =   141 },
> > +	{ .mbps =  120,	.reg = 0x01, .min =   91,  .max =   152 },
> > +	{ .mbps =  130,	.reg = 0x11, .min =  100,  .max =   162 },
> > +	{ .mbps =  140,	.reg = 0x21, .min =  110,  .max =   173 },
> > +	{ .mbps =  150,	.reg = 0x31, .min =  119,  .max =   183 },
> > +	{ .mbps =  160,	.reg = 0x02, .min =  129,  .max =   194 },
> > +	{ .mbps =  170,	.reg = 0x12, .min =  138,  .max =   204 },
> > +	{ .mbps =  180,	.reg = 0x22, .min =  148,  .max =   215 },
> > +	{ .mbps =  190,	.reg = 0x32, .min =  157,  .max =   225 },
> > +	{ .mbps =  205,	.reg = 0x03, .min =  171,  .max =   241 },
> > +	{ .mbps =  220,	.reg = 0x13, .min =  186,  .max =   257 },
> > +	{ .mbps =  235,	.reg = 0x23, .min =  200,  .max =   273 },
> > +	{ .mbps =  250,	.reg = 0x33, .min =  238,  .max =   276 },
> > +	{ .mbps =  275,	.reg = 0x04, .min =  250,  .max =   301 },
> > +	{ .mbps =  300,	.reg = 0x14, .min =  274,  .max =   328 },
> > +	{ .mbps =  325,	.reg = 0x05, .min =  297,  .max =   354 },
> > +	{ .mbps =  350,	.reg = 0x15, .min =  321,  .max =   393 },
> > +	{ .mbps =  400,	.reg = 0x25, .min =  357,  .max =   446 },
> > +	{ .mbps =  450,	.reg = 0x06, .min =  404,  .max =   498 },
> > +	{ .mbps =  500,	.reg = 0x16, .min =  452,  .max =   551 },
> > +	{ .mbps =  550,	.reg = 0x07, .min =  499,  .max =   603 },
> > +	{ .mbps =  600,	.reg = 0x17, .min =  547,  .max =   656 },
> > +	{ .mbps =  650,	.reg = 0x08, .min =  594,  .max =   708 },
> > +	{ .mbps =  700,	.reg = 0x18, .min =  642,  .max =   761 },
> > +	{ .mbps =  750,	.reg = 0x09, .min =  689,  .max =   813 },
> > +	{ .mbps =  800,	.reg = 0x19, .min =  737,  .max =   866 },
> > +	{ .mbps =  850,	.reg = 0x29, .min =  784,  .max =   918 },
> > +	{ .mbps =  900,	.reg = 0x39, .min =  832,  .max =   971 },
> > +	{ .mbps =  950,	.reg = 0x0a, .min =  879,  .max =  1023 },
> > +	{ .mbps = 1000,	.reg = 0x1a, .min =  927,  .max =  1076 },
> > +	{ .mbps = 1050,	.reg = 0x2a, .min =  974,  .max =  1128 },
> > +	{ .mbps = 1100,	.reg = 0x3a, .min = 1022,  .max =  1181 },
> > +	{ .mbps = 1150,	.reg = 0x0b, .min = 1069,  .max =  1233 },
> > +	{ .mbps = 1200,	.reg = 0x1b, .min = 1117,  .max =  1286 },
> > +	{ .mbps = 1250,	.reg = 0x2b, .min = 1164,  .max =  1338 },
> > +	{ .mbps = 1300,	.reg = 0x3b, .min = 1212,  .max =  1391 },
> > +	{ .mbps = 1350,	.reg = 0x0c, .min = 1259,  .max =  1443 },
> > +	{ .mbps = 1400,	.reg = 0x1c, .min = 1307,  .max =  1496 },
> > +	{ .mbps = 1450,	.reg = 0x2c, .min = 1354,  .max =  1500 },
> > +	{ .mbps = 1500,	.reg = 0x3c, .min = 1402,  .max =  1500 },
> >  	{ /* sentinel */ },
> >  };
> >  
> > @@ -430,11 +432,18 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
> >  
> >  static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
> >  {
> > -	const struct rcsi2_mbps_reg *hsfreq;
> > +	const struct rcsi2_mbps_reg *hsfreq, *hsfreq_prev = NULL;
> >  
> > -	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
> > +	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) {
> >  		if (hsfreq->mbps >= mbps)
> >  			break;
> > +		hsfreq_prev = hsfreq;
> > +	}
> > +
> > +	if (hsfreq_prev &&
> > +	    ((mbps - hsfreq_prev->mbps) <= (hsfreq->mbps - mbps)) &&
> > +	    (hsfreq_prev->max >= mbps && hsfreq_prev->min <= mbps))
> > +		hsfreq = hsfreq_prev;
> 
> Would it be possible to rework this to get rid of the mbps variable from 
> strcut rcsi2_mbps_reg? I'm thinking of making the logic selecting the 
> first entry in the table where mbps argument falls between the min and 
> max values introduced in this patch.
> 
> >  
> >  	if (!hsfreq->mbps) {
> >  		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Regards,
> Niklas Söderlund

-- 
Regards,
Niklas Söderlund

[PATCH v2] [RFC] rcar-vin: rcar-csi2: Correct the selection of hsfreqrange

[Suresh Udipi]

hsfreqrange should be chosen based on the calculated mbps which
is closer to the default bit rate  and within the range as per
table[1]. But current calculation always selects first value which
is greater than or equal to the calculated mbps which may lead
to chosing a wrong range in some cases.

For example for 360 mbps for H3/M3N
Existing logic selects
Calculated value 360Mbps : Default 400Mbps Range [368.125 -433.125 mbps]

This hsfreqrange is out of range.

The logic is changed to get the default value which is closest to the
calculated value [1]

Calculated value 360Mbps : Default 350Mbps  Range [320.625 -380.625 mpbs]

[1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]

There is one exectpion value 227Mbps, which may cause out of
range.

Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")

Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
Signed-off-by: Kazuyoshi Akiyama <akiyama@nds-osk.co.jp>
---
Changes in v2:
  - Added the boundary check for the maximum bit rate.
  
  - Simplified the logic by remmoving range check 
    as only the closest default value covers most 
    of the use cases.

  - Aligning the commit message based on the above change

 drivers/media/platform/rcar-vin/rcar-csi2.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index faa9fb2..6573625 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -199,6 +199,7 @@ static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
 /* PHY Frequency Control */
 #define PHYPLL_REG			0x68
 #define PHYPLL_HSFREQRANGE(n)		((n) << 16)
+#define PHYPLL_HSFREQRANGE_MAX		1500
 
 static const struct rcsi2_mbps_reg hsfreqrange_h3_v3h_m3n[] = {
 	{ .mbps =   80, .reg = 0x00 },
@@ -431,16 +432,23 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
 static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
 {
 	const struct rcsi2_mbps_reg *hsfreq;
+	const struct rcsi2_mbps_reg *hsfreq_prev = NULL;
 
-	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
-		if (hsfreq->mbps >= mbps)
-			break;
-
-	if (!hsfreq->mbps) {
+	if (mbps > PHYPLL_HSFREQRANGE_MAX) {
 		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
 		return -ERANGE;
 	}
 
+	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) {
+		if (hsfreq->mbps >= mbps)
+			break;
+		hsfreq_prev = hsfreq;
+	}
+
+	if (hsfreq_prev &&
+	    ((mbps - hsfreq_prev->mbps) <= (hsfreq->mbps - mbps)))
+		hsfreq = hsfreq_prev;
+
 	rcsi2_write(priv, PHYPLL_REG, PHYPLL_HSFREQRANGE(hsfreq->reg));
 
 	return 0;
-- 
2.7.4

Re: [PATCH v2] [RFC] rcar-vin: rcar-csi2: Correct the selection of hsfreqrange

[Niklas Söderlund]

Hi Suresh,

Thanks for your work.

On 2020-03-17 16:08:25 +0900, Suresh Udipi wrote:
> hsfreqrange should be chosen based on the calculated mbps which
> is closer to the default bit rate  and within the range as per
> table[1]. But current calculation always selects first value which
> is greater than or equal to the calculated mbps which may lead
> to chosing a wrong range in some cases.
> 
> For example for 360 mbps for H3/M3N
> Existing logic selects
> Calculated value 360Mbps : Default 400Mbps Range [368.125 -433.125 mbps]
> 
> This hsfreqrange is out of range.
> 
> The logic is changed to get the default value which is closest to the
> calculated value [1]
> 
> Calculated value 360Mbps : Default 350Mbps  Range [320.625 -380.625 mpbs]
> 
> [1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]
> 
> There is one exectpion value 227Mbps, which may cause out of
> range.

Then something else is needed I think :-)

I liked v1 of this RFC more, where you added a u16 min and max to struct 
rcsi2_mbps_reg. I think that is the right solution.

What I tried to express in my review of v1 was

- You should remove the mbps member from struct rcsi2_mbps_reg.
- Update the walk of the array in rcsi2_set_phypll() so that it finds 
  the first entry where the calculated target value is between min and 
  max and use the reg setting for that entry.

Would that solution make sens too you? Sorry if I expressed myself a but 
muddy in v1 about this.

> 
> Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")
> 
> Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
> Signed-off-by: Kazuyoshi Akiyama <akiyama@nds-osk.co.jp>
> ---
> Changes in v2:
>   - Added the boundary check for the maximum bit rate.
>   
>   - Simplified the logic by remmoving range check 
>     as only the closest default value covers most 
>     of the use cases.
> 
>   - Aligning the commit message based on the above change
> 
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index faa9fb2..6573625 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -199,6 +199,7 @@ static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
>  /* PHY Frequency Control */
>  #define PHYPLL_REG			0x68
>  #define PHYPLL_HSFREQRANGE(n)		((n) << 16)
> +#define PHYPLL_HSFREQRANGE_MAX		1500
>  
>  static const struct rcsi2_mbps_reg hsfreqrange_h3_v3h_m3n[] = {
>  	{ .mbps =   80, .reg = 0x00 },
> @@ -431,16 +432,23 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
>  static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
>  {
>  	const struct rcsi2_mbps_reg *hsfreq;
> +	const struct rcsi2_mbps_reg *hsfreq_prev = NULL;
>  
> -	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
> -		if (hsfreq->mbps >= mbps)
> -			break;
> -
> -	if (!hsfreq->mbps) {
> +	if (mbps > PHYPLL_HSFREQRANGE_MAX) {
>  		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
>  		return -ERANGE;
>  	}
>  
> +	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) {
> +		if (hsfreq->mbps >= mbps)
> +			break;
> +		hsfreq_prev = hsfreq;
> +	}
> +
> +	if (hsfreq_prev &&
> +	    ((mbps - hsfreq_prev->mbps) <= (hsfreq->mbps - mbps)))
> +		hsfreq = hsfreq_prev;
> +
>  	rcsi2_write(priv, PHYPLL_REG, PHYPLL_HSFREQRANGE(hsfreq->reg));
>  
>  	return 0;
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v2] [RFC] rcar-vin: rcar-csi2: Correct the selection of hsfreqrange

[Suresh Udipi]

On Tue, Mar 17, 2020 at 11:37:56AM +0100, Niklas Söderlund wrote:
> Hi Suresh,
> 
> Thanks for your work.
> 
> On 2020-03-17 16:08:25 +0900, Suresh Udipi wrote:
> > hsfreqrange should be chosen based on the calculated mbps which
> > is closer to the default bit rate  and within the range as per
> > table[1]. But current calculation always selects first value which
> > is greater than or equal to the calculated mbps which may lead
> > to chosing a wrong range in some cases.
> > 
> > For example for 360 mbps for H3/M3N
> > Existing logic selects
> > Calculated value 360Mbps : Default 400Mbps Range [368.125 -433.125 mbps]
> > 
> > This hsfreqrange is out of range.
> > 
> > The logic is changed to get the default value which is closest to the
> > calculated value [1]
> > 
> > Calculated value 360Mbps : Default 350Mbps  Range [320.625 -380.625 mpbs]
> > 
> > [1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]
> > 
> > There is one exectpion value 227Mbps, which may cause out of
> > range.
> 
> Then something else is needed I think :-)
> 
> I liked v1 of this RFC more, where you added a u16 min and max to struct 
> rcsi2_mbps_reg. I think that is the right solution.
> 
> What I tried to express in my review of v1 was
> 
> - You should remove the mbps member from struct rcsi2_mbps_reg.
> - Update the walk of the array in rcsi2_set_phypll() so that it finds 
>   the first entry where the calculated target value is between min and 
>   max and use the reg setting for that entry.
> 
> Would that solution make sens too you? Sorry if I expressed myself a but 
> muddy in v1 about this.
 
Thank you for your feedback. Checking the range make more sense.

We can further optimize it, by checking only the Max range value.

- Remove mbps and min member from struct rcsi2_mbps_reg.
- Update the walk of the array in rcsi2_set_phypll() so that it finds
  the first entry where the calculated bit rate is less than the max.

Lower bit rates less than 80Mbps 
like 48Mbps(Raspberry pi camera 640x480 connected to Kingfisher)
can also be supported by selecting the lowest default bit rate 80Mbps
as done before this fix.

Please let me know your opinion on the same.
Meanwhile iam working on creating the patch, test it and update the same.
> > 
> > Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")
> > 
> > Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
> > Signed-off-by: Kazuyoshi Akiyama <akiyama@nds-osk.co.jp>
> > ---
> > Changes in v2:
> >   - Added the boundary check for the maximum bit rate.
> >   
> >   - Simplified the logic by remmoving range check 
> >     as only the closest default value covers most 
> >     of the use cases.
> > 
> >   - Aligning the commit message based on the above change
> > 
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index faa9fb2..6573625 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -199,6 +199,7 @@ static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
> >  /* PHY Frequency Control */
> >  #define PHYPLL_REG			0x68
> >  #define PHYPLL_HSFREQRANGE(n)		((n) << 16)
> > +#define PHYPLL_HSFREQRANGE_MAX		1500
> >  
> >  static const struct rcsi2_mbps_reg hsfreqrange_h3_v3h_m3n[] = {
> >  	{ .mbps =   80, .reg = 0x00 },
> > @@ -431,16 +432,23 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
> >  static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
> >  {
> >  	const struct rcsi2_mbps_reg *hsfreq;
> > +	const struct rcsi2_mbps_reg *hsfreq_prev = NULL;
> >  
> > -	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
> > -		if (hsfreq->mbps >= mbps)
> > -			break;
> > -
> > -	if (!hsfreq->mbps) {
> > +	if (mbps > PHYPLL_HSFREQRANGE_MAX) {
> >  		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
> >  		return -ERANGE;
> >  	}
> >  
> > +	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) {
> > +		if (hsfreq->mbps >= mbps)
> > +			break;
> > +		hsfreq_prev = hsfreq;
> > +	}
> > +
> > +	if (hsfreq_prev &&
> > +	    ((mbps - hsfreq_prev->mbps) <= (hsfreq->mbps - mbps)))
> > +		hsfreq = hsfreq_prev;
> > +
> >  	rcsi2_write(priv, PHYPLL_REG, PHYPLL_HSFREQRANGE(hsfreq->reg));
> >  
> >  	return 0;
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Regards,
> Niklas Söderlund

-- 
Best Regards,
Suresh Udipi.

Re: [PATCH v2] [RFC] rcar-vin: rcar-csi2: Correct the selection of hsfreqrange

[Niklas Söderlund]

Hi Suresh,

Thanks for your feedback.

On 2020-03-18 18:00:29 +0900, Suresh Udipi wrote:
> On Tue, Mar 17, 2020 at 11:37:56AM +0100, Niklas Söderlund wrote:
> > Hi Suresh,
> > 
> > Thanks for your work.
> > 
> > On 2020-03-17 16:08:25 +0900, Suresh Udipi wrote:
> > > hsfreqrange should be chosen based on the calculated mbps which
> > > is closer to the default bit rate  and within the range as per
> > > table[1]. But current calculation always selects first value which
> > > is greater than or equal to the calculated mbps which may lead
> > > to chosing a wrong range in some cases.
> > > 
> > > For example for 360 mbps for H3/M3N
> > > Existing logic selects
> > > Calculated value 360Mbps : Default 400Mbps Range [368.125 -433.125 mbps]
> > > 
> > > This hsfreqrange is out of range.
> > > 
> > > The logic is changed to get the default value which is closest to the
> > > calculated value [1]
> > > 
> > > Calculated value 360Mbps : Default 350Mbps  Range [320.625 -380.625 mpbs]
> > > 
> > > [1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]
> > > 
> > > There is one exectpion value 227Mbps, which may cause out of
> > > range.
> > 
> > Then something else is needed I think :-)
> > 
> > I liked v1 of this RFC more, where you added a u16 min and max to struct 
> > rcsi2_mbps_reg. I think that is the right solution.
> > 
> > What I tried to express in my review of v1 was
> > 
> > - You should remove the mbps member from struct rcsi2_mbps_reg.
> > - Update the walk of the array in rcsi2_set_phypll() so that it finds 
> >   the first entry where the calculated target value is between min and 
> >   max and use the reg setting for that entry.
> > 
> > Would that solution make sens too you? Sorry if I expressed myself a but 
> > muddy in v1 about this.
>  
> Thank you for your feedback. Checking the range make more sense.
> 
> We can further optimize it, by checking only the Max range value.
> 
> - Remove mbps and min member from struct rcsi2_mbps_reg.
> - Update the walk of the array in rcsi2_set_phypll() so that it finds
>   the first entry where the calculated bit rate is less than the max.
> 
> Lower bit rates less than 80Mbps 
> like 48Mbps(Raspberry pi camera 640x480 connected to Kingfisher)
> can also be supported by selecting the lowest default bit rate 80Mbps
> as done before this fix.

I think this approach sounds nice.

> 
> Please let me know your opinion on the same.
> Meanwhile iam working on creating the patch, test it and update the same.

Looking forward to it!

> > > 
> > > Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")
> > > 
> > > Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
> > > Signed-off-by: Kazuyoshi Akiyama <akiyama@nds-osk.co.jp>
> > > ---
> > > Changes in v2:
> > >   - Added the boundary check for the maximum bit rate.
> > >   
> > >   - Simplified the logic by remmoving range check 
> > >     as only the closest default value covers most 
> > >     of the use cases.
> > > 
> > >   - Aligning the commit message based on the above change
> > > 
> > >  drivers/media/platform/rcar-vin/rcar-csi2.c | 18 +++++++++++++-----
> > >  1 file changed, 13 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > index faa9fb2..6573625 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > @@ -199,6 +199,7 @@ static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
> > >  /* PHY Frequency Control */
> > >  #define PHYPLL_REG			0x68
> > >  #define PHYPLL_HSFREQRANGE(n)		((n) << 16)
> > > +#define PHYPLL_HSFREQRANGE_MAX		1500
> > >  
> > >  static const struct rcsi2_mbps_reg hsfreqrange_h3_v3h_m3n[] = {
> > >  	{ .mbps =   80, .reg = 0x00 },
> > > @@ -431,16 +432,23 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
> > >  static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
> > >  {
> > >  	const struct rcsi2_mbps_reg *hsfreq;
> > > +	const struct rcsi2_mbps_reg *hsfreq_prev = NULL;
> > >  
> > > -	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
> > > -		if (hsfreq->mbps >= mbps)
> > > -			break;
> > > -
> > > -	if (!hsfreq->mbps) {
> > > +	if (mbps > PHYPLL_HSFREQRANGE_MAX) {
> > >  		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
> > >  		return -ERANGE;
> > >  	}
> > >  
> > > +	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) {
> > > +		if (hsfreq->mbps >= mbps)
> > > +			break;
> > > +		hsfreq_prev = hsfreq;
> > > +	}
> > > +
> > > +	if (hsfreq_prev &&
> > > +	    ((mbps - hsfreq_prev->mbps) <= (hsfreq->mbps - mbps)))
> > > +		hsfreq = hsfreq_prev;
> > > +
> > >  	rcsi2_write(priv, PHYPLL_REG, PHYPLL_HSFREQRANGE(hsfreq->reg));
> > >  
> > >  	return 0;
> > > -- 
> > > 2.7.4
> > > 
> > 
> > -- 
> > Regards,
> > Niklas Söderlund
> 
> -- 
> Best Regards,
> Suresh Udipi.

-- 
Regards,
Niklas Söderlund

[PATCH v3] media: rcar-csi2: Correct the selection of hsfreqrange

[Suresh Udipi]

hsfreqrange should be chosen based on the calculated mbps which
is within the range as per table[1]. But current calculation
always selects first value which is greater than or equal to the
calculated mbps which may lead to chosing a wrong range in some cases.

For example for 360 mbps for H3/M3N
Existing logic selects
Calculated value 360Mbps : Default 400Mbps Range [368.125 -433.125 mbps]

This hsfreqrange is out of range.

The logic is changed to select the first hsfreqrange whose max range[1] is
greater than the calculated bit rate.

Calculated value 360Mbps : max range 380.625 mbps is selected
 i.e Default 350Mbps  Range [320.625 -380.625 mpbs]

[1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]

Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")

Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
Signed-off-by: Kazuyoshi Akiyama <akiyama@nds-osk.co.jp>
---
 Changes in v2:
  - Added the boundary check for the maximum bit rate.
  
  - Simplified the logic by remmoving range check 
    as only the closest default value covers most 
    of the use cases.

  - Aligning the commit message based on the above change


 Changes in v3:
    - Added max member from struct rcsi2_mbps_reg.
      mbps varialbe cannot be removed from rcsi2_mbps_reg, 
      since this structure is reused for 
      phtw_mbps_h3_v3h_m3n/phtw_mbps_v3m_e3 where mbps is 
      used.
	  
	
   -  Update the walk of the array in rcsi2_set_phypll() so that it finds
      the first entry where the calculated bit rate is less than the max.

   - Support lower bit rates less than 80Mbps like 48Mbps
     (Raspberry pi camera 640x480 connected to Kingfisher)
     can also be supported by selecting the lowest default bit rate 80Mbps
     as done before this fix

   - Alignement of the commit message based on above changes.

 drivers/media/platform/rcar-vin/rcar-csi2.c | 180 ++++++++++++++--------------
 1 file changed, 91 insertions(+), 89 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index faa9fb2..ff375b4 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -134,6 +134,7 @@ struct phtw_value {
 struct rcsi2_mbps_reg {
 	u16 mbps;
 	u16 reg;
+	u16 max;
 };
 
 static const struct rcsi2_mbps_reg phtw_mbps_h3_v3h_m3n[] = {
@@ -201,96 +202,96 @@ static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
 #define PHYPLL_HSFREQRANGE(n)		((n) << 16)
 
 static const struct rcsi2_mbps_reg hsfreqrange_h3_v3h_m3n[] = {
-	{ .mbps =   80, .reg = 0x00 },
-	{ .mbps =   90, .reg = 0x10 },
-	{ .mbps =  100, .reg = 0x20 },
-	{ .mbps =  110, .reg = 0x30 },
-	{ .mbps =  120, .reg = 0x01 },
-	{ .mbps =  130, .reg = 0x11 },
-	{ .mbps =  140, .reg = 0x21 },
-	{ .mbps =  150, .reg = 0x31 },
-	{ .mbps =  160, .reg = 0x02 },
-	{ .mbps =  170, .reg = 0x12 },
-	{ .mbps =  180, .reg = 0x22 },
-	{ .mbps =  190, .reg = 0x32 },
-	{ .mbps =  205, .reg = 0x03 },
-	{ .mbps =  220, .reg = 0x13 },
-	{ .mbps =  235, .reg = 0x23 },
-	{ .mbps =  250, .reg = 0x33 },
-	{ .mbps =  275, .reg = 0x04 },
-	{ .mbps =  300, .reg = 0x14 },
-	{ .mbps =  325, .reg = 0x25 },
-	{ .mbps =  350, .reg = 0x35 },
-	{ .mbps =  400, .reg = 0x05 },
-	{ .mbps =  450, .reg = 0x16 },
-	{ .mbps =  500, .reg = 0x26 },
-	{ .mbps =  550, .reg = 0x37 },
-	{ .mbps =  600, .reg = 0x07 },
-	{ .mbps =  650, .reg = 0x18 },
-	{ .mbps =  700, .reg = 0x28 },
-	{ .mbps =  750, .reg = 0x39 },
-	{ .mbps =  800, .reg = 0x09 },
-	{ .mbps =  850, .reg = 0x19 },
-	{ .mbps =  900, .reg = 0x29 },
-	{ .mbps =  950, .reg = 0x3a },
-	{ .mbps = 1000, .reg = 0x0a },
-	{ .mbps = 1050, .reg = 0x1a },
-	{ .mbps = 1100, .reg = 0x2a },
-	{ .mbps = 1150, .reg = 0x3b },
-	{ .mbps = 1200, .reg = 0x0b },
-	{ .mbps = 1250, .reg = 0x1b },
-	{ .mbps = 1300, .reg = 0x2b },
-	{ .mbps = 1350, .reg = 0x3c },
-	{ .mbps = 1400, .reg = 0x0c },
-	{ .mbps = 1450, .reg = 0x1c },
-	{ .mbps = 1500, .reg = 0x2c },
+	{ .reg = 0x00, .max =   97 },
+	{ .reg = 0x10, .max =  107 },
+	{ .reg = 0x20, .max =  118 },
+	{ .reg = 0x30, .max =  128 },
+	{ .reg = 0x01, .max =  139 },
+	{ .reg = 0x11, .max =  149 },
+	{ .reg = 0x21, .max =  160 },
+	{ .reg = 0x31, .max =  170 },
+	{ .reg = 0x02, .max =  181 },
+	{ .reg = 0x12, .max =  191 },
+	{ .reg = 0x22, .max =  202 },
+	{ .reg = 0x32, .max =  212 },
+	{ .reg = 0x03, .max =  228 },
+	{ .reg = 0x13, .max =  224 },
+	{ .reg = 0x23, .max =  259 },
+	{ .reg = 0x33, .max =  275 },
+	{ .reg = 0x04, .max =  301 },
+	{ .reg = 0x14, .max =  328 },
+	{ .reg = 0x25, .max =  354 },
+	{ .reg = 0x35, .max =  380 },
+	{ .reg = 0x05, .max =  433 },
+	{ .reg = 0x16, .max =  485 },
+	{ .reg = 0x26, .max =  538 },
+	{ .reg = 0x37, .max =  590 },
+	{ .reg = 0x07, .max =  643 },
+	{ .reg = 0x18, .max =  695 },
+	{ .reg = 0x28, .max =  748 },
+	{ .reg = 0x39, .max =  800 },
+	{ .reg = 0x09, .max =  853 },
+	{ .reg = 0x19, .max =  905 },
+	{ .reg = 0x29, .max =  958 },
+	{ .reg = 0x3a, .max = 1010 },
+	{ .reg = 0x0a, .max = 1063 },
+	{ .reg = 0x1a, .max = 1115 },
+	{ .reg = 0x2a, .max = 1168 },
+	{ .reg = 0x3b, .max = 1220 },
+	{ .reg = 0x0b, .max = 1273 },
+	{ .reg = 0x1b, .max = 1325 },
+	{ .reg = 0x2b, .max = 1378 },
+	{ .reg = 0x3c, .max = 1430 },
+	{ .reg = 0x0c, .max = 1483 },
+	{ .reg = 0x1c, .max = 1500 },
+	{ .reg = 0x2c, .max = 1500 },
 	{ /* sentinel */ },
 };
 
 static const struct rcsi2_mbps_reg hsfreqrange_m3w_h3es1[] = {
-	{ .mbps =   80,	.reg = 0x00 },
-	{ .mbps =   90,	.reg = 0x10 },
-	{ .mbps =  100,	.reg = 0x20 },
-	{ .mbps =  110,	.reg = 0x30 },
-	{ .mbps =  120,	.reg = 0x01 },
-	{ .mbps =  130,	.reg = 0x11 },
-	{ .mbps =  140,	.reg = 0x21 },
-	{ .mbps =  150,	.reg = 0x31 },
-	{ .mbps =  160,	.reg = 0x02 },
-	{ .mbps =  170,	.reg = 0x12 },
-	{ .mbps =  180,	.reg = 0x22 },
-	{ .mbps =  190,	.reg = 0x32 },
-	{ .mbps =  205,	.reg = 0x03 },
-	{ .mbps =  220,	.reg = 0x13 },
-	{ .mbps =  235,	.reg = 0x23 },
-	{ .mbps =  250,	.reg = 0x33 },
-	{ .mbps =  275,	.reg = 0x04 },
-	{ .mbps =  300,	.reg = 0x14 },
-	{ .mbps =  325,	.reg = 0x05 },
-	{ .mbps =  350,	.reg = 0x15 },
-	{ .mbps =  400,	.reg = 0x25 },
-	{ .mbps =  450,	.reg = 0x06 },
-	{ .mbps =  500,	.reg = 0x16 },
-	{ .mbps =  550,	.reg = 0x07 },
-	{ .mbps =  600,	.reg = 0x17 },
-	{ .mbps =  650,	.reg = 0x08 },
-	{ .mbps =  700,	.reg = 0x18 },
-	{ .mbps =  750,	.reg = 0x09 },
-	{ .mbps =  800,	.reg = 0x19 },
-	{ .mbps =  850,	.reg = 0x29 },
-	{ .mbps =  900,	.reg = 0x39 },
-	{ .mbps =  950,	.reg = 0x0a },
-	{ .mbps = 1000,	.reg = 0x1a },
-	{ .mbps = 1050,	.reg = 0x2a },
-	{ .mbps = 1100,	.reg = 0x3a },
-	{ .mbps = 1150,	.reg = 0x0b },
-	{ .mbps = 1200,	.reg = 0x1b },
-	{ .mbps = 1250,	.reg = 0x2b },
-	{ .mbps = 1300,	.reg = 0x3b },
-	{ .mbps = 1350,	.reg = 0x0c },
-	{ .mbps = 1400,	.reg = 0x1c },
-	{ .mbps = 1450,	.reg = 0x2c },
-	{ .mbps = 1500,	.reg = 0x3c },
+	{ .reg = 0x00, .max =  110 },
+	{ .reg = 0x10, .max =  120 },
+	{ .reg = 0x20, .max =  131 },
+	{ .reg = 0x30, .max =  141 },
+	{ .reg = 0x01, .max =  152 },
+	{ .reg = 0x11, .max =  162 },
+	{ .reg = 0x21, .max =  173 },
+	{ .reg = 0x31, .max =  183 },
+	{ .reg = 0x02, .max =  194 },
+	{ .reg = 0x12, .max =  204 },
+	{ .reg = 0x22, .max =  215 },
+	{ .reg = 0x32, .max =  225 },
+	{ .reg = 0x03, .max =  241 },
+	{ .reg = 0x13, .max =  257 },
+	{ .reg = 0x23, .max =  273 },
+	{ .reg = 0x33, .max =  275 },
+	{ .reg = 0x04, .max =  301 },
+	{ .reg = 0x14, .max =  328 },
+	{ .reg = 0x05, .max =  354 },
+	{ .reg = 0x15, .max =  393 },
+	{ .reg = 0x25, .max =  446 },
+	{ .reg = 0x06, .max =  498 },
+	{ .reg = 0x16, .max =  551 },
+	{ .reg = 0x07, .max =  603 },
+	{ .reg = 0x17, .max =  656 },
+	{ .reg = 0x08, .max =  708 },
+	{ .reg = 0x18, .max =  761 },
+	{ .reg = 0x09, .max =  813 },
+	{ .reg = 0x19, .max =  866 },
+	{ .reg = 0x29, .max =  918 },
+	{ .reg = 0x39, .max =  971 },
+	{ .reg = 0x0a, .max = 1023 },
+	{ .reg = 0x1a, .max = 1076 },
+	{ .reg = 0x2a, .max = 1128 },
+	{ .reg = 0x3a, .max = 1181 },
+	{ .reg = 0x0b, .max = 1233 },
+	{ .reg = 0x1b, .max = 1286 },
+	{ .reg = 0x2b, .max = 1338 },
+	{ .reg = 0x3b, .max = 1391 },
+	{ .reg = 0x0c, .max = 1443 },
+	{ .reg = 0x1c, .max = 1496 },
+	{ .reg = 0x2c, .max = 1500 },
+	{ .reg = 0x3c, .max = 1500 },
 	{ /* sentinel */ },
 };
 
@@ -432,11 +433,12 @@ static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
 {
 	const struct rcsi2_mbps_reg *hsfreq;
 
-	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
-		if (hsfreq->mbps >= mbps)
+	for (hsfreq = priv->info->hsfreqrange; hsfreq->max != 0; hsfreq++) {
+		if (hsfreq->max >= mbps)
 			break;
+	}
 
-	if (!hsfreq->mbps) {
+	if (!hsfreq->max) {
 		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
 		return -ERANGE;
 	}
-- 
2.7.4

[PATCH v4] media: rcar-csi2: Correct the selection of hsfreqrange

[Suresh Udipi]

hsfreqrange should be chosen based on the calculated mbps which
is within the range as per table[1]. But current calculation
always selects first value which is greater than or equal to the
calculated mbps which may lead to chosing a wrong range in some cases.

For example for 360 mbps for H3/M3N
Existing logic selects
Calculated value 360Mbps : Default 400Mbps Range [368.125 -433.125 mbps]

This hsfreqrange is out of range.

The logic is changed to select the first hsfreqrange whose max range[1] is
greater than the calculated bit rate.

Calculated value 360Mbps : max range 380.625 mbps is selected
 i.e Default 350Mbps  Range [320.625 -380.625 mpbs]

[1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]

Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")

Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
Signed-off-by: Kazuyoshi Akiyama <akiyama@nds-osk.co.jp>
---
 Changes in v2:
  - Added the boundary check for the maximum bit rate.

  - Simplified the logic by remmoving range check
    as only the closest default value covers most
    of the use cases.

  - Aligning the commit message based on the above change


 Changes in v3:
    - Added max member from struct rcsi2_mbps_reg.
      mbps varialbe cannot be removed from rcsi2_mbps_reg,
      since this structure is reused for
      phtw_mbps_h3_v3h_m3n/phtw_mbps_v3m_e3 where mbps is
      used.


   -  Update the walk of the array in rcsi2_set_phypll() so that it finds
      the first entry where the calculated bit rate is less than the max.

   - Support lower bit rates less than 80Mbps like 48Mbps
     (Raspberry pi camera 640x480 connected to Kingfisher)
     can also be supported by selecting the lowest default bit rate 80Mbps
     as done before this fix

   - Alignement of the commit message based on above changes.

 Changes in v4:
  -  Remove unncessary braces.

 drivers/media/platform/rcar-vin/rcar-csi2.c | 179 ++++++++++++++--------------
 1 file changed, 90 insertions(+), 89 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index faa9fb2..ca0321d 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -134,6 +134,7 @@ struct phtw_value {
 struct rcsi2_mbps_reg {
 	u16 mbps;
 	u16 reg;
+	u16 max;
 };
 
 static const struct rcsi2_mbps_reg phtw_mbps_h3_v3h_m3n[] = {
@@ -201,96 +202,96 @@ static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
 #define PHYPLL_HSFREQRANGE(n)		((n) << 16)
 
 static const struct rcsi2_mbps_reg hsfreqrange_h3_v3h_m3n[] = {
-	{ .mbps =   80, .reg = 0x00 },
-	{ .mbps =   90, .reg = 0x10 },
-	{ .mbps =  100, .reg = 0x20 },
-	{ .mbps =  110, .reg = 0x30 },
-	{ .mbps =  120, .reg = 0x01 },
-	{ .mbps =  130, .reg = 0x11 },
-	{ .mbps =  140, .reg = 0x21 },
-	{ .mbps =  150, .reg = 0x31 },
-	{ .mbps =  160, .reg = 0x02 },
-	{ .mbps =  170, .reg = 0x12 },
-	{ .mbps =  180, .reg = 0x22 },
-	{ .mbps =  190, .reg = 0x32 },
-	{ .mbps =  205, .reg = 0x03 },
-	{ .mbps =  220, .reg = 0x13 },
-	{ .mbps =  235, .reg = 0x23 },
-	{ .mbps =  250, .reg = 0x33 },
-	{ .mbps =  275, .reg = 0x04 },
-	{ .mbps =  300, .reg = 0x14 },
-	{ .mbps =  325, .reg = 0x25 },
-	{ .mbps =  350, .reg = 0x35 },
-	{ .mbps =  400, .reg = 0x05 },
-	{ .mbps =  450, .reg = 0x16 },
-	{ .mbps =  500, .reg = 0x26 },
-	{ .mbps =  550, .reg = 0x37 },
-	{ .mbps =  600, .reg = 0x07 },
-	{ .mbps =  650, .reg = 0x18 },
-	{ .mbps =  700, .reg = 0x28 },
-	{ .mbps =  750, .reg = 0x39 },
-	{ .mbps =  800, .reg = 0x09 },
-	{ .mbps =  850, .reg = 0x19 },
-	{ .mbps =  900, .reg = 0x29 },
-	{ .mbps =  950, .reg = 0x3a },
-	{ .mbps = 1000, .reg = 0x0a },
-	{ .mbps = 1050, .reg = 0x1a },
-	{ .mbps = 1100, .reg = 0x2a },
-	{ .mbps = 1150, .reg = 0x3b },
-	{ .mbps = 1200, .reg = 0x0b },
-	{ .mbps = 1250, .reg = 0x1b },
-	{ .mbps = 1300, .reg = 0x2b },
-	{ .mbps = 1350, .reg = 0x3c },
-	{ .mbps = 1400, .reg = 0x0c },
-	{ .mbps = 1450, .reg = 0x1c },
-	{ .mbps = 1500, .reg = 0x2c },
+	{ .reg = 0x00, .max =   97 },
+	{ .reg = 0x10, .max =  107 },
+	{ .reg = 0x20, .max =  118 },
+	{ .reg = 0x30, .max =  128 },
+	{ .reg = 0x01, .max =  139 },
+	{ .reg = 0x11, .max =  149 },
+	{ .reg = 0x21, .max =  160 },
+	{ .reg = 0x31, .max =  170 },
+	{ .reg = 0x02, .max =  181 },
+	{ .reg = 0x12, .max =  191 },
+	{ .reg = 0x22, .max =  202 },
+	{ .reg = 0x32, .max =  212 },
+	{ .reg = 0x03, .max =  228 },
+	{ .reg = 0x13, .max =  224 },
+	{ .reg = 0x23, .max =  259 },
+	{ .reg = 0x33, .max =  275 },
+	{ .reg = 0x04, .max =  301 },
+	{ .reg = 0x14, .max =  328 },
+	{ .reg = 0x25, .max =  354 },
+	{ .reg = 0x35, .max =  380 },
+	{ .reg = 0x05, .max =  433 },
+	{ .reg = 0x16, .max =  485 },
+	{ .reg = 0x26, .max =  538 },
+	{ .reg = 0x37, .max =  590 },
+	{ .reg = 0x07, .max =  643 },
+	{ .reg = 0x18, .max =  695 },
+	{ .reg = 0x28, .max =  748 },
+	{ .reg = 0x39, .max =  800 },
+	{ .reg = 0x09, .max =  853 },
+	{ .reg = 0x19, .max =  905 },
+	{ .reg = 0x29, .max =  958 },
+	{ .reg = 0x3a, .max = 1010 },
+	{ .reg = 0x0a, .max = 1063 },
+	{ .reg = 0x1a, .max = 1115 },
+	{ .reg = 0x2a, .max = 1168 },
+	{ .reg = 0x3b, .max = 1220 },
+	{ .reg = 0x0b, .max = 1273 },
+	{ .reg = 0x1b, .max = 1325 },
+	{ .reg = 0x2b, .max = 1378 },
+	{ .reg = 0x3c, .max = 1430 },
+	{ .reg = 0x0c, .max = 1483 },
+	{ .reg = 0x1c, .max = 1500 },
+	{ .reg = 0x2c, .max = 1500 },
 	{ /* sentinel */ },
 };
 
 static const struct rcsi2_mbps_reg hsfreqrange_m3w_h3es1[] = {
-	{ .mbps =   80,	.reg = 0x00 },
-	{ .mbps =   90,	.reg = 0x10 },
-	{ .mbps =  100,	.reg = 0x20 },
-	{ .mbps =  110,	.reg = 0x30 },
-	{ .mbps =  120,	.reg = 0x01 },
-	{ .mbps =  130,	.reg = 0x11 },
-	{ .mbps =  140,	.reg = 0x21 },
-	{ .mbps =  150,	.reg = 0x31 },
-	{ .mbps =  160,	.reg = 0x02 },
-	{ .mbps =  170,	.reg = 0x12 },
-	{ .mbps =  180,	.reg = 0x22 },
-	{ .mbps =  190,	.reg = 0x32 },
-	{ .mbps =  205,	.reg = 0x03 },
-	{ .mbps =  220,	.reg = 0x13 },
-	{ .mbps =  235,	.reg = 0x23 },
-	{ .mbps =  250,	.reg = 0x33 },
-	{ .mbps =  275,	.reg = 0x04 },
-	{ .mbps =  300,	.reg = 0x14 },
-	{ .mbps =  325,	.reg = 0x05 },
-	{ .mbps =  350,	.reg = 0x15 },
-	{ .mbps =  400,	.reg = 0x25 },
-	{ .mbps =  450,	.reg = 0x06 },
-	{ .mbps =  500,	.reg = 0x16 },
-	{ .mbps =  550,	.reg = 0x07 },
-	{ .mbps =  600,	.reg = 0x17 },
-	{ .mbps =  650,	.reg = 0x08 },
-	{ .mbps =  700,	.reg = 0x18 },
-	{ .mbps =  750,	.reg = 0x09 },
-	{ .mbps =  800,	.reg = 0x19 },
-	{ .mbps =  850,	.reg = 0x29 },
-	{ .mbps =  900,	.reg = 0x39 },
-	{ .mbps =  950,	.reg = 0x0a },
-	{ .mbps = 1000,	.reg = 0x1a },
-	{ .mbps = 1050,	.reg = 0x2a },
-	{ .mbps = 1100,	.reg = 0x3a },
-	{ .mbps = 1150,	.reg = 0x0b },
-	{ .mbps = 1200,	.reg = 0x1b },
-	{ .mbps = 1250,	.reg = 0x2b },
-	{ .mbps = 1300,	.reg = 0x3b },
-	{ .mbps = 1350,	.reg = 0x0c },
-	{ .mbps = 1400,	.reg = 0x1c },
-	{ .mbps = 1450,	.reg = 0x2c },
-	{ .mbps = 1500,	.reg = 0x3c },
+	{ .reg = 0x00, .max =  110 },
+	{ .reg = 0x10, .max =  120 },
+	{ .reg = 0x20, .max =  131 },
+	{ .reg = 0x30, .max =  141 },
+	{ .reg = 0x01, .max =  152 },
+	{ .reg = 0x11, .max =  162 },
+	{ .reg = 0x21, .max =  173 },
+	{ .reg = 0x31, .max =  183 },
+	{ .reg = 0x02, .max =  194 },
+	{ .reg = 0x12, .max =  204 },
+	{ .reg = 0x22, .max =  215 },
+	{ .reg = 0x32, .max =  225 },
+	{ .reg = 0x03, .max =  241 },
+	{ .reg = 0x13, .max =  257 },
+	{ .reg = 0x23, .max =  273 },
+	{ .reg = 0x33, .max =  275 },
+	{ .reg = 0x04, .max =  301 },
+	{ .reg = 0x14, .max =  328 },
+	{ .reg = 0x05, .max =  354 },
+	{ .reg = 0x15, .max =  393 },
+	{ .reg = 0x25, .max =  446 },
+	{ .reg = 0x06, .max =  498 },
+	{ .reg = 0x16, .max =  551 },
+	{ .reg = 0x07, .max =  603 },
+	{ .reg = 0x17, .max =  656 },
+	{ .reg = 0x08, .max =  708 },
+	{ .reg = 0x18, .max =  761 },
+	{ .reg = 0x09, .max =  813 },
+	{ .reg = 0x19, .max =  866 },
+	{ .reg = 0x29, .max =  918 },
+	{ .reg = 0x39, .max =  971 },
+	{ .reg = 0x0a, .max = 1023 },
+	{ .reg = 0x1a, .max = 1076 },
+	{ .reg = 0x2a, .max = 1128 },
+	{ .reg = 0x3a, .max = 1181 },
+	{ .reg = 0x0b, .max = 1233 },
+	{ .reg = 0x1b, .max = 1286 },
+	{ .reg = 0x2b, .max = 1338 },
+	{ .reg = 0x3b, .max = 1391 },
+	{ .reg = 0x0c, .max = 1443 },
+	{ .reg = 0x1c, .max = 1496 },
+	{ .reg = 0x2c, .max = 1500 },
+	{ .reg = 0x3c, .max = 1500 },
 	{ /* sentinel */ },
 };
 
@@ -432,11 +433,11 @@ static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
 {
 	const struct rcsi2_mbps_reg *hsfreq;
 
-	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
-		if (hsfreq->mbps >= mbps)
+	for (hsfreq = priv->info->hsfreqrange; hsfreq->max != 0; hsfreq++)
+		if (hsfreq->max >= mbps)
 			break;
 
-	if (!hsfreq->mbps) {
+	if (!hsfreq->max) {
 		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
 		return -ERANGE;
 	}
-- 
2.7.4

Re: [PATCH v4] media: rcar-csi2: Correct the selection of hsfreqrange

[Niklas Söderlund]

Hello Suresh,

Thanks for your patch.

On 2020-03-23 13:48:44 +0900, Suresh Udipi wrote:
> hsfreqrange should be chosen based on the calculated mbps which
> is within the range as per table[1]. But current calculation
> always selects first value which is greater than or equal to the
> calculated mbps which may lead to chosing a wrong range in some cases.
> 
> For example for 360 mbps for H3/M3N
> Existing logic selects
> Calculated value 360Mbps : Default 400Mbps Range [368.125 -433.125 mbps]
> 
> This hsfreqrange is out of range.
> 
> The logic is changed to select the first hsfreqrange whose max range[1] is
> greater than the calculated bit rate.
> 
> Calculated value 360Mbps : max range 380.625 mbps is selected
>  i.e Default 350Mbps  Range [320.625 -380.625 mpbs]
> 
> [1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]
> 
> Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")
> 
> Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
> Signed-off-by: Kazuyoshi Akiyama <akiyama@nds-osk.co.jp>
> ---
>  Changes in v2:
>   - Added the boundary check for the maximum bit rate.
> 
>   - Simplified the logic by remmoving range check
>     as only the closest default value covers most
>     of the use cases.
> 
>   - Aligning the commit message based on the above change
> 
> 
>  Changes in v3:
>     - Added max member from struct rcsi2_mbps_reg.
>       mbps varialbe cannot be removed from rcsi2_mbps_reg,
>       since this structure is reused for
>       phtw_mbps_h3_v3h_m3n/phtw_mbps_v3m_e3 where mbps is
>       used.
> 
> 
>    -  Update the walk of the array in rcsi2_set_phypll() so that it finds
>       the first entry where the calculated bit rate is less than the max.
> 
>    - Support lower bit rates less than 80Mbps like 48Mbps
>      (Raspberry pi camera 640x480 connected to Kingfisher)
>      can also be supported by selecting the lowest default bit rate 80Mbps
>      as done before this fix
> 
>    - Alignement of the commit message based on above changes.
> 
>  Changes in v4:
>   -  Remove unncessary braces.
> 
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 179 ++++++++++++++--------------
>  1 file changed, 90 insertions(+), 89 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index faa9fb2..ca0321d 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -134,6 +134,7 @@ struct phtw_value {
>  struct rcsi2_mbps_reg {
>  	u16 mbps;

You should remove the mbps field and then update the other tables with 
the max value. Apart from this I like this patch, nice work.

>  	u16 reg;
> +	u16 max;
>  };
>  
>  static const struct rcsi2_mbps_reg phtw_mbps_h3_v3h_m3n[] = {
> @@ -201,96 +202,96 @@ static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
>  #define PHYPLL_HSFREQRANGE(n)		((n) << 16)
>  
>  static const struct rcsi2_mbps_reg hsfreqrange_h3_v3h_m3n[] = {
> -	{ .mbps =   80, .reg = 0x00 },
> -	{ .mbps =   90, .reg = 0x10 },
> -	{ .mbps =  100, .reg = 0x20 },
> -	{ .mbps =  110, .reg = 0x30 },
> -	{ .mbps =  120, .reg = 0x01 },
> -	{ .mbps =  130, .reg = 0x11 },
> -	{ .mbps =  140, .reg = 0x21 },
> -	{ .mbps =  150, .reg = 0x31 },
> -	{ .mbps =  160, .reg = 0x02 },
> -	{ .mbps =  170, .reg = 0x12 },
> -	{ .mbps =  180, .reg = 0x22 },
> -	{ .mbps =  190, .reg = 0x32 },
> -	{ .mbps =  205, .reg = 0x03 },
> -	{ .mbps =  220, .reg = 0x13 },
> -	{ .mbps =  235, .reg = 0x23 },
> -	{ .mbps =  250, .reg = 0x33 },
> -	{ .mbps =  275, .reg = 0x04 },
> -	{ .mbps =  300, .reg = 0x14 },
> -	{ .mbps =  325, .reg = 0x25 },
> -	{ .mbps =  350, .reg = 0x35 },
> -	{ .mbps =  400, .reg = 0x05 },
> -	{ .mbps =  450, .reg = 0x16 },
> -	{ .mbps =  500, .reg = 0x26 },
> -	{ .mbps =  550, .reg = 0x37 },
> -	{ .mbps =  600, .reg = 0x07 },
> -	{ .mbps =  650, .reg = 0x18 },
> -	{ .mbps =  700, .reg = 0x28 },
> -	{ .mbps =  750, .reg = 0x39 },
> -	{ .mbps =  800, .reg = 0x09 },
> -	{ .mbps =  850, .reg = 0x19 },
> -	{ .mbps =  900, .reg = 0x29 },
> -	{ .mbps =  950, .reg = 0x3a },
> -	{ .mbps = 1000, .reg = 0x0a },
> -	{ .mbps = 1050, .reg = 0x1a },
> -	{ .mbps = 1100, .reg = 0x2a },
> -	{ .mbps = 1150, .reg = 0x3b },
> -	{ .mbps = 1200, .reg = 0x0b },
> -	{ .mbps = 1250, .reg = 0x1b },
> -	{ .mbps = 1300, .reg = 0x2b },
> -	{ .mbps = 1350, .reg = 0x3c },
> -	{ .mbps = 1400, .reg = 0x0c },
> -	{ .mbps = 1450, .reg = 0x1c },
> -	{ .mbps = 1500, .reg = 0x2c },
> +	{ .reg = 0x00, .max =   97 },
> +	{ .reg = 0x10, .max =  107 },
> +	{ .reg = 0x20, .max =  118 },
> +	{ .reg = 0x30, .max =  128 },
> +	{ .reg = 0x01, .max =  139 },
> +	{ .reg = 0x11, .max =  149 },
> +	{ .reg = 0x21, .max =  160 },
> +	{ .reg = 0x31, .max =  170 },
> +	{ .reg = 0x02, .max =  181 },
> +	{ .reg = 0x12, .max =  191 },
> +	{ .reg = 0x22, .max =  202 },
> +	{ .reg = 0x32, .max =  212 },
> +	{ .reg = 0x03, .max =  228 },
> +	{ .reg = 0x13, .max =  224 },
> +	{ .reg = 0x23, .max =  259 },
> +	{ .reg = 0x33, .max =  275 },
> +	{ .reg = 0x04, .max =  301 },
> +	{ .reg = 0x14, .max =  328 },
> +	{ .reg = 0x25, .max =  354 },
> +	{ .reg = 0x35, .max =  380 },
> +	{ .reg = 0x05, .max =  433 },
> +	{ .reg = 0x16, .max =  485 },
> +	{ .reg = 0x26, .max =  538 },
> +	{ .reg = 0x37, .max =  590 },
> +	{ .reg = 0x07, .max =  643 },
> +	{ .reg = 0x18, .max =  695 },
> +	{ .reg = 0x28, .max =  748 },
> +	{ .reg = 0x39, .max =  800 },
> +	{ .reg = 0x09, .max =  853 },
> +	{ .reg = 0x19, .max =  905 },
> +	{ .reg = 0x29, .max =  958 },
> +	{ .reg = 0x3a, .max = 1010 },
> +	{ .reg = 0x0a, .max = 1063 },
> +	{ .reg = 0x1a, .max = 1115 },
> +	{ .reg = 0x2a, .max = 1168 },
> +	{ .reg = 0x3b, .max = 1220 },
> +	{ .reg = 0x0b, .max = 1273 },
> +	{ .reg = 0x1b, .max = 1325 },
> +	{ .reg = 0x2b, .max = 1378 },
> +	{ .reg = 0x3c, .max = 1430 },
> +	{ .reg = 0x0c, .max = 1483 },
> +	{ .reg = 0x1c, .max = 1500 },
> +	{ .reg = 0x2c, .max = 1500 },
>  	{ /* sentinel */ },
>  };
>  
>  static const struct rcsi2_mbps_reg hsfreqrange_m3w_h3es1[] = {
> -	{ .mbps =   80,	.reg = 0x00 },
> -	{ .mbps =   90,	.reg = 0x10 },
> -	{ .mbps =  100,	.reg = 0x20 },
> -	{ .mbps =  110,	.reg = 0x30 },
> -	{ .mbps =  120,	.reg = 0x01 },
> -	{ .mbps =  130,	.reg = 0x11 },
> -	{ .mbps =  140,	.reg = 0x21 },
> -	{ .mbps =  150,	.reg = 0x31 },
> -	{ .mbps =  160,	.reg = 0x02 },
> -	{ .mbps =  170,	.reg = 0x12 },
> -	{ .mbps =  180,	.reg = 0x22 },
> -	{ .mbps =  190,	.reg = 0x32 },
> -	{ .mbps =  205,	.reg = 0x03 },
> -	{ .mbps =  220,	.reg = 0x13 },
> -	{ .mbps =  235,	.reg = 0x23 },
> -	{ .mbps =  250,	.reg = 0x33 },
> -	{ .mbps =  275,	.reg = 0x04 },
> -	{ .mbps =  300,	.reg = 0x14 },
> -	{ .mbps =  325,	.reg = 0x05 },
> -	{ .mbps =  350,	.reg = 0x15 },
> -	{ .mbps =  400,	.reg = 0x25 },
> -	{ .mbps =  450,	.reg = 0x06 },
> -	{ .mbps =  500,	.reg = 0x16 },
> -	{ .mbps =  550,	.reg = 0x07 },
> -	{ .mbps =  600,	.reg = 0x17 },
> -	{ .mbps =  650,	.reg = 0x08 },
> -	{ .mbps =  700,	.reg = 0x18 },
> -	{ .mbps =  750,	.reg = 0x09 },
> -	{ .mbps =  800,	.reg = 0x19 },
> -	{ .mbps =  850,	.reg = 0x29 },
> -	{ .mbps =  900,	.reg = 0x39 },
> -	{ .mbps =  950,	.reg = 0x0a },
> -	{ .mbps = 1000,	.reg = 0x1a },
> -	{ .mbps = 1050,	.reg = 0x2a },
> -	{ .mbps = 1100,	.reg = 0x3a },
> -	{ .mbps = 1150,	.reg = 0x0b },
> -	{ .mbps = 1200,	.reg = 0x1b },
> -	{ .mbps = 1250,	.reg = 0x2b },
> -	{ .mbps = 1300,	.reg = 0x3b },
> -	{ .mbps = 1350,	.reg = 0x0c },
> -	{ .mbps = 1400,	.reg = 0x1c },
> -	{ .mbps = 1450,	.reg = 0x2c },
> -	{ .mbps = 1500,	.reg = 0x3c },
> +	{ .reg = 0x00, .max =  110 },
> +	{ .reg = 0x10, .max =  120 },
> +	{ .reg = 0x20, .max =  131 },
> +	{ .reg = 0x30, .max =  141 },
> +	{ .reg = 0x01, .max =  152 },
> +	{ .reg = 0x11, .max =  162 },
> +	{ .reg = 0x21, .max =  173 },
> +	{ .reg = 0x31, .max =  183 },
> +	{ .reg = 0x02, .max =  194 },
> +	{ .reg = 0x12, .max =  204 },
> +	{ .reg = 0x22, .max =  215 },
> +	{ .reg = 0x32, .max =  225 },
> +	{ .reg = 0x03, .max =  241 },
> +	{ .reg = 0x13, .max =  257 },
> +	{ .reg = 0x23, .max =  273 },
> +	{ .reg = 0x33, .max =  275 },
> +	{ .reg = 0x04, .max =  301 },
> +	{ .reg = 0x14, .max =  328 },
> +	{ .reg = 0x05, .max =  354 },
> +	{ .reg = 0x15, .max =  393 },
> +	{ .reg = 0x25, .max =  446 },
> +	{ .reg = 0x06, .max =  498 },
> +	{ .reg = 0x16, .max =  551 },
> +	{ .reg = 0x07, .max =  603 },
> +	{ .reg = 0x17, .max =  656 },
> +	{ .reg = 0x08, .max =  708 },
> +	{ .reg = 0x18, .max =  761 },
> +	{ .reg = 0x09, .max =  813 },
> +	{ .reg = 0x19, .max =  866 },
> +	{ .reg = 0x29, .max =  918 },
> +	{ .reg = 0x39, .max =  971 },
> +	{ .reg = 0x0a, .max = 1023 },
> +	{ .reg = 0x1a, .max = 1076 },
> +	{ .reg = 0x2a, .max = 1128 },
> +	{ .reg = 0x3a, .max = 1181 },
> +	{ .reg = 0x0b, .max = 1233 },
> +	{ .reg = 0x1b, .max = 1286 },
> +	{ .reg = 0x2b, .max = 1338 },
> +	{ .reg = 0x3b, .max = 1391 },
> +	{ .reg = 0x0c, .max = 1443 },
> +	{ .reg = 0x1c, .max = 1496 },
> +	{ .reg = 0x2c, .max = 1500 },
> +	{ .reg = 0x3c, .max = 1500 },
>  	{ /* sentinel */ },
>  };
>  
> @@ -432,11 +433,11 @@ static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
>  {
>  	const struct rcsi2_mbps_reg *hsfreq;
>  
> -	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
> -		if (hsfreq->mbps >= mbps)
> +	for (hsfreq = priv->info->hsfreqrange; hsfreq->max != 0; hsfreq++)
> +		if (hsfreq->max >= mbps)
>  			break;
>  
> -	if (!hsfreq->mbps) {
> +	if (!hsfreq->max) {
>  		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
>  		return -ERANGE;
>  	}
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund

[PATCH v5] media: rcar-csi2: Correct the selection of hsfreqrange

[Suresh Udipi]

hsfreqrange should be chosen based on the calculated mbps which
is within the range as per table[1]. But current calculation
always selects first value which is greater than or equal to the
calculated mbps which may lead to chosing a wrong range in some cases.

For example for 360 mbps for H3/M3N
Existing logic selects
Calculated value 360Mbps : Default 400Mbps Range [368.125 -433.125 mbps]

This hsfreqrange is out of range.

The logic is changed to select the first hsfreqrange whose max range[1] is
greater than the calculated bit rate.

Calculated value 360Mbps : max range 380.625 mbps is selected
 i.e Default 350Mbps  Range [320.625 -380.625 mpbs]

[1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]

Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")

Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
Signed-off-by: Kazuyoshi Akiyama <akiyama@nds-osk.co.jp>
---
 Changes in v2:
  - Added the boundary check for the maximum bit rate.

  - Simplified the logic by remmoving range check
    as only the closest default value covers most
    of the use cases.

  - Aligning the commit message based on the above change


 Changes in v3:
    - Added max member from struct rcsi2_mbps_reg.
      mbps varialbe cannot be removed from rcsi2_mbps_reg,
      since this structure is reused for
      phtw_mbps_h3_v3h_m3n/phtw_mbps_v3m_e3 where mbps is
      used.


   -  Update the walk of the array in rcsi2_set_phypll() so that it finds
      the first entry where the calculated bit rate is less than the max.

   - Support lower bit rates less than 80Mbps like 48Mbps
     (Raspberry pi camera 640x480 connected to Kingfisher)
     can also be supported by selecting the lowest default bit rate 80Mbps
     as done before this fix

   - Alignement of the commit message based on above changes.

 Changes in v4:
   - Remove unncessary braces.
 
 Changes in v5:
   - Removed mbps variable in rcsi2_mbps_reg and aligned all 
     tables accordingly

 drivers/media/platform/rcar-vin/rcar-csi2.c | 282 ++++++++++++++--------------
 1 file changed, 141 insertions(+), 141 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index faa9fb2..d45bf80 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -132,63 +132,63 @@ struct phtw_value {
 };
 
 struct rcsi2_mbps_reg {
-	u16 mbps;
 	u16 reg;
+	u16 max;
 };
 
 static const struct rcsi2_mbps_reg phtw_mbps_h3_v3h_m3n[] = {
-	{ .mbps =   80, .reg = 0x86 },
-	{ .mbps =   90, .reg = 0x86 },
-	{ .mbps =  100, .reg = 0x87 },
-	{ .mbps =  110, .reg = 0x87 },
-	{ .mbps =  120, .reg = 0x88 },
-	{ .mbps =  130, .reg = 0x88 },
-	{ .mbps =  140, .reg = 0x89 },
-	{ .mbps =  150, .reg = 0x89 },
-	{ .mbps =  160, .reg = 0x8a },
-	{ .mbps =  170, .reg = 0x8a },
-	{ .mbps =  180, .reg = 0x8b },
-	{ .mbps =  190, .reg = 0x8b },
-	{ .mbps =  205, .reg = 0x8c },
-	{ .mbps =  220, .reg = 0x8d },
-	{ .mbps =  235, .reg = 0x8e },
-	{ .mbps =  250, .reg = 0x8e },
+	{ .reg = 0x86, .max =  80  },
+	{ .reg = 0x86, .max =  90  },
+	{ .reg = 0x87, .max =  100 },
+	{ .reg = 0x87, .max =  110 },
+	{ .reg = 0x88, .max =  120 },
+	{ .reg = 0x88, .max =  130 },
+	{ .reg = 0x89, .max =  140 },
+	{ .reg = 0x89, .max =  150 },
+	{ .reg = 0x8a, .max =  160 },
+	{ .reg = 0x8a, .max =  170 },
+	{ .reg = 0x8b, .max =  180 },
+	{ .reg = 0x8b, .max =  190 },
+	{ .reg = 0x8c, .max =  205 },
+	{ .reg = 0x8d, .max =  220 },
+	{ .reg = 0x8e, .max =  235 },
+	{ .reg = 0x8e, .max =  250 },
 	{ /* sentinel */ },
 };
 
 static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
-	{ .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 },
+	{ .reg = 0x00, .max = 80   },
+	{ .reg = 0x20, .max = 90   },
+	{ .reg = 0x40, .max = 100  },
+	{ .reg = 0x02, .max = 110  },
+	{ .reg = 0x22, .max = 130  },
+	{ .reg = 0x42, .max = 140  },
+	{ .reg = 0x04, .max = 150  },
+	{ .reg = 0x24, .max = 170  },
+	{ .reg = 0x44, .max = 180  },
+	{ .reg = 0x06, .max = 200  },
+	{ .reg = 0x26, .max = 220  },
+	{ .reg = 0x46, .max = 240  },
+	{ .reg = 0x08, .max = 250  },
+	{ .reg = 0x28, .max = 270  },
+	{ .reg = 0x0a, .max = 300  },
+	{ .reg = 0x2a, .max = 330  },
+	{ .reg = 0x4a, .max = 360  },
+	{ .reg = 0x0c, .max = 400  },
+	{ .reg = 0x2c, .max = 450  },
+	{ .reg = 0x0e, .max = 500  },
+	{ .reg = 0x2e, .max = 550  },
+	{ .reg = 0x10, .max = 600  },
+	{ .reg = 0x30, .max = 650  },
+	{ .reg = 0x12, .max = 700  },
+	{ .reg = 0x32, .max = 750  },
+	{ .reg = 0x52, .max = 800  },
+	{ .reg = 0x72, .max = 850  },
+	{ .reg = 0x14, .max = 900  },
+	{ .reg = 0x34, .max = 950  },
+	{ .reg = 0x54, .max = 1000 },
+	{ .reg = 0x74, .max = 1050 },
+	{ .reg = 0x16, .max = 1125 },
 	{ /* sentinel */ },
 };
 
@@ -201,96 +201,96 @@ static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
 #define PHYPLL_HSFREQRANGE(n)		((n) << 16)
 
 static const struct rcsi2_mbps_reg hsfreqrange_h3_v3h_m3n[] = {
-	{ .mbps =   80, .reg = 0x00 },
-	{ .mbps =   90, .reg = 0x10 },
-	{ .mbps =  100, .reg = 0x20 },
-	{ .mbps =  110, .reg = 0x30 },
-	{ .mbps =  120, .reg = 0x01 },
-	{ .mbps =  130, .reg = 0x11 },
-	{ .mbps =  140, .reg = 0x21 },
-	{ .mbps =  150, .reg = 0x31 },
-	{ .mbps =  160, .reg = 0x02 },
-	{ .mbps =  170, .reg = 0x12 },
-	{ .mbps =  180, .reg = 0x22 },
-	{ .mbps =  190, .reg = 0x32 },
-	{ .mbps =  205, .reg = 0x03 },
-	{ .mbps =  220, .reg = 0x13 },
-	{ .mbps =  235, .reg = 0x23 },
-	{ .mbps =  250, .reg = 0x33 },
-	{ .mbps =  275, .reg = 0x04 },
-	{ .mbps =  300, .reg = 0x14 },
-	{ .mbps =  325, .reg = 0x25 },
-	{ .mbps =  350, .reg = 0x35 },
-	{ .mbps =  400, .reg = 0x05 },
-	{ .mbps =  450, .reg = 0x16 },
-	{ .mbps =  500, .reg = 0x26 },
-	{ .mbps =  550, .reg = 0x37 },
-	{ .mbps =  600, .reg = 0x07 },
-	{ .mbps =  650, .reg = 0x18 },
-	{ .mbps =  700, .reg = 0x28 },
-	{ .mbps =  750, .reg = 0x39 },
-	{ .mbps =  800, .reg = 0x09 },
-	{ .mbps =  850, .reg = 0x19 },
-	{ .mbps =  900, .reg = 0x29 },
-	{ .mbps =  950, .reg = 0x3a },
-	{ .mbps = 1000, .reg = 0x0a },
-	{ .mbps = 1050, .reg = 0x1a },
-	{ .mbps = 1100, .reg = 0x2a },
-	{ .mbps = 1150, .reg = 0x3b },
-	{ .mbps = 1200, .reg = 0x0b },
-	{ .mbps = 1250, .reg = 0x1b },
-	{ .mbps = 1300, .reg = 0x2b },
-	{ .mbps = 1350, .reg = 0x3c },
-	{ .mbps = 1400, .reg = 0x0c },
-	{ .mbps = 1450, .reg = 0x1c },
-	{ .mbps = 1500, .reg = 0x2c },
+	{ .reg = 0x00, .max =   97 },
+	{ .reg = 0x10, .max =  107 },
+	{ .reg = 0x20, .max =  118 },
+	{ .reg = 0x30, .max =  128 },
+	{ .reg = 0x01, .max =  139 },
+	{ .reg = 0x11, .max =  149 },
+	{ .reg = 0x21, .max =  160 },
+	{ .reg = 0x31, .max =  170 },
+	{ .reg = 0x02, .max =  181 },
+	{ .reg = 0x12, .max =  191 },
+	{ .reg = 0x22, .max =  202 },
+	{ .reg = 0x32, .max =  212 },
+	{ .reg = 0x03, .max =  228 },
+	{ .reg = 0x13, .max =  224 },
+	{ .reg = 0x23, .max =  259 },
+	{ .reg = 0x33, .max =  275 },
+	{ .reg = 0x04, .max =  301 },
+	{ .reg = 0x14, .max =  328 },
+	{ .reg = 0x25, .max =  354 },
+	{ .reg = 0x35, .max =  380 },
+	{ .reg = 0x05, .max =  433 },
+	{ .reg = 0x16, .max =  485 },
+	{ .reg = 0x26, .max =  538 },
+	{ .reg = 0x37, .max =  590 },
+	{ .reg = 0x07, .max =  643 },
+	{ .reg = 0x18, .max =  695 },
+	{ .reg = 0x28, .max =  748 },
+	{ .reg = 0x39, .max =  800 },
+	{ .reg = 0x09, .max =  853 },
+	{ .reg = 0x19, .max =  905 },
+	{ .reg = 0x29, .max =  958 },
+	{ .reg = 0x3a, .max = 1010 },
+	{ .reg = 0x0a, .max = 1063 },
+	{ .reg = 0x1a, .max = 1115 },
+	{ .reg = 0x2a, .max = 1168 },
+	{ .reg = 0x3b, .max = 1220 },
+	{ .reg = 0x0b, .max = 1273 },
+	{ .reg = 0x1b, .max = 1325 },
+	{ .reg = 0x2b, .max = 1378 },
+	{ .reg = 0x3c, .max = 1430 },
+	{ .reg = 0x0c, .max = 1483 },
+	{ .reg = 0x1c, .max = 1500 },
+	{ .reg = 0x2c, .max = 1500 },
 	{ /* sentinel */ },
 };
 
 static const struct rcsi2_mbps_reg hsfreqrange_m3w_h3es1[] = {
-	{ .mbps =   80,	.reg = 0x00 },
-	{ .mbps =   90,	.reg = 0x10 },
-	{ .mbps =  100,	.reg = 0x20 },
-	{ .mbps =  110,	.reg = 0x30 },
-	{ .mbps =  120,	.reg = 0x01 },
-	{ .mbps =  130,	.reg = 0x11 },
-	{ .mbps =  140,	.reg = 0x21 },
-	{ .mbps =  150,	.reg = 0x31 },
-	{ .mbps =  160,	.reg = 0x02 },
-	{ .mbps =  170,	.reg = 0x12 },
-	{ .mbps =  180,	.reg = 0x22 },
-	{ .mbps =  190,	.reg = 0x32 },
-	{ .mbps =  205,	.reg = 0x03 },
-	{ .mbps =  220,	.reg = 0x13 },
-	{ .mbps =  235,	.reg = 0x23 },
-	{ .mbps =  250,	.reg = 0x33 },
-	{ .mbps =  275,	.reg = 0x04 },
-	{ .mbps =  300,	.reg = 0x14 },
-	{ .mbps =  325,	.reg = 0x05 },
-	{ .mbps =  350,	.reg = 0x15 },
-	{ .mbps =  400,	.reg = 0x25 },
-	{ .mbps =  450,	.reg = 0x06 },
-	{ .mbps =  500,	.reg = 0x16 },
-	{ .mbps =  550,	.reg = 0x07 },
-	{ .mbps =  600,	.reg = 0x17 },
-	{ .mbps =  650,	.reg = 0x08 },
-	{ .mbps =  700,	.reg = 0x18 },
-	{ .mbps =  750,	.reg = 0x09 },
-	{ .mbps =  800,	.reg = 0x19 },
-	{ .mbps =  850,	.reg = 0x29 },
-	{ .mbps =  900,	.reg = 0x39 },
-	{ .mbps =  950,	.reg = 0x0a },
-	{ .mbps = 1000,	.reg = 0x1a },
-	{ .mbps = 1050,	.reg = 0x2a },
-	{ .mbps = 1100,	.reg = 0x3a },
-	{ .mbps = 1150,	.reg = 0x0b },
-	{ .mbps = 1200,	.reg = 0x1b },
-	{ .mbps = 1250,	.reg = 0x2b },
-	{ .mbps = 1300,	.reg = 0x3b },
-	{ .mbps = 1350,	.reg = 0x0c },
-	{ .mbps = 1400,	.reg = 0x1c },
-	{ .mbps = 1450,	.reg = 0x2c },
-	{ .mbps = 1500,	.reg = 0x3c },
+	{ .reg = 0x00, .max =  110 },
+	{ .reg = 0x10, .max =  120 },
+	{ .reg = 0x20, .max =  131 },
+	{ .reg = 0x30, .max =  141 },
+	{ .reg = 0x01, .max =  152 },
+	{ .reg = 0x11, .max =  162 },
+	{ .reg = 0x21, .max =  173 },
+	{ .reg = 0x31, .max =  183 },
+	{ .reg = 0x02, .max =  194 },
+	{ .reg = 0x12, .max =  204 },
+	{ .reg = 0x22, .max =  215 },
+	{ .reg = 0x32, .max =  225 },
+	{ .reg = 0x03, .max =  241 },
+	{ .reg = 0x13, .max =  257 },
+	{ .reg = 0x23, .max =  273 },
+	{ .reg = 0x33, .max =  275 },
+	{ .reg = 0x04, .max =  301 },
+	{ .reg = 0x14, .max =  328 },
+	{ .reg = 0x05, .max =  354 },
+	{ .reg = 0x15, .max =  393 },
+	{ .reg = 0x25, .max =  446 },
+	{ .reg = 0x06, .max =  498 },
+	{ .reg = 0x16, .max =  551 },
+	{ .reg = 0x07, .max =  603 },
+	{ .reg = 0x17, .max =  656 },
+	{ .reg = 0x08, .max =  708 },
+	{ .reg = 0x18, .max =  761 },
+	{ .reg = 0x09, .max =  813 },
+	{ .reg = 0x19, .max =  866 },
+	{ .reg = 0x29, .max =  918 },
+	{ .reg = 0x39, .max =  971 },
+	{ .reg = 0x0a, .max = 1023 },
+	{ .reg = 0x1a, .max = 1076 },
+	{ .reg = 0x2a, .max = 1128 },
+	{ .reg = 0x3a, .max = 1181 },
+	{ .reg = 0x0b, .max = 1233 },
+	{ .reg = 0x1b, .max = 1286 },
+	{ .reg = 0x2b, .max = 1338 },
+	{ .reg = 0x3b, .max = 1391 },
+	{ .reg = 0x0c, .max = 1443 },
+	{ .reg = 0x1c, .max = 1496 },
+	{ .reg = 0x2c, .max = 1500 },
+	{ .reg = 0x3c, .max = 1500 },
 	{ /* sentinel */ },
 };
 
@@ -432,11 +432,11 @@ static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
 {
 	const struct rcsi2_mbps_reg *hsfreq;
 
-	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
-		if (hsfreq->mbps >= mbps)
+	for (hsfreq = priv->info->hsfreqrange; hsfreq->max != 0; hsfreq++)
+		if (hsfreq->max >= mbps)
 			break;
 
-	if (!hsfreq->mbps) {
+	if (!hsfreq->max) {
 		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
 		return -ERANGE;
 	}
@@ -907,11 +907,11 @@ static int rcsi2_phtw_write_mbps(struct rcar_csi2 *priv, unsigned int mbps,
 {
 	const struct rcsi2_mbps_reg *value;
 
-	for (value = values; value->mbps; value++)
-		if (value->mbps >= mbps)
+	for (value = values; value->max; value++)
+		if (value->max >= mbps)
 			break;
 
-	if (!value->mbps) {
+	if (!value->max) {
 		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
 		return -ERANGE;
 	}
-- 
2.7.4

Re: [PATCH v5] media: rcar-csi2: Correct the selection of hsfreqrange

[Michael Rodin]

Hello Suresh, Niklas,

I think that we should not change the structs for PHTW register configuration,
because there are no "max" values in the table "PHTW Set Values" from the
Renesas Hardware Manual. The patch looks just wrong. A major issue is also
that the Hardware Manual does not provide any algorithm, how to select the
right table entry for the configuration of PHTW and PHYPLL registers, so we
can not say for sure that the patch is correct. It is also not clear, what
are the "default" bit rates? It looks like they are made exactly for usage
in driver code:
 - both PHTW and PHYPLL tables have "default" values
 - only PHYPLL (HSFREQRANGE) tables have min/max values and it requires
   additional rounding in order to hardcode them in a driver
But these are just my speculations. If they are correct, then the only
exception, where the patch v2 failed, could be a typo in the Hardware Manual.
Otherwise patch v2 looks much better to me. I think, we should wait for more
details from Renesas before fixing this issue. 

On Thu, Apr 30, 2020 at 03:03:10PM +0900, Suresh Udipi wrote:
> hsfreqrange should be chosen based on the calculated mbps which
> is within the range as per table[1]. But current calculation
> always selects first value which is greater than or equal to the
> calculated mbps which may lead to chosing a wrong range in some cases.
> 
> For example for 360 mbps for H3/M3N
> Existing logic selects
> Calculated value 360Mbps : Default 400Mbps Range [368.125 -433.125 mbps]
> 
> This hsfreqrange is out of range.
> 
> The logic is changed to select the first hsfreqrange whose max range[1] is
> greater than the calculated bit rate.
> 
> Calculated value 360Mbps : max range 380.625 mbps is selected
>  i.e Default 350Mbps  Range [320.625 -380.625 mpbs]
> 
> [1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]
> 
> Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")
> 
> Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
> Signed-off-by: Kazuyoshi Akiyama <akiyama@nds-osk.co.jp>
> ---
>  Changes in v2:
>   - Added the boundary check for the maximum bit rate.
> 
>   - Simplified the logic by remmoving range check
>     as only the closest default value covers most
>     of the use cases.
> 
>   - Aligning the commit message based on the above change
> 
> 
>  Changes in v3:
>     - Added max member from struct rcsi2_mbps_reg.
>       mbps varialbe cannot be removed from rcsi2_mbps_reg,
>       since this structure is reused for
>       phtw_mbps_h3_v3h_m3n/phtw_mbps_v3m_e3 where mbps is
>       used.
> 
> 
>    -  Update the walk of the array in rcsi2_set_phypll() so that it finds
>       the first entry where the calculated bit rate is less than the max.
> 
>    - Support lower bit rates less than 80Mbps like 48Mbps
>      (Raspberry pi camera 640x480 connected to Kingfisher)
>      can also be supported by selecting the lowest default bit rate 80Mbps
>      as done before this fix
> 
>    - Alignement of the commit message based on above changes.
> 
>  Changes in v4:
>    - Remove unncessary braces.
>  
>  Changes in v5:
>    - Removed mbps variable in rcsi2_mbps_reg and aligned all 
>      tables accordingly
> 
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 282 ++++++++++++++--------------
>  1 file changed, 141 insertions(+), 141 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index faa9fb2..d45bf80 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -132,63 +132,63 @@ struct phtw_value {
>  };
>  
>  struct rcsi2_mbps_reg {
> -	u16 mbps;
>  	u16 reg;
> +	u16 max;
>  };
>  
>  static const struct rcsi2_mbps_reg phtw_mbps_h3_v3h_m3n[] = {
> -	{ .mbps =   80, .reg = 0x86 },
> -	{ .mbps =   90, .reg = 0x86 },
> -	{ .mbps =  100, .reg = 0x87 },
> -	{ .mbps =  110, .reg = 0x87 },
> -	{ .mbps =  120, .reg = 0x88 },
> -	{ .mbps =  130, .reg = 0x88 },
> -	{ .mbps =  140, .reg = 0x89 },
> -	{ .mbps =  150, .reg = 0x89 },
> -	{ .mbps =  160, .reg = 0x8a },
> -	{ .mbps =  170, .reg = 0x8a },
> -	{ .mbps =  180, .reg = 0x8b },
> -	{ .mbps =  190, .reg = 0x8b },
> -	{ .mbps =  205, .reg = 0x8c },
> -	{ .mbps =  220, .reg = 0x8d },
> -	{ .mbps =  235, .reg = 0x8e },
> -	{ .mbps =  250, .reg = 0x8e },
> +	{ .reg = 0x86, .max =  80  },
> +	{ .reg = 0x86, .max =  90  },
> +	{ .reg = 0x87, .max =  100 },
> +	{ .reg = 0x87, .max =  110 },
> +	{ .reg = 0x88, .max =  120 },
> +	{ .reg = 0x88, .max =  130 },
> +	{ .reg = 0x89, .max =  140 },
> +	{ .reg = 0x89, .max =  150 },
> +	{ .reg = 0x8a, .max =  160 },
> +	{ .reg = 0x8a, .max =  170 },
> +	{ .reg = 0x8b, .max =  180 },
> +	{ .reg = 0x8b, .max =  190 },
> +	{ .reg = 0x8c, .max =  205 },
> +	{ .reg = 0x8d, .max =  220 },
> +	{ .reg = 0x8e, .max =  235 },
> +	{ .reg = 0x8e, .max =  250 },
>  	{ /* sentinel */ },
>  };
>  
>  static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
> -	{ .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 },
> +	{ .reg = 0x00, .max = 80   },
> +	{ .reg = 0x20, .max = 90   },
> +	{ .reg = 0x40, .max = 100  },
> +	{ .reg = 0x02, .max = 110  },
> +	{ .reg = 0x22, .max = 130  },
> +	{ .reg = 0x42, .max = 140  },
> +	{ .reg = 0x04, .max = 150  },
> +	{ .reg = 0x24, .max = 170  },
> +	{ .reg = 0x44, .max = 180  },
> +	{ .reg = 0x06, .max = 200  },
> +	{ .reg = 0x26, .max = 220  },
> +	{ .reg = 0x46, .max = 240  },
> +	{ .reg = 0x08, .max = 250  },
> +	{ .reg = 0x28, .max = 270  },
> +	{ .reg = 0x0a, .max = 300  },
> +	{ .reg = 0x2a, .max = 330  },
> +	{ .reg = 0x4a, .max = 360  },
> +	{ .reg = 0x0c, .max = 400  },
> +	{ .reg = 0x2c, .max = 450  },
> +	{ .reg = 0x0e, .max = 500  },
> +	{ .reg = 0x2e, .max = 550  },
> +	{ .reg = 0x10, .max = 600  },
> +	{ .reg = 0x30, .max = 650  },
> +	{ .reg = 0x12, .max = 700  },
> +	{ .reg = 0x32, .max = 750  },
> +	{ .reg = 0x52, .max = 800  },
> +	{ .reg = 0x72, .max = 850  },
> +	{ .reg = 0x14, .max = 900  },
> +	{ .reg = 0x34, .max = 950  },
> +	{ .reg = 0x54, .max = 1000 },
> +	{ .reg = 0x74, .max = 1050 },
> +	{ .reg = 0x16, .max = 1125 },
>  	{ /* sentinel */ },
>  };
>  
> @@ -201,96 +201,96 @@ static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
>  #define PHYPLL_HSFREQRANGE(n)		((n) << 16)
>  
>  static const struct rcsi2_mbps_reg hsfreqrange_h3_v3h_m3n[] = {
> -	{ .mbps =   80, .reg = 0x00 },
> -	{ .mbps =   90, .reg = 0x10 },
> -	{ .mbps =  100, .reg = 0x20 },
> -	{ .mbps =  110, .reg = 0x30 },
> -	{ .mbps =  120, .reg = 0x01 },
> -	{ .mbps =  130, .reg = 0x11 },
> -	{ .mbps =  140, .reg = 0x21 },
> -	{ .mbps =  150, .reg = 0x31 },
> -	{ .mbps =  160, .reg = 0x02 },
> -	{ .mbps =  170, .reg = 0x12 },
> -	{ .mbps =  180, .reg = 0x22 },
> -	{ .mbps =  190, .reg = 0x32 },
> -	{ .mbps =  205, .reg = 0x03 },
> -	{ .mbps =  220, .reg = 0x13 },
> -	{ .mbps =  235, .reg = 0x23 },
> -	{ .mbps =  250, .reg = 0x33 },
> -	{ .mbps =  275, .reg = 0x04 },
> -	{ .mbps =  300, .reg = 0x14 },
> -	{ .mbps =  325, .reg = 0x25 },
> -	{ .mbps =  350, .reg = 0x35 },
> -	{ .mbps =  400, .reg = 0x05 },
> -	{ .mbps =  450, .reg = 0x16 },
> -	{ .mbps =  500, .reg = 0x26 },
> -	{ .mbps =  550, .reg = 0x37 },
> -	{ .mbps =  600, .reg = 0x07 },
> -	{ .mbps =  650, .reg = 0x18 },
> -	{ .mbps =  700, .reg = 0x28 },
> -	{ .mbps =  750, .reg = 0x39 },
> -	{ .mbps =  800, .reg = 0x09 },
> -	{ .mbps =  850, .reg = 0x19 },
> -	{ .mbps =  900, .reg = 0x29 },
> -	{ .mbps =  950, .reg = 0x3a },
> -	{ .mbps = 1000, .reg = 0x0a },
> -	{ .mbps = 1050, .reg = 0x1a },
> -	{ .mbps = 1100, .reg = 0x2a },
> -	{ .mbps = 1150, .reg = 0x3b },
> -	{ .mbps = 1200, .reg = 0x0b },
> -	{ .mbps = 1250, .reg = 0x1b },
> -	{ .mbps = 1300, .reg = 0x2b },
> -	{ .mbps = 1350, .reg = 0x3c },
> -	{ .mbps = 1400, .reg = 0x0c },
> -	{ .mbps = 1450, .reg = 0x1c },
> -	{ .mbps = 1500, .reg = 0x2c },
> +	{ .reg = 0x00, .max =   97 },
> +	{ .reg = 0x10, .max =  107 },
> +	{ .reg = 0x20, .max =  118 },
> +	{ .reg = 0x30, .max =  128 },
> +	{ .reg = 0x01, .max =  139 },
> +	{ .reg = 0x11, .max =  149 },
> +	{ .reg = 0x21, .max =  160 },
> +	{ .reg = 0x31, .max =  170 },
> +	{ .reg = 0x02, .max =  181 },
> +	{ .reg = 0x12, .max =  191 },
> +	{ .reg = 0x22, .max =  202 },
> +	{ .reg = 0x32, .max =  212 },
> +	{ .reg = 0x03, .max =  228 },
> +	{ .reg = 0x13, .max =  224 },
> +	{ .reg = 0x23, .max =  259 },
> +	{ .reg = 0x33, .max =  275 },
> +	{ .reg = 0x04, .max =  301 },
> +	{ .reg = 0x14, .max =  328 },
> +	{ .reg = 0x25, .max =  354 },
> +	{ .reg = 0x35, .max =  380 },
> +	{ .reg = 0x05, .max =  433 },
> +	{ .reg = 0x16, .max =  485 },
> +	{ .reg = 0x26, .max =  538 },
> +	{ .reg = 0x37, .max =  590 },
> +	{ .reg = 0x07, .max =  643 },
> +	{ .reg = 0x18, .max =  695 },
> +	{ .reg = 0x28, .max =  748 },
> +	{ .reg = 0x39, .max =  800 },
> +	{ .reg = 0x09, .max =  853 },
> +	{ .reg = 0x19, .max =  905 },
> +	{ .reg = 0x29, .max =  958 },
> +	{ .reg = 0x3a, .max = 1010 },
> +	{ .reg = 0x0a, .max = 1063 },
> +	{ .reg = 0x1a, .max = 1115 },
> +	{ .reg = 0x2a, .max = 1168 },
> +	{ .reg = 0x3b, .max = 1220 },
> +	{ .reg = 0x0b, .max = 1273 },
> +	{ .reg = 0x1b, .max = 1325 },
> +	{ .reg = 0x2b, .max = 1378 },
> +	{ .reg = 0x3c, .max = 1430 },
> +	{ .reg = 0x0c, .max = 1483 },
> +	{ .reg = 0x1c, .max = 1500 },
> +	{ .reg = 0x2c, .max = 1500 },
>  	{ /* sentinel */ },
>  };
>  
>  static const struct rcsi2_mbps_reg hsfreqrange_m3w_h3es1[] = {
> -	{ .mbps =   80,	.reg = 0x00 },
> -	{ .mbps =   90,	.reg = 0x10 },
> -	{ .mbps =  100,	.reg = 0x20 },
> -	{ .mbps =  110,	.reg = 0x30 },
> -	{ .mbps =  120,	.reg = 0x01 },
> -	{ .mbps =  130,	.reg = 0x11 },
> -	{ .mbps =  140,	.reg = 0x21 },
> -	{ .mbps =  150,	.reg = 0x31 },
> -	{ .mbps =  160,	.reg = 0x02 },
> -	{ .mbps =  170,	.reg = 0x12 },
> -	{ .mbps =  180,	.reg = 0x22 },
> -	{ .mbps =  190,	.reg = 0x32 },
> -	{ .mbps =  205,	.reg = 0x03 },
> -	{ .mbps =  220,	.reg = 0x13 },
> -	{ .mbps =  235,	.reg = 0x23 },
> -	{ .mbps =  250,	.reg = 0x33 },
> -	{ .mbps =  275,	.reg = 0x04 },
> -	{ .mbps =  300,	.reg = 0x14 },
> -	{ .mbps =  325,	.reg = 0x05 },
> -	{ .mbps =  350,	.reg = 0x15 },
> -	{ .mbps =  400,	.reg = 0x25 },
> -	{ .mbps =  450,	.reg = 0x06 },
> -	{ .mbps =  500,	.reg = 0x16 },
> -	{ .mbps =  550,	.reg = 0x07 },
> -	{ .mbps =  600,	.reg = 0x17 },
> -	{ .mbps =  650,	.reg = 0x08 },
> -	{ .mbps =  700,	.reg = 0x18 },
> -	{ .mbps =  750,	.reg = 0x09 },
> -	{ .mbps =  800,	.reg = 0x19 },
> -	{ .mbps =  850,	.reg = 0x29 },
> -	{ .mbps =  900,	.reg = 0x39 },
> -	{ .mbps =  950,	.reg = 0x0a },
> -	{ .mbps = 1000,	.reg = 0x1a },
> -	{ .mbps = 1050,	.reg = 0x2a },
> -	{ .mbps = 1100,	.reg = 0x3a },
> -	{ .mbps = 1150,	.reg = 0x0b },
> -	{ .mbps = 1200,	.reg = 0x1b },
> -	{ .mbps = 1250,	.reg = 0x2b },
> -	{ .mbps = 1300,	.reg = 0x3b },
> -	{ .mbps = 1350,	.reg = 0x0c },
> -	{ .mbps = 1400,	.reg = 0x1c },
> -	{ .mbps = 1450,	.reg = 0x2c },
> -	{ .mbps = 1500,	.reg = 0x3c },
> +	{ .reg = 0x00, .max =  110 },
> +	{ .reg = 0x10, .max =  120 },
> +	{ .reg = 0x20, .max =  131 },
> +	{ .reg = 0x30, .max =  141 },
> +	{ .reg = 0x01, .max =  152 },
> +	{ .reg = 0x11, .max =  162 },
> +	{ .reg = 0x21, .max =  173 },
> +	{ .reg = 0x31, .max =  183 },
> +	{ .reg = 0x02, .max =  194 },
> +	{ .reg = 0x12, .max =  204 },
> +	{ .reg = 0x22, .max =  215 },
> +	{ .reg = 0x32, .max =  225 },
> +	{ .reg = 0x03, .max =  241 },
> +	{ .reg = 0x13, .max =  257 },
> +	{ .reg = 0x23, .max =  273 },
> +	{ .reg = 0x33, .max =  275 },
> +	{ .reg = 0x04, .max =  301 },
> +	{ .reg = 0x14, .max =  328 },
> +	{ .reg = 0x05, .max =  354 },
> +	{ .reg = 0x15, .max =  393 },
> +	{ .reg = 0x25, .max =  446 },
> +	{ .reg = 0x06, .max =  498 },
> +	{ .reg = 0x16, .max =  551 },
> +	{ .reg = 0x07, .max =  603 },
> +	{ .reg = 0x17, .max =  656 },
> +	{ .reg = 0x08, .max =  708 },
> +	{ .reg = 0x18, .max =  761 },
> +	{ .reg = 0x09, .max =  813 },
> +	{ .reg = 0x19, .max =  866 },
> +	{ .reg = 0x29, .max =  918 },
> +	{ .reg = 0x39, .max =  971 },
> +	{ .reg = 0x0a, .max = 1023 },
> +	{ .reg = 0x1a, .max = 1076 },
> +	{ .reg = 0x2a, .max = 1128 },
> +	{ .reg = 0x3a, .max = 1181 },
> +	{ .reg = 0x0b, .max = 1233 },
> +	{ .reg = 0x1b, .max = 1286 },
> +	{ .reg = 0x2b, .max = 1338 },
> +	{ .reg = 0x3b, .max = 1391 },
> +	{ .reg = 0x0c, .max = 1443 },
> +	{ .reg = 0x1c, .max = 1496 },
> +	{ .reg = 0x2c, .max = 1500 },
> +	{ .reg = 0x3c, .max = 1500 },
>  	{ /* sentinel */ },
>  };
>  
> @@ -432,11 +432,11 @@ static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
>  {
>  	const struct rcsi2_mbps_reg *hsfreq;
>  
> -	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
> -		if (hsfreq->mbps >= mbps)
> +	for (hsfreq = priv->info->hsfreqrange; hsfreq->max != 0; hsfreq++)
> +		if (hsfreq->max >= mbps)
>  			break;
>  
> -	if (!hsfreq->mbps) {
> +	if (!hsfreq->max) {
>  		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
>  		return -ERANGE;
>  	}
> @@ -907,11 +907,11 @@ static int rcsi2_phtw_write_mbps(struct rcar_csi2 *priv, unsigned int mbps,
>  {
>  	const struct rcsi2_mbps_reg *value;
>  
> -	for (value = values; value->mbps; value++)
> -		if (value->mbps >= mbps)
> +	for (value = values; value->max; value++)
> +		if (value->max >= mbps)
>  			break;
>  
> -	if (!value->mbps) {
> +	if (!value->max) {
>  		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
>  		return -ERANGE;
>  	}
> -- 
> 2.7.4
> 

-- 
Best Regards,
Michael

Re: [PATCH v5] media: rcar-csi2: Correct the selection of hsfreqrange

[Niklas Söderlund]

Hi Michael and Suresh,

Thanks for your comments Michael.

On 2020-05-04 17:01:22 +0200, Michael Rodin wrote:
> Hello Suresh, Niklas,
> 
> I think that we should not change the structs for PHTW register configuration,
> because there are no "max" values in the table "PHTW Set Values" from the
> Renesas Hardware Manual. The patch looks just wrong. A major issue is also
> that the Hardware Manual does not provide any algorithm, how to select the
> right table entry for the configuration of PHTW and PHYPLL registers, so we
> can not say for sure that the patch is correct. It is also not clear, what
> are the "default" bit rates? It looks like they are made exactly for usage
> in driver code:
>  - both PHTW and PHYPLL tables have "default" values
>  - only PHYPLL (HSFREQRANGE) tables have min/max values and it requires
>    additional rounding in order to hardcode them in a driver
> But these are just my speculations. If they are correct, then the only
> exception, where the patch v2 failed, could be a typo in the Hardware Manual.
> Otherwise patch v2 looks much better to me. I think, we should wait for more
> details from Renesas before fixing this issue. 

I see your point.

As a middle ground as Suresh is having a real problem and the logic in 
the driver is wrong and should be fixed. How about keeping the 
definition of the struct rcsi2_mbps_reg as it is but updating all the 
tables .mbps values from the default to the max value?

> 
> On Thu, Apr 30, 2020 at 03:03:10PM +0900, Suresh Udipi wrote:
> > hsfreqrange should be chosen based on the calculated mbps which
> > is within the range as per table[1]. But current calculation
> > always selects first value which is greater than or equal to the
> > calculated mbps which may lead to chosing a wrong range in some cases.
> > 
> > For example for 360 mbps for H3/M3N
> > Existing logic selects
> > Calculated value 360Mbps : Default 400Mbps Range [368.125 -433.125 mbps]
> > 
> > This hsfreqrange is out of range.
> > 
> > The logic is changed to select the first hsfreqrange whose max range[1] is
> > greater than the calculated bit rate.
> > 
> > Calculated value 360Mbps : max range 380.625 mbps is selected
> >  i.e Default 350Mbps  Range [320.625 -380.625 mpbs]
> > 
> > [1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]
> > 
> > Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")
> > 
> > Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
> > Signed-off-by: Kazuyoshi Akiyama <akiyama@nds-osk.co.jp>
> > ---
> >  Changes in v2:
> >   - Added the boundary check for the maximum bit rate.
> > 
> >   - Simplified the logic by remmoving range check
> >     as only the closest default value covers most
> >     of the use cases.
> > 
> >   - Aligning the commit message based on the above change
> > 
> > 
> >  Changes in v3:
> >     - Added max member from struct rcsi2_mbps_reg.
> >       mbps varialbe cannot be removed from rcsi2_mbps_reg,
> >       since this structure is reused for
> >       phtw_mbps_h3_v3h_m3n/phtw_mbps_v3m_e3 where mbps is
> >       used.
> > 
> > 
> >    -  Update the walk of the array in rcsi2_set_phypll() so that it finds
> >       the first entry where the calculated bit rate is less than the max.
> > 
> >    - Support lower bit rates less than 80Mbps like 48Mbps
> >      (Raspberry pi camera 640x480 connected to Kingfisher)
> >      can also be supported by selecting the lowest default bit rate 80Mbps
> >      as done before this fix
> > 
> >    - Alignement of the commit message based on above changes.
> > 
> >  Changes in v4:
> >    - Remove unncessary braces.
> >  
> >  Changes in v5:
> >    - Removed mbps variable in rcsi2_mbps_reg and aligned all 
> >      tables accordingly
> > 
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 282 ++++++++++++++--------------
> >  1 file changed, 141 insertions(+), 141 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index faa9fb2..d45bf80 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -132,63 +132,63 @@ struct phtw_value {
> >  };
> >  
> >  struct rcsi2_mbps_reg {
> > -	u16 mbps;
> >  	u16 reg;
> > +	u16 max;
> >  };
> >  
> >  static const struct rcsi2_mbps_reg phtw_mbps_h3_v3h_m3n[] = {
> > -	{ .mbps =   80, .reg = 0x86 },
> > -	{ .mbps =   90, .reg = 0x86 },
> > -	{ .mbps =  100, .reg = 0x87 },
> > -	{ .mbps =  110, .reg = 0x87 },
> > -	{ .mbps =  120, .reg = 0x88 },
> > -	{ .mbps =  130, .reg = 0x88 },
> > -	{ .mbps =  140, .reg = 0x89 },
> > -	{ .mbps =  150, .reg = 0x89 },
> > -	{ .mbps =  160, .reg = 0x8a },
> > -	{ .mbps =  170, .reg = 0x8a },
> > -	{ .mbps =  180, .reg = 0x8b },
> > -	{ .mbps =  190, .reg = 0x8b },
> > -	{ .mbps =  205, .reg = 0x8c },
> > -	{ .mbps =  220, .reg = 0x8d },
> > -	{ .mbps =  235, .reg = 0x8e },
> > -	{ .mbps =  250, .reg = 0x8e },
> > +	{ .reg = 0x86, .max =  80  },
> > +	{ .reg = 0x86, .max =  90  },
> > +	{ .reg = 0x87, .max =  100 },
> > +	{ .reg = 0x87, .max =  110 },
> > +	{ .reg = 0x88, .max =  120 },
> > +	{ .reg = 0x88, .max =  130 },
> > +	{ .reg = 0x89, .max =  140 },
> > +	{ .reg = 0x89, .max =  150 },
> > +	{ .reg = 0x8a, .max =  160 },
> > +	{ .reg = 0x8a, .max =  170 },
> > +	{ .reg = 0x8b, .max =  180 },
> > +	{ .reg = 0x8b, .max =  190 },
> > +	{ .reg = 0x8c, .max =  205 },
> > +	{ .reg = 0x8d, .max =  220 },
> > +	{ .reg = 0x8e, .max =  235 },
> > +	{ .reg = 0x8e, .max =  250 },
> >  	{ /* sentinel */ },
> >  };
> >  
> >  static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
> > -	{ .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 },
> > +	{ .reg = 0x00, .max = 80   },
> > +	{ .reg = 0x20, .max = 90   },
> > +	{ .reg = 0x40, .max = 100  },
> > +	{ .reg = 0x02, .max = 110  },
> > +	{ .reg = 0x22, .max = 130  },
> > +	{ .reg = 0x42, .max = 140  },
> > +	{ .reg = 0x04, .max = 150  },
> > +	{ .reg = 0x24, .max = 170  },
> > +	{ .reg = 0x44, .max = 180  },
> > +	{ .reg = 0x06, .max = 200  },
> > +	{ .reg = 0x26, .max = 220  },
> > +	{ .reg = 0x46, .max = 240  },
> > +	{ .reg = 0x08, .max = 250  },
> > +	{ .reg = 0x28, .max = 270  },
> > +	{ .reg = 0x0a, .max = 300  },
> > +	{ .reg = 0x2a, .max = 330  },
> > +	{ .reg = 0x4a, .max = 360  },
> > +	{ .reg = 0x0c, .max = 400  },
> > +	{ .reg = 0x2c, .max = 450  },
> > +	{ .reg = 0x0e, .max = 500  },
> > +	{ .reg = 0x2e, .max = 550  },
> > +	{ .reg = 0x10, .max = 600  },
> > +	{ .reg = 0x30, .max = 650  },
> > +	{ .reg = 0x12, .max = 700  },
> > +	{ .reg = 0x32, .max = 750  },
> > +	{ .reg = 0x52, .max = 800  },
> > +	{ .reg = 0x72, .max = 850  },
> > +	{ .reg = 0x14, .max = 900  },
> > +	{ .reg = 0x34, .max = 950  },
> > +	{ .reg = 0x54, .max = 1000 },
> > +	{ .reg = 0x74, .max = 1050 },
> > +	{ .reg = 0x16, .max = 1125 },
> >  	{ /* sentinel */ },
> >  };
> >  
> > @@ -201,96 +201,96 @@ static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
> >  #define PHYPLL_HSFREQRANGE(n)		((n) << 16)
> >  
> >  static const struct rcsi2_mbps_reg hsfreqrange_h3_v3h_m3n[] = {
> > -	{ .mbps =   80, .reg = 0x00 },
> > -	{ .mbps =   90, .reg = 0x10 },
> > -	{ .mbps =  100, .reg = 0x20 },
> > -	{ .mbps =  110, .reg = 0x30 },
> > -	{ .mbps =  120, .reg = 0x01 },
> > -	{ .mbps =  130, .reg = 0x11 },
> > -	{ .mbps =  140, .reg = 0x21 },
> > -	{ .mbps =  150, .reg = 0x31 },
> > -	{ .mbps =  160, .reg = 0x02 },
> > -	{ .mbps =  170, .reg = 0x12 },
> > -	{ .mbps =  180, .reg = 0x22 },
> > -	{ .mbps =  190, .reg = 0x32 },
> > -	{ .mbps =  205, .reg = 0x03 },
> > -	{ .mbps =  220, .reg = 0x13 },
> > -	{ .mbps =  235, .reg = 0x23 },
> > -	{ .mbps =  250, .reg = 0x33 },
> > -	{ .mbps =  275, .reg = 0x04 },
> > -	{ .mbps =  300, .reg = 0x14 },
> > -	{ .mbps =  325, .reg = 0x25 },
> > -	{ .mbps =  350, .reg = 0x35 },
> > -	{ .mbps =  400, .reg = 0x05 },
> > -	{ .mbps =  450, .reg = 0x16 },
> > -	{ .mbps =  500, .reg = 0x26 },
> > -	{ .mbps =  550, .reg = 0x37 },
> > -	{ .mbps =  600, .reg = 0x07 },
> > -	{ .mbps =  650, .reg = 0x18 },
> > -	{ .mbps =  700, .reg = 0x28 },
> > -	{ .mbps =  750, .reg = 0x39 },
> > -	{ .mbps =  800, .reg = 0x09 },
> > -	{ .mbps =  850, .reg = 0x19 },
> > -	{ .mbps =  900, .reg = 0x29 },
> > -	{ .mbps =  950, .reg = 0x3a },
> > -	{ .mbps = 1000, .reg = 0x0a },
> > -	{ .mbps = 1050, .reg = 0x1a },
> > -	{ .mbps = 1100, .reg = 0x2a },
> > -	{ .mbps = 1150, .reg = 0x3b },
> > -	{ .mbps = 1200, .reg = 0x0b },
> > -	{ .mbps = 1250, .reg = 0x1b },
> > -	{ .mbps = 1300, .reg = 0x2b },
> > -	{ .mbps = 1350, .reg = 0x3c },
> > -	{ .mbps = 1400, .reg = 0x0c },
> > -	{ .mbps = 1450, .reg = 0x1c },
> > -	{ .mbps = 1500, .reg = 0x2c },
> > +	{ .reg = 0x00, .max =   97 },
> > +	{ .reg = 0x10, .max =  107 },
> > +	{ .reg = 0x20, .max =  118 },
> > +	{ .reg = 0x30, .max =  128 },
> > +	{ .reg = 0x01, .max =  139 },
> > +	{ .reg = 0x11, .max =  149 },
> > +	{ .reg = 0x21, .max =  160 },
> > +	{ .reg = 0x31, .max =  170 },
> > +	{ .reg = 0x02, .max =  181 },
> > +	{ .reg = 0x12, .max =  191 },
> > +	{ .reg = 0x22, .max =  202 },
> > +	{ .reg = 0x32, .max =  212 },
> > +	{ .reg = 0x03, .max =  228 },
> > +	{ .reg = 0x13, .max =  224 },
> > +	{ .reg = 0x23, .max =  259 },
> > +	{ .reg = 0x33, .max =  275 },
> > +	{ .reg = 0x04, .max =  301 },
> > +	{ .reg = 0x14, .max =  328 },
> > +	{ .reg = 0x25, .max =  354 },
> > +	{ .reg = 0x35, .max =  380 },
> > +	{ .reg = 0x05, .max =  433 },
> > +	{ .reg = 0x16, .max =  485 },
> > +	{ .reg = 0x26, .max =  538 },
> > +	{ .reg = 0x37, .max =  590 },
> > +	{ .reg = 0x07, .max =  643 },
> > +	{ .reg = 0x18, .max =  695 },
> > +	{ .reg = 0x28, .max =  748 },
> > +	{ .reg = 0x39, .max =  800 },
> > +	{ .reg = 0x09, .max =  853 },
> > +	{ .reg = 0x19, .max =  905 },
> > +	{ .reg = 0x29, .max =  958 },
> > +	{ .reg = 0x3a, .max = 1010 },
> > +	{ .reg = 0x0a, .max = 1063 },
> > +	{ .reg = 0x1a, .max = 1115 },
> > +	{ .reg = 0x2a, .max = 1168 },
> > +	{ .reg = 0x3b, .max = 1220 },
> > +	{ .reg = 0x0b, .max = 1273 },
> > +	{ .reg = 0x1b, .max = 1325 },
> > +	{ .reg = 0x2b, .max = 1378 },
> > +	{ .reg = 0x3c, .max = 1430 },
> > +	{ .reg = 0x0c, .max = 1483 },
> > +	{ .reg = 0x1c, .max = 1500 },
> > +	{ .reg = 0x2c, .max = 1500 },
> >  	{ /* sentinel */ },
> >  };
> >  
> >  static const struct rcsi2_mbps_reg hsfreqrange_m3w_h3es1[] = {
> > -	{ .mbps =   80,	.reg = 0x00 },
> > -	{ .mbps =   90,	.reg = 0x10 },
> > -	{ .mbps =  100,	.reg = 0x20 },
> > -	{ .mbps =  110,	.reg = 0x30 },
> > -	{ .mbps =  120,	.reg = 0x01 },
> > -	{ .mbps =  130,	.reg = 0x11 },
> > -	{ .mbps =  140,	.reg = 0x21 },
> > -	{ .mbps =  150,	.reg = 0x31 },
> > -	{ .mbps =  160,	.reg = 0x02 },
> > -	{ .mbps =  170,	.reg = 0x12 },
> > -	{ .mbps =  180,	.reg = 0x22 },
> > -	{ .mbps =  190,	.reg = 0x32 },
> > -	{ .mbps =  205,	.reg = 0x03 },
> > -	{ .mbps =  220,	.reg = 0x13 },
> > -	{ .mbps =  235,	.reg = 0x23 },
> > -	{ .mbps =  250,	.reg = 0x33 },
> > -	{ .mbps =  275,	.reg = 0x04 },
> > -	{ .mbps =  300,	.reg = 0x14 },
> > -	{ .mbps =  325,	.reg = 0x05 },
> > -	{ .mbps =  350,	.reg = 0x15 },
> > -	{ .mbps =  400,	.reg = 0x25 },
> > -	{ .mbps =  450,	.reg = 0x06 },
> > -	{ .mbps =  500,	.reg = 0x16 },
> > -	{ .mbps =  550,	.reg = 0x07 },
> > -	{ .mbps =  600,	.reg = 0x17 },
> > -	{ .mbps =  650,	.reg = 0x08 },
> > -	{ .mbps =  700,	.reg = 0x18 },
> > -	{ .mbps =  750,	.reg = 0x09 },
> > -	{ .mbps =  800,	.reg = 0x19 },
> > -	{ .mbps =  850,	.reg = 0x29 },
> > -	{ .mbps =  900,	.reg = 0x39 },
> > -	{ .mbps =  950,	.reg = 0x0a },
> > -	{ .mbps = 1000,	.reg = 0x1a },
> > -	{ .mbps = 1050,	.reg = 0x2a },
> > -	{ .mbps = 1100,	.reg = 0x3a },
> > -	{ .mbps = 1150,	.reg = 0x0b },
> > -	{ .mbps = 1200,	.reg = 0x1b },
> > -	{ .mbps = 1250,	.reg = 0x2b },
> > -	{ .mbps = 1300,	.reg = 0x3b },
> > -	{ .mbps = 1350,	.reg = 0x0c },
> > -	{ .mbps = 1400,	.reg = 0x1c },
> > -	{ .mbps = 1450,	.reg = 0x2c },
> > -	{ .mbps = 1500,	.reg = 0x3c },
> > +	{ .reg = 0x00, .max =  110 },
> > +	{ .reg = 0x10, .max =  120 },
> > +	{ .reg = 0x20, .max =  131 },
> > +	{ .reg = 0x30, .max =  141 },
> > +	{ .reg = 0x01, .max =  152 },
> > +	{ .reg = 0x11, .max =  162 },
> > +	{ .reg = 0x21, .max =  173 },
> > +	{ .reg = 0x31, .max =  183 },
> > +	{ .reg = 0x02, .max =  194 },
> > +	{ .reg = 0x12, .max =  204 },
> > +	{ .reg = 0x22, .max =  215 },
> > +	{ .reg = 0x32, .max =  225 },
> > +	{ .reg = 0x03, .max =  241 },
> > +	{ .reg = 0x13, .max =  257 },
> > +	{ .reg = 0x23, .max =  273 },
> > +	{ .reg = 0x33, .max =  275 },
> > +	{ .reg = 0x04, .max =  301 },
> > +	{ .reg = 0x14, .max =  328 },
> > +	{ .reg = 0x05, .max =  354 },
> > +	{ .reg = 0x15, .max =  393 },
> > +	{ .reg = 0x25, .max =  446 },
> > +	{ .reg = 0x06, .max =  498 },
> > +	{ .reg = 0x16, .max =  551 },
> > +	{ .reg = 0x07, .max =  603 },
> > +	{ .reg = 0x17, .max =  656 },
> > +	{ .reg = 0x08, .max =  708 },
> > +	{ .reg = 0x18, .max =  761 },
> > +	{ .reg = 0x09, .max =  813 },
> > +	{ .reg = 0x19, .max =  866 },
> > +	{ .reg = 0x29, .max =  918 },
> > +	{ .reg = 0x39, .max =  971 },
> > +	{ .reg = 0x0a, .max = 1023 },
> > +	{ .reg = 0x1a, .max = 1076 },
> > +	{ .reg = 0x2a, .max = 1128 },
> > +	{ .reg = 0x3a, .max = 1181 },
> > +	{ .reg = 0x0b, .max = 1233 },
> > +	{ .reg = 0x1b, .max = 1286 },
> > +	{ .reg = 0x2b, .max = 1338 },
> > +	{ .reg = 0x3b, .max = 1391 },
> > +	{ .reg = 0x0c, .max = 1443 },
> > +	{ .reg = 0x1c, .max = 1496 },
> > +	{ .reg = 0x2c, .max = 1500 },
> > +	{ .reg = 0x3c, .max = 1500 },
> >  	{ /* sentinel */ },
> >  };
> >  
> > @@ -432,11 +432,11 @@ static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
> >  {
> >  	const struct rcsi2_mbps_reg *hsfreq;
> >  
> > -	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
> > -		if (hsfreq->mbps >= mbps)
> > +	for (hsfreq = priv->info->hsfreqrange; hsfreq->max != 0; hsfreq++)
> > +		if (hsfreq->max >= mbps)
> >  			break;
> >  
> > -	if (!hsfreq->mbps) {
> > +	if (!hsfreq->max) {
> >  		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
> >  		return -ERANGE;
> >  	}
> > @@ -907,11 +907,11 @@ static int rcsi2_phtw_write_mbps(struct rcar_csi2 *priv, unsigned int mbps,
> >  {
> >  	const struct rcsi2_mbps_reg *value;
> >  
> > -	for (value = values; value->mbps; value++)
> > -		if (value->mbps >= mbps)
> > +	for (value = values; value->max; value++)
> > +		if (value->max >= mbps)
> >  			break;
> >  
> > -	if (!value->mbps) {
> > +	if (!value->max) {
> >  		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
> >  		return -ERANGE;
> >  	}
> > -- 
> > 2.7.4
> > 
> 
> -- 
> Best Regards,
> Michael

-- 
Regards,
Niklas Söderlund

Re: [PATCH v5] media: rcar-csi2: Correct the selection of hsfreqrange

[Michael Rodin]

On Tue, May 12, 2020 at 11:30:45PM +0200, Niklas Söderlund wrote:
> Hi Michael and Suresh,
> 
> Thanks for your comments Michael.
> 
> On 2020-05-04 17:01:22 +0200, Michael Rodin wrote:
> > Hello Suresh, Niklas,
> > 
> > I think that we should not change the structs for PHTW register configuration,
> > because there are no "max" values in the table "PHTW Set Values" from the
> > Renesas Hardware Manual. The patch looks just wrong. A major issue is also
> > that the Hardware Manual does not provide any algorithm, how to select the
> > right table entry for the configuration of PHTW and PHYPLL registers, so we
> > can not say for sure that the patch is correct. It is also not clear, what
> > are the "default" bit rates? It looks like they are made exactly for usage
> > in driver code:
> >  - both PHTW and PHYPLL tables have "default" values
> >  - only PHYPLL (HSFREQRANGE) tables have min/max values and it requires
> >    additional rounding in order to hardcode them in a driver
> > But these are just my speculations. If they are correct, then the only
> > exception, where the patch v2 failed, could be a typo in the Hardware Manual.
> > Otherwise patch v2 looks much better to me. I think, we should wait for more
> > details from Renesas before fixing this issue. 
> 
> I see your point.
> 
> As a middle ground as Suresh is having a real problem and the logic in 
> the driver is wrong and should be fixed. How about keeping the 
> definition of the struct rcsi2_mbps_reg as it is but updating all the 
> tables .mbps values from the default to the max value?

Do you mean we should rename "max" back to "mbps" in this patch but keep
the maximum value in the tables? This does not sound much better to me. If
it is "max", it should be called "max" :). One of the reasons why I like
[PATCH v2] is because we are currently using something very similar, and
we don't have any issues yet. Of course maybe we dont have any use case
with values around 227 mbps where we would have the exception mentioned by
Suresh for the range [197.125 - 224.125]. I will send the patches to make
more clear what I am talking about.

> > 
> > On Thu, Apr 30, 2020 at 03:03:10PM +0900, Suresh Udipi wrote:
> > > hsfreqrange should be chosen based on the calculated mbps which
> > > is within the range as per table[1]. But current calculation
> > > always selects first value which is greater than or equal to the
> > > calculated mbps which may lead to chosing a wrong range in some cases.
> > > 
> > > For example for 360 mbps for H3/M3N
> > > Existing logic selects
> > > Calculated value 360Mbps : Default 400Mbps Range [368.125 -433.125 mbps]
> > > 
> > > This hsfreqrange is out of range.
> > > 
> > > The logic is changed to select the first hsfreqrange whose max range[1] is
> > > greater than the calculated bit rate.
> > > 
> > > Calculated value 360Mbps : max range 380.625 mbps is selected
> > >  i.e Default 350Mbps  Range [320.625 -380.625 mpbs]
> > > 
> > > [1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]
> > > 
> > > Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")
> > > 
> > > Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
> > > Signed-off-by: Kazuyoshi Akiyama <akiyama@nds-osk.co.jp>
> > > ---
> > >  Changes in v2:
> > >   - Added the boundary check for the maximum bit rate.
> > > 
> > >   - Simplified the logic by remmoving range check
> > >     as only the closest default value covers most
> > >     of the use cases.
> > > 
> > >   - Aligning the commit message based on the above change
> > > 
> > > 
> > >  Changes in v3:
> > >     - Added max member from struct rcsi2_mbps_reg.
> > >       mbps varialbe cannot be removed from rcsi2_mbps_reg,
> > >       since this structure is reused for
> > >       phtw_mbps_h3_v3h_m3n/phtw_mbps_v3m_e3 where mbps is
> > >       used.
> > > 
> > > 
> > >    -  Update the walk of the array in rcsi2_set_phypll() so that it finds
> > >       the first entry where the calculated bit rate is less than the max.
> > > 
> > >    - Support lower bit rates less than 80Mbps like 48Mbps
> > >      (Raspberry pi camera 640x480 connected to Kingfisher)
> > >      can also be supported by selecting the lowest default bit rate 80Mbps
> > >      as done before this fix
> > > 
> > >    - Alignement of the commit message based on above changes.
> > > 
> > >  Changes in v4:
> > >    - Remove unncessary braces.
> > >  
> > >  Changes in v5:
> > >    - Removed mbps variable in rcsi2_mbps_reg and aligned all 
> > >      tables accordingly
> > > 
> > >  drivers/media/platform/rcar-vin/rcar-csi2.c | 282 ++++++++++++++--------------
> > >  1 file changed, 141 insertions(+), 141 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > index faa9fb2..d45bf80 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > @@ -132,63 +132,63 @@ struct phtw_value {
> > >  };
> > >  
> > >  struct rcsi2_mbps_reg {
> > > -	u16 mbps;
> > >  	u16 reg;
> > > +	u16 max;
> > >  };
> > >  
> > >  static const struct rcsi2_mbps_reg phtw_mbps_h3_v3h_m3n[] = {
> > > -	{ .mbps =   80, .reg = 0x86 },
> > > -	{ .mbps =   90, .reg = 0x86 },
> > > -	{ .mbps =  100, .reg = 0x87 },
> > > -	{ .mbps =  110, .reg = 0x87 },
> > > -	{ .mbps =  120, .reg = 0x88 },
> > > -	{ .mbps =  130, .reg = 0x88 },
> > > -	{ .mbps =  140, .reg = 0x89 },
> > > -	{ .mbps =  150, .reg = 0x89 },
> > > -	{ .mbps =  160, .reg = 0x8a },
> > > -	{ .mbps =  170, .reg = 0x8a },
> > > -	{ .mbps =  180, .reg = 0x8b },
> > > -	{ .mbps =  190, .reg = 0x8b },
> > > -	{ .mbps =  205, .reg = 0x8c },
> > > -	{ .mbps =  220, .reg = 0x8d },
> > > -	{ .mbps =  235, .reg = 0x8e },
> > > -	{ .mbps =  250, .reg = 0x8e },
> > > +	{ .reg = 0x86, .max =  80  },
> > > +	{ .reg = 0x86, .max =  90  },
> > > +	{ .reg = 0x87, .max =  100 },
> > > +	{ .reg = 0x87, .max =  110 },
> > > +	{ .reg = 0x88, .max =  120 },
> > > +	{ .reg = 0x88, .max =  130 },
> > > +	{ .reg = 0x89, .max =  140 },
> > > +	{ .reg = 0x89, .max =  150 },
> > > +	{ .reg = 0x8a, .max =  160 },
> > > +	{ .reg = 0x8a, .max =  170 },
> > > +	{ .reg = 0x8b, .max =  180 },
> > > +	{ .reg = 0x8b, .max =  190 },
> > > +	{ .reg = 0x8c, .max =  205 },
> > > +	{ .reg = 0x8d, .max =  220 },
> > > +	{ .reg = 0x8e, .max =  235 },
> > > +	{ .reg = 0x8e, .max =  250 },
> > >  	{ /* sentinel */ },
> > >  };
> > >  
> > >  static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
> > > -	{ .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 },
> > > +	{ .reg = 0x00, .max = 80   },
> > > +	{ .reg = 0x20, .max = 90   },
> > > +	{ .reg = 0x40, .max = 100  },
> > > +	{ .reg = 0x02, .max = 110  },
> > > +	{ .reg = 0x22, .max = 130  },
> > > +	{ .reg = 0x42, .max = 140  },
> > > +	{ .reg = 0x04, .max = 150  },
> > > +	{ .reg = 0x24, .max = 170  },
> > > +	{ .reg = 0x44, .max = 180  },
> > > +	{ .reg = 0x06, .max = 200  },
> > > +	{ .reg = 0x26, .max = 220  },
> > > +	{ .reg = 0x46, .max = 240  },
> > > +	{ .reg = 0x08, .max = 250  },
> > > +	{ .reg = 0x28, .max = 270  },
> > > +	{ .reg = 0x0a, .max = 300  },
> > > +	{ .reg = 0x2a, .max = 330  },
> > > +	{ .reg = 0x4a, .max = 360  },
> > > +	{ .reg = 0x0c, .max = 400  },
> > > +	{ .reg = 0x2c, .max = 450  },
> > > +	{ .reg = 0x0e, .max = 500  },
> > > +	{ .reg = 0x2e, .max = 550  },
> > > +	{ .reg = 0x10, .max = 600  },
> > > +	{ .reg = 0x30, .max = 650  },
> > > +	{ .reg = 0x12, .max = 700  },
> > > +	{ .reg = 0x32, .max = 750  },
> > > +	{ .reg = 0x52, .max = 800  },
> > > +	{ .reg = 0x72, .max = 850  },
> > > +	{ .reg = 0x14, .max = 900  },
> > > +	{ .reg = 0x34, .max = 950  },
> > > +	{ .reg = 0x54, .max = 1000 },
> > > +	{ .reg = 0x74, .max = 1050 },
> > > +	{ .reg = 0x16, .max = 1125 },
> > >  	{ /* sentinel */ },
> > >  };
> > >  
> > > @@ -201,96 +201,96 @@ static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
> > >  #define PHYPLL_HSFREQRANGE(n)		((n) << 16)
> > >  
> > >  static const struct rcsi2_mbps_reg hsfreqrange_h3_v3h_m3n[] = {
> > > -	{ .mbps =   80, .reg = 0x00 },
> > > -	{ .mbps =   90, .reg = 0x10 },
> > > -	{ .mbps =  100, .reg = 0x20 },
> > > -	{ .mbps =  110, .reg = 0x30 },
> > > -	{ .mbps =  120, .reg = 0x01 },
> > > -	{ .mbps =  130, .reg = 0x11 },
> > > -	{ .mbps =  140, .reg = 0x21 },
> > > -	{ .mbps =  150, .reg = 0x31 },
> > > -	{ .mbps =  160, .reg = 0x02 },
> > > -	{ .mbps =  170, .reg = 0x12 },
> > > -	{ .mbps =  180, .reg = 0x22 },
> > > -	{ .mbps =  190, .reg = 0x32 },
> > > -	{ .mbps =  205, .reg = 0x03 },
> > > -	{ .mbps =  220, .reg = 0x13 },
> > > -	{ .mbps =  235, .reg = 0x23 },
> > > -	{ .mbps =  250, .reg = 0x33 },
> > > -	{ .mbps =  275, .reg = 0x04 },
> > > -	{ .mbps =  300, .reg = 0x14 },
> > > -	{ .mbps =  325, .reg = 0x25 },
> > > -	{ .mbps =  350, .reg = 0x35 },
> > > -	{ .mbps =  400, .reg = 0x05 },
> > > -	{ .mbps =  450, .reg = 0x16 },
> > > -	{ .mbps =  500, .reg = 0x26 },
> > > -	{ .mbps =  550, .reg = 0x37 },
> > > -	{ .mbps =  600, .reg = 0x07 },
> > > -	{ .mbps =  650, .reg = 0x18 },
> > > -	{ .mbps =  700, .reg = 0x28 },
> > > -	{ .mbps =  750, .reg = 0x39 },
> > > -	{ .mbps =  800, .reg = 0x09 },
> > > -	{ .mbps =  850, .reg = 0x19 },
> > > -	{ .mbps =  900, .reg = 0x29 },
> > > -	{ .mbps =  950, .reg = 0x3a },
> > > -	{ .mbps = 1000, .reg = 0x0a },
> > > -	{ .mbps = 1050, .reg = 0x1a },
> > > -	{ .mbps = 1100, .reg = 0x2a },
> > > -	{ .mbps = 1150, .reg = 0x3b },
> > > -	{ .mbps = 1200, .reg = 0x0b },
> > > -	{ .mbps = 1250, .reg = 0x1b },
> > > -	{ .mbps = 1300, .reg = 0x2b },
> > > -	{ .mbps = 1350, .reg = 0x3c },
> > > -	{ .mbps = 1400, .reg = 0x0c },
> > > -	{ .mbps = 1450, .reg = 0x1c },
> > > -	{ .mbps = 1500, .reg = 0x2c },
> > > +	{ .reg = 0x00, .max =   97 },
> > > +	{ .reg = 0x10, .max =  107 },
> > > +	{ .reg = 0x20, .max =  118 },
> > > +	{ .reg = 0x30, .max =  128 },
> > > +	{ .reg = 0x01, .max =  139 },
> > > +	{ .reg = 0x11, .max =  149 },
> > > +	{ .reg = 0x21, .max =  160 },
> > > +	{ .reg = 0x31, .max =  170 },
> > > +	{ .reg = 0x02, .max =  181 },
> > > +	{ .reg = 0x12, .max =  191 },
> > > +	{ .reg = 0x22, .max =  202 },
> > > +	{ .reg = 0x32, .max =  212 },
> > > +	{ .reg = 0x03, .max =  228 },
> > > +	{ .reg = 0x13, .max =  224 },
> > > +	{ .reg = 0x23, .max =  259 },
> > > +	{ .reg = 0x33, .max =  275 },
> > > +	{ .reg = 0x04, .max =  301 },
> > > +	{ .reg = 0x14, .max =  328 },
> > > +	{ .reg = 0x25, .max =  354 },
> > > +	{ .reg = 0x35, .max =  380 },
> > > +	{ .reg = 0x05, .max =  433 },
> > > +	{ .reg = 0x16, .max =  485 },
> > > +	{ .reg = 0x26, .max =  538 },
> > > +	{ .reg = 0x37, .max =  590 },
> > > +	{ .reg = 0x07, .max =  643 },
> > > +	{ .reg = 0x18, .max =  695 },
> > > +	{ .reg = 0x28, .max =  748 },
> > > +	{ .reg = 0x39, .max =  800 },
> > > +	{ .reg = 0x09, .max =  853 },
> > > +	{ .reg = 0x19, .max =  905 },
> > > +	{ .reg = 0x29, .max =  958 },
> > > +	{ .reg = 0x3a, .max = 1010 },
> > > +	{ .reg = 0x0a, .max = 1063 },
> > > +	{ .reg = 0x1a, .max = 1115 },
> > > +	{ .reg = 0x2a, .max = 1168 },
> > > +	{ .reg = 0x3b, .max = 1220 },
> > > +	{ .reg = 0x0b, .max = 1273 },
> > > +	{ .reg = 0x1b, .max = 1325 },
> > > +	{ .reg = 0x2b, .max = 1378 },
> > > +	{ .reg = 0x3c, .max = 1430 },
> > > +	{ .reg = 0x0c, .max = 1483 },
> > > +	{ .reg = 0x1c, .max = 1500 },
> > > +	{ .reg = 0x2c, .max = 1500 },
> > >  	{ /* sentinel */ },
> > >  };
> > >  
> > >  static const struct rcsi2_mbps_reg hsfreqrange_m3w_h3es1[] = {
> > > -	{ .mbps =   80,	.reg = 0x00 },
> > > -	{ .mbps =   90,	.reg = 0x10 },
> > > -	{ .mbps =  100,	.reg = 0x20 },
> > > -	{ .mbps =  110,	.reg = 0x30 },
> > > -	{ .mbps =  120,	.reg = 0x01 },
> > > -	{ .mbps =  130,	.reg = 0x11 },
> > > -	{ .mbps =  140,	.reg = 0x21 },
> > > -	{ .mbps =  150,	.reg = 0x31 },
> > > -	{ .mbps =  160,	.reg = 0x02 },
> > > -	{ .mbps =  170,	.reg = 0x12 },
> > > -	{ .mbps =  180,	.reg = 0x22 },
> > > -	{ .mbps =  190,	.reg = 0x32 },
> > > -	{ .mbps =  205,	.reg = 0x03 },
> > > -	{ .mbps =  220,	.reg = 0x13 },
> > > -	{ .mbps =  235,	.reg = 0x23 },
> > > -	{ .mbps =  250,	.reg = 0x33 },
> > > -	{ .mbps =  275,	.reg = 0x04 },
> > > -	{ .mbps =  300,	.reg = 0x14 },
> > > -	{ .mbps =  325,	.reg = 0x05 },
> > > -	{ .mbps =  350,	.reg = 0x15 },
> > > -	{ .mbps =  400,	.reg = 0x25 },
> > > -	{ .mbps =  450,	.reg = 0x06 },
> > > -	{ .mbps =  500,	.reg = 0x16 },
> > > -	{ .mbps =  550,	.reg = 0x07 },
> > > -	{ .mbps =  600,	.reg = 0x17 },
> > > -	{ .mbps =  650,	.reg = 0x08 },
> > > -	{ .mbps =  700,	.reg = 0x18 },
> > > -	{ .mbps =  750,	.reg = 0x09 },
> > > -	{ .mbps =  800,	.reg = 0x19 },
> > > -	{ .mbps =  850,	.reg = 0x29 },
> > > -	{ .mbps =  900,	.reg = 0x39 },
> > > -	{ .mbps =  950,	.reg = 0x0a },
> > > -	{ .mbps = 1000,	.reg = 0x1a },
> > > -	{ .mbps = 1050,	.reg = 0x2a },
> > > -	{ .mbps = 1100,	.reg = 0x3a },
> > > -	{ .mbps = 1150,	.reg = 0x0b },
> > > -	{ .mbps = 1200,	.reg = 0x1b },
> > > -	{ .mbps = 1250,	.reg = 0x2b },
> > > -	{ .mbps = 1300,	.reg = 0x3b },
> > > -	{ .mbps = 1350,	.reg = 0x0c },
> > > -	{ .mbps = 1400,	.reg = 0x1c },
> > > -	{ .mbps = 1450,	.reg = 0x2c },
> > > -	{ .mbps = 1500,	.reg = 0x3c },
> > > +	{ .reg = 0x00, .max =  110 },
> > > +	{ .reg = 0x10, .max =  120 },
> > > +	{ .reg = 0x20, .max =  131 },
> > > +	{ .reg = 0x30, .max =  141 },
> > > +	{ .reg = 0x01, .max =  152 },
> > > +	{ .reg = 0x11, .max =  162 },
> > > +	{ .reg = 0x21, .max =  173 },
> > > +	{ .reg = 0x31, .max =  183 },
> > > +	{ .reg = 0x02, .max =  194 },
> > > +	{ .reg = 0x12, .max =  204 },
> > > +	{ .reg = 0x22, .max =  215 },
> > > +	{ .reg = 0x32, .max =  225 },
> > > +	{ .reg = 0x03, .max =  241 },
> > > +	{ .reg = 0x13, .max =  257 },
> > > +	{ .reg = 0x23, .max =  273 },
> > > +	{ .reg = 0x33, .max =  275 },
> > > +	{ .reg = 0x04, .max =  301 },
> > > +	{ .reg = 0x14, .max =  328 },
> > > +	{ .reg = 0x05, .max =  354 },
> > > +	{ .reg = 0x15, .max =  393 },
> > > +	{ .reg = 0x25, .max =  446 },
> > > +	{ .reg = 0x06, .max =  498 },
> > > +	{ .reg = 0x16, .max =  551 },
> > > +	{ .reg = 0x07, .max =  603 },
> > > +	{ .reg = 0x17, .max =  656 },
> > > +	{ .reg = 0x08, .max =  708 },
> > > +	{ .reg = 0x18, .max =  761 },
> > > +	{ .reg = 0x09, .max =  813 },
> > > +	{ .reg = 0x19, .max =  866 },
> > > +	{ .reg = 0x29, .max =  918 },
> > > +	{ .reg = 0x39, .max =  971 },
> > > +	{ .reg = 0x0a, .max = 1023 },
> > > +	{ .reg = 0x1a, .max = 1076 },
> > > +	{ .reg = 0x2a, .max = 1128 },
> > > +	{ .reg = 0x3a, .max = 1181 },
> > > +	{ .reg = 0x0b, .max = 1233 },
> > > +	{ .reg = 0x1b, .max = 1286 },
> > > +	{ .reg = 0x2b, .max = 1338 },
> > > +	{ .reg = 0x3b, .max = 1391 },
> > > +	{ .reg = 0x0c, .max = 1443 },
> > > +	{ .reg = 0x1c, .max = 1496 },
> > > +	{ .reg = 0x2c, .max = 1500 },
> > > +	{ .reg = 0x3c, .max = 1500 },
> > >  	{ /* sentinel */ },
> > >  };
> > >  
> > > @@ -432,11 +432,11 @@ static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
> > >  {
> > >  	const struct rcsi2_mbps_reg *hsfreq;
> > >  
> > > -	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
> > > -		if (hsfreq->mbps >= mbps)
> > > +	for (hsfreq = priv->info->hsfreqrange; hsfreq->max != 0; hsfreq++)
> > > +		if (hsfreq->max >= mbps)
> > >  			break;
> > >  
> > > -	if (!hsfreq->mbps) {
> > > +	if (!hsfreq->max) {
> > >  		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
> > >  		return -ERANGE;
> > >  	}
> > > @@ -907,11 +907,11 @@ static int rcsi2_phtw_write_mbps(struct rcar_csi2 *priv, unsigned int mbps,
> > >  {
> > >  	const struct rcsi2_mbps_reg *value;
> > >  
> > > -	for (value = values; value->mbps; value++)
> > > -		if (value->mbps >= mbps)
> > > +	for (value = values; value->max; value++)
> > > +		if (value->max >= mbps)
> > >  			break;
> > >  
> > > -	if (!value->mbps) {
> > > +	if (!value->max) {
> > >  		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
> > >  		return -ERANGE;
> > >  	}
> > > -- 
> > > 2.7.4
> > > 
> > 
> > -- 
> > Best Regards,
> > Michael
> 
> -- 
> Regards,
> Niklas Söderlund

-- 
Best Regards,
Michael

[PATCH] rcar-vin: rcar-csi2: Correct the selection of hsfreqrange

[Michael Rodin]

From: Suresh Udipi <sudipi@jp.adit-jv.com>

hsfreqrange should be chosen based on the calculated mbps which
is closer to the default bit rate  and within the range as per
table[1]. But current calculation always selects first value which
is greater than or equal to the calculated mbps which may lead
to chosing a wrong range in some cases.

For example for 360 mbps for H3/M3N
Existing logic selects
Calculated value 360Mbps : Default 400Mbps Range [368.125 -433.125 mbps]

This hsfreqrange is out of range.

The logic is changed to get the default value which is closest to the
calculated value [1]

Calculated value 360Mbps : Default 350Mbps  Range [320.625 -380.625 mpbs]

[1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]

There is one exectpion value 227Mbps, which may cause out of
range. This needs to be further handled if required.

Fixes: ADIT v4.14 commit 9e568b895ee0 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")

Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
---
 drivers/media/platform/rcar-vin/rcar-csi2.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index c473a56..73d9830 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -199,6 +199,7 @@ static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
 /* PHY Frequency Control */
 #define PHYPLL_REG			0x68
 #define PHYPLL_HSFREQRANGE(n)		((n) << 16)
+#define PHYPLL_HSFREQRANGE_MAX		1500
 
 static const struct rcsi2_mbps_reg hsfreqrange_h3_v3h_m3n[] = {
 	{ .mbps =   80, .reg = 0x00 },
@@ -446,16 +447,23 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
 static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
 {
 	const struct rcsi2_mbps_reg *hsfreq;
+	const struct rcsi2_mbps_reg *hsfreq_prev = NULL;
 
-	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
-		if (hsfreq->mbps >= mbps)
-			break;
-
-	if (!hsfreq->mbps) {
+	if (mbps > PHYPLL_HSFREQRANGE_MAX) {
 		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
 		return -ERANGE;
 	}
 
+	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) {
+		if (hsfreq->mbps >= mbps)
+			break;
+		hsfreq_prev = hsfreq;
+	}
+
+	if (hsfreq_prev &&
+	    ((mbps - hsfreq_prev->mbps) <= (hsfreq->mbps - mbps)))
+		hsfreq = hsfreq_prev;
+
 	rcsi2_write(priv, PHYPLL_REG, PHYPLL_HSFREQRANGE(hsfreq->reg));
 
 	return 0;
-- 
2.7.4

Re: [PATCH] rcar-vin: rcar-csi2: Correct the selection of hsfreqrange

[Michael Rodin]

Hello Niklas and Suresh,

Renesas confirmed that there is a typo in the Hardware Manual (Table 25.9):
The correct range for 220 Mbps is 197.125-244.125 and not 197.125 - 224.125
so the both patches are correct, we can do configuration based only on
the "default" bit rates. I would say that now we can correct the commit
message with respect to the exception value and use these patches. I would
additionally add a warning for bitrates below 80 Mbps. They seem to work
(for example Raspberry Pi camera), however they are not officially supported
so the user needs to be warned.

On Wed, May 27, 2020 at 06:16:07PM +0200, Michael Rodin wrote:
> From: Suresh Udipi <sudipi@jp.adit-jv.com>
> 
> hsfreqrange should be chosen based on the calculated mbps which
> is closer to the default bit rate  and within the range as per
> table[1]. But current calculation always selects first value which
> is greater than or equal to the calculated mbps which may lead
> to chosing a wrong range in some cases.
> 
> For example for 360 mbps for H3/M3N
> Existing logic selects
> Calculated value 360Mbps : Default 400Mbps Range [368.125 -433.125 mbps]
> 
> This hsfreqrange is out of range.
> 
> The logic is changed to get the default value which is closest to the
> calculated value [1]
> 
> Calculated value 360Mbps : Default 350Mbps  Range [320.625 -380.625 mpbs]
> 
> [1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]
> 
> There is one exectpion value 227Mbps, which may cause out of
> range. This needs to be further handled if required.
> 
> Fixes: ADIT v4.14 commit 9e568b895ee0 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")
> 
> Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
> Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index c473a56..73d9830 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -199,6 +199,7 @@ static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
>  /* PHY Frequency Control */
>  #define PHYPLL_REG			0x68
>  #define PHYPLL_HSFREQRANGE(n)		((n) << 16)
> +#define PHYPLL_HSFREQRANGE_MAX		1500
>  
>  static const struct rcsi2_mbps_reg hsfreqrange_h3_v3h_m3n[] = {
>  	{ .mbps =   80, .reg = 0x00 },
> @@ -446,16 +447,23 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
>  static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
>  {
>  	const struct rcsi2_mbps_reg *hsfreq;
> +	const struct rcsi2_mbps_reg *hsfreq_prev = NULL;
>  
> -	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
> -		if (hsfreq->mbps >= mbps)
> -			break;
> -
> -	if (!hsfreq->mbps) {
> +	if (mbps > PHYPLL_HSFREQRANGE_MAX) {
>  		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
>  		return -ERANGE;
>  	}
>  
> +	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) {
> +		if (hsfreq->mbps >= mbps)
> +			break;
> +		hsfreq_prev = hsfreq;
> +	}
> +
> +	if (hsfreq_prev &&
> +	    ((mbps - hsfreq_prev->mbps) <= (hsfreq->mbps - mbps)))
> +		hsfreq = hsfreq_prev;
> +
>  	rcsi2_write(priv, PHYPLL_REG, PHYPLL_HSFREQRANGE(hsfreq->reg));
>  
>  	return 0;
> -- 
> 2.7.4
> 

-- 
Best Regards,
Michael

[PATCH v6] media: rcar-csi2: Correct the selection of hsfreqrange

[Suresh Udipi]

hsfreqrange should be chosen based on the calculated mbps which
is closer to the default bit rate  and within the range as per
table[1]. But current calculation always selects first value which
is greater than or equal to the calculated mbps which may lead
to chosing a wrong range in some cases.

For example for 360 mbps for H3/M3N
Existing logic selects
Calculated value 360Mbps : Default 400Mbps Range [368.125 -433.125 mbps]

This hsfreqrange is out of range.

The logic is changed to get the default value which is closest to the
calculated value [1]

Calculated value 360Mbps : Default 350Mbps  Range [320.625 -380.625 mpbs]

[1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]

Please note that According to Renesas in Table 25.9 the range for
220 default value is corrected as below

 |Range (Mbps)     |  Default  Bit rate (Mbps) |
 -----------------------------------------------
 | 197.125-244.125 |     220                   |
 -----------------------------------------------

Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")

Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
Signed-off-by: Kazuyoshi Akiyama <akiyama@nds-osk.co.jp>
Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
---
 Changes in v2:
  - Added the boundary check for the maximum bit rate.

  - Simplified the logic by remmoving range check
    as only the closest default value covers most
    of the use cases.

  - Aligning the commit message based on the above change


 Changes in v3:
    - Added max member from struct rcsi2_mbps_reg.
      mbps varialbe cannot be removed from rcsi2_mbps_reg,
      since this structure is reused for
      phtw_mbps_h3_v3h_m3n/phtw_mbps_v3m_e3 where mbps is
      used.


   -  Update the walk of the array in rcsi2_set_phypll() so that it finds
      the first entry where the calculated bit rate is less than the max.

   - Support lower bit rates less than 80Mbps like 48Mbps
     (Raspberry pi camera 640x480 connected to Kingfisher)
     can also be supported by selecting the lowest default bit rate 80Mbps
     as done before this fix

   - Alignement of the commit message based on above changes.

 Changes in v4:
  -  Remove unncessary braces.

 Changes in v5:
   - Removed mbps variable in rcsi2_mbps_reg and aligned all 
     tables accordingly
	 
 Changes in v6
   - Renesas correct the range of default value 220Mbps. Now
     if we select the nearest value to the default value all
	 the values are in range. So reverting back to original patch
	 
   - Added warning for values less than Minimum 80Mbps


 drivers/media/platform/rcar-vin/rcar-csi2.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index 151e6a9..8c502b7 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -199,6 +199,8 @@ static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
 /* PHY Frequency Control */
 #define PHYPLL_REG			0x68
 #define PHYPLL_HSFREQRANGE(n)		((n) << 16)
+#define PHYPLL_HSFREQRANGE_MAX		1500
+#define PHYPLL_HSFREQRANGE_MIN		  80
 
 static const struct rcsi2_mbps_reg hsfreqrange_h3_v3h_m3n[] = {
 	{ .mbps =   80, .reg = 0x00 },
@@ -431,16 +433,27 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
 static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
 {
 	const struct rcsi2_mbps_reg *hsfreq;
+	const struct rcsi2_mbps_reg *hsfreq_prev = NULL;
 
-	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
-		if (hsfreq->mbps >= mbps)
-			break;
-
-	if (!hsfreq->mbps) {
+	if (mbps > PHYPLL_HSFREQRANGE_MAX) {
 		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
 		return -ERANGE;
 	}
 
+	if (mbps < PHYPLL_HSFREQRANGE_MIN)
+		dev_warn(priv->dev, "PHY speed (%u Mbps) less \
+			 than Min 80Mbps\n", mbps);
+
+	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) {
+		if (hsfreq->mbps >= mbps)
+			break;
+		hsfreq_prev = hsfreq;
+	}
+
+	if (hsfreq_prev &&
+	    ((mbps - hsfreq_prev->mbps) <= (hsfreq->mbps - mbps)))
+		hsfreq = hsfreq_prev;
+
 	rcsi2_write(priv, PHYPLL_REG, PHYPLL_HSFREQRANGE(hsfreq->reg));
 
 	return 0;
-- 
2.7.4

Re: [PATCH v6] media: rcar-csi2: Correct the selection of hsfreqrange

[Niklas Söderlund]

Hi Suresh,

Thanks for your persistent work!

On 2020-06-08 12:25:03 +0900, Suresh Udipi wrote:
> hsfreqrange should be chosen based on the calculated mbps which
> is closer to the default bit rate  and within the range as per
> table[1]. But current calculation always selects first value which
> is greater than or equal to the calculated mbps which may lead
> to chosing a wrong range in some cases.
> 
> For example for 360 mbps for H3/M3N
> Existing logic selects
> Calculated value 360Mbps : Default 400Mbps Range [368.125 -433.125 mbps]
> 
> This hsfreqrange is out of range.
> 
> The logic is changed to get the default value which is closest to the
> calculated value [1]
> 
> Calculated value 360Mbps : Default 350Mbps  Range [320.625 -380.625 mpbs]
> 
> [1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]
> 
> Please note that According to Renesas in Table 25.9 the range for
> 220 default value is corrected as below
> 
>  |Range (Mbps)     |  Default  Bit rate (Mbps) |
>  -----------------------------------------------
>  | 197.125-244.125 |     220                   |
>  -----------------------------------------------
> 
> Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")
> 
> Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
> Signed-off-by: Kazuyoshi Akiyama <akiyama@nds-osk.co.jp>
> Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
> ---
>  Changes in v2:
>   - Added the boundary check for the maximum bit rate.
> 
>   - Simplified the logic by remmoving range check
>     as only the closest default value covers most
>     of the use cases.
> 
>   - Aligning the commit message based on the above change
> 
> 
>  Changes in v3:
>     - Added max member from struct rcsi2_mbps_reg.
>       mbps varialbe cannot be removed from rcsi2_mbps_reg,
>       since this structure is reused for
>       phtw_mbps_h3_v3h_m3n/phtw_mbps_v3m_e3 where mbps is
>       used.
> 
> 
>    -  Update the walk of the array in rcsi2_set_phypll() so that it finds
>       the first entry where the calculated bit rate is less than the max.
> 
>    - Support lower bit rates less than 80Mbps like 48Mbps
>      (Raspberry pi camera 640x480 connected to Kingfisher)
>      can also be supported by selecting the lowest default bit rate 80Mbps
>      as done before this fix
> 
>    - Alignement of the commit message based on above changes.
> 
>  Changes in v4:
>   -  Remove unncessary braces.
> 
>  Changes in v5:
>    - Removed mbps variable in rcsi2_mbps_reg and aligned all 
>      tables accordingly
> 	 
>  Changes in v6
>    - Renesas correct the range of default value 220Mbps. Now
>      if we select the nearest value to the default value all
> 	 the values are in range. So reverting back to original patch
> 	 
>    - Added warning for values less than Minimum 80Mbps
> 
> 
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index 151e6a9..8c502b7 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -199,6 +199,8 @@ static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
>  /* PHY Frequency Control */
>  #define PHYPLL_REG			0x68
>  #define PHYPLL_HSFREQRANGE(n)		((n) << 16)
> +#define PHYPLL_HSFREQRANGE_MAX		1500
> +#define PHYPLL_HSFREQRANGE_MIN		  80
>  
>  static const struct rcsi2_mbps_reg hsfreqrange_h3_v3h_m3n[] = {
>  	{ .mbps =   80, .reg = 0x00 },
> @@ -431,16 +433,27 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
>  static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
>  {
>  	const struct rcsi2_mbps_reg *hsfreq;
> +	const struct rcsi2_mbps_reg *hsfreq_prev = NULL;
>  
> -	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
> -		if (hsfreq->mbps >= mbps)
> -			break;
> -
> -	if (!hsfreq->mbps) {
> +	if (mbps > PHYPLL_HSFREQRANGE_MAX) {
>  		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
>  		return -ERANGE;
>  	}
>  
> +	if (mbps < PHYPLL_HSFREQRANGE_MIN)
> +		dev_warn(priv->dev, "PHY speed (%u Mbps) less \
> +			 than Min 80Mbps\n", mbps);

I would drop this warning.

> +
> +	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) {
> +		if (hsfreq->mbps >= mbps)
> +			break;
> +		hsfreq_prev = hsfreq;
> +	}
> +
> +	if (hsfreq_prev &&
> +	    ((mbps - hsfreq_prev->mbps) <= (hsfreq->mbps - mbps)))

Longer lines are now OK [1] and I think it would add to the readability 
here.

> +		hsfreq = hsfreq_prev;
> +

How about

static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
{
    const struct rcsi2_mbps_reg *hsfreq;
    const struct rcsi2_mbps_reg *hsfreq_prev = NULL;

    for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) {
	if (hsfreq->mbps >= mbps)
		break;
	hsfreq_prev = hsfreq;
    }

    if (!hsfreq->mbps) {
	dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
	return -ERANGE;
    }

    if (hsfreq_prev && ((mbps - hsfreq_prev->mbps) <= (hsfreq->mbps - mbps)))
	hsfreq = hsfreq_prev;

    rcsi2_write(priv, PHYPLL_REG, PHYPLL_HSFREQRANGE(hsfreq->reg));

    return 0;
}

>  	rcsi2_write(priv, PHYPLL_REG, PHYPLL_HSFREQRANGE(hsfreq->reg));
>  
>  	return 0;
> -- 
> 2.7.4
> 

1. https://lkml.org/lkml/2020/5/29/1038

-- 
Regards,
Niklas Söderlund

Re: [PATCH v6] media: rcar-csi2: Correct the selection of hsfreqrange

[Suresh Udipi]

On Wed, Jun 10, 2020 at 03:40:04PM +0200, Niklas Söderlund wrote:
> Hi Suresh,
> 
> Thanks for your persistent work!
> 
> On 2020-06-08 12:25:03 +0900, Suresh Udipi wrote:
> > hsfreqrange should be chosen based on the calculated mbps which
> > is closer to the default bit rate  and within the range as per
> > table[1]. But current calculation always selects first value which
> > is greater than or equal to the calculated mbps which may lead
> > to chosing a wrong range in some cases.
> > 
> > For example for 360 mbps for H3/M3N
> > Existing logic selects
> > Calculated value 360Mbps : Default 400Mbps Range [368.125 -433.125 mbps]
> > 
> > This hsfreqrange is out of range.
> > 
> > The logic is changed to get the default value which is closest to the
> > calculated value [1]
> > 
> > Calculated value 360Mbps : Default 350Mbps  Range [320.625 -380.625 mpbs]
> > 
> > [1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]
> > 
> > Please note that According to Renesas in Table 25.9 the range for
> > 220 default value is corrected as below
> > 
> >  |Range (Mbps)     |  Default  Bit rate (Mbps) |
> >  -----------------------------------------------
> >  | 197.125-244.125 |     220                   |
> >  -----------------------------------------------
> > 
> > Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")
> > 
> > Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
> > Signed-off-by: Kazuyoshi Akiyama <akiyama@nds-osk.co.jp>
> > Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
> > ---
> >  Changes in v2:
> >   - Added the boundary check for the maximum bit rate.
> > 
> >   - Simplified the logic by remmoving range check
> >     as only the closest default value covers most
> >     of the use cases.
> > 
> >   - Aligning the commit message based on the above change
> > 
> > 
> >  Changes in v3:
> >     - Added max member from struct rcsi2_mbps_reg.
> >       mbps varialbe cannot be removed from rcsi2_mbps_reg,
> >       since this structure is reused for
> >       phtw_mbps_h3_v3h_m3n/phtw_mbps_v3m_e3 where mbps is
> >       used.
> > 
> > 
> >    -  Update the walk of the array in rcsi2_set_phypll() so that it finds
> >       the first entry where the calculated bit rate is less than the max.
> > 
> >    - Support lower bit rates less than 80Mbps like 48Mbps
> >      (Raspberry pi camera 640x480 connected to Kingfisher)
> >      can also be supported by selecting the lowest default bit rate 80Mbps
> >      as done before this fix
> > 
> >    - Alignement of the commit message based on above changes.
> > 
> >  Changes in v4:
> >   -  Remove unncessary braces.
> > 
> >  Changes in v5:
> >    - Removed mbps variable in rcsi2_mbps_reg and aligned all 
> >      tables accordingly
> > 	 
> >  Changes in v6
> >    - Renesas correct the range of default value 220Mbps. Now
> >      if we select the nearest value to the default value all
> > 	 the values are in range. So reverting back to original patch
> > 	 
> >    - Added warning for values less than Minimum 80Mbps
> > 
> > 
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 23 ++++++++++++++++++-----
> >  1 file changed, 18 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index 151e6a9..8c502b7 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -199,6 +199,8 @@ static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
> >  /* PHY Frequency Control */
> >  #define PHYPLL_REG			0x68
> >  #define PHYPLL_HSFREQRANGE(n)		((n) << 16)
> > +#define PHYPLL_HSFREQRANGE_MAX		1500
> > +#define PHYPLL_HSFREQRANGE_MIN		  80
> >  
> >  static const struct rcsi2_mbps_reg hsfreqrange_h3_v3h_m3n[] = {
> >  	{ .mbps =   80, .reg = 0x00 },
> > @@ -431,16 +433,27 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
> >  static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
> >  {
> >  	const struct rcsi2_mbps_reg *hsfreq;
> > +	const struct rcsi2_mbps_reg *hsfreq_prev = NULL;
> >  
> > -	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
> > -		if (hsfreq->mbps >= mbps)
> > -			break;
> > -
> > -	if (!hsfreq->mbps) {
> > +	if (mbps > PHYPLL_HSFREQRANGE_MAX) {
> >  		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
> >  		return -ERANGE;
> >  	}
> >  
> > +	if (mbps < PHYPLL_HSFREQRANGE_MIN)
> > +		dev_warn(priv->dev, "PHY speed (%u Mbps) less \
> > +			 than Min 80Mbps\n", mbps);
> 
> I would drop this warning.
> 

  This was suggested by Michael. Michael is it ok to drop this warning
  as it is not available before this changes also. 

> > +
> > +	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) {
> > +		if (hsfreq->mbps >= mbps)
> > +			break;
> > +		hsfreq_prev = hsfreq;
> > +	}
> > +
> > +	if (hsfreq_prev &&
> > +	    ((mbps - hsfreq_prev->mbps) <= (hsfreq->mbps - mbps)))
> 
> Longer lines are now OK [1] and I think it would add to the readability 
> here.
> 
> > +		hsfreq = hsfreq_prev;
> > +
> 
> How about
> 
> static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
> {
>     const struct rcsi2_mbps_reg *hsfreq;
>     const struct rcsi2_mbps_reg *hsfreq_prev = NULL;
> 
>     for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) {
> 	if (hsfreq->mbps >= mbps)
> 		break;
> 	hsfreq_prev = hsfreq;
>     }
> 
>     if (!hsfreq->mbps) {
> 	dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
> 	return -ERANGE;
>     }
> 
>     if (hsfreq_prev && ((mbps - hsfreq_prev->mbps) <= (hsfreq->mbps - mbps)))
> 	hsfreq = hsfreq_prev;
> 
>     rcsi2_write(priv, PHYPLL_REG, PHYPLL_HSFREQRANGE(hsfreq->reg));
> 
>     return 0;
> }
> 
> >  	rcsi2_write(priv, PHYPLL_REG, PHYPLL_HSFREQRANGE(hsfreq->reg));
> >  
> >  	return 0;

Agreed I will do the changes and update.
> > -- 
> > 2.7.4
> > 
> 
> 1. https://lkml.org/lkml/2020/5/29/1038
> 
> -- 
> Regards,
> Niklas Söderlund

-- 
Best Regards,
Suresh Udipi.

Re: [PATCH v6] media: rcar-csi2: Correct the selection of hsfreqrange

[Michael Rodin]

Hi Niklas,
Hi Suresh,

On Fri, Jun 12, 2020 at 12:10:51PM +0900, Suresh Udipi wrote:
> On Wed, Jun 10, 2020 at 03:40:04PM +0200, Niklas Söderlund wrote:
> > Hi Suresh,
> > 
> > Thanks for your persistent work!
> > 
> > On 2020-06-08 12:25:03 +0900, Suresh Udipi wrote:
> > > hsfreqrange should be chosen based on the calculated mbps which
> > > is closer to the default bit rate  and within the range as per
> > > table[1]. But current calculation always selects first value which
> > > is greater than or equal to the calculated mbps which may lead
> > > to chosing a wrong range in some cases.
> > > 
> > > For example for 360 mbps for H3/M3N
> > > Existing logic selects
> > > Calculated value 360Mbps : Default 400Mbps Range [368.125 -433.125 mbps]
> > > 
> > > This hsfreqrange is out of range.
> > > 
> > > The logic is changed to get the default value which is closest to the
> > > calculated value [1]
> > > 
> > > Calculated value 360Mbps : Default 350Mbps  Range [320.625 -380.625 mpbs]
> > > 
> > > [1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]
> > > 
> > > Please note that According to Renesas in Table 25.9 the range for
> > > 220 default value is corrected as below
> > > 
> > >  |Range (Mbps)     |  Default  Bit rate (Mbps) |
> > >  -----------------------------------------------
> > >  | 197.125-244.125 |     220                   |
> > >  -----------------------------------------------
> > > 
> > > Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")
> > > 
> > > Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
> > > Signed-off-by: Kazuyoshi Akiyama <akiyama@nds-osk.co.jp>
> > > Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
> > > ---
> > >  Changes in v2:
> > >   - Added the boundary check for the maximum bit rate.
> > > 
> > >   - Simplified the logic by remmoving range check
> > >     as only the closest default value covers most
> > >     of the use cases.
> > > 
> > >   - Aligning the commit message based on the above change
> > > 
> > > 
> > >  Changes in v3:
> > >     - Added max member from struct rcsi2_mbps_reg.
> > >       mbps varialbe cannot be removed from rcsi2_mbps_reg,
> > >       since this structure is reused for
> > >       phtw_mbps_h3_v3h_m3n/phtw_mbps_v3m_e3 where mbps is
> > >       used.
> > > 
> > > 
> > >    -  Update the walk of the array in rcsi2_set_phypll() so that it finds
> > >       the first entry where the calculated bit rate is less than the max.
> > > 
> > >    - Support lower bit rates less than 80Mbps like 48Mbps
> > >      (Raspberry pi camera 640x480 connected to Kingfisher)
> > >      can also be supported by selecting the lowest default bit rate 80Mbps
> > >      as done before this fix
> > > 
> > >    - Alignement of the commit message based on above changes.
> > > 
> > >  Changes in v4:
> > >   -  Remove unncessary braces.
> > > 
> > >  Changes in v5:
> > >    - Removed mbps variable in rcsi2_mbps_reg and aligned all 
> > >      tables accordingly
> > > 	 
> > >  Changes in v6
> > >    - Renesas correct the range of default value 220Mbps. Now
> > >      if we select the nearest value to the default value all
> > > 	 the values are in range. So reverting back to original patch
> > > 	 
> > >    - Added warning for values less than Minimum 80Mbps
> > > 
> > > 
> > >  drivers/media/platform/rcar-vin/rcar-csi2.c | 23 ++++++++++++++++++-----
> > >  1 file changed, 18 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > index 151e6a9..8c502b7 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > @@ -199,6 +199,8 @@ static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
> > >  /* PHY Frequency Control */
> > >  #define PHYPLL_REG			0x68
> > >  #define PHYPLL_HSFREQRANGE(n)		((n) << 16)
> > > +#define PHYPLL_HSFREQRANGE_MAX		1500
> > > +#define PHYPLL_HSFREQRANGE_MIN		  80
> > >  
> > >  static const struct rcsi2_mbps_reg hsfreqrange_h3_v3h_m3n[] = {
> > >  	{ .mbps =   80, .reg = 0x00 },
> > > @@ -431,16 +433,27 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
> > >  static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
> > >  {
> > >  	const struct rcsi2_mbps_reg *hsfreq;
> > > +	const struct rcsi2_mbps_reg *hsfreq_prev = NULL;
> > >  
> > > -	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
> > > -		if (hsfreq->mbps >= mbps)
> > > -			break;
> > > -
> > > -	if (!hsfreq->mbps) {
> > > +	if (mbps > PHYPLL_HSFREQRANGE_MAX) {
> > >  		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
> > >  		return -ERANGE;
> > >  	}
> > >  
> > > +	if (mbps < PHYPLL_HSFREQRANGE_MIN)
> > > +		dev_warn(priv->dev, "PHY speed (%u Mbps) less \
> > > +			 than Min 80Mbps\n", mbps);
> > 
> > I would drop this warning.
> > 
> 
>   This was suggested by Michael. Michael is it ok to drop this warning
>   as it is not available before this changes also. 
> 

I strongly disagree. We should keep the warning for the following reasons:

 1. Renesas explicitly states in the hardware manual tables, that 80Mbps is
    the lowest supported bit rate (I guess, for a good reason), so using
    devices with lower bit rates is at our own risk.
 2. Failing for mbps > PHYPLL_HSFREQRANGE_MAX with an ERANGE error but
    silently succeeding for mbps < PHYPLL_HSFREQRANGE_MIN does not look
    consistent. Both values are out of the official hardware specs.
 3. Although rcar csi2 seems to work with at least 1 device in the range
    mbps < PHYPLL_HSFREQRANGE_MIN, there is no guarantee that ALL devices
    work. And from my experience, users are very happy about any warning,
    which points them to a possible reason, why their new device does not
    work ;)

> > > +
> > > +	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) {
> > > +		if (hsfreq->mbps >= mbps)
> > > +			break;
> > > +		hsfreq_prev = hsfreq;
> > > +	}
> > > +
> > > +	if (hsfreq_prev &&
> > > +	    ((mbps - hsfreq_prev->mbps) <= (hsfreq->mbps - mbps)))
> > 
> > Longer lines are now OK [1] and I think it would add to the readability 
> > here.
> > 
> > > +		hsfreq = hsfreq_prev;
> > > +
> > 
> > How about
> > 
> > static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
> > {
> >     const struct rcsi2_mbps_reg *hsfreq;
> >     const struct rcsi2_mbps_reg *hsfreq_prev = NULL;
> > 
> >     for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) {
> > 	if (hsfreq->mbps >= mbps)
> > 		break;
> > 	hsfreq_prev = hsfreq;
> >     }
> > 
> >     if (!hsfreq->mbps) {
> > 	dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
> > 	return -ERANGE;
> >     }
> > 
> >     if (hsfreq_prev && ((mbps - hsfreq_prev->mbps) <= (hsfreq->mbps - mbps)))
> > 	hsfreq = hsfreq_prev;
> > 
> >     rcsi2_write(priv, PHYPLL_REG, PHYPLL_HSFREQRANGE(hsfreq->reg));
> > 
> >     return 0;
> > }
> > 
> > >  	rcsi2_write(priv, PHYPLL_REG, PHYPLL_HSFREQRANGE(hsfreq->reg));
> > >  
> > >  	return 0;
> 
> Agreed I will do the changes and update.
> > > -- 
> > > 2.7.4
> > > 
> > 
> > 1. https://lkml.org/lkml/2020/5/29/1038
> > 
> > -- 
> > Regards,
> > Niklas Söderlund
> 
> -- 
> Best Regards,
> Suresh Udipi.

-- 
Best Regards,
Michael

Re: [PATCH v6] media: rcar-csi2: Correct the selection of hsfreqrange

[Niklas Söderlund]

Hello,

On 2020-06-12 19:28:06 +0200, Michael Rodin wrote:
> Hi Niklas,
> Hi Suresh,
> 
> On Fri, Jun 12, 2020 at 12:10:51PM +0900, Suresh Udipi wrote:
> > On Wed, Jun 10, 2020 at 03:40:04PM +0200, Niklas Söderlund wrote:
> > > Hi Suresh,
> > > 
> > > Thanks for your persistent work!
> > > 
> > > On 2020-06-08 12:25:03 +0900, Suresh Udipi wrote:
> > > > hsfreqrange should be chosen based on the calculated mbps which
> > > > is closer to the default bit rate  and within the range as per
> > > > table[1]. But current calculation always selects first value which
> > > > is greater than or equal to the calculated mbps which may lead
> > > > to chosing a wrong range in some cases.
> > > > 
> > > > For example for 360 mbps for H3/M3N
> > > > Existing logic selects
> > > > Calculated value 360Mbps : Default 400Mbps Range [368.125 -433.125 mbps]
> > > > 
> > > > This hsfreqrange is out of range.
> > > > 
> > > > The logic is changed to get the default value which is closest to the
> > > > calculated value [1]
> > > > 
> > > > Calculated value 360Mbps : Default 350Mbps  Range [320.625 -380.625 mpbs]
> > > > 
> > > > [1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]
> > > > 
> > > > Please note that According to Renesas in Table 25.9 the range for
> > > > 220 default value is corrected as below
> > > > 
> > > >  |Range (Mbps)     |  Default  Bit rate (Mbps) |
> > > >  -----------------------------------------------
> > > >  | 197.125-244.125 |     220                   |
> > > >  -----------------------------------------------
> > > > 
> > > > Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")
> > > > 
> > > > Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
> > > > Signed-off-by: Kazuyoshi Akiyama <akiyama@nds-osk.co.jp>
> > > > Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
> > > > ---
> > > >  Changes in v2:
> > > >   - Added the boundary check for the maximum bit rate.
> > > > 
> > > >   - Simplified the logic by remmoving range check
> > > >     as only the closest default value covers most
> > > >     of the use cases.
> > > > 
> > > >   - Aligning the commit message based on the above change
> > > > 
> > > > 
> > > >  Changes in v3:
> > > >     - Added max member from struct rcsi2_mbps_reg.
> > > >       mbps varialbe cannot be removed from rcsi2_mbps_reg,
> > > >       since this structure is reused for
> > > >       phtw_mbps_h3_v3h_m3n/phtw_mbps_v3m_e3 where mbps is
> > > >       used.
> > > > 
> > > > 
> > > >    -  Update the walk of the array in rcsi2_set_phypll() so that it finds
> > > >       the first entry where the calculated bit rate is less than the max.
> > > > 
> > > >    - Support lower bit rates less than 80Mbps like 48Mbps
> > > >      (Raspberry pi camera 640x480 connected to Kingfisher)
> > > >      can also be supported by selecting the lowest default bit rate 80Mbps
> > > >      as done before this fix
> > > > 
> > > >    - Alignement of the commit message based on above changes.
> > > > 
> > > >  Changes in v4:
> > > >   -  Remove unncessary braces.
> > > > 
> > > >  Changes in v5:
> > > >    - Removed mbps variable in rcsi2_mbps_reg and aligned all 
> > > >      tables accordingly
> > > > 	 
> > > >  Changes in v6
> > > >    - Renesas correct the range of default value 220Mbps. Now
> > > >      if we select the nearest value to the default value all
> > > > 	 the values are in range. So reverting back to original patch
> > > > 	 
> > > >    - Added warning for values less than Minimum 80Mbps
> > > > 
> > > > 
> > > >  drivers/media/platform/rcar-vin/rcar-csi2.c | 23 ++++++++++++++++++-----
> > > >  1 file changed, 18 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > > index 151e6a9..8c502b7 100644
> > > > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > > > @@ -199,6 +199,8 @@ static const struct rcsi2_mbps_reg phtw_mbps_v3m_e3[] = {
> > > >  /* PHY Frequency Control */
> > > >  #define PHYPLL_REG			0x68
> > > >  #define PHYPLL_HSFREQRANGE(n)		((n) << 16)
> > > > +#define PHYPLL_HSFREQRANGE_MAX		1500
> > > > +#define PHYPLL_HSFREQRANGE_MIN		  80
> > > >  
> > > >  static const struct rcsi2_mbps_reg hsfreqrange_h3_v3h_m3n[] = {
> > > >  	{ .mbps =   80, .reg = 0x00 },
> > > > @@ -431,16 +433,27 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
> > > >  static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
> > > >  {
> > > >  	const struct rcsi2_mbps_reg *hsfreq;
> > > > +	const struct rcsi2_mbps_reg *hsfreq_prev = NULL;
> > > >  
> > > > -	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
> > > > -		if (hsfreq->mbps >= mbps)
> > > > -			break;
> > > > -
> > > > -	if (!hsfreq->mbps) {
> > > > +	if (mbps > PHYPLL_HSFREQRANGE_MAX) {
> > > >  		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
> > > >  		return -ERANGE;
> > > >  	}
> > > >  
> > > > +	if (mbps < PHYPLL_HSFREQRANGE_MIN)
> > > > +		dev_warn(priv->dev, "PHY speed (%u Mbps) less \
> > > > +			 than Min 80Mbps\n", mbps);
> > > 
> > > I would drop this warning.
> > > 
> > 
> >   This was suggested by Michael. Michael is it ok to drop this warning
> >   as it is not available before this changes also. 
> > 
> 
> I strongly disagree. We should keep the warning for the following reasons:
> 
>  1. Renesas explicitly states in the hardware manual tables, that 80Mbps is
>     the lowest supported bit rate (I guess, for a good reason), so using
>     devices with lower bit rates is at our own risk.
>  2. Failing for mbps > PHYPLL_HSFREQRANGE_MAX with an ERANGE error but
>     silently succeeding for mbps < PHYPLL_HSFREQRANGE_MIN does not look
>     consistent. Both values are out of the official hardware specs.
>  3. Although rcar csi2 seems to work with at least 1 device in the range
>     mbps < PHYPLL_HSFREQRANGE_MIN, there is no guarantee that ALL devices
>     work. And from my experience, users are very happy about any warning,
>     which points them to a possible reason, why their new device does not
>     work ;)

You convinced me, lets add a warning then. But please do so in a 
separate patch on-top of this change.

Also if possible I would like to avoid adding a PHYPLL_HSFREQRANGE_MIN 
define and instead get the minimum setting from the data tables. As we 
already require the tables to be sorted for the logic touched in this 
change to work it should be doable to get the minimum freq from the same 
source.

> 
> > > > +
> > > > +	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) {
> > > > +		if (hsfreq->mbps >= mbps)
> > > > +			break;
> > > > +		hsfreq_prev = hsfreq;
> > > > +	}
> > > > +
> > > > +	if (hsfreq_prev &&
> > > > +	    ((mbps - hsfreq_prev->mbps) <= (hsfreq->mbps - mbps)))
> > > 
> > > Longer lines are now OK [1] and I think it would add to the readability 
> > > here.
> > > 
> > > > +		hsfreq = hsfreq_prev;
> > > > +
> > > 
> > > How about
> > > 
> > > static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
> > > {
> > >     const struct rcsi2_mbps_reg *hsfreq;
> > >     const struct rcsi2_mbps_reg *hsfreq_prev = NULL;
> > > 
> > >     for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) {
> > > 	if (hsfreq->mbps >= mbps)
> > > 		break;
> > > 	hsfreq_prev = hsfreq;
> > >     }
> > > 
> > >     if (!hsfreq->mbps) {
> > > 	dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
> > > 	return -ERANGE;
> > >     }
> > > 
> > >     if (hsfreq_prev && ((mbps - hsfreq_prev->mbps) <= (hsfreq->mbps - mbps)))
> > > 	hsfreq = hsfreq_prev;
> > > 
> > >     rcsi2_write(priv, PHYPLL_REG, PHYPLL_HSFREQRANGE(hsfreq->reg));
> > > 
> > >     return 0;
> > > }
> > > 
> > > >  	rcsi2_write(priv, PHYPLL_REG, PHYPLL_HSFREQRANGE(hsfreq->reg));
> > > >  
> > > >  	return 0;
> > 
> > Agreed I will do the changes and update.
> > > > -- 
> > > > 2.7.4
> > > > 
> > > 
> > > 1. https://lkml.org/lkml/2020/5/29/1038
> > > 
> > > -- 
> > > Regards,
> > > Niklas Söderlund
> > 
> > -- 
> > Best Regards,
> > Suresh Udipi.
> 
> -- 
> Best Regards,
> Michael

-- 
Regards,
Niklas Söderlund

[PATCH v7 1/2] media: rcar-csi2: Correct the selection of hsfreqrange

[Suresh Udipi]

hsfreqrange should be chosen based on the calculated mbps which
is closer to the default bit rate  and within the range as per
table[1]. But current calculation always selects first value which
is greater than or equal to the calculated mbps which may lead
to chosing a wrong range in some cases.

For example for 360 mbps for H3/M3N
Existing logic selects
Calculated value 360Mbps : Default 400Mbps Range [368.125 -433.125 mbps]

This hsfreqrange is out of range.

The logic is changed to get the default value which is closest to the
calculated value [1]

Calculated value 360Mbps : Default 350Mbps  Range [320.625 -380.625 mpbs]

[1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]

Please note that According to Renesas in Table 25.9 the range for
220 default value is corrected as below

 |Range (Mbps)     |  Default  Bit rate (Mbps) |
 -----------------------------------------------
 | 197.125-244.125 |     220                   |
 -----------------------------------------------

Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")

Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
Signed-off-by: Kazuyoshi Akiyama <akiyama@nds-osk.co.jp>
Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
---
 Changes in v2:
  - Added the boundary check for the maximum bit rate.

  - Simplified the logic by remmoving range check
    as only the closest default value covers most
    of the use cases.

  - Aligning the commit message based on the above change


 Changes in v3:
    - Added max member from struct rcsi2_mbps_reg.
      mbps varialbe cannot be removed from rcsi2_mbps_reg,
      since this structure is reused for
      phtw_mbps_h3_v3h_m3n/phtw_mbps_v3m_e3 where mbps is
      used.


   -  Update the walk of the array in rcsi2_set_phypll() so that it finds
      the first entry where the calculated bit rate is less than the max.

   - Support lower bit rates less than 80Mbps like 48Mbps
     (Raspberry pi camera 640x480 connected to Kingfisher)
     can also be supported by selecting the lowest default bit rate 80Mbps
     as done before this fix

   - Alignement of the commit message based on above changes.

 Changes in v4:
  -  Remove unncessary braces.

 Changes in v5:
   - Removed mbps variable in rcsi2_mbps_reg and aligned all
     tables accordingly


 Changes in v6
   - Renesas correct the range of default value 220Mbps. Now
     if we select the nearest value to the default value all
         the values are in range. So reverting back to original patch
  
   - Added warning for values less than Minimum 80Mbps
      
Changes in v7
  - Create a seperate commit for the warning message less
    than minimum 80Mbps 
  
  - Reorder the statements to increase readability

 drivers/media/platform/rcar-vin/rcar-csi2.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index 151e6a9..f18dedc 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -431,16 +431,23 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
 static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
 {
 	const struct rcsi2_mbps_reg *hsfreq;
+	const struct rcsi2_mbps_reg *hsfreq_prev = NULL;
 
-	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
+	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) {
 		if (hsfreq->mbps >= mbps)
 			break;
+		hsfreq_prev = hsfreq;
+	}
 
 	if (!hsfreq->mbps) {
 		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
 		return -ERANGE;
 	}
 
+	if (hsfreq_prev &&
+	    ((mbps - hsfreq_prev->mbps) <= (hsfreq->mbps - mbps)))
+		hsfreq = hsfreq_prev;
+
 	rcsi2_write(priv, PHYPLL_REG, PHYPLL_HSFREQRANGE(hsfreq->reg));
 
 	return 0;
-- 
2.7.4

[PATCH v7 2/2] media: rcar-csi2: Add warning for PHY speed less than minimum

[Suresh Udipi]

Add a warning message when the selected PHY speed is less
than supported minimum PHY speed given in the hsfreq table[1].

For raspberry pi camera capture on Kingfisher board with resolution
640x480, the calculated PHY speed is 48 mbps which is less than
the minimum PHY speed 80 Mbps from the table[1]. But in this cases
capture is successful.

[1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]

Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
---
 drivers/media/platform/rcar-vin/rcar-csi2.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index f18dedc..1184527 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -433,6 +433,10 @@ static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
 	const struct rcsi2_mbps_reg *hsfreq;
 	const struct rcsi2_mbps_reg *hsfreq_prev = NULL;
 
+	if (mbps < priv->info->hsfreqrange->mbps)
+		dev_warn(priv->dev, "%u Mbps less than min PHY speed %u Mbps",
+			 mbps, priv->info->hsfreqrange->mbps);
+
 	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) {
 		if (hsfreq->mbps >= mbps)
 			break;
-- 
2.7.4

Re: [PATCH v7 1/2] media: rcar-csi2: Correct the selection of hsfreqrange

[Michael Rodin]

Hi Suresh,

thanks for your work! It looks like your PHTW patch is missing:

"[PATCH] rcar-vin: rcar-csi2: Select the correct PHTW register"

-- 
Best Regards,
Michael

[PATCH v8 1/3] media: rcar-csi2: Correct the selection of hsfreqrange

[Suresh Udipi]

hsfreqrange should be chosen based on the calculated mbps which
is closer to the default bit rate  and within the range as per
table[1]. But current calculation always selects first value which
is greater than or equal to the calculated mbps which may lead
to chosing a wrong range in some cases.

For example for 360 mbps for H3/M3N
Existing logic selects
Calculated value 360Mbps : Default 400Mbps Range [368.125 -433.125 mbps]

This hsfreqrange is out of range.

The logic is changed to get the default value which is closest to the
calculated value [1]

Calculated value 360Mbps : Default 350Mbps  Range [320.625 -380.625 mpbs]

[1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]

Please note that According to Renesas in Table 25.9 the range for
220 default value is corrected as below

 |Range (Mbps)     |  Default  Bit rate (Mbps) |
 -----------------------------------------------
 | 197.125-244.125 |     220                   |
 -----------------------------------------------

Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")

Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
Signed-off-by: Kazuyoshi Akiyama <akiyama@nds-osk.co.jp>
Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
---
 Changes in v2:
  - Added the boundary check for the maximum bit rate.

  - Simplified the logic by remmoving range check
    as only the closest default value covers most
    of the use cases.

  - Aligning the commit message based on the above change


 Changes in v3:
    - Added max member from struct rcsi2_mbps_reg.
      mbps varialbe cannot be removed from rcsi2_mbps_reg,
      since this structure is reused for
      phtw_mbps_h3_v3h_m3n/phtw_mbps_v3m_e3 where mbps is
      used.


   -  Update the walk of the array in rcsi2_set_phypll() so that it finds
      the first entry where the calculated bit rate is less than the max.

   - Support lower bit rates less than 80Mbps like 48Mbps
     (Raspberry pi camera 640x480 connected to Kingfisher)
     can also be supported by selecting the lowest default bit rate 80Mbps
     as done before this fix

   - Alignement of the commit message based on above changes.

 Changes in v4:
  -  Remove unncessary braces.

 Changes in v5:
   - Removed mbps variable in rcsi2_mbps_reg and aligned all
     tables accordingly


 Changes in v6
   - Renesas correct the range of default value 220Mbps. Now
     if we select the nearest value to the default value all
         the values are in range. So reverting back to original patch
  
   - Added warning for values less than Minimum 80Mbps
      
Changes in v7
  - Create a seperate commit for the warning message less
    than minimum 80Mbps 
  
  - Reorder the statements to increase readability
  
Changes in v8
  - Extended the logic of selection of nearest mbps to
    PHTW registers value less than 250Mbps. A new commit
    is added
	
 drivers/media/platform/rcar-vin/rcar-csi2.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index 151e6a9..f18dedc 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -431,16 +431,23 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
 static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
 {
 	const struct rcsi2_mbps_reg *hsfreq;
+	const struct rcsi2_mbps_reg *hsfreq_prev = NULL;
 
-	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
+	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) {
 		if (hsfreq->mbps >= mbps)
 			break;
+		hsfreq_prev = hsfreq;
+	}
 
 	if (!hsfreq->mbps) {
 		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
 		return -ERANGE;
 	}
 
+	if (hsfreq_prev &&
+	    ((mbps - hsfreq_prev->mbps) <= (hsfreq->mbps - mbps)))
+		hsfreq = hsfreq_prev;
+
 	rcsi2_write(priv, PHYPLL_REG, PHYPLL_HSFREQRANGE(hsfreq->reg));
 
 	return 0;
-- 
2.7.4

[PATCH v8 2/3] media: rcar-csi2: Add warning for PHY speed less than minimum

[Suresh Udipi]

Add a warning message when the selected PHY speed is less
than supported minimum PHY speed given in the hsfreq table[1].

For raspberry pi camera capture on Kingfisher board with resolution
640x480, the calculated PHY speed is 48 mbps which is less than
the minimum PHY speed 80 Mbps from the table[1]. But in this cases
capture is successful.

[1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]

Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
---
 drivers/media/platform/rcar-vin/rcar-csi2.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index f18dedc..1184527 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -433,6 +433,10 @@ static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
 	const struct rcsi2_mbps_reg *hsfreq;
 	const struct rcsi2_mbps_reg *hsfreq_prev = NULL;
 
+	if (mbps < priv->info->hsfreqrange->mbps)
+		dev_warn(priv->dev, "%u Mbps less than min PHY speed %u Mbps",
+			 mbps, priv->info->hsfreqrange->mbps);
+
 	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) {
 		if (hsfreq->mbps >= mbps)
 			break;
-- 
2.7.4

Re: [PATCH v8 2/3] media: rcar-csi2: Add warning for PHY speed less than minimum

[Niklas Söderlund]

Hi Suresh,

Thanks for your patch.

On 2020-06-18 19:34:31 +0900, Suresh Udipi wrote:
> Add a warning message when the selected PHY speed is less
> than supported minimum PHY speed given in the hsfreq table[1].
> 
> For raspberry pi camera capture on Kingfisher board with resolution
> 640x480, the calculated PHY speed is 48 mbps which is less than
> the minimum PHY speed 80 Mbps from the table[1]. But in this cases
> capture is successful.
> 
> [1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]
> 
> Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
> Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index f18dedc..1184527 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -433,6 +433,10 @@ static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
>  	const struct rcsi2_mbps_reg *hsfreq;
>  	const struct rcsi2_mbps_reg *hsfreq_prev = NULL;
>  
> +	if (mbps < priv->info->hsfreqrange->mbps)
> +		dev_warn(priv->dev, "%u Mbps less than min PHY speed %u Mbps",
> +			 mbps, priv->info->hsfreqrange->mbps);
> +
>  	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) {
>  		if (hsfreq->mbps >= mbps)
>  			break;
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund

[PATCH v8 3/3] media: rcar-csi2: Optimize the selection PHTW register

[Suresh Udipi]

PHTW register is selected based on default bit rate from Table[1].
for the bit rates less than or equal to 250. Currently first
value of default bit rate which is greater than or equal to
the caculated mbps is selected.This selection can be further
improved by selecting the default bit rate which is nearest to
the calculated value.

[1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.12]

Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")

Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
---
 drivers/media/platform/rcar-vin/rcar-csi2.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index 1184527..d7bf59f 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -917,10 +917,18 @@ static int rcsi2_phtw_write_mbps(struct rcar_csi2 *priv, unsigned int mbps,
 				 const struct rcsi2_mbps_reg *values, u16 code)
 {
 	const struct rcsi2_mbps_reg *value;
+	const struct rcsi2_mbps_reg *prev_value = NULL;
 
-	for (value = values; value->mbps; value++)
+	for (value = values; value->mbps; value++) {
 		if (value->mbps >= mbps)
 			break;
+		prev_value = value;
+	}
+
+	if (prev_value &&
+	    ((mbps - prev_value->mbps) <= (value->mbps - mbps)))
+		value = prev_value;
+
 
 	if (!value->mbps) {
 		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
-- 
2.7.4

Re: [PATCH v8 3/3] media: rcar-csi2: Optimize the selection PHTW register

[Niklas Söderlund]

Hi Suresh,

Thanks for your work.

On 2020-06-18 19:34:32 +0900, Suresh Udipi wrote:
> PHTW register is selected based on default bit rate from Table[1].
> for the bit rates less than or equal to 250. Currently first
> value of default bit rate which is greater than or equal to
> the caculated mbps is selected.This selection can be further

Missing space 'selected.This'.

> improved by selecting the default bit rate which is nearest to
> the calculated value.
> 
> [1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.12]
> 
> Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")

Nit-pick: No need for a blank line between fixes and SoB.

> 
> Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
> Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index 1184527..d7bf59f 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -917,10 +917,18 @@ static int rcsi2_phtw_write_mbps(struct rcar_csi2 *priv, unsigned int mbps,
>  				 const struct rcsi2_mbps_reg *values, u16 code)
>  {
>  	const struct rcsi2_mbps_reg *value;
> +	const struct rcsi2_mbps_reg *prev_value = NULL;
>  
> -	for (value = values; value->mbps; value++)
> +	for (value = values; value->mbps; value++) {
>  		if (value->mbps >= mbps)
>  			break;
> +		prev_value = value;
> +	}
> +
> +	if (prev_value &&
> +	    ((mbps - prev_value->mbps) <= (value->mbps - mbps)))
> +		value = prev_value;
> +

One to many blank lines. With this and the commit message fixed,

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Nice work, thanks again for being persistent with this!

>  
>  	if (!value->mbps) {
>  		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund

[PATCH v9 1/3] media: rcar-csi2: Correct the selection of hsfreqrange

[Suresh Udipi]

hsfreqrange should be chosen based on the calculated mbps which
is closer to the default bit rate  and within the range as per
table[1]. But current calculation always selects first value which
is greater than or equal to the calculated mbps which may lead
to chosing a wrong range in some cases.

For example for 360 mbps for H3/M3N
Existing logic selects
Calculated value 360Mbps : Default 400Mbps Range [368.125 -433.125 mbps]

This hsfreqrange is out of range.

The logic is changed to get the default value which is closest to the
calculated value [1]

Calculated value 360Mbps : Default 350Mbps  Range [320.625 -380.625 mpbs]

[1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]

Please note that According to Renesas in Table 25.9 the range for
220 default value is corrected as below

 |Range (Mbps)     |  Default  Bit rate (Mbps) |
 -----------------------------------------------
 | 197.125-244.125 |     220                   |
 -----------------------------------------------

Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")
Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
Signed-off-by: Kazuyoshi Akiyama <akiyama@nds-osk.co.jp>
Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
Changes in v2:
  - Added the boundary check for the maximum bit rate.
  - Simplified the logic by remmoving range check
    as only the closest default value covers most
    of the use cases.
  - Aligning the commit message based on the above change
 
Changes in v3:
  - Added max member from struct rcsi2_mbps_reg.
    mbps varialbe cannot be removed from rcsi2_mbps_reg,
    since this structure is reused for
    phtw_mbps_h3_v3h_m3n/phtw_mbps_v3m_e3 where mbps is
    used.

  - Update the walk of the array in rcsi2_set_phypll() so that it finds
    the first entry where the calculated bit rate is less than the max.
 
  - Support lower bit rates less than 80Mbps like 48Mbps
    (Raspberry pi camera 640x480 connected to Kingfisher)
    can also be supported by selecting the lowest default bit rate 80Mbps
    as done before this fix

  - Alignement of the commit message based on above changes.
 
Changes in v4:
  -  Remove unncessary braces.

Changes in v5:
   - Removed mbps variable in rcsi2_mbps_reg and aligned all
     tables accordingly

Changes in v6
   - Renesas correct the range of default value 220Mbps. Now
    if we select the nearest value to the default value all
    the values are in range. So reverting back to original patch
  
   - Added warning for values less than Minimum 80Mbps
       
Changes in v7
   - Create a seperate commit for the warning message less
     than minimum 80Mbps
  
   - Reorder the statements to increase readability
   
Changes in v8
   - Extended the logic of selection of nearest mbps to
     PHTW registers value less than 250Mbps. A new commit
     is added
 	
Changes in v9
  - Added Reviewed-by.
  - Removed  unncessary space in commit msg and commits

 drivers/media/platform/rcar-vin/rcar-csi2.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index 151e6a9..f18dedc 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -431,16 +431,23 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
 static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
 {
 	const struct rcsi2_mbps_reg *hsfreq;
+	const struct rcsi2_mbps_reg *hsfreq_prev = NULL;
 
-	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
+	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) {
 		if (hsfreq->mbps >= mbps)
 			break;
+		hsfreq_prev = hsfreq;
+	}
 
 	if (!hsfreq->mbps) {
 		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
 		return -ERANGE;
 	}
 
+	if (hsfreq_prev &&
+	    ((mbps - hsfreq_prev->mbps) <= (hsfreq->mbps - mbps)))
+		hsfreq = hsfreq_prev;
+
 	rcsi2_write(priv, PHYPLL_REG, PHYPLL_HSFREQRANGE(hsfreq->reg));
 
 	return 0;
-- 
2.7.4

[PATCH v9 2/3] media: rcar-csi2: Add warning for PHY speed less than minimum

[Suresh Udipi]

Add a warning message when the selected PHY speed is less
than supported minimum PHY speed given in the hsfreq table[1].

For raspberry pi camera capture on Kingfisher board with resolution
640x480, the calculated PHY speed is 48 mbps which is less than
the minimum PHY speed 80 Mbps from the table[1]. But in this cases
capture is successful.

[1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]

Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-csi2.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index f18dedc..1184527 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -433,6 +433,10 @@ static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
 	const struct rcsi2_mbps_reg *hsfreq;
 	const struct rcsi2_mbps_reg *hsfreq_prev = NULL;
 
+	if (mbps < priv->info->hsfreqrange->mbps)
+		dev_warn(priv->dev, "%u Mbps less than min PHY speed %u Mbps",
+			 mbps, priv->info->hsfreqrange->mbps);
+
 	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) {
 		if (hsfreq->mbps >= mbps)
 			break;
-- 
2.7.4

[PATCH v9 3/3] media: rcar-csi2: Optimize the selection PHTW register

[Suresh Udipi]

PHTW register is selected based on default bit rate from Table[1].
for the bit rates less than or equal to 250. Currently first
value of default bit rate which is greater than or equal to
the caculated mbps is selected. This selection can be further
improved by selecting the default bit rate which is nearest to
the calculated value.

[1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.12]

Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")
Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-csi2.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index 1184527..3efa1cb 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -917,10 +917,17 @@ static int rcsi2_phtw_write_mbps(struct rcar_csi2 *priv, unsigned int mbps,
 				 const struct rcsi2_mbps_reg *values, u16 code)
 {
 	const struct rcsi2_mbps_reg *value;
+	const struct rcsi2_mbps_reg *prev_value = NULL;
 
-	for (value = values; value->mbps; value++)
+	for (value = values; value->mbps; value++) {
 		if (value->mbps >= mbps)
 			break;
+		prev_value = value;
+	}
+
+	if (prev_value &&
+	    ((mbps - prev_value->mbps) <= (value->mbps - mbps)))
+		value = prev_value;
 
 	if (!value->mbps) {
 		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
-- 
2.7.4

Re: [PATCH v8 1/3] media: rcar-csi2: Correct the selection of hsfreqrange

[Niklas Söderlund]

Hi Suresh,

Thanks for your work.

On 2020-06-18 19:34:30 +0900, Suresh Udipi wrote:
> hsfreqrange should be chosen based on the calculated mbps which
> is closer to the default bit rate  and within the range as per
> table[1]. But current calculation always selects first value which
> is greater than or equal to the calculated mbps which may lead
> to chosing a wrong range in some cases.
> 
> For example for 360 mbps for H3/M3N
> Existing logic selects
> Calculated value 360Mbps : Default 400Mbps Range [368.125 -433.125 mbps]
> 
> This hsfreqrange is out of range.
> 
> The logic is changed to get the default value which is closest to the
> calculated value [1]
> 
> Calculated value 360Mbps : Default 350Mbps  Range [320.625 -380.625 mpbs]
> 
> [1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.9]
> 
> Please note that According to Renesas in Table 25.9 the range for
> 220 default value is corrected as below
> 
>  |Range (Mbps)     |  Default  Bit rate (Mbps) |
>  -----------------------------------------------
>  | 197.125-244.125 |     220                   |
>  -----------------------------------------------
> 
> Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")
> 
> Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
> Signed-off-by: Kazuyoshi Akiyama <akiyama@nds-osk.co.jp>
> Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
>  Changes in v2:
>   - Added the boundary check for the maximum bit rate.
> 
>   - Simplified the logic by remmoving range check
>     as only the closest default value covers most
>     of the use cases.
> 
>   - Aligning the commit message based on the above change
> 
> 
>  Changes in v3:
>     - Added max member from struct rcsi2_mbps_reg.
>       mbps varialbe cannot be removed from rcsi2_mbps_reg,
>       since this structure is reused for
>       phtw_mbps_h3_v3h_m3n/phtw_mbps_v3m_e3 where mbps is
>       used.
> 
> 
>    -  Update the walk of the array in rcsi2_set_phypll() so that it finds
>       the first entry where the calculated bit rate is less than the max.
> 
>    - Support lower bit rates less than 80Mbps like 48Mbps
>      (Raspberry pi camera 640x480 connected to Kingfisher)
>      can also be supported by selecting the lowest default bit rate 80Mbps
>      as done before this fix
> 
>    - Alignement of the commit message based on above changes.
> 
>  Changes in v4:
>   -  Remove unncessary braces.
> 
>  Changes in v5:
>    - Removed mbps variable in rcsi2_mbps_reg and aligned all
>      tables accordingly
> 
> 
>  Changes in v6
>    - Renesas correct the range of default value 220Mbps. Now
>      if we select the nearest value to the default value all
>          the values are in range. So reverting back to original patch
>   
>    - Added warning for values less than Minimum 80Mbps
>       
> Changes in v7
>   - Create a seperate commit for the warning message less
>     than minimum 80Mbps 
>   
>   - Reorder the statements to increase readability
>   
> Changes in v8
>   - Extended the logic of selection of nearest mbps to
>     PHTW registers value less than 250Mbps. A new commit
>     is added
> 	
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index 151e6a9..f18dedc 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -431,16 +431,23 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
>  static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
>  {
>  	const struct rcsi2_mbps_reg *hsfreq;
> +	const struct rcsi2_mbps_reg *hsfreq_prev = NULL;
>  
> -	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
> +	for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++) {
>  		if (hsfreq->mbps >= mbps)
>  			break;
> +		hsfreq_prev = hsfreq;
> +	}
>  
>  	if (!hsfreq->mbps) {
>  		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
>  		return -ERANGE;
>  	}
>  
> +	if (hsfreq_prev &&
> +	    ((mbps - hsfreq_prev->mbps) <= (hsfreq->mbps - mbps)))
> +		hsfreq = hsfreq_prev;
> +
>  	rcsi2_write(priv, PHYPLL_REG, PHYPLL_HSFREQRANGE(hsfreq->reg));
>  
>  	return 0;
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund

[PATCH] rcar-vin: rcar-csi2: Select the correct PHTW register

[Michael Rodin]

From: Suresh Udipi <sudipi@jp.adit-jv.com>

PHTW register is selected based on default bit rate from Table[1].
for the bit rates less than or equal to 250. Currently first
value of default bit rate which is greater than or equal to
the caculated mbps is selected.This selection can be further
improved by selecting the default bit rate which is nearest to
the calculated value.

[1] specs r19uh0105ej0200-r-car-3rd-generation.pdf [Table 25.12]

Fixes: ADIT v4.14 commit 9e568b895ee0 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")

Signed-off-by: Suresh Udipi <sudipi@jp.adit-jv.com>
Signed-off-by: Michael Rodin <mrodin@de.adit-jv.com>
---
 drivers/media/platform/rcar-vin/rcar-csi2.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
index 687baee..115b0af 100644
--- a/drivers/media/platform/rcar-vin/rcar-csi2.c
+++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
@@ -886,10 +886,18 @@ static int rcsi2_phtw_write_mbps(struct rcar_csi2 *priv, unsigned int mbps,
 				 const struct rcsi2_mbps_reg *values, u16 code)
 {
 	const struct rcsi2_mbps_reg *value;
+	const struct rcsi2_mbps_reg *prev_value = NULL;
 
-	for (value = values; value->mbps; value++)
+	for (value = values; value->mbps; value++) {
 		if (value->mbps >= mbps)
 			break;
+		prev_value = value;
+	}
+
+	if (prev_value &&
+	    ((mbps - prev_value->mbps) <= (value->mbps - mbps)))
+		value = prev_value;
+
 
 	if (!value->mbps) {
 		dev_err(priv->dev, "Unsupported PHY speed (%u Mbps)", mbps);
-- 
2.7.4