* [PATCH] [RFC] media: rcar-csi2: Corrects the selection of hsfreqrange @ 2020-02-20 4:49 Suresh Udipi 2020-03-04 19:46 ` Niklas Söderlund 0 siblings, 1 reply; 34+ messages in thread From: Suresh Udipi @ 2020-02-20 4:49 UTC (permalink / raw) To: Niklas Söderlund, Laurent Pinchart, Hans Verkuil, Jacopo Mondi Cc: Michael Rodin, Eugen Friedrich, akiyama, Eugeniu Rosca, linux-media, linux-renesas-soc, linux-kernel, securitycheck 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 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH] [RFC] media: rcar-csi2: Corrects the selection of hsfreqrange 2020-02-20 4:49 [PATCH] [RFC] media: rcar-csi2: Corrects the selection of hsfreqrange Suresh Udipi @ 2020-03-04 19:46 ` Niklas Söderlund 2020-03-16 13:02 ` Niklas Söderlund 0 siblings, 1 reply; 34+ messages in thread From: Niklas Söderlund @ 2020-03-04 19:46 UTC (permalink / raw) To: Suresh Udipi Cc: Laurent Pinchart, Hans Verkuil, Jacopo Mondi, Michael Rodin, Eugen Friedrich, akiyama, Eugeniu Rosca, linux-media, linux-renesas-soc, linux-kernel, securitycheck 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH] [RFC] media: rcar-csi2: Corrects the selection of hsfreqrange 2020-03-04 19:46 ` Niklas Söderlund @ 2020-03-16 13:02 ` Niklas Söderlund 2020-03-17 7:08 ` [PATCH v2] [RFC] rcar-vin: rcar-csi2: Correct " Suresh Udipi 0 siblings, 1 reply; 34+ messages in thread From: Niklas Söderlund @ 2020-03-16 13:02 UTC (permalink / raw) To: Suresh Udipi Cc: Laurent Pinchart, Hans Verkuil, Jacopo Mondi, Michael Rodin, Eugen Friedrich, akiyama, Eugeniu Rosca, linux-media, linux-renesas-soc, linux-kernel, securitycheck 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v2] [RFC] rcar-vin: rcar-csi2: Correct the selection of hsfreqrange 2020-03-16 13:02 ` Niklas Söderlund @ 2020-03-17 7:08 ` Suresh Udipi 2020-03-17 10:37 ` Niklas Söderlund 0 siblings, 1 reply; 34+ messages in thread From: Suresh Udipi @ 2020-03-17 7:08 UTC (permalink / raw) To: niklas.soderlund Cc: akiyama, efriedrich, erosca, hverkuil-cisco, jacopo+renesas, laurent.pinchart, linux-kernel, linux-media, linux-renesas-soc, mrodin, securitycheck, sudipi 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 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v2] [RFC] rcar-vin: rcar-csi2: Correct the selection of hsfreqrange 2020-03-17 7:08 ` [PATCH v2] [RFC] rcar-vin: rcar-csi2: Correct " Suresh Udipi @ 2020-03-17 10:37 ` Niklas Söderlund 2020-03-18 9:00 ` Suresh Udipi 0 siblings, 1 reply; 34+ messages in thread From: Niklas Söderlund @ 2020-03-17 10:37 UTC (permalink / raw) To: Suresh Udipi Cc: akiyama, efriedrich, erosca, hverkuil-cisco, jacopo+renesas, laurent.pinchart, linux-kernel, linux-media, linux-renesas-soc, mrodin, securitycheck 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v2] [RFC] rcar-vin: rcar-csi2: Correct the selection of hsfreqrange 2020-03-17 10:37 ` Niklas Söderlund @ 2020-03-18 9:00 ` Suresh Udipi 2020-03-18 10:19 ` Niklas Söderlund 0 siblings, 1 reply; 34+ messages in thread From: Suresh Udipi @ 2020-03-18 9:00 UTC (permalink / raw) To: Niklas Söderlund Cc: Suresh Udipi, akiyama, efriedrich, erosca, hverkuil-cisco, jacopo+renesas, laurent.pinchart, linux-kernel, linux-media, linux-renesas-soc, mrodin, securitycheck 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. ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v2] [RFC] rcar-vin: rcar-csi2: Correct the selection of hsfreqrange 2020-03-18 9:00 ` Suresh Udipi @ 2020-03-18 10:19 ` Niklas Söderlund 2020-03-20 8:00 ` [PATCH v3] media: " Suresh Udipi 0 siblings, 1 reply; 34+ messages in thread From: Niklas Söderlund @ 2020-03-18 10:19 UTC (permalink / raw) To: Suresh Udipi Cc: akiyama, efriedrich, erosca, hverkuil-cisco, jacopo+renesas, laurent.pinchart, linux-kernel, linux-media, linux-renesas-soc, mrodin, securitycheck 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v3] media: rcar-csi2: Correct the selection of hsfreqrange 2020-03-18 10:19 ` Niklas Söderlund @ 2020-03-20 8:00 ` Suresh Udipi 2020-03-23 4:48 ` [PATCH v4] " Suresh Udipi 0 siblings, 1 reply; 34+ messages in thread From: Suresh Udipi @ 2020-03-20 8:00 UTC (permalink / raw) To: niklas.soderlund Cc: akiyama, efriedrich, erosca, hverkuil-cisco, jacopo+renesas, laurent.pinchart, linux-kernel, linux-media, linux-renesas-soc, mrodin, securitycheck, sudipi 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 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v4] media: rcar-csi2: Correct the selection of hsfreqrange 2020-03-20 8:00 ` [PATCH v3] media: " Suresh Udipi @ 2020-03-23 4:48 ` Suresh Udipi 2020-04-14 11:56 ` Niklas Söderlund 0 siblings, 1 reply; 34+ messages in thread From: Suresh Udipi @ 2020-03-23 4:48 UTC (permalink / raw) To: niklas.soderlund Cc: akiyama, efriedrich, erosca, hverkuil-cisco, jacopo+renesas, laurent.pinchart, linux-kernel, linux-media, linux-renesas-soc, mrodin, sudipi, securitycheck 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 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v4] media: rcar-csi2: Correct the selection of hsfreqrange 2020-03-23 4:48 ` [PATCH v4] " Suresh Udipi @ 2020-04-14 11:56 ` Niklas Söderlund 2020-04-30 6:03 ` [PATCH v5] " Suresh Udipi 0 siblings, 1 reply; 34+ messages in thread From: Niklas Söderlund @ 2020-04-14 11:56 UTC (permalink / raw) To: Suresh Udipi Cc: akiyama, efriedrich, erosca, hverkuil-cisco, jacopo+renesas, laurent.pinchart, linux-kernel, linux-media, linux-renesas-soc, mrodin, securitycheck 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v5] media: rcar-csi2: Correct the selection of hsfreqrange 2020-04-14 11:56 ` Niklas Söderlund @ 2020-04-30 6:03 ` Suresh Udipi 2020-05-04 15:01 ` Michael Rodin 0 siblings, 1 reply; 34+ messages in thread From: Suresh Udipi @ 2020-04-30 6:03 UTC (permalink / raw) To: niklas.soderlund Cc: akiyama, efriedrich, erosca, hverkuil-cisco, jacopo+renesas, laurent.pinchart, linux-kernel, linux-media, linux-renesas-soc, mrodin, securitycheck, sudipi 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 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v5] media: rcar-csi2: Correct the selection of hsfreqrange 2020-04-30 6:03 ` [PATCH v5] " Suresh Udipi @ 2020-05-04 15:01 ` Michael Rodin 2020-05-12 21:30 ` Niklas Söderlund 0 siblings, 1 reply; 34+ messages in thread From: Michael Rodin @ 2020-05-04 15:01 UTC (permalink / raw) To: Suresh Udipi Cc: niklas.soderlund, akiyama, efriedrich, erosca, hverkuil-cisco, jacopo+renesas, laurent.pinchart, linux-kernel, linux-media, linux-renesas-soc, mrodin, securitycheck 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v5] media: rcar-csi2: Correct the selection of hsfreqrange 2020-05-04 15:01 ` Michael Rodin @ 2020-05-12 21:30 ` Niklas Söderlund 2020-05-27 15:47 ` Michael Rodin ` (2 more replies) 0 siblings, 3 replies; 34+ messages in thread From: Niklas Söderlund @ 2020-05-12 21:30 UTC (permalink / raw) To: Michael Rodin Cc: Suresh Udipi, akiyama, efriedrich, erosca, hverkuil-cisco, jacopo+renesas, laurent.pinchart, linux-kernel, linux-media, linux-renesas-soc, securitycheck 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v5] media: rcar-csi2: Correct the selection of hsfreqrange 2020-05-12 21:30 ` Niklas Söderlund @ 2020-05-27 15:47 ` Michael Rodin 2020-05-27 16:16 ` [PATCH] rcar-vin: " Michael Rodin 2020-05-27 16:17 ` [PATCH] rcar-vin: rcar-csi2: Select the correct PHTW register Michael Rodin 2 siblings, 0 replies; 34+ messages in thread From: Michael Rodin @ 2020-05-27 15:47 UTC (permalink / raw) To: Niklas Söderlund Cc: Michael Rodin, Suresh Udipi, akiyama, efriedrich, erosca, hverkuil-cisco, jacopo+renesas, laurent.pinchart, linux-kernel, linux-media, linux-renesas-soc, securitycheck 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH] rcar-vin: rcar-csi2: Correct the selection of hsfreqrange 2020-05-12 21:30 ` Niklas Söderlund 2020-05-27 15:47 ` Michael Rodin @ 2020-05-27 16:16 ` Michael Rodin 2020-06-05 18:44 ` Michael Rodin 2020-05-27 16:17 ` [PATCH] rcar-vin: rcar-csi2: Select the correct PHTW register Michael Rodin 2 siblings, 1 reply; 34+ messages in thread From: Michael Rodin @ 2020-05-27 16:16 UTC (permalink / raw) To: Niklas Söderlund, Mauro Carvalho Chehab, linux-media, linux-renesas-soc, linux-kernel Cc: Michael Rodin, michael, efriedrich, erosca, Suresh Udipi 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 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH] rcar-vin: rcar-csi2: Correct the selection of hsfreqrange 2020-05-27 16:16 ` [PATCH] rcar-vin: " Michael Rodin @ 2020-06-05 18:44 ` Michael Rodin 2020-06-08 3:25 ` [PATCH v6] media: " Suresh Udipi 0 siblings, 1 reply; 34+ messages in thread From: Michael Rodin @ 2020-06-05 18:44 UTC (permalink / raw) To: Niklas Söderlund, Mauro Carvalho Chehab, linux-media, linux-renesas-soc, linux-kernel Cc: Michael Rodin, michael, efriedrich, erosca, Suresh Udipi 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v6] media: rcar-csi2: Correct the selection of hsfreqrange 2020-06-05 18:44 ` Michael Rodin @ 2020-06-08 3:25 ` Suresh Udipi 2020-06-10 13:40 ` Niklas Söderlund 0 siblings, 1 reply; 34+ messages in thread From: Suresh Udipi @ 2020-06-08 3:25 UTC (permalink / raw) To: mrodin Cc: efriedrich, erosca, linux-kernel, linux-media, linux-renesas-soc, mchehab, michael, niklas.soderlund, sudipi, securitycheck, Kazuyoshi Akiyama 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 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v6] media: rcar-csi2: Correct the selection of hsfreqrange 2020-06-08 3:25 ` [PATCH v6] media: " Suresh Udipi @ 2020-06-10 13:40 ` Niklas Söderlund 2020-06-12 3:10 ` Suresh Udipi 0 siblings, 1 reply; 34+ messages in thread From: Niklas Söderlund @ 2020-06-10 13:40 UTC (permalink / raw) To: Suresh Udipi Cc: mrodin, efriedrich, erosca, linux-kernel, linux-media, linux-renesas-soc, mchehab, michael, securitycheck, Kazuyoshi Akiyama 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v6] media: rcar-csi2: Correct the selection of hsfreqrange 2020-06-10 13:40 ` Niklas Söderlund @ 2020-06-12 3:10 ` Suresh Udipi 2020-06-12 17:28 ` Michael Rodin 0 siblings, 1 reply; 34+ messages in thread From: Suresh Udipi @ 2020-06-12 3:10 UTC (permalink / raw) To: Niklas Söderlund Cc: Suresh Udipi, mrodin, efriedrich, erosca, linux-kernel, linux-media, linux-renesas-soc, mchehab, michael, securitycheck, Kazuyoshi Akiyama 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. ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v6] media: rcar-csi2: Correct the selection of hsfreqrange 2020-06-12 3:10 ` Suresh Udipi @ 2020-06-12 17:28 ` Michael Rodin 2020-06-15 14:11 ` Niklas Söderlund 0 siblings, 1 reply; 34+ messages in thread From: Michael Rodin @ 2020-06-12 17:28 UTC (permalink / raw) To: Suresh Udipi Cc: Niklas Söderlund, mrodin, efriedrich, erosca, linux-kernel, linux-media, linux-renesas-soc, mchehab, michael, securitycheck, Kazuyoshi Akiyama 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v6] media: rcar-csi2: Correct the selection of hsfreqrange 2020-06-12 17:28 ` Michael Rodin @ 2020-06-15 14:11 ` Niklas Söderlund 2020-06-17 4:57 ` [PATCH v7 1/2] " Suresh Udipi 0 siblings, 1 reply; 34+ messages in thread From: Niklas Söderlund @ 2020-06-15 14:11 UTC (permalink / raw) To: Michael Rodin Cc: Suresh Udipi, efriedrich, erosca, linux-kernel, linux-media, linux-renesas-soc, mchehab, michael, securitycheck, Kazuyoshi Akiyama 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v7 1/2] media: rcar-csi2: Correct the selection of hsfreqrange 2020-06-15 14:11 ` Niklas Söderlund @ 2020-06-17 4:57 ` Suresh Udipi 2020-06-17 4:57 ` [PATCH v7 2/2] media: rcar-csi2: Add warning for PHY speed less than minimum Suresh Udipi 2020-06-17 13:37 ` [PATCH v7 1/2] media: rcar-csi2: Correct the selection of hsfreqrange Michael Rodin 0 siblings, 2 replies; 34+ messages in thread From: Suresh Udipi @ 2020-06-17 4:57 UTC (permalink / raw) To: niklas.soderlund Cc: akiyama, efriedrich, erosca, linux-kernel, linux-media, linux-renesas-soc, mchehab, michael, mrodin, securitycheck, sudipi 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 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v7 2/2] media: rcar-csi2: Add warning for PHY speed less than minimum 2020-06-17 4:57 ` [PATCH v7 1/2] " Suresh Udipi @ 2020-06-17 4:57 ` Suresh Udipi 2020-06-17 13:37 ` [PATCH v7 1/2] media: rcar-csi2: Correct the selection of hsfreqrange Michael Rodin 1 sibling, 0 replies; 34+ messages in thread From: Suresh Udipi @ 2020-06-17 4:57 UTC (permalink / raw) To: niklas.soderlund Cc: akiyama, efriedrich, erosca, linux-kernel, linux-media, linux-renesas-soc, mchehab, michael, mrodin, securitycheck, sudipi 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 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v7 1/2] media: rcar-csi2: Correct the selection of hsfreqrange 2020-06-17 4:57 ` [PATCH v7 1/2] " Suresh Udipi 2020-06-17 4:57 ` [PATCH v7 2/2] media: rcar-csi2: Add warning for PHY speed less than minimum Suresh Udipi @ 2020-06-17 13:37 ` Michael Rodin 2020-06-18 10:34 ` [PATCH v8 1/3] " Suresh Udipi 1 sibling, 1 reply; 34+ messages in thread From: Michael Rodin @ 2020-06-17 13:37 UTC (permalink / raw) To: Suresh Udipi Cc: niklas.soderlund, akiyama, efriedrich, erosca, linux-kernel, linux-media, linux-renesas-soc, mchehab, michael, mrodin, securitycheck 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v8 1/3] media: rcar-csi2: Correct the selection of hsfreqrange 2020-06-17 13:37 ` [PATCH v7 1/2] media: rcar-csi2: Correct the selection of hsfreqrange Michael Rodin @ 2020-06-18 10:34 ` Suresh Udipi 2020-06-18 10:34 ` [PATCH v8 2/3] media: rcar-csi2: Add warning for PHY speed less than minimum Suresh Udipi ` (2 more replies) 0 siblings, 3 replies; 34+ messages in thread From: Suresh Udipi @ 2020-06-18 10:34 UTC (permalink / raw) To: mrodin Cc: akiyama, efriedrich, erosca, linux-kernel, linux-media, linux-renesas-soc, mchehab, michael, niklas.soderlund, securitycheck, sudipi 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 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v8 2/3] media: rcar-csi2: Add warning for PHY speed less than minimum 2020-06-18 10:34 ` [PATCH v8 1/3] " Suresh Udipi @ 2020-06-18 10:34 ` Suresh Udipi 2020-06-30 22:05 ` Niklas Söderlund 2020-06-18 10:34 ` [PATCH v8 3/3] media: rcar-csi2: Optimize the selection PHTW register Suresh Udipi 2020-06-30 22:03 ` [PATCH v8 1/3] media: rcar-csi2: Correct the selection of hsfreqrange Niklas Söderlund 2 siblings, 1 reply; 34+ messages in thread From: Suresh Udipi @ 2020-06-18 10:34 UTC (permalink / raw) To: mrodin Cc: akiyama, efriedrich, erosca, linux-kernel, linux-media, linux-renesas-soc, mchehab, michael, niklas.soderlund, securitycheck, sudipi 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 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v8 2/3] media: rcar-csi2: Add warning for PHY speed less than minimum 2020-06-18 10:34 ` [PATCH v8 2/3] media: rcar-csi2: Add warning for PHY speed less than minimum Suresh Udipi @ 2020-06-30 22:05 ` Niklas Söderlund 0 siblings, 0 replies; 34+ messages in thread From: Niklas Söderlund @ 2020-06-30 22:05 UTC (permalink / raw) To: Suresh Udipi Cc: mrodin, akiyama, efriedrich, erosca, linux-kernel, linux-media, linux-renesas-soc, mchehab, michael, securitycheck 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v8 3/3] media: rcar-csi2: Optimize the selection PHTW register 2020-06-18 10:34 ` [PATCH v8 1/3] " Suresh Udipi 2020-06-18 10:34 ` [PATCH v8 2/3] media: rcar-csi2: Add warning for PHY speed less than minimum Suresh Udipi @ 2020-06-18 10:34 ` Suresh Udipi 2020-06-30 22:08 ` Niklas Söderlund 2020-06-30 22:03 ` [PATCH v8 1/3] media: rcar-csi2: Correct the selection of hsfreqrange Niklas Söderlund 2 siblings, 1 reply; 34+ messages in thread From: Suresh Udipi @ 2020-06-18 10:34 UTC (permalink / raw) To: mrodin Cc: akiyama, efriedrich, erosca, linux-kernel, linux-media, linux-renesas-soc, mchehab, michael, niklas.soderlund, securitycheck, sudipi 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 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v8 3/3] media: rcar-csi2: Optimize the selection PHTW register 2020-06-18 10:34 ` [PATCH v8 3/3] media: rcar-csi2: Optimize the selection PHTW register Suresh Udipi @ 2020-06-30 22:08 ` Niklas Söderlund 2020-07-01 9:52 ` [PATCH v9 1/3] media: rcar-csi2: Correct the selection of hsfreqrange Suresh Udipi 0 siblings, 1 reply; 34+ messages in thread From: Niklas Söderlund @ 2020-06-30 22:08 UTC (permalink / raw) To: Suresh Udipi Cc: mrodin, akiyama, efriedrich, erosca, linux-kernel, linux-media, linux-renesas-soc, mchehab, michael, securitycheck 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v9 1/3] media: rcar-csi2: Correct the selection of hsfreqrange 2020-06-30 22:08 ` Niklas Söderlund @ 2020-07-01 9:52 ` Suresh Udipi 2020-07-01 9:53 ` [PATCH v9 2/3] media: rcar-csi2: Add warning for PHY speed less than minimum Suresh Udipi 2020-07-01 9:53 ` [PATCH v9 3/3] media: rcar-csi2: Optimize the selection PHTW register Suresh Udipi 0 siblings, 2 replies; 34+ messages in thread From: Suresh Udipi @ 2020-07-01 9:52 UTC (permalink / raw) To: niklas.soderlund Cc: akiyama, efriedrich, erosca, linux-kernel, linux-media, linux-renesas-soc, mchehab, michael, mrodin, securitycheck, sudipi 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 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v9 2/3] media: rcar-csi2: Add warning for PHY speed less than minimum 2020-07-01 9:52 ` [PATCH v9 1/3] media: rcar-csi2: Correct the selection of hsfreqrange Suresh Udipi @ 2020-07-01 9:53 ` Suresh Udipi 2020-07-01 9:53 ` [PATCH v9 3/3] media: rcar-csi2: Optimize the selection PHTW register Suresh Udipi 1 sibling, 0 replies; 34+ messages in thread From: Suresh Udipi @ 2020-07-01 9:53 UTC (permalink / raw) To: niklas.soderlund Cc: akiyama, efriedrich, erosca, linux-kernel, linux-media, linux-renesas-soc, mchehab, michael, mrodin, securitycheck, sudipi 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 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v9 3/3] media: rcar-csi2: Optimize the selection PHTW register 2020-07-01 9:52 ` [PATCH v9 1/3] media: rcar-csi2: Correct the selection of hsfreqrange Suresh Udipi 2020-07-01 9:53 ` [PATCH v9 2/3] media: rcar-csi2: Add warning for PHY speed less than minimum Suresh Udipi @ 2020-07-01 9:53 ` Suresh Udipi 1 sibling, 0 replies; 34+ messages in thread From: Suresh Udipi @ 2020-07-01 9:53 UTC (permalink / raw) To: niklas.soderlund Cc: akiyama, efriedrich, erosca, linux-kernel, linux-media, linux-renesas-soc, mchehab, michael, mrodin, securitycheck, sudipi 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 ^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v8 1/3] media: rcar-csi2: Correct the selection of hsfreqrange 2020-06-18 10:34 ` [PATCH v8 1/3] " Suresh Udipi 2020-06-18 10:34 ` [PATCH v8 2/3] media: rcar-csi2: Add warning for PHY speed less than minimum Suresh Udipi 2020-06-18 10:34 ` [PATCH v8 3/3] media: rcar-csi2: Optimize the selection PHTW register Suresh Udipi @ 2020-06-30 22:03 ` Niklas Söderlund 2 siblings, 0 replies; 34+ messages in thread From: Niklas Söderlund @ 2020-06-30 22:03 UTC (permalink / raw) To: Suresh Udipi Cc: mrodin, akiyama, efriedrich, erosca, linux-kernel, linux-media, linux-renesas-soc, mchehab, michael, securitycheck 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 ^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH] rcar-vin: rcar-csi2: Select the correct PHTW register 2020-05-12 21:30 ` Niklas Söderlund 2020-05-27 15:47 ` Michael Rodin 2020-05-27 16:16 ` [PATCH] rcar-vin: " Michael Rodin @ 2020-05-27 16:17 ` Michael Rodin 2 siblings, 0 replies; 34+ messages in thread From: Michael Rodin @ 2020-05-27 16:17 UTC (permalink / raw) To: Niklas Söderlund, Mauro Carvalho Chehab, linux-media, linux-renesas-soc, linux-kernel Cc: Michael Rodin, michael, efriedrich, erosca, Suresh Udipi 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 ^ permalink raw reply related [flat|nested] 34+ messages in thread
end of thread, other threads:[~2020-07-01 9:53 UTC | newest] Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-20 4:49 [PATCH] [RFC] media: rcar-csi2: Corrects the selection of hsfreqrange Suresh Udipi 2020-03-04 19:46 ` Niklas Söderlund 2020-03-16 13:02 ` Niklas Söderlund 2020-03-17 7:08 ` [PATCH v2] [RFC] rcar-vin: rcar-csi2: Correct " Suresh Udipi 2020-03-17 10:37 ` Niklas Söderlund 2020-03-18 9:00 ` Suresh Udipi 2020-03-18 10:19 ` Niklas Söderlund 2020-03-20 8:00 ` [PATCH v3] media: " Suresh Udipi 2020-03-23 4:48 ` [PATCH v4] " Suresh Udipi 2020-04-14 11:56 ` Niklas Söderlund 2020-04-30 6:03 ` [PATCH v5] " Suresh Udipi 2020-05-04 15:01 ` Michael Rodin 2020-05-12 21:30 ` Niklas Söderlund 2020-05-27 15:47 ` Michael Rodin 2020-05-27 16:16 ` [PATCH] rcar-vin: " Michael Rodin 2020-06-05 18:44 ` Michael Rodin 2020-06-08 3:25 ` [PATCH v6] media: " Suresh Udipi 2020-06-10 13:40 ` Niklas Söderlund 2020-06-12 3:10 ` Suresh Udipi 2020-06-12 17:28 ` Michael Rodin 2020-06-15 14:11 ` Niklas Söderlund 2020-06-17 4:57 ` [PATCH v7 1/2] " Suresh Udipi 2020-06-17 4:57 ` [PATCH v7 2/2] media: rcar-csi2: Add warning for PHY speed less than minimum Suresh Udipi 2020-06-17 13:37 ` [PATCH v7 1/2] media: rcar-csi2: Correct the selection of hsfreqrange Michael Rodin 2020-06-18 10:34 ` [PATCH v8 1/3] " Suresh Udipi 2020-06-18 10:34 ` [PATCH v8 2/3] media: rcar-csi2: Add warning for PHY speed less than minimum Suresh Udipi 2020-06-30 22:05 ` Niklas Söderlund 2020-06-18 10:34 ` [PATCH v8 3/3] media: rcar-csi2: Optimize the selection PHTW register Suresh Udipi 2020-06-30 22:08 ` Niklas Söderlund 2020-07-01 9:52 ` [PATCH v9 1/3] media: rcar-csi2: Correct the selection of hsfreqrange Suresh Udipi 2020-07-01 9:53 ` [PATCH v9 2/3] media: rcar-csi2: Add warning for PHY speed less than minimum Suresh Udipi 2020-07-01 9:53 ` [PATCH v9 3/3] media: rcar-csi2: Optimize the selection PHTW register Suresh Udipi 2020-06-30 22:03 ` [PATCH v8 1/3] media: rcar-csi2: Correct the selection of hsfreqrange Niklas Söderlund 2020-05-27 16:17 ` [PATCH] rcar-vin: rcar-csi2: Select the correct PHTW register Michael Rodin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).