HI Lubomir, On Tue, Nov 20, 2018 at 11:03:19AM +0100, Lubomir Rintel wrote: > The sensor needs the MCLK clock running when it's being probed. On > platforms where the sensor is instantiated from a DT (MMP2) it is going > to happen asynchronously. > > Therefore, the current modus operandi, where the bridge driver fiddles > with the sensor power and clock itself is not going to fly. As the comments > wisely note, this doesn't even belong there. > > Luckily, the ov7670 driver is already able to control its power and > reset lines, we can just drop the MMP platform glue altogether. > > It also requests the clock via the standard clock subsystem. Good -- let's > set up a clock instance so that the sensor can ask us to enable the clock. > Note that this is pretty dumb at the moment: the clock is hardwired to a > particular frequency and parent. It was always the case. > > Signed-off-by: Lubomir Rintel > > --- > Changes since v1: > - [kbuild/ia64] depend on COMMON_CLK. > - [smatch] fix bad return in mcam_v4l_open() leading to lock not getting > released on error. > > drivers/media/platform/marvell-ccic/Kconfig | 2 + > .../media/platform/marvell-ccic/cafe-driver.c | 9 +- > .../media/platform/marvell-ccic/mcam-core.c | 156 +++++++++++++++--- > .../media/platform/marvell-ccic/mcam-core.h | 3 + > .../media/platform/marvell-ccic/mmp-driver.c | 152 ++--------------- > .../linux/platform_data/media/mmp-camera.h | 2 - > 6 files changed, 161 insertions(+), 163 deletions(-) > > diff --git a/drivers/media/platform/marvell-ccic/Kconfig b/drivers/media/platform/marvell-ccic/Kconfig > index cf12e077203a..3e12eb25740a 100644 > --- a/drivers/media/platform/marvell-ccic/Kconfig > +++ b/drivers/media/platform/marvell-ccic/Kconfig > @@ -1,6 +1,7 @@ > config VIDEO_CAFE_CCIC > tristate "Marvell 88ALP01 (Cafe) CMOS Camera Controller support" > depends on PCI && I2C && VIDEO_V4L2 > + depends on COMMON_CLK > select VIDEO_OV7670 > select VIDEOBUF2_VMALLOC > select VIDEOBUF2_DMA_CONTIG > @@ -14,6 +15,7 @@ config VIDEO_MMP_CAMERA > tristate "Marvell Armada 610 integrated camera controller support" > depends on I2C && VIDEO_V4L2 > depends on ARCH_MMP || COMPILE_TEST > + depends on COMMON_CLK > select VIDEO_OV7670 > select I2C_GPIO > select VIDEOBUF2_VMALLOC > diff --git a/drivers/media/platform/marvell-ccic/cafe-driver.c b/drivers/media/platform/marvell-ccic/cafe-driver.c > index 658294d319c0..0e712bb941ba 100644 > --- a/drivers/media/platform/marvell-ccic/cafe-driver.c > +++ b/drivers/media/platform/marvell-ccic/cafe-driver.c > @@ -33,6 +33,7 @@ > #include > #include > #include > +#include > > #include "mcam-core.h" > > @@ -533,11 +534,10 @@ static int cafe_pci_probe(struct pci_dev *pdev, > goto out_iounmap; > > /* > - * Initialize the controller and leave it powered up. It will > - * stay that way until the sensor driver shows up. > + * Initialize the controller. > */ > cafe_ctlr_init(mcam); > - cafe_ctlr_power_up(mcam); > + > /* > * Set up I2C/SMBUS communications. We have to drop the mutex here > * because the sensor could attach in this call chain, leading to > @@ -555,6 +555,9 @@ static int cafe_pci_probe(struct pci_dev *pdev, > if (ret) > goto out_smbus_shutdown; > > + clkdev_create(mcam->mclk, "xclk", "%d-%04x", > + i2c_adapter_id(cam->i2c_adapter), ov7670_info.addr); > + > if (i2c_new_device(cam->i2c_adapter, &ov7670_info)) { > cam->registered = 1; > return 0; > diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c b/drivers/media/platform/marvell-ccic/mcam-core.c > index 87812b7287f0..982fbac6472d 100644 > --- a/drivers/media/platform/marvell-ccic/mcam-core.c > +++ b/drivers/media/platform/marvell-ccic/mcam-core.c > @@ -22,6 +22,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -303,9 +304,6 @@ static void mcam_enable_mipi(struct mcam_camera *mcam) > */ > mcam_reg_write(mcam, REG_CSI2_CTRL0, > CSI2_C0_MIPI_EN | CSI2_C0_ACT_LANE(mcam->lane)); > - mcam_reg_write(mcam, REG_CLKCTRL, > - (mcam->mclk_src << 29) | mcam->mclk_div); > - > mcam->mipi_enabled = true; > } > } > @@ -846,11 +844,6 @@ static void mcam_ctlr_init(struct mcam_camera *cam) > * but it's good to be sure. > */ > mcam_reg_clear_bit(cam, REG_CTRL0, C0_ENABLE); > - /* > - * Clock the sensor appropriately. Controller clock should > - * be 48MHz, sensor "typical" value is half that. > - */ > - mcam_reg_write_mask(cam, REG_CLKCTRL, 2, CLK_DIV_MASK); > spin_unlock_irqrestore(&cam->dev_lock, flags); > } > > @@ -898,14 +891,15 @@ static int mcam_ctlr_power_up(struct mcam_camera *cam) > int ret; > > spin_lock_irqsave(&cam->dev_lock, flags); > - ret = cam->plat_power_up(cam); > - if (ret) { > - spin_unlock_irqrestore(&cam->dev_lock, flags); > - return ret; > + if (cam->plat_power_up) { > + ret = cam->plat_power_up(cam); > + if (ret) { > + spin_unlock_irqrestore(&cam->dev_lock, flags); > + return ret; > + } > } > mcam_reg_clear_bit(cam, REG_CTRL1, C1_PWRDWN); > spin_unlock_irqrestore(&cam->dev_lock, flags); > - msleep(5); /* Just to be sure */ > return 0; > } > > @@ -920,10 +914,101 @@ static void mcam_ctlr_power_down(struct mcam_camera *cam) > * power down routine. > */ > mcam_reg_set_bit(cam, REG_CTRL1, C1_PWRDWN); > - cam->plat_power_down(cam); > + if (cam->plat_power_down) > + cam->plat_power_down(cam); > spin_unlock_irqrestore(&cam->dev_lock, flags); > } > > +/* ---------------------------------------------------------------------- */ > +/* > + * Controller clocks. > + */ > +static void mcam_clk_enable(struct mcam_camera *mcam) > +{ > + unsigned int i; > + > + for (i = 0; i < NR_MCAM_CLK; i++) { > + if (!IS_ERR(mcam->clk[i])) > + clk_prepare_enable(mcam->clk[i]); > + } > +} > + > +static void mcam_clk_disable(struct mcam_camera *mcam) > +{ > + int i; > + > + for (i = NR_MCAM_CLK - 1; i >= 0; i--) { > + if (!IS_ERR(mcam->clk[i])) > + clk_disable_unprepare(mcam->clk[i]); > + } > +} > + > +/* ---------------------------------------------------------------------- */ > +/* > + * Master sensor clock. > + */ > +static int mclk_prepare(struct clk_hw *hw) > +{ > + struct mcam_camera *cam = container_of(hw, struct mcam_camera, mclk_hw); > + > + clk_prepare(cam->clk[0]); > + return 0; > +} > + > +static void mclk_unprepare(struct clk_hw *hw) > +{ > + struct mcam_camera *cam = container_of(hw, struct mcam_camera, mclk_hw); > + > + clk_unprepare(cam->clk[0]); > +} > + > +static int mclk_enable(struct clk_hw *hw) > +{ > + struct mcam_camera *cam = container_of(hw, struct mcam_camera, mclk_hw); > + int mclk_src; > + int mclk_div; > + > + /* > + * Clock the sensor appropriately. Controller clock should > + * be 48MHz, sensor "typical" value is half that. > + */ > + if (cam->bus_type == V4L2_MBUS_CSI2_DPHY) { > + mclk_src = cam->mclk_src; > + mclk_div = cam->mclk_div; > + } else { > + mclk_src = 3; > + mclk_div = 2; > + } > + > + clk_enable(cam->clk[0]); > + mcam_reg_write(cam, REG_CLKCTRL, (mclk_src << 29) | mclk_div); > + mcam_ctlr_power_up(cam); > + > + return 0; > +} > + > +static void mclk_disable(struct clk_hw *hw) > +{ > + struct mcam_camera *cam = container_of(hw, struct mcam_camera, mclk_hw); > + > + mcam_ctlr_power_down(cam); > + clk_disable(cam->clk[0]); > +} > + > +static unsigned long mclk_recalc_rate(struct clk_hw *hw, > + unsigned long parent_rate) > +{ > + return 48000000; > +} > + > +static const struct clk_ops mclk_ops = { > + .prepare = mclk_prepare, > + .unprepare = mclk_unprepare, > + .enable = mclk_enable, > + .disable = mclk_disable, > + .recalc_rate = mclk_recalc_rate, > +}; > + > /* -------------------------------------------------------------------- */ > /* > * Communications with the sensor. > @@ -948,7 +1033,6 @@ static int mcam_cam_init(struct mcam_camera *cam) > ret = __mcam_cam_reset(cam); > /* Get/set parameters? */ > cam->state = S_IDLE; > - mcam_ctlr_power_down(cam); > return ret; > } > > @@ -1584,9 +1668,10 @@ static int mcam_v4l_open(struct file *filp) > if (ret) > goto out; > if (v4l2_fh_is_singular_file(filp)) { > - ret = mcam_ctlr_power_up(cam); > + ret = sensor_call(cam, core, s_power, 1); > if (ret) > goto out; > + mcam_clk_enable(cam); > __mcam_cam_reset(cam); > mcam_set_config_needed(cam, 1); > } > @@ -1608,7 +1693,8 @@ static int mcam_v4l_release(struct file *filp) > _vb2_fop_release(filp, NULL); > if (last_open) { > mcam_disable_mipi(cam); > - mcam_ctlr_power_down(cam); > + sensor_call(cam, core, s_power, 0); > + mcam_clk_disable(cam); > if (cam->buffer_mode == B_vmalloc && alloc_bufs_at_read) > mcam_free_dma_bufs(cam); > } > @@ -1806,6 +1892,7 @@ static const struct v4l2_async_notifier_operations mccic_notify_ops = { > > int mccic_register(struct mcam_camera *cam) > { > + struct clk_init_data mclk_init = { }; > int ret; > > /* > @@ -1838,7 +1925,10 @@ int mccic_register(struct mcam_camera *cam) > mcam_set_config_needed(cam, 1); > cam->pix_format = mcam_def_pix_format; > cam->mbus_code = mcam_def_mbus_code; > - mcam_ctlr_init(cam); > + > + mcam_clk_enable(cam); Am I mis-interpreting the clock bindings, or here you expose a clock source, and sensors are supposed to reference it if they need to. It is then the sensor driver that by calling clk_prepare_enable() on the referenced clock triggers the call of the 'enable' function. It seems to me that here you have exposed a clock provider, but the provider itself enables its clocks... Am I confused? Thanks j > + mcam_ctlr_init(cam); // XXX? > + mcam_clk_disable(cam); > > /* > * Register sensor notifier. > @@ -1857,6 +1947,26 @@ int mccic_register(struct mcam_camera *cam) > goto out; > } > > + /* > + * Register sensor master clock. > + */ > + mclk_init.parent_names = NULL; > + mclk_init.num_parents = 0; > + mclk_init.ops = &mclk_ops; > + mclk_init.name = "mclk"; > + > + of_property_read_string(cam->dev->of_node, "clock-output-names", > + &mclk_init.name); > + > + cam->mclk_hw.init = &mclk_init; > + > + cam->mclk = devm_clk_register(cam->dev, &cam->mclk_hw); > + if (IS_ERR(cam->mclk)) { > + ret = PTR_ERR(cam->mclk); > + dev_err(cam->dev, "can't register clock\n"); > + goto out; > + } > + > /* > * If so requested, try to get our DMA buffers now. > */ > @@ -1884,7 +1994,7 @@ void mccic_shutdown(struct mcam_camera *cam) > */ > if (!list_empty(&cam->vdev.fh_list)) { > cam_warn(cam, "Removing a device with users!\n"); > - mcam_ctlr_power_down(cam); > + sensor_call(cam, core, s_power, 0); > } > if (cam->buffer_mode == B_vmalloc) > mcam_free_dma_bufs(cam); > @@ -1906,7 +2016,8 @@ void mccic_suspend(struct mcam_camera *cam) > enum mcam_state cstate = cam->state; > > mcam_ctlr_stop_dma(cam); > - mcam_ctlr_power_down(cam); > + sensor_call(cam, core, s_power, 0); > + mcam_clk_disable(cam); > cam->state = cstate; > } > mutex_unlock(&cam->s_mutex); > @@ -1919,14 +2030,15 @@ int mccic_resume(struct mcam_camera *cam) > > mutex_lock(&cam->s_mutex); > if (!list_empty(&cam->vdev.fh_list)) { > - ret = mcam_ctlr_power_up(cam); > + mcam_clk_enable(cam); > + ret = sensor_call(cam, core, s_power, 1); > if (ret) { > mutex_unlock(&cam->s_mutex); > return ret; > } > __mcam_cam_reset(cam); > } else { > - mcam_ctlr_power_down(cam); > + sensor_call(cam, core, s_power, 0); > } > mutex_unlock(&cam->s_mutex); > > diff --git a/drivers/media/platform/marvell-ccic/mcam-core.h b/drivers/media/platform/marvell-ccic/mcam-core.h > index 4a72213aca1a..2e3a7567a76a 100644 > --- a/drivers/media/platform/marvell-ccic/mcam-core.h > +++ b/drivers/media/platform/marvell-ccic/mcam-core.h > @@ -8,6 +8,7 @@ > #define _MCAM_CORE_H > > #include > +#include > #include > #include > #include > @@ -125,6 +126,8 @@ struct mcam_camera { > > /* clock tree support */ > struct clk *clk[NR_MCAM_CLK]; > + struct clk_hw mclk_hw; > + struct clk *mclk; > > /* > * Callbacks from the core to the platform code. > diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c b/drivers/media/platform/marvell-ccic/mmp-driver.c > index efbffb06e25c..7e0783dc9152 100644 > --- a/drivers/media/platform/marvell-ccic/mmp-driver.c > +++ b/drivers/media/platform/marvell-ccic/mmp-driver.c > @@ -22,9 +22,7 @@ > #include > #include > #include > -#include > #include > -#include > #include > #include > #include > @@ -38,7 +36,6 @@ MODULE_LICENSE("GPL"); > static char *mcam_clks[] = {"axi", "func", "phy"}; > > struct mmp_camera { > - void __iomem *power_regs; > struct platform_device *pdev; > struct mcam_camera mcam; > struct list_head devlist; > @@ -94,94 +91,6 @@ static struct mmp_camera *mmpcam_find_device(struct platform_device *pdev) > return NULL; > } > > - > - > - > -/* > - * Power-related registers; this almost certainly belongs > - * somewhere else. > - * > - * ARMADA 610 register manual, sec 7.2.1, p1842. > - */ > -#define CPU_SUBSYS_PMU_BASE 0xd4282800 > -#define REG_CCIC_DCGCR 0x28 /* CCIC dyn clock gate ctrl reg */ > -#define REG_CCIC_CRCR 0x50 /* CCIC clk reset ctrl reg */ > - > -static void mcam_clk_enable(struct mcam_camera *mcam) > -{ > - unsigned int i; > - > - for (i = 0; i < NR_MCAM_CLK; i++) { > - if (!IS_ERR(mcam->clk[i])) > - clk_prepare_enable(mcam->clk[i]); > - } > -} > - > -static void mcam_clk_disable(struct mcam_camera *mcam) > -{ > - int i; > - > - for (i = NR_MCAM_CLK - 1; i >= 0; i--) { > - if (!IS_ERR(mcam->clk[i])) > - clk_disable_unprepare(mcam->clk[i]); > - } > -} > - > -/* > - * Power control. > - */ > -static void mmpcam_power_up_ctlr(struct mmp_camera *cam) > -{ > - iowrite32(0x3f, cam->power_regs + REG_CCIC_DCGCR); > - iowrite32(0x3805b, cam->power_regs + REG_CCIC_CRCR); > - mdelay(1); > -} > - > -static int mmpcam_power_up(struct mcam_camera *mcam) > -{ > - struct mmp_camera *cam = mcam_to_cam(mcam); > - struct mmp_camera_platform_data *pdata; > - > -/* > - * Turn on power and clocks to the controller. > - */ > - mmpcam_power_up_ctlr(cam); > - mcam_clk_enable(mcam); > -/* > - * Provide power to the sensor. > - */ > - mcam_reg_write(mcam, REG_CLKCTRL, 0x60000002); > - pdata = cam->pdev->dev.platform_data; > - gpio_set_value(pdata->sensor_power_gpio, 1); > - mdelay(5); > - mcam_reg_clear_bit(mcam, REG_CTRL1, 0x10000000); > - gpio_set_value(pdata->sensor_reset_gpio, 0); /* reset is active low */ > - mdelay(5); > - gpio_set_value(pdata->sensor_reset_gpio, 1); /* reset is active low */ > - mdelay(5); > - > - return 0; > -} > - > -static void mmpcam_power_down(struct mcam_camera *mcam) > -{ > - struct mmp_camera *cam = mcam_to_cam(mcam); > - struct mmp_camera_platform_data *pdata; > -/* > - * Turn off clocks and set reset lines > - */ > - iowrite32(0, cam->power_regs + REG_CCIC_DCGCR); > - iowrite32(0, cam->power_regs + REG_CCIC_CRCR); > -/* > - * Shut down the sensor. > - */ > - pdata = cam->pdev->dev.platform_data; > - gpio_set_value(pdata->sensor_power_gpio, 0); > - gpio_set_value(pdata->sensor_reset_gpio, 0); > - > - mcam_clk_disable(mcam); > -} > - > /* > * calc the dphy register values > * There are three dphy registers being used. > @@ -327,8 +236,6 @@ static int mmpcam_probe(struct platform_device *pdev) > INIT_LIST_HEAD(&cam->devlist); > > mcam = &cam->mcam; > - mcam->plat_power_up = mmpcam_power_up; > - mcam->plat_power_down = mmpcam_power_down; > mcam->calc_dphy = mmpcam_calc_dphy; > mcam->dev = &pdev->dev; > pdata = pdev->dev.platform_data; > @@ -366,33 +273,6 @@ static int mmpcam_probe(struct platform_device *pdev) > if (IS_ERR(mcam->regs)) > return PTR_ERR(mcam->regs); > mcam->regs_size = resource_size(res); > - /* > - * Power/clock memory is elsewhere; get it too. Perhaps this > - * should really be managed outside of this driver? > - */ > - res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > - cam->power_regs = devm_ioremap_resource(&pdev->dev, res); > - if (IS_ERR(cam->power_regs)) > - return PTR_ERR(cam->power_regs); > - /* > - * Sensor GPIO pins. > - */ > - ret = devm_gpio_request(&pdev->dev, pdata->sensor_power_gpio, > - "cam-power"); > - if (ret) { > - dev_err(&pdev->dev, "Can't get sensor power gpio %d", > - pdata->sensor_power_gpio); > - return ret; > - } > - gpio_direction_output(pdata->sensor_power_gpio, 0); > - ret = devm_gpio_request(&pdev->dev, pdata->sensor_reset_gpio, > - "cam-reset"); > - if (ret) { > - dev_err(&pdev->dev, "Can't get sensor reset gpio %d", > - pdata->sensor_reset_gpio); > - return ret; > - } > - gpio_direction_output(pdata->sensor_reset_gpio, 0); > > mcam_init_clk(mcam); > > @@ -410,14 +290,21 @@ static int mmpcam_probe(struct platform_device *pdev) > fwnode_handle_put(ep); > > /* > - * Power the device up and hand it off to the core. > + * Register the device with the core. > */ > - ret = mmpcam_power_up(mcam); > - if (ret) > - return ret; > ret = mccic_register(mcam); > if (ret) > - goto out_power_down; > + return ret; > + > + /* > + * Add OF clock provider. > + */ > + ret = of_clk_add_provider(pdev->dev.of_node, of_clk_src_simple_get, > + mcam->mclk); > + if (ret) { > + dev_err(&pdev->dev, "can't add DT clock provider\n"); > + goto out; > + } > > /* > * Finally, set up our IRQ now that the core is ready to > @@ -426,7 +313,7 @@ static int mmpcam_probe(struct platform_device *pdev) > res = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > if (res == NULL) { > ret = -ENODEV; > - goto out_unregister; > + goto out; > } > cam->irq = res->start; > ret = devm_request_irq(&pdev->dev, cam->irq, mmpcam_irq, IRQF_SHARED, > @@ -436,10 +323,10 @@ static int mmpcam_probe(struct platform_device *pdev) > return 0; > } > > -out_unregister: > +out: > + fwnode_handle_put(mcam->asd.match.fwnode); > mccic_shutdown(mcam); > -out_power_down: > - mmpcam_power_down(mcam); > + > return ret; > } > > @@ -450,7 +337,6 @@ static int mmpcam_remove(struct mmp_camera *cam) > > mmpcam_remove_device(cam); > mccic_shutdown(mcam); > - mmpcam_power_down(mcam); > return 0; > } > > @@ -482,12 +368,6 @@ static int mmpcam_resume(struct platform_device *pdev) > { > struct mmp_camera *cam = mmpcam_find_device(pdev); > > - /* > - * Power up unconditionally just in case the core tries to > - * touch a register even if nothing was active before; trust > - * me, it's better this way. > - */ > - mmpcam_power_up_ctlr(cam); > return mccic_resume(&cam->mcam); > } > > diff --git a/include/linux/platform_data/media/mmp-camera.h b/include/linux/platform_data/media/mmp-camera.h > index c573ebc40035..53adaab64f28 100644 > --- a/include/linux/platform_data/media/mmp-camera.h > +++ b/include/linux/platform_data/media/mmp-camera.h > @@ -12,8 +12,6 @@ enum dphy3_algo { > }; > > struct mmp_camera_platform_data { > - int sensor_power_gpio; > - int sensor_reset_gpio; > enum v4l2_mbus_type bus_type; > int mclk_src; /* which clock source the MCLK derives from */ > int mclk_div; /* Clock Divider Value for MCLK */ > -- > 2.19.1 >