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

[permalink] [raw]

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

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 <[email protected]>
> Signed-off-by: Kazuyoshi Akiyama <[email protected]>
> ---
> 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

2020-03-16 13:03:51

[permalink] [raw]

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

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 <[email protected]>
> > Signed-off-by: Kazuyoshi Akiyama <[email protected]>
> > ---
> > 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

2020-03-17 07:10:21

[permalink] [raw]

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

2020-03-17 10:38:36

[permalink] [raw]

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

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 <[email protected]>
> Signed-off-by: Kazuyoshi Akiyama <[email protected]>
> ---
> 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

2020-03-18 09:01:16

[permalink] [raw]

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

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 <[email protected]>
> > Signed-off-by: Kazuyoshi Akiyama <[email protected]>
> > ---
> > 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.

2020-03-18 10:21:28

[permalink] [raw]

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

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 <[email protected]>
> > > Signed-off-by: Kazuyoshi Akiyama <[email protected]>
> > > ---
> > > 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

2020-03-20 08:01:56

[permalink] [raw]

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

2020-03-23 04:53:49

[permalink] [raw]

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

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 <[email protected]>
Signed-off-by: Kazuyoshi Akiyama <[email protected]>
---
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

2020-04-14 15:26:00

[permalink] [raw]

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

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 <[email protected]>
> Signed-off-by: Kazuyoshi Akiyama <[email protected]>
> ---
> 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

2020-04-30 06:11:22

[permalink] [raw]

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

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 <[email protected]>
Signed-off-by: Kazuyoshi Akiyama <[email protected]>
---
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

2020-05-04 18:44:28

[permalink] [raw]

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

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 <[email protected]>
> Signed-off-by: Kazuyoshi Akiyama <[email protected]>
> ---
> 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

2020-05-12 21:33:36

[permalink] [raw]

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

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 <[email protected]>
> > Signed-off-by: Kazuyoshi Akiyama <[email protected]>
> > ---
> > 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

2020-05-27 18:59:12

[permalink] [raw]

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

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 <[email protected]>
> > > Signed-off-by: Kazuyoshi Akiyama <[email protected]>
> > > ---
> > > 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

2020-05-27 19:08:42

[permalink] [raw]

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

From: Suresh Udipi <[email protected]>

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 <[email protected]>
Signed-off-by: Michael Rodin <[email protected]>
---
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

2020-05-27 19:10:49

[permalink] [raw]

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

From: Suresh Udipi <[email protected]>

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 <[email protected]>
Signed-off-by: Michael Rodin <[email protected]>
---
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

2020-06-05 18:49:27

[permalink] [raw]

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

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 <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Michael Rodin <[email protected]>
> ---
> 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

2020-06-08 03:39:13

[permalink] [raw]

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

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 <[email protected]>
Signed-off-by: Kazuyoshi Akiyama <[email protected]>
Signed-off-by: Michael Rodin <[email protected]>
---
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

2020-06-10 16:38:41

[permalink] [raw]

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

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 <[email protected]>
> Signed-off-by: Kazuyoshi Akiyama <[email protected]>
> Signed-off-by: Michael Rodin <[email protected]>
> ---
> 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

2020-06-12 03:15:14

[permalink] [raw]

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

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 <[email protected]>
> > Signed-off-by: Kazuyoshi Akiyama <[email protected]>
> > Signed-off-by: Michael Rodin <[email protected]>
> > ---
> > 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.

2020-06-12 17:32:46

[permalink] [raw]

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

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 <[email protected]>
> > > Signed-off-by: Kazuyoshi Akiyama <[email protected]>
> > > Signed-off-by: Michael Rodin <[email protected]>
> > > ---
> > > 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

2020-06-15 14:13:56

[permalink] [raw]

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

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 <[email protected]>
> > > > Signed-off-by: Kazuyoshi Akiyama <[email protected]>
> > > > Signed-off-by: Michael Rodin <[email protected]>
> > > > ---
> > > > 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

2020-06-17 04:59:48

[permalink] [raw]

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

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 <[email protected]>
Signed-off-by: Kazuyoshi Akiyama <[email protected]>
Signed-off-by: Michael Rodin <[email protected]>
---
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

2020-06-17 05:00:10

[permalink] [raw]

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

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 <[email protected]>
Signed-off-by: Michael Rodin <[email protected]>
---
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

2020-06-17 13:40:34

[permalink] [raw]

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

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

2020-06-18 10:37:59

[permalink] [raw]

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

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 <[email protected]>
Signed-off-by: Kazuyoshi Akiyama <[email protected]>
Signed-off-by: Michael Rodin <[email protected]>
---
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

2020-06-18 10:38:25

[permalink] [raw]

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

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 <[email protected]>
Signed-off-by: Michael Rodin <[email protected]>
---
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

2020-06-30 22:14:39

[permalink] [raw]

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

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 <[email protected]>
> Signed-off-by: Kazuyoshi Akiyama <[email protected]>
> Signed-off-by: Michael Rodin <[email protected]>

Reviewed-by: Niklas S?derlund <[email protected]>

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

2020-06-30 22:16:14

[permalink] [raw]

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

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 <[email protected]>
> Signed-off-by: Michael Rodin <[email protected]>
> ---
> 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 <[email protected]>

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

2020-07-01 09:55:59