* [PATCH v3 1/1] i2c: qcom-geni: Provide an option to disable DMA processing
From: Lee Jones @ 2019-09-05 10:22 UTC (permalink / raw)
To: alokc, agross, robh+dt, mark.rutland, bjorn.andersson, vkoul, wsa
Cc: devicetree, linux-arm-msm, linux-kernel, linux-i2c, Lee Jones,
linux-arm-kernel
We have a production-level laptop (Lenovo Yoga C630) which is exhibiting
a rather horrific bug. When I2C HID devices are being scanned for at
boot-time the QCom Geni based I2C (Serial Engine) attempts to use DMA.
When it does, the laptop reboots and the user never sees the OS.
The beautiful thing about this approach is that, *if* the Geni SE DMA
ever starts working, we can remove the C code and any old properties
left in older DTs just become NOOP. Older kernels with newer DTs (less
of a priority) *still* will not work - but they do not work now anyway.
Fixes: 8bc529b25354 ("soc: qcom: geni: Add support for ACPI")
Signed-off-by: Lee Jones <lee.jones@linaro.org>
Reviewed-by: Vinod Koul <vkoul@kernel.org>
---
drivers/i2c/busses/i2c-qcom-geni.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index a89bfce5388e..17abf60c94ae 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -355,11 +355,13 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
{
dma_addr_t rx_dma;
unsigned long time_left;
- void *dma_buf;
+ void *dma_buf = NULL;
struct geni_se *se = &gi2c->se;
size_t len = msg->len;
- dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
+ if (!of_machine_is_compatible("lenovo,yoga-c630"))
+ dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
+
if (dma_buf)
geni_se_select_mode(se, GENI_SE_DMA);
else
@@ -394,11 +396,13 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
{
dma_addr_t tx_dma;
unsigned long time_left;
- void *dma_buf;
+ void *dma_buf = NULL;
struct geni_se *se = &gi2c->se;
size_t len = msg->len;
- dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
+ if (!of_machine_is_compatible("lenovo,yoga-c630"))
+ dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
+
if (dma_buf)
geni_se_select_mode(se, GENI_SE_DMA);
else
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v3 13/16] ARM: mmp: move cputype.h to include/linux/soc/
From: Arnd Bergmann @ 2019-09-05 10:20 UTC (permalink / raw)
To: kbuild test robot
Cc: Mark Rutland, DTML, Jason Cooper, Stephen Boyd, Michael Turquette,
linux-kernel@vger.kernel.org, Russell King, Lubomir Rintel,
Cc : Rob Herring, kbuild-all, To : Olof Johansson,
Thomas Gleixner, Kishon Vijay Abraham I, linux-clk, Linux ARM
In-Reply-To: <201909021510.0g4L7Wva%lkp@intel.com>
On Mon, Sep 2, 2019 at 10:16 AM kbuild test robot <lkp@intel.com> wrote:
>
> vim +5 include/linux/soc/mmp/cputype.h
>
> 49cbe78637eb05 arch/arm/mach-mmp/include/mach/cputype.h Eric Miao 2009-01-20 4
> 49cbe78637eb05 arch/arm/mach-mmp/include/mach/cputype.h Eric Miao 2009-01-20 @5 #include <asm/cputype.h>
> 49cbe78637eb05 arch/arm/mach-mmp/include/mach/cputype.h Eric Miao 2009-01-20 6
>
You can probably do something like
#ifdef CONFIG_ARM
#include <asm/cputype.h>
#else
static inline read_cpuid_id(void) { return 0; }
#endif
Then again, ideally drivers don't even have to know about this,
but would distinguish between devices based on the
compatible string for the particular device.
Arnd
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 net-next 00/13] net: stmmac: Improvements for -next
From: David Miller @ 2019-09-05 10:20 UTC (permalink / raw)
To: Jose.Abreu
Cc: Joao.Pinto, alexandre.torgue, netdev, linux-kernel,
mcoquelin.stm32, peppe.cavallaro, linux-stm32, linux-arm-kernel
In-Reply-To: <cover.1567602867.git.joabreu@synopsys.com>
From: Jose Abreu <Jose.Abreu@synopsys.com>
Date: Wed, 4 Sep 2019 15:16:52 +0200
> Couple of improvements for -next tree. More info in commit logs.
Series applied, thank you.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [V2, 2/2] media: i2c: Add DW9768 VCM driver
From: Andy Shevchenko @ 2019-09-05 10:19 UTC (permalink / raw)
To: Sakari Ailus, Javier Martinez Canillas
Cc: mark.rutland, devicetree, drinkcat, srv_heupstream, sam.hung,
shengnan.wang, tfiga, sj.huang, robh+dt, linux-mediatek,
dongchun.zhu, matthias.bgg, bingbu.cao, mchehab, linux-arm-kernel,
linux-media
In-Reply-To: <20190905082134.GY5475@paasikivi.fi.intel.com>
On Thu, Sep 05, 2019 at 11:21:34AM +0300, Sakari Ailus wrote:
> On Thu, Sep 05, 2019 at 03:21:42PM +0800, dongchun.zhu@mediatek.com wrote:
> > From: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > +static const struct i2c_device_id dw9768_id_table[] = {
> > + { DW9768_NAME, 0 },
> > + { },
>
> Could you drop the I²C ID table?
But why?
It will allow you to instanciate the device from user space.
+Cc: Javier.
Javier, is it needed in new code?
--
With Best Regards,
Andy Shevchenko
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v3 16/16] ARM: dts: mmp3: Add MMP3 SoC dts file
From: Arnd Bergmann @ 2019-09-05 10:17 UTC (permalink / raw)
To: Lubomir Rintel
Cc: Mark Rutland, DTML, Jason Cooper, Stephen Boyd, Michael Turquette,
linux-kernel@vger.kernel.org, Russell King, Cc : Rob Herring,
To : Olof Johansson, Thomas Gleixner, Kishon Vijay Abraham I,
linux-clk, Linux ARM
In-Reply-To: <20190830220743.439670-17-lkundrak@v3.sk>
On Sat, Aug 31, 2019 at 12:12 AM Lubomir Rintel <lkundrak@v3.sk> wrote:
> +/ {
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + aliases {
> + serial0 = &uart1;
> + serial1 = &uart2;
> + serial2 = &uart3;
> + serial3 = &uart4;
> + };
Better move the aliases into the per-board file, not every board
would have all four uarts connected, or labeled in that particular
order.
Arnd
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v12 01/12] lib: introduce copy_struct_{to, from}_user helpers
From: Gabriel Paubert @ 2019-09-05 10:13 UTC (permalink / raw)
To: Andreas Schwab
Cc: linux-ia64, linux-sh, Peter Zijlstra, Rasmus Villemoes,
Alexei Starovoitov, linux-mips, David Howells, linux-kselftest,
sparclinux, Jiri Olsa, linux-arch, linux-s390, Tycho Andersen,
Aleksa Sarai, Shuah Khan, Alexander Shishkin, Ingo Molnar,
Christian Brauner, linux-xtensa, Kees Cook, Arnd Bergmann,
Jann Horn, Oleg Nesterov, Aleksa Sarai, Al Viro, Andy Lutomirski,
Shuah Khan, Namhyung Kim, David Drysdale, linux-arm-kernel,
J. Bruce Fields, linux-parisc, linux-m68k, linux-api, Chanho Min,
Linus Torvalds, Jeff Layton, linux-kernel, Eric Biederman,
linux-alpha, linux-fsdevel, Andrew Morton, linuxppc-dev,
containers
In-Reply-To: <mvma7bj85yo.fsf@linux-m68k.org>
On Thu, Sep 05, 2019 at 11:09:35AM +0200, Andreas Schwab wrote:
> On Sep 05 2019, Aleksa Sarai <cyphar@cyphar.com> wrote:
>
> > diff --git a/lib/struct_user.c b/lib/struct_user.c
> > new file mode 100644
> > index 000000000000..7301ab1bbe98
> > --- /dev/null
> > +++ b/lib/struct_user.c
> > @@ -0,0 +1,182 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (C) 2019 SUSE LLC
> > + * Copyright (C) 2019 Aleksa Sarai <cyphar@cyphar.com>
> > + */
> > +
> > +#include <linux/types.h>
> > +#include <linux/export.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/kernel.h>
> > +#include <linux/string.h>
> > +
> > +#define BUFFER_SIZE 64
> > +
> > +/*
> > + * "memset(p, 0, size)" but for user space buffers. Caller must have already
> > + * checked access_ok(p, size).
> > + */
> > +static int __memzero_user(void __user *p, size_t s)
> > +{
> > + const char zeros[BUFFER_SIZE] = {};
>
> Perhaps make that static?
On SMP? It should at least be per cpu, and I'm not even sure
with preemption.
Gabriel
>
> Andreas.
>
> --
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
> "And now for something completely different."
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [V2, 1/2] media: i2c: dw9768: Add DT support and MAINTAINERS entry
From: Andy Shevchenko @ 2019-09-05 10:14 UTC (permalink / raw)
To: dongchun.zhu
Cc: mark.rutland, devicetree, drinkcat, srv_heupstream, sam.hung,
shengnan.wang, tfiga, sj.huang, robh+dt, linux-mediatek,
sakari.ailus, matthias.bgg, bingbu.cao, mchehab, linux-arm-kernel,
linux-media
In-Reply-To: <20190905072142.14606-2-dongchun.zhu@mediatek.com>
On Thu, Sep 05, 2019 at 03:21:41PM +0800, dongchun.zhu@mediatek.com wrote:
> From: Dongchun Zhu <dongchun.zhu@mediatek.com>
>
> This patch is to add the Devicetree binding documentation and
> MAINTAINERS entry for dw9768 actuator.
>
> Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> ---
> Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.txt | 9 +++++++++
> MAINTAINERS | 7 +++++++
This should be:
1) two separate patches
2) binding in YAML
--
With Best Regards,
Andy Shevchenko
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH -next 25/36] spi: s3c24xx: use devm_platform_ioremap_resource() to simplify code
From: Sylwester Nawrocki @ 2019-09-05 10:08 UTC (permalink / raw)
To: YueHaibing, broonie, f.fainelli, rjui, sbranden, eric, wahrenst,
shc_work, agross, khilman, matthias.bgg, shawnguo, s.hauer,
kernel, festevam, linux-imx, avifishman70, tmaimon77, tali.perry1,
venture, yuenn, benjaminfair, kgene, krzk, andi, palmer,
paul.walmsley, baohua, mripard, wens, ldewangan, thierry.reding,
jonathanh, yamada.masahiro, michal.simek
Cc: linux-samsung-soc, linux-arm-msm, openbmc, linux-mediatek,
linux-kernel, linux-spi, bcm-kernel-feedback-list,
linux-rpi-kernel, linux-tegra, linux-amlogic, linux-riscv,
linux-arm-kernel
In-Reply-To: <20190904135918.25352-26-yuehaibing@huawei.com>
On 9/4/19 15:59, YueHaibing wrote:
> Use devm_platform_ioremap_resource() to simplify the code a bit.
> This is detected by coccinelle.
>
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH 2/2] clk: imx8mn: Use common 1443X/1416X PLL clock structure
From: Anson Huang @ 2019-09-05 21:58 UTC (permalink / raw)
To: mturquette, sboyd, shawnguo, s.hauer, kernel, festevam,
leonard.crestez, abel.vesa, peng.fan, ping.bai, chen.fang,
shengjiu.wang, aisheng.dong, sfr, l.stach, linux-clk,
linux-arm-kernel, linux-kernel
Cc: Linux-imx
In-Reply-To: <1567720699-23514-1-git-send-email-Anson.Huang@nxp.com>
Use common 1413X/1416X PLL clock structure to save a lot
of duplicated code on i.MX8MN clock driver.
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
drivers/clk/imx/clk-imx8mn.c | 89 +++++---------------------------------------
drivers/clk/imx/clk.c | 2 +
2 files changed, 12 insertions(+), 79 deletions(-)
diff --git a/drivers/clk/imx/clk-imx8mn.c b/drivers/clk/imx/clk-imx8mn.c
index cc65c13..91b6da8 100644
--- a/drivers/clk/imx/clk-imx8mn.c
+++ b/drivers/clk/imx/clk-imx8mn.c
@@ -39,75 +39,6 @@ enum {
NR_PLLS,
};
-static const struct imx_pll14xx_rate_table imx8mn_pll1416x_tbl[] = {
- PLL_1416X_RATE(1800000000U, 225, 3, 0),
- PLL_1416X_RATE(1600000000U, 200, 3, 0),
- PLL_1416X_RATE(1500000000U, 375, 3, 1),
- PLL_1416X_RATE(1400000000U, 350, 3, 1),
- PLL_1416X_RATE(1200000000U, 300, 3, 1),
- PLL_1416X_RATE(1000000000U, 250, 3, 1),
- PLL_1416X_RATE(800000000U, 200, 3, 1),
- PLL_1416X_RATE(750000000U, 250, 2, 2),
- PLL_1416X_RATE(700000000U, 350, 3, 2),
- PLL_1416X_RATE(600000000U, 300, 3, 2),
-};
-
-static const struct imx_pll14xx_rate_table imx8mn_audiopll_tbl[] = {
- PLL_1443X_RATE(393216000U, 262, 2, 3, 9437),
- PLL_1443X_RATE(361267200U, 361, 3, 3, 17511),
-};
-
-static const struct imx_pll14xx_rate_table imx8mn_videopll_tbl[] = {
- PLL_1443X_RATE(650000000U, 325, 3, 2, 0),
- PLL_1443X_RATE(594000000U, 198, 2, 2, 0),
-};
-
-static const struct imx_pll14xx_rate_table imx8mn_drampll_tbl[] = {
- PLL_1443X_RATE(650000000U, 325, 3, 2, 0),
-};
-
-static struct imx_pll14xx_clk imx8mn_audio_pll = {
- .type = PLL_1443X,
- .rate_table = imx8mn_audiopll_tbl,
- .rate_count = ARRAY_SIZE(imx8mn_audiopll_tbl),
-};
-
-static struct imx_pll14xx_clk imx8mn_video_pll = {
- .type = PLL_1443X,
- .rate_table = imx8mn_videopll_tbl,
- .rate_count = ARRAY_SIZE(imx8mn_videopll_tbl),
-};
-
-static struct imx_pll14xx_clk imx8mn_dram_pll = {
- .type = PLL_1443X,
- .rate_table = imx8mn_drampll_tbl,
- .rate_count = ARRAY_SIZE(imx8mn_drampll_tbl),
-};
-
-static struct imx_pll14xx_clk imx8mn_arm_pll = {
- .type = PLL_1416X,
- .rate_table = imx8mn_pll1416x_tbl,
- .rate_count = ARRAY_SIZE(imx8mn_pll1416x_tbl),
-};
-
-static struct imx_pll14xx_clk imx8mn_gpu_pll = {
- .type = PLL_1416X,
- .rate_table = imx8mn_pll1416x_tbl,
- .rate_count = ARRAY_SIZE(imx8mn_pll1416x_tbl),
-};
-
-static struct imx_pll14xx_clk imx8mn_vpu_pll = {
- .type = PLL_1416X,
- .rate_table = imx8mn_pll1416x_tbl,
- .rate_count = ARRAY_SIZE(imx8mn_pll1416x_tbl),
-};
-
-static struct imx_pll14xx_clk imx8mn_sys_pll = {
- .type = PLL_1416X,
- .rate_table = imx8mn_pll1416x_tbl,
- .rate_count = ARRAY_SIZE(imx8mn_pll1416x_tbl),
-};
-
static const char * const pll_ref_sels[] = { "osc_24m", "dummy", "dummy", "dummy", };
static const char * const audio_pll1_bypass_sels[] = {"audio_pll1", "audio_pll1_ref_sel", };
static const char * const audio_pll2_bypass_sels[] = {"audio_pll2", "audio_pll2_ref_sel", };
@@ -409,16 +340,16 @@ static int imx8mn_clocks_probe(struct platform_device *pdev)
clks[IMX8MN_SYS_PLL2_REF_SEL] = imx_clk_mux("sys_pll2_ref_sel", base + 0x104, 0, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels));
clks[IMX8MN_SYS_PLL3_REF_SEL] = imx_clk_mux("sys_pll3_ref_sel", base + 0x114, 0, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels));
- clks[IMX8MN_AUDIO_PLL1] = imx_clk_pll14xx("audio_pll1", "audio_pll1_ref_sel", base, &imx8mn_audio_pll);
- clks[IMX8MN_AUDIO_PLL2] = imx_clk_pll14xx("audio_pll2", "audio_pll2_ref_sel", base + 0x14, &imx8mn_audio_pll);
- clks[IMX8MN_VIDEO_PLL1] = imx_clk_pll14xx("video_pll1", "video_pll1_ref_sel", base + 0x28, &imx8mn_video_pll);
- clks[IMX8MN_DRAM_PLL] = imx_clk_pll14xx("dram_pll", "dram_pll_ref_sel", base + 0x50, &imx8mn_dram_pll);
- clks[IMX8MN_GPU_PLL] = imx_clk_pll14xx("gpu_pll", "gpu_pll_ref_sel", base + 0x64, &imx8mn_gpu_pll);
- clks[IMX8MN_VPU_PLL] = imx_clk_pll14xx("vpu_pll", "vpu_pll_ref_sel", base + 0x74, &imx8mn_vpu_pll);
- clks[IMX8MN_ARM_PLL] = imx_clk_pll14xx("arm_pll", "arm_pll_ref_sel", base + 0x84, &imx8mn_arm_pll);
- clks[IMX8MN_SYS_PLL1] = imx_clk_pll14xx("sys_pll1", "sys_pll1_ref_sel", base + 0x94, &imx8mn_sys_pll);
- clks[IMX8MN_SYS_PLL2] = imx_clk_pll14xx("sys_pll2", "sys_pll2_ref_sel", base + 0x104, &imx8mn_sys_pll);
- clks[IMX8MN_SYS_PLL3] = imx_clk_pll14xx("sys_pll3", "sys_pll3_ref_sel", base + 0x114, &imx8mn_sys_pll);
+ clks[IMX8MN_AUDIO_PLL1] = imx_clk_pll14xx("audio_pll1", "audio_pll1_ref_sel", base, &imx_1443x_pll);
+ clks[IMX8MN_AUDIO_PLL2] = imx_clk_pll14xx("audio_pll2", "audio_pll2_ref_sel", base + 0x14, &imx_1443x_pll);
+ clks[IMX8MN_VIDEO_PLL1] = imx_clk_pll14xx("video_pll1", "video_pll1_ref_sel", base + 0x28, &imx_1443x_pll);
+ clks[IMX8MN_DRAM_PLL] = imx_clk_pll14xx("dram_pll", "dram_pll_ref_sel", base + 0x50, &imx_1443x_pll);
+ clks[IMX8MN_GPU_PLL] = imx_clk_pll14xx("gpu_pll", "gpu_pll_ref_sel", base + 0x64, &imx_1416x_pll);
+ clks[IMX8MN_VPU_PLL] = imx_clk_pll14xx("vpu_pll", "vpu_pll_ref_sel", base + 0x74, &imx_1416x_pll);
+ clks[IMX8MN_ARM_PLL] = imx_clk_pll14xx("arm_pll", "arm_pll_ref_sel", base + 0x84, &imx_1416x_pll);
+ clks[IMX8MN_SYS_PLL1] = imx_clk_pll14xx("sys_pll1", "sys_pll1_ref_sel", base + 0x94, &imx_1416x_pll);
+ clks[IMX8MN_SYS_PLL2] = imx_clk_pll14xx("sys_pll2", "sys_pll2_ref_sel", base + 0x104, &imx_1416x_pll);
+ clks[IMX8MN_SYS_PLL3] = imx_clk_pll14xx("sys_pll3", "sys_pll3_ref_sel", base + 0x114, &imx_1416x_pll);
/* PLL bypass out */
clks[IMX8MN_AUDIO_PLL1_BYPASS] = imx_clk_mux_flags("audio_pll1_bypass", base, 4, 1, audio_pll1_bypass_sels, ARRAY_SIZE(audio_pll1_bypass_sels), CLK_SET_RATE_PARENT);
diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
index 788e4eb..864e865e 100644
--- a/drivers/clk/imx/clk.c
+++ b/drivers/clk/imx/clk.c
@@ -17,6 +17,8 @@ DEFINE_SPINLOCK(imx_ccm_lock);
const struct imx_pll14xx_rate_table imx_pll1416x_tbl[] = {
PLL_1416X_RATE(1800000000U, 225, 3, 0),
PLL_1416X_RATE(1600000000U, 200, 3, 0),
+ PLL_1416X_RATE(1500000000U, 375, 3, 1),
+ PLL_1416X_RATE(1400000000U, 350, 3, 1),
PLL_1416X_RATE(1200000000U, 300, 3, 1),
PLL_1416X_RATE(1000000000U, 250, 3, 1),
PLL_1416X_RATE(800000000U, 200, 3, 1),
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [PATCH 1/2] clk: imx8mm: Move 1443X/1416X PLL clock structure to common place
From: Anson Huang @ 2019-09-05 21:58 UTC (permalink / raw)
To: mturquette, sboyd, shawnguo, s.hauer, kernel, festevam,
leonard.crestez, abel.vesa, peng.fan, ping.bai, chen.fang,
shengjiu.wang, aisheng.dong, sfr, l.stach, linux-clk,
linux-arm-kernel, linux-kernel
Cc: Linux-imx
Many i.MX8M SoCs use same 1443X/1416X PLL, such as i.MX8MM,
i.MX8MN and later i.MX8M SoCs, moving these PLL definitions
to common place can save a lot of duplicated code on each
platform.
Meanwhile, no need to define PLL clock structure for every
module which uses same type of PLL, e.g., audio/video/dram use
1443X PLL, arm/gpu/vpu/sys use 1416X PLL, define 2 PLL clock
structure for each group is enough.
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
drivers/clk/imx/clk-imx8mm.c | 87 +++++---------------------------------------
drivers/clk/imx/clk.c | 30 +++++++++++++++
drivers/clk/imx/clk.h | 3 ++
3 files changed, 43 insertions(+), 77 deletions(-)
diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
index 2758e3f..9649250 100644
--- a/drivers/clk/imx/clk-imx8mm.c
+++ b/drivers/clk/imx/clk-imx8mm.c
@@ -26,73 +26,6 @@ static u32 share_count_disp;
static u32 share_count_pdm;
static u32 share_count_nand;
-static const struct imx_pll14xx_rate_table imx8mm_pll1416x_tbl[] = {
- PLL_1416X_RATE(1800000000U, 225, 3, 0),
- PLL_1416X_RATE(1600000000U, 200, 3, 0),
- PLL_1416X_RATE(1200000000U, 300, 3, 1),
- PLL_1416X_RATE(1000000000U, 250, 3, 1),
- PLL_1416X_RATE(800000000U, 200, 3, 1),
- PLL_1416X_RATE(750000000U, 250, 2, 2),
- PLL_1416X_RATE(700000000U, 350, 3, 2),
- PLL_1416X_RATE(600000000U, 300, 3, 2),
-};
-
-static const struct imx_pll14xx_rate_table imx8mm_audiopll_tbl[] = {
- PLL_1443X_RATE(393216000U, 262, 2, 3, 9437),
- PLL_1443X_RATE(361267200U, 361, 3, 3, 17511),
-};
-
-static const struct imx_pll14xx_rate_table imx8mm_videopll_tbl[] = {
- PLL_1443X_RATE(650000000U, 325, 3, 2, 0),
- PLL_1443X_RATE(594000000U, 198, 2, 2, 0),
-};
-
-static const struct imx_pll14xx_rate_table imx8mm_drampll_tbl[] = {
- PLL_1443X_RATE(650000000U, 325, 3, 2, 0),
-};
-
-static struct imx_pll14xx_clk imx8mm_audio_pll = {
- .type = PLL_1443X,
- .rate_table = imx8mm_audiopll_tbl,
- .rate_count = ARRAY_SIZE(imx8mm_audiopll_tbl),
-};
-
-static struct imx_pll14xx_clk imx8mm_video_pll = {
- .type = PLL_1443X,
- .rate_table = imx8mm_videopll_tbl,
- .rate_count = ARRAY_SIZE(imx8mm_videopll_tbl),
-};
-
-static struct imx_pll14xx_clk imx8mm_dram_pll = {
- .type = PLL_1443X,
- .rate_table = imx8mm_drampll_tbl,
- .rate_count = ARRAY_SIZE(imx8mm_drampll_tbl),
-};
-
-static struct imx_pll14xx_clk imx8mm_arm_pll = {
- .type = PLL_1416X,
- .rate_table = imx8mm_pll1416x_tbl,
- .rate_count = ARRAY_SIZE(imx8mm_pll1416x_tbl),
-};
-
-static struct imx_pll14xx_clk imx8mm_gpu_pll = {
- .type = PLL_1416X,
- .rate_table = imx8mm_pll1416x_tbl,
- .rate_count = ARRAY_SIZE(imx8mm_pll1416x_tbl),
-};
-
-static struct imx_pll14xx_clk imx8mm_vpu_pll = {
- .type = PLL_1416X,
- .rate_table = imx8mm_pll1416x_tbl,
- .rate_count = ARRAY_SIZE(imx8mm_pll1416x_tbl),
-};
-
-static struct imx_pll14xx_clk imx8mm_sys_pll = {
- .type = PLL_1416X,
- .rate_table = imx8mm_pll1416x_tbl,
- .rate_count = ARRAY_SIZE(imx8mm_pll1416x_tbl),
-};
-
static const char *pll_ref_sels[] = { "osc_24m", "dummy", "dummy", "dummy", };
static const char *audio_pll1_bypass_sels[] = {"audio_pll1", "audio_pll1_ref_sel", };
static const char *audio_pll2_bypass_sels[] = {"audio_pll2", "audio_pll2_ref_sel", };
@@ -396,16 +329,16 @@ static int imx8mm_clocks_probe(struct platform_device *pdev)
clks[IMX8MM_SYS_PLL2_REF_SEL] = imx_clk_mux("sys_pll2_ref_sel", base + 0x104, 0, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels));
clks[IMX8MM_SYS_PLL3_REF_SEL] = imx_clk_mux("sys_pll3_ref_sel", base + 0x114, 0, 2, pll_ref_sels, ARRAY_SIZE(pll_ref_sels));
- clks[IMX8MM_AUDIO_PLL1] = imx_clk_pll14xx("audio_pll1", "audio_pll1_ref_sel", base, &imx8mm_audio_pll);
- clks[IMX8MM_AUDIO_PLL2] = imx_clk_pll14xx("audio_pll2", "audio_pll2_ref_sel", base + 0x14, &imx8mm_audio_pll);
- clks[IMX8MM_VIDEO_PLL1] = imx_clk_pll14xx("video_pll1", "video_pll1_ref_sel", base + 0x28, &imx8mm_video_pll);
- clks[IMX8MM_DRAM_PLL] = imx_clk_pll14xx("dram_pll", "dram_pll_ref_sel", base + 0x50, &imx8mm_dram_pll);
- clks[IMX8MM_GPU_PLL] = imx_clk_pll14xx("gpu_pll", "gpu_pll_ref_sel", base + 0x64, &imx8mm_gpu_pll);
- clks[IMX8MM_VPU_PLL] = imx_clk_pll14xx("vpu_pll", "vpu_pll_ref_sel", base + 0x74, &imx8mm_vpu_pll);
- clks[IMX8MM_ARM_PLL] = imx_clk_pll14xx("arm_pll", "arm_pll_ref_sel", base + 0x84, &imx8mm_arm_pll);
- clks[IMX8MM_SYS_PLL1] = imx_clk_pll14xx("sys_pll1", "sys_pll1_ref_sel", base + 0x94, &imx8mm_sys_pll);
- clks[IMX8MM_SYS_PLL2] = imx_clk_pll14xx("sys_pll2", "sys_pll2_ref_sel", base + 0x104, &imx8mm_sys_pll);
- clks[IMX8MM_SYS_PLL3] = imx_clk_pll14xx("sys_pll3", "sys_pll3_ref_sel", base + 0x114, &imx8mm_sys_pll);
+ clks[IMX8MM_AUDIO_PLL1] = imx_clk_pll14xx("audio_pll1", "audio_pll1_ref_sel", base, &imx_1443x_pll);
+ clks[IMX8MM_AUDIO_PLL2] = imx_clk_pll14xx("audio_pll2", "audio_pll2_ref_sel", base + 0x14, &imx_1443x_pll);
+ clks[IMX8MM_VIDEO_PLL1] = imx_clk_pll14xx("video_pll1", "video_pll1_ref_sel", base + 0x28, &imx_1443x_pll);
+ clks[IMX8MM_DRAM_PLL] = imx_clk_pll14xx("dram_pll", "dram_pll_ref_sel", base + 0x50, &imx_1443x_pll);
+ clks[IMX8MM_GPU_PLL] = imx_clk_pll14xx("gpu_pll", "gpu_pll_ref_sel", base + 0x64, &imx_1416x_pll);
+ clks[IMX8MM_VPU_PLL] = imx_clk_pll14xx("vpu_pll", "vpu_pll_ref_sel", base + 0x74, &imx_1416x_pll);
+ clks[IMX8MM_ARM_PLL] = imx_clk_pll14xx("arm_pll", "arm_pll_ref_sel", base + 0x84, &imx_1416x_pll);
+ clks[IMX8MM_SYS_PLL1] = imx_clk_pll14xx("sys_pll1", "sys_pll1_ref_sel", base + 0x94, &imx_1416x_pll);
+ clks[IMX8MM_SYS_PLL2] = imx_clk_pll14xx("sys_pll2", "sys_pll2_ref_sel", base + 0x104, &imx_1416x_pll);
+ clks[IMX8MM_SYS_PLL3] = imx_clk_pll14xx("sys_pll3", "sys_pll3_ref_sel", base + 0x114, &imx_1416x_pll);
/* PLL bypass out */
clks[IMX8MM_AUDIO_PLL1_BYPASS] = imx_clk_mux_flags("audio_pll1_bypass", base, 4, 1, audio_pll1_bypass_sels, ARRAY_SIZE(audio_pll1_bypass_sels), CLK_SET_RATE_PARENT);
diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
index cfc05e4..788e4eb 100644
--- a/drivers/clk/imx/clk.c
+++ b/drivers/clk/imx/clk.c
@@ -14,6 +14,36 @@
DEFINE_SPINLOCK(imx_ccm_lock);
+const struct imx_pll14xx_rate_table imx_pll1416x_tbl[] = {
+ PLL_1416X_RATE(1800000000U, 225, 3, 0),
+ PLL_1416X_RATE(1600000000U, 200, 3, 0),
+ PLL_1416X_RATE(1200000000U, 300, 3, 1),
+ PLL_1416X_RATE(1000000000U, 250, 3, 1),
+ PLL_1416X_RATE(800000000U, 200, 3, 1),
+ PLL_1416X_RATE(750000000U, 250, 2, 2),
+ PLL_1416X_RATE(700000000U, 350, 3, 2),
+ PLL_1416X_RATE(600000000U, 300, 3, 2),
+};
+
+const struct imx_pll14xx_rate_table imx_pll1443x_tbl[] = {
+ PLL_1443X_RATE(650000000U, 325, 3, 2, 0),
+ PLL_1443X_RATE(594000000U, 198, 2, 2, 0),
+ PLL_1443X_RATE(393216000U, 262, 2, 3, 9437),
+ PLL_1443X_RATE(361267200U, 361, 3, 3, 17511),
+};
+
+struct imx_pll14xx_clk imx_1443x_pll = {
+ .type = PLL_1443X,
+ .rate_table = imx_pll1443x_tbl,
+ .rate_count = ARRAY_SIZE(imx_pll1443x_tbl),
+};
+
+struct imx_pll14xx_clk imx_1416x_pll = {
+ .type = PLL_1416X,
+ .rate_table = imx_pll1416x_tbl,
+ .rate_count = ARRAY_SIZE(imx_pll1416x_tbl),
+};
+
void imx_unregister_clocks(struct clk *clks[], unsigned int count)
{
unsigned int i;
diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
index f7a389a..bc5bb6a 100644
--- a/drivers/clk/imx/clk.h
+++ b/drivers/clk/imx/clk.h
@@ -50,6 +50,9 @@ struct imx_pll14xx_clk {
int flags;
};
+extern struct imx_pll14xx_clk imx_1416x_pll;
+extern struct imx_pll14xx_clk imx_1443x_pll;
+
#define imx_clk_cpu(name, parent_name, div, mux, pll, step) \
imx_clk_hw_cpu(name, parent_name, div, mux, pll, step)->clk
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v12 01/12] lib: introduce copy_struct_{to,from}_user helpers
From: Aleksa Sarai @ 2019-09-05 9:50 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: linux-ia64, linux-sh, Peter Zijlstra, Alexei Starovoitov,
linux-kernel, David Howells, linux-kselftest, sparclinux,
Jiri Olsa, linux-arch, linux-s390, Tycho Andersen, Aleksa Sarai,
Shuah Khan, Alexander Shishkin, Ingo Molnar, linux-arm-kernel,
linux-mips, linux-xtensa, Kees Cook, Arnd Bergmann, Jann Horn,
linuxppc-dev, linux-m68k, Al Viro, Andy Lutomirski, Shuah Khan,
Namhyung Kim, David Drysdale, Christian Brauner, J. Bruce Fields,
linux-parisc, linux-api, Chanho Min, Jeff Layton, Oleg Nesterov,
Eric Biederman, linux-alpha, linux-fsdevel, Andrew Morton,
Linus Torvalds, containers
In-Reply-To: <57ba3752-c4a6-d2a4-1a4d-a0e13bccd473@rasmusvillemoes.dk>
[-- Attachment #1.1: Type: text/plain, Size: 11250 bytes --]
On 2019-09-05, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> On 04/09/2019 22.19, Aleksa Sarai wrote:
> > A common pattern for syscall extensions is increasing the size of a
> > struct passed from userspace, such that the zero-value of the new fields
> > result in the old kernel behaviour (allowing for a mix of userspace and
> > kernel vintages to operate on one another in most cases). This is done
> > in both directions -- hence two helpers -- though it's more common to
> > have to copy user space structs into kernel space.
> >
> > Previously there was no common lib/ function that implemented
> > the necessary extension-checking semantics (and different syscalls
> > implemented them slightly differently or incompletely[1]). A future
> > patch replaces all of the common uses of this pattern to use the new
> > copy_struct_{to,from}_user() helpers.
> >
> > [1]: For instance {sched_setattr,perf_event_open,clone3}(2) all do do
> > similar checks to copy_struct_from_user() while rt_sigprocmask(2)
> > always rejects differently-sized struct arguments.
> >
> > Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> > ---
> > diff --git a/lib/struct_user.c b/lib/struct_user.c
> > new file mode 100644
> > index 000000000000..7301ab1bbe98
> > --- /dev/null
> > +++ b/lib/struct_user.c
> > @@ -0,0 +1,182 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (C) 2019 SUSE LLC
> > + * Copyright (C) 2019 Aleksa Sarai <cyphar@cyphar.com>
> > + */
> > +
> > +#include <linux/types.h>
> > +#include <linux/export.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/kernel.h>
> > +#include <linux/string.h>
> > +
> > +#define BUFFER_SIZE 64
> > +
> > +/*
> > + * "memset(p, 0, size)" but for user space buffers. Caller must have already
> > + * checked access_ok(p, size).
> > + */
>
> Isn't this __clear_user() exactly (perhaps except for the return value)?
> Perhaps not every arch has that?
I didn't know about clear_user() -- I will switch to it.
> > +static int __memzero_user(void __user *p, size_t s)
> > +{
> > + const char zeros[BUFFER_SIZE] = {};
> > + while (s > 0) {
> > + size_t n = min(s, sizeof(zeros));
> > +
> > + if (__copy_to_user(p, zeros, n))
> > + return -EFAULT;
> > +
> > + p += n;
> > + s -= n;
> > + }
> > + return 0;
> > +}
> > +
> > +/**
> > + * copy_struct_to_user: copy a struct to user space
> > + * @dst: Destination address, in user space.
> > + * @usize: Size of @dst struct.
> > + * @src: Source address, in kernel space.
> > + * @ksize: Size of @src struct.
> > + *
> > + * Returns (in all cases, some data may have been copied):
> > + * * -EFBIG: (@usize < @ksize) and there are non-zero trailing bytes in @src.
> > + * * -EFAULT: access to user space failed.
> > + */
> > +int copy_struct_to_user(void __user *dst, size_t usize,
> > + const void *src, size_t ksize)
> > +{
> > + size_t size = min(ksize, usize);
> > + size_t rest = abs(ksize - usize);
>
> Eh, I'd avoid abs() here due to the funkiness of the implicit type
> conversions - ksize-usize has type size_t, then that's coerced to an int
> (or a long maybe?), the abs is applied which return an int/long (or
> unsigned versions?). Something like "rest = max(ksize, usize) - size;"
> is more obviously correct and doesn't fall into any
> narrowing/widening/sign extending traps.
Yeah, I originally used "max(ksize, usize) - size" for that reason but
was worried it looked too funky (and some quick tests showed that abs()
gives the right results in most cases -- though I just realised it would
probably not give the right results around SIZE_MAX). I'll switch back.
> > + if (unlikely(usize > PAGE_SIZE))
> > + return -EFAULT;
>
> Please don't. That is a restriction on all future extensions - once a
> kernel is shipped with a syscall using this helper with that arbitrary
> restriction in place, that syscall is forever prevented from extending
> its arg struct beyond PAGE_SIZE (which is arch-dependent anyway). Sure,
> it's hard to imagine, but who'd have thought 32 O_* or CLONE_* bits
> weren't enough for everybody?
>
> This is only for future compatibility, and if someone runs an app
> compiled against 7.3 headers on a 5.4 kernel, they probably don't care
> about performance, but they would like their app to run.
I'm not sure I agree that the limit is in place *forever* -- it's
generally not a break in compatibility to convert an error into a
success (though, there are counterexamples such as mknod(2) -- but that
was a very specific case).
You're right that it would mean that some very new code won't run on
very ancient kernels (assuming we ever pass around structs that
massive), but there should be a reasonable trade-off here IMHO.
If we allow very large sizes, a program could probably DoS the kernel by
allocating a moderately-large block of memory and then spawning a bunch
of threads that all cause the kernel to re-check that the same 1GB block
of memory is zeroed. I haven't tried, but it seems like it's best to
avoid the possibility altogether.
> > + }
> > + /* Copy the interoperable parts of the struct. */
> > + if (__copy_to_user(dst, src, size))
> > + return -EFAULT;
>
> I think I understand why you put this last instead of handling the
> buffer in the "natural" order. However,
> I'm wondering whether we should actually do this copy before checking
> that the extra kernel bytes are 0 - the user will still be told that
> there was some extra information via the -EFBIG/-E2BIG return, but maybe
> in some cases the part he understands is good enough. But I also guess
> we have to look to existing users to see whether that would prevent them
> from being converted to using this helper.
>
> linux-api folks, WDYT?
Regarding the order, I just copied what sched and perf already do. I
wouldn't mind doing it the other way around -- though I am a little
cautious about implicitly making guarantees like that. The syscall that
uses copy_struct_to_user() might not want to make that guarantee (it
might not make sense for them), and there are some -E2BIG returns that
won't result in data being copied (usize > PAGE_SIZE).
As for feedback, this is syscall-dependent at the moment. The sched and
perf users explicitly return the size of the kernel structure (by
overwriting uattr->size if -E2BIG is returned) for copies in either
direction. So users arguably already have some kind of feedback about
size issues. clone3() on the other hand doesn't do that (though it
doesn't copy anything to user-space so this isn't relevant to this
particular question).
Effectively, I'd like to see someone argue that this is something that
they would personally want (before we do it).
> > + return 0;
>
> Maybe more useful to "return size;", some users might want to know/pass
> on how much was actually copied.
Even though it is "just" min(ksize, usize), I don't see any harm in
returning it. Will do.
> > +}
> > +EXPORT_SYMBOL(copy_struct_to_user);
>
> Can't we wait with this until a modular user shows up? The primary users
> are syscalls, which can't be modular AFAIK.
Yeah, I'll drop it. You could use them for ioctl()s but we can always
add EXPORT_SYMBOL() later.
> > +/**
> > + * copy_struct_from_user: copy a struct from user space
> > + * @dst: Destination address, in kernel space. This buffer must be @ksize
> > + * bytes long.
> > + * @ksize: Size of @dst struct.
> > + * @src: Source address, in user space.
> > + * @usize: (Alleged) size of @src struct.
> > + *
> > + * Copies a struct from user space to kernel space, in a way that guarantees
> > + * backwards-compatibility for struct syscall arguments (as long as future
> > + * struct extensions are made such that all new fields are *appended* to the
> > + * old struct, and zeroed-out new fields have the same meaning as the old
> > + * struct).
> > + *
> > + * @ksize is just sizeof(*dst), and @usize should've been passed by user space.
> > + * The recommended usage is something like the following:
> > + *
> > + * SYSCALL_DEFINE2(foobar, const struct foo __user *, uarg, size_t, usize)
> > + * {
> > + * int err;
> > + * struct foo karg = {};
> > + *
> > + * err = copy_struct_from_user(&karg, sizeof(karg), uarg, size);
> > + * if (err)
> > + * return err;
> > + *
> > + * // ...
> > + * }
> > + *
> > + * There are three cases to consider:
> > + * * If @usize == @ksize, then it's copied verbatim.
> > + * * If @usize < @ksize, then the user space has passed an old struct to a
> > + * newer kernel. The rest of the trailing bytes in @dst (@ksize - @usize)
> > + * are to be zero-filled.
> > + * * If @usize > @ksize, then the user space has passed a new struct to an
> > + * older kernel. The trailing bytes unknown to the kernel (@usize - @ksize)
> > + * are checked to ensure they are zeroed, otherwise -E2BIG is returned.
> > + *
> > + * Returns (in all cases, some data may have been copied):
> > + * * -E2BIG: (@usize > @ksize) and there are non-zero trailing bytes in @src.
> > + * * -E2BIG: @usize is "too big" (at time of writing, >PAGE_SIZE).
> > + * * -EFAULT: access to user space failed.
> > + */
> > +int copy_struct_from_user(void *dst, size_t ksize,
> > + const void __user *src, size_t usize)
> > +{
> > + size_t size = min(ksize, usize);
> > + size_t rest = abs(ksize - usize);
>
> As above.
>
> > + if (unlikely(usize > PAGE_SIZE))
> > + return -EFAULT;
>
> As above.
>
> > + if (unlikely(!access_ok(src, usize)))
> > + return -EFAULT;
> > +
> > + /* Deal with trailing bytes. */
> > + if (usize < ksize)
> > + memset(dst + size, 0, rest);
> > + else if (usize > ksize) {
> > + const void __user *addr = src + size;
> > + char buffer[BUFFER_SIZE] = {};
> > +
> > + while (rest > 0) {
> > + size_t bufsize = min(rest, sizeof(buffer));
> > +
> > + if (__copy_from_user(buffer, addr, bufsize))
> > + return -EFAULT;
> > + if (memchr_inv(buffer, 0, bufsize))
> > + return -E2BIG;
> > +
> > + addr += bufsize;
> > + rest -= bufsize;
> > + }
>
> I'd create a __user_is_zero() helper for this - that way the two
> branches in the two helpers become nicely symmetric, each just calling a
> single helper that deals appropriately with the tail. And we can discuss
> how to implement __user_is_zero() in another bikeshed.
Will do.
>
> > + }
> > + /* Copy the interoperable parts of the struct. */
> > + if (__copy_from_user(dst, src, size))
> > + return -EFAULT;
>
> If you do move up the __copy_to_user(), please move this as well - on
> the kernel side, we certainly don't care that we copied some bytes to a
> local buffer which we then ignore because the user had a non-zero tail.
> But if __copy_to_user() is kept last in copy_struct_to_user(), this
> should stay for symmetry.
I will keep that in mind.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.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
^ permalink raw reply
* Re: [PATCH] arm64: dts: mt8183: Add node for the Mali GPU
From: Nicolas Boichat @ 2019-09-05 9:49 UTC (permalink / raw)
To: Rob Herring
Cc: Mark Rutland, devicetree, linux-kernel@vger.kernel.org,
Boris Brezillon, moderated list:ARM/Mediatek SoC support,
Alyssa Rosenzweig, Matthias Brugger, Nick Fan,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <CAL_JsqJCO2G90TTT9Mpy4kjVKQyXWw4aXEEnbRp_SE8X=EGc5g@mail.gmail.com>
Thanks for the quick review!
On Thu, Sep 5, 2019 at 5:09 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Thu, Sep 5, 2019 at 9:16 AM Nicolas Boichat <drinkcat@chromium.org> wrote:
> >
> > Add a basic GPU node and opp table for mt8183.
> >
> > The binding we use with out-of-tree Mali drivers includes more
> > clocks, I assume this would be required eventually if we have an
> > in-tree driver:
>
> We have an in-tree driver...
Right but AFAICT it does not support Bifrost GPU (yet?).
>
> > clocks =
> > <&topckgen CLK_TOP_MFGPLL_CK>,
> > <&topckgen CLK_TOP_MUX_MFG>,
> > <&clk26m>,
> > <&mfgcfg CLK_MFG_BG3D>;
> > clock-names =
> > "clk_main_parent",
> > "clk_mux",
> > "clk_sub_parent",
> > "subsys_mfg_cg";
Do you think we should add those to the binding document? May not be
easy to match what the amlogic binding does (I'm not sure to
understand the details of this device, but I can dig further/ask).
> >
> > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> >
> > ---
> > Upstreaming what matches existing bindings from our Chromium OS tree:
> > https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-4.19/arch/arm64/boot/dts/mediatek/mt8183.dtsi#1348
> >
> > The evb part of this change depends on this patch to add PMIC dtsi:
> > https://patchwork.kernel.org/patch/10928161/
> >
> > arch/arm64/boot/dts/mediatek/mt8183-evb.dts | 7 ++
> > arch/arm64/boot/dts/mediatek/mt8183.dtsi | 103 ++++++++++++++++++++
> > 2 files changed, 110 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8183-evb.dts b/arch/arm64/boot/dts/mediatek/mt8183-evb.dts
> > index 1fb195c683c3d01..200d8e65a6368a1 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8183-evb.dts
> > +++ b/arch/arm64/boot/dts/mediatek/mt8183-evb.dts
> > @@ -7,6 +7,7 @@
> >
> > /dts-v1/;
> > #include "mt8183.dtsi"
> > +#include "mt6358.dtsi"
> >
> > / {
> > model = "MediaTek MT8183 evaluation board";
> > @@ -30,6 +31,12 @@
> > status = "okay";
> > };
> >
> > +&gpu {
> > + supply-names = "mali", "mali_sram";
> > + mali-supply = <&mt6358_vgpu_reg>;
> > + mali_sram-supply = <&mt6358_vsram_gpu_reg>;
>
> Not documented. Just 'sram-supply' is enough.
Will fix.
> Note that the binding doc queued up for 5.4 has been converted to DT schema.
Yep I see that in linux-next.
>
> > +};
> > +
> > &i2c0 {
> > pinctrl-names = "default";
> > pinctrl-0 = <&i2c_pins_0>;
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> > index 97f84aa9fc6e1c1..8ea548a762ea252 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> > +++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> > @@ -579,6 +579,109 @@
> > #clock-cells = <1>;
> > };
> >
> > + gpu: mali@13040000 {
>
> gpu@...
>
> > + compatible = "mediatek,mt8183-mali", "arm,mali-bifrost";
>
> You need to add this compatible string too.
Will do.
>
> > + reg = <0 0x13040000 0 0x4000>;
> > + interrupts =
> > + <GIC_SPI 280 IRQ_TYPE_LEVEL_LOW>,
> > + <GIC_SPI 279 IRQ_TYPE_LEVEL_LOW>,
> > + <GIC_SPI 278 IRQ_TYPE_LEVEL_LOW>;
> > + interrupt-names = "job", "mmu", "gpu";
> > +
> > + clocks = <&topckgen CLK_TOP_MFGPLL_CK>;
> > + power-domains =
> > + <&scpsys MT8183_POWER_DOMAIN_MFG_CORE0>,
> > + <&scpsys MT8183_POWER_DOMAIN_MFG_CORE1>,
> > + <&scpsys MT8183_POWER_DOMAIN_MFG_2D>;
>
> This needs to be documented too.
I see that Documentation/devicetree/bindings/gpu/arm,mali-midgard.yaml
has power-domains in the example both not in the yaml, is that
expected?
>
> > +
> > + operating-points-v2 = <&gpu_opp_table>;
> > + };
> > +
> > + gpu_opp_table: opp_table0 {
> > + compatible = "operating-points-v2";
> > + opp-shared;
> > +
> > + opp-300000000 {
> > + opp-hz = /bits/ 64 <300000000>;
> > + opp-microvolt = <625000>, <850000>;
> > + };
> > +
> > + opp-320000000 {
> > + opp-hz = /bits/ 64 <320000000>;
> > + opp-microvolt = <631250>, <850000>;
> > + };
> > +
> > + opp-340000000 {
> > + opp-hz = /bits/ 64 <340000000>;
> > + opp-microvolt = <637500>, <850000>;
> > + };
> > +
> > + opp-360000000 {
> > + opp-hz = /bits/ 64 <360000000>;
> > + opp-microvolt = <643750>, <850000>;
> > + };
> > +
> > + opp-380000000 {
> > + opp-hz = /bits/ 64 <380000000>;
> > + opp-microvolt = <650000>, <850000>;
> > + };
> > +
> > + opp-400000000 {
> > + opp-hz = /bits/ 64 <400000000>;
> > + opp-microvolt = <656250>, <850000>;
> > + };
> > +
> > + opp-420000000 {
> > + opp-hz = /bits/ 64 <420000000>;
> > + opp-microvolt = <662500>, <850000>;
> > + };
> > +
> > + opp-460000000 {
> > + opp-hz = /bits/ 64 <460000000>;
> > + opp-microvolt = <675000>, <850000>;
> > + };
> > +
> > + opp-500000000 {
> > + opp-hz = /bits/ 64 <500000000>;
> > + opp-microvolt = <687500>, <850000>;
> > + };
> > +
> > + opp-540000000 {
> > + opp-hz = /bits/ 64 <540000000>;
> > + opp-microvolt = <700000>, <850000>;
> > + };
> > +
> > + opp-580000000 {
> > + opp-hz = /bits/ 64 <580000000>;
> > + opp-microvolt = <712500>, <850000>;
> > + };
> > +
> > + opp-620000000 {
> > + opp-hz = /bits/ 64 <620000000>;
> > + opp-microvolt = <725000>, <850000>;
> > + };
> > +
> > + opp-653000000 {
> > + opp-hz = /bits/ 64 <653000000>;
> > + opp-microvolt = <743750>, <850000>;
> > + };
> > +
> > + opp-698000000 {
> > + opp-hz = /bits/ 64 <698000000>;
> > + opp-microvolt = <768750>, <868750>;
> > + };
> > +
> > + opp-743000000 {
> > + opp-hz = /bits/ 64 <743000000>;
> > + opp-microvolt = <793750>, <893750>;
> > + };
> > +
> > + opp-800000000 {
> > + opp-hz = /bits/ 64 <800000000>;
> > + opp-microvolt = <825000>, <925000>;
> > + };
>
> Okay, but I seriously doubt the OPP selection logic is sophisticated
> enough or will ever be to use all these levels...
>
> > + };
> > +
> > mmsys: syscon@14000000 {
> > compatible = "mediatek,mt8183-mmsys", "syscon";
> > reg = <0 0x14000000 0 0x1000>;
> > --
> > 2.23.0.187.g17f5b7556c-goog
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2] drm: bridge/dw_hdmi: add audio sample channel status setting
From: Cheng-yi Chiang @ 2019-09-05 9:45 UTC (permalink / raw)
To: linux-kernel
Cc: moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
kuninori.morimoto.gx, David Airlie, dri-devel, cain.cai,
Andrzej Hajda, Laurent Pinchart, Yakir Yang, sam, Xing Zheng,
linux-rockchip, Dylan Reid, tzungbi, Jonas Karlman, Jeffy Chen,
蔡枫, linux-arm-kernel, Jernej Skrabec, Doug Anderson,
Daniel Vetter, Enric Balletbo i Serra, kuankuan.y
In-Reply-To: <20190905094325.33156-1-cychiang@chromium.org>
Sorry for the noise.
Removed original author ykk@rock-chips.com from the thread because
that mail is obsolete.
Yakir is now using kuankuan.y@gmail.com.
On Thu, Sep 5, 2019 at 5:43 PM Cheng-Yi Chiang <cychiang@chromium.org> wrote:
>
> From: Yakir Yang <ykk@rock-chips.com>
>
> When transmitting IEC60985 linear PCM audio, we configure the
> Aduio Sample Channel Status information of all the channel
> status bits in the IEC60958 frame.
> Refer to 60958-3 page 10 for frequency, original frequency, and
> wordlength setting.
>
> This fix the issue that audio does not come out on some monitors
> (e.g. LG 22CV241)
>
> Note that these registers are only for interfaces:
> I2S audio interface, General Purpose Audio (GPA), or AHB audio DMA
> (AHBAUDDMA).
> For S/PDIF interface this information comes from the stream.
>
> Currently this function dw_hdmi_set_channel_status is only called
> from dw-hdmi-i2s-audio in I2S setup.
>
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
> ---
> Original patch by Yakir Yang is at
>
> https://lore.kernel.org/patchwork/patch/539653/
>
> Change from v1 to v2:
> 1. Remove the version check because this will only be called by
> dw-hdmi-i2s-audio, and the registers are available in I2S setup.
> 2. Set these registers in dw_hdmi_i2s_hw_params.
> 3. Fix the sample width setting so it can use 16 or 24 bits.
>
> .../drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 1 +
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 70 +++++++++++++++++++
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 20 ++++++
> include/drm/bridge/dw_hdmi.h | 2 +
> 4 files changed, 93 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
> index 34d8e837555f..b801a28b0f17 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
> @@ -102,6 +102,7 @@ static int dw_hdmi_i2s_hw_params(struct device *dev, void *data,
> }
>
> dw_hdmi_set_sample_rate(hdmi, hparms->sample_rate);
> + dw_hdmi_set_channel_status(hdmi, hparms->sample_width);
> dw_hdmi_set_channel_count(hdmi, hparms->channels);
> dw_hdmi_set_channel_allocation(hdmi, hparms->cea.channel_allocation);
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index bd65d0479683..d1daa369c8ae 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -582,6 +582,76 @@ static unsigned int hdmi_compute_n(unsigned int freq, unsigned long pixel_clk)
> return n;
> }
>
> +/*
> + * When transmitting IEC60958 linear PCM audio, these registers allow to
> + * configure the channel status information of all the channel status
> + * bits in the IEC60958 frame. For the moment this configuration is only
> + * used when the I2S audio interface, General Purpose Audio (GPA),
> + * or AHB audio DMA (AHBAUDDMA) interface is active
> + * (for S/PDIF interface this information comes from the stream).
> + */
> +void dw_hdmi_set_channel_status(struct dw_hdmi *hdmi,
> + unsigned int sample_width)
> +{
> + u8 aud_schnl_samplerate;
> + u8 aud_schnl_8;
> + u8 word_length_bits;
> +
> + switch (hdmi->sample_rate) {
> + case 32000:
> + aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_32K;
> + break;
> + case 44100:
> + aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_44K1;
> + break;
> + case 48000:
> + aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_48K;
> + break;
> + case 88200:
> + aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_88K2;
> + break;
> + case 96000:
> + aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_96K;
> + break;
> + case 176400:
> + aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_176K4;
> + break;
> + case 192000:
> + aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_192K;
> + break;
> + case 768000:
> + aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_768K;
> + break;
> + default:
> + dev_warn(hdmi->dev, "Unsupported audio sample rate (%u)\n",
> + hdmi->sample_rate);
> + return;
> + }
> +
> + /* set channel status register */
> + hdmi_modb(hdmi, aud_schnl_samplerate, HDMI_FC_AUDSCHNLS7_SMPRATE_MASK,
> + HDMI_FC_AUDSCHNLS7);
> +
> + /*
> + * Set original frequency to be the same as frequency.
> + * Use one-complement value as stated in IEC60958-3 page 13.
> + */
> + aud_schnl_8 = (~aud_schnl_samplerate) <<
> + HDMI_FC_AUDSCHNLS8_ORIGSAMPFREQ_OFFSET;
> +
> + /*
> + * Refer to IEC60958-3 page 12. We can accept 16 bits or 24 bits.
> + * Otherwise, set the register to 0t o indicate using default value.
> + */
> + word_length_bits = (sample_width == 16) ? 0x2 :
> + ((sample_width == 24) ? 0xb : 0);
> +
> + aud_schnl_8 |= word_length_bits << HDMI_FC_AUDSCHNLS8_WORDLEGNTH_OFFSET;
> +
> + hdmi_writeb(hdmi, aud_schnl_8, HDMI_FC_AUDSCHNLS8);
> +}
> +EXPORT_SYMBOL_GPL(dw_hdmi_set_channel_status);
> +
> static void hdmi_set_clk_regenerator(struct dw_hdmi *hdmi,
> unsigned long pixel_clk, unsigned int sample_rate)
> {
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
> index 6988f12d89d9..619ebc1c8354 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
> @@ -158,6 +158,17 @@
> #define HDMI_FC_SPDDEVICEINF 0x1062
> #define HDMI_FC_AUDSCONF 0x1063
> #define HDMI_FC_AUDSSTAT 0x1064
> +#define HDMI_FC_AUDSV 0x1065
> +#define HDMI_FC_AUDSU 0x1066
> +#define HDMI_FC_AUDSCHNLS0 0x1067
> +#define HDMI_FC_AUDSCHNLS1 0x1068
> +#define HDMI_FC_AUDSCHNLS2 0x1069
> +#define HDMI_FC_AUDSCHNLS3 0x106a
> +#define HDMI_FC_AUDSCHNLS4 0x106b
> +#define HDMI_FC_AUDSCHNLS5 0x106c
> +#define HDMI_FC_AUDSCHNLS6 0x106d
> +#define HDMI_FC_AUDSCHNLS7 0x106e
> +#define HDMI_FC_AUDSCHNLS8 0x106f
> #define HDMI_FC_DATACH0FILL 0x1070
> #define HDMI_FC_DATACH1FILL 0x1071
> #define HDMI_FC_DATACH2FILL 0x1072
> @@ -706,6 +717,15 @@ enum {
> /* HDMI_FC_AUDSCHNLS7 field values */
> HDMI_FC_AUDSCHNLS7_ACCURACY_OFFSET = 4,
> HDMI_FC_AUDSCHNLS7_ACCURACY_MASK = 0x30,
> + HDMI_FC_AUDSCHNLS7_SMPRATE_MASK = 0x0f,
> + HDMI_FC_AUDSCHNLS7_SMPRATE_192K = 0xe,
> + HDMI_FC_AUDSCHNLS7_SMPRATE_176K4 = 0xc,
> + HDMI_FC_AUDSCHNLS7_SMPRATE_96K = 0xa,
> + HDMI_FC_AUDSCHNLS7_SMPRATE_768K = 0x9,
> + HDMI_FC_AUDSCHNLS7_SMPRATE_88K2 = 0x8,
> + HDMI_FC_AUDSCHNLS7_SMPRATE_32K = 0x3,
> + HDMI_FC_AUDSCHNLS7_SMPRATE_48K = 0x2,
> + HDMI_FC_AUDSCHNLS7_SMPRATE_44K1 = 0x0,
>
> /* HDMI_FC_AUDSCHNLS8 field values */
> HDMI_FC_AUDSCHNLS8_ORIGSAMPFREQ_MASK = 0xf0,
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index cf528c289857..12144d2f80f4 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -156,6 +156,8 @@ void dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense);
>
> void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate);
> void dw_hdmi_set_channel_count(struct dw_hdmi *hdmi, unsigned int cnt);
> +void dw_hdmi_set_channel_status(struct dw_hdmi *hdmi,
> + unsigned int sample_width);
> void dw_hdmi_set_channel_allocation(struct dw_hdmi *hdmi, unsigned int ca);
> void dw_hdmi_audio_enable(struct dw_hdmi *hdmi);
> void dw_hdmi_audio_disable(struct dw_hdmi *hdmi);
> --
> 2.23.0.187.g17f5b7556c-goog
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH v2] drm: bridge/dw_hdmi: add audio sample channel status setting
From: Cheng-Yi Chiang @ 2019-09-05 9:43 UTC (permalink / raw)
To: linux-kernel
Cc: alsa-devel, kuninori.morimoto.gx, airlied, dri-devel, cain.cai,
a.hajda, Laurent.pinchart, Yakir Yang, sam, cychiang, zhengxing,
linux-rockchip, dgreid, tzungbi, jonas, jeffy.chen, eddie.cai,
linux-arm-kernel, jernej.skrabec, dianders, daniel,
enric.balletbo, kuankuan.y
From: Yakir Yang <ykk@rock-chips.com>
When transmitting IEC60985 linear PCM audio, we configure the
Aduio Sample Channel Status information of all the channel
status bits in the IEC60958 frame.
Refer to 60958-3 page 10 for frequency, original frequency, and
wordlength setting.
This fix the issue that audio does not come out on some monitors
(e.g. LG 22CV241)
Note that these registers are only for interfaces:
I2S audio interface, General Purpose Audio (GPA), or AHB audio DMA
(AHBAUDDMA).
For S/PDIF interface this information comes from the stream.
Currently this function dw_hdmi_set_channel_status is only called
from dw-hdmi-i2s-audio in I2S setup.
Signed-off-by: Yakir Yang <ykk@rock-chips.com>
Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
---
Original patch by Yakir Yang is at
https://lore.kernel.org/patchwork/patch/539653/
Change from v1 to v2:
1. Remove the version check because this will only be called by
dw-hdmi-i2s-audio, and the registers are available in I2S setup.
2. Set these registers in dw_hdmi_i2s_hw_params.
3. Fix the sample width setting so it can use 16 or 24 bits.
.../drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 1 +
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 70 +++++++++++++++++++
drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 20 ++++++
include/drm/bridge/dw_hdmi.h | 2 +
4 files changed, 93 insertions(+)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
index 34d8e837555f..b801a28b0f17 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
@@ -102,6 +102,7 @@ static int dw_hdmi_i2s_hw_params(struct device *dev, void *data,
}
dw_hdmi_set_sample_rate(hdmi, hparms->sample_rate);
+ dw_hdmi_set_channel_status(hdmi, hparms->sample_width);
dw_hdmi_set_channel_count(hdmi, hparms->channels);
dw_hdmi_set_channel_allocation(hdmi, hparms->cea.channel_allocation);
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index bd65d0479683..d1daa369c8ae 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -582,6 +582,76 @@ static unsigned int hdmi_compute_n(unsigned int freq, unsigned long pixel_clk)
return n;
}
+/*
+ * When transmitting IEC60958 linear PCM audio, these registers allow to
+ * configure the channel status information of all the channel status
+ * bits in the IEC60958 frame. For the moment this configuration is only
+ * used when the I2S audio interface, General Purpose Audio (GPA),
+ * or AHB audio DMA (AHBAUDDMA) interface is active
+ * (for S/PDIF interface this information comes from the stream).
+ */
+void dw_hdmi_set_channel_status(struct dw_hdmi *hdmi,
+ unsigned int sample_width)
+{
+ u8 aud_schnl_samplerate;
+ u8 aud_schnl_8;
+ u8 word_length_bits;
+
+ switch (hdmi->sample_rate) {
+ case 32000:
+ aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_32K;
+ break;
+ case 44100:
+ aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_44K1;
+ break;
+ case 48000:
+ aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_48K;
+ break;
+ case 88200:
+ aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_88K2;
+ break;
+ case 96000:
+ aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_96K;
+ break;
+ case 176400:
+ aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_176K4;
+ break;
+ case 192000:
+ aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_192K;
+ break;
+ case 768000:
+ aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_768K;
+ break;
+ default:
+ dev_warn(hdmi->dev, "Unsupported audio sample rate (%u)\n",
+ hdmi->sample_rate);
+ return;
+ }
+
+ /* set channel status register */
+ hdmi_modb(hdmi, aud_schnl_samplerate, HDMI_FC_AUDSCHNLS7_SMPRATE_MASK,
+ HDMI_FC_AUDSCHNLS7);
+
+ /*
+ * Set original frequency to be the same as frequency.
+ * Use one-complement value as stated in IEC60958-3 page 13.
+ */
+ aud_schnl_8 = (~aud_schnl_samplerate) <<
+ HDMI_FC_AUDSCHNLS8_ORIGSAMPFREQ_OFFSET;
+
+ /*
+ * Refer to IEC60958-3 page 12. We can accept 16 bits or 24 bits.
+ * Otherwise, set the register to 0t o indicate using default value.
+ */
+ word_length_bits = (sample_width == 16) ? 0x2 :
+ ((sample_width == 24) ? 0xb : 0);
+
+ aud_schnl_8 |= word_length_bits << HDMI_FC_AUDSCHNLS8_WORDLEGNTH_OFFSET;
+
+ hdmi_writeb(hdmi, aud_schnl_8, HDMI_FC_AUDSCHNLS8);
+}
+EXPORT_SYMBOL_GPL(dw_hdmi_set_channel_status);
+
static void hdmi_set_clk_regenerator(struct dw_hdmi *hdmi,
unsigned long pixel_clk, unsigned int sample_rate)
{
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
index 6988f12d89d9..619ebc1c8354 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
@@ -158,6 +158,17 @@
#define HDMI_FC_SPDDEVICEINF 0x1062
#define HDMI_FC_AUDSCONF 0x1063
#define HDMI_FC_AUDSSTAT 0x1064
+#define HDMI_FC_AUDSV 0x1065
+#define HDMI_FC_AUDSU 0x1066
+#define HDMI_FC_AUDSCHNLS0 0x1067
+#define HDMI_FC_AUDSCHNLS1 0x1068
+#define HDMI_FC_AUDSCHNLS2 0x1069
+#define HDMI_FC_AUDSCHNLS3 0x106a
+#define HDMI_FC_AUDSCHNLS4 0x106b
+#define HDMI_FC_AUDSCHNLS5 0x106c
+#define HDMI_FC_AUDSCHNLS6 0x106d
+#define HDMI_FC_AUDSCHNLS7 0x106e
+#define HDMI_FC_AUDSCHNLS8 0x106f
#define HDMI_FC_DATACH0FILL 0x1070
#define HDMI_FC_DATACH1FILL 0x1071
#define HDMI_FC_DATACH2FILL 0x1072
@@ -706,6 +717,15 @@ enum {
/* HDMI_FC_AUDSCHNLS7 field values */
HDMI_FC_AUDSCHNLS7_ACCURACY_OFFSET = 4,
HDMI_FC_AUDSCHNLS7_ACCURACY_MASK = 0x30,
+ HDMI_FC_AUDSCHNLS7_SMPRATE_MASK = 0x0f,
+ HDMI_FC_AUDSCHNLS7_SMPRATE_192K = 0xe,
+ HDMI_FC_AUDSCHNLS7_SMPRATE_176K4 = 0xc,
+ HDMI_FC_AUDSCHNLS7_SMPRATE_96K = 0xa,
+ HDMI_FC_AUDSCHNLS7_SMPRATE_768K = 0x9,
+ HDMI_FC_AUDSCHNLS7_SMPRATE_88K2 = 0x8,
+ HDMI_FC_AUDSCHNLS7_SMPRATE_32K = 0x3,
+ HDMI_FC_AUDSCHNLS7_SMPRATE_48K = 0x2,
+ HDMI_FC_AUDSCHNLS7_SMPRATE_44K1 = 0x0,
/* HDMI_FC_AUDSCHNLS8 field values */
HDMI_FC_AUDSCHNLS8_ORIGSAMPFREQ_MASK = 0xf0,
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index cf528c289857..12144d2f80f4 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -156,6 +156,8 @@ void dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense);
void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate);
void dw_hdmi_set_channel_count(struct dw_hdmi *hdmi, unsigned int cnt);
+void dw_hdmi_set_channel_status(struct dw_hdmi *hdmi,
+ unsigned int sample_width);
void dw_hdmi_set_channel_allocation(struct dw_hdmi *hdmi, unsigned int ca);
void dw_hdmi_audio_enable(struct dw_hdmi *hdmi);
void dw_hdmi_audio_disable(struct dw_hdmi *hdmi);
--
2.23.0.187.g17f5b7556c-goog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v12 01/12] lib: introduce copy_struct_{to,from}_user helpers
From: Peter Zijlstra @ 2019-09-05 9:43 UTC (permalink / raw)
To: Aleksa Sarai
Cc: linux-ia64, linux-sh, Alexander Shishkin, Rasmus Villemoes,
Alexei Starovoitov, linux-kernel, David Howells, linux-kselftest,
sparclinux, Jiri Olsa, linux-arch, linux-s390, Tycho Andersen,
Aleksa Sarai, Shuah Khan, Ingo Molnar, linux-arm-kernel,
linux-mips, linux-xtensa, Kees Cook, Arnd Bergmann, Jann Horn,
linuxppc-dev, linux-m68k, Al Viro, Andy Lutomirski, Shuah Khan,
Namhyung Kim, David Drysdale, Christian Brauner, J. Bruce Fields,
linux-parisc, linux-api, Chanho Min, Jeff Layton, Oleg Nesterov,
Eric Biederman, linux-alpha, linux-fsdevel, Andrew Morton,
Linus Torvalds, containers
In-Reply-To: <20190905092622.tlb6nn3uisssdfbu@yavin.dot.cyphar.com>
On Thu, Sep 05, 2019 at 07:26:22PM +1000, Aleksa Sarai wrote:
> On 2019-09-05, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Thu, Sep 05, 2019 at 06:19:22AM +1000, Aleksa Sarai wrote:
> > > +/**
> > > + * copy_struct_to_user: copy a struct to user space
> > > + * @dst: Destination address, in user space.
> > > + * @usize: Size of @dst struct.
> > > + * @src: Source address, in kernel space.
> > > + * @ksize: Size of @src struct.
> > > + *
> > > + * Copies a struct from kernel space to user space, in a way that guarantees
> > > + * backwards-compatibility for struct syscall arguments (as long as future
> > > + * struct extensions are made such that all new fields are *appended* to the
> > > + * old struct, and zeroed-out new fields have the same meaning as the old
> > > + * struct).
> > > + *
> > > + * @ksize is just sizeof(*dst), and @usize should've been passed by user space.
> > > + * The recommended usage is something like the following:
> > > + *
> > > + * SYSCALL_DEFINE2(foobar, struct foo __user *, uarg, size_t, usize)
> > > + * {
> > > + * int err;
> > > + * struct foo karg = {};
> > > + *
> > > + * // do something with karg
> > > + *
> > > + * err = copy_struct_to_user(uarg, usize, &karg, sizeof(karg));
> > > + * if (err)
> > > + * return err;
> > > + *
> > > + * // ...
> > > + * }
> > > + *
> > > + * There are three cases to consider:
> > > + * * If @usize == @ksize, then it's copied verbatim.
> > > + * * If @usize < @ksize, then kernel space is "returning" a newer struct to an
> > > + * older user space. In order to avoid user space getting incomplete
> > > + * information (new fields might be important), all trailing bytes in @src
> > > + * (@ksize - @usize) must be zerored
> >
> > s/zerored/zero/, right?
>
> It should've been "zeroed".
That reads wrong to me; that way it reads like this function must take
that action and zero out the 'rest'; which is just wrong.
This function must verify those bytes are zero, not make them zero.
> > > , otherwise -EFBIG is returned.
> >
> > 'Funny' that, copy_struct_from_user() below seems to use E2BIG.
>
> This is a copy of the semantics that sched_[sg]etattr(2) uses -- E2BIG for
> a "too big" struct passed to the kernel, and EFBIG for a "too big"
> struct passed to user-space. I would personally have preferred EMSGSIZE
> instead of EFBIG, but felt using the existing error codes would be less
> confusing.
Sadly a recent commit:
1251201c0d34 ("sched/core: Fix uclamp ABI bug, clean up and robustify sched_read_attr() ABI logic and code")
Made the situation even 'worse'.
> > > + if (unlikely(!access_ok(src, usize)))
> > > + return -EFAULT;
> > > +
> > > + /* Deal with trailing bytes. */
> > > + if (usize < ksize)
> > > + memset(dst + size, 0, rest);
> > > + else if (usize > ksize) {
> > > + const void __user *addr = src + size;
> > > + char buffer[BUFFER_SIZE] = {};
> >
> > Isn't that too big for on-stack?
>
> Is a 64-byte buffer too big? I picked the number "at random" to be the
> size of a cache line, but I could shrink it down to 32 bytes if the size
> is an issue (I wanted to avoid needless allocations -- hence it being
> on-stack).
Ah, my ctags gave me a definition of BUFFER_SIZE that was 512. I suppose
64 should be OK.
> > > +
> > > + while (rest > 0) {
> > > + size_t bufsize = min(rest, sizeof(buffer));
> > > +
> > > + if (__copy_from_user(buffer, addr, bufsize))
> > > + return -EFAULT;
> > > + if (memchr_inv(buffer, 0, bufsize))
> > > + return -E2BIG;
> > > +
> > > + addr += bufsize;
> > > + rest -= bufsize;
> > > + }
> >
> > The perf implementation uses get_user(); but if that is too slow, surely
> > we can do something with uaccess_try() here?
>
> Is there a non-x86-specific way to do that (unless I'm mistaken only x86
> has uaccess_try() or the other *_try() wrappers)? The main "performance
> improvement" (if you can even call it that) is that we use memchr_inv()
> which finds non-matching characters more efficiently than just doing a
> loop.
Oh, you're right, that's x86 only :/
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [V3, 2/2] media: i2c: Add Omnivision OV02A10 camera sensor driver
From: Dongchun Zhu @ 2019-09-05 9:41 UTC (permalink / raw)
To: Sakari Ailus
Cc: mark.rutland, devicetree, drinkcat, srv_heupstream, shengnan.wang,
tfiga, louis.kuo, sj.huang, robh+dt, linux-mediatek, matthias.bgg,
bingbu.cao, mchehab, linux-arm-kernel, linux-media
In-Reply-To: <20190819083009.GC6133@paasikivi.fi.intel.com>
Hi Sakari,
On Mon, 2019-08-19 at 11:30 +0300, Sakari Ailus wrote:
> Hi Dongchun,
>
> Thanks for the update.
>
> On Mon, Aug 19, 2019 at 11:43:31AM +0800, dongchun.zhu@mediatek.com wrote:
> > From: Dongchun Zhu <dongchun.zhu@mediatek.com>
> >
> > This patch adds a V4L2 sub-device driver for OV02A10 image sensor.
> > The OV02A10 is a 1/5" CMOS sensor from Omnivision,
> > which supports output format: 10-bit Raw.
> > The OV02A10 has a single MIPI lane interface and use the I2C bus
> > for control and the CSI-2 bus for data.
> >
> > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > ---
> > MAINTAINERS | 1 +
> > drivers/media/i2c/Kconfig | 11 +
> > drivers/media/i2c/Makefile | 1 +
> > drivers/media/i2c/ov02a10.c | 1018 +++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 1031 insertions(+)
> > create mode 100644 drivers/media/i2c/ov02a10.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 41734fb..4b714a2 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -11821,6 +11821,7 @@ M: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > L: linux-media@vger.kernel.org
> > T: git git://linuxtv.org/media_tree.git
> > S: Maintained
> > +F: drivers/media/i2c/ov02a10.c
> > F: Documentation/devicetree/bindings/media/i2c/ov02a10.txt
> >
> > OMNIVISION OV2680 SENSOR DRIVER
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index 79ce9ec..d063a82 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -617,6 +617,17 @@ config VIDEO_IMX355
> > To compile this driver as a module, choose M here: the
> > module will be called imx355.
> >
> > +config VIDEO_OV02A10
> > + tristate "OmniVision OV02A10 sensor support"
> > + depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
> > + depends on MEDIA_CAMERA_SUPPORT
> > + help
> > + This is a Video4Linux2 sensor driver for the OmniVision
> > + OV02A10 camera.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called ov02a10.
> > +
> > config VIDEO_OV2640
> > tristate "OmniVision OV2640 sensor support"
> > depends on VIDEO_V4L2 && I2C
> > diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> > index fd4ea86..f34a7ac 100644
> > --- a/drivers/media/i2c/Makefile
> > +++ b/drivers/media/i2c/Makefile
> > @@ -71,6 +71,7 @@ obj-$(CONFIG_VIDEO_OV5645) += ov5645.o
> > obj-$(CONFIG_VIDEO_OV5647) += ov5647.o
> > obj-$(CONFIG_VIDEO_OV5670) += ov5670.o
> > obj-$(CONFIG_VIDEO_OV5695) += ov5695.o
> > +obj-$(CONFIG_VIDEO_OV02A10) += ov02a10.o
> > obj-$(CONFIG_VIDEO_OV6650) += ov6650.o
> > obj-$(CONFIG_VIDEO_OV7251) += ov7251.o
> > obj-$(CONFIG_VIDEO_OV7640) += ov7640.o
> > diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c
> > new file mode 100644
> > index 0000000..ff5460a
> > --- /dev/null
> > +++ b/drivers/media/i2c/ov02a10.c
> > @@ -0,0 +1,1018 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2018 MediaTek Inc.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/device.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <media/media-entity.h>
> > +#include <media/v4l2-async.h>
> > +#include <media/v4l2-ctrls.h>
> > +#include <media/v4l2-subdev.h>
> > +
> > +#define CHIP_ID 0x2509
> > +#define OV02A10_REG_CHIP_ID_H 0x02
> > +#define OV02A10_REG_CHIP_ID_L 0x03
> > +#define OV02A10_ID(_msb, _lsb) ((_msb) << 8 | (_lsb))
> > +
> > +/* Bit[1] vertical upside down */
> > +/* Bit[0] horizontal mirror */
> > +#define REG_MIRROR_FLIP_CONTROL 0x3f
> > +
> > +/* Orientation */
> > +#define REG_CONFIG_MIRROR_FLIP 0x03
> > +
> > +#define REG_PAGE_SWITCH 0xfd
> > +#define REG_GLOBAL_EFFECTIVE 0x01
> > +#define REG_ENABLE BIT(0)
> > +
> > +#define REG_SC_CTRL_MODE 0xac
> > +#define SC_CTRL_MODE_STANDBY 0x00
> > +#define SC_CTRL_MODE_STREAMING 0x01
> > +
> > +#define OV02A10_REG_EXPOSURE_H 0x03
> > +#define OV02A10_REG_EXPOSURE_L 0x04
> > +#define OV02A10_EXPOSURE_MIN 4
> > +#define OV02A10_EXPOSURE_STEP 1
> > +
> > +#define OV02A10_REG_VTS_H 0x05
> > +#define OV02A10_REG_VTS_L 0x06
> > +#define OV02A10_VTS_MAX 0x209f
> > +#define OV02A10_VTS_MIN 0x04cf
> > +#define OV02A10_BASIC_LINE 1224
> > +
> > +#define OV02A10_REG_GAIN 0x24
> > +#define OV02A10_GAIN_MIN 0x10
> > +#define OV02A10_GAIN_MAX 0xf8
> > +#define OV02A10_GAIN_STEP 0x01
> > +#define OV02A10_GAIN_DEFAULT 0x40
> > +
> > +#define REG_NULL 0xff
> > +
> > +#define OV02A10_LANES 1
> > +#define OV02A10_BITS_PER_SAMPLE 10
> > +
> > +static const char * const ov02a10_supply_names[] = {
> > + "dovdd", /* Digital I/O power */
> > + "avdd", /* Analog power */
> > + "dvdd", /* Digital core power */
> > +};
> > +
> > +#define OV02A10_NUM_SUPPLIES ARRAY_SIZE(ov02a10_supply_names)
> > +
> > +struct regval {
> > + u16 addr;
> > + u8 val;
> > +};
> > +
> > +struct ov02a10_mode {
> > + u32 width;
> > + u32 height;
> > + u32 exp_def;
> > + u32 hts_def;
> > + u32 vts_def;
> > + const struct regval *reg_list;
> > +};
> > +
> > +struct ov02a10 {
> > + struct clk *xvclk;
> > + struct gpio_desc *powerdown_gpio;
> > + struct gpio_desc *reset_gpio;
> > + struct regulator_bulk_data supplies[OV02A10_NUM_SUPPLIES];
> > +
> > + bool streaming;
> > + bool upside_down;
> > +
> > + /*
> > + * Serialize control access, get/set format, get selection
> > + * and start streaming.
> > + */
> > + struct mutex mutex;
> > + struct v4l2_subdev subdev;
> > + struct media_pad pad;
> > + struct v4l2_ctrl *anal_gain;
> > + struct v4l2_ctrl *exposure;
> > + struct v4l2_ctrl *hblank;
> > + struct v4l2_ctrl *vblank;
> > + struct v4l2_ctrl *hflip;
> > + struct v4l2_ctrl *vflip;
> > + struct v4l2_ctrl *test_pattern;
> > + struct v4l2_mbus_framefmt fmt;
> > + struct v4l2_ctrl_handler ctrl_handler;
> > +
> > + const struct ov02a10_mode *cur_mode;
> > +};
> > +
> > +#define to_ov02a10(sd) container_of(sd, struct ov02a10, subdev)
> > +
> > +static inline void msleep_range(unsigned int delay_base)
> > +{
> > + usleep_range(delay_base * 1000, delay_base * 1000 + 500);
> > +}
> > +
> > +/* MIPI color bar enable output */
> > +static const struct regval ov02a10_test_pattern_enable_regs[] = {
> > + {0xfd, 0x01},
> > + {0x0d, 0x00},
> > + {0xb6, 0x01},
> > + {0x01, 0x01},
> > + {0xfd, 0x01},
> > + {0xac, 0x01},
> > + {REG_NULL, 0x00}
> > +};
> > +
> > +/* MIPI color bar disable output */
> > +static const struct regval ov02a10_test_pattern_disable_regs[] = {
> > + {0xfd, 0x01},
> > + {0x0d, 0x00},
> > + {0xb6, 0x00},
> > + {0x01, 0x01},
> > + {0xfd, 0x01},
> > + {0xac, 0x01},
> > + {REG_NULL, 0x00}
> > +};
> > +
> > +/*
> > + * xvclk 24Mhz
> > + * pclk 39Mhz
> > + * linelength 934(0x3a6)
> > + * framelength 1390(0x56e)
> > + * grabwindow_width 1600
> > + * grabwindow_height 1200
> > + * max_framerate 30fps
> > + * mipi_datarate per lane 780Mbps
> > + */
> > +static const struct regval ov02a10_1600x1200_regs[] = {
> > + {0xfd, 0x01},
> > + {0xac, 0x00},
> > + {0xfd, 0x00},
> > + {0x2f, 0x29},
> > + {0x34, 0x00},
> > + {0x35, 0x21},
> > + {0x30, 0x15},
> > + {0x33, 0x01},
> > + {0xfd, 0x01},
> > + {0x44, 0x00},
> > + {0x2a, 0x4c},
> > + {0x2b, 0x1e},
> > + {0x2c, 0x60},
> > + {0x25, 0x11},
> > + {0x03, 0x01},
> > + {0x04, 0xae},
> > + {0x09, 0x00},
> > + {0x0a, 0x02},
> > + {0x06, 0xa6},
> > + {0x31, 0x00},
> > + {0x24, 0x40},
> > + {0x01, 0x01},
> > + {0xfb, 0x73},
> > + {0xfd, 0x01},
> > + {0x16, 0x04},
> > + {0x1c, 0x09},
> > + {0x21, 0x42},
> > + {0x12, 0x04},
> > + {0x13, 0x10},
> > + {0x11, 0x40},
> > + {0x33, 0x81},
> > + {0xd0, 0x00},
> > + {0xd1, 0x01},
> > + {0xd2, 0x00},
> > + {0x50, 0x10},
> > + {0x51, 0x23},
> > + {0x52, 0x20},
> > + {0x53, 0x10},
> > + {0x54, 0x02},
> > + {0x55, 0x20},
> > + {0x56, 0x02},
> > + {0x58, 0x48},
> > + {0x5d, 0x15},
> > + {0x5e, 0x05},
> > + {0x66, 0x66},
> > + {0x68, 0x68},
> > + {0x6b, 0x00},
> > + {0x6c, 0x00},
> > + {0x6f, 0x40},
> > + {0x70, 0x40},
> > + {0x71, 0x0a},
> > + {0x72, 0xf0},
> > + {0x73, 0x10},
> > + {0x75, 0x80},
> > + {0x76, 0x10},
> > + {0x84, 0x00},
> > + {0x85, 0x10},
> > + {0x86, 0x10},
> > + {0x87, 0x00},
> > + {0x8a, 0x22},
> > + {0x8b, 0x22},
> > + {0x19, 0xf1},
> > + {0x29, 0x01},
> > + {0xfd, 0x01},
> > + {0x9d, 0xd6},
> > + {0xa0, 0x29},
> > + {0xa1, 0x03},
> > + {0xad, 0x62},
> > + {0xae, 0x00},
> > + {0xaf, 0x85},
> > + {0xb1, 0x01},
> > + {0x8e, 0x06},
> > + {0x8f, 0x40},
> > + {0x90, 0x04},
> > + {0x91, 0xb0},
> > + {0x45, 0x01},
> > + {0x46, 0x00},
> > + {0x47, 0x6c},
> > + {0x48, 0x03},
> > + {0x49, 0x8b},
> > + {0x4a, 0x00},
> > + {0x4b, 0x07},
> > + {0x4c, 0x04},
> > + {0x4d, 0xb7},
> > + {0xf0, 0x40},
> > + {0xf1, 0x40},
> > + {0xf2, 0x40},
> > + {0xf3, 0x40},
> > + {0x3f, 0x00},
> > + {0xfd, 0x01},
> > + {0x05, 0x00},
> > + {0x06, 0xa6},
> > + {0xfd, 0x01},
> > + {REG_NULL, 0x00}
> > +};
> > +
> > +#define OV02A10_LINK_FREQ_390MHZ 390000000
> > +static const s64 link_freq_menu_items[] = {
> > + OV02A10_LINK_FREQ_390MHZ
> > +};
> > +
> > +static const char * const ov02a10_test_pattern_menu[] = {
> > + "Disabled",
> > + "Color Bar",
> > +};
> > +
> > +static const struct ov02a10_mode supported_modes[] = {
> > + {
> > + .width = 1600,
> > + .height = 1200,
> > + .exp_def = 0x01ae,
> > + .hts_def = 0x03a6,
> > + .vts_def = 0x056e,
> > + .reg_list = ov02a10_1600x1200_regs,
> > + },
> > +};
> > +
> > +/* Write a register */
> > +static int ov02a10_write_reg(struct ov02a10 *ov02a10, u8 addr, u8 val)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > + u8 buf[2] = {addr, val};
> > + int ret;
> > +
> > + ret = i2c_master_send(client, buf, 2);
> > +
> > + if (ret != 2) {
> > + dev_err(&client->dev, "%s: error: reg=%x, val=%x\n",
> > + __func__, addr, val);
> > + return -EIO;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int ov02a10_write_array(struct ov02a10 *ov02a10,
> > + const struct regval *regs)
> > +{
> > + u32 i;
>
> unsigned int, please.
>
Fixed in next release.
> > + int ret;
> > +
> > + for (i = 0; regs[i].addr != REG_NULL; i++) {
> > + ret = ov02a10_write_reg(ov02a10, regs[i].addr, regs[i].val);
> > + if (ret < 0)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/* Read a register */
> > +static int ov02a10_read_reg(struct ov02a10 *ov02a10, u8 reg, u8 *val)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > + u8 data = reg;
> > + struct i2c_msg msg = {
> > + .addr = client->addr,
> > + .flags = 0,
> > + .len = 1,
> > + .buf = &data,
> > + };
> > + int ret;
> > +
> > + ret = i2c_transfer(client->adapter, &msg, 1);
> > + if (ret < 0)
> > + goto err_wr;
> > +
> > + msg.flags = I2C_M_RD;
> > + ret = i2c_transfer(client->adapter, &msg, 1);
> > + if (ret < 0)
> > + goto err_rd;
> > +
> > + *val = data;
> > + return 0;
> > +
> > +err_rd:
> > + dev_err(&client->dev, "i2c_transfer --I2C_M_RD failed\n");
> > +err_wr:
> > + dev_err(&client->dev, "read error: reg=0x%02x: %d\n", reg, ret);
> > + return ret;
> > +}
> > +
> > +static void ov02a10_fill_fmt(const struct ov02a10_mode *mode,
> > + struct v4l2_mbus_framefmt *fmt)
> > +{
> > + fmt->width = mode->width;
> > + fmt->height = mode->height;
> > + fmt->field = V4L2_FIELD_NONE;
> > +}
> > +
> > +static int ov02a10_set_fmt(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_format *fmt)
> > +{
> > + struct ov02a10 *ov02a10 = to_ov02a10(sd);
> > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;
> > +
> > + mutex_lock(&ov02a10->mutex);
> > +
> > + if (ov02a10->streaming) {
> > + mutex_unlock(&ov02a10->mutex);
> > + return -EBUSY;
> > + }
> > +
> > + /* Only one sensor mode supported */
> > + mbus_fmt->code = ov02a10->fmt.code;
> > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt);
> > + ov02a10->fmt = fmt->format;
> > +
> > + mutex_unlock(&ov02a10->mutex);
> > +
> > + return 0;
> > +}
> > +
> > +static int ov02a10_get_fmt(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_format *fmt)
> > +{
> > + struct ov02a10 *ov02a10 = to_ov02a10(sd);
> > + struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;
> > +
> > + mutex_lock(&ov02a10->mutex);
> > +
> > + fmt->format = ov02a10->fmt;
> > + mbus_fmt->code = ov02a10->fmt.code;
> > + ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt);
> > +
> > + mutex_unlock(&ov02a10->mutex);
> > +
> > + return 0;
> > +}
> > +
> > +static int ov02a10_enum_mbus_code(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_mbus_code_enum *code)
> > +{
> > + struct ov02a10 *ov02a10 = to_ov02a10(sd);
> > +
> > + if (code->index >= ARRAY_SIZE(supported_modes) || !(code->index))
>
> This doesn't make sense. The supported modes, should more be added in the
> future, are presumably using the same mbus code. If you only have one, then
> non-zero code->index would result in -EINVAL (and not zero code->index).
>
Thanks for kind reminder.
!(code->index) would be removed in next release.
> > + return -EINVAL;
> > +
> > + code->code = ov02a10->fmt.code;
> > +
> > + return 0;
> > +}
> > +
> > +static int ov02a10_enum_frame_sizes(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_pad_config *cfg,
> > + struct v4l2_subdev_frame_size_enum *fse)
> > +{
> > + if (fse->index >= ARRAY_SIZE(supported_modes) || !(fse->index))
> > + return -EINVAL;
> > +
> > + fse->min_width = supported_modes[fse->index].width;
> > + fse->max_width = supported_modes[fse->index].width;
> > + fse->max_height = supported_modes[fse->index].height;
> > + fse->min_height = supported_modes[fse->index].height;
> > +
> > + return 0;
> > +}
> > +
> > +static int __ov02a10_power_on(struct ov02a10 *ov02a10)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > + struct device *dev = &client->dev;
> > + int ret;
> > +
> > + ret = clk_prepare_enable(ov02a10->xvclk);
> > + if (ret < 0) {
> > + dev_err(dev, "Failed to enable xvclk\n");
> > + return ret;
> > + }
> > +
> > + /* Note: set 0 is high, set 1 is low */
> > + gpiod_set_value_cansleep(ov02a10->reset_gpio, 1);
> > + gpiod_set_value_cansleep(ov02a10->powerdown_gpio, 0);
> > +
> > + ret = regulator_bulk_enable(OV02A10_NUM_SUPPLIES, ov02a10->supplies);
> > + if (ret < 0) {
> > + dev_err(dev, "Failed to enable regulators\n");
> > + goto disable_clk;
> > + }
> > + msleep_range(7);
>
> This has some potential of clashing with more generic functions in the
> future. Please use usleep_range directly, or msleep.
>
Did you mean using usleep_range(7*1000, 8*1000), as used in patch v1?
https://patchwork.kernel.org/patch/10957225/
> > +
> > + gpiod_set_value_cansleep(ov02a10->powerdown_gpio, 1);
> > + msleep_range(10);
> > +
> > + gpiod_set_value_cansleep(ov02a10->reset_gpio, 0);
> > + msleep_range(10);
> > +
> > + return 0;
> > +
> > +disable_clk:
> > + clk_disable_unprepare(ov02a10->xvclk);
> > +
> > + return ret;
> > +}
> > +
> > +static void __ov02a10_power_off(struct ov02a10 *ov02a10)
> > +{
> > + clk_disable_unprepare(ov02a10->xvclk);
> > + gpiod_set_value_cansleep(ov02a10->reset_gpio, 1);
> > + gpiod_set_value_cansleep(ov02a10->powerdown_gpio, 1);
> > + regulator_bulk_disable(OV02A10_NUM_SUPPLIES, ov02a10->supplies);
> > +}
> > +
> > +static int __ov02a10_start_stream(struct ov02a10 *ov02a10)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > + int ret;
> > +
> > + /* Apply default values of current mode */
> > + ret = ov02a10_write_array(ov02a10, ov02a10->cur_mode->reg_list);
> > + if (ret)
> > + return ret;
> > +
> > + /* Apply customized values from user */
> > + ret = __v4l2_ctrl_handler_setup(ov02a10->subdev.ctrl_handler);
> > + if (ret)
> > + return ret;
> > +
> > + /* Set orientation to 180 degree */
> > + if (ov02a10->upside_down) {
> > + ret = ov02a10_write_reg(ov02a10, REG_MIRROR_FLIP_CONTROL,
> > + REG_CONFIG_MIRROR_FLIP);
> > + if (ret) {
> > + dev_err(&client->dev, "%s failed to set orientation\n",
> > + __func__);
> > + return ret;
> > + }
> > + ret = ov02a10_write_reg(ov02a10, REG_GLOBAL_EFFECTIVE,
> > + REG_ENABLE);
> > + if (ret < 0)
> > + return ret;
> > + }
> > +
> > + /* Set stream on register */
> > + return ov02a10_write_reg(ov02a10,
> > + REG_SC_CTRL_MODE, SC_CTRL_MODE_STREAMING);
> > +}
> > +
> > +static int __ov02a10_stop_stream(struct ov02a10 *ov02a10)
> > +{
> > + return ov02a10_write_reg(ov02a10,
> > + REG_SC_CTRL_MODE, SC_CTRL_MODE_STANDBY);
> > +}
> > +
> > +static int ov02a10_entity_init_cfg(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_pad_config *cfg)
> > +{
> > + struct v4l2_subdev_format fmt = { 0 };
> > +
> > + fmt.which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
> > + fmt.format.width = 1600;
> > + fmt.format.height = 1200;
> > +
> > + ov02a10_set_fmt(subdev, cfg, &fmt);
> > +
> > + return 0;
> > +}
> > +
> > +static int ov02a10_s_stream(struct v4l2_subdev *sd, int on)
> > +{
> > + struct ov02a10 *ov02a10 = to_ov02a10(sd);
> > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > + int ret = 0;
> > +
> > + mutex_lock(&ov02a10->mutex);
> > +
> > + if (ov02a10->streaming == on)
> > + goto unlock_and_return;
> > +
> > + if (on) {
> > + ret = pm_runtime_get_sync(&client->dev);
> > + if (ret < 0) {
> > + pm_runtime_put_noidle(&client->dev);
> > + goto unlock_and_return;
> > + }
> > +
> > + ret = __ov02a10_start_stream(ov02a10);
> > + if (ret) {
> > + __ov02a10_stop_stream(ov02a10);
> > + ov02a10->streaming = !on;
> > + goto err_rpm_put;
> > + }
> > + } else {
> > + __ov02a10_stop_stream(ov02a10);
> > + pm_runtime_put(&client->dev);
> > + }
> > +
> > + ov02a10->streaming = on;
> > + mutex_unlock(&ov02a10->mutex);
> > +
> > + return ret;
> > +
> > +err_rpm_put:
> > + pm_runtime_put(&client->dev);
> > +unlock_and_return:
> > + mutex_unlock(&ov02a10->mutex);
> > +
> > + return ret;
> > +}
> > +
> > +static int ov02a10_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> > +{
> > + struct ov02a10 *ov02a10 = to_ov02a10(sd);
> > + struct v4l2_mbus_framefmt *try_fmt = v4l2_subdev_get_try_format(sd,
> > + fh->pad,
> > + 0);
> > +
> > + mutex_lock(&ov02a10->mutex);
> > + /* Initialize try_fmt */
> > + try_fmt->code = ov02a10->fmt.code;
> > + ov02a10_fill_fmt(&supported_modes[0], try_fmt);
> > +
> > + mutex_unlock(&ov02a10->mutex);
>
> No need for the mutex: the supported values are static and there is no
> parallel access to try_fmt.
>
> Btw. you don't need to do this here as you have init_cfg implemented, i.e.
> you can omit the open callback.
>
Understood.
mutex here would be removed in next release.
> > +
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused ov02a10_runtime_resume(struct device *dev)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > + struct ov02a10 *ov02a10 = to_ov02a10(sd);
> > +
> > + return __ov02a10_power_on(ov02a10);
> > +}
> > +
> > +static int __maybe_unused ov02a10_runtime_suspend(struct device *dev)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > + struct ov02a10 *ov02a10 = to_ov02a10(sd);
> > +
> > + __ov02a10_power_off(ov02a10);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct dev_pm_ops ov02a10_pm_ops = {
> > + SET_RUNTIME_PM_OPS(ov02a10_runtime_suspend,
> > + ov02a10_runtime_resume, NULL)
> > +};
> > +
> > +static int ov02a10_set_test_pattern(struct ov02a10 *ov02a10, s32 value)
> > +{
> > + if (value)
> > + return ov02a10_write_array(ov02a10,
> > + ov02a10_test_pattern_enable_regs);
> > +
> > + return ov02a10_write_array(ov02a10,
> > + ov02a10_test_pattern_disable_regs);
>
> Fits on previous line.
>
Sorry for the typo.
Fixed in next release.
> > +}
> > +
> > +static int ov02a10_set_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > + struct ov02a10 *ov02a10 = container_of(ctrl->handler,
> > + struct ov02a10, ctrl_handler);
> > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > + s64 max_expo;
> > + int ret;
> > +
> > + /* Propagate change of current control to all related controls */
> > + if (ctrl->id == V4L2_CID_VBLANK) {
> > + /* Update max exposure while meeting expected vblanking */
> > + max_expo = ov02a10->cur_mode->height + ctrl->val - 4;
> > + __v4l2_ctrl_modify_range(ov02a10->exposure,
> > + ov02a10->exposure->minimum, max_expo,
> > + ov02a10->exposure->step,
> > + ov02a10->exposure->default_value);
> > + }
> > +
> > + /* V4L2 controls values will be applied only when power is already up */
> > + if (!pm_runtime_get_if_in_use(&client->dev))
> > + return 0;
> > +
> > + switch (ctrl->id) {
> > + case V4L2_CID_EXPOSURE:
> > + ret = ov02a10_write_reg(ov02a10, REG_PAGE_SWITCH, REG_ENABLE);
> > + if (ret < 0)
> > + return ret;
> > + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_EXPOSURE_H,
> > + ((ctrl->val >> 8) & 0xFF));
> > + if (!ret) {
> > + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_EXPOSURE_L,
> > + (ctrl->val & 0xFF));
> > + if (ret < 0)
> > + return ret;
> > + }
> > + ret = ov02a10_write_reg(ov02a10, REG_GLOBAL_EFFECTIVE,
> > + REG_ENABLE);
> > + if (ret < 0)
> > + return ret;
> > + break;
> > + case V4L2_CID_ANALOGUE_GAIN:
> > + ret = ov02a10_write_reg(ov02a10, REG_PAGE_SWITCH, REG_ENABLE);
> > + if (ret < 0)
> > + return ret;
> > + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_GAIN,
> > + (ctrl->val & 0xFF));
> > + if (ret < 0)
> > + return ret;
> > + ret = ov02a10_write_reg(ov02a10, REG_GLOBAL_EFFECTIVE,
> > + REG_ENABLE);
> > + if (ret < 0)
> > + return ret;
> > + break;
> > + case V4L2_CID_VBLANK:
> > + ret = ov02a10_write_reg(ov02a10, REG_PAGE_SWITCH, REG_ENABLE);
> > + if (ret < 0)
> > + return ret;
> > + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_VTS_H,
> > + (((ctrl->val +
> > + ov02a10->cur_mode->height -
> > + OV02A10_BASIC_LINE) >> 8)
> > + & 0xFF));
> > + if (!ret) {
> > + ret = ov02a10_write_reg(ov02a10, OV02A10_REG_VTS_L,
> > + ((ctrl->val +
> > + ov02a10->cur_mode->height -
> > + OV02A10_BASIC_LINE) & 0xFF));
> > + if (ret < 0)
> > + return ret;
> > + }
> > + ret = ov02a10_write_reg(ov02a10, REG_GLOBAL_EFFECTIVE,
> > + REG_ENABLE);
> > + if (ret < 0)
> > + return ret;
> > + break;
> > + case V4L2_CID_TEST_PATTERN:
> > + ret = ov02a10_set_test_pattern(ov02a10, ctrl->val);
> > + if (ret < 0)
> > + return ret;
> > + break;
> > + default:
> > + dev_warn(&client->dev, "%s Unhandled id:0x%x, val:0x%x\n",
> > + __func__, ctrl->id, ctrl->val);
> > + ret = -EINVAL;
> > + break;
> > + };
> > +
> > + pm_runtime_put(&client->dev);
> > +
> > + return ret;
> > +}
> > +
> > +static const struct v4l2_subdev_video_ops ov02a10_video_ops = {
> > + .s_stream = ov02a10_s_stream,
> > +};
> > +
> > +static const struct v4l2_subdev_pad_ops ov02a10_pad_ops = {
> > + .init_cfg = ov02a10_entity_init_cfg,
> > + .enum_mbus_code = ov02a10_enum_mbus_code,
> > + .enum_frame_size = ov02a10_enum_frame_sizes,
> > + .get_fmt = ov02a10_get_fmt,
> > + .set_fmt = ov02a10_set_fmt,
> > +};
> > +
> > +static const struct v4l2_subdev_ops ov02a10_subdev_ops = {
> > + .video = &ov02a10_video_ops,
> > + .pad = &ov02a10_pad_ops,
> > +};
> > +
> > +static const struct media_entity_operations ov02a10_subdev_entity_ops = {
> > + .link_validate = v4l2_subdev_link_validate,
> > +};
> > +
> > +static const struct v4l2_subdev_internal_ops ov02a10_internal_ops = {
> > + .open = ov02a10_open,
> > +};
> > +
> > +static const struct v4l2_ctrl_ops ov02a10_ctrl_ops = {
> > + .s_ctrl = ov02a10_set_ctrl,
> > +};
> > +
> > +static int ov02a10_initialize_controls(struct ov02a10 *ov02a10)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > + const struct ov02a10_mode *mode;
> > + struct v4l2_ctrl_handler *handler;
> > + struct v4l2_ctrl *ctrl;
> > + u64 exposure_max;
> > + u32 pixel_rate, h_blank;
> > + int ret;
> > +
> > + handler = &ov02a10->ctrl_handler;
> > + mode = ov02a10->cur_mode;
> > + ret = v4l2_ctrl_handler_init(handler, 10);
> > + if (ret)
> > + return ret;
>
> An extra newline here, please.
>
Sorry for the typo.
Fixed in next release.
> > + handler->lock = &ov02a10->mutex;
> > +
> > + ctrl = v4l2_ctrl_new_int_menu(handler, NULL, V4L2_CID_LINK_FREQ,
> > + 0, 0, link_freq_menu_items);
> > + if (ctrl)
> > + ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +
> > + pixel_rate = (link_freq_menu_items[0] * 2 * OV02A10_LANES) /
> > + OV02A10_BITS_PER_SAMPLE;
>
> Please use do_div(). The values are static so the compiler may not emit the
> instructions for division but better be safe than sorry.
>
Fixed in next release.
> > + v4l2_ctrl_new_std(handler, NULL, V4L2_CID_PIXEL_RATE,
> > + 0, pixel_rate, 1, pixel_rate);
> > +
> > + h_blank = mode->hts_def - mode->width;
> > + ov02a10->hblank = v4l2_ctrl_new_std(handler, NULL, V4L2_CID_HBLANK,
> > + h_blank, h_blank, 1, h_blank);
> > + if (ov02a10->hblank)
> > + ov02a10->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +
> > + ov02a10->vblank = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops,
> > + V4L2_CID_VBLANK, mode->vts_def -
> > + mode->height,
> > + OV02A10_VTS_MAX - mode->height, 1,
> > + mode->vts_def - mode->height);
> > +
> > + exposure_max = mode->vts_def - 4;
> > + ov02a10->exposure = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops,
> > + V4L2_CID_EXPOSURE,
> > + OV02A10_EXPOSURE_MIN,
> > + exposure_max,
> > + OV02A10_EXPOSURE_STEP,
> > + mode->exp_def);
> > +
> > + ov02a10->anal_gain = v4l2_ctrl_new_std(handler, &ov02a10_ctrl_ops,
> > + V4L2_CID_ANALOGUE_GAIN,
> > + OV02A10_GAIN_MIN,
> > + OV02A10_GAIN_MAX,
> > + OV02A10_GAIN_STEP,
> > + OV02A10_GAIN_DEFAULT);
> > +
> > + ov02a10->test_pattern =
> > + v4l2_ctrl_new_std_menu_items(handler,
>
> Indentation. If some of the arguments slip over 80 I guess we can live with
> that.
>
Got it.
Fixed in next release.
> > + &ov02a10_ctrl_ops,
> > + V4L2_CID_TEST_PATTERN,
> > + ARRAY_SIZE(ov02a10_test_pattern_menu) -
> > + 1, 0, 0, ov02a10_test_pattern_menu);
> > +
> > + if (handler->error) {
> > + ret = handler->error;
> > + dev_err(&client->dev,
> > + "Failed to init controls(%d)\n", ret);
>
> Fits on the previous line.
>
Fixed in next release.
> > + goto err_free_handler;
> > + }
> > +
> > + ov02a10->subdev.ctrl_handler = handler;
> > +
> > + return 0;
> > +
> > +err_free_handler:
> > + v4l2_ctrl_handler_free(handler);
> > +
> > + return ret;
> > +}
> > +
> > +static int ov02a10_check_sensor_id(struct ov02a10 *ov02a10)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > + u16 id;
> > + u8 pid = 0;
> > + u8 ver = 0;
> > + int ret;
> > +
> > + /* Check sensor revision */
> > + ret = ov02a10_read_reg(ov02a10, OV02A10_REG_CHIP_ID_H, &pid);
> > + if (ret)
> > + return ret;
> > +
> > + ret = ov02a10_read_reg(ov02a10, OV02A10_REG_CHIP_ID_L, &ver);
> > + if (ret)
> > + return ret;
> > +
> > + id = OV02A10_ID(pid, ver);
> > + if (id != CHIP_ID) {
> > + dev_err(&client->dev, "Unexpected sensor id(%04x)\n", id);
> > + return ret;
> > + }
> > +
> > + dev_info(&client->dev, "Detected OV%04X sensor\n", id);
> > +
> > + return 0;
> > +}
> > +
> > +static int ov02a10_configure_regulators(struct ov02a10 *ov02a10)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(&ov02a10->subdev);
> > + unsigned int i;
> > +
> > + for (i = 0; i < OV02A10_NUM_SUPPLIES; i++)
> > + ov02a10->supplies[i].supply = ov02a10_supply_names[i];
> > +
> > + return devm_regulator_bulk_get(&client->dev,
> > + OV02A10_NUM_SUPPLIES,
> > + ov02a10->supplies);
> > +}
> > +
> > +static int ov02a10_probe(struct i2c_client *client)
> > +{
> > + struct device *dev = &client->dev;
> > + struct ov02a10 *ov02a10;
> > + u32 rotation;
> > + u32 xclk_freq;
> > + int ret;
> > +
> > + ov02a10 = devm_kzalloc(dev, sizeof(*ov02a10), GFP_KERNEL);
> > + if (!ov02a10)
> > + return -ENOMEM;
> > +
> > + v4l2_i2c_subdev_init(&ov02a10->subdev, client, &ov02a10_subdev_ops);
> > + ov02a10->fmt.code = MEDIA_BUS_FMT_SBGGR10_1X10;
> > +
> > + /* Optional indication of physical rotation of sensor */
> > + ret = fwnode_property_read_u32(dev_fwnode(dev), "rotation",
> > + &rotation);
> > + if (!ret) {
> > + switch (rotation) {
> > + case 180:
> > + ov02a10->upside_down = true;
> > + ov02a10->fmt.code = MEDIA_BUS_FMT_SRGGB10_1X10;
> > + break;
> > + case 0:
> > + break;
> > + default:
> > + dev_warn(dev, "%u degrees rotation is not supported, ignoring...\n",
> > + rotation);
> > + }
> > + }
> > +
>
> Please also check that the link frequencies in DT match with what the
> driver expects. See e.g. drivers/media/i2c/ov8856.c driver for an example.
>
I would have a try and set one check rule for link frequencies.
> > + /* Get system clock (xvclk) */
> > + ov02a10->xvclk = devm_clk_get(dev, "xvclk");
> > + if (IS_ERR(ov02a10->xvclk)) {
> > + dev_err(dev, "Failed to get xvclk\n");
> > + return -EINVAL;
> > + }
> > +
> > + ret = of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq);
> > + if (ret) {
> > + dev_err(dev, "Failed to get xclk frequency\n");
> > + return ret;
> > + }
> > +
> > + /* External clock must be 24MHz, allow 1% tolerance */
> > + if (xclk_freq < 23760000 || xclk_freq > 24240000) {
> > + dev_err(dev, "external clock frequency %u is not supported\n",
> > + xclk_freq);
> > + return -EINVAL;
> > + }
> > + dev_dbg(dev, "external clock frequency %u\n", xclk_freq);
> > +
> > + ret = clk_set_rate(ov02a10->xvclk, xclk_freq);
> > + if (ret) {
> > + dev_err(dev, "Failed to set xvclk frequency (24MHz)\n");
> > + return ret;
> > + }
> > +
> > + ov02a10->powerdown_gpio = devm_gpiod_get(dev, "powerdown",
> > + GPIOD_OUT_LOW);
> > + if (IS_ERR(ov02a10->powerdown_gpio)) {
> > + dev_err(dev, "Failed to get powerdown-gpios\n");
> > + return -EINVAL;
> > + }
> > +
> > + ov02a10->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> > + if (IS_ERR(ov02a10->reset_gpio)) {
> > + dev_err(dev, "Failed to get reset-gpios\n");
> > + return -EINVAL;
> > + }
> > +
> > + ret = ov02a10_configure_regulators(ov02a10);
> > + if (ret) {
> > + dev_err(dev, "Failed to get power regulators\n");
> > + return ret;
> > + }
> > +
> > + mutex_init(&ov02a10->mutex);
> > + ov02a10->cur_mode = &supported_modes[0];
> > + ret = ov02a10_initialize_controls(ov02a10);
> > + if (ret) {
> > + dev_err(dev, "Failed to initialize controls\n");
> > + goto err_destroy_mutex;
> > + }
> > +
> > + ret = __ov02a10_power_on(ov02a10);
> > + if (ret)
> > + goto err_free_handler;
> > +
> > + ret = ov02a10_check_sensor_id(ov02a10);
> > + if (ret)
> > + goto err_power_off;
> > +
> > + ov02a10->subdev.internal_ops = &ov02a10_internal_ops;
> > + ov02a10->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > + ov02a10->subdev.entity.ops = &ov02a10_subdev_entity_ops;
> > + ov02a10->subdev.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> > + ov02a10->pad.flags = MEDIA_PAD_FL_SOURCE;
> > + ret = media_entity_pads_init(&ov02a10->subdev.entity, 1, &ov02a10->pad);
> > + if (ret < 0) {
> > + dev_err(dev, "failed to init entity pads: %d", ret);
> > + goto err_power_off;
> > + }
> > +
> > + ret = v4l2_async_register_subdev(&ov02a10->subdev);
> > + if (ret) {
> > + dev_err(dev, "failed to register V4L2 subdev: %d",
> > + ret);
> > + goto err_clean_entity;
> > + }
> > +
> > + pm_runtime_set_active(dev);
> > + pm_runtime_enable(dev);
> > + pm_runtime_idle(dev);
> > +
> > + dev_info(dev, "ov02a10 probe --\n");
>
> This isn't very informative. The driver already prints errors when probing
> fails and otherwise it succeeds. Please remove.
>
This would be removed in next release.
> > + return 0;
> > +
> > +err_clean_entity:
> > + media_entity_cleanup(&ov02a10->subdev.entity);
> > +err_power_off:
> > + __ov02a10_power_off(ov02a10);
> > +err_free_handler:
> > + v4l2_ctrl_handler_free(ov02a10->subdev.ctrl_handler);
> > +err_destroy_mutex:
> > + mutex_destroy(&ov02a10->mutex);
> > +
> > + return ret;
> > +}
> > +
> > +static int ov02a10_remove(struct i2c_client *client)
> > +{
> > + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> > + struct ov02a10 *ov02a10 = to_ov02a10(sd);
> > +
> > + v4l2_async_unregister_subdev(sd);
> > + media_entity_cleanup(&sd->entity);
> > + v4l2_ctrl_handler_free(sd->ctrl_handler);
> > + pm_runtime_disable(&client->dev);
> > + if (!pm_runtime_status_suspended(&client->dev))
> > + __ov02a10_power_off(ov02a10);
> > + pm_runtime_set_suspended(&client->dev);
> > + mutex_destroy(&ov02a10->mutex);
> > +
> > + return 0;
> > +}
> > +
> > +#if IS_ENABLED(CONFIG_OF)
> > +static const struct of_device_id ov02a10_of_match[] = {
> > + { .compatible = "ovti,ov02a10" },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, ov02a10_of_match);
> > +#endif
> > +
> > +static struct i2c_driver ov02a10_i2c_driver = {
> > + .driver = {
> > + .name = "ov02a10",
> > + .pm = &ov02a10_pm_ops,
> > + .of_match_table = ov02a10_of_match,
> > + },
> > + .probe_new = &ov02a10_probe,
> > + .remove = &ov02a10_remove,
> > +};
> > +
> > +module_i2c_driver(ov02a10_i2c_driver);
> > +
> > +MODULE_AUTHOR("Dongchun Zhu <dongchun.zhu@mediatek.com>");
> > +MODULE_DESCRIPTION("OmniVision OV02A10 sensor driver");
> > +MODULE_LICENSE("GPL v2");
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH 1/1] arm64: dts: renesas: hihope-common: Fix eMMC status
From: Simon Horman @ 2019-09-05 9:34 UTC (permalink / raw)
To: linux-renesas-soc
Cc: Fabrizio Castro, Simon Horman, Magnus Damm, linux-arm-kernel
In-Reply-To: <cover.1567675986.git.horms+renesas@verge.net.au>
From: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
SDHI3 got accidentally disabled while adding USB 2.0 support,
this patch fixes it.
Fixes: 734d277f412a ("arm64: dts: renesas: hihope-common: Add USB 2.0 support")
Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
arch/arm64/boot/dts/renesas/hihope-common.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/boot/dts/renesas/hihope-common.dtsi b/arch/arm64/boot/dts/renesas/hihope-common.dtsi
index 3311a982fff8..23fd0224ca90 100644
--- a/arch/arm64/boot/dts/renesas/hihope-common.dtsi
+++ b/arch/arm64/boot/dts/renesas/hihope-common.dtsi
@@ -279,6 +279,7 @@
mmc-hs200-1_8v;
non-removable;
fixed-emmc-driver-type = <1>;
+ status = "okay";
};
&usb_extal_clk {
--
2.11.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* [GIT PULL] Second Round of Renesas ARM Based SoC Fixes for v5.3
From: Simon Horman @ 2019-09-05 9:34 UTC (permalink / raw)
To: arm, soc
Cc: Arnd Bergmann, Kevin Hilman, Magnus Damm, linux-renesas-soc,
Olof Johansson, Simon Horman, linux-arm-kernel
Hi Olof, Hi Kevin, Hi Arnd,
Please consider these second round of Renesas ARM based SoC fixes for v5.3.
This pull request is based on the previous round of
such requests, tagged as renesas-next-20190813-v5.3-rc1,
which you have already pulled.
The following changes since commit 45f5d5a9e34d3fe4140a9a3b5f7ebe86c252440a:
arm64: dts: renesas: r8a77995: draak: Fix backlight regulator name (2019-08-09 11:58:17 -0700)
are available in the git repository at:
https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas.git tags/renesas-fixes2-for-v5.3
for you to fetch changes up to ae688e1720fd387de34f2140a735917411672bf1:
arm64: dts: renesas: hihope-common: Fix eMMC status (2019-08-31 11:23:18 +0200)
----------------------------------------------------------------
Second Round of Renesas ARM Based SoC Fixes for v5.3
* RZ/G2M based HiHope main board
- Re-enabled accidently disabled SDHI3 (eMMC) support
----------------------------------------------------------------
Fabrizio Castro (1):
arm64: dts: renesas: hihope-common: Fix eMMC status
arch/arm64/boot/dts/renesas/hihope-common.dtsi | 1 +
1 file changed, 1 insertion(+)
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH V3 0/6] PCI: tegra: Enable PCIe C5 controller of Tegra194 in p2972-0000 platform
From: Lorenzo Pieralisi @ 2019-09-05 9:34 UTC (permalink / raw)
To: Vidya Sagar
Cc: devicetree, mperttunen, mmaddireddy, kthota, gustavo.pimentel,
linux-kernel, kishon, linux-tegra, robh+dt, thierry.reding,
linux-pci, bhelgaas, andrew.murray, digetx, jonathanh,
linux-arm-kernel, sagar.tv
In-Reply-To: <7751a77d-5812-49b7-0c6b-00e6740e209b@nvidia.com>
On Thu, Sep 05, 2019 at 01:44:46PM +0530, Vidya Sagar wrote:
> Hi Lorenzo / Bjorn,
> Can you please review this series?
> I have Reviewed-by and Acked-by from Rob, Thierry and Andrew already.
Rebase it on top of my pci/tegra branch (it does not apply),
resend it and I will merge it.
Thanks,
Lorenzo
> Thanks,
> Vidya Sagar
>
> On 8/28/2019 10:58 PM, Vidya Sagar wrote:
> > This patch series enables Tegra194's C5 controller which owns x16 slot in
> > p2972-0000 platform. C5 controller's PERST# and CLKREQ# are not configured as
> > output and bi-directional signals by default and hence they need to be
> > configured explicitly. Also, x16 slot's 3.3V and 12V supplies are controlled
> > through GPIOs and hence they need to be enabled through regulator framework.
> > This patch series adds required infrastructural support to address both the
> > aforementioned requirements.
> > Testing done on p2972-0000 platform
> > - Able to enumerate devices connected to x16 slot (owned by C5 controller)
> > - Enumerated device's functionality verified
> > - Suspend-Resume sequence is verified with device connected to x16 slot
> >
> > V3:
> > * Addressed some more review comments from Andrew Murray and Thierry Reding
> >
> > V2:
> > * Changed the order of patches in the series for easy merging
> > * Addressed review comments from Thierry Reding and Andrew Murray
> >
> > Vidya Sagar (6):
> > dt-bindings: PCI: tegra: Add sideband pins configuration entries
> > dt-bindings: PCI: tegra: Add PCIe slot supplies regulator entries
> > PCI: tegra: Add support to configure sideband pins
> > PCI: tegra: Add support to enable slot regulators
> > arm64: tegra: Add configuration for PCIe C5 sideband signals
> > arm64: tegra: Add PCIe slot supply information in p2972-0000 platform
> >
> > .../bindings/pci/nvidia,tegra194-pcie.txt | 16 ++++
> > .../arm64/boot/dts/nvidia/tegra194-p2888.dtsi | 24 +++++
> > .../boot/dts/nvidia/tegra194-p2972-0000.dts | 4 +-
> > arch/arm64/boot/dts/nvidia/tegra194.dtsi | 38 +++++++-
> > drivers/pci/controller/dwc/pcie-tegra194.c | 94 ++++++++++++++++++-
> > 5 files changed, 172 insertions(+), 4 deletions(-)
> >
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 1/2] i2c: qcom-geni: Provide an option to select FIFO processing
From: Lee Jones @ 2019-09-05 9:34 UTC (permalink / raw)
To: Wolfram Sang
Cc: mark.rutland, devicetree, linux-kernel, linux-arm-msm, agross,
robh+dt, Bjorn Andersson, alokc, linux-i2c, linux-arm-kernel
In-Reply-To: <20190905091617.GC1157@kunai>
On Thu, 05 Sep 2019, Wolfram Sang wrote:
> > > It looks like a workaround to me. It would be interesting to hear which
> > > I2C client breaks with DMA and if it's driver can't be fixed somehow
> > > instead. But even if we agree on a workaround short term, adding a
>
> So, are there investigations running why this reboot happens?
Yes, but they have been running for months, literally.
Unfortunately, since these are production level platforms, all of the
usual low-level debugging avenues (JTAG) have been closed off. Also,
only a very small number of people have access to documentation, but
even those who are in possession are stumped.
Andy Gross did have one idea as to what might be happening, but it
turned out to be a red herring.
> > > Is there no other way to disable DMA which is local to this driver so we
> > > can easily revert the workaround later?
> >
> > This is the most local low-impact solution (nomenclature aside).
>
> I disagree. You could use of_machine_is_compatible() and disable DMA for
> that machine. Less impact because we save the workaround binding.
That could also work.
> > The beautiful thing about this approach is that, *if* the Geni SE DMA
>
> I'd say 'advantage' instead of 'beautiful' ;)
Okay, "the advantage thing about ..." ;)
> > ever starts working, we can remove the C code and any old properties
> > left in older DTs just become NOOP. Older kernels with newer DTs
> > (less of a priority) *still* won't work, but they don't work now
> > anyway.
>
> Which is a clear disadvantage of that solution. It won't fix older
> kernels. My suggestion above should fix them, too.
Not sure how this is possible. Unless you mean LTS?
> > The offending line can be found at [0]. There is no obvious bug to
> > fix and this code obviously works well on some of the hardware
> > platforms using it. But on our platform (Lenovo Yoga C630 - QCom
> > SMD850) that final command, which initiates the DMA transaction, ends
> > up rebooting the machine.
>
> Unless we know why the reboot happens on your platform, I'd be careful
> with saying "work obviously well" on other platforms.
Someone must have tested it? Surely ... ;)
> > With regards to the nomenclature, my original suggestion was
> > 'qcom,geni-se-no-dma'. Would that better suit your request?
>
> My suggestion:
>
> For 5.3, use of_machine_is_compatible() and we backport that. For later,
> try to find out the root cause and fix it. If that can't be done, try to
> set up a generic "disable-dma" property and use it.
>
> What do you think about that?
Sounds okay to me. Let me code that up.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 2/2] i2c: qcom-geni: Provide an option to disable DMA processing
From: Lee Jones @ 2019-09-05 9:28 UTC (permalink / raw)
To: Wolfram Sang
Cc: mark.rutland, devicetree, linux-kernel, linux-arm-msm, agross,
robh+dt, bjorn.andersson, vkoul, alokc, linux-i2c,
linux-arm-kernel
In-Reply-To: <20190905091800.GD1157@kunai>
On Thu, 05 Sep 2019, Wolfram Sang wrote:
>
> > Fixes: 8bc529b25354 ("soc: qcom: geni: Add support for ACPI")
>
> Are you sure? From visual inspection, I don't see a correlation between
> this commit and the fix here.
This patch should have been part of the commit, or at the very least,
part of the set, alluded to above. Unfortunately, I was carrying
Bjorn's hack which simply returned early from geni_se_rx_dma_prep()
with an error, so it masked the issue.
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > Reviewed-by: Vinod Koul <vkoul@kernel.org>
> > ---
> > drivers/i2c/busses/i2c-qcom-geni.c | 14 ++++++++++----
> > 1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> > index a89bfce5388e..8822dea82980 100644
> > --- a/drivers/i2c/busses/i2c-qcom-geni.c
> > +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> > @@ -353,13 +353,16 @@ static void geni_i2c_tx_fsm_rst(struct geni_i2c_dev *gi2c)
> > static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
> > u32 m_param)
> > {
> > + struct device_node *np = gi2c->se.dev->of_node;
> > dma_addr_t rx_dma;
> > unsigned long time_left;
> > - void *dma_buf;
> > + void *dma_buf = NULL;
> > struct geni_se *se = &gi2c->se;
> > size_t len = msg->len;
> >
> > - dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
> > + if (!of_property_read_bool(np, "qcom,geni-se-no-dma"))
> > + dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
> > +
> > if (dma_buf)
> > geni_se_select_mode(se, GENI_SE_DMA);
> > else
> > @@ -392,13 +395,16 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
> > static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
> > u32 m_param)
> > {
> > + struct device_node *np = gi2c->se.dev->of_node;
> > dma_addr_t tx_dma;
> > unsigned long time_left;
> > - void *dma_buf;
> > + void *dma_buf = NULL;
> > struct geni_se *se = &gi2c->se;
> > size_t len = msg->len;
> >
> > - dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
> > + if (!of_property_read_bool(np, "qcom,geni-se-no-dma"))
> > + dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
> > +
> > if (dma_buf)
> > geni_se_select_mode(se, GENI_SE_DMA);
> > else
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v12 01/12] lib: introduce copy_struct_{to,from}_user helpers
From: Aleksa Sarai @ 2019-09-05 9:26 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-ia64, linux-sh, Alexander Shishkin, Rasmus Villemoes,
Alexei Starovoitov, linux-kernel, David Howells, linux-kselftest,
sparclinux, Jiri Olsa, linux-arch, linux-s390, Tycho Andersen,
Aleksa Sarai, Shuah Khan, Ingo Molnar, linux-arm-kernel,
linux-mips, linux-xtensa, Kees Cook, Arnd Bergmann, Jann Horn,
linuxppc-dev, linux-m68k, Al Viro, Andy Lutomirski, Shuah Khan,
Namhyung Kim, David Drysdale, Christian Brauner, J. Bruce Fields,
linux-parisc, linux-api, Chanho Min, Jeff Layton, Oleg Nesterov,
Eric Biederman, linux-alpha, linux-fsdevel, Andrew Morton,
Linus Torvalds, containers
In-Reply-To: <20190905073205.GY2332@hirez.programming.kicks-ass.net>
[-- Attachment #1.1: Type: text/plain, Size: 7846 bytes --]
On 2019-09-05, Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, Sep 05, 2019 at 06:19:22AM +1000, Aleksa Sarai wrote:
> > +/**
> > + * copy_struct_to_user: copy a struct to user space
> > + * @dst: Destination address, in user space.
> > + * @usize: Size of @dst struct.
> > + * @src: Source address, in kernel space.
> > + * @ksize: Size of @src struct.
> > + *
> > + * Copies a struct from kernel space to user space, in a way that guarantees
> > + * backwards-compatibility for struct syscall arguments (as long as future
> > + * struct extensions are made such that all new fields are *appended* to the
> > + * old struct, and zeroed-out new fields have the same meaning as the old
> > + * struct).
> > + *
> > + * @ksize is just sizeof(*dst), and @usize should've been passed by user space.
> > + * The recommended usage is something like the following:
> > + *
> > + * SYSCALL_DEFINE2(foobar, struct foo __user *, uarg, size_t, usize)
> > + * {
> > + * int err;
> > + * struct foo karg = {};
> > + *
> > + * // do something with karg
> > + *
> > + * err = copy_struct_to_user(uarg, usize, &karg, sizeof(karg));
> > + * if (err)
> > + * return err;
> > + *
> > + * // ...
> > + * }
> > + *
> > + * There are three cases to consider:
> > + * * If @usize == @ksize, then it's copied verbatim.
> > + * * If @usize < @ksize, then kernel space is "returning" a newer struct to an
> > + * older user space. In order to avoid user space getting incomplete
> > + * information (new fields might be important), all trailing bytes in @src
> > + * (@ksize - @usize) must be zerored
>
> s/zerored/zero/, right?
It should've been "zeroed".
> > , otherwise -EFBIG is returned.
>
> 'Funny' that, copy_struct_from_user() below seems to use E2BIG.
This is a copy of the semantics that sched_[sg]etattr(2) uses -- E2BIG for
a "too big" struct passed to the kernel, and EFBIG for a "too big"
struct passed to user-space. I would personally have preferred EMSGSIZE
instead of EFBIG, but felt using the existing error codes would be less
confusing.
>
> > + * * If @usize > @ksize, then the kernel is "returning" an older struct to a
> > + * newer user space. The trailing bytes in @dst (@usize - @ksize) will be
> > + * zero-filled.
> > + *
> > + * Returns (in all cases, some data may have been copied):
> > + * * -EFBIG: (@usize < @ksize) and there are non-zero trailing bytes in @src.
> > + * * -EFAULT: access to user space failed.
> > + */
> > +int copy_struct_to_user(void __user *dst, size_t usize,
> > + const void *src, size_t ksize)
> > +{
> > + size_t size = min(ksize, usize);
> > + size_t rest = abs(ksize - usize);
> > +
> > + if (unlikely(usize > PAGE_SIZE))
> > + return -EFAULT;
>
> Not documented above. Implementation consistent with *from*, but see
> below.
Will update the kernel-doc.
> > + if (unlikely(!access_ok(dst, usize)))
> > + return -EFAULT;
> > +
> > + /* Deal with trailing bytes. */
> > + if (usize < ksize) {
> > + if (memchr_inv(src + size, 0, rest))
> > + return -EFBIG;
> > + } else if (usize > ksize) {
> > + if (__memzero_user(dst + size, rest))
> > + return -EFAULT;
> > + }
> > + /* Copy the interoperable parts of the struct. */
> > + if (__copy_to_user(dst, src, size))
> > + return -EFAULT;
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(copy_struct_to_user);
> > +
> > +/**
> > + * copy_struct_from_user: copy a struct from user space
> > + * @dst: Destination address, in kernel space. This buffer must be @ksize
> > + * bytes long.
> > + * @ksize: Size of @dst struct.
> > + * @src: Source address, in user space.
> > + * @usize: (Alleged) size of @src struct.
> > + *
> > + * Copies a struct from user space to kernel space, in a way that guarantees
> > + * backwards-compatibility for struct syscall arguments (as long as future
> > + * struct extensions are made such that all new fields are *appended* to the
> > + * old struct, and zeroed-out new fields have the same meaning as the old
> > + * struct).
> > + *
> > + * @ksize is just sizeof(*dst), and @usize should've been passed by user space.
> > + * The recommended usage is something like the following:
> > + *
> > + * SYSCALL_DEFINE2(foobar, const struct foo __user *, uarg, size_t, usize)
> > + * {
> > + * int err;
> > + * struct foo karg = {};
> > + *
> > + * err = copy_struct_from_user(&karg, sizeof(karg), uarg, size);
> > + * if (err)
> > + * return err;
> > + *
> > + * // ...
> > + * }
> > + *
> > + * There are three cases to consider:
> > + * * If @usize == @ksize, then it's copied verbatim.
> > + * * If @usize < @ksize, then the user space has passed an old struct to a
> > + * newer kernel. The rest of the trailing bytes in @dst (@ksize - @usize)
> > + * are to be zero-filled.
> > + * * If @usize > @ksize, then the user space has passed a new struct to an
> > + * older kernel. The trailing bytes unknown to the kernel (@usize - @ksize)
> > + * are checked to ensure they are zeroed, otherwise -E2BIG is returned.
> > + *
> > + * Returns (in all cases, some data may have been copied):
> > + * * -E2BIG: (@usize > @ksize) and there are non-zero trailing bytes in @src.
> > + * * -E2BIG: @usize is "too big" (at time of writing, >PAGE_SIZE).
> > + * * -EFAULT: access to user space failed.
> > + */
> > +int copy_struct_from_user(void *dst, size_t ksize,
> > + const void __user *src, size_t usize)
> > +{
> > + size_t size = min(ksize, usize);
> > + size_t rest = abs(ksize - usize);
> > +
> > + if (unlikely(usize > PAGE_SIZE))
> > + return -EFAULT;
>
> Documented above as returning -E2BIG.
I will switch this (and to) back to -E2BIG -- I must've had a brain-fart
when doing some refactoring.
>
> > + if (unlikely(!access_ok(src, usize)))
> > + return -EFAULT;
> > +
> > + /* Deal with trailing bytes. */
> > + if (usize < ksize)
> > + memset(dst + size, 0, rest);
> > + else if (usize > ksize) {
> > + const void __user *addr = src + size;
> > + char buffer[BUFFER_SIZE] = {};
>
> Isn't that too big for on-stack?
Is a 64-byte buffer too big? I picked the number "at random" to be the
size of a cache line, but I could shrink it down to 32 bytes if the size
is an issue (I wanted to avoid needless allocations -- hence it being
on-stack).
> > +
> > + while (rest > 0) {
> > + size_t bufsize = min(rest, sizeof(buffer));
> > +
> > + if (__copy_from_user(buffer, addr, bufsize))
> > + return -EFAULT;
> > + if (memchr_inv(buffer, 0, bufsize))
> > + return -E2BIG;
> > +
> > + addr += bufsize;
> > + rest -= bufsize;
> > + }
>
> The perf implementation uses get_user(); but if that is too slow, surely
> we can do something with uaccess_try() here?
Is there a non-x86-specific way to do that (unless I'm mistaken only x86
has uaccess_try() or the other *_try() wrappers)? The main "performance
improvement" (if you can even call it that) is that we use memchr_inv()
which finds non-matching characters more efficiently than just doing a
loop.
> > + }
> > + /* Copy the interoperable parts of the struct. */
> > + if (__copy_from_user(dst, src, size))
> > + return -EFAULT;
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(copy_struct_from_user);
>
> And personally I'm not a big fan of EXPORT_SYMBOL().
I don't have much of an opinion (after all, it only really makes sense a
lot of sense for syscalls) -- though out-of-tree modules that define
ioctl()s wouldn't be able to make use of them.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.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
^ permalink raw reply
* Re: [PATCH 1/1] KVM: inject data abort if instruction cannot be decoded
From: Daniel P. Berrangé @ 2019-09-05 9:23 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Peter Maydell, Suzuki K Pouloze, Marc Zyngier,
Heinrich Schuchardt, Julien Thierry, linux-kernel, James Morse,
kvmarm, linux-arm-kernel
In-Reply-To: <20190905092039.GG32415@stefanha-x1.localdomain>
On Thu, Sep 05, 2019 at 10:20:39AM +0100, Stefan Hajnoczi wrote:
> On Wed, Sep 04, 2019 at 08:07:36PM +0200, Heinrich Schuchardt wrote:
> > If an application tries to access memory that is not mapped, an error
> > ENOSYS, "load/store instruction decoding not implemented" may occur.
> > QEMU will hang with a register dump.
> >
> > Instead create a data abort that can be handled gracefully by the
> > application running in the virtual environment.
> >
> > Now the virtual machine can react to the event in the most appropriate
> > way - by recovering, by writing an informative log, or by rebooting.
> >
> > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > ---
> > virt/kvm/arm/mmio.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c
> > index a8a6a0c883f1..0cbed7d6a0f4 100644
> > --- a/virt/kvm/arm/mmio.c
> > +++ b/virt/kvm/arm/mmio.c
> > @@ -161,8 +161,8 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
> > if (ret)
> > return ret;
> > } else {
> > - kvm_err("load/store instruction decoding not implemented\n");
> > - return -ENOSYS;
> > + kvm_inject_dabt(vcpu, kvm_vcpu_get_hfar(vcpu));
> > + return 1;
>
> I see this more as a temporary debugging hack than something to merge.
>
> It sounds like in your case the guest environment provided good
> debugging information and you preferred it over debugging this from the
> host side. That's fine, but allowing the guest to continue running in
> the general case makes it much harder to track down the root cause of a
> problem because many guest CPU instructions may be executed after the
> original problem occurs. Other guest software may fail silently in
> weird ways. IMO it's best to fail early.
The current error message is quite limited in its usefulness - mostly
you have to be able to google the message and hope to hit a previous
report which explains the problem, or find someone on IRC who remembers
the problem, etc.
Could we put a text doc in the kernel tree explaining the problem in
enough detail that people can identify their next steps to resolve it,
and then make this error message tell people to read that text doc.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 1/1] KVM: inject data abort if instruction cannot be decoded
From: Christoffer Dall @ 2019-09-05 9:22 UTC (permalink / raw)
To: Peter Maydell
Cc: Daniel P . Berrangé, Marc Zyngier,
lkml - Kernel Mailing List, Stefan Hajnoczi, Heinrich Schuchardt,
kvmarm, arm-mail-list
In-Reply-To: <CAFEAcA9qkqkOTqSVrhTpt-NkZSNXomSBNiWo_D6Kr=QKYRRf=w@mail.gmail.com>
On Thu, Sep 05, 2019 at 09:56:44AM +0100, Peter Maydell wrote:
> On Thu, 5 Sep 2019 at 09:52, Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Thu, 05 Sep 2019 09:16:54 +0100,
> > Peter Maydell <peter.maydell@linaro.org> wrote:
> > > This is true, but the problem is that barfing out to userspace
> > > makes it harder to debug the guest because it means that
> > > the VM is immediately destroyed, whereas AIUI if we
> > > inject some kind of exception then (assuming you're set up
> > > to do kernel-debug via gdbstub) you can actually examine
> > > the offending guest code with a debugger because at least
> > > your VM is still around to inspect...
> >
> > To Christoffer's point, I find the benefit a bit dubious. Yes, you get
> > an exception, but the instruction that caused it may be completely
> > legal (store with post-increment, for example), leading to an even
> > more puzzled developer (that exception should never have been
> > delivered the first place).
>
> Right, but the combination of "host kernel prints a message
> about an unsupported load/store insn" and "within-guest debug
> dump/stack trace/etc" is much more useful than just having
> "host kernel prints message" and "QEMU exits"; and it requires
> about 3 lines of code change...
>
> > I'm far more in favour of dumping the state of the access in the run
> > structure (much like we do for a MMIO access) and let userspace do
> > something about it (such as dumping information on the console or
> > breaking). It could even inject an exception *if* the user has asked
> > for it.
>
> ...whereas this requires agreement on a kernel-userspace API,
> larger changes in the kernel, somebody to implement the userspace
> side of things, and the user to update both the kernel and QEMU.
> It's hard for me to see that the benefit here over the 3-line
> approach really outweighs the extra effort needed. In practice
> saying "we should do this" is saying "we're going to do nothing",
> based on the historical record.
>
How about something like the following (completely untested, liable for
ABI discussions etc. etc., but for illustration purposes).
I think it raises the question (and likely many other) of whether we can
break the existing 'ABI' and change behavior for missing ISV
retrospectively for legacy user space when the issue has occurred?
Someone might have written code that reacts to the -ENOSYS, so I've
taken the conservative approach for this for the time being.
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 8a37c8e89777..19a92c49039c 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -76,6 +76,14 @@ struct kvm_arch {
/* Mandated version of PSCI */
u32 psci_version;
+
+ /*
+ * If we encounter a data abort without valid instruction syndrome
+ * information, report this to user space. User space can (and
+ * should) opt in to this feature if KVM_CAP_ARM_NISV_TO_USER is
+ * supported.
+ */
+ bool return_nisv_io_abort_to_user;
};
#define KVM_NR_MEM_OBJS 40
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index f656169db8c3..019bc560edc1 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -83,6 +83,14 @@ struct kvm_arch {
/* Mandated version of PSCI */
u32 psci_version;
+
+ /*
+ * If we encounter a data abort without valid instruction syndrome
+ * information, report this to user space. User space can (and
+ * should) opt in to this feature if KVM_CAP_ARM_NISV_TO_USER is
+ * supported.
+ */
+ bool return_nisv_io_abort_to_user;
};
#define KVM_NR_MEM_OBJS 40
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 5e3f12d5359e..a4dd004d0db9 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -235,6 +235,7 @@ struct kvm_hyperv_exit {
#define KVM_EXIT_S390_STSI 25
#define KVM_EXIT_IOAPIC_EOI 26
#define KVM_EXIT_HYPERV 27
+#define KVM_EXIT_ARM_NISV 28
/* For KVM_EXIT_INTERNAL_ERROR */
/* Emulate instruction failed. */
@@ -996,6 +997,7 @@ struct kvm_ppc_resize_hpt {
#define KVM_CAP_ARM_PTRAUTH_ADDRESS 171
#define KVM_CAP_ARM_PTRAUTH_GENERIC 172
#define KVM_CAP_PMU_EVENT_FILTER 173
+#define KVM_CAP_ARM_NISV_TO_USER 174
#ifdef KVM_CAP_IRQ_ROUTING
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 35a069815baf..2ce94bd9d4a9 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -98,6 +98,26 @@ int kvm_arch_check_processor_compat(void)
return 0;
}
+int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
+ struct kvm_enable_cap *cap)
+{
+ int r;
+
+ if (cap->flags)
+ return -EINVAL;
+
+ switch (cap->cap) {
+ case KVM_CAP_ARM_NISV_TO_USER:
+ r = 0;
+ kvm->arch.return_nisv_io_abort_to_user = true;
+ break;
+ default:
+ r = -EINVAL;
+ break;
+ }
+
+ return r;
+}
/**
* kvm_arch_init_vm - initializes a VM data structure
@@ -196,6 +216,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
case KVM_CAP_MP_STATE:
case KVM_CAP_IMMEDIATE_EXIT:
case KVM_CAP_VCPU_EVENTS:
+ case KVM_CAP_ARM_NISV_TO_USER:
r = 1;
break;
case KVM_CAP_ARM_SET_DEVICE_ADDR:
@@ -673,6 +694,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
ret = kvm_handle_mmio_return(vcpu, vcpu->run);
if (ret)
return ret;
+ } else if (run->exit_reason == KVM_EXIT_ARM_NISV) {
+ kvm_inject_undefined(vcpu);
}
if (run->immediate_exit)
diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c
index 6af5c91337f2..62e6ef47a6de 100644
--- a/virt/kvm/arm/mmio.c
+++ b/virt/kvm/arm/mmio.c
@@ -167,8 +167,15 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
if (ret)
return ret;
} else {
- kvm_err("load/store instruction decoding not implemented\n");
- return -ENOSYS;
+ if (vcpu->kvm->arch.return_nisv_io_abort_to_user) {
+ run->exit_reason = KVM_EXIT_ARM_NISV;
+ run->mmio.phys_addr = fault_ipa;
+ vcpu->stat.mmio_exit_user++;
+ return 0;
+ } else {
+ kvm_info("encountered data abort without syndrome info\n");
+ return -ENOSYS;
+ }
}
rt = vcpu->arch.mmio_decode.rt;
Thanks,
Christoffer
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH] dt-bindings: arm: Convert Realtek board/soc bindings to json-schema
From: Rob Herring @ 2019-09-05 9:21 UTC (permalink / raw)
To: jamestai.sky
Cc: Mark Rutland, devicetree, james.tai, CY_Huang,
linux-kernel@vger.kernel.org, Phinex Hung, Andreas Färber,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <20190905081721.1548-1-james.tai@realtek.com>
On Thu, Sep 5, 2019 at 9:19 AM <jamestai.sky@gmail.com> wrote:
>
> From: "james.tai" <james.tai@realtek.com>
>
> Convert Realtek SoC bindings to DT schema format using json-schema.
>
> Signed-off-by: james.tai <james.tai@realtek.com>
> ---
> .../devicetree/bindings/arm/realtek.txt | 22 -------------------
> .../devicetree/bindings/arm/realtek.yaml | 17 ++++++++++++++
> 2 files changed, 17 insertions(+), 22 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/arm/realtek.txt
> create mode 100644 Documentation/devicetree/bindings/arm/realtek.yaml
I've already submitted a patch for this that's *still* waiting on
Andreas to apply or comment on the licensing.
Also, your patch isn't valid schema. Please check with 'make dt_binding_check'.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox