This series support CSI on Allwinner A64. Tested 640x480, 320x240, 720p, 1080p resolutions UYVY8_2X8 format. Changes for v5: - Add mod_rate quirk for better handling clk_set code Changes for v4: - update the compatible string order - add proper commit message - included BPI-M64 patch - skipped amarula-a64 patch Changes for v3: - update dt-bindings for A64 - set mod clock via csi driver - remove assign clocks from dtsi - remove i2c-gpio opendrian - fix avdd and dovdd supplies - remove vcc-csi pin group supply Note: - This series created on top of H3 changes [1] - Tested on top of Maxim's ov5640 changes[2] along with the change I praposed wrt 15fps change on this patch[3] - Here is the test log[4], for anyone's information. [4] https://paste.ubuntu.com/p/pC9ZQKNzQf/ [3] https://patchwork.kernel.org/patch/10708931/ [2] https://patchwork.kernel.org/cover/10708911/ [1] https://patchwork.kernel.org/cover/10705905/ Jagan Teki (6): dt-bindings: media: sun6i: Add A64 CSI compatible media: sun6i: Add mod_rate quirk media: sun6i: Add A64 CSI block support arm64: dts: allwinner: a64: Add A64 CSI controller arm64: dts: allwinner: a64: Add pinmux setting for CSI MCLK on PE1 [DO NOT MERGE] arm64: dts: allwinner: bananapi-m64: Add HDF5640 camera module .../devicetree/bindings/media/sun6i-csi.txt | 1 + .../dts/allwinner/sun50i-a64-bananapi-m64.dts | 65 +++++++++++++++++++ arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 25 +++++++ .../platform/sunxi/sun6i-csi/sun6i_csi.c | 39 +++++++++-- 4 files changed, 124 insertions(+), 6 deletions(-) -- 2.18.0.321.gffc6fa0e3 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Allwinner A64 CSI is a single channel time-multiplexed BT.656 protocol interface. Add separate compatible string for A64 since it require explicit change in sun6i_csi driver to update default CSI_SCLK rate. Reviewed-by: Rob Herring <robh@kernel.org> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> --- Documentation/devicetree/bindings/media/sun6i-csi.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/media/sun6i-csi.txt b/Documentation/devicetree/bindings/media/sun6i-csi.txt index cc37cf7fd051..0dd540bb03db 100644 --- a/Documentation/devicetree/bindings/media/sun6i-csi.txt +++ b/Documentation/devicetree/bindings/media/sun6i-csi.txt @@ -8,6 +8,7 @@ Required properties: * "allwinner,sun6i-a31-csi" * "allwinner,sun8i-h3-csi" * "allwinner,sun8i-v3s-csi" + * "allwinner,sun50i-a64-csi" - reg: base address and size of the memory-mapped region. - interrupts: interrupt associated to this IP - clocks: phandles to the clocks feeding the CSI -- 2.18.0.321.gffc6fa0e3 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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); + 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); -- 2.18.0.321.gffc6fa0e3 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
CSI block in Allwinner A64 has similar features as like in H3, but default mod clock rate in BSP along with latest mainline testing require to operate it at 300MHz. So, add A64 CSI compatibe along with mod_rate quirk. Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> --- drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c index fe002beae09c..48919aabefdb 100644 --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c @@ -913,10 +913,15 @@ static int sun6i_csi_remove(struct platform_device *pdev) static const struct sun6i_csi_variant sun6i_a31_csi = { }; +static const struct sun6i_csi_variant sun50i_a64_csi = { + .mod_rate = 300000000, +}; + static const struct of_device_id sun6i_csi_of_match[] = { { .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, }, + { .compatible = "allwinner,sun50i-a64-csi", .data = &sun50i_a64_csi, }, {}, }; MODULE_DEVICE_TABLE(of, sun6i_csi_of_match); -- 2.18.0.321.gffc6fa0e3 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Add dts node details for Allwinner A64 CSI controller. A64 CSI has similar features as like in H3, but the CSI_SCLK need to update it to 300MHz than default clock rate. Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> --- arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi index 384c417cb7a2..89a0deb3fe6a 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi @@ -532,6 +532,12 @@ interrupt-controller; #interrupt-cells = <3>; + csi_pins: csi-pins { + pins = "PE0", "PE2", "PE3", "PE4", "PE5", "PE6", + "PE7", "PE8", "PE9", "PE10", "PE11"; + function = "csi0"; + }; + i2c0_pins: i2c0_pins { pins = "PH0", "PH1"; function = "i2c0"; @@ -899,6 +905,20 @@ status = "disabled"; }; + csi: csi@1cb0000 { + compatible = "allwinner,sun50i-a64-csi"; + reg = <0x01cb0000 0x1000>; + interrupts = <GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&ccu CLK_BUS_CSI>, + <&ccu CLK_CSI_SCLK>, + <&ccu CLK_DRAM_CSI>; + clock-names = "bus", "mod", "ram"; + resets = <&ccu RST_BUS_CSI>; + pinctrl-names = "default"; + pinctrl-0 = <&csi_pins>; + status = "disabled"; + }; + hdmi: hdmi@1ee0000 { compatible = "allwinner,sun50i-a64-dw-hdmi", "allwinner,sun8i-a83t-dw-hdmi"; -- 2.18.0.321.gffc6fa0e3 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Some camera modules have the SoC feeding a master clock to the sensor instead of having a standalone crystal. This clock signal is generated from the clock control unit and output from the CSI MCLK function of pin PE1. Add a pinmux setting for it for camera sensors to reference. Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> --- arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi index 89a0deb3fe6a..dd5740bc3fc9 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi @@ -538,6 +538,11 @@ function = "csi0"; }; + csi_mclk_pin: csi-mclk { + pins = "PE1"; + function = "csi0"; + }; + i2c0_pins: i2c0_pins { pins = "PH0", "PH1"; function = "i2c0"; -- 2.18.0.321.gffc6fa0e3 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Bananapi M64 comes with an optional sensor based on the ov5640, add support for it with below pin information. - PE13, PE12 via i2c-gpio bitbanging - CLK_CSI_MCLK as external clock - PE1 as external clock pin muxing - DLDO3 as AVDD supply - ALDO1 as DOVDD supply - ELDO3 as DVDD supply - PE16 gpio for reset pin - PE17 gpio for powerdown pin Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> --- .../dts/allwinner/sun50i-a64-bananapi-m64.dts | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts index 83e30e0afe5b..c185ceec8c81 100644 --- a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts @@ -60,6 +60,41 @@ stdout-path = "serial0:115200n8"; }; + i2c-csi { + compatible = "i2c-gpio"; + sda-gpios = <&pio 4 13 GPIO_ACTIVE_HIGH>; /* CSI0-SDA: PE13 */ + scl-gpios = <&pio 4 12 GPIO_ACTIVE_HIGH>; /* CSI0-SCK: PE12 */ + i2c-gpio,delay-us = <5>; + #address-cells = <1>; + #size-cells = <0>; + + ov5640: camera@3c { + compatible = "ovti,ov5640"; + reg = <0x3c>; + pinctrl-names = "default"; + pinctrl-0 = <&csi_mclk_pin>; + clocks = <&ccu CLK_CSI_MCLK>; + clock-names = "xclk"; + + AVDD-supply = <®_dldo3>; + DOVDD-supply = <®_aldo1>; + DVDD-supply = <®_eldo3>; + reset-gpios = <&pio 4 16 GPIO_ACTIVE_LOW>; /* CSI0-RST: PE16 */ + powerdown-gpios = <&pio 4 17 GPIO_ACTIVE_HIGH>; /* CSI0-PWDN: PE17 */ + + port { + ov5640_ep: endpoint { + remote-endpoint = <&csi_ep>; + bus-width = <8>; + hsync-active = <1>; /* Active high */ + vsync-active = <0>; /* Active low */ + data-active = <1>; /* Active high */ + pclk-sample = <1>; /* Rising */ + }; + }; + }; + }; + hdmi-connector { compatible = "hdmi-connector"; type = "a"; @@ -106,6 +141,24 @@ status = "okay"; }; +&csi { + status = "okay"; + + port { + #address-cells = <1>; + #size-cells = <0>; + + csi_ep: endpoint { + remote-endpoint = <&ov5640_ep>; + bus-width = <8>; + hsync-active = <1>; /* Active high */ + vsync-active = <0>; /* Active low */ + data-active = <1>; /* Active high */ + pclk-sample = <1>; /* Rising */ + }; + }; +}; + &dai { status = "okay"; }; @@ -296,6 +349,12 @@ regulator-name = "vcc-wifi"; }; +®_dldo3 { + regulator-min-microvolt = <2800000>; + regulator-max-microvolt = <2800000>; + regulator-name = "avdd-csi"; +}; + ®_dldo4 { regulator-min-microvolt = <1800000>; regulator-max-microvolt = <3300000>; @@ -313,6 +372,12 @@ regulator-name = "cpvdd"; }; +®_eldo3 { + regulator-min-microvolt = <1500000>; + regulator-max-microvolt = <1500000>; + regulator-name = "dvdd-csi"; +}; + ®_fldo1 { regulator-min-microvolt = <1200000>; regulator-max-microvolt = <1200000>; -- 2.18.0.321.gffc6fa0e3 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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? _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
[-- Attachment #1.1: Type: text/plain, Size: 3368 bytes --] 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 -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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? _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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? _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
[-- Attachment #1.1: Type: text/plain, Size: 3198 bytes --] 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 -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 176 bytes --] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel