| Message ID | 20210104165739.116404-2-ezequiel@collabora.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Remove last users of v4l2-clk and remove v4l2-clk | expand |
Dne 04. 01. 21 v 17:57 Ezequiel Garcia napsal(a): > The pxa-camera capture driver currently registers a v4l2-clk > clock, named "mclk", to represent the mt9m111 sensor clock. > > Register a proper fixed-rate clock using the generic clock framework, > which will allow to remove the v4l2-clk clock in the pxa-camera > driver in a follow-up commit. > BTW the mclk output to a sensor is actually a variable rate, divided from lcdclk (which can be changed too). PXA camera driver is using variable pcdev->mclk_divisor to generate the mclk from lcdclk. The rate change is done in pxa_camera_activate(): https://elixir.bootlin.com/linux/v5.11-rc2/source/drivers/media/platform/pxa_camera.c#L1136 __raw_writel(pcdev->mclk_divisor | cicr4, pcdev->base + CICR4); Would it be possible to register a correct clock type with possibility to change the divisor by the standard way? Petr > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Robert Jarzmik <robert.jarzmik@free.fr> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > arch/arm/mach-pxa/devices.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/arch/arm/mach-pxa/devices.c b/arch/arm/mach-pxa/devices.c > index 524d6093e0c7..09b8495f3fd9 100644 > --- a/arch/arm/mach-pxa/devices.c > +++ b/arch/arm/mach-pxa/devices.c > @@ -4,6 +4,7 @@ > #include <linux/init.h> > #include <linux/platform_device.h> > #include <linux/clkdev.h> > +#include <linux/clk-provider.h> > #include <linux/dma-mapping.h> > #include <linux/dmaengine.h> > #include <linux/spi/pxa2xx_spi.h> > @@ -634,6 +635,13 @@ static struct platform_device pxa27x_device_camera = { > > void __init pxa_set_camera_info(struct pxacamera_platform_data *info) > { > + struct clk *mclk; > + > + /* Register a fixed-rate clock for camera sensors. */ > + mclk = clk_register_fixed_rate(NULL, "pxa_camera_clk", NULL, 0, > + info->mclk_10khz * 10000); > + if (!IS_ERR(mclk)) > + clkdev_create(mclk, "mclk", NULL); > pxa_register_device(&pxa27x_device_camera, info); > } > >
Hi Petr, Thanks a lot for reviewing and testing the series. On Tue, 2021-01-05 at 17:41 +0100, Petr Cvek wrote: > > Dne 04. 01. 21 v 17:57 Ezequiel Garcia napsal(a): > > The pxa-camera capture driver currently registers a v4l2-clk > > clock, named "mclk", to represent the mt9m111 sensor clock. > > > > Register a proper fixed-rate clock using the generic clock framework, > > which will allow to remove the v4l2-clk clock in the pxa-camera > > driver in a follow-up commit. > > > > BTW the mclk output to a sensor is actually a variable rate, divided from lcdclk (which can be changed too). PXA camera driver is using variable > pcdev->mclk_divisor to generate the mclk from lcdclk. > Hm, now that I look at this, I see the pxa-camera driver is requiring a clock: pcdev->clk = devm_clk_get(&pdev->dev, NULL); if (IS_ERR(pcdev->clk)) return PTR_ERR(pcdev->clk); Where is this clock registered in the non-devicetree case? > The rate change is done in pxa_camera_activate(): > > https://elixir.bootlin.com/linux/v5.11-rc2/source/drivers/media/platform/pxa_camera.c#L1136 > > __raw_writel(pcdev->mclk_divisor | cicr4, pcdev->base + CICR4); > > Would it be possible to register a correct clock type with possibility to change the divisor by the standard way? > Right, so you mean the pxa-camera driver is the one providing the clock for the sensors? In that case, I guess the pxa-camera driver should be the one registering a CCF clock. Other drivers are doing this, through clk_register for instance. However, for the sake of this series, which is meant to get rid of the v4l2-clk API, I would say it's fine to just register a fixed-rate. This is similar to what v4l2_clk_register was doing, which was registering a dummy clock. Having said that, as I mentioned above, I'm wondering if the mach-pxa boards are really working, given I'm not seeing the clock for pxa-camera. Maybe the best way forward is to just accept that pxa-camera is only supported for the device tree platforms, and therefore drop the support from mach-pxa/ boards. Thanks, Ezequiel > Petr > > > > Cc: Arnd Bergmann <arnd@arndb.de> > > Cc: Robert Jarzmik <robert.jarzmik@free.fr> > > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> > > --- > > arch/arm/mach-pxa/devices.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/arch/arm/mach-pxa/devices.c b/arch/arm/mach-pxa/devices.c > > index 524d6093e0c7..09b8495f3fd9 100644 > > --- a/arch/arm/mach-pxa/devices.c > > +++ b/arch/arm/mach-pxa/devices.c > > @@ -4,6 +4,7 @@ > > #include <linux/init.h> > > #include <linux/platform_device.h> > > #include <linux/clkdev.h> > > +#include <linux/clk-provider.h> > > #include <linux/dma-mapping.h> > > #include <linux/dmaengine.h> > > #include <linux/spi/pxa2xx_spi.h> > > @@ -634,6 +635,13 @@ static struct platform_device pxa27x_device_camera = { > > > > void __init pxa_set_camera_info(struct pxacamera_platform_data *info) > > { > > + struct clk *mclk; > > + > > + /* Register a fixed-rate clock for camera sensors. */ > > + mclk = clk_register_fixed_rate(NULL, "pxa_camera_clk", NULL, 0, > > + info->mclk_10khz * 10000); > > + if (!IS_ERR(mclk)) > > + clkdev_create(mclk, "mclk", NULL); > > pxa_register_device(&pxa27x_device_camera, info); > > } > > > >
Dne 06. 01. 21 v 16:53 Ezequiel Garcia napsal(a): > Hi Petr, > > Thanks a lot for reviewing and testing the series. > > On Tue, 2021-01-05 at 17:41 +0100, Petr Cvek wrote: >> >> Dne 04. 01. 21 v 17:57 Ezequiel Garcia napsal(a): >>> The pxa-camera capture driver currently registers a v4l2-clk >>> clock, named "mclk", to represent the mt9m111 sensor clock. >>> >>> Register a proper fixed-rate clock using the generic clock framework, >>> which will allow to remove the v4l2-clk clock in the pxa-camera >>> driver in a follow-up commit. >>> >> >> BTW the mclk output to a sensor is actually a variable rate, divided from lcdclk (which can be changed too). PXA camera driver is using variable >> pcdev->mclk_divisor to generate the mclk from lcdclk. >> > > Hm, now that I look at this, I see the pxa-camera driver > is requiring a clock: > > pcdev->clk = devm_clk_get(&pdev->dev, NULL); > if (IS_ERR(pcdev->clk)) > return PTR_ERR(pcdev->clk); > > Where is this clock registered in the non-devicetree case? > I think this is where the clock is defined PXA27X_CKEN_1RATE("pxa27x-camera.0", NULL, CAMERA, pxa27x_lcd_bus_parents, 0), https://elixir.bootlin.com/linux/v5.10.2/source/drivers/clk/pxa/clk-pxa27x.c#L180 >> The rate change is done in pxa_camera_activate(): >> >> https://elixir.bootlin.com/linux/v5.11-rc2/source/drivers/media/platform/pxa_camera.c#L1136 >> >> __raw_writel(pcdev->mclk_divisor | cicr4, pcdev->base + CICR4); >> >> Would it be possible to register a correct clock type with possibility to change the divisor by the standard way? >> > > Right, so you mean the pxa-camera driver is the one providing the clock for the sensors? > > In that case, I guess the pxa-camera driver should be the one registering > a CCF clock. Other drivers are doing this, through clk_register for instance. > Yeah that would make the sense, because the camera controller controls the divider and enable signals. > However, for the sake of this series, which is meant to get rid > of the v4l2-clk API, I would say it's fine to just register a fixed-rate. > > This is similar to what v4l2_clk_register was doing, which was registering > a dummy clock. > I guess. Just the 1:1 replacement. > Having said that, as I mentioned above, I'm wondering if the mach-pxa > boards are really working, given I'm not seeing the clock for pxa-camera. > > Maybe the best way forward is to just accept that pxa-camera > is only supported for the device tree platforms, and therefore drop the > support from mach-pxa/ boards. > PXA camera worked without devicetree without problems (I'm not sure if I ever used devicetree). The definition should be in that file above (but I'm not that familiar with the clock framework). best regards, Petr > Thanks, > Ezequiel > > >> Petr >> >> >>> Cc: Arnd Bergmann <arnd@arndb.de> >>> Cc: Robert Jarzmik <robert.jarzmik@free.fr> >>> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> >>> --- >>> arch/arm/mach-pxa/devices.c | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>> diff --git a/arch/arm/mach-pxa/devices.c b/arch/arm/mach-pxa/devices.c >>> index 524d6093e0c7..09b8495f3fd9 100644 >>> --- a/arch/arm/mach-pxa/devices.c >>> +++ b/arch/arm/mach-pxa/devices.c >>> @@ -4,6 +4,7 @@ >>> #include <linux/init.h> >>> #include <linux/platform_device.h> >>> #include <linux/clkdev.h> >>> +#include <linux/clk-provider.h> >>> #include <linux/dma-mapping.h> >>> #include <linux/dmaengine.h> >>> #include <linux/spi/pxa2xx_spi.h> >>> @@ -634,6 +635,13 @@ static struct platform_device pxa27x_device_camera = { >>> >>> void __init pxa_set_camera_info(struct pxacamera_platform_data *info) >>> { >>> + struct clk *mclk; >>> + >>> + /* Register a fixed-rate clock for camera sensors. */ >>> + mclk = clk_register_fixed_rate(NULL, "pxa_camera_clk", NULL, 0, >>> + info->mclk_10khz * 10000); >>> + if (!IS_ERR(mclk)) >>> + clkdev_create(mclk, "mclk", NULL); >>> pxa_register_device(&pxa27x_device_camera, info); >>> } >>> >>> > >
On Fri, 2021-01-08 at 12:02 +0100, Petr Cvek wrote: > Dne 06. 01. 21 v 16:53 Ezequiel Garcia napsal(a): > > Hi Petr, > > > > Thanks a lot for reviewing and testing the series. > > > > On Tue, 2021-01-05 at 17:41 +0100, Petr Cvek wrote: > > > > > > Dne 04. 01. 21 v 17:57 Ezequiel Garcia napsal(a): > > > > The pxa-camera capture driver currently registers a v4l2-clk > > > > clock, named "mclk", to represent the mt9m111 sensor clock. > > > > > > > > Register a proper fixed-rate clock using the generic clock framework, > > > > which will allow to remove the v4l2-clk clock in the pxa-camera > > > > driver in a follow-up commit. > > > > > > > > > > BTW the mclk output to a sensor is actually a variable rate, divided from lcdclk (which can be changed too). PXA camera driver is using > > > variable > > > pcdev->mclk_divisor to generate the mclk from lcdclk. > > > > > > > Hm, now that I look at this, I see the pxa-camera driver > > is requiring a clock: > > > > pcdev->clk = devm_clk_get(&pdev->dev, NULL); > > if (IS_ERR(pcdev->clk)) > > return PTR_ERR(pcdev->clk); > > > > Where is this clock registered in the non-devicetree case? > > > > I think this is where the clock is defined > > PXA27X_CKEN_1RATE("pxa27x-camera.0", NULL, CAMERA, pxa27x_lcd_bus_parents, 0), > > https://elixir.bootlin.com/linux/v5.10.2/source/drivers/clk/pxa/clk-pxa27x.c#L180 > Ah, nice. That's the part I was missing. > > > > The rate change is done in pxa_camera_activate(): > > > > > > https://elixir.bootlin.com/linux/v5.11-rc2/source/drivers/media/platform/pxa_camera.c#L1136 > > > > > > __raw_writel(pcdev->mclk_divisor | cicr4, pcdev->base + CICR4); > > > > > > Would it be possible to register a correct clock type with possibility to change the divisor by the standard way? > > > > > > > Right, so you mean the pxa-camera driver is the one providing the clock for the sensors? > > > > In that case, I guess the pxa-camera driver should be the one registering > > a CCF clock. Other drivers are doing this, through clk_register for instance. > > > > Yeah that would make the sense, because the camera controller controls the divider and enable signals. > > > However, for the sake of this series, which is meant to get rid > > of the v4l2-clk API, I would say it's fine to just register a fixed-rate. > > > > This is similar to what v4l2_clk_register was doing, which was registering > > a dummy clock. > > > > I guess. Just the 1:1 replacement. > Exactly. > > Having said that, as I mentioned above, I'm wondering if the mach-pxa > > boards are really working, given I'm not seeing the clock for pxa-camera. > > > > Maybe the best way forward is to just accept that pxa-camera > > is only supported for the device tree platforms, and therefore drop the > > support from mach-pxa/ boards. > > > > PXA camera worked without devicetree without problems (I'm not sure if I ever used devicetree). The definition should be in that file above (but I'm > not that familiar with the clock framework). > That's fine. I'm quite sure this series should be fine, since we are just replacing the dummy v4l2-clk with dummy proper CCF clock. Thanks! Ezequiel
On Mon, Jan 4, 2021 at 5:57 PM Ezequiel Garcia <ezequiel@collabora.com> wrote: > > The pxa-camera capture driver currently registers a v4l2-clk > clock, named "mclk", to represent the mt9m111 sensor clock. > > Register a proper fixed-rate clock using the generic clock framework, > which will allow to remove the v4l2-clk clock in the pxa-camera > driver in a follow-up commit. > > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Robert Jarzmik <robert.jarzmik@free.fr> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> If there are no objections to the change itself, please take it through the v4l2 git tree. For arch/arm/mach-*/ Acked-by: Arnd Bergmann <arnd@arndb.de>
diff --git a/arch/arm/mach-pxa/devices.c b/arch/arm/mach-pxa/devices.c index 524d6093e0c7..09b8495f3fd9 100644 --- a/arch/arm/mach-pxa/devices.c +++ b/arch/arm/mach-pxa/devices.c @@ -4,6 +4,7 @@ #include <linux/init.h> #include <linux/platform_device.h> #include <linux/clkdev.h> +#include <linux/clk-provider.h> #include <linux/dma-mapping.h> #include <linux/dmaengine.h> #include <linux/spi/pxa2xx_spi.h> @@ -634,6 +635,13 @@ static struct platform_device pxa27x_device_camera = { void __init pxa_set_camera_info(struct pxacamera_platform_data *info) { + struct clk *mclk; + + /* Register a fixed-rate clock for camera sensors. */ + mclk = clk_register_fixed_rate(NULL, "pxa_camera_clk", NULL, 0, + info->mclk_10khz * 10000); + if (!IS_ERR(mclk)) + clkdev_create(mclk, "mclk", NULL); pxa_register_device(&pxa27x_device_camera, info); }
The pxa-camera capture driver currently registers a v4l2-clk clock, named "mclk", to represent the mt9m111 sensor clock. Register a proper fixed-rate clock using the generic clock framework, which will allow to remove the v4l2-clk clock in the pxa-camera driver in a follow-up commit. Cc: Arnd Bergmann <arnd@arndb.de> Cc: Robert Jarzmik <robert.jarzmik@free.fr> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com> --- arch/arm/mach-pxa/devices.c | 8 ++++++++ 1 file changed, 8 insertions(+)