diff mbox series

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

Message ID 1591586703-32246-1-git-send-email-sudipi@jp.adit-jv.com (mailing list archive)
State Superseded
Delegated to: Kieran Bingham
Headers show
Series [v6] media: rcar-csi2: Correct the selection of hsfreqrange | expand

Commit Message

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

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

This hsfreqrange is out of range.

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

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

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

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

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

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

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

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

  - Aligning the commit message based on the above change


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


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

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

   - Alignement of the commit message based on above changes.

 Changes in v4:
  -  Remove unncessary braces.

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


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

Comments

Niklas Söderlund June 10, 2020, 1:40 p.m. UTC | #1
Hi Suresh,

Thanks for your persistent work!

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

I would drop this warning.

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

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

> +		hsfreq = hsfreq_prev;
> +

How about

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

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

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

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

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

    return 0;
}

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

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

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

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

Agreed I will do the changes and update.
> > -- 
> > 2.7.4
> > 
> 
> 1. https://lkml.org/lkml/2020/5/29/1038
> 
> -- 
> Regards,
> Niklas Söderlund
Michael Rodin June 12, 2020, 5:28 p.m. UTC | #3
Hi Niklas,
Hi Suresh,

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

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

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

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

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

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

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

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

Patch

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;