LinuxTV Patchwork [v5,2/6] media: sun6i: Add mod_rate quirk

login
register
mail settings
Submitter Jagan Teki
Date Dec. 20, 2018, 12:54 p.m.
Message ID <20181220125438.11700-3-jagan@amarulasolutions.com>
Download mbox | patch
Permalink /patch/53592/
State Superseded
Delegated to: Sakari Ailus
Headers show

Comments

Jagan Teki - Dec. 20, 2018, 12:54 p.m.
Unfortunately default CSI_SCLK rate cannot work properly to
drive the connected sensor interface, particularly on few
Allwinner SoC's like A64.

So, add mod_rate quirk via driver data so-that the respective
SoC's which require to alter the default mod clock rate can assign
the operating clock rate.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 .../platform/sunxi/sun6i-csi/sun6i_csi.c      | 34 +++++++++++++++----
 1 file changed, 28 insertions(+), 6 deletions(-)
Maxime Ripard - Dec. 21, 2018, 1 p.m.
On Thu, Dec 20, 2018 at 06:24:34PM +0530, Jagan Teki wrote:
> Unfortunately default CSI_SCLK rate cannot work properly to
> drive the connected sensor interface, particularly on few
> Allwinner SoC's like A64.
> 
> So, add mod_rate quirk via driver data so-that the respective
> SoC's which require to alter the default mod clock rate can assign
> the operating clock rate.
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
>  .../platform/sunxi/sun6i-csi/sun6i_csi.c      | 34 +++++++++++++++----
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> index ee882b66a5ea..fe002beae09c 100644
> --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> @@ -15,6 +15,7 @@
>  #include <linux/ioctl.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/regmap.h>
> @@ -28,8 +29,13 @@
>  
>  #define MODULE_NAME	"sun6i-csi"
>  
> +struct sun6i_csi_variant {
> +	unsigned long			mod_rate;
> +};
> +
>  struct sun6i_csi_dev {
>  	struct sun6i_csi		csi;
> +	const struct sun6i_csi_variant	*variant;
>  	struct device			*dev;
>  
>  	struct regmap			*regmap;
> @@ -822,33 +828,43 @@ static int sun6i_csi_resource_request(struct sun6i_csi_dev *sdev,
>  		return PTR_ERR(sdev->clk_mod);
>  	}
>  
> +	if (sdev->variant->mod_rate)
> +		clk_set_rate_exclusive(sdev->clk_mod, sdev->variant->mod_rate);
> +

It still doesn't make any sense to do it in the probe function...

We discussed this in the previous iteration already.

What we didn't discuss is the variant function that you introduce,
while the previous approach was enough.

Maxime
Jagan Teki - Dec. 24, 2018, 3:27 p.m.
On Fri, Dec 21, 2018 at 6:30 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Thu, Dec 20, 2018 at 06:24:34PM +0530, Jagan Teki wrote:
> > Unfortunately default CSI_SCLK rate cannot work properly to
> > drive the connected sensor interface, particularly on few
> > Allwinner SoC's like A64.
> >
> > So, add mod_rate quirk via driver data so-that the respective
> > SoC's which require to alter the default mod clock rate can assign
> > the operating clock rate.
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> >  .../platform/sunxi/sun6i-csi/sun6i_csi.c      | 34 +++++++++++++++----
> >  1 file changed, 28 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > index ee882b66a5ea..fe002beae09c 100644
> > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > @@ -15,6 +15,7 @@
> >  #include <linux/ioctl.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> > +#include <linux/of_device.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/regmap.h>
> > @@ -28,8 +29,13 @@
> >
> >  #define MODULE_NAME  "sun6i-csi"
> >
> > +struct sun6i_csi_variant {
> > +     unsigned long                   mod_rate;
> > +};
> > +
> >  struct sun6i_csi_dev {
> >       struct sun6i_csi                csi;
> > +     const struct sun6i_csi_variant  *variant;
> >       struct device                   *dev;
> >
> >       struct regmap                   *regmap;
> > @@ -822,33 +828,43 @@ static int sun6i_csi_resource_request(struct sun6i_csi_dev *sdev,
> >               return PTR_ERR(sdev->clk_mod);
> >       }
> >
> > +     if (sdev->variant->mod_rate)
> > +             clk_set_rate_exclusive(sdev->clk_mod, sdev->variant->mod_rate);
> > +
>
> It still doesn't make any sense to do it in the probe function...

I'm not sure we discussed about the context wrt probe, we discussed
about exclusive put clock. Since clocks were enabling in set_power and
clock rate can be set during probe in single time instead of setting
it in set_power for every power enablement. anything wrong with that.

>
> We discussed this in the previous iteration already.
>
> What we didn't discuss is the variant function that you introduce,
> while the previous approach was enough.

We discussed about clk_rate_exclusive_put, and that even handle it in
.remove right? so I have variant to handle it in sun6i_csi_remove.
Jagan Teki - Jan. 6, 2019, 4:30 p.m.
On Mon, Dec 24, 2018 at 8:57 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> On Fri, Dec 21, 2018 at 6:30 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >
> > On Thu, Dec 20, 2018 at 06:24:34PM +0530, Jagan Teki wrote:
> > > Unfortunately default CSI_SCLK rate cannot work properly to
> > > drive the connected sensor interface, particularly on few
> > > Allwinner SoC's like A64.
> > >
> > > So, add mod_rate quirk via driver data so-that the respective
> > > SoC's which require to alter the default mod clock rate can assign
> > > the operating clock rate.
> > >
> > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > ---
> > >  .../platform/sunxi/sun6i-csi/sun6i_csi.c      | 34 +++++++++++++++----
> > >  1 file changed, 28 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > > index ee882b66a5ea..fe002beae09c 100644
> > > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > > @@ -15,6 +15,7 @@
> > >  #include <linux/ioctl.h>
> > >  #include <linux/module.h>
> > >  #include <linux/of.h>
> > > +#include <linux/of_device.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/pm_runtime.h>
> > >  #include <linux/regmap.h>
> > > @@ -28,8 +29,13 @@
> > >
> > >  #define MODULE_NAME  "sun6i-csi"
> > >
> > > +struct sun6i_csi_variant {
> > > +     unsigned long                   mod_rate;
> > > +};
> > > +
> > >  struct sun6i_csi_dev {
> > >       struct sun6i_csi                csi;
> > > +     const struct sun6i_csi_variant  *variant;
> > >       struct device                   *dev;
> > >
> > >       struct regmap                   *regmap;
> > > @@ -822,33 +828,43 @@ static int sun6i_csi_resource_request(struct sun6i_csi_dev *sdev,
> > >               return PTR_ERR(sdev->clk_mod);
> > >       }
> > >
> > > +     if (sdev->variant->mod_rate)
> > > +             clk_set_rate_exclusive(sdev->clk_mod, sdev->variant->mod_rate);
> > > +
> >
> > It still doesn't make any sense to do it in the probe function...
>
> I'm not sure we discussed about the context wrt probe, we discussed
> about exclusive put clock. Since clocks were enabling in set_power and
> clock rate can be set during probe in single time instead of setting
> it in set_power for every power enablement. anything wrong with that.
>
> >
> > We discussed this in the previous iteration already.
> >
> > What we didn't discuss is the variant function that you introduce,
> > while the previous approach was enough.
>
> We discussed about clk_rate_exclusive_put, and that even handle it in
> .remove right? so I have variant to handle it in sun6i_csi_remove.

Any comments?
Maxime Ripard - Jan. 7, 2019, 1:29 p.m.
On Mon, Dec 24, 2018 at 08:57:48PM +0530, Jagan Teki wrote:
> On Fri, Dec 21, 2018 at 6:30 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >
> > On Thu, Dec 20, 2018 at 06:24:34PM +0530, Jagan Teki wrote:
> > > Unfortunately default CSI_SCLK rate cannot work properly to
> > > drive the connected sensor interface, particularly on few
> > > Allwinner SoC's like A64.
> > >
> > > So, add mod_rate quirk via driver data so-that the respective
> > > SoC's which require to alter the default mod clock rate can assign
> > > the operating clock rate.
> > >
> > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > ---
> > >  .../platform/sunxi/sun6i-csi/sun6i_csi.c      | 34 +++++++++++++++----
> > >  1 file changed, 28 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > > index ee882b66a5ea..fe002beae09c 100644
> > > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > > @@ -15,6 +15,7 @@
> > >  #include <linux/ioctl.h>
> > >  #include <linux/module.h>
> > >  #include <linux/of.h>
> > > +#include <linux/of_device.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/pm_runtime.h>
> > >  #include <linux/regmap.h>
> > > @@ -28,8 +29,13 @@
> > >
> > >  #define MODULE_NAME  "sun6i-csi"
> > >
> > > +struct sun6i_csi_variant {
> > > +     unsigned long                   mod_rate;
> > > +};
> > > +
> > >  struct sun6i_csi_dev {
> > >       struct sun6i_csi                csi;
> > > +     const struct sun6i_csi_variant  *variant;
> > >       struct device                   *dev;
> > >
> > >       struct regmap                   *regmap;
> > > @@ -822,33 +828,43 @@ static int sun6i_csi_resource_request(struct sun6i_csi_dev *sdev,
> > >               return PTR_ERR(sdev->clk_mod);
> > >       }
> > >
> > > +     if (sdev->variant->mod_rate)
> > > +             clk_set_rate_exclusive(sdev->clk_mod, sdev->variant->mod_rate);
> > > +
> >
> > It still doesn't make any sense to do it in the probe function...
> 
> I'm not sure we discussed about the context wrt probe, we discussed
> about exclusive put clock.

https://lkml.org/lkml/2018/12/18/584

"Doing it here is not really optimal either, since you'll put a
constraint on the system (maintaining that clock at 300MHz), while
it's not in use."

> Since clocks were enabling in set_power and clock rate can be set
> during probe in single time instead of setting it in set_power for
> every power enablement. anything wrong with that.

See above.

Plus, a clock running draws power. It doesn't really make sense to
draw power for something that is unused.

> > We discussed this in the previous iteration already.
> >
> > What we didn't discuss is the variant function that you introduce,
> > while the previous approach was enough.
> 
> We discussed about clk_rate_exclusive_put, and that even handle it in
> .remove right? so I have variant to handle it in sun6i_csi_remove.

We indeed discussed the clk_rate_exclusive_put. However, you chose to
implement it using a variant structure which really isn't needed.

Maxime
Jagan Teki - Jan. 11, 2019, 6:05 a.m.
On Mon, Jan 7, 2019 at 6:59 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Mon, Dec 24, 2018 at 08:57:48PM +0530, Jagan Teki wrote:
> > On Fri, Dec 21, 2018 at 6:30 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > >
> > > On Thu, Dec 20, 2018 at 06:24:34PM +0530, Jagan Teki wrote:
> > > > Unfortunately default CSI_SCLK rate cannot work properly to
> > > > drive the connected sensor interface, particularly on few
> > > > Allwinner SoC's like A64.
> > > >
> > > > So, add mod_rate quirk via driver data so-that the respective
> > > > SoC's which require to alter the default mod clock rate can assign
> > > > the operating clock rate.
> > > >
> > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > ---
> > > >  .../platform/sunxi/sun6i-csi/sun6i_csi.c      | 34 +++++++++++++++----
> > > >  1 file changed, 28 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > > > index ee882b66a5ea..fe002beae09c 100644
> > > > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > > > @@ -15,6 +15,7 @@
> > > >  #include <linux/ioctl.h>
> > > >  #include <linux/module.h>
> > > >  #include <linux/of.h>
> > > > +#include <linux/of_device.h>
> > > >  #include <linux/platform_device.h>
> > > >  #include <linux/pm_runtime.h>
> > > >  #include <linux/regmap.h>
> > > > @@ -28,8 +29,13 @@
> > > >
> > > >  #define MODULE_NAME  "sun6i-csi"
> > > >
> > > > +struct sun6i_csi_variant {
> > > > +     unsigned long                   mod_rate;
> > > > +};
> > > > +
> > > >  struct sun6i_csi_dev {
> > > >       struct sun6i_csi                csi;
> > > > +     const struct sun6i_csi_variant  *variant;
> > > >       struct device                   *dev;
> > > >
> > > >       struct regmap                   *regmap;
> > > > @@ -822,33 +828,43 @@ static int sun6i_csi_resource_request(struct sun6i_csi_dev *sdev,
> > > >               return PTR_ERR(sdev->clk_mod);
> > > >       }
> > > >
> > > > +     if (sdev->variant->mod_rate)
> > > > +             clk_set_rate_exclusive(sdev->clk_mod, sdev->variant->mod_rate);
> > > > +
> > >
> > > It still doesn't make any sense to do it in the probe function...
> >
> > I'm not sure we discussed about the context wrt probe, we discussed
> > about exclusive put clock.
>
> https://lkml.org/lkml/2018/12/18/584
>
> "Doing it here is not really optimal either, since you'll put a
> constraint on the system (maintaining that clock at 300MHz), while
> it's not in use."
>
> > Since clocks were enabling in set_power and clock rate can be set
> > during probe in single time instead of setting it in set_power for
> > every power enablement. anything wrong with that.
>
> See above.
>
> Plus, a clock running draws power. It doesn't really make sense to
> draw power for something that is unused.

True, but clock is enabled only on sun6i_csi_set_power so setting
clock frequency in probe will draw power?
Jagan Teki - Jan. 11, 2019, 6:24 a.m.
On Mon, Jan 7, 2019 at 6:59 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Mon, Dec 24, 2018 at 08:57:48PM +0530, Jagan Teki wrote:
> > On Fri, Dec 21, 2018 at 6:30 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > >
> > > On Thu, Dec 20, 2018 at 06:24:34PM +0530, Jagan Teki wrote:
> > > > Unfortunately default CSI_SCLK rate cannot work properly to
> > > > drive the connected sensor interface, particularly on few
> > > > Allwinner SoC's like A64.
> > > >
> > > > So, add mod_rate quirk via driver data so-that the respective
> > > > SoC's which require to alter the default mod clock rate can assign
> > > > the operating clock rate.
> > > >
> > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > ---
> > > >  .../platform/sunxi/sun6i-csi/sun6i_csi.c      | 34 +++++++++++++++----
> > > >  1 file changed, 28 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > > > index ee882b66a5ea..fe002beae09c 100644
> > > > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > > > @@ -15,6 +15,7 @@
> > > >  #include <linux/ioctl.h>
> > > >  #include <linux/module.h>
> > > >  #include <linux/of.h>
> > > > +#include <linux/of_device.h>
> > > >  #include <linux/platform_device.h>
> > > >  #include <linux/pm_runtime.h>
> > > >  #include <linux/regmap.h>
> > > > @@ -28,8 +29,13 @@
> > > >
> > > >  #define MODULE_NAME  "sun6i-csi"
> > > >
> > > > +struct sun6i_csi_variant {
> > > > +     unsigned long                   mod_rate;
> > > > +};
> > > > +
> > > >  struct sun6i_csi_dev {
> > > >       struct sun6i_csi                csi;
> > > > +     const struct sun6i_csi_variant  *variant;
> > > >       struct device                   *dev;
> > > >
> > > >       struct regmap                   *regmap;
> > > > @@ -822,33 +828,43 @@ static int sun6i_csi_resource_request(struct sun6i_csi_dev *sdev,
> > > >               return PTR_ERR(sdev->clk_mod);
> > > >       }
> > > >
> > > > +     if (sdev->variant->mod_rate)
> > > > +             clk_set_rate_exclusive(sdev->clk_mod, sdev->variant->mod_rate);
> > > > +
> > >
> > > It still doesn't make any sense to do it in the probe function...
> >
> > I'm not sure we discussed about the context wrt probe, we discussed
> > about exclusive put clock.
>
> https://lkml.org/lkml/2018/12/18/584
>
> "Doing it here is not really optimal either, since you'll put a
> constraint on the system (maintaining that clock at 300MHz), while
> it's not in use."

But this constraint is only set, for SoC's who need mod_rate change
not for whole SoCs.

>
> > Since clocks were enabling in set_power and clock rate can be set
> > during probe in single time instead of setting it in set_power for
> > every power enablement. anything wrong with that.
>
> See above.
>
> Plus, a clock running draws power. It doesn't really make sense to
> draw power for something that is unused.

True, but clock is enabled only on sun6i_csi_set_power so setting
clock frequency in probe will draw power?
(sorry same message sent accidentally)

>
> > > We discussed this in the previous iteration already.
> > >
> > > What we didn't discuss is the variant function that you introduce,
> > > while the previous approach was enough.
> >
> > We discussed about clk_rate_exclusive_put, and that even handle it in
> > .remove right? so I have variant to handle it in sun6i_csi_remove.
>
> We indeed discussed the clk_rate_exclusive_put. However, you chose to
> implement it using a variant structure which really isn't needed.

Because clk_rate_exclusive_put will also do it .remove so adding
driver variant with mod_rate can do this job easily. do you have any
suggestion?
Maxime Ripard - Jan. 16, 2019, 11:11 a.m.
On Fri, Jan 11, 2019 at 11:54:12AM +0530, Jagan Teki wrote:
> On Mon, Jan 7, 2019 at 6:59 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > On Mon, Dec 24, 2018 at 08:57:48PM +0530, Jagan Teki wrote:
> > > On Fri, Dec 21, 2018 at 6:30 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > >
> > > > On Thu, Dec 20, 2018 at 06:24:34PM +0530, Jagan Teki wrote:
> > > > > Unfortunately default CSI_SCLK rate cannot work properly to
> > > > > drive the connected sensor interface, particularly on few
> > > > > Allwinner SoC's like A64.
> > > > >
> > > > > So, add mod_rate quirk via driver data so-that the respective
> > > > > SoC's which require to alter the default mod clock rate can assign
> > > > > the operating clock rate.
> > > > >
> > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > ---
> > > > >  .../platform/sunxi/sun6i-csi/sun6i_csi.c      | 34 +++++++++++++++----
> > > > >  1 file changed, 28 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > > > > index ee882b66a5ea..fe002beae09c 100644
> > > > > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > > > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > > > > @@ -15,6 +15,7 @@
> > > > >  #include <linux/ioctl.h>
> > > > >  #include <linux/module.h>
> > > > >  #include <linux/of.h>
> > > > > +#include <linux/of_device.h>
> > > > >  #include <linux/platform_device.h>
> > > > >  #include <linux/pm_runtime.h>
> > > > >  #include <linux/regmap.h>
> > > > > @@ -28,8 +29,13 @@
> > > > >
> > > > >  #define MODULE_NAME  "sun6i-csi"
> > > > >
> > > > > +struct sun6i_csi_variant {
> > > > > +     unsigned long                   mod_rate;
> > > > > +};
> > > > > +
> > > > >  struct sun6i_csi_dev {
> > > > >       struct sun6i_csi                csi;
> > > > > +     const struct sun6i_csi_variant  *variant;
> > > > >       struct device                   *dev;
> > > > >
> > > > >       struct regmap                   *regmap;
> > > > > @@ -822,33 +828,43 @@ static int sun6i_csi_resource_request(struct sun6i_csi_dev *sdev,
> > > > >               return PTR_ERR(sdev->clk_mod);
> > > > >       }
> > > > >
> > > > > +     if (sdev->variant->mod_rate)
> > > > > +             clk_set_rate_exclusive(sdev->clk_mod, sdev->variant->mod_rate);
> > > > > +
> > > >
> > > > It still doesn't make any sense to do it in the probe function...
> > >
> > > I'm not sure we discussed about the context wrt probe, we discussed
> > > about exclusive put clock.
> >
> > https://lkml.org/lkml/2018/12/18/584
> >
> > "Doing it here is not really optimal either, since you'll put a
> > constraint on the system (maintaining that clock at 300MHz), while
> > it's not in use."
> 
> But this constraint is only set, for SoC's who need mod_rate change
> not for whole SoCs.

Still, that constraint is there for the whole system on affected
SoCs. Whether it applies to one SoC or not is not really relevant.

Maxime
Jagan Teki - Jan. 17, 2019, 4:34 a.m.
On Thu, Jan 17, 2019 at 12:48 AM Maxime Ripard
<maxime.ripard@bootlin.com> wrote:
>
> On Fri, Jan 11, 2019 at 11:54:12AM +0530, Jagan Teki wrote:
> > On Mon, Jan 7, 2019 at 6:59 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > On Mon, Dec 24, 2018 at 08:57:48PM +0530, Jagan Teki wrote:
> > > > On Fri, Dec 21, 2018 at 6:30 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > > >
> > > > > On Thu, Dec 20, 2018 at 06:24:34PM +0530, Jagan Teki wrote:
> > > > > > Unfortunately default CSI_SCLK rate cannot work properly to
> > > > > > drive the connected sensor interface, particularly on few
> > > > > > Allwinner SoC's like A64.
> > > > > >
> > > > > > So, add mod_rate quirk via driver data so-that the respective
> > > > > > SoC's which require to alter the default mod clock rate can assign
> > > > > > the operating clock rate.
> > > > > >
> > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > > ---
> > > > > >  .../platform/sunxi/sun6i-csi/sun6i_csi.c      | 34 +++++++++++++++----
> > > > > >  1 file changed, 28 insertions(+), 6 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > > > > > index ee882b66a5ea..fe002beae09c 100644
> > > > > > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > > > > > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
> > > > > > @@ -15,6 +15,7 @@
> > > > > >  #include <linux/ioctl.h>
> > > > > >  #include <linux/module.h>
> > > > > >  #include <linux/of.h>
> > > > > > +#include <linux/of_device.h>
> > > > > >  #include <linux/platform_device.h>
> > > > > >  #include <linux/pm_runtime.h>
> > > > > >  #include <linux/regmap.h>
> > > > > > @@ -28,8 +29,13 @@
> > > > > >
> > > > > >  #define MODULE_NAME  "sun6i-csi"
> > > > > >
> > > > > > +struct sun6i_csi_variant {
> > > > > > +     unsigned long                   mod_rate;
> > > > > > +};
> > > > > > +
> > > > > >  struct sun6i_csi_dev {
> > > > > >       struct sun6i_csi                csi;
> > > > > > +     const struct sun6i_csi_variant  *variant;
> > > > > >       struct device                   *dev;
> > > > > >
> > > > > >       struct regmap                   *regmap;
> > > > > > @@ -822,33 +828,43 @@ static int sun6i_csi_resource_request(struct sun6i_csi_dev *sdev,
> > > > > >               return PTR_ERR(sdev->clk_mod);
> > > > > >       }
> > > > > >
> > > > > > +     if (sdev->variant->mod_rate)
> > > > > > +             clk_set_rate_exclusive(sdev->clk_mod, sdev->variant->mod_rate);
> > > > > > +
> > > > >
> > > > > It still doesn't make any sense to do it in the probe function...
> > > >
> > > > I'm not sure we discussed about the context wrt probe, we discussed
> > > > about exclusive put clock.
> > >
> > > https://lkml.org/lkml/2018/12/18/584
> > >
> > > "Doing it here is not really optimal either, since you'll put a
> > > constraint on the system (maintaining that clock at 300MHz), while
> > > it's not in use."
> >
> > But this constraint is only set, for SoC's who need mod_rate change
> > not for whole SoCs.
>
> Still, that constraint is there for the whole system on affected
> SoCs. Whether it applies to one SoC or not is not really relevant.

OK, please see this code change and let me know the comments.

https://gist.github.com/openedev/518b9e141f53814bb3adfac6f2bfd78c

Patch

diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
index ee882b66a5ea..fe002beae09c 100644
--- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
+++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c
@@ -15,6 +15,7 @@ 
 #include <linux/ioctl.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/regmap.h>
@@ -28,8 +29,13 @@ 
 
 #define MODULE_NAME	"sun6i-csi"
 
+struct sun6i_csi_variant {
+	unsigned long			mod_rate;
+};
+
 struct sun6i_csi_dev {
 	struct sun6i_csi		csi;
+	const struct sun6i_csi_variant	*variant;
 	struct device			*dev;
 
 	struct regmap			*regmap;
@@ -822,33 +828,43 @@  static int sun6i_csi_resource_request(struct sun6i_csi_dev *sdev,
 		return PTR_ERR(sdev->clk_mod);
 	}
 
+	if (sdev->variant->mod_rate)
+		clk_set_rate_exclusive(sdev->clk_mod, sdev->variant->mod_rate);
+
 	sdev->clk_ram = devm_clk_get(&pdev->dev, "ram");
 	if (IS_ERR(sdev->clk_ram)) {
 		dev_err(&pdev->dev, "Unable to acquire dram-csi clock\n");
-		return PTR_ERR(sdev->clk_ram);
+		ret = PTR_ERR(sdev->clk_ram);
+		goto err_unprotect_clk;
 	}
 
 	sdev->rstc_bus = devm_reset_control_get_shared(&pdev->dev, NULL);
 	if (IS_ERR(sdev->rstc_bus)) {
 		dev_err(&pdev->dev, "Cannot get reset controller\n");
 		return PTR_ERR(sdev->rstc_bus);
+		goto err_unprotect_clk;
 	}
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0) {
 		dev_err(&pdev->dev, "No csi IRQ specified\n");
 		ret = -ENXIO;
-		return ret;
+		goto err_unprotect_clk;
 	}
 
 	ret = devm_request_irq(&pdev->dev, irq, sun6i_csi_isr, 0, MODULE_NAME,
 			       sdev);
 	if (ret) {
 		dev_err(&pdev->dev, "Cannot request csi IRQ\n");
-		return ret;
+		goto err_unprotect_clk;
 	}
 
 	return 0;
+
+err_unprotect_clk:
+	if (sdev->variant->mod_rate)
+		clk_rate_exclusive_put(sdev->clk_mod);
+	return ret;
 }
 
 /*
@@ -871,6 +887,7 @@  static int sun6i_csi_probe(struct platform_device *pdev)
 	sdev->dev = &pdev->dev;
 	/* The DMA bus has the memory mapped at 0 */
 	sdev->dev->dma_pfn_offset = PHYS_OFFSET >> PAGE_SHIFT;
+	sdev->variant = of_device_get_match_data(sdev->dev);
 
 	ret = sun6i_csi_resource_request(sdev, pdev);
 	if (ret)
@@ -887,14 +904,19 @@  static int sun6i_csi_remove(struct platform_device *pdev)
 	struct sun6i_csi_dev *sdev = platform_get_drvdata(pdev);
 
 	sun6i_csi_v4l2_cleanup(&sdev->csi);
+	if (sdev->variant->mod_rate)
+		clk_rate_exclusive_put(sdev->clk_mod);
 
 	return 0;
 }
 
+static const struct sun6i_csi_variant sun6i_a31_csi = {
+};
+
 static const struct of_device_id sun6i_csi_of_match[] = {
-	{ .compatible = "allwinner,sun6i-a31-csi", },
-	{ .compatible = "allwinner,sun8i-h3-csi", },
-	{ .compatible = "allwinner,sun8i-v3s-csi", },
+	{ .compatible = "allwinner,sun6i-a31-csi", .data = &sun6i_a31_csi, },
+	{ .compatible = "allwinner,sun8i-h3-csi", .data = &sun6i_a31_csi, },
+	{ .compatible = "allwinner,sun8i-v3s-csi", .data = &sun6i_a31_csi, },
 	{},
 };
 MODULE_DEVICE_TABLE(of, sun6i_csi_of_match);

Privacy Policy