Re: [PATCH 5/6] media: sun4i: Add H3 deinterlace driver
Dne četrtek, 12. september 2019 ob 22:26:47 CEST je Maxime Ripard napisal(a): > Hi, > > On Thu, Sep 12, 2019 at 07:51:31PM +0200, Jernej Skrabec wrote: > > + dev->regmap = devm_regmap_init_mmio(dev->dev, dev->base, > > + &deinterlace_regmap_config); > > + if (IS_ERR(dev->regmap)) { > > + dev_err(dev->dev, "Couldn't create deinterlace regmap\n"); > > + > > + return PTR_ERR(dev->regmap); > > + } > > + > > + ret = clk_prepare_enable(dev->bus_clk); > > + if (ret) { > > + dev_err(dev->dev, "Failed to enable bus clock\n"); > > + > > + return ret; > > + } > > Do you need to keep the bus clock enabled all the time? Usually, for > the SoCs that have a reset line, you only need it to read / write to > the registers, not to have the controller actually running. > > If you don't, then regmap_init_mmio_clk will take care of that for > you. I just tested and using regmap_init_mmio_clk() with "bus" clock doesn't work. I guess it has to be enabled whole time. I'll just leave it as-is. Best regards, Jernej > > > + clk_set_rate(dev->mod_clk, 3); > > + > > + ret = clk_prepare_enable(dev->mod_clk); > > + if (ret) { > > + dev_err(dev->dev, "Failed to enable mod clock\n"); > > + > > + goto err_bus_clk; > > + } > > + > > + ret = clk_prepare_enable(dev->ram_clk); > > + if (ret) { > > + dev_err(dev->dev, "Failed to enable ram clock\n"); > > + > > + goto err_mod_clk; > > + } > > + > > + ret = reset_control_reset(dev->rstc); > > + if (ret) { > > + dev_err(dev->dev, "Failed to apply reset\n"); > > + > > + goto err_ram_clk; > > + } > > This could be moved to a runtime_pm hook, with get_sync called in the > open. That way you won't leave the device powered on if it's unused. > > > +struct deinterlace_dev { > > + struct v4l2_device v4l2_dev; > > + struct video_device vfd; > > + struct device *dev; > > + struct v4l2_m2m_dev *m2m_dev; > > + > > + /* Device file mutex */ > > + struct mutexdev_mutex; > > + > > + void __iomem*base; > > + struct regmap *regmap; > > Do you need to store the base address in that structure if you're > using the regmap? > > Maxime
Re: [PATCH 5/6] media: sun4i: Add H3 deinterlace driver
Hi, On Sat, Sep 14, 2019 at 08:42:22AM +0200, Jernej Škrabec wrote: > Dne četrtek, 12. september 2019 ob 22:26:47 CEST je Maxime Ripard napisal(a): > > Hi, > > > > On Thu, Sep 12, 2019 at 07:51:31PM +0200, Jernej Skrabec wrote: > > > + dev->regmap = devm_regmap_init_mmio(dev->dev, dev->base, > > > + > &deinterlace_regmap_config); > > > + if (IS_ERR(dev->regmap)) { > > > + dev_err(dev->dev, "Couldn't create deinterlace > regmap\n"); > > > + > > > + return PTR_ERR(dev->regmap); > > > + } > > > + > > > + ret = clk_prepare_enable(dev->bus_clk); > > > + if (ret) { > > > + dev_err(dev->dev, "Failed to enable bus clock\n"); > > > + > > > + return ret; > > > + } > > > > Do you need to keep the bus clock enabled all the time? Usually, for > > the SoCs that have a reset line, you only need it to read / write to > > the registers, not to have the controller actually running. > > > > If you don't, then regmap_init_mmio_clk will take care of that for > > you. > > > > > + clk_set_rate(dev->mod_clk, 3); > > > + > > > + ret = clk_prepare_enable(dev->mod_clk); > > > + if (ret) { > > > + dev_err(dev->dev, "Failed to enable mod clock\n"); > > > + > > > + goto err_bus_clk; > > > + } > > > + > > > + ret = clk_prepare_enable(dev->ram_clk); > > > + if (ret) { > > > + dev_err(dev->dev, "Failed to enable ram clock\n"); > > > + > > > + goto err_mod_clk; > > > + } > > > + > > > + ret = reset_control_reset(dev->rstc); > > > + if (ret) { > > > + dev_err(dev->dev, "Failed to apply reset\n"); > > > + > > > + goto err_ram_clk; > > > + } > > > > This could be moved to a runtime_pm hook, with get_sync called in the > > open. That way you won't leave the device powered on if it's unused. > > Currently I'm looking at sun4i_csi.c as an example of runtime ops, but it > seems a bit wrong to have suspend and resume function marked with > __maybe_unused because they are the only functions which enable needed clocks. > If CONFIG_PM is not enabled, then this driver simply won't work, because > clocks will never get enabled. I guess I can implement runtime pm ops in the > same way and add additional handling when CONFIG_PM is not enabled, right? Ah, right. I guess you can either add a depends on PM, or you can call the function directly and use set_active like we're doing in the SPI driver. > BTW, which callback is get_sync? I don't see it in dev_pm_ops. I suppose I > need only runtime_suspend and runtime_resume. get_sync is the user facing API, ie what you call when you want the device to be powered up. This will call runtime_resume if needed (there were no users, and you become the first one), and on the parent devices if needed too (even though it's not our case). > Off topic: sun6i_csi.c includes linux/pm_runtime.h but it doesn't have any > kind > of power management as far as I can see. That's probably something we can remove then Thanks! Maxime signature.asc Description: PGP signature
Re: [PATCH 5/6] media: sun4i: Add H3 deinterlace driver
Hi! Dne četrtek, 12. september 2019 ob 22:26:47 CEST je Maxime Ripard napisal(a): > Hi, > > On Thu, Sep 12, 2019 at 07:51:31PM +0200, Jernej Skrabec wrote: > > + dev->regmap = devm_regmap_init_mmio(dev->dev, dev->base, > > + &deinterlace_regmap_config); > > + if (IS_ERR(dev->regmap)) { > > + dev_err(dev->dev, "Couldn't create deinterlace regmap\n"); > > + > > + return PTR_ERR(dev->regmap); > > + } > > + > > + ret = clk_prepare_enable(dev->bus_clk); > > + if (ret) { > > + dev_err(dev->dev, "Failed to enable bus clock\n"); > > + > > + return ret; > > + } > > Do you need to keep the bus clock enabled all the time? Usually, for > the SoCs that have a reset line, you only need it to read / write to > the registers, not to have the controller actually running. > > If you don't, then regmap_init_mmio_clk will take care of that for > you. > > > + clk_set_rate(dev->mod_clk, 3); > > + > > + ret = clk_prepare_enable(dev->mod_clk); > > + if (ret) { > > + dev_err(dev->dev, "Failed to enable mod clock\n"); > > + > > + goto err_bus_clk; > > + } > > + > > + ret = clk_prepare_enable(dev->ram_clk); > > + if (ret) { > > + dev_err(dev->dev, "Failed to enable ram clock\n"); > > + > > + goto err_mod_clk; > > + } > > + > > + ret = reset_control_reset(dev->rstc); > > + if (ret) { > > + dev_err(dev->dev, "Failed to apply reset\n"); > > + > > + goto err_ram_clk; > > + } > > This could be moved to a runtime_pm hook, with get_sync called in the > open. That way you won't leave the device powered on if it's unused. Currently I'm looking at sun4i_csi.c as an example of runtime ops, but it seems a bit wrong to have suspend and resume function marked with __maybe_unused because they are the only functions which enable needed clocks. If CONFIG_PM is not enabled, then this driver simply won't work, because clocks will never get enabled. I guess I can implement runtime pm ops in the same way and add additional handling when CONFIG_PM is not enabled, right? BTW, which callback is get_sync? I don't see it in dev_pm_ops. I suppose I need only runtime_suspend and runtime_resume. Off topic: sun6i_csi.c includes linux/pm_runtime.h but it doesn't have any kind of power management as far as I can see. Best regards, Jernej > > > +struct deinterlace_dev { > > + struct v4l2_device v4l2_dev; > > + struct video_device vfd; > > + struct device *dev; > > + struct v4l2_m2m_dev *m2m_dev; > > + > > + /* Device file mutex */ > > + struct mutexdev_mutex; > > + > > + void __iomem*base; > > + struct regmap *regmap; > > Do you need to store the base address in that structure if you're > using the regmap? > > Maxime
Re: [PATCH 5/6] media: sun4i: Add H3 deinterlace driver
Hi! Dne petek, 13. september 2019 ob 11:11:47 CEST je Maxime Ripard napisal(a): > Hi, > > On Thu, Sep 12, 2019 at 10:43:28PM +0200, Jernej Škrabec wrote: > > Dne četrtek, 12. september 2019 ob 22:26:47 CEST je Maxime Ripard napisal(a): > > > > + clk_set_rate(dev->mod_clk, 3); > > I just realized I missed this too. If you really need the rate to be > fixed, and if the controller cannot operate properly at any other > frequency, you probably want to use clk_set_rate_exclusive there. I don't think that's needed. Parents of deinterlace clock are pll-periph0 and pll-periph1 which both have fixed clock and thus deinterlace clock will never be changed. I just set it to same frequency as it is set in BSP driver. I think it works with 600 MHz too, but that's overkill. Best regards, Jernej
Re: [PATCH 5/6] media: sun4i: Add H3 deinterlace driver
Hi, On Thu, Sep 12, 2019 at 10:43:28PM +0200, Jernej Škrabec wrote: > Dne četrtek, 12. september 2019 ob 22:26:47 CEST je Maxime Ripard napisal(a): > > > + clk_set_rate(dev->mod_clk, 3); I just realized I missed this too. If you really need the rate to be fixed, and if the controller cannot operate properly at any other frequency, you probably want to use clk_set_rate_exclusive there. Maxime signature.asc Description: PGP signature
Re: [PATCH 5/6] media: sun4i: Add H3 deinterlace driver
Dne četrtek, 12. september 2019 ob 22:26:47 CEST je Maxime Ripard napisal(a): > Hi, > > On Thu, Sep 12, 2019 at 07:51:31PM +0200, Jernej Skrabec wrote: > > + dev->regmap = devm_regmap_init_mmio(dev->dev, dev->base, > > + &deinterlace_regmap_config); > > + if (IS_ERR(dev->regmap)) { > > + dev_err(dev->dev, "Couldn't create deinterlace regmap\n"); > > + > > + return PTR_ERR(dev->regmap); > > + } > > + > > + ret = clk_prepare_enable(dev->bus_clk); > > + if (ret) { > > + dev_err(dev->dev, "Failed to enable bus clock\n"); > > + > > + return ret; > > + } > > Do you need to keep the bus clock enabled all the time? Usually, for > the SoCs that have a reset line, you only need it to read / write to > the registers, not to have the controller actually running. > > If you don't, then regmap_init_mmio_clk will take care of that for > you. I'll test that. > > > + clk_set_rate(dev->mod_clk, 3); > > + > > + ret = clk_prepare_enable(dev->mod_clk); > > + if (ret) { > > + dev_err(dev->dev, "Failed to enable mod clock\n"); > > + > > + goto err_bus_clk; > > + } > > + > > + ret = clk_prepare_enable(dev->ram_clk); > > + if (ret) { > > + dev_err(dev->dev, "Failed to enable ram clock\n"); > > + > > + goto err_mod_clk; > > + } > > + > > + ret = reset_control_reset(dev->rstc); > > + if (ret) { > > + dev_err(dev->dev, "Failed to apply reset\n"); > > + > > + goto err_ram_clk; > > + } > > This could be moved to a runtime_pm hook, with get_sync called in the > open. That way you won't leave the device powered on if it's unused. Ok. > > > +struct deinterlace_dev { > > + struct v4l2_device v4l2_dev; > > + struct video_device vfd; > > + struct device *dev; > > + struct v4l2_m2m_dev *m2m_dev; > > + > > + /* Device file mutex */ > > + struct mutexdev_mutex; > > + > > + void __iomem*base; > > + struct regmap *regmap; > > Do you need to store the base address in that structure if you're > using the regmap? Probably not. I'll remove it in v2. Best regards, Jernej > > Maxime
Re: [PATCH 5/6] media: sun4i: Add H3 deinterlace driver
Hi, On Thu, Sep 12, 2019 at 07:51:31PM +0200, Jernej Skrabec wrote: > + dev->regmap = devm_regmap_init_mmio(dev->dev, dev->base, > + &deinterlace_regmap_config); > + if (IS_ERR(dev->regmap)) { > + dev_err(dev->dev, "Couldn't create deinterlace regmap\n"); > + > + return PTR_ERR(dev->regmap); > + } > + > + ret = clk_prepare_enable(dev->bus_clk); > + if (ret) { > + dev_err(dev->dev, "Failed to enable bus clock\n"); > + > + return ret; > + } Do you need to keep the bus clock enabled all the time? Usually, for the SoCs that have a reset line, you only need it to read / write to the registers, not to have the controller actually running. If you don't, then regmap_init_mmio_clk will take care of that for you. > + clk_set_rate(dev->mod_clk, 3); > + > + ret = clk_prepare_enable(dev->mod_clk); > + if (ret) { > + dev_err(dev->dev, "Failed to enable mod clock\n"); > + > + goto err_bus_clk; > + } > + > + ret = clk_prepare_enable(dev->ram_clk); > + if (ret) { > + dev_err(dev->dev, "Failed to enable ram clock\n"); > + > + goto err_mod_clk; > + } > + > + ret = reset_control_reset(dev->rstc); > + if (ret) { > + dev_err(dev->dev, "Failed to apply reset\n"); > + > + goto err_ram_clk; > + } This could be moved to a runtime_pm hook, with get_sync called in the open. That way you won't leave the device powered on if it's unused. > +struct deinterlace_dev { > + struct v4l2_device v4l2_dev; > + struct video_device vfd; > + struct device *dev; > + struct v4l2_m2m_dev *m2m_dev; > + > + /* Device file mutex */ > + struct mutexdev_mutex; > + > + void __iomem*base; > + struct regmap *regmap; Do you need to store the base address in that structure if you're using the regmap? Maxime