Re: [PATCH v2 2/3] drivers: char: ipmi: Add Aspeed SSIF BMC driver

[Philipp Zabel]

Hi Quan,

On Tue, Mar 30, 2021 at 09:10:28PM +0700, Quan Nguyen wrote:
> The SMBus system interface (SSIF) IPMI BMC driver can be used to perform
> in-band IPMI communication with their host in management (BMC) side.
> 
> This commits adds support specifically for Aspeed AST2500 which commonly
> used as Board Management Controllers.
> 
> Signed-off-by: Quan Nguyen 
> ---
[...]
> diff --git a/drivers/char/ipmi/ssif_bmc_aspeed.c 
> b/drivers/char/ipmi/ssif_bmc_aspeed.c
> new file mode 100644
> index ..a563fcff5acc
> --- /dev/null
> +++ b/drivers/char/ipmi/ssif_bmc_aspeed.c
> @@ -0,0 +1,132 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * The driver for BMC side of Aspeed SSIF interface
> + *
> + * Copyright (c) 2021, Ampere Computing LLC
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see .
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "ssif_bmc.h"
> +
> +struct aspeed_i2c_bus {
> + struct i2c_adapter  adap;
> + struct device   *dev;

This device handle is apparently unused.

> + void __iomem*base;
> + struct reset_control*rst;

This reset control handle is unused as well.

> + /* Synchronizes I/O mem access to base. */
> + spinlock_t  lock;
> +};

regards
Philipp

Re: [PATCH -next] media: imx-pxp: remove redundant dev_err call in pxp_probe()

[Philipp Zabel]

On Thu, Apr 01, 2021 at 06:26:07PM +0800, Yang Yingliang wrote:
> There is an error message within devm_ioremap_resource
> already, so remove the dev_err call to avoid redundant
> error message.
> 
> Reported-by: Hulk Robot 
> Signed-off-by: Yang Yingliang 

Thank you,
Reviewed-by: Philipp Zabel 

regards
Philipp

Re: [PATCH v6 02/13] dt-bindings: media: nxp,imx8mq-vpu: Update the bindings for G2 support

[Philipp Zabel]

On Fri, Mar 26, 2021 at 03:26:15PM +0100, Benjamin Gaignard wrote:
> 
> Le 26/03/2021 à 15:11, Philipp Zabel a écrit :
> > On Thu, Mar 18, 2021 at 09:20:35AM +0100, Benjamin Gaignard wrote:
> > > Introducing G2 hevc video decoder lead to modify the bindings to allow
> > > to get one node per VPUs.
> > > VPUs share one hardware control block which is provided as a phandle on
> > > an syscon.
> > > Each node got now one reg and one interrupt.
> > > Add a compatible for G2 hardware block: nxp,imx8mq-vpu-g2.
> > > 
> > > To be compatible with older DT the driver is still capable to use 'ctrl'
> > > reg-name even if it is deprecated now.
> > > 
> > > Signed-off-by: Benjamin Gaignard 
> > > ---
> > > version 5:
> > > - This version doesn't break the backward compatibilty between kernel
> > >and DT.
> > > 
> > >   .../bindings/media/nxp,imx8mq-vpu.yaml| 53 ---
> > >   1 file changed, 34 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml 
> > > b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> > > index 762be3f96ce9..79502fc8bde5 100644
> > > --- a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> > > +++ b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> > > @@ -15,22 +15,18 @@ description:
> > >   properties:
> > > compatible:
> > > -const: nxp,imx8mq-vpu
> > > +oneOf:
> > > +  - const: nxp,imx8mq-vpu
> > > +  - const: nxp,imx8mq-vpu-g2
> > > reg:
> > > -maxItems: 3
> > > -
> > > -  reg-names:
> > > -items:
> > > -  - const: g1
> > > -  - const: g2
> > > -  - const: ctrl
> > > +maxItems: 1
> > > interrupts:
> > > -maxItems: 2
> > > +maxItems: 1
> > > interrupt-names:
> > > -items:
> > > +oneOf:
> > > - const: g1
> > > - const: g2
> > > @@ -46,14 +42,18 @@ properties:
> > > power-domains:
> > >   maxItems: 1
> > > +  nxp,imx8mq-vpu-ctrl:
> > > +description: Specifies a phandle to syscon VPU hardware control block
> > > +$ref: "/schemas/types.yaml#/definitions/phandle"
> > > +
> > Should we drop the 'q' here, i.e. nxp,imx8m-vpu-ctrl so we can use the same
> > binding for i.MX8MM later?
> 
> I don't know if the control block is the same or not on IMX8MM, so I have only
> put a compatible targeting IMX8MQ.

Oh, the compatible property of the control handle node can be different.
I'm just suggesting that this phandle property be called the same.
Otherwise we'd have to add another nxp,imx8mm-vpu-ctrl property and then
mark either of the two as required, depending on the compatible.

> > 
> > >   required:
> > > - compatible
> > > - reg
> > > -  - reg-names
> > > - interrupts
> > > - interrupt-names
> > > - clocks
> > > - clock-names
> > > +  - nxp,imx8mq-vpu-ctrl
> > >   additionalProperties: false
> > > @@ -62,18 +62,33 @@ examples:
> > >   #include 
> > >   #include 
> > > -vpu: video-codec@3830 {
> > > +vpu_ctrl: syscon@3832 {
> > > + compatible = "nxp,imx8mq-vpu-ctrl", "syscon";
> > > + reg = <0x3832 0x1>;
> > > +};
> > > +
> > > +vpu_g1: video-codec@3830 {
> > >   compatible = "nxp,imx8mq-vpu";
> > > -reg = <0x3830 0x1>,
> > > -  <0x3831 0x1>,
> > > -  <0x3832 0x1>;
> > > -reg-names = "g1", "g2", "ctrl";
> > > -interrupts = ,
> > > - ;
> > > -interrupt-names = "g1", "g2";
> > > +reg = <0x3830 0x1>;
> > > +interrupts = ;
> > > +interrupt-names = "g1";
> > > +clocks = < IMX8MQ_CLK_VPU_G1_ROOT>,
> > > + < IMX8MQ_CLK_VPU_G2_ROOT>,
> > Does the G1 VPU require the G2 clock and vice versa?
> 
> Yes either the control hardware block won't work.

Ok.

Reviewed-by: Philipp Zabel 

regards
Philipp

Re: [PATCH v6 13/13] arm64: dts: imx8mq: Add node to G2 hardware

[Philipp Zabel]

On Thu, Mar 18, 2021 at 09:20:46AM +0100, Benjamin Gaignard wrote:
> Split VPU node in two: one for G1 and one for G2 since they are
> different hardware blocks.
> Add syscon for hardware control block.
> Remove reg-names property that is useless.
> Each VPU node only need one interrupt.
> 
> Signed-off-by: Benjamin Gaignard 
> ---
> version 5:
>  - use syscon instead of VPU reset
> 
>  arch/arm64/boot/dts/freescale/imx8mq.dtsi | 43 ++-
>  1 file changed, 34 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi 
> b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> index 17c449e12c2e..b537d153ebbd 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> @@ -1329,15 +1329,16 @@ usb3_phy1: usb-phy@382f0040 {
>   status = "disabled";
>   };
>  
> - vpu: video-codec@3830 {
> + vpu_ctrl: syscon@3832 {
> + compatible = "nxp,imx8mq-vpu-ctrl", "syscon";
> + reg = <0x3832 0x1>;
> + };
> +
> + vpu_g1: video-codec@3830 {
>   compatible = "nxp,imx8mq-vpu";
> - reg = <0x3830 0x1>,
> -   <0x3831 0x1>,
> -   <0x3832 0x1>;
> - reg-names = "g1", "g2", "ctrl";
> - interrupts = ,
> -  ;
> - interrupt-names = "g1", "g2";
> + reg = <0x3830 0x1>;
> + interrupts = ;
> + interrupt-names = "g1";
>   clocks = < IMX8MQ_CLK_VPU_G1_ROOT>,
>< IMX8MQ_CLK_VPU_G2_ROOT>,
>< IMX8MQ_CLK_VPU_DEC_ROOT>;
> @@ -1350,9 +1351,33 @@ vpu: video-codec@3830 {
>< IMX8MQ_VPU_PLL_OUT>,
>< IMX8MQ_SYS1_PLL_800M>,
>< IMX8MQ_VPU_PLL>;
> - assigned-clock-rates = <6>, <6>,
> + assigned-clock-rates = <6>, <3>,

I'd like to see this mentioned in the commit message.

> +<8>, <0>;
> + power-domains = <_vpu>;
> + nxp,imx8mq-vpu-ctrl = <_ctrl>;
> + };
> +
> + vpu_g2: video-codec@3831 {
> + compatible = "nxp,imx8mq-vpu-g2";
> + reg = <0x3831 0x1>;
> + interrupts = ;
> + interrupt-names = "g2";
> + clocks = < IMX8MQ_CLK_VPU_G1_ROOT>,
> +  < IMX8MQ_CLK_VPU_G2_ROOT>,
> +  < IMX8MQ_CLK_VPU_DEC_ROOT>;
> + clock-names = "g1", "g2",  "bus";
> + assigned-clocks = < IMX8MQ_CLK_VPU_G1>,

Can the G1 clock configuration be dropped from the G2 device node and
the G2 clock configuration from the G1 device node? It looks weird that
these devices configure each other's clocks.

regards
Philipp

Re: [PATCH v6 12/13] media: hantro: IMX8M: add variant for G2/HEVC codec

[Philipp Zabel]

On Thu, Mar 18, 2021 at 09:20:45AM +0100, Benjamin Gaignard wrote:
> Add variant to IMX8M to enable G2/HEVC codec.
> Define the capabilities for the hardware up to 3840x2160.
> G2 doesn't have postprocessor, use the same clocks and got it
> own interruption.
> 
> Signed-off-by: Benjamin Gaignard 

Reviewed-by: Philipp Zabel 

regards
Philipp

Re: [PATCH v6 03/13] media: hantro: Use syscon instead of 'ctrl' register

[Philipp Zabel]

On Thu, Mar 18, 2021 at 09:20:36AM +0100, Benjamin Gaignard wrote:
> In order to be able to share the control hardware block between
> VPUs use a syscon instead a ioremap it in the driver.
> To keep the compatibility with older DT if 'nxp,imx8mq-vpu-ctrl'
> phandle is not found look at 'ctrl' reg-name.
> With the method it becomes useless to provide a list of register
> names so remove it.
> 
> Signed-off-by: Benjamin Gaignard 
> ---
> version 5:
>  - use syscon instead of VPU reset driver.
>  - if DT doesn't provide syscon keep backward compatibilty by using
>'ctrl' reg-name.
> 
>  drivers/staging/media/hantro/hantro.h   |  5 +-
>  drivers/staging/media/hantro/imx8m_vpu_hw.c | 52 -
>  2 files changed, 34 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro.h 
> b/drivers/staging/media/hantro/hantro.h
> index 65f9f7ea7dcf..a99a96b84b5e 100644
> --- a/drivers/staging/media/hantro/hantro.h
> +++ b/drivers/staging/media/hantro/hantro.h
> @@ -13,6 +13,7 @@
>  #define HANTRO_H_
>  
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -167,7 +168,7 @@ hantro_vdev_to_func(struct video_device *vdev)
>   * @reg_bases:   Mapped addresses of VPU registers.
>   * @enc_base:Mapped address of VPU encoder register for 
> convenience.
>   * @dec_base:Mapped address of VPU decoder register for 
> convenience.
> - * @ctrl_base:   Mapped address of VPU control block.
> + * @ctrl_base:   Regmap of VPU control block.
>   * @vpu_mutex:   Mutex to synchronize V4L2 calls.
>   * @irqlock: Spinlock to synchronize access to data structures
>   *   shared with interrupt handlers.
> @@ -186,7 +187,7 @@ struct hantro_dev {
>   void __iomem **reg_bases;
>   void __iomem *enc_base;
>   void __iomem *dec_base;
> - void __iomem *ctrl_base;
> + struct regmap *ctrl_base;
>  
>   struct mutex vpu_mutex; /* video_device lock */
>   spinlock_t irqlock;
> diff --git a/drivers/staging/media/hantro/imx8m_vpu_hw.c 
> b/drivers/staging/media/hantro/imx8m_vpu_hw.c
> index c222de075ef4..bd9d135dd440 100644
> --- a/drivers/staging/media/hantro/imx8m_vpu_hw.c
> +++ b/drivers/staging/media/hantro/imx8m_vpu_hw.c
> @@ -7,6 +7,7 @@
>  
>  #include 
>  #include 
> +#include 
>  
>  #include "hantro.h"
>  #include "hantro_jpeg.h"
> @@ -24,30 +25,28 @@
>  #define CTRL_G1_PP_FUSE  0x0c
>  #define CTRL_G2_DEC_FUSE 0x10
>  
> +static const struct regmap_config ctrl_regmap_ctrl = {
> + .reg_bits = 32,
> + .val_bits = 32,
> + .reg_stride = 0x14,
> +};
> +
>  static void imx8m_soft_reset(struct hantro_dev *vpu, u32 reset_bits)
>  {
> - u32 val;
> -
>   /* Assert */
> - val = readl(vpu->ctrl_base + CTRL_SOFT_RESET);
> - val &= ~reset_bits;
> - writel(val, vpu->ctrl_base + CTRL_SOFT_RESET);
> + regmap_update_bits(vpu->ctrl_base, CTRL_SOFT_RESET, reset_bits, 0);
>  
>   udelay(2);
>  
>   /* Release */
> - val = readl(vpu->ctrl_base + CTRL_SOFT_RESET);
> - val |= reset_bits;
> - writel(val, vpu->ctrl_base + CTRL_SOFT_RESET);
> + regmap_update_bits(vpu->ctrl_base, CTRL_SOFT_RESET,
> +reset_bits, reset_bits);
>  }
>  
>  static void imx8m_clk_enable(struct hantro_dev *vpu, u32 clock_bits)
>  {
> - u32 val;
> -
> - val = readl(vpu->ctrl_base + CTRL_CLOCK_ENABLE);
> - val |= clock_bits;
> - writel(val, vpu->ctrl_base + CTRL_CLOCK_ENABLE);
> + regmap_update_bits(vpu->ctrl_base, CTRL_CLOCK_ENABLE,
> +clock_bits, clock_bits);
>  }
>  
>  static int imx8mq_runtime_resume(struct hantro_dev *vpu)
> @@ -64,9 +63,9 @@ static int imx8mq_runtime_resume(struct hantro_dev *vpu)
>   imx8m_clk_enable(vpu, CLOCK_G1 | CLOCK_G2);
>  
>   /* Set values of the fuse registers */
> - writel(0x, vpu->ctrl_base + CTRL_G1_DEC_FUSE);
> - writel(0x, vpu->ctrl_base + CTRL_G1_PP_FUSE);
> - writel(0x, vpu->ctrl_base + CTRL_G2_DEC_FUSE);
> + regmap_write(vpu->ctrl_base, CTRL_G1_DEC_FUSE, 0x);
> + regmap_write(vpu->ctrl_base, CTRL_G1_PP_FUSE, 0x);
> + regmap_write(vpu->ctrl_base, CTRL_G2_DEC_FUSE, 0x);
>  
>   clk_bulk_disable_unprepare(vpu->variant->num_clocks, vpu->clocks);
>  
> @@ -150,8 +149,22 @@ static irqreturn_t imx8m_vpu_g1_irq(int irq, void 
> *dev_id)
>  
>  static int imx8mq_vpu_hw_init(struct hantro_dev *vpu)
>  {
> - vpu->dec_base = vpu->reg_bases[0];
> - vpu->ctrl_base = vpu->reg_bases[vpu->variant->num_regs - 1];
> + struct device_node *np = vpu->dev->of_node;
> +
> + vpu->ctrl_base = syscon_regmap_lookup_by_phandle(np, 
> "nxp,imx8mq-vpu-ctrl");

I think calling this nxp,imx8m-vpu-ctrl would allow to share this with
i.MX8MM later. Otherwise,

Reviewed-by: Philipp Zabel 

thanks
Philipp

Re: [PATCH v6 02/13] dt-bindings: media: nxp,imx8mq-vpu: Update the bindings for G2 support

[Philipp Zabel]

On Thu, Mar 18, 2021 at 09:20:35AM +0100, Benjamin Gaignard wrote:
> Introducing G2 hevc video decoder lead to modify the bindings to allow
> to get one node per VPUs.
> VPUs share one hardware control block which is provided as a phandle on
> an syscon.
> Each node got now one reg and one interrupt.
> Add a compatible for G2 hardware block: nxp,imx8mq-vpu-g2.
> 
> To be compatible with older DT the driver is still capable to use 'ctrl'
> reg-name even if it is deprecated now.
> 
> Signed-off-by: Benjamin Gaignard 
> ---
> version 5:
> - This version doesn't break the backward compatibilty between kernel
>   and DT.
> 
>  .../bindings/media/nxp,imx8mq-vpu.yaml| 53 ---
>  1 file changed, 34 insertions(+), 19 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml 
> b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> index 762be3f96ce9..79502fc8bde5 100644
> --- a/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> +++ b/Documentation/devicetree/bindings/media/nxp,imx8mq-vpu.yaml
> @@ -15,22 +15,18 @@ description:
>  
>  properties:
>compatible:
> -const: nxp,imx8mq-vpu
> +oneOf:
> +  - const: nxp,imx8mq-vpu
> +  - const: nxp,imx8mq-vpu-g2
>  
>reg:
> -maxItems: 3
> -
> -  reg-names:
> -items:
> -  - const: g1
> -  - const: g2
> -  - const: ctrl
> +maxItems: 1
>  
>interrupts:
> -maxItems: 2
> +maxItems: 1
>  
>interrupt-names:
> -items:
> +oneOf:
>- const: g1
>- const: g2
>  
> @@ -46,14 +42,18 @@ properties:
>power-domains:
>  maxItems: 1
>  
> +  nxp,imx8mq-vpu-ctrl:
> +description: Specifies a phandle to syscon VPU hardware control block
> +$ref: "/schemas/types.yaml#/definitions/phandle"
> +

Should we drop the 'q' here, i.e. nxp,imx8m-vpu-ctrl so we can use the same
binding for i.MX8MM later?

>  required:
>- compatible
>- reg
> -  - reg-names
>- interrupts
>- interrupt-names
>- clocks
>- clock-names
> +  - nxp,imx8mq-vpu-ctrl
>  
>  additionalProperties: false
>  
> @@ -62,18 +62,33 @@ examples:
>  #include 
>  #include 
>  
> -vpu: video-codec@3830 {
> +vpu_ctrl: syscon@3832 {
> + compatible = "nxp,imx8mq-vpu-ctrl", "syscon";
> + reg = <0x3832 0x1>;
> +};
> +
> +vpu_g1: video-codec@3830 {
>  compatible = "nxp,imx8mq-vpu";
> -reg = <0x3830 0x1>,
> -  <0x3831 0x1>,
> -  <0x3832 0x1>;
> -reg-names = "g1", "g2", "ctrl";
> -interrupts = ,
> - ;
> -interrupt-names = "g1", "g2";
> +reg = <0x3830 0x1>;
> +interrupts = ;
> +interrupt-names = "g1";
> +clocks = < IMX8MQ_CLK_VPU_G1_ROOT>,
> + < IMX8MQ_CLK_VPU_G2_ROOT>,

Does the G1 VPU require the G2 clock and vice versa?

regards
Philipp

Re: [PATCH] [v3] drm/imx: imx-ldb: fix out of bounds array access warning

[Philipp Zabel]

On Thu, Mar 25, 2021 at 10:03:23AM +0800, Liu Ying wrote:
> On Wed, 2021-03-24 at 17:47 +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann 
> > 
> > When CONFIG_OF is disabled, building with 'make W=1' produces warnings
> > about out of bounds array access:
> > 
> > drivers/gpu/drm/imx/imx-ldb.c: In function 'imx_ldb_set_clock.constprop':
> > drivers/gpu/drm/imx/imx-ldb.c:186:8: error: array subscript -22 is below 
> > array bounds of 'struct clk *[4]' [-Werror=array-bounds]
> > 
> > Add an error check before the index is used, which helps with the
> > warning, as well as any possible other error condition that may be
> > triggered at runtime.
> > 
> > The warning could be fixed by adding a Kconfig depedency on CONFIG_OF,
> > but Liu Ying points out that the driver may hit the out-of-bounds
> > problem at runtime anyway.
> > 
> > Signed-off-by: Arnd Bergmann 
> Reviewed-by: Liu Ying 

Thank you, applied to imx-drm/fixes.

regards
Philipp

Re: [PATCH] drm/imx: imx-ldb: Register LDB channel1 when it is the only channel to be used

[Philipp Zabel]

On Mon, Mar 22, 2021 at 10:56:40AM +0800, Liu Ying wrote:
> LDB channel1 should be registered if it is the only channel to be used.
> Without this patch, imx_ldb_bind() would skip registering LDB channel1
> if LDB channel0 is not used, no matter LDB channel1 needs to be used or
> not.
> 
> Fixes: 8767f4711b2b (drm/imx: imx-ldb: move initialization into probe)
> Signed-off-by: Liu Ying 

Thank you, applied to imx-drm/fixes.

regards
Philipp

Re: [PATCH] drm/imx: fix memory leak when fails to init

[Philipp Zabel]

On Wed, Jan 20, 2021 at 01:16:08AM -0800, Pan Bian wrote:
> Put DRM device on initialization failure path rather than directly
> return error code.
> 
> Fixes: a67d5088ceb8 ("drm/imx: drop explicit drm_mode_config_cleanup")
> Signed-off-by: Pan Bian 

Thank you, applied to imx-drm/fixes.

regards
Philipp

Re: [PATCH 2/3] net: ethernet: actions: Add Actions Semi Owl Ethernet MAC driver

[Philipp Zabel]

Hi Cristian,

On Thu, Mar 11, 2021 at 03:20:13AM +0200, Cristian Ciocaltea wrote:
> Add new driver for the Ethernet MAC used on the Actions Semi Owl
> family of SoCs.
> 
> Currently this has been tested only on the Actions Semi S500 SoC
> variant.
> 
> Signed-off-by: Cristian Ciocaltea 
> ---
[...]
> diff --git a/drivers/net/ethernet/actions/owl-emac.c 
> b/drivers/net/ethernet/actions/owl-emac.c
> new file mode 100644
> index ..ebd8ea88bca4
> --- /dev/null
> +++ b/drivers/net/ethernet/actions/owl-emac.c
> @@ -0,0 +1,1660 @@
[...]
> +static int owl_emac_probe(struct platform_device *pdev)
> +{
[...]
> + priv->reset = devm_reset_control_get(dev, NULL);

Please use

priv->reset = devm_reset_control_get_exclusive(dev, NULL);

instead, to explicitly state that the driver requires exclusive
control over the reset line.

> + if (IS_ERR(priv->reset)) {
> + ret = PTR_ERR(priv->reset);
> + dev_err(dev, "failed to get reset control: %d\n", ret);
> + return ret;

You could use:

return dev_err_probe(dev, PTR_ERR(priv->reset),
 "failed to get reset control");

regards
Philipp

Re: [PATCH] reset: zynqmp: replace spaces with tabs

[Philipp Zabel]

On Thu, 2021-03-04 at 17:03 +0100, Michal Simek wrote:
> 
> On 3/4/21 5:00 PM, Philipp Zabel wrote:
> > Fixes checkpatch issues:
> > 
> >   ERROR: code indent should use tabs where possible
> >   #86: FILE: drivers/reset/reset-zynqmp.c:86:
> >   +.reset_id = 0,$
> > 
> >   WARNING: please, no spaces at the start of a line
> >   #86: FILE: drivers/reset/reset-zynqmp.c:86:
> >   +.reset_id = 0,$
> > 
> >   ERROR: code indent should use tabs where possible
> >   #87: FILE: drivers/reset/reset-zynqmp.c:87:
> >   +.num_resets = VERSAL_NR_RESETS,$
> > 
> >   WARNING: please, no spaces at the start of a line
> >   #87: FILE: drivers/reset/reset-zynqmp.c:87:
> >   +.num_resets = VERSAL_NR_RESETS,$
> > 
> > Signed-off-by: Philipp Zabel 
> > ---
> >  drivers/reset/reset-zynqmp.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/reset/reset-zynqmp.c b/drivers/reset/reset-zynqmp.c
> > index ebd433fa09dd..daa425e74c96 100644
> > --- a/drivers/reset/reset-zynqmp.c
> > +++ b/drivers/reset/reset-zynqmp.c
> > @@ -83,8 +83,8 @@ static const struct zynqmp_reset_soc_data 
> > zynqmp_reset_data = {
> >  };
> >  
> >  static const struct zynqmp_reset_soc_data versal_reset_data = {
> > -.reset_id = 0,
> > -.num_resets = VERSAL_NR_RESETS,
> > +   .reset_id = 0,
> > +   .num_resets = VERSAL_NR_RESETS,
> >  };
> >  
> >  static const struct reset_control_ops zynqmp_reset_ops = {
> > 
> 
> Reviewed-by: Michal Simek 

Thank you, applied to reset/next

regards
Philipp

Re: [PATCH v6 3/3] hwrng: bcm2835: add reset support

[Philipp Zabel]

On Fri, 2021-03-05 at 08:01 +0100, Álvaro Fernández Rojas wrote:
> BCM6368 devices need to reset the IPSEC controller in order to generate true
> random numbers.
> 
> This is what BCM6368 produces without a reset:
> root@OpenWrt:/# cat /dev/hwrng | rngtest -c 1000
> rngtest 6.10
> Copyright (c) 2004 by Henrique de Moraes Holschuh
> This is free software; see the source for copying conditions.  There is NO 
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> 
> rngtest: starting FIPS tests...
> rngtest: bits received from input: 2032
> rngtest: FIPS 140-2 successes: 0
> rngtest: FIPS 140-2 failures: 1000
> rngtest: FIPS 140-2(2001-10-10) Monobit: 2
> rngtest: FIPS 140-2(2001-10-10) Poker: 1000
> rngtest: FIPS 140-2(2001-10-10) Runs: 1000
> rngtest: FIPS 140-2(2001-10-10) Long run: 30
> rngtest: FIPS 140-2(2001-10-10) Continuous run: 0
> rngtest: input channel speed: (min=37.253; avg=320.827; max=635.783)Mibits/s
> rngtest: FIPS tests speed: (min=12.141; avg=15.034; max=16.428)Mibits/s
> rngtest: Program run time: 1336176 microseconds
> 
> Signed-off-by: Álvaro Fernández Rojas 

Reviewed-by: Philipp Zabel 

regards
Philipp

> ---
>  v6: fix commit description.
>  v5: remove reset_control_rearm().
>  v4: add reset_control_rearm().
>  v3: no changes.
>  v2: no changes.
> 
>  drivers/char/hw_random/bcm2835-rng.c | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/char/hw_random/bcm2835-rng.c 
> b/drivers/char/hw_random/bcm2835-rng.c
> index be5be395b341..e7dd457e9b22 100644
> --- a/drivers/char/hw_random/bcm2835-rng.c
> +++ b/drivers/char/hw_random/bcm2835-rng.c
> @@ -13,6 +13,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #define RNG_CTRL 0x0
>  #define RNG_STATUS   0x4
> @@ -32,6 +33,7 @@ struct bcm2835_rng_priv {
>   void __iomem *base;
>   bool mask_interrupts;
>   struct clk *clk;
> + struct reset_control *reset;
>  };
>  
>  static inline struct bcm2835_rng_priv *to_rng_priv(struct hwrng *rng)
> @@ -92,6 +94,10 @@ static int bcm2835_rng_init(struct hwrng *rng)
>   if (ret)
>   return ret;
>  
> + ret = reset_control_reset(priv->reset);
> + if (ret)
> + return ret;
> +
>   if (priv->mask_interrupts) {
>   /* mask the interrupt */
>   val = rng_readl(priv, RNG_INT_MASK);
> @@ -156,6 +162,10 @@ static int bcm2835_rng_probe(struct platform_device 
> *pdev)
>   if (IS_ERR(priv->clk))
>   return PTR_ERR(priv->clk);
>  
> + priv->reset = devm_reset_control_get_optional_exclusive(dev, NULL);
> + if (IS_ERR(priv->reset))
> + return PTR_ERR(priv->reset);
> +
>   priv->rng.name = pdev->name;
>   priv->rng.init = bcm2835_rng_init;
>   priv->rng.read = bcm2835_rng_read;

[PATCH] reset: axs10x: Replace GPLv2 boilerplate/reference with SPDX

[Philipp Zabel]

reset-axs10x is GPL-2.0-only, add a SPDX-License-Identifier.

Signed-off-by: Philipp Zabel 
---
 drivers/reset/reset-axs10x.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/reset/reset-axs10x.c b/drivers/reset/reset-axs10x.c
index a854ef41364d..ca78b859936c 100644
--- a/drivers/reset/reset-axs10x.c
+++ b/drivers/reset/reset-axs10x.c
@@ -1,11 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0-only
 /*
  * Copyright (C) 2017 Synopsys.
  *
  * Synopsys AXS10x reset driver.
- *
- * This file is licensed under the terms of the GNU General Public
- * License version 2. This program is licensed "as is" without any
- * warranty of any kind, whether express or implied.
  */
 
 #include 
-- 
2.29.2

[PATCH] reset: oxnas: replace file name with short description

[Philipp Zabel]

Fixes a checkpatch warning:

  WARNING: It's generally not useful to have the filename in the file
  #3: FILE: drivers/reset/reset-oxnas.c:3:
  + * drivers/reset/reset-oxnas.c

Signed-off-by: Philipp Zabel 
---
 drivers/reset/reset-oxnas.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/reset/reset-oxnas.c b/drivers/reset/reset-oxnas.c
index c4013165bdda..8209f922dc16 100644
--- a/drivers/reset/reset-oxnas.c
+++ b/drivers/reset/reset-oxnas.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /*
- * drivers/reset/reset-oxnas.c
+ * Oxford Semiconductor Reset Controller driver
  *
  * Copyright (C) 2016 Neil Armstrong 
  * Copyright (C) 2014 Ma Haijun 
-- 
2.29.2

[PATCH] reset: ti-syscon: fix to_ti_syscon_reset_data macro

[Philipp Zabel]

The to_ti_syscon_reset_data macro currently only works if the
parameter passed into it is called 'rcdev'.

Fixes a checkpatch --strict issue:

  CHECK: Macro argument reuse 'rcdev' - possible side-effects?
  #53: FILE: drivers/reset/reset-ti-syscon.c:53:
  +#define to_ti_syscon_reset_data(rcdev)   \
  + container_of(rcdev, struct ti_syscon_reset_data, rcdev)

Signed-off-by: Philipp Zabel 
---
 drivers/reset/reset-ti-syscon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/reset/reset-ti-syscon.c b/drivers/reset/reset-ti-syscon.c
index 218370faf37b..2b92775d58f0 100644
--- a/drivers/reset/reset-ti-syscon.c
+++ b/drivers/reset/reset-ti-syscon.c
@@ -58,8 +58,8 @@ struct ti_syscon_reset_data {
unsigned int nr_controls;
 };
 
-#define to_ti_syscon_reset_data(rcdev) \
-   container_of(rcdev, struct ti_syscon_reset_data, rcdev)
+#define to_ti_syscon_reset_data(_rcdev)\
+   container_of(_rcdev, struct ti_syscon_reset_data, rcdev)
 
 /**
  * ti_syscon_reset_assert() - assert device reset
-- 
2.29.2

[PATCH] reset: uniphier: enclose UNIPHIER_RESET_ID_END value in parentheses

[Philipp Zabel]

Fixes a checkpatch error:

  ERROR: Macros with complex values should be enclosed in parentheses
  #23: FILE: drivers/reset/reset-uniphier.c:23:
  +#define UNIPHIER_RESET_ID_END(unsigned int)(-1)

Signed-off-by: Philipp Zabel 
---
 drivers/reset/reset-uniphier.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/reset/reset-uniphier.c b/drivers/reset/reset-uniphier.c
index 279e535bf5d8..5f75783f9397 100644
--- a/drivers/reset/reset-uniphier.c
+++ b/drivers/reset/reset-uniphier.c
@@ -20,7 +20,7 @@ struct uniphier_reset_data {
 #define UNIPHIER_RESET_ACTIVE_LOW  BIT(0)
 };
 
-#define UNIPHIER_RESET_ID_END  (unsigned int)(-1)
+#define UNIPHIER_RESET_ID_END  ((unsigned int)(-1))
 
 #define UNIPHIER_RESET_END \
{ .id = UNIPHIER_RESET_ID_END }
-- 
2.29.2

[PATCH] reset: sti/syscfg: replace comma with semicolon

[Philipp Zabel]

Fixes a checkpatch warning:

  WARNING: Possible comma where semicolon could be used
  #156: FILE: drivers/reset/sti/reset-syscfg.c:156:
  + rc->rst.ops = _reset_ops,
  + rc->rst.of_node = dev->of_node;

Signed-off-by: Philipp Zabel 
---
 drivers/reset/sti/reset-syscfg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/reset/sti/reset-syscfg.c b/drivers/reset/sti/reset-syscfg.c
index 99b63035fe72..b4b46e0f207e 100644
--- a/drivers/reset/sti/reset-syscfg.c
+++ b/drivers/reset/sti/reset-syscfg.c
@@ -153,7 +153,7 @@ static int syscfg_reset_controller_register(struct device 
*dev,
if (!rc->channels)
return -ENOMEM;
 
-   rc->rst.ops = _reset_ops,
+   rc->rst.ops = _reset_ops;
rc->rst.of_node = dev->of_node;
rc->rst.nr_resets = data->nr_channels;
rc->active_low = data->active_low;
-- 
2.29.2

[PATCH] reset: zynqmp: replace spaces with tabs

[Philipp Zabel]

Fixes checkpatch issues:

  ERROR: code indent should use tabs where possible
  #86: FILE: drivers/reset/reset-zynqmp.c:86:
  +.reset_id = 0,$

  WARNING: please, no spaces at the start of a line
  #86: FILE: drivers/reset/reset-zynqmp.c:86:
  +.reset_id = 0,$

  ERROR: code indent should use tabs where possible
  #87: FILE: drivers/reset/reset-zynqmp.c:87:
  +.num_resets = VERSAL_NR_RESETS,$

  WARNING: please, no spaces at the start of a line
  #87: FILE: drivers/reset/reset-zynqmp.c:87:
  +.num_resets = VERSAL_NR_RESETS,$

Signed-off-by: Philipp Zabel 
---
 drivers/reset/reset-zynqmp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/reset/reset-zynqmp.c b/drivers/reset/reset-zynqmp.c
index ebd433fa09dd..daa425e74c96 100644
--- a/drivers/reset/reset-zynqmp.c
+++ b/drivers/reset/reset-zynqmp.c
@@ -83,8 +83,8 @@ static const struct zynqmp_reset_soc_data zynqmp_reset_data = 
{
 };
 
 static const struct zynqmp_reset_soc_data versal_reset_data = {
-.reset_id = 0,
-.num_resets = VERSAL_NR_RESETS,
+   .reset_id = 0,
+   .num_resets = VERSAL_NR_RESETS,
 };
 
 static const struct reset_control_ops zynqmp_reset_ops = {
-- 
2.29.2

[PATCH] reset: berlin: replace unsigned with unsigned int

[Philipp Zabel]

Fixes a checkpatch warning:

  WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
  #55: FILE: drivers/reset/reset-berlin.c:55:
  + unsigned offset, bit;

Signed-off-by: Philipp Zabel 
---
 drivers/reset/reset-berlin.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/reset/reset-berlin.c b/drivers/reset/reset-berlin.c
index 371197bbd055..094dba98cebc 100644
--- a/drivers/reset/reset-berlin.c
+++ b/drivers/reset/reset-berlin.c
@@ -55,7 +55,7 @@ static const struct reset_control_ops berlin_reset_ops = {
 static int berlin_reset_xlate(struct reset_controller_dev *rcdev,
  const struct of_phandle_args *reset_spec)
 {
-   unsigned offset, bit;
+   unsigned int offset, bit;
 
offset = reset_spec->args[0];
bit = reset_spec->args[1];
-- 
2.29.2

[PATCH] reset: whitespace fixes

[Philipp Zabel]

Fixes checkpatch issues:

  CHECK: Alignment should match open parenthesis
  #87: FILE: drivers/reset/core.c:87:
  +static int of_reset_simple_xlate(struct reset_controller_dev *rcdev,
  +   const struct of_phandle_args *reset_spec)

  CHECK: Lines should not end with a '('
  #540: FILE: drivers/reset/core.c:540:
  +static struct reset_control *__reset_control_get_internal(

  CHECK: Alignment should match open parenthesis
  #603: FILE: drivers/reset/core.c:603:
  +struct reset_control *__of_reset_control_get(struct device_node *node,
  +  const char *id, int index, bool shared,

  CHECK: Alignment should match open parenthesis
  #781: FILE: drivers/reset/core.c:781:
  +struct reset_control *__devm_reset_control_get(struct device *dev,
  +  const char *id, int index, bool shared,

Signed-off-by: Philipp Zabel 
---
 drivers/reset/core.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index dbf881b586d9..123b0c53a857 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -84,7 +84,7 @@ static const char *rcdev_name(struct reset_controller_dev 
*rcdev)
  * without gaps.
  */
 static int of_reset_simple_xlate(struct reset_controller_dev *rcdev,
- const struct of_phandle_args *reset_spec)
+const struct of_phandle_args *reset_spec)
 {
if (reset_spec->args[0] >= rcdev->nr_resets)
return -EINVAL;
@@ -610,9 +610,9 @@ void reset_control_release(struct reset_control *rstc)
 }
 EXPORT_SYMBOL_GPL(reset_control_release);
 
-static struct reset_control *__reset_control_get_internal(
-   struct reset_controller_dev *rcdev,
-   unsigned int index, bool shared, bool acquired)
+static struct reset_control *
+__reset_control_get_internal(struct reset_controller_dev *rcdev,
+unsigned int index, bool shared, bool acquired)
 {
struct reset_control *rstc;
 
@@ -672,9 +672,9 @@ static void __reset_control_put_internal(struct 
reset_control *rstc)
kref_put(>refcnt, __reset_control_release);
 }
 
-struct reset_control *__of_reset_control_get(struct device_node *node,
-const char *id, int index, bool shared,
-bool optional, bool acquired)
+struct reset_control *
+__of_reset_control_get(struct device_node *node, const char *id, int index,
+  bool shared, bool optional, bool acquired)
 {
struct reset_control *rstc;
struct reset_controller_dev *r, *rcdev;
@@ -850,9 +850,9 @@ static void devm_reset_control_release(struct device *dev, 
void *res)
reset_control_put(*(struct reset_control **)res);
 }
 
-struct reset_control *__devm_reset_control_get(struct device *dev,
-const char *id, int index, bool shared,
-bool optional, bool acquired)
+struct reset_control *
+__devm_reset_control_get(struct device *dev, const char *id, int index,
+bool shared, bool optional, bool acquired)
 {
struct reset_control **ptr, *rstc;
 
-- 
2.29.2

Re: [RFC PATCH 5/6] PCI: designware: Add SiFive FU740 PCIe host controller driver

[Philipp Zabel]

On Tue, 2021-03-02 at 18:59 +0800, Greentime Hu wrote:
[...]
> +static int fu740_pcie_probe(struct platform_device *pdev)
> +{
[...]
> +   /* Fetch reset */
> +   afp->rst = devm_reset_control_get(dev, NULL);

Please use
   afp->rst = devm_reset_control_get_exclusive(dev, NULL);  

regards
Philipp

Re: [RFC PATCH 2/6] clk: sifive: Use reset-simple in prci driver for PCIe driver

[Philipp Zabel]

On Tue, 2021-03-02 at 18:59 +0800, Greentime Hu wrote:
> We use reset-simple in this patch so that pcie driver can use
> devm_reset_control_get() to get this reset data structure and use
> reset_control_deassert() to deassert pcie_power_up_rst_n.
> 
> Signed-off-by: Greentime Hu 
> ---
>  drivers/clk/sifive/Kconfig   |  2 ++
>  drivers/clk/sifive/sifive-prci.c | 14 ++
>  drivers/clk/sifive/sifive-prci.h |  4 
>  drivers/reset/Kconfig|  3 ++-
>  4 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/sifive/Kconfig b/drivers/clk/sifive/Kconfig
> index 1c14eb20c066..9132c3c4aa86 100644
> --- a/drivers/clk/sifive/Kconfig
> +++ b/drivers/clk/sifive/Kconfig
> @@ -10,6 +10,8 @@ if CLK_SIFIVE
>  
>  config CLK_SIFIVE_PRCI
>   bool "PRCI driver for SiFive SoCs"
> + select RESET_CONTROLLER
> + select RESET_SIMPLE
>   select CLK_ANALOGBITS_WRPLL_CLN28HPC
>   help
> Supports the Power Reset Clock interface (PRCI) IP block found in
> diff --git a/drivers/clk/sifive/sifive-prci.c 
> b/drivers/clk/sifive/sifive-prci.c
> index baf7313dac92..925affc6de55 100644
> --- a/drivers/clk/sifive/sifive-prci.c
> +++ b/drivers/clk/sifive/sifive-prci.c
> @@ -583,7 +583,21 @@ static int sifive_prci_probe(struct platform_device 
> *pdev)
>   if (IS_ERR(pd->va))
>   return PTR_ERR(pd->va);
>  
> + pd->reset.rcdev.owner = THIS_MODULE;
> + pd->reset.rcdev.nr_resets = PRCI_RST_NR;
> + pd->reset.rcdev.ops = _simple_ops;
> + pd->reset.rcdev.of_node = pdev->dev.of_node;
> + pd->reset.active_low = true;
> + pd->reset.membase = pd->va + PRCI_DEVICESRESETREG_OFFSET;
> + spin_lock_init(>reset.lock);
> +
> + r = devm_reset_controller_register(>dev, >reset.rcdev);
> + if (r) {
> + dev_err(dev, "could not register reset controller: %d\n", r);
> + return r;
> + }
>   r = __prci_register_clocks(dev, pd, desc);
> +

Accidental whitespace?

Otherwise,

Reviewed-by: Philipp Zabel 

regards
Philipp

Re: [PATCH v4 01/13] phy: cadence: Sierra: Fix PHY power_on sequence

[Philipp Zabel]

On Thu, 2021-03-04 at 10:11 +0530, Kishon Vijay Abraham I wrote:
> Commit 44d30d622821d ("phy: cadence: Add driver for Sierra PHY")
> de-asserts PHY_RESET even before the configurations are loaded in
> phy_init(). However PHY_RESET should be de-asserted only after
> all the configurations has been initialized, instead of de-asserting
> in probe. Fix it here.
> 
> Fixes: 44d30d622821d ("phy: cadence: Add driver for Sierra PHY")
> Signed-off-by: Kishon Vijay Abraham I 
> Cc:  # v5.4+

Reviewed-by: Philipp Zabel 

regards
Philipp

Re: [PATCH v4 07/13] phy: cadence: cadence-sierra: Explicitly request exclusive reset control

[Philipp Zabel]

On Thu, 2021-03-04 at 10:11 +0530, Kishon Vijay Abraham I wrote:
> No functional change. Since the reset controls obtained in
> Sierra is exclusively used by the Sierra device, use
> exclusive reset control request API calls.
> 
> Signed-off-by: Kishon Vijay Abraham I 
> ---
>  drivers/phy/cadence/phy-cadence-sierra.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/phy/cadence/phy-cadence-sierra.c 
> b/drivers/phy/cadence/phy-cadence-sierra.c
> index 935f165404e4..44c52a0842dc 100644
> --- a/drivers/phy/cadence/phy-cadence-sierra.c
> +++ b/drivers/phy/cadence/phy-cadence-sierra.c
> @@ -514,14 +514,14 @@ static int cdns_sierra_phy_get_resets(struct 
> cdns_sierra_phy *sp,
>  {
>   struct reset_control *rst;
>  
> - rst = devm_reset_control_get(dev, "sierra_reset");
> + rst = devm_reset_control_get_exclusive(dev, "sierra_reset");
>   if (IS_ERR(rst)) {
>   dev_err(dev, "failed to get reset\n");
>   return PTR_ERR(rst);
>   }
>   sp->phy_rst = rst;
>  
> - rst = devm_reset_control_get_optional(dev, "sierra_apb");
> + rst = devm_reset_control_get_optional_exclusive(dev, "sierra_apb");
>   if (IS_ERR(rst)) {
>   dev_err(dev, "failed to get apb reset\n");
>   return PTR_ERR(rst);

Oh, nevermind my comment on the previous patch.

Reviewed-by: Philipp Zabel 

regards
Philipp

Re: [PATCH v4 06/13] phy: cadence: cadence-sierra: Move all reset_control_get*() to a separate function

[Philipp Zabel]

On Thu, 2021-03-04 at 10:11 +0530, Kishon Vijay Abraham I wrote:
> No functional change. Group devm_reset_control_get() and
> devm_reset_control_get_optional() to a separate function.
> 
> Signed-off-by: Kishon Vijay Abraham I 
> ---
>  drivers/phy/cadence/phy-cadence-sierra.c | 36 
>  1 file changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/phy/cadence/phy-cadence-sierra.c 
> b/drivers/phy/cadence/phy-cadence-sierra.c
> index 7bf1b4c7774a..935f165404e4 100644
> --- a/drivers/phy/cadence/phy-cadence-sierra.c
> +++ b/drivers/phy/cadence/phy-cadence-sierra.c
> @@ -509,6 +509,28 @@ static int cdns_sierra_phy_get_clocks(struct 
> cdns_sierra_phy *sp,
>   return 0;
>  }
>  
> +static int cdns_sierra_phy_get_resets(struct cdns_sierra_phy *sp,
> +   struct device *dev)
> +{
> + struct reset_control *rst;
> +
> + rst = devm_reset_control_get(dev, "sierra_reset");

Please use
rst = devm_reset_control_get_exclusive(dev, "sierra_reset");
here ...

> + if (IS_ERR(rst)) {
> + dev_err(dev, "failed to get reset\n");
> + return PTR_ERR(rst);
> + }
> + sp->phy_rst = rst;
> +
> + rst = devm_reset_control_get_optional(dev, "sierra_apb");

... and
    rst = devm_reset_control_get_optional_exclusive(dev, "sierra_apb"); 
here (no functional change).

With that,

Reviewed-by: Philipp Zabel 

regards
Philipp

Re: [PATCH v5 2/2] hwrng: bcm2835: add reset support

[Philipp Zabel]

On Thu, 2021-03-04 at 08:33 +0100, Álvaro Fernández Rojas wrote:
> BCM6368 devices need to reset the in order to generate true random numbers.
> This is what BCM6368 produces without a reset:
> root@OpenWrt:/# cat /dev/hwrng | rngtest -c 1000
> rngtest 6.10
> Copyright (c) 2004 by Henrique de Moraes Holschuh
> This is free software; see the source for copying conditions.  There is NO 
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> 
> rngtest: starting FIPS tests...
> rngtest: bits received from input: 2032
> rngtest: FIPS 140-2 successes: 0
> rngtest: FIPS 140-2 failures: 1000
> rngtest: FIPS 140-2(2001-10-10) Monobit: 2
> rngtest: FIPS 140-2(2001-10-10) Poker: 1000
> rngtest: FIPS 140-2(2001-10-10) Runs: 1000
> rngtest: FIPS 140-2(2001-10-10) Long run: 30
> rngtest: FIPS 140-2(2001-10-10) Continuous run: 0
> rngtest: input channel speed: (min=37.253; avg=320.827; max=635.783)Mibits/s
> rngtest: FIPS tests speed: (min=12.141; avg=15.034; max=16.428)Mibits/s
> rngtest: Program run time: 1336176 microseconds
> cat: write error: Broken pipe
> 
> Signed-off-by: Álvaro Fernández Rojas 
> ---
>  v5: remove reset_control_rearm().

Reviewed-by: Philipp Zabel 

regards
Philipp

Re: [PATCH v3 0/5] Reset driver for IMX8MQ VPU hardware block

[Philipp Zabel]

On Wed, 2021-03-03 at 16:20 +0100, Benjamin Gaignard wrote:
> Le 03/03/2021 à 15:17, Philipp Zabel a écrit :
> > Hi Benjamin,
> > 
> > On Mon, 2021-03-01 at 16:17 +0100, Benjamin Gaignard wrote:
> > > The two VPUs inside IMX8MQ share the same control block which can be see
> > > as a reset hardware block.
> > This isn't a reset controller though. The control block also contains
> > clock gates of some sort and a filter register for the featureset fuses.
> > Those shouldn't be manipulated via the reset API.
> 
> They are all part of the control block and of the reset process for this
> hardware that why I put them here. I guess it is border line :-)

I'm pushing back to keep the reset control framework focused on
controlling reset lines. Every side effect (such as the asymmetric clock
ungating) in a random driver makes it harder to reason about behaviour
at the API level, and to review patches for hardware I am not familiar
with.

> > > In order to be able to add the second VPU (for HECV decoding) it will be
> > > more handy if the both VPU drivers instance don't have to share the
> > > control block registers. This lead to implement it as an independ reset
> > > driver and to change the VPU driver to use it.
> > Why not switch to a syscon regmap for the control block? That should
> > also allow to keep backwards compatibility with the old binding with
> > minimal effort.
> 
> I will give a try in this direction.

Thank you.

> > > Please note that this series break the compatibility between the DTB and
> > > kernel. This break is limited to IMX8MQ SoC and is done when the driver
> > > is still in staging directory.
> > I know in this case we are pretty sure there are no users of this
> > binding except for a staging driver, but it would still be nice to keep
> > support for the deprecated binding, to avoid the requirement of updating
> > kernel and DT in lock-step.
> 
> If I want to use a syscon (or a reset) the driver must not ioremap the "ctrl"
> registers. It means that "ctrl" has to be removed from the driver requested
> reg-names (imx8mq_reg_names[]). Doing that break the kernel/DT compatibility.
> Somehow syscon and "ctrl" are exclusive.

The way the driver is set up currently, yes. You could add a bit of
platform specific probe code, though, that would set up the regmap
either by calling
syscon_regmap_lookup_by_phandle();
for the new binding, or, if the phandle is not available, fall back to
platform_get_resource_byname(..., "ctrl");
devm_ioremap_resource();
devm_regmap_init_mmio();
for the old binding.
The actual codec .reset and variant .runtime_resume ops could be
identical then.

regards
Philipp

Re: [PATCH v3 3/5] reset: Add reset driver for IMX8MQ VPU block

[Philipp Zabel]

On Mon, 2021-03-01 at 16:17 +0100, Benjamin Gaignard wrote:
> IMX8MQ SoC got a dedicated hardware block to reset the video processor
> units (G1 and G2).
> 
> Signed-off-by: Benjamin Gaignard 
> ---
>  drivers/reset/Kconfig|   8 ++
>  drivers/reset/Makefile   |   1 +
>  drivers/reset/reset-imx8mq-vpu.c | 169 +++
>  3 files changed, 178 insertions(+)
>  create mode 100644 drivers/reset/reset-imx8mq-vpu.c
> 
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 71ab75a46491..fa95380b271a 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -80,6 +80,14 @@ config RESET_IMX7
>   help
> This enables the reset controller driver for i.MX7 SoCs.
>  
> +config RESET_VPU_IMX8MQ
> + tristate "i.MX8MQ VPU Reset Driver"
> + depends on HAS_IOMEM
> + depends on (ARM64 && ARCH_MXC) || COMPILE_TEST
> + select MFD_SYSCON
> + help
> +   This enables the VPU reset controller driver for i.MX8MQ SoCs.
> +
>  config RESET_INTEL_GW
>   bool "Intel Reset Controller Driver"
>   depends on OF && HAS_IOMEM
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 1054123fd187..6007e0cdfc05 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_RESET_BRCMSTB) += reset-brcmstb.o
>  obj-$(CONFIG_RESET_BRCMSTB_RESCAL) += reset-brcmstb-rescal.o
>  obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o
>  obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
> +obj-$(CONFIG_RESET_VPU_IMX8MQ) += reset-imx8mq-vpu.o
>  obj-$(CONFIG_RESET_INTEL_GW) += reset-intel-gw.o
>  obj-$(CONFIG_RESET_LANTIQ) += reset-lantiq.o
>  obj-$(CONFIG_RESET_LPC18XX) += reset-lpc18xx.o
> diff --git a/drivers/reset/reset-imx8mq-vpu.c 
> b/drivers/reset/reset-imx8mq-vpu.c
> new file mode 100644
> index ..14c589f19266
> --- /dev/null
> +++ b/drivers/reset/reset-imx8mq-vpu.c
> @@ -0,0 +1,169 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2021, Collabora
> + *
> + * i.MX8MQ VPU Reset Controller driver
> + *
> + * Author: Benjamin Gaignard 
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define CTRL_SOFT_RESET  0x00
> +#define RESET_G1 ((u32)BIT(1))
> +#define RESET_G2 ((u32)BIT(0))
> +
> +#define CTRL_ENABLE  0x04
> +#define ENABLE_G1BIT(1)
> +#define ENABLE_G2BIT(0)
> +
> +#define CTRL_G1_DEC_FUSE 0x08
> +#define CTRL_G1_PP_FUSE  0x0c
> +#define CTRL_G2_DEC_FUSE 0x10
> +
> +struct imx8mq_vpu_reset {
> + struct reset_controller_dev rcdev;
> + struct regmap *regmap;
> + struct clk_bulk_data *clocks;
> + int num_clocks;
> + struct device *dev;
> +};
> +
> +static inline struct imx8mq_vpu_reset *to_imx8mq_vpu_reset(struct 
> reset_controller_dev *rcdev)
> +{
> + return container_of(rcdev, struct imx8mq_vpu_reset, rcdev);
> +}
> +
> +static int imx8mq_vpu_reset_assert(struct reset_controller_dev *rcdev,
> +unsigned long id)
> +{
> + struct imx8mq_vpu_reset *reset = to_imx8mq_vpu_reset(rcdev);
> + int ret = -EINVAL;
> +
> + ret = clk_bulk_prepare_enable(reset->num_clocks, reset->clocks);
> + if (ret) {
> + dev_err(reset->dev, "Failed to prepare clocks\n");
> + return ret;
> + }
> +
> + switch (id) {
> + case IMX8MQ_RESET_VPU_RESET_G1:
> + ret = regmap_update_bits(reset->regmap, CTRL_SOFT_RESET, 
> RESET_G1, ~RESET_G1);
> + ret |= regmap_update_bits(reset->regmap, CTRL_ENABLE, 
> ENABLE_G1, ENABLE_G1);
> + break;
> + case IMX8MQ_RESET_VPU_RESET_G2:
> + ret = regmap_update_bits(reset->regmap, CTRL_SOFT_RESET, 
> RESET_G2, ~RESET_G2);
> + ret |= regmap_update_bits(reset->regmap, CTRL_ENABLE, 
> ENABLE_G2, ENABLE_G2);

This doesn't belong in reset_assert.

> + break;
> + }
> +
> + /* Set values of the fuse registers */
> + ret |= regmap_write(reset->regmap, CTRL_G1_DEC_FUSE, 0x);
> + ret |= regmap_write(reset->regmap, CTRL_G1_PP_FUSE, 0x);
> + ret |= regmap_write(reset->regmap, CTRL_G2_DEC_FUSE, 0x);

Same as above, this doesn't belong in reset_assert.

> + clk_bulk_disable_unprepare(reset->num_clocks, reset->clocks);

Also I assume that only the VPU_DEC_ROOT clock is required to control
these registers. Enabling the VPU_G1_ROOT and VPU_G2_ROOT clocks
(presumably to make sure the resets propagate into the respective VPU
core) would be the reset consumer's responsibility.

regards
Philipp

Re: [PATCH v6 22/37] reset: reset-scmi: port driver to the new scmi_reset_proto_ops interface

[Philipp Zabel]

On Tue, 2021-02-02 at 22:15 +, Cristian Marussi wrote:
> Port driver to the new SCMI Reset interface based on protocol handles
> and common devm_get_ops().
> 
> Cc: Philipp Zabel 
> Signed-off-by: Cristian Marussi 

Acked-by: Philipp Zabel 

regards
Philipp

Re: [PATCH v3 4/5] media: hantro: Use reset driver

[Philipp Zabel]

On Mon, 2021-03-01 at 16:17 +0100, Benjamin Gaignard wrote:
> Rather use a reset like feature inside the driver use the reset
> controller API to get the same result.
> 
> Signed-off-by: Benjamin Gaignard 
> ---
>  drivers/staging/media/hantro/Kconfig|  1 +
>  drivers/staging/media/hantro/imx8m_vpu_hw.c | 61 -
>  2 files changed, 12 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/staging/media/hantro/Kconfig 
> b/drivers/staging/media/hantro/Kconfig
> index 5b6cf9f62b1a..dd1d4dde2658 100644
> --- a/drivers/staging/media/hantro/Kconfig
> +++ b/drivers/staging/media/hantro/Kconfig
> @@ -20,6 +20,7 @@ config VIDEO_HANTRO_IMX8M
>   bool "Hantro VPU i.MX8M support"
>   depends on VIDEO_HANTRO
>   depends on ARCH_MXC || COMPILE_TEST
> + select RESET_VPU_IMX8MQ
>   default y
>   help
> Enable support for i.MX8M SoCs.
> diff --git a/drivers/staging/media/hantro/imx8m_vpu_hw.c 
> b/drivers/staging/media/hantro/imx8m_vpu_hw.c
> index c222de075ef4..d5b4312b9391 100644
> --- a/drivers/staging/media/hantro/imx8m_vpu_hw.c
> +++ b/drivers/staging/media/hantro/imx8m_vpu_hw.c
> @@ -7,49 +7,12 @@
>  
>  #include 
>  #include 
> +#include 
>  
>  #include "hantro.h"
>  #include "hantro_jpeg.h"
>  #include "hantro_g1_regs.h"
>  
> -#define CTRL_SOFT_RESET  0x00
> -#define RESET_G1 BIT(1)
> -#define RESET_G2 BIT(0)
> -
> -#define CTRL_CLOCK_ENABLE0x04
> -#define CLOCK_G1 BIT(1)
> -#define CLOCK_G2 BIT(0)
> -
> -#define CTRL_G1_DEC_FUSE 0x08
> -#define CTRL_G1_PP_FUSE  0x0c
> -#define CTRL_G2_DEC_FUSE 0x10
> -
> -static void imx8m_soft_reset(struct hantro_dev *vpu, u32 reset_bits)
> -{
> - u32 val;
> -
> - /* Assert */
> - val = readl(vpu->ctrl_base + CTRL_SOFT_RESET);
> - val &= ~reset_bits;
> - writel(val, vpu->ctrl_base + CTRL_SOFT_RESET);
> -
> - udelay(2);
> -
> - /* Release */
> - val = readl(vpu->ctrl_base + CTRL_SOFT_RESET);
> - val |= reset_bits;
> - writel(val, vpu->ctrl_base + CTRL_SOFT_RESET);
> -}
> -
> -static void imx8m_clk_enable(struct hantro_dev *vpu, u32 clock_bits)
> -{
> - u32 val;
> -
> - val = readl(vpu->ctrl_base + CTRL_CLOCK_ENABLE);
> - val |= clock_bits;
> - writel(val, vpu->ctrl_base + CTRL_CLOCK_ENABLE);

The way it is implemented in the reset driver, the clocks are now
ungated between assert and deassert instead of afterwards. Is this on
purpose?

regards
Philipp

Re: [PATCH v3 0/5] Reset driver for IMX8MQ VPU hardware block

[Philipp Zabel]

Hi Benjamin,

On Mon, 2021-03-01 at 16:17 +0100, Benjamin Gaignard wrote:
> The two VPUs inside IMX8MQ share the same control block which can be see
> as a reset hardware block.

This isn't a reset controller though. The control block also contains
clock gates of some sort and a filter register for the featureset fuses.
Those shouldn't be manipulated via the reset API.

> In order to be able to add the second VPU (for HECV decoding) it will be
> more handy if the both VPU drivers instance don't have to share the
> control block registers. This lead to implement it as an independ reset 
> driver and to change the VPU driver to use it.

Why not switch to a syscon regmap for the control block? That should
also allow to keep backwards compatibility with the old binding with
minimal effort.

> Please note that this series break the compatibility between the DTB and
> kernel. This break is limited to IMX8MQ SoC and is done when the driver
> is still in staging directory.

I know in this case we are pretty sure there are no users of this
binding except for a staging driver, but it would still be nice to keep
support for the deprecated binding, to avoid the requirement of updating
kernel and DT in lock-step.

regards
Philipp

Re: [PATCH v4 2/2] hwrng: bcm2835: add reset support

[Philipp Zabel]

Hi Álvaro,

On Wed, 2021-02-24 at 09:22 +0100, Álvaro Fernández Rojas wrote:
[...]
> @@ -115,6 +121,8 @@ static void bcm2835_rng_cleanup(struct hwrng *rng)
>   /* disable rng hardware */
>   rng_writel(priv, 0, RNG_CTRL);
>  
> + reset_control_rearm(priv->reset);
> +
>   if (!IS_ERR(priv->clk))
>   clk_disable_unprepare(priv->clk);
>  }
> @@ -159,6 +167,10 @@ static int bcm2835_rng_probe(struct platform_device 
> *pdev)
>   if (PTR_ERR(priv->clk) == -EPROBE_DEFER)
>   return -EPROBE_DEFER;
>  
> + priv->reset = devm_reset_control_get_optional_exclusive(dev, NULL);
> + if (IS_ERR(priv->reset))
> + return PTR_ERR(priv->reset);
> +
>   priv->rng.name = pdev->name;
>   priv->rng.init = bcm2835_rng_init;
>   priv->rng.read = bcm2835_rng_read;

That doesn't seem right. reset_control_rearm() doesn't do anything if
the reset control is exclusive. Either the reset control should be
requested as shared, or the _rearm should be removed.

regards
Philipp

[RFC] reset: add reset_control_bulk API

[Philipp Zabel]

Follow the clock and regulator subsystems' lead and add a bulk API
for reset controls.

Signed-off-by: Philipp Zabel 
---
 drivers/reset/core.c  | 213 +
 include/linux/reset.h | 238 ++
 2 files changed, 451 insertions(+)

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index dbf881b586d9..8de41a4e8901 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -358,6 +358,30 @@ int reset_control_reset(struct reset_control *rstc)
 }
 EXPORT_SYMBOL_GPL(reset_control_reset);
 
+/**
+ * reset_control_bulk_reset - reset the controlled devices in order
+ * @num_rstcs: number of entries in rstcs array
+ * @rstcs: array of struct reset_control_bulk_data with reset controls set
+ *
+ * Issue a reset on all provided reset controls, in order.
+ *
+ * See also: reset_control_reset()
+ */
+int reset_control_bulk_reset(int num_rstcs,
+struct reset_control_bulk_data *rstcs)
+{
+   int ret, i;
+
+   for (i = 0; i < num_rstcs; i++) {
+   ret = reset_control_reset(rstcs[i].rstc);
+   if (ret)
+   return ret;
+   }
+
+   return 0;
+}
+EXPORT_SYMBOL_GPL(reset_control_bulk_reset);
+
 /**
  * reset_control_rearm - allow shared reset line to be re-triggered"
  * @rstc: reset controller
@@ -461,6 +485,36 @@ int reset_control_assert(struct reset_control *rstc)
 }
 EXPORT_SYMBOL_GPL(reset_control_assert);
 
+/**
+ * reset_control_bulk_assert - asserts the reset lines in order
+ * @num_rstcs: number of entries in rstcs array
+ * @rstcs: array of struct reset_control_bulk_data with reset controls set
+ *
+ * Assert the reset lines for all provided reset controls, in order.
+ * If an assertion fails, already asserted resets are deasserted again.
+ *
+ * See also: reset_control_assert()
+ */
+int reset_control_bulk_assert(int num_rstcs,
+ struct reset_control_bulk_data *rstcs)
+{
+   int ret, i;
+
+   for (i = 0; i < num_rstcs; i++) {
+   ret = reset_control_assert(rstcs[i].rstc);
+   if (ret)
+   goto err;
+   }
+
+   return 0;
+
+err:
+   while (i--)
+   reset_control_deassert(rstcs[i].rstc);
+   return ret;
+}
+EXPORT_SYMBOL_GPL(reset_control_bulk_assert);
+
 /**
  * reset_control_deassert - deasserts the reset line
  * @rstc: reset controller
@@ -511,6 +565,36 @@ int reset_control_deassert(struct reset_control *rstc)
 }
 EXPORT_SYMBOL_GPL(reset_control_deassert);
 
+/**
+ * reset_control_bulk_deassert - asserts the reset lines in order
+ * @num_rstcs: number of entries in rstcs array
+ * @rstcs: array of struct reset_control_bulk_data with reset controls set
+ *
+ * Deassert the reset lines for all provided reset controls, in order.
+ * If a deassertion fails, already deasserted resets are asserted again.
+ *
+ * See also: reset_control_assert()
+ */
+int reset_control_bulk_deassert(int num_rstcs,
+   struct reset_control_bulk_data *rstcs)
+{
+   int ret, i;
+
+   for (i = 0; i < num_rstcs; i++) {
+   ret = reset_control_deassert(rstcs[i].rstc);
+   if (ret)
+   goto err;
+   }
+
+   return 0;
+
+err:
+   while (i--)
+   reset_control_assert(rstcs[i].rstc);
+   return ret;
+}
+EXPORT_SYMBOL_GPL(reset_control_bulk_deassert);
+
 /**
  * reset_control_status - returns a negative errno if not supported, a
  * positive value if the reset line is asserted, or zero if the reset
@@ -588,6 +672,36 @@ int reset_control_acquire(struct reset_control *rstc)
 }
 EXPORT_SYMBOL_GPL(reset_control_acquire);
 
+/**
+ * reset_control_bulk_acquire - acquires reset controls for exclusive use
+ * @num_rstcs: number of entries in rstcs array
+ * @rstcs: array of struct reset_control_bulk_data with reset controls set
+ *
+ * This is used to explicitly acquire reset controls requested with
+ * reset_control_bulk_get_exclusive_release() for temporary exclusive use.
+ *
+ * See also: reset_control_acquire(), reset_control_bulk_release()
+ */
+int reset_control_bulk_acquire(int num_rstcs,
+  struct reset_control_bulk_data *rstcs)
+{
+   int ret, i;
+
+   for (i = 0; i < num_rstcs; i++) {
+   ret = reset_control_acquire(rstcs[i].rstc);
+   if (ret)
+   goto err;
+   }
+
+   return 0;
+
+err:
+   while (i--)
+   reset_control_release(rstcs[i].rstc);
+   return ret;
+}
+EXPORT_SYMBOL_GPL(reset_control_bulk_acquire);
+
 /**
  * reset_control_release() - releases exclusive access to a reset control
  * @rstc: reset control
@@ -610,6 +724,26 @@ void reset_control_release(struct reset_control *rstc)
 }
 EXPORT_SYMBOL_GPL(reset_control_release);
 
+/**
+ * reset_control_bulk_release() - releases exclusive access to reset controls
+ * @num_rstcs: number of

Re: [PATCH v1 5/5] ASoC: tegra30: i2s: Add reset control

[Philipp Zabel]

Hi Dmitry,

On Wed, 2021-03-03 at 11:28 +0300, Dmitry Osipenko wrote:
> 02.03.2021 14:21, Dmitry Osipenko пишет:
> > The I2S reset may be asserted at a boot time. Tegra30 I2S driver doesn't
> > manage the reset control and currently it happens to work because reset
> > is implicitly deasserted by the Tegra AHUB driver, but the reset of I2C
> > controller should be synchronous and I2S clock is disabled when AHUB is
> > reset. Add reset control to the Tegra30 I2S driver.
> > 
> > Note that I2S reset was always specified in Tegra30+ device-trees, hence
> > DTB ABI changes aren't required. Also note that AHUB touches I2S resets,
> > hence AHUB resets are now requested in a released state, allowing both
> > drivers to control the I2S resets together.
> > 
> > Signed-off-by: Dmitry Osipenko 
> > ---
> >  sound/soc/tegra/tegra30_ahub.c | 14 ++---
> >  sound/soc/tegra/tegra30_i2s.c  | 36 +-
> >  sound/soc/tegra/tegra30_i2s.h  |  1 +
> >  3 files changed, 47 insertions(+), 4 deletions(-)
> 
> ...
> > @@ -579,7 +587,7 @@ static int tegra30_ahub_probe(struct platform_device 
> > *pdev)
> > if (ret)
> > return ret;
> >  
> > -   ahub->reset = devm_reset_control_array_get_exclusive(>dev);
> > +   ahub->reset = 
> > devm_reset_control_array_get_exclusive_released(>dev);
> 
> Thinking a bit more about this, it looks like we actually want something
> like:
> 
>   devm_reset_control_array_get_exclusive_released_named()
> 
> that will request resets by given names and in a given order, similarly
> to devm_clk_bulk_get(). This will be very handy for both Tegra audio and
> GPU drivers. I'll prepare a v2 if there are no objections.

I do have an untested reset control bulk API patch that I've just never
finished, because I had no user. I'll send you an RFC, let me know if
you can build on that.

regards
Philipp

Re: [BUG REPORT] media: coda: mpeg4 decode corruption on i.MX6qp only

[Philipp Zabel]

Hi Sven,

On Wed, Feb 10, 2021 at 01:29:29PM -0500, Sven Van Asbroeck wrote:
> Found it!
> 
> The i.MX6QuadPlus has two pairs of PREs, which use the extended
> section of the iRAM. The Classic does not have any PREs or extended
> iRAM:
> 
> pre1: pre@21c8000 {
>compatible = "fsl,imx6qp-pre";
> 
> fsl,iram = <>;
> };
> 
> pre3: pre@21ca000 {
> compatible = "fsl,imx6qp-pre";
> 
> fsl,iram = <>;
> };
> 
> The CODA (VPU) driver uses the common section of iRAM:
> 
> vpu: vpu@204 {
> compatible = "cnm,coda960";
> 
> iram = <>;
> };
> 
> The VPU or the PREs are overrunning their assigned iRAM area. How do I
> know? Because if I change the PRE iRAM order, the problem disappears!
> 
> PRE1: ocram2 change to ocram3
> PRE2: ocram2 change to ocram3
> PRE3: ocram3 change to ocram2
> PRE4: ocram3 change to ocram2

Thank you for debugging this. Given that CODA uses the OCRAM address
range 0x90-0x94 and the PREs use OCRAM2 at 0x94-0x96
and OCRAM3 at 0x96-0x98, it seems unlikely that the PREs would
overrun into the CODA iRAM. But maybe there is some stride related
overflow that causes it to write at negative offsets or some other kind
of oversight.

Could you check /sys/kernel/debug/dri/?/state while running the error case?

Another thing that might help to identify who is writing where might be to
clear the whole OCRAM region and dump it after running only decode or only
PRE/PRG scanout, for example:

--8<--
/* Clear OCRAM */
#include 
#include 
#include 
#include 
#include 

#define OCRAM_START 0x90
#define OCRAM_SIZE  0x8

int main(int argc, char *argv[])
{
int fd = open("/dev/mem", O_RDWR | O_SYNC);
void *map = mmap(NULL, OCRAM_SIZE, PROT_WRITE, MAP_SHARED, fd, 
OCRAM_START);
if (map == MAP_FAILED)
return EXIT_FAILURE;
memset(map, 0, OCRAM_SIZE);
munmap(map, OCRAM_SIZE);
close(fd);
return EXIT_SUCCESS;
}
-->8--

--8<--
/* Dump OCRAM to stdout */
#include 
#include 
#include 
#include 

#define OCRAM_START 0x90
#define OCRAM_SIZE  0x8

int main(int argc, char *argv[])
{
int fd = open("/dev/mem", O_RDONLY | O_SYNC);
void *map = mmap(NULL, OCRAM_SIZE, PROT_READ, MAP_SHARED, fd, 
OCRAM_START);
if (map == MAP_FAILED)
return EXIT_FAILURE;
write(1, map, OCRAM_SIZE);
munmap(map, OCRAM_SIZE);
close(fd);
return EXIT_SUCCESS;
}
-->8--

regards
Philipp

Re: [PATCH v1] reset: Add devm_reset_control_get_optional_exclusive_released()

[Philipp Zabel]

Hi Dmitry,

On Sat, 2021-01-23 at 19:34 +0300, Dmitry Osipenko wrote:
> NVIDIA Tegra DRM and media drivers will need a resource-managed-optional
> variant of reset_control_get_exclusive_released() in order to switch away
> from a legacy Tegra-specific PD API to a GENPD API without much hassle.
> Add the new reset helper to the reset API.
> 
> Tested-by: Peter Geis  # Ouya T30
> Tested-by: Nicolas Chauvet  # PAZ00 T20
> Tested-by: Matt Merhar  # Ouya T30
> Signed-off-by: Dmitry Osipenko 
> ---
> 
> Hello Philipp,
> 
> This patch is a prerequisite for a power domain enablement using
> GENPD API on NVIDIA Tegra20/30 SoCs. The hardware resets are acquired
> by a Tegra PMC (Power Management Controller) driver until device is
> RPM-resumed if GENPD API is used, and thus, device drivers need to
> request resets in a released state. The resets are also optional,
> depending on hardware and DTB versions. This is why we will need the
> new helper. Will be awesome if you could pick up this patch for v5.12,
> this will help to avoid inter-subsystem dependencies for the driver
> patches that will target v5.13. Thanks in advance!

Thank you, looks good to me. Applied to reset/next for v5.12.

regards
Philipp

[PATCH] reset: bcm6345: Make reset_control_ops const

[Philipp Zabel]

The bcm6345_reset_ops structure is never modified. Make it const.

Signed-off-by: Philipp Zabel 
---
 drivers/reset/reset-bcm6345.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/reset/reset-bcm6345.c b/drivers/reset/reset-bcm6345.c
index 737e4e81f6b7..ac6c7ad1deda 100644
--- a/drivers/reset/reset-bcm6345.c
+++ b/drivers/reset/reset-bcm6345.c
@@ -86,7 +86,7 @@ static int bcm6345_reset_status(struct reset_controller_dev 
*rcdev,
return !(__raw_readl(bcm6345_reset->base) & BIT(id));
 }
 
-static struct reset_control_ops bcm6345_reset_ops = {
+static const struct reset_control_ops bcm6345_reset_ops = {
.assert = bcm6345_reset_assert,
.deassert = bcm6345_reset_deassert,
.reset = bcm6345_reset_reset,
-- 
2.20.1

Re: [PATCH 1/3] dt-bindings: reset: microchip sparx5 reset driver bindings

[Philipp Zabel]

Hi Steen,

On Wed, 2021-01-13 at 21:19 +0100, Steen Hegelund wrote:
> Signed-off-by: Steen Hegelund 
> ---
>  .../bindings/reset/microchip,rst.yaml | 52 +++
>  1 file changed, 52 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/microchip,rst.yaml
> 
> diff --git a/Documentation/devicetree/bindings/reset/microchip,rst.yaml 
> b/Documentation/devicetree/bindings/reset/microchip,rst.yaml
> new file mode 100644
> index ..b5526753e85d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/microchip,rst.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/reset/microchip,rst.yaml#;
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#;
> +
> +title: Microchip Sparx5 Switch Reset Controller
> +
> +maintainers:
> +  - Steen Hegelund 
> +  - Lars Povlsen 
> +
> +description: |
> +  The Microchip Sparx5 Switch provides reset control and implements the 
> following
> +  functions
> +- One Time Switch Core Reset (Soft Reset)
> +
> +properties:
> +  $nodename:
> +pattern: "^reset-controller@[0-9a-f]+$"
> +
> +  compatible:
> +const: microchip,sparx5-switch-reset
> +
> +  reg:
> +maxItems: 1
> +
> +  "#reset-cells":
> +const: 1
> +
> +  syscons:
> +$ref: "/schemas/types.yaml#/definitions/phandle-array"
> +description: Array of syscons used to access reset registers
> +minItems: 2

The order seems to be important in the driver, so this should specify
which is the CPU syscon and which is the GCB syscon. I'm not sure if it
would be better to have two separately named syscon properties with a
single phandle each.

regards
Philipp

Re: [PATCH 2/3] reset: mchp: sparx5: add switch reset driver

[Philipp Zabel]

Hi Steen,

thank you for the patch. In addition to Andrew's comments, I have a few
more below:

On Wed, 2021-01-13 at 21:19 +0100, Steen Hegelund wrote:
> Signed-off-by: Steen Hegelund 
> ---
>  drivers/reset/Kconfig  |   8 ++
>  drivers/reset/Makefile |   1 +
>  drivers/reset/reset-microchip-sparx5.c | 145 +
>  3 files changed, 154 insertions(+)
>  create mode 100644 drivers/reset/reset-microchip-sparx5.c
> 
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 71ab75a46491..05c240c47a8a 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -101,6 +101,14 @@ config RESET_LPC18XX
>   help
> This enables the reset controller driver for NXP LPC18xx/43xx SoCs.
>  
> +config RESET_MCHP_SPARX5
> + bool "Microchip Sparx5 reset driver"
> + depends on HAS_IOMEM || COMPILE_TEST
> + default y if SPARX5_SWITCH
> + select MFD_SYSCON
> + help
> +   This driver supports switch core reset for the Microchip Sparx5 SoC.
> +
>  config RESET_MESON
>   tristate "Meson Reset Driver"
>   depends on ARCH_MESON || COMPILE_TEST
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 1054123fd187..341fd9ab4bf6 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
>  obj-$(CONFIG_RESET_INTEL_GW) += reset-intel-gw.o
>  obj-$(CONFIG_RESET_LANTIQ) += reset-lantiq.o
>  obj-$(CONFIG_RESET_LPC18XX) += reset-lpc18xx.o
> +obj-$(CONFIG_RESET_MCHP_SPARX5) += reset-microchip-sparx5.o
>  obj-$(CONFIG_RESET_MESON) += reset-meson.o
>  obj-$(CONFIG_RESET_MESON_AUDIO_ARB) += reset-meson-audio-arb.o
>  obj-$(CONFIG_RESET_NPCM) += reset-npcm.o
> diff --git a/drivers/reset/reset-microchip-sparx5.c 
> b/drivers/reset/reset-microchip-sparx5.c
> new file mode 100644
> index ..bb636ebd22d2
> --- /dev/null
> +++ b/drivers/reset/reset-microchip-sparx5.c
> @@ -0,0 +1,145 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/* Microchip Sparx5 Switch Reset driver
> + *
> + * Copyright (c) 2020 Microchip Technology Inc. and its subsidiaries.
> + *
> + * The Sparx5 Chip Register Model can be browsed at this location:
> + * https://github.com/microchip-ung/sparx-5_reginfo
> + */
> +#include 
> +#include 
> +#include 

Please drop all unused headers.

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define PROTECT_REG0x84
> +#define PROTECT_BITBIT(10)
> +#define SOFT_RESET_REG 0x08
> +#define SOFT_RESET_BIT BIT(1)
> +
> +struct mchp_reset_context {
> + struct device *dev;
> + struct regmap *cpu_ctrl;
> + struct regmap *gcb_ctrl;
> + struct reset_controller_dev reset_ctrl;

For consistency, I'd like this to be called rcdev, or something else
that doesn't sound like this should be a struct reset_control.

> +};
> +
> +static u32 sparx5_read_soft_rst(struct mchp_reset_context *ctx)
> +{
> + u32 val;
> +
> + regmap_read(ctx->gcb_ctrl, SOFT_RESET_REG, );
> + return val;
> +}

This can be dropped if you use regmap_read_poll_timeout() below.

> +static int sparx5_switch_reset(struct reset_controller_dev *rcdev,
> +   unsigned long id)
> +{
> + struct mchp_reset_context *ctx =
> + container_of(rcdev, struct mchp_reset_context, reset_ctrl);
> + u32 val;
> +
> + /* Make sure the core is PROTECTED from reset */
> + regmap_update_bits(ctx->cpu_ctrl, PROTECT_REG, PROTECT_BIT, 
> PROTECT_BIT);
> +
> + dev_info(ctx->dev, "soft reset of switchcore\n");
> +
> + /* Start soft reset */
> + regmap_write(ctx->gcb_ctrl, SOFT_RESET_REG, SOFT_RESET_BIT);
> +
> + /* Wait for soft reset done */
> + return read_poll_timeout(sparx5_read_soft_rst, val,
> +  (val & SOFT_RESET_BIT) == 0,
> +  1, 100, false,
> +  ctx);

This looks like you could use regmap_read_poll_timeout() here.

> +}
> +
> +static const struct reset_control_ops sparx5_reset_ops = {
> + .reset = sparx5_switch_reset,
> +};
> +
> +static int mchp_sparx5_reset_config(struct platform_device *pdev,
> + struct mchp_reset_context *ctx)
> +{
> + struct device_node *dn = pdev->dev.of_node;
> + struct regmap *cpu_ctrl, *gcb_ctrl;
> + struct device_node *syscon_np;
> + int err;
> +
> + syscon_np = of_parse_phandle(dn, "syscons", 0);
> + if (!syscon_np)
> + return -ENODEV;
> + cpu_ctrl = syscon_node_to_regmap(syscon_np);
> + if (IS_ERR(cpu_ctrl))
> + goto err_cpu;
> + of_node_put(syscon_np);

If you move the of_node_put() up before the IS_ERR() check, you don't
have to repeat it at the err_cpu: label. In fact, if you also move the
error message up here, you can return here and drop the label.

> +
> + syscon_np = of_parse_phandle(dn, "syscons", 1);
> + if (!syscon_np)
> +  

Re: [PATCH v6 14/16] reset: core: fix a kernel-doc markup

[Philipp Zabel]

Hi Mauro,

On Thu, 2021-01-14 at 09:04 +0100, Mauro Carvalho Chehab wrote:
> A function has a different name between their prototype
> and its kernel-doc markup:
> 
>   ../drivers/reset/core.c:888: warning: expecting prototype for 
> device_reset(). Prototype was for __device_reset() instead
> 
> Signed-off-by: Mauro Carvalho Chehab 
> ---
>  drivers/reset/core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index 34e89aa0fb5e..dbf881b586d9 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -875,8 +875,8 @@ struct reset_control *__devm_reset_control_get(struct 
> device *dev,
>  EXPORT_SYMBOL_GPL(__devm_reset_control_get);
>  
>  /**
> - * device_reset - find reset controller associated with the device
> - *and perform reset
> + * __device_reset - find reset controller associated with the device
> + *  and perform reset
>   * @dev: device to be reset by the controller
>   * @optional: whether it is optional to reset the device
>   *

Thank you, applied to reset/next.

regards
Philipp

Re: [PATCH v7 9/9] media: imx-jpeg: Use v4l2 jpeg helpers in mxc-jpeg

[Philipp Zabel]

V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE);
>   if (q_data_out->w == 0 && q_data_out->h == 0) {
>   dev_warn(dev, "Invalid user resolution 0x0");
>   dev_warn(dev, "Keeping resolution from JPEG: %dx%d",
> -  sof.width, sof.height);
> -  q_data_out->w = sof.width;
> -  q_data_out->h = sof.height;
> - } else if (sof.width != q_data_out->w || sof.height != q_data_out->h) {
> +  header.frame.width, header.frame.height);
> +  q_data_out->w = header.frame.width;
> +  q_data_out->h = header.frame.height;
> + } else if (header.frame.width != q_data_out->w ||
> +header.frame.height != q_data_out->h) {
>   dev_err(dev,
>   "Resolution mismatch: %dx%d (JPEG) versus %dx%d(user)",
> - sof.width, sof.height, q_data_out->w, q_data_out->h);
> + header.frame.width, header.frame.height,
> + q_data_out->w, q_data_out->h);
>   return -EINVAL;
>   }
> - if (sof.width % 8 != 0 || sof.height % 8 != 0) {
> + if (header.frame.width % 8 != 0 || header.frame.height % 8 != 0) {
>   dev_err(dev, "JPEG width or height not multiple of 8: %dx%d\n",
> - sof.width, sof.height);
> + header.frame.width, header.frame.height);
>   return -EINVAL;
>   }
> - if (sof.width > MXC_JPEG_MAX_WIDTH ||
> - sof.height > MXC_JPEG_MAX_HEIGHT) {
> + if (header.frame.width > MXC_JPEG_MAX_WIDTH ||
> + header.frame.height > MXC_JPEG_MAX_HEIGHT) {
>   dev_err(dev, "JPEG width or height should be <= 8192: %dx%d\n",
> - sof.width, sof.height);
> + header.frame.width, header.frame.height);
>   return -EINVAL;
>   }
> - if (sof.width < MXC_JPEG_MIN_WIDTH ||
> - sof.height < MXC_JPEG_MIN_HEIGHT) {
> + if (header.frame.width < MXC_JPEG_MIN_WIDTH ||
> + header.frame.height < MXC_JPEG_MIN_HEIGHT) {
>   dev_err(dev, "JPEG width or height should be > 64: %dx%d\n",
> - sof.width, sof.height);
> + header.frame.width, header.frame.height);
>   return -EINVAL;
>   }
> - if (sof.components_no > MXC_JPEG_MAX_COMPONENTS) {
> + if (header.frame.num_components > V4L2_JPEG_MAX_COMPONENTS) {
>   dev_err(dev, "JPEG number of components should be <=%d",
> - MXC_JPEG_MAX_COMPONENTS);
> + V4L2_JPEG_MAX_COMPONENTS);
>   return -EINVAL;
>   }
>   /* check and, if necessary, patch component IDs*/
> + psof = (struct mxc_jpeg_sof *)header.sof.start;
> + psos = (struct mxc_jpeg_sos *)header.sos.start;
>   if (!mxc_jpeg_valid_comp_id(dev, psof, psos))
>   dev_warn(dev, "JPEG component ids should be 0-3 or 1-4");
>  
> - img_fmt = mxc_jpeg_get_image_format(dev, );
> - if (img_fmt == MXC_JPEG_INVALID)
> + fourcc = mxc_jpeg_get_image_format(dev, header);
> + if (fourcc == 0)
>   return -EINVAL;
>  
>   /*
> @@ -1413,12 +1282,11 @@ static int mxc_jpeg_parse(struct mxc_jpeg_ctx *ctx,
>* encoded with 3 components have RGB colorspace, see Recommendation
>* ITU-T T.872 chapter 6.5.3 APP14 marker segment for colour encoding
>*/
> - if (img_fmt == MXC_JPEG_YUV444 && app14 && app14_transform == 0)
> - img_fmt = MXC_JPEG_RGB;
> -
> - if (mxc_jpeg_imgfmt_to_fourcc(img_fmt, )) {
> - dev_err(dev, "Fourcc not found for %d", img_fmt);
> - return -EINVAL;
> + if (fourcc == V4L2_PIX_FMT_YUV24 || fourcc == V4L2_PIX_FMT_RGB24) {
> + if (header.app14_tf == V4L2_JPEG_APP14_TF_CMYK_RGB)
> + fourcc = V4L2_PIX_FMT_RGB24;
> + else
> + fourcc = V4L2_PIX_FMT_YUV24;
>   }

See above, this fixup could be moved into mxc_jpeg_get_image_format().
With that,

Reviewed-by: Philipp Zabel 

regards
Philipp

Re: [PATCH v7 6/9] media: Add parsing for APP14 data segment in jpeg helpers

[Philipp Zabel]

On Mon, 2021-01-11 at 21:28 +0200, Mirela Rabulea wrote:
> From: Mirela Rabulea 
> 
> According to Rec. ITU-T T.872 (06/2012) 6.5.3
> APP14 segment is for color encoding, it contains a transform flag, which
> may have values of 0, 1 and 2 and are interpreted as follows:
> 0 - CMYK for images that are encoded with four components
>   - RGB for images that are encoded with three components
> 1 - An image encoded with three components using YCbCr colour encoding.
> 2 - An image encoded with four components using YCCK colour encoding.
> 
> This is used in imx-jpeg decoder, to distinguish between
> YUV444 and RGB24.
> 
> Signed-off-by: Mirela Rabulea 
> ---
> Changes in v7:
>   Check there are 6 bytes available in the stream before checking for 
> "Adobe\0"
>   Change jpeg_parse_app14_data function to differentiate between the 3 
> scenarios: app14 missing, or app14 present but with/without parsing errors:
> App14 missing => Added V4L2_JPEG_APP14_TF_UNKNOWN to the enum 
> v4l2_jpeg_app14_tf, use it to indicate app14 & TF is missing
>   App14 present without parsing errors => Return the transform flag value 
> as enum v4l2_jpeg_app14_tf (new paramater of jpeg_parse_app14_data function)
> App14 present with parsing errors => Return -EINVAL from 
> jpeg_parse_app14_data, also return from calling function 
> (v4l2_jpeg_parse_header) when this error is met.
>
>  drivers/media/v4l2-core/v4l2-jpeg.c | 47 +++--
>  include/media/v4l2-jpeg.h   | 20 
>  2 files changed, 65 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-jpeg.c 
> b/drivers/media/v4l2-core/v4l2-jpeg.c
> index 8947fd95c6f1..8d5fedb136dd 100644
> --- a/drivers/media/v4l2-core/v4l2-jpeg.c
> +++ b/drivers/media/v4l2-core/v4l2-jpeg.c
> @@ -45,6 +45,7 @@ MODULE_LICENSE("GPL");
>  #define DHP  0xffde  /* hierarchical progression */
>  #define EXP  0xffdf  /* expand reference */
>  #define APP0 0xffe0  /* application data */
> +#define APP140xffee  /* application data for colour encoding */
>  #define APP150xffef
>  #define JPG0 0xfff0  /* extensions */
>  #define JPG130xfffd
> @@ -444,8 +445,44 @@ static int jpeg_skip_segment(struct jpeg_stream *stream)
>   return jpeg_skip(stream, len - 2);
>  }
>  
> +/* Rec. ITU-T T.872 (06/2012) 6.5.3 */
> +static int jpeg_parse_app14_data(struct jpeg_stream *stream,
> +  enum v4l2_jpeg_app14_tf *tf)
> +{
> + int ret;
> + int lp;
> + int skip;
> +
> + lp = jpeg_get_word_be(stream);
> + if (lp < 0)
> + return lp;
> +
> + /* Check for "Adobe\0" in Ap1..6 */
> + if (stream->curr + 6 > stream->end ||
> + strncmp(stream->curr, "Adobe\0", 6))
> + return -EINVAL;
> +
> + /* get to Ap12 */
> + ret = jpeg_skip(stream, 11);
> + if (ret < 0)
> + return ret;
> +
> + ret = jpeg_get_byte(stream);
> + if (ret < 0)
> + return ret;
> +
> + *tf = ret;
> +
> + skip = lp - 2 - 11;

> + ret = jpeg_skip(stream, skip);
> + if (ret < 0)
> + return ret;
> +
> + return 0;

This could be simplified to

return jpeg_skip(stream, skip);

although it would be better style to move the *tf = ... assignment down
past the last error return instead. Either way,

Reviewed-by: Philipp Zabel 

regards
Philipp

Re: linux-next: Tree for Jan 7 (drivers/gpu/drm/imx/parallel-display.o)

[Philipp Zabel]

On Fri, 2021-01-08 at 10:03 +0100, Daniel Vetter wrote:
> On Fri, Jan 8, 2021 at 9:55 AM Randy Dunlap  wrote:
> > On 1/6/21 7:01 PM, Stephen Rothwell wrote:
> > > Hi all,
> > > 
> > > Changes since 20210106:
> > > 
> > 
> > on x86_64:
> > 
> > ld: drivers/gpu/drm/imx/parallel-display.o: in function 
> > `imx_pd_connector_get_modes':
> > parallel-display.c:(.text+0x8d): undefined reference to 
> > `of_get_drm_display_mode'
>
> Probably something in the pull from philipp that I just merged
> yesterday.

That is likely, although I'm not quite sure how that activated, the
of_get_drm_display_mode call was there before.

> Philip, can you pls take care?
> -Daniel

Thank you for the notice, since the of_get_drm_display_mode() is only
for legacy device trees I'd like to stub it out and keep compile test
coverage:

https://lore.kernel.org/dri-devel/20210108101343.23695-1-p.za...@pengutronix.de/T/#u

regards
Philipp

Re: [PATCH v2 -next] media: hantro: convert comma to semicolon

[Philipp Zabel]

On Fri, 2021-01-08 at 17:22 +0800, Zheng Yongjun wrote:
> Replace a comma between expression statements by a semicolon.
> 
> Signed-off-by: Zheng Yongjun 
> ---
>  drivers/staging/media/hantro/hantro_v4l2.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_v4l2.c 
> b/drivers/staging/media/hantro/hantro_v4l2.c
> index b668a82d40ad..e1081c16f56a 100644
> --- a/drivers/staging/media/hantro/hantro_v4l2.c
> +++ b/drivers/staging/media/hantro/hantro_v4l2.c
> @@ -316,7 +316,7 @@ hantro_reset_fmt(struct v4l2_pix_format_mplane *fmt,
>  
>   fmt->pixelformat = vpu_fmt->fourcc;
>   fmt->field = V4L2_FIELD_NONE;
> - fmt->colorspace = V4L2_COLORSPACE_JPEG,
> + fmt->colorspace = V4L2_COLORSPACE_JPEG;
>   fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
>   fmt->quantization = V4L2_QUANTIZATION_DEFAULT;
>   fmt->xfer_func = V4L2_XFER_FUNC_DEFAULT;

Thank you,

Reviewed-by: Philipp Zabel 

regards
Philipp

Re: [PATCH 3/5] dt-bindings: media: rockchip-vpu: Add PX30 compatible

[Philipp Zabel]

Hi Paul,

On Thu, 2021-01-07 at 14:40 +0100, Paul Kocialkowski wrote:
> The Rockchip PX30 SoC has a Hantro VPU that features a decoder (VDPU2)
> and an encoder (VEPU2). It is similar to the RK3399's VPU but takes an
> extra clock (SCLK).
> 
> Signed-off-by: Paul Kocialkowski 
> ---
>  .../bindings/media/rockchip-vpu.yaml  | 25 +--
>  1 file changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/rockchip-vpu.yaml 
> b/Documentation/devicetree/bindings/media/rockchip-vpu.yaml
> index c81dbc3e8960..c446b9ead21b 100644
> --- a/Documentation/devicetree/bindings/media/rockchip-vpu.yaml
> +++ b/Documentation/devicetree/bindings/media/rockchip-vpu.yaml
> @@ -15,10 +15,13 @@ description:
>  
>  properties:
>compatible:
> -enum:
> -  - rockchip,rk3288-vpu
> -  - rockchip,rk3328-vpu
> -  - rockchip,rk3399-vpu
> +oneOf:
> +  - const: rockchip,rk3288-vpu
> +  - const: rockchip,rk3328-vpu
> +  - const: rockchip,rk3399-vpu
> +  - items:
> +- const: rockchip,px30-vpu
> +- const: rockchip,rk3399-vpu
>  
>reg:
>  maxItems: 1
> @@ -35,12 +38,18 @@ properties:
>- const: vdpu
>  
>clocks:
> -maxItems: 2
> +minItems: 2
> +maxItems: 3
>  
>clock-names:
> -items:
> -  - const: aclk
> -  - const: hclk
> +oneOf:
> +  - items:
> +- const: aclk
> +- const: hclk
> +  - items:
> +- const: aclk
> +- const: hclk
> +- const: sclk

You could make this:

clock-names:
  minItems: 2
  items:
- const: aclk
- const: hclk
- const: sclk

And then:

allOf:
  - if:
  properties:
compatible:
  contains:
const: rockchip,px30-vpu
then:
  properties:
clock-names:
  minItems: 3

to make sure each variant has the correct clocks set.

regards
Philipp

Re: [PATCH -next] media: hantro: use resource_size

[Philipp Zabel]

Hi Zheng,

On Wed, 2021-01-06 at 21:18 +0800, Zheng Yongjun wrote:
> Use resource_size rather than a verbose computation on
> the end and start fields.
> 
> Signed-off-by: Zheng Yongjun 
> ---
>  drivers/staging/media/hantro/hantro_v4l2.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_v4l2.c 
> b/drivers/staging/media/hantro/hantro_v4l2.c
> index b668a82d40ad..e1081c16f56a 100644
> --- a/drivers/staging/media/hantro/hantro_v4l2.c
> +++ b/drivers/staging/media/hantro/hantro_v4l2.c
> @@ -316,7 +316,7 @@ hantro_reset_fmt(struct v4l2_pix_format_mplane *fmt,
>  
>   fmt->pixelformat = vpu_fmt->fourcc;
>   fmt->field = V4L2_FIELD_NONE;
> - fmt->colorspace = V4L2_COLORSPACE_JPEG,
> + fmt->colorspace = V4L2_COLORSPACE_JPEG;
>   fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
>   fmt->quantization = V4L2_QUANTIZATION_DEFAULT;
>   fmt->xfer_func = V4L2_XFER_FUNC_DEFAULT;

Subject and commit message do not describe the patch.

regards
Philipp

Re: [PATCH v6 8/9] media: Avoid parsing quantization and huffman tables

[Philipp Zabel]

On Tue, 2020-12-15 at 13:18 +0200, Mirela Rabulea (OSS) wrote:
> From: Mirela Rabulea 
> 
> These are optional in struct v4l2_jpeg_header, so skip DHT/DQT segment
> parsing if huffman_tables/quantization_tables were not requested by user,
> to save time.
> However, do count them (num_dht/num_dqt).
> 
> Signed-off-by: Mirela Rabulea 
> ---
> Changes in v6:
>   Call jpeg_skip_segment() instead of 
> jpeg_parse_quantization_table()/jpeg_parse_huffman_tables(),
>   when quantization/huffman tables are not requested by user.
>   Remove the NULL pointer check in the lower-level function.
>   Thanks Philipp & Hans for feedback
> 
>  drivers/media/v4l2-core/v4l2-jpeg.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-jpeg.c 
> b/drivers/media/v4l2-core/v4l2-jpeg.c
> index dc9def4c2648..f3d03d39defb 100644
> --- a/drivers/media/v4l2-core/v4l2-jpeg.c
> +++ b/drivers/media/v4l2-core/v4l2-jpeg.c
> @@ -537,6 +537,10 @@ int v4l2_jpeg_parse_header(void *buf, size_t len, struct 
> v4l2_jpeg_header *out)
>   >dht[out->num_dht++ % 4]);
>   if (ret < 0)
>   return ret;
> + if (!out->huffman_tables) {
> + ret = jpeg_skip_segment();
> + break;
> + }
>   ret = jpeg_parse_huffman_tables(,
>   out->huffman_tables);
>   break;
> @@ -545,6 +549,10 @@ int v4l2_jpeg_parse_header(void *buf, size_t len, struct 
> v4l2_jpeg_header *out)
>   >dqt[out->num_dqt++ % 4]);
>   if (ret < 0)
>   return ret;
> + if (!out->quantization_tables) {
> + ret = jpeg_skip_segment();
> + break;
> + }
>   ret = jpeg_parse_quantization_tables(,
>   out->frame.precision,
>   out->quantization_tables);

Reviewed-by: Philipp Zabel 

regards
Philipp

Re: [PATCH v6 6/9] media: Add parsing for APP14 data segment in jpeg helpers

[Philipp Zabel]

Hi Mirela,

thank you for the update. Just two issues below:

On Tue, 2020-12-15 at 13:18 +0200, Mirela Rabulea (OSS) wrote:
> From: Mirela Rabulea 
> 
> According to Rec. ITU-T T.872 (06/2012) 6.5.3
> APP14 segment is for color encoding, it contains a transform flag, which
> may have values of 0, 1 and 2 and are interpreted as follows:
> 0 - CMYK for images that are encoded with four components
>   - RGB for images that are encoded with three components
> 1 - An image encoded with three components using YCbCr colour encoding.
> 2 - An image encoded with four components using YCCK colour encoding.
> 
> This is used in imx-jpeg decoder, to distinguish between
> YUV444 and RGB24.
> 
> Signed-off-by: Mirela Rabulea 
> ---
> Changes in v6:
>   Switch variable to lowercase Lp->lp
>   Check for "Adobe\0" in Ap1..6
>   Make the transform flag an enum
>   Removed a change in comment section, a leftover from a previous version
>   Thanks Philipp for feedback.
> 
>  drivers/media/v4l2-core/v4l2-jpeg.c | 43 +++--
>  include/media/v4l2-jpeg.h   | 18 
>  2 files changed, 59 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-jpeg.c 
> b/drivers/media/v4l2-core/v4l2-jpeg.c
> index 8947fd95c6f1..d1483e7a775c 100644
> --- a/drivers/media/v4l2-core/v4l2-jpeg.c
> +++ b/drivers/media/v4l2-core/v4l2-jpeg.c
> @@ -45,6 +45,7 @@ MODULE_LICENSE("GPL");
>  #define DHP  0xffde  /* hierarchical progression */
>  #define EXP  0xffdf  /* expand reference */
>  #define APP0 0xffe0  /* application data */
> +#define APP140xffee  /* application data for colour encoding */
>  #define APP150xffef
>  #define JPG0 0xfff0  /* extensions */
>  #define JPG130xfffd
> @@ -444,8 +445,41 @@ static int jpeg_skip_segment(struct jpeg_stream *stream)
>   return jpeg_skip(stream, len - 2);
>  }
>  
> +/* Rec. ITU-T T.872 (06/2012) 6.5.3 */
> +static int jpeg_parse_app14_data(struct jpeg_stream *stream)
> +{
> + int ret;
> + int lp;
> + int skip;
> + int tf;
> +
> + lp = jpeg_get_word_be(stream);
> + if (lp < 0)
> + return lp;

Here we should check that there are still 6 bytes available to compare:

if (stream->curr + 6 > stream->end)
return -EINVAL;

> + /* Check for "Adobe\0" in Ap1..6 */
> + if (strncmp(stream->curr, "Adobe\0", 6))
> + return -EINVAL;
> +
> + /* get to Ap12 */
> + ret = jpeg_skip(stream, 11);
> + if (ret < 0)
> + return -EINVAL;
> +
> + tf = jpeg_get_byte(stream);
> + if (tf < 0)
> + return tf;
> +
> + skip = lp - 2 - 11;
> + ret = jpeg_skip(stream, skip);
> + if (ret < 0)
> + return -EINVAL;
> + else
> + return tf;
> +}
> +
>  /**
> - * jpeg_parse_header - locate marker segments and optionally parse headers
> + * v4l2_jpeg_parse_header - locate marker segments and optionally parse 
> headers
>   * @buf: address of the JPEG buffer, should start with a SOI marker
>   * @len: length of the JPEG buffer
>   * @out: returns marker segment positions and optionally parsed headers
> @@ -476,6 +510,9 @@ int v4l2_jpeg_parse_header(void *buf, size_t len, struct 
> v4l2_jpeg_header *out)
>   if (marker != SOI)
>   return -EINVAL;
>  
> + /* init value to signal if this marker is not present */
> + out->app14_tf = -EINVAL;
> +

Here we set app14_tf to a value that is not part of the enum.
You could define a value V4L2_JPEG_APP14_TF_UNKNOWN for the
uninitialized / error state.

>   /* loop through marker segments */
>   while ((marker = jpeg_next_marker()) >= 0) {
>   switch (marker) {
> @@ -519,7 +556,9 @@ int v4l2_jpeg_parse_header(void *buf, size_t len, struct 
> v4l2_jpeg_header *out)
>   ret = jpeg_parse_restart_interval(,
>   >restart_interval);
>   break;
> -
> + case APP14:
> + out->app14_tf = jpeg_parse_app14_data();

Same as above in case of -EINVAL return. Apart from this,

Reviewed-by: Philipp Zabel 

regards
Philipp

Re: [PATCH -next] media/platform/coda: convert comma to semicolon

[Philipp Zabel]

On Wed, 2020-12-16 at 21:21 +0800, Zheng Yongjun wrote:
> Replace a comma between expression statements by a semicolon.
> 
> Signed-off-by: Zheng Yongjun 
> ---
>  drivers/media/platform/coda/coda-common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/coda/coda-common.c 
> b/drivers/media/platform/coda/coda-common.c
> index 87a2c706f747..547ad34b1424 100644
> --- a/drivers/media/platform/coda/coda-common.c
> +++ b/drivers/media/platform/coda/coda-common.c
> @@ -2861,7 +2861,7 @@ static int coda_register_device(struct coda_dev *dev, 
> int i)
>   strscpy(vfd->name, dev->devtype->vdevs[i]->name, sizeof(vfd->name));
>   vfd->fops   = _fops;
>   vfd->ioctl_ops  = _ioctl_ops;
> - vfd->release= video_device_release_empty,
> + vfd->release= video_device_release_empty;
>   vfd->lock   = >dev_mutex;
>   vfd->v4l2_dev   = >v4l2_dev;
>   vfd->vfl_dir= VFL_DIR_M2M;

Thank you,

Reviewed-by: Philipp Zabel 

regards
Philipp

Re: [PATCH -next] gpu: drm: imx: convert comma to semicolon

[Philipp Zabel]

On Fri, 2020-12-11 at 16:58 +0800, Zheng Yongjun wrote:
> Replace a comma between expression statements by a semicolon.
> 
> Signed-off-by: Zheng Yongjun 
> ---
>  drivers/gpu/drm/imx/parallel-display.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/imx/parallel-display.c 
> b/drivers/gpu/drm/imx/parallel-display.c
> index 2eb8df4697df..c4dab79a9385 100644
> --- a/drivers/gpu/drm/imx/parallel-display.c
> +++ b/drivers/gpu/drm/imx/parallel-display.c
> @@ -74,7 +74,7 @@ static int imx_pd_connector_get_modes(struct drm_connector 
> *connector)
>   return ret;
>  
>   drm_mode_copy(mode, >mode);
> - mode->type |= DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED,
> + mode->type |= DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
>   drm_mode_probed_add(connector, mode);
>   num_modes++;
>   }

Thank you, applied to imx-drm/next.

regards
Philipp

Re: [PATCH v3 0/4] dt-bindings: reset: convert Hisilicon reset controller bindings to json-schema

[Philipp Zabel]

On Tue, 2020-12-08 at 20:46 +0800, Zhen Lei wrote:
> v2 --> v3:
> 1. Keep device tree patches and reset driver patch separate, as they were in 
> v1.
>That is, revert v2.
> 2. When the new compatible match failed, fall back to the deprecated 
> compatible.
> 3. Fix a typo, correct "hi3660,rst-syscon" to "hisilicon,rst-syscon".
> 
> v1 --> v2:
> Merge the driver and DT modification(correct vendor prefix hisi to hisilicon) 
> into one patch.
> 
> v1:
> Patch 1-3 change the vendor prefix from "hisi" to "hisilicon", to eliminate 
> below warnings:
>   crg_rst_controller: 'hisi,rst-syscon' does not match any of the regexes: 
> '^#.*', ... , '^hisilicon,.*', ...
>   From schema: 
> /root/leizhen/linux-next/Documentation/devicetree/bindings/vendor-prefixes.yaml
> 
> Patch 4 does the json-schema conversion.

Thank you, I've applied patches 1, 3, and 4 to the reset/next branch.

regards
Philipp

Re: [PATCH 1/2] dt-bindings: reset: document Broadcom's BCM4908 PCIe reset binding

[Philipp Zabel]

On Fri, 2020-11-27 at 12:14 +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki 
> 
> BCM4908 was built using older PCIe hardware block that requires using
> external reset block controlling PERST# signals.
> 
> Signed-off-by: Rafał Miłecki 

Thank you, both applied to reset/next.

regards
Philipp

Re: [PATCH v2 1/3] reset: hisilicon: correct vendor prefix

[Philipp Zabel]

Hi Zhen,

On Fri, 2020-12-04 at 09:42 +0800, Zhen Lei wrote:
> The vendor prefix of "Hisilicon Limited" is "hisilicon", it is clearly
> stated in "vendor-prefixes.yaml".
> 
> Fixes: 1527058736fa ("reset: hisilicon: add reset-hi3660")
> Fixes: 35ca8168133c ("arm64: dts: Add dts files for Hisilicon Hi3660 SoC")
> Fixes: dd8c7b78c11b ("arm64: dts: Add devicetree for Hisilicon Hi3670 SoC")
> Signed-off-by: Zhen Lei 
> Cc: Zhangfei Gao 
> Cc: Chen Feng 
> Cc: Manivannan Sadhasivam 
> ---
>  arch/arm64/boot/dts/hisilicon/hi3660.dtsi | 4 ++--
>  arch/arm64/boot/dts/hisilicon/hi3670.dtsi | 2 +-
>  drivers/reset/hisilicon/reset-hi3660.c| 2 +-

Please keep device tree patches and reset driver patch separate, as they
were in v1.

>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi 
> b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> index 49c19c6879f95ce..bfb1375426d2b58 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi3660.dtsi
> @@ -345,7 +345,7 @@
>   crg_rst: crg_rst_controller {
>   compatible = "hisilicon,hi3660-reset";
>   #reset-cells = <2>;
> - hisi,rst-syscon = <_ctrl>;
> + hisilicon,rst-syscon = <_ctrl>;
>   };
>  
>  
> @@ -376,7 +376,7 @@
>  
>   iomcu_rst: reset {
>   compatible = "hisilicon,hi3660-reset";
> - hisi,rst-syscon = <>;
> + hisilicon,rst-syscon = <>;
>   #reset-cells = <2>;
>   };
>  
> diff --git a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi 
> b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> index 85b0dfb35d6d396..5c5a5dc964ea848 100644
> --- a/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> +++ b/arch/arm64/boot/dts/hisilicon/hi3670.dtsi
> @@ -155,7 +155,7 @@
>   compatible = "hisilicon,hi3670-reset",
>"hisilicon,hi3660-reset";
>   #reset-cells = <2>;
> - hisi,rst-syscon = <_ctrl>;
> + hisilicon,rst-syscon = <_ctrl>;
>   };
>  
>   pctrl: pctrl@e8a09000 {
> diff --git a/drivers/reset/hisilicon/reset-hi3660.c 
> b/drivers/reset/hisilicon/reset-hi3660.c
> index a7d4445924e558c..8f1953159a65b31 100644
> --- a/drivers/reset/hisilicon/reset-hi3660.c
> +++ b/drivers/reset/hisilicon/reset-hi3660.c
> @@ -83,7 +83,7 @@ static int hi3660_reset_probe(struct platform_device *pdev)
>   if (!rc)
>   return -ENOMEM;
>  
> - rc->map = syscon_regmap_lookup_by_phandle(np, "hisi,rst-syscon");
> + rc->map = syscon_regmap_lookup_by_phandle(np, "hisilicon,rst-syscon");

This should fall back to the deprecated compatible, for example:

rc->map = syscon_regmap_lookup_by_phandle(np, "hisilicon,rst-syscon");
if (PTR_ERR(rc->map) == -ENODEV)
rc->map = syscon_regmap_lookup_by_phandle(np, "hisi,rst-
syscon");

>   if (IS_ERR(rc->map)) {
>   dev_err(dev, "failed to get hi3660,rst-syscon\n");
>   return PTR_ERR(rc->map);

regards
Philipp

Re: [PATCH] staging:hantro: Fixed "replace comma with semicolon" Warning:

[Philipp Zabel]

Hi Travis,

On Fri, 2020-12-04 at 17:51 -0600, Travis Carter wrote:
> Corrected the following Warning:
> drivers/staging/media/hantro/hantro_v4l2.c:319: WARNING: Possible comma where 
> semicolon could be used
> 
> Signed-off-by: Travis Carter 
> ---
>  drivers/staging/media/hantro/hantro_v4l2.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/hantro/hantro_v4l2.c 
> b/drivers/staging/media/hantro/hantro_v4l2.c
> index b668a82d40ad..e1081c16f56a 100644
> --- a/drivers/staging/media/hantro/hantro_v4l2.c
> +++ b/drivers/staging/media/hantro/hantro_v4l2.c
> @@ -316,7 +316,7 @@ hantro_reset_fmt(struct v4l2_pix_format_mplane *fmt,
>  
>   fmt->pixelformat = vpu_fmt->fourcc;
>   fmt->field = V4L2_FIELD_NONE;
> - fmt->colorspace = V4L2_COLORSPACE_JPEG,
> + fmt->colorspace = V4L2_COLORSPACE_JPEG;
>   fmt->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
>   fmt->quantization = V4L2_QUANTIZATION_DEFAULT;
>   fmt->xfer_func = V4L2_XFER_FUNC_DEFAULT;

Thank you,

Reviewed-by: Philipp Zabel 

regards
Philipp

Re: [PATCH 2/2] reset: bcm4908-usb: add driver for BCM4908 USB reset controller

[Philipp Zabel]

Hi Rafał,

On Fri, 2020-12-04 at 10:37 +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki 
> 
> This controller is responsible for OHCI, EHCI, XHCI and PHYs setup that
> has to be handled in the proper order.
> 
> One unusual thing about this controller is that is provides access to
> the MDIO bus. There are two registers (in the middle of block space)
> responsible for that. For that reason this driver initializes regmap so
> a proper MDIO driver can use them.
> 
> Signed-off-by: Rafał Miłecki 

This doesn't look like a reset controller to me, but rather like
something that belongs in drivers/usb.

regards
Philipp

> ---
>  drivers/reset/Kconfig |   8 +
>  drivers/reset/Makefile|   1 +
>  drivers/reset/reset-bcm4908-usb.c | 250 ++
>  3 files changed, 259 insertions(+)
>  create mode 100644 drivers/reset/reset-bcm4908-usb.c
> 
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index f393f7e17e33..bb4d2bea9e95 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -35,6 +35,14 @@ config RESET_AXS10X
>   help
> This enables the reset controller driver for AXS10x.
>  
> +config RESET_BCM4908_USB
> + tristate "Broadcom BCM4908 USB controller"
> + depends on ARM64 || COMPILE_TEST
> + default ARCH_BCM4908
> + help
> +   This enables driver for the Broadcom BCM4908 USB controller
> +   responsible for initializing OHCI, EHCI, XHCI and PHYs.
> +
>  config RESET_BERLIN
>   bool "Berlin Reset Driver" if COMPILE_TEST
>   default ARCH_BERLIN
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 0dd5d42050dc..f2627bbc7ad4 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_ARCH_TEGRA) += tegra/
>  obj-$(CONFIG_RESET_A10SR) += reset-a10sr.o
>  obj-$(CONFIG_RESET_ATH79) += reset-ath79.o
>  obj-$(CONFIG_RESET_AXS10X) += reset-axs10x.o
> +obj-$(CONFIG_RESET_BCM4908_USB) += reset-bcm4908-usb.o
>  obj-$(CONFIG_RESET_BERLIN) += reset-berlin.o
>  obj-$(CONFIG_RESET_BRCM_PMB) += reset-brcm-pmb.o
>  obj-$(CONFIG_RESET_BRCMSTB) += reset-brcmstb.o
> diff --git a/drivers/reset/reset-bcm4908-usb.c 
> b/drivers/reset/reset-bcm4908-usb.c
> new file mode 100644
> index ..e9b7d369c894
> --- /dev/null
> +++ b/drivers/reset/reset-bcm4908-usb.c
> @@ -0,0 +1,250 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2013 Broadcom
> + * Copyright (C) 2020 Rafał Miłecki 
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define BCM4908_USB_RESET_SETUP  
> 0x
> +#define  BCM4908_USBH_IPP(1<<5)
> +#define  BCM4908_USBH_IOC(1<<4)
> +#define  BCM4908_USB2_OC_DISABLE_PORT0   
> (1<<28)
> +#define  BCM4908_USB2_OC_DISABLE_PORT1   
> (1<<29)
> +#define  BCM4908_USB3_OC_DISABLE_PORT0   
> (1<<30)
> +#define  BCM4908_USB3_OC_DISABLE_PORT1   
> (1<<31)
> +#define BCM4908_USB_RESET_PLL_CTL0x0004
> +#define BCM4908_USB_RESET_FLADJ_VALUE
> 0x0008
> +#define BCM4908_USB_RESET_BRIDGE_CTL 0x000c
> +#define BCM4908_USB_RESET_SPARE1 0x0010
> +#define BCM4908_USB_RESET_MDIO   
> 0x0014
> +#define BCM4908_USB_RESET_MDIO2  
> 0x0018
> +#define BCM4908_USB_RESET_TEST_PORT_CONTROL  0x001c
> +#define BCM4908_USB_RESET_USB_SIMCTL 0x0020
> +#define  BCM4908_USBH_OHCI_MEM_REQ_DIS   
> (1<<1)
> +#define BCM4908_USB_RESET_USB_TESTCTL
> 0x0024
> +#define BCM4908_USB_RESET_USB_TESTMON
> 0x0028
> +#define BCM4908_USB_RESET_UTMI_CTL_1 0x002c
> +#define BCM4908_USB_RESET_SPARE2 0x0030
> +#define BCM4908_USB_RESET_USB_PM 0x0034
> +#define  BCM4908_XHC_SOFT_RESETB (1<<22)
> +#define  BCM4908_USB_PWRDWN  (1<<31)
> +#define BCM4908_USB_RESET_USB_PM_STATUS  
> 0x0038
> +#define BCM4908_USB_RESET_SPARE3 0x003c
> +#define BCM4908_USB_RESET_PLL_LDO_CTL
> 0x0040
> +#define BCM4908_USB_RESET_PLL_LDO_PLLBIAS0x0044
> +#define BCM4908_USB_RESET_PLL_AFE_BG_CNTL0x0048
> 

Re: [EXT] Re: [PATCH v5 07/10] media: Add parsing for APP14 data segment in jpeg helpers

[Philipp Zabel]

On Fri, 2020-12-04 at 14:13 +, Mirela Rabulea (OSS) wrote:
> Hi Phipipp,
>
> On Wed, 2020-12-02 at 16:18 +0100, Philipp Zabel wrote:
> > Hi Mirela,
> > 
> > On Thu, 2020-11-12 at 05:05 +0200, Mirela Rabulea (OSS) wrote:
> > > From: Mirela Rabulea 
> > > 
> > > According to Rec. ITU-T T.872 (06/2012) 6.5.3
> > > APP14 segment is for color encoding, it contains a transform flag,
> > > which
> > > may have values of 0, 1 and 2 and are interpreted as follows:
> > > 0 - CMYK for images that are encoded with four components
> > >   - RGB for images that are encoded with three components
> > > 1 - An image encoded with three components using YCbCr colour
> > > encoding.
> > > 2 - An image encoded with four components using YCCK colour
> > > encoding.
> > > 
> > > This is used in imx-jpeg decoder, to distinguish between
> > > YUV444 and RGB24.
> > > 
> > > Signed-off-by: Mirela Rabulea 
> > > ---
> > > Changes in v5:
> > > This was patch 8 in previous version
> > > Replaced a struct for app14 data with just an int, since the
> > > transform flag is the only meaningfull information from this
> > > segment
> > 
> > Could we turn this into an enum for the transform flag, and include
> > the
> > above spec reference in its kerneldoc comment? I think this would be
> > better than checking for (app14_tf == ) in the drivers.
> 
> Appreciate your feedback, for all patches, I'll address it in v6.
> Where would be a better place for this enum, v4l2-jpeg.h, or maybe
> include/uapi/linux/v4l2-controls.h?

v4l2-jpeg.h seems like the right place to me.

regards
Philipp

Re: [PATCH 1/5] media: dt-bindings: add the required property 'additionalProperties'

[Philipp Zabel]

On Fri, 2020-12-04 at 17:38 +0800, Zhen Lei wrote:
> When I do dt_binding_check for any YAML file, below wanring is always
> reported:
> 
> xxx/media/coda.yaml: 'additionalProperties' is a required property
> xxx/media/coda.yaml: ignoring, error in schema:
> warning: no schema found in file: xxx/media/coda.yaml
> 
> There are three properties defined in allOf, they should be explicitly
> declared. Otherwise, "additionalProperties: false" will prohibit them.
> 
> Signed-off-by: Zhen Lei 

Thank you, there already is a patch to fix this:

https://lore.kernel.org/linux-media/20201117200752.4004368-1-r...@kernel.org/

regards
Philipp

Re: [PATCH 1/4] reset: hisilicon: correct vendor prefix

[Philipp Zabel]

On Thu, 2020-12-03 at 20:02 +0800, Zhen Lei wrote:
> The vendor prefix of "Hisilicon Limited" is "hisilicon", it is clearly
> stated in "vendor-prefixes.yaml".
> 
> Fixes: 1527058736fa ("reset: hisilicon: add reset-hi3660")
> Signed-off-by: Zhen Lei 
> Cc: Zhangfei Gao 
> ---
>  drivers/reset/hisilicon/reset-hi3660.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/reset/hisilicon/reset-hi3660.c 
> b/drivers/reset/hisilicon/reset-hi3660.c
> index a7d4445924e558c..8f1953159a65b31 100644
> --- a/drivers/reset/hisilicon/reset-hi3660.c
> +++ b/drivers/reset/hisilicon/reset-hi3660.c
> @@ -83,7 +83,7 @@ static int hi3660_reset_probe(struct platform_device *pdev)
>   if (!rc)
>   return -ENOMEM;
>  
> - rc->map = syscon_regmap_lookup_by_phandle(np, "hisi,rst-syscon");
> + rc->map = syscon_regmap_lookup_by_phandle(np, "hisilicon,rst-syscon");

What about those that don't upgrade kernel and DT in lock-step?
It would be easy to fall back to the old compatible if the new one
fails.

regards
Philipp

Re: [v6,3/3] reset-controller: ti: force the write operation when assert or deassert

[Philipp Zabel]

On Wed, 2020-09-30 at 10:21 +0800, Crystal Guo wrote:
> Force the write operation in case the read already happens
> to return the correct value.
> 
> Signed-off-by: Crystal Guo 
> ---
>  drivers/reset/reset-ti-syscon.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/reset/reset-ti-syscon.c b/drivers/reset/reset-ti-syscon.c
> index 5d1f8306cd4f..c34394f1e9e2 100644
> --- a/drivers/reset/reset-ti-syscon.c
> +++ b/drivers/reset/reset-ti-syscon.c
> @@ -97,7 +97,7 @@ static int ti_syscon_reset_assert(struct 
> reset_controller_dev *rcdev,
>   mask = BIT(control->assert_bit);
>   value = (control->flags & ASSERT_SET) ? mask : 0x0;
>  
> - return regmap_update_bits(data->regmap, control->assert_offset, mask, 
> value);
> + return regmap_write_bits(data->regmap, control->assert_offset, mask, 
> value);
>  }
>  
>  /**
> @@ -128,7 +128,7 @@ static int ti_syscon_reset_deassert(struct 
> reset_controller_dev *rcdev,
>   mask = BIT(control->deassert_bit);
>   value = (control->flags & DEASSERT_SET) ? mask : 0x0;
>  
> - return regmap_update_bits(data->regmap, control->deassert_offset, mask, 
> value);
> + return regmap_write_bits(data->regmap, control->deassert_offset, mask, 
> value);
>  }
>  
>  /**
> -- 
> 2.18.0

Thank you. Since Suman tested v4, this should be safe.
Applied to reset/next.

regards
Philipp

Re: [v6,1/3] dt-binding: reset-controller: mediatek: add YAML schemas

[Philipp Zabel]

Hi,

On Wed, 2020-09-30 at 10:21 +0800, Crystal Guo wrote:
> Add a YAML documentation for Mediatek, which uses ti reset-controller
> driver directly. The TI reset controller provides a common reset
> management, and is suitable for Mediatek SoCs.
> 
> Signed-off-by: Crystal Guo 
> ---
>  .../bindings/reset/mediatek-syscon-reset.yaml | 51 +++
>  1 file changed, 51 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/reset/mediatek-syscon-reset.yaml
> 
> diff --git 
> a/Documentation/devicetree/bindings/reset/mediatek-syscon-reset.yaml 
> b/Documentation/devicetree/bindings/reset/mediatek-syscon-reset.yaml
> new file mode 100644
> index ..7871550c3c69
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/mediatek-syscon-reset.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/reset/mediatek-syscon-reset.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Mediatek Reset Controller
> +
> +maintainers:
> +  - Crystal Guo 
> +
> +description:
> +  The bindings describe the reset-controller for Mediatek SoCs,
> +  which is based on TI reset controller. For more detail, please
> +  visit Documentation/devicetree/bindings/reset/ti-syscon-reset.txt.
> +
> +properties:
> +  compatible:
> +const: mediatek,syscon-reset
> +
> +  '#reset-cells':
> +const: 1
> +
> +  mediatek,reset-bits:
> +description: >
> +  Contains the reset control register information, please refer to
> +  Documentation/devicetree/bindings/reset/ti-syscon-reset.txt.

I would really like some input from Rob on this, in v4 he asked not to
repeat 'ti,reset-bits'.

regards
Philipp

Re: [PATCH v4] media: coda: Convert the driver to DT-only

[Philipp Zabel]

On Wed, 2020-12-02 at 11:13 -0300, Fabio Estevam wrote:
> Since 5.10-rc1 i.MX is a devicetree-only platform, so simplify the code
> by removing the unused non-DT support.
> 
> Signed-off-by: Fabio Estevam 

Thank you, this looks fine to me now.

Reviewed-by: Philipp Zabel 

regards
Philipp

Re: [PATCH v5 10/10] media: imx-jpeg: Use v4l2 jpeg helpers in mxc-jpeg

[Philipp Zabel]

On Thu, 2020-11-12 at 05:05 +0200, Mirela Rabulea (OSS) wrote:
> From: Mirela Rabulea 
> 
> Use v4l2_jpeg_parse_header in mxc_jpeg_parse, remove the old
> parsing way, which was duplicated in other jpeg drivers.
> 
> Signed-off-by: Mirela Rabulea 
> ---
> Changes in v5:
> This was patch 11 in previous version
> Change triggered by patch 7 (app14 data change struct -> int)
> 
>  drivers/media/platform/imx-jpeg/Kconfig|   1 +
>  drivers/media/platform/imx-jpeg/mxc-jpeg.c | 267 ++---
>  drivers/media/platform/imx-jpeg/mxc-jpeg.h |  26 +-
>  3 files changed, 80 insertions(+), 214 deletions(-)
> 
> diff --git a/drivers/media/platform/imx-jpeg/Kconfig 
> b/drivers/media/platform/imx-jpeg/Kconfig
> index 7cc89e5eff90..d875f7c88cda 100644
> --- a/drivers/media/platform/imx-jpeg/Kconfig
> +++ b/drivers/media/platform/imx-jpeg/Kconfig
> @@ -4,6 +4,7 @@ config VIDEO_IMX8_JPEG
>   depends on VIDEO_DEV && VIDEO_V4L2
>   select VIDEOBUF2_DMA_CONTIG
>   select V4L2_MEM2MEM_DEV
> + select V4L2_JPEG_HELPER
>   default m
>   help
> This is a video4linux2 driver for the i.MX8 QXP/QM integrated
> diff --git a/drivers/media/platform/imx-jpeg/mxc-jpeg.c 
> b/drivers/media/platform/imx-jpeg/mxc-jpeg.c
> index 8f297803f2c3..d3b7581ce46e 100644
> --- a/drivers/media/platform/imx-jpeg/mxc-jpeg.c
> +++ b/drivers/media/platform/imx-jpeg/mxc-jpeg.c
[...]
> @@ -1448,12 +1317,11 @@ static int mxc_jpeg_parse(struct mxc_jpeg_ctx *ctx,
>* encoded with 3 components have RGB colorspace, see Recommendation
>* ITU-T T.872 chapter 6.5.3 APP14 marker segment for colour encoding
>*/
> - if (img_fmt == MXC_JPEG_YUV444 && app14 && app14_transform == 0)
> - img_fmt = MXC_JPEG_RGB;
> -
> - if (mxc_jpeg_imgfmt_to_fourcc(img_fmt, )) {
> - dev_err(dev, "Fourcc not found for %d", img_fmt);
> - return -EINVAL;
> + if (fourcc == V4L2_PIX_FMT_YUV24 || fourcc == V4L2_PIX_FMT_RGB24) {
> + if (header.app14_tf == 0)

This is what I meant in patch 7, I think it would be more clear to have
an enum value that says "RGB color coding" than to rely on the reader to
know what the value 0 means.

> + fourcc = V4L2_PIX_FMT_RGB24;
> + else
> + fourcc = V4L2_PIX_FMT_YUV24;
>   }
>  
>   /*
> 

Otherwise this patch looks fine to me.

regards
Philipp

Re: [PATCH v5 09/10] media: Avoid parsing quantization and huffman tables

[Philipp Zabel]

On Wed, 2020-12-02 at 13:12 +0100, Hans Verkuil wrote:
> On 12/11/2020 04:05, Mirela Rabulea (OSS) wrote:
> > From: Mirela Rabulea 
> > 
> > These are optional in struct v4l2_jpeg_header, so do not parse if
> > not requested, save some time.
> > 
> > Signed-off-by: Mirela Rabulea 
> > ---
> >  drivers/media/v4l2-core/v4l2-jpeg.c | 6 ++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-jpeg.c 
> > b/drivers/media/v4l2-core/v4l2-jpeg.c
> > index d77e04083d57..7576cd0ce6b9 100644
> > --- a/drivers/media/v4l2-core/v4l2-jpeg.c
> > +++ b/drivers/media/v4l2-core/v4l2-jpeg.c
> > @@ -307,6 +307,9 @@ static int jpeg_parse_quantization_tables(struct 
> > jpeg_stream *stream,
> >  {
> > int len = jpeg_get_word_be(stream);
> >  
> > +   if (!tables)
> > +   return 0;
> > +
> 
> It feels more natural to check for a non-NULL out->quantization_tables
> or non-NULL out->huffman_tables pointer in v4l2_jpeg_parse_header()
> rather than in these low-level functions. It's weird to have this check here.

Ah, now I get it.

Yes, if you want to skip the entire DQT for performance reasons, it is
probably better to just call jpeg_skip_segment() instead of
jpeg_parse_quantization_table(). Otherwise the next jpeg_next_marker has
to scan the whole quantization table for segment markers.

regards
Philipp

Re: [PATCH v5 09/10] media: Avoid parsing quantization and huffman tables

[Philipp Zabel]

Hi Mirela,

On Thu, 2020-11-12 at 05:05 +0200, Mirela Rabulea (OSS) wrote:
> From: Mirela Rabulea 
> 
> These are optional in struct v4l2_jpeg_header, so do not parse if
> not requested, save some time.
> 
> Signed-off-by: Mirela Rabulea 
> ---
>  drivers/media/v4l2-core/v4l2-jpeg.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-jpeg.c 
> b/drivers/media/v4l2-core/v4l2-jpeg.c
> index d77e04083d57..7576cd0ce6b9 100644
> --- a/drivers/media/v4l2-core/v4l2-jpeg.c
> +++ b/drivers/media/v4l2-core/v4l2-jpeg.c
> @@ -307,6 +307,9 @@ static int jpeg_parse_quantization_tables(struct 
> jpeg_stream *stream,
>  {
>   int len = jpeg_get_word_be(stream);
>  
> + if (!tables)
> + return 0;
> +
>   if (len < 0)
>   return len;
>   /* Lq = 2 + n * 65 (for baseline DCT), n >= 1 */
> @@ -361,6 +364,9 @@ static int jpeg_parse_huffman_tables(struct jpeg_stream 
> *stream,
>   int mt;
>   int len = jpeg_get_word_be(stream);
>  
> + if (!tables)
> + return 0;
> +
>   if (len < 0)
>   return len;
>   /* Table B.5 - Huffman table specification parameter sizes and values */

I don't understand this. jpeg_parse_quantization_tables() is only called
if v4l2_jpeg_parse_header() finds a DQT marker. The entire quantization
table-specification parameter block is optional, but when a DQT marker
is present, IIUC the block must contain at least one table (see B.2.4.1,
specifically figure B.6).

regards
Philipp

Re: [PATCH v5 08/10] media: Quit parsing stream if doesn't start with SOI

[Philipp Zabel]

On Thu, 2020-11-12 at 05:05 +0200, Mirela Rabulea (OSS) wrote:
> From: Mirela Rabulea 
> 
> In the case we get an invalid stream, such as from v4l2-compliance
> streaming test, jpeg_next_marker will end up parsing the entire
> stream. The standard describes the high level syntax of a jpeg
> as starting with SOI, ending with EOI, so return error if the very
> first 2 bytes are not SOI.
> 
> Signed-off-by: Mirela Rabulea 
> ---
>  drivers/media/v4l2-core/v4l2-jpeg.c | 7 ++-
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-jpeg.c 
> b/drivers/media/v4l2-core/v4l2-jpeg.c
> index 3181ce544f79..d77e04083d57 100644
> --- a/drivers/media/v4l2-core/v4l2-jpeg.c
> +++ b/drivers/media/v4l2-core/v4l2-jpeg.c
> @@ -499,11 +499,8 @@ int v4l2_jpeg_parse_header(void *buf, size_t len, struct 
> v4l2_jpeg_header *out)
>   out->num_dht = 0;
>   out->num_dqt = 0;
>  
> - /* the first marker must be SOI */
> - marker = jpeg_next_marker();
> - if (marker < 0)
> - return marker;
> - if (marker != SOI)
> + /* the first bytes must be SOI, B.2.1 High-level syntax */
> + if (jpeg_get_word_be() != SOI)
>   return -EINVAL;
>  
>   /* init value to signal if this marker is not present */

Yes, shorter, potentially faster code, and it adheres to the
specification more strictly.

Reviewed-by: Philipp Zabel 

regards
Philipp

Re: [PATCH v5 07/10] media: Add parsing for APP14 data segment in jpeg helpers

[Philipp Zabel]

Hi Mirela,

On Thu, 2020-11-12 at 05:05 +0200, Mirela Rabulea (OSS) wrote:
> From: Mirela Rabulea 
> 
> According to Rec. ITU-T T.872 (06/2012) 6.5.3
> APP14 segment is for color encoding, it contains a transform flag, which
> may have values of 0, 1 and 2 and are interpreted as follows:
> 0 - CMYK for images that are encoded with four components
>   - RGB for images that are encoded with three components
> 1 - An image encoded with three components using YCbCr colour encoding.
> 2 - An image encoded with four components using YCCK colour encoding.
> 
> This is used in imx-jpeg decoder, to distinguish between
> YUV444 and RGB24.
> 
> Signed-off-by: Mirela Rabulea 
> ---
> Changes in v5:
> This was patch 8 in previous version
> Replaced a struct for app14 data with just an int, since the 
> transform flag is the only meaningfull information from this segment

Could we turn this into an enum for the transform flag, and include the
above spec reference in its kerneldoc comment? I think this would be
better than checking for (app14_tf == ) in the drivers.

>  drivers/media/v4l2-core/v4l2-jpeg.c | 39 +++--
>  include/media/v4l2-jpeg.h   |  6 +++--
>  2 files changed, 41 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-jpeg.c 
> b/drivers/media/v4l2-core/v4l2-jpeg.c
> index 8947fd95c6f1..3181ce544f79 100644
> --- a/drivers/media/v4l2-core/v4l2-jpeg.c
> +++ b/drivers/media/v4l2-core/v4l2-jpeg.c
> @@ -45,6 +45,7 @@ MODULE_LICENSE("GPL");
>  #define DHP  0xffde  /* hierarchical progression */
>  #define EXP  0xffdf  /* expand reference */
>  #define APP0 0xffe0  /* application data */
> +#define APP140xffee  /* application data for colour encoding */
>  #define APP150xffef
>  #define JPG0 0xfff0  /* extensions */
>  #define JPG130xfffd
> @@ -444,8 +445,37 @@ static int jpeg_skip_segment(struct jpeg_stream *stream)
>   return jpeg_skip(stream, len - 2);
>  }
>  
> +/* Rec. ITU-T T.872 (06/2012) 6.5.3 */
> +static int jpeg_parse_app14_data(struct jpeg_stream *stream)
> +{
> + int ret;
> + int Lp;

Let's keep variables lower case.

> + int skip;
> + int tf;
> +
> + Lp = jpeg_get_word_be(stream);
> + if (Lp < 0)
> + return Lp;

Should we check that Ap1 to 6 are actually "Adobe\0"?

> + /* get to Ap12 */
> + ret = jpeg_skip(stream, 11);
> + if (ret < 0)
> + return -EINVAL;
> +
> + tf = jpeg_get_byte(stream);
> + if (tf < 0)
> + return tf;
> +
> + skip = Lp - 2 - 11;
> + ret = jpeg_skip(stream, skip);
> + if (ret < 0)
> + return -EINVAL;
> + else
> + return tf;
> +}
> +
>  /**
> - * jpeg_parse_header - locate marker segments and optionally parse headers
> + * v4l2_jpeg_parse_header - locate marker segments and optionally parse 
> headers
>   * @buf: address of the JPEG buffer, should start with a SOI marker
>   * @len: length of the JPEG buffer
>   * @out: returns marker segment positions and optionally parsed headers
> @@ -476,6 +506,9 @@ int v4l2_jpeg_parse_header(void *buf, size_t len, struct 
> v4l2_jpeg_header *out)
>   if (marker != SOI)
>   return -EINVAL;
>  
> + /* init value to signal if this marker is not present */
> + out->app14_tf = -EINVAL;
> +
>   /* loop through marker segments */
>   while ((marker = jpeg_next_marker()) >= 0) {
>   switch (marker) {
> @@ -519,7 +552,9 @@ int v4l2_jpeg_parse_header(void *buf, size_t len, struct 
> v4l2_jpeg_header *out)
>   ret = jpeg_parse_restart_interval(,
>   >restart_interval);
>   break;
> -
> + case APP14:
> + out->app14_tf = jpeg_parse_app14_data();
> + break;
>   case SOS:
>   ret = jpeg_reference_segment(, >sos);
>   if (ret < 0)
> diff --git a/include/media/v4l2-jpeg.h b/include/media/v4l2-jpeg.h
> index ddba2a56c321..008e0476d928 100644
> --- a/include/media/v4l2-jpeg.h
> +++ b/include/media/v4l2-jpeg.h
> @@ -100,10 +100,11 @@ struct v4l2_jpeg_scan_header {
>   *  order, optional
>   * @restart_interval: number of MCU per restart interval, Ri
>   * @ecs_offset: buffer offset in bytes to the entropy coded segment
> + * @app14_tf: transform flag from app14 data
>   *
>   * When this structure is passed to v4l2_jpeg_parse_header, the optional 
> scan,
> - * quantization_tables, and huffman_tables pointers must be initialized to 
> NULL
> - * or point at valid memory.
> + * quantization_tables and huffman_tables pointers must be initialized
> + * to NULL or point at valid memory.

Unnecessary, unrelated change? I'd drop this.

>   */
>  struct v4l2_jpeg_header {
>   struct v4l2_jpeg_reference sof;
> @@ -119,6 +120,7 @@ struct v4l2_jpeg_header {
>   struct v4l2_jpeg_reference *huffman_tables;
>   

[PATCH v2] docs: add a reset controller chapter to the driver API docs

[Philipp Zabel]

Add initial reset controller API documentation. This is mostly intended
to describe the concepts to users of the consumer API, and to tie the
kerneldoc comments we already have into the driver API documentation.

Signed-off-by: Philipp Zabel 
Reviewed-by: Randy Dunlap 
Reviewed-by: Amjad Ouled-Ameur 
---
Changes since v1 [1]:
- Added a note that reset_control_status() does not accept reset control
  array handles (Randy Dunlap)

[1] https://lore.kernel.org/lkml/20201117103306.17010-1-p.za...@pengutronix.de/
---
 Documentation/driver-api/index.rst |   1 +
 Documentation/driver-api/reset.rst | 221 +
 MAINTAINERS|   1 +
 3 files changed, 223 insertions(+)
 create mode 100644 Documentation/driver-api/reset.rst

diff --git a/Documentation/driver-api/index.rst 
b/Documentation/driver-api/index.rst
index f357f3eb400c..08c32952fce3 100644
--- a/Documentation/driver-api/index.rst
+++ b/Documentation/driver-api/index.rst
@@ -29,6 +29,7 @@ available subsections can be seen below.
infiniband
frame-buffer
regulator
+   reset
iio/index
input
usb/index
diff --git a/Documentation/driver-api/reset.rst 
b/Documentation/driver-api/reset.rst
new file mode 100644
index ..84e03d7039cc
--- /dev/null
+++ b/Documentation/driver-api/reset.rst
@@ -0,0 +1,221 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+
+Reset controller API
+
+
+Introduction
+
+
+Reset controllers are central units that control the reset signals to multiple
+peripherals.
+The reset controller API is split into two parts:
+the `consumer driver interface <#consumer-driver-interface>`__ (`API reference
+<#reset-consumer-api>`__), which allows peripheral drivers to request control
+over their reset input signals, and the `reset controller driver interface
+<#reset-controller-driver-interface>`__ (`API reference
+<#reset-controller-driver-api>`__), which is used by drivers for reset
+controller devices to register their reset controls to provide them to the
+consumers.
+
+While some reset controller hardware units also implement system restart
+functionality, restart handlers are out of scope for the reset controller API.
+
+Glossary
+
+
+The reset controller API uses these terms with a specific meaning:
+
+Reset line
+
+Physical reset line carrying a reset signal from a reset controller
+hardware unit to a peripheral module.
+
+Reset control
+
+Control method that determines the state of one or multiple reset lines.
+Most commonly this is a single bit in reset controller register space that
+either allows direct control over the physical state of the reset line, or
+is self-clearing and can be used to trigger a predetermined pulse on the
+reset line.
+In more complicated reset controls, a single trigger action can launch a
+carefully timed sequence of pulses on multiple reset lines.
+
+Reset controller
+
+A hardware module that provides a number of reset controls to control a
+number of reset lines.
+
+Reset consumer
+
+Peripheral module or external IC that is put into reset by the signal on a
+reset line.
+
+Consumer driver interface
+=
+
+This interface provides an API that is similar to the kernel clock framework.
+Consumer drivers use get and put operations to acquire and release reset
+controls.
+Functions are provided to assert and deassert the controlled reset lines,
+trigger reset pulses, or to query reset line status.
+
+When requesting reset controls, consumers can use symbolic names for their
+reset inputs, which are mapped to an actual reset control on an existing reset
+controller device by the core.
+
+A stub version of this API is provided when the reset controller framework is
+not in use in order to minimize the need to use ifdefs.
+
+Shared and exclusive resets
+---
+
+The reset controller API provides either reference counted deassertion and
+assertion or direct, exclusive control.
+The distinction between shared and exclusive reset controls is made at the time
+the reset control is requested, either via devm_reset_control_get_shared() or
+via devm_reset_control_get_exclusive().
+This choice determines the behavior of the API calls made with the reset
+control.
+
+Shared resets behave similarly to clocks in the kernel clock framework.
+They provide reference counted deassertion, where only the first deassert,
+which increments the deassertion reference count to one, and the last assert
+which decrements the deassertion reference count back to zero, have a physical
+effect on the reset line.
+
+Exclusive resets on the other hand guarantee direct control.
+That is, an assert causes the reset line to be asserted immediately, and a
+deassert causes the reset line to be deasserted immediately.
+
+Assertion and deassertion
+-
+
+Consumer drivers use 

Re: [PATCHv2] reset: socfpga: add error handling and release mem-region

[Philipp Zabel]

On Mon, 2020-11-09 at 13:21 -0600, Dinh Nguyen wrote:
> In case of an error, call release_mem_region when an error happens
> during allocation of resources. Also add error handling for the case
> that reset_controller_register fails.
> 
> Reported-by: kernel test robot 
> Reported-by: Dan Carpenter 
> Signed-off-by: Dinh Nguyen 
> ---
> v2: return ret value

Thank you, applied to reset/next.

regards
Philipp

Re: [PATCH] phy: amlogic: replace devm_reset_control_array_get()

[Philipp Zabel]

Hi Yejune,

On Wed, 2020-11-18 at 10:36 +0800, Yejune Deng wrote:
> devm_reset_control_array_get_exclusive() looks more readable
> 
> Signed-off-by: Yejune Deng 
> ---
>  drivers/phy/amlogic/phy-meson-axg-pcie.c   | 2 +-
>  drivers/phy/amlogic/phy-meson-g12a-usb3-pcie.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/phy/amlogic/phy-meson-axg-pcie.c 
> b/drivers/phy/amlogic/phy-meson-axg-pcie.c
> index 377ed0d..3204f02 100644
> --- a/drivers/phy/amlogic/phy-meson-axg-pcie.c
> +++ b/drivers/phy/amlogic/phy-meson-axg-pcie.c
> @@ -155,7 +155,7 @@ static int phy_axg_pcie_probe(struct platform_device 
> *pdev)
>   if (IS_ERR(priv->regmap))
>   return PTR_ERR(priv->regmap);
>  
> - priv->reset = devm_reset_control_array_get(dev, false, false);
> + priv->reset = devm_reset_control_array_get_exclusive(dev);
>   if (IS_ERR(priv->reset))
>   return PTR_ERR(priv->reset);
>  
> diff --git a/drivers/phy/amlogic/phy-meson-g12a-usb3-pcie.c 
> b/drivers/phy/amlogic/phy-meson-g12a-usb3-pcie.c
> index 08e3227..bab6345 100644
> --- a/drivers/phy/amlogic/phy-meson-g12a-usb3-pcie.c
> +++ b/drivers/phy/amlogic/phy-meson-g12a-usb3-pcie.c
> @@ -418,7 +418,7 @@ static int phy_g12a_usb3_pcie_probe(struct 
> platform_device *pdev)
>   if (ret)
>   goto err_disable_clk_ref;
>  
> - priv->reset = devm_reset_control_array_get(dev, false, false);
> + priv->reset = devm_reset_control_array_get_exclusive(dev);
>   if (IS_ERR(priv->reset))
>   return PTR_ERR(priv->reset);
>  

Reviewed-by: Philipp Zabel 

regards
Philipp

Re: [PATCH] soc: amlogic: replace devm_reset_control_array_get()

[Philipp Zabel]

Hi Yejune,

On Wed, 2020-11-18 at 10:48 +0800, Yejune Deng wrote:
> devm_reset_control_array_get_exclusive() looks more readable
> 
> Signed-off-by: Yejune Deng 
> ---
>  drivers/soc/amlogic/meson-ee-pwrc.c | 3 +--
>  drivers/soc/amlogic/meson-gx-pwrc-vpu.c | 2 +-
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/soc/amlogic/meson-ee-pwrc.c 
> b/drivers/soc/amlogic/meson-ee-pwrc.c
> index ed7d2fb..50bf5d2 100644
> --- a/drivers/soc/amlogic/meson-ee-pwrc.c
> +++ b/drivers/soc/amlogic/meson-ee-pwrc.c
> @@ -413,8 +413,7 @@ static int meson_ee_pwrc_init_domain(struct 
> platform_device *pdev,
>   dev_warn(>dev, "Invalid resets count %d for 
> domain %s\n",
>count, dom->desc.name);
>  
> - dom->rstc = devm_reset_control_array_get(>dev, false,
> -  false);
> + dom->rstc = devm_reset_control_array_get_exclusive(>dev);
>   if (IS_ERR(dom->rstc))
>   return PTR_ERR(dom->rstc);
>   }
> diff --git a/drivers/soc/amlogic/meson-gx-pwrc-vpu.c 
> b/drivers/soc/amlogic/meson-gx-pwrc-vpu.c
> index 8790627..b4615b2 100644
> --- a/drivers/soc/amlogic/meson-gx-pwrc-vpu.c
> +++ b/drivers/soc/amlogic/meson-gx-pwrc-vpu.c
> @@ -304,7 +304,7 @@ static int meson_gx_pwrc_vpu_probe(struct platform_device 
> *pdev)
>   return PTR_ERR(regmap_hhi);
>   }
>  
> - rstc = devm_reset_control_array_get(>dev, false, false);
> + rstc = devm_reset_control_array_get_exclusive(>dev);
>   if (IS_ERR(rstc)) {
>   if (PTR_ERR(rstc) != -EPROBE_DEFER)
>   dev_err(>dev, "failed to get reset lines\n");

Reviewed-by: Philipp Zabel 

regards
Philipp

Re: [PATCH v2] drm/imx: depend on COMMON_CLK to fix compile tests

[Philipp Zabel]

Hi Krzysztof,

On Tue, 2020-11-17 at 19:24 +0100, Krzysztof Kozlowski wrote:
> The iMX DRM LVDS driver uses Common Clock Framework thus it cannot be
> built on platforms without it (e.g. compile test on MIPS with RALINK and
> SOC_RT305X):
> 
> /usr/bin/mips-linux-gnu-ld: drivers/gpu/drm/imx/imx-ldb.o: in function 
> `imx_ldb_encoder_disable':
> imx-ldb.c:(.text+0x400): undefined reference to `clk_set_parent'
> 
> Signed-off-by: Krzysztof Kozlowski 
> 
> ---
> 
> Changes since v1:
> 1. Put depends in DRM_IMX_LDB option only.
> ---
>  drivers/gpu/drm/imx/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/imx/Kconfig b/drivers/gpu/drm/imx/Kconfig
> index 6231048aa5aa..73fe2bc5633c 100644
> --- a/drivers/gpu/drm/imx/Kconfig
> +++ b/drivers/gpu/drm/imx/Kconfig
> @@ -28,6 +28,7 @@ config DRM_IMX_TVE
>  config DRM_IMX_LDB
>   tristate "Support for LVDS displays"
>   depends on DRM_IMX && MFD_SYSCON
> + depends on COMMON_CLK
>   select DRM_PANEL
>   help
> Choose this to enable the internal LVDS Display Bridge (LDB)

Thank you, applied to imx-drm/next.

regards
Philipp

Re: [PATCH 41/42] gpu/ipu-v3/ipu-di: Strip out 2 unused 'di_sync_config' entries

[Philipp Zabel]

Hi Lee,

On Mon, 2020-11-16 at 17:41 +, Lee Jones wrote:
> They're taking up too much space on the stack.
> 
> Fixes the following W=1 kernel build warning(s):
> 
>  drivers/gpu/ipu-v3/ipu-di.c: In function ‘ipu_di_sync_config_noninterlaced’:
>  drivers/gpu/ipu-v3/ipu-di.c:391:1: warning: the frame size of 1064 bytes is 
> larger than 1024 bytes [-Wframe-larger-than=]
> 
> Cc: Philipp Zabel 
> Cc: Sascha Hauer 
> Cc: dri-de...@lists.freedesktop.org
> Signed-off-by: Lee Jones 
> ---
>  drivers/gpu/ipu-v3/ipu-di.c | 4 
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/gpu/ipu-v3/ipu-di.c b/drivers/gpu/ipu-v3/ipu-di.c
> index b4a31d506fccf..e617f60afeea3 100644
> --- a/drivers/gpu/ipu-v3/ipu-di.c
> +++ b/drivers/gpu/ipu-v3/ipu-di.c
> @@ -310,10 +310,6 @@ static void ipu_di_sync_config_noninterlaced(struct 
> ipu_di *di,
>   /* unused */
>   } , {
>   /* unused */
> - } , {
> - /* unused */
> - } , {
> - /* unused */
>   },
>   };
>   /* can't use #7 and #8 for line active and pixel active counters */

Thank you, applied to imx-drm/next.

regards
Philipp

Re: [PATCH] drm/imx: depend on COMMON_CLK to fix compile tests

[Philipp Zabel]

On Mon, 2020-11-16 at 19:14 +0100, Krzysztof Kozlowski wrote:
> The iMX DRM drivers use Common Clock Framework thus they cannot be built
> on platforms without it (e.g. compile test on MIPS with RALINK and
> SOC_RT305X):
> 
> /usr/bin/mips-linux-gnu-ld: drivers/gpu/drm/imx/imx-ldb.o: in function 
> `imx_ldb_encoder_disable':
> imx-ldb.c:(.text+0x400): undefined reference to `clk_set_parent'
> 
> Signed-off-by: Krzysztof Kozlowski 

Thank you, but could this be added to

config DRM_IMX_LDB

instead?

The core DRM_IMX code does not use the Common Clock Framework directly.
DRM_IMX_TVE already depends on COMMON_CLK because it implements a clock.

regards
Philipp

[PATCH] docs: add a reset controller chapter to the driver API docs

[Philipp Zabel]

Add initial reset controller API documentation. This is mostly intended
to describe the concepts to users of the consumer API, and to tie the
kerneldoc comments we already have into the driver API documentation.

Signed-off-by: Philipp Zabel 
---
Changes since the RFC [1]:
- Replaced all :c:func:`function` with function() (Jonathan Corbet)
- Typo fixes and wording improvements (Randy Dunlap)
- Mention new reset_control_rearm() API as a counterpart to
  reset_control_reset() for shared resets in the Triggering section
- Grammar fix in the Querying section
- Add reset.rst to MAINTAINERS

[1] https://lore.kernel.org/lkml/20191022164547.22632-1-p.za...@pengutronix.de/
---
 Documentation/driver-api/index.rst |   1 +
 Documentation/driver-api/reset.rst | 219 +
 MAINTAINERS|   1 +
 3 files changed, 221 insertions(+)
 create mode 100644 Documentation/driver-api/reset.rst

diff --git a/Documentation/driver-api/index.rst 
b/Documentation/driver-api/index.rst
index f357f3eb400c..08c32952fce3 100644
--- a/Documentation/driver-api/index.rst
+++ b/Documentation/driver-api/index.rst
@@ -29,6 +29,7 @@ available subsections can be seen below.
infiniband
frame-buffer
regulator
+   reset
iio/index
input
usb/index
diff --git a/Documentation/driver-api/reset.rst 
b/Documentation/driver-api/reset.rst
new file mode 100644
index ..00a6ad79dd6c
--- /dev/null
+++ b/Documentation/driver-api/reset.rst
@@ -0,0 +1,219 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+
+Reset controller API
+
+
+Introduction
+
+
+Reset controllers are central units that control the reset signals to multiple
+peripherals.
+The reset controller API is split into two parts:
+the `consumer driver interface <#consumer-driver-interface>`__ (`API reference
+<#reset-consumer-api>`__), which allows peripheral drivers to request control
+over their reset input signals, and the `reset controller driver interface
+<#reset-controller-driver-interface>`__ (`API reference
+<#reset-controller-driver-api>`__), which is used by drivers for reset
+controller devices to register their reset controls to provide them to the
+consumers.
+
+While some reset controller hardware units also implement system restart
+functionality, restart handlers are out of scope for the reset controller API.
+
+Glossary
+
+
+The reset controller API uses these terms with a specific meaning:
+
+Reset line
+
+Physical reset line carrying a reset signal from a reset controller
+hardware unit to a peripheral module.
+
+Reset control
+
+Control method that determines the state of one or multiple reset lines.
+Most commonly this is a single bit in reset controller register space that
+either allows direct control over the physical state of the reset line, or
+is self-clearing and can be used to trigger a predetermined pulse on the
+reset line.
+In more complicated reset controls, a single trigger action can launch a
+carefully timed sequence of pulses on multiple reset lines.
+
+Reset controller
+
+A hardware module that provides a number of reset controls to control a
+number of reset lines.
+
+Reset consumer
+
+Peripheral module or external IC that is put into reset by the signal on a
+reset line.
+
+Consumer driver interface
+=
+
+This interface provides an API that is similar to the kernel clock framework.
+Consumer drivers use get and put operations to acquire and release reset
+controls.
+Functions are provided to assert and deassert the controlled reset lines,
+trigger reset pulses, or to query reset line status.
+
+When requesting reset controls, consumers can use symbolic names for their
+reset inputs, which are mapped to an actual reset control on an existing reset
+controller device by the core.
+
+A stub version of this API is provided when the reset controller framework is
+not in use in order to minimize the need to use ifdefs.
+
+Shared and exclusive resets
+---
+
+The reset controller API provides either reference counted deassertion and
+assertion or direct, exclusive control.
+The distinction between shared and exclusive reset controls is made at the time
+the reset control is requested, either via devm_reset_control_get_shared() or
+via devm_reset_control_get_exclusive().
+This choice determines the behavior of the API calls made with the reset
+control.
+
+Shared resets behave similarly to clocks in the kernel clock framework.
+They provide reference counted deassertion, where only the first deassert,
+which increments the deassertion reference count to one, and the last assert
+which decrements the deassertion reference count back to zero, have a physical
+effect on the reset line.
+
+Exclusive resets on the other hand guarantee direct control.
+That is, an assert causes the reset line to be asserted immediately

Re: [PATCH v2] reset: make shared pulsed reset controls re-triggerable

[Philipp Zabel]

On Fri, 2020-11-13 at 16:13 +0100, Jerome Brunet wrote:
> On Fri 13 Nov 2020 at 16:04, Philipp Zabel  wrote:
> 
> > On Fri, 2020-11-13 at 00:00 +0100, Amjad Ouled-Ameur wrote:
> > > The current reset framework API does not allow to release what is done by
> > > reset_control_reset(), IOW decrement triggered_count. Add the new
> > > reset_control_rearm() call to do so.
> > > 
> > > When reset_control_reset() has been called once, the counter
> > > triggered_count, in the reset framework, is incremented i.e the resource
> > > under the reset is in-use and the reset should not be done again.
> > > reset_control_rearm() would be the way to state that the resource is
> > > no longer used and, that from the caller's perspective, the reset can be
> > > fired again if necessary.
> > > 
> > > Signed-off-by: Amjad Ouled-Ameur 
> > > Reported-by: Jerome Brunet 
> > > ---
> > > Change since v1: [0]
> > > * Renamed the new call from reset_control_(array_)resettable to 
> > > reset_control_(array_)rearm
> > > * Open-coded reset_control_array_rearm to check for errors before
> > > decrementing triggered_count because we cannot roll back in case an
> > > error occurs while decrementing one of the rstc.
> > > * Reworded the new call's description.
> > 
> > Thank you, applied to reset/next.
> 
> Hi Philipp,
> 
> Would it be possible to get an immutable branch/tag with this ?
> It would allow to move forward on the USB side, without waiting for the
> next rc1.

Here you go,

The following changes since commit 3650b228f83adda7e5ee532e2b90429c03f7b9ec:

  Linux 5.10-rc1 (2020-10-25 15:14:11 -0700)

are available in the Git repository at:

  git://git.pengutronix.de/git/pza/linux.git reset/shared-retrigger

for you to fetch changes up to 557acb3d2cd9c82de19f944f6cc967a347735385:

  reset: make shared pulsed reset controls re-triggerable (2020-11-16 17:05:28 
+0100)


Amjad Ouled-Ameur (1):
  reset: make shared pulsed reset controls re-triggerable

 drivers/reset/core.c  | 73 +++
 include/linux/reset.h |  1 +
 2 files changed, 74 insertions(+)

regards
Philipp

Re: [PATCH v2] spi: cadence-quadspi: Fix error return code in cqspi_probe

[Philipp Zabel]

On Mon, 2020-11-16 at 22:18 +0800, Zhihao Cheng wrote:
> Fix to return the error code from
> devm_reset_control_get_optional_exclusive() instaed of 0
> in cqspi_probe().
> 
> Fixes: 31fb632b5d43ca ("spi: Move cadence-quadspi driver to drivers/spi/")
> Reported-by: Hulk Robot 
> Signed-off-by: Zhihao Cheng 

Thank you,

Reviewed-by: Philipp Zabel 

regards
Philipp

Re: [PATCH] spi: cadence-quadspi: Fix error return code in cqspi_probe

[Philipp Zabel]

Hi Zhihao,

On Mon, 2020-11-16 at 22:10 +0800, Zhihao Cheng wrote:
> Fix to return the error code from
> devm_reset_control_get_optional_exclusive() instaed of 0
> in cqspi_probe().
>
> Fixes: 31fb632b5d43ca ("spi: Move cadence-quadspi driver to drivers/spi/")
> Reported-by: Hulk Robot 
> Signed-off-by: Zhihao Cheng 
> ---
>  drivers/spi/spi-cadence-quadspi.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/spi/spi-cadence-quadspi.c 
> b/drivers/spi/spi-cadence-quadspi.c
> index 40938cf3806d..22d7158edb71 100644
> --- a/drivers/spi/spi-cadence-quadspi.c
> +++ b/drivers/spi/spi-cadence-quadspi.c
> @@ -1260,12 +1260,14 @@ static int cqspi_probe(struct platform_device *pdev)
>   /* Obtain QSPI reset control */
>   rstc = devm_reset_control_get_optional_exclusive(dev, "qspi");
>   if (IS_ERR(rstc)) {
> + ret = PTR_ERR(rstc);
>   dev_err(dev, "Cannot get QSPI reset.\n");
>   goto probe_reset_failed;
>   }
>  
>   rstc_ocp = devm_reset_control_get_optional_exclusive(dev, "qspi-ocp");
>   if (IS_ERR(rstc_ocp)) {
> + ret = PTR_ERR(rstc);

This should be
ret = PTR_ERR(rstc_ocp);
instead.

regards
Philipp

Re: [PATCH v2] reset: make shared pulsed reset controls re-triggerable

[Philipp Zabel]

On Fri, 2020-11-13 at 00:00 +0100, Amjad Ouled-Ameur wrote:
> The current reset framework API does not allow to release what is done by
> reset_control_reset(), IOW decrement triggered_count. Add the new
> reset_control_rearm() call to do so.
> 
> When reset_control_reset() has been called once, the counter
> triggered_count, in the reset framework, is incremented i.e the resource
> under the reset is in-use and the reset should not be done again.
> reset_control_rearm() would be the way to state that the resource is
> no longer used and, that from the caller's perspective, the reset can be
> fired again if necessary.
> 
> Signed-off-by: Amjad Ouled-Ameur 
> Reported-by: Jerome Brunet 
> ---
> Change since v1: [0]
> * Renamed the new call from reset_control_(array_)resettable to 
> reset_control_(array_)rearm
> * Open-coded reset_control_array_rearm to check for errors before
> decrementing triggered_count because we cannot roll back in case an
> error occurs while decrementing one of the rstc.
> * Reworded the new call's description.

Thank you, applied to reset/next.

regards
Philipp

Re: [PATCH v2 1/2] reset: brcmstb rescal: implement {de}assert() instead of reset()

[Philipp Zabel]

On Mon, 2020-11-09 at 11:21 -0500, Jim Quinlan wrote:
> On Mon, Nov 9, 2020 at 5:05 AM Philipp Zabel  wrote:
> > Hi Jim,
> > 
> > On Fri, 2020-11-06 at 14:17 -0500, Jim Quinlan wrote:
> > > Before, only control_reset() was implemented.  However, the reset core 
> > > only
> > > invokes control_reset() once in its lifetime.  Because we need it to 
> > > invoke
> > > control_reset() again after resume out of S2 or S3, we have switched to
> > > putting the reset functionality into the control_deassert() method and
> > > having an empty control_assert() method.
> > > 
> > > Signed-off-by: Jim Quinlan 
> > 
> > You are switching to the wrong abstraction to work around a deficiency
> > of the reset controller framework. Instead, it would be better to allow
> > to "reactivate" shared pulsed resets so they can be triggered again.
> 
> True.
> > 
> > Could you please have a look at [1], which tries to implement this with
> > a new API call, and check if this can fix your problem? If so, it would
> > be great if you could coordinate with Amjad to see this fixed in the
> > core.
> > 
> > [1] 
> > https://lore.kernel.org/lkml/20201001132758.12280-1-aouledam...@baylibre.com/
> 
> Yes, this would work for our usage.  Amjad please let me know if I can
> help here.  The only "nit" I have is that I favor the name 'unreset'
> over 'resettable' but truly I don't care one way or the other.

Both unreset and resettable are adjectives, maybe it would be better to
have an imperative verb like the other API functions. I would have liked
reset_control_trigger/rearm as a pair, but I can't find anything I like
that fits with the somewhat unfortunate reset_control_reset name in my
mind.
In that sense, I don't have a preference one way or the other either.

regards
Philipp

Re: [PATCH] staging: media: imx: Split config option in 2

[Philipp Zabel]

On Mon, 2020-11-09 at 10:46 +0100, Martin Kepplinger wrote:
> As described in NXPs' linux tree, the imx8m SoC includes the same
> CSI bridge hardware that is part of imx7d. We should be able to
> use the "fsl,imx7-csi" driver for imx8m directly.
> 
> Since ipuv3 is not relevant for imx8m we create VIDEO_IMX7_MEDIA and
> split up the configuration option in 2 menus (on 1 entry each
> for now but that can be changed later).
> 
> Signed-off-by: Martin Kepplinger 
> ---
> 
> thanks, you're right. did you have something like this in mind?

Not quite, we need a separate option for the imx-media-common module, so
the Makefile has to be changed as well. That option should be selected
by VIDEO_IMX_MEDIA.
I'm not sure if introducing VIDEO_IMX7_MEDIA is necessary, the new
option could also be hidden if selected VIDEO_IMX7_CSI directly.

regards
Philipp

Re: [PATCH] reset: socfpga: add error handling and release mem-region

[Philipp Zabel]

On Mon, 2020-11-02 at 13:57 -0600, Dinh Nguyen wrote:
> In case of an error, call release_mem_region when an error happens
> during allocation of resources. Also add error handling for the case
> that reset_controller_register fails.
> 
> Reported-by: kernel test robot 
> Reported-by: Dan Carpenter 
> Signed-off-by: Dinh Nguyen 
> ---
>  drivers/reset/reset-socfpga.c | 11 +--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/reset/reset-socfpga.c b/drivers/reset/reset-socfpga.c
> index bdd984296196..af9041ec37c6 100644
> --- a/drivers/reset/reset-socfpga.c
> +++ b/drivers/reset/reset-socfpga.c
> @@ -44,7 +44,7 @@ static int a10_reset_init(struct device_node *np)
>   data->membase = ioremap(res.start, size);
>   if (!data->membase) {
>   ret = -ENOMEM;
> - goto err_alloc;
> + goto release_region;
>   }
>  
>   if (of_property_read_u32(np, "altr,modrst-offset", _offset))
> @@ -59,7 +59,14 @@ static int a10_reset_init(struct device_node *np)
>   data->rcdev.of_node = np;
>   data->status_active_low = true;
>  
> - return reset_controller_register(>rcdev);
> + ret = reset_controller_register(>rcdev);
> + if (ret)
> + pr_err("unable to register device\n");
> +
> + return 0;

Please don't return 0 if reset_controller_register() failed.

regards
Philipp

Re: [PATCH v2 1/2] reset: brcmstb rescal: implement {de}assert() instead of reset()

[Philipp Zabel]

Hi Jim,

On Fri, 2020-11-06 at 14:17 -0500, Jim Quinlan wrote:
> Before, only control_reset() was implemented.  However, the reset core only
> invokes control_reset() once in its lifetime.  Because we need it to invoke
> control_reset() again after resume out of S2 or S3, we have switched to
> putting the reset functionality into the control_deassert() method and
> having an empty control_assert() method.
> 
> Signed-off-by: Jim Quinlan 

You are switching to the wrong abstraction to work around a deficiency
of the reset controller framework. Instead, it would be better to allow
to "reactivate" shared pulsed resets so they can be triggered again.

Could you please have a look at [1], which tries to implement this with
a new API call, and check if this can fix your problem? If so, it would
be great if you could coordinate with Amjad to see this fixed in the
core.

[1] 
https://lore.kernel.org/lkml/20201001132758.12280-1-aouledam...@baylibre.com/

regards
Philipp

> ---
>  drivers/reset/reset-brcmstb-rescal.c | 13 ++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/reset/reset-brcmstb-rescal.c 
> b/drivers/reset/reset-brcmstb-rescal.c
> index b6f074d6a65f..1f54ae4f91fe 100644
> --- a/drivers/reset/reset-brcmstb-rescal.c
> +++ b/drivers/reset/reset-brcmstb-rescal.c
> @@ -20,8 +20,8 @@ struct brcm_rescal_reset {
>   struct reset_controller_dev rcdev;
>  };
>  
> -static int brcm_rescal_reset_set(struct reset_controller_dev *rcdev,
> -  unsigned long id)
> +static int brcm_rescal_reset_deassert(struct reset_controller_dev *rcdev,
> + unsigned long id)
>  {
>   struct brcm_rescal_reset *data =
>   container_of(rcdev, struct brcm_rescal_reset, rcdev);
> @@ -52,6 +52,12 @@ static int brcm_rescal_reset_set(struct 
> reset_controller_dev *rcdev,
>   return 0;
>  }
>  
> +static int brcm_rescal_reset_assert(struct reset_controller_dev *rcdev,
> +   unsigned long id)
> +{
> + return 0;
> +}
> +
>  static int brcm_rescal_reset_xlate(struct reset_controller_dev *rcdev,
>  const struct of_phandle_args *reset_spec)
>  {
> @@ -60,7 +66,8 @@ static int brcm_rescal_reset_xlate(struct 
> reset_controller_dev *rcdev,
>  }
>  
>  static const struct reset_control_ops brcm_rescal_reset_ops = {
> - .reset = brcm_rescal_reset_set,
> + .deassert = brcm_rescal_reset_deassert,
> + .assert = brcm_rescal_reset_assert,
>  };
>  
>  static int brcm_rescal_reset_probe(struct platform_device *pdev)

Re: [PATCH] staging: media: imx: drop dependency on ipuv3

[Philipp Zabel]

Hi Martin,

On Mon, 2020-11-09 at 10:13 +0100, Martin Kepplinger wrote:
> As described in NXPs' linux tree, the imx8m SoC includes the same
> CSI bridge hardware that is part of imx7d. We should be able to
> use the "fsl,imx7-csi" driver for imx8m directly.
> 
> Since ipuv3 is not relevant for imx8m, drop the build dependency
> for it.
> 
> Signed-off-by: Martin Kepplinger 
> ---
>  drivers/staging/media/imx/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/imx/Kconfig 
> b/drivers/staging/media/imx/Kconfig
> index f555aac8a9d5..98272fd92fe4 100644
> --- a/drivers/staging/media/imx/Kconfig
> +++ b/drivers/staging/media/imx/Kconfig
> @@ -2,7 +2,7 @@
>  config VIDEO_IMX_MEDIA
>   tristate "i.MX5/6 V4L2 media core driver"

VIDEO_IMX_MEDIA builds imx6-media, which does depend on IMX_IPUV3_CORE.
You only want imx-media-common. I think we have to split the
configuration option in two.

>   depends on ARCH_MXC || COMPILE_TEST
> - depends on VIDEO_V4L2 && IMX_IPUV3_CORE
> + depends on VIDEO_V4L2
>   select MEDIA_CONTROLLER
>   select VIDEO_V4L2_SUBDEV_API
>   depends on HAS_DMA

regards
Philipp

Re: [PATCH 6/9] phy: cadence: sierra: Don't configure if any plls are already locked

[Philipp Zabel]

On Tue, 2020-11-03 at 09:25 +0530, Kishon Vijay Abraham I wrote:
> From: Faiz Abbas 
> 
> Serdes lanes might be shared between multiple cores in some usecases
> and its not possible to lock PLLs for both the lanes independently
> by the two cores. This requires a bootloader to configure both the
> lanes at early boot time.
> 
> To handle this case, skip all configuration if any of the plls are
> already locked. This is done by adding an already_configured flag
> and using it to gate every register access as well as any phy_ops.
> 
> Signed-off-by: Faiz Abbas 
> Signed-off-by: Kishon Vijay Abraham I 
> ---
>  drivers/phy/cadence/phy-cadence-sierra.c | 127 ++-
>  1 file changed, 78 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/phy/cadence/phy-cadence-sierra.c 
> b/drivers/phy/cadence/phy-cadence-sierra.c
> index e08548417bce..145e42837b7b 100644
> --- a/drivers/phy/cadence/phy-cadence-sierra.c
> +++ b/drivers/phy/cadence/phy-cadence-sierra.c
> @@ -364,6 +364,10 @@ static const struct phy_ops ops = {
>   .owner  = THIS_MODULE,
>  };
>  
> +static const struct phy_ops noop_ops = {
> + .owner  = THIS_MODULE,
> +};
> +
>  static int cdns_sierra_get_optional(struct cdns_sierra_inst *inst,
>   struct device_node *child)
>  {
> @@ -477,6 +481,49 @@ static int cdns_regmap_init_blocks(struct 
> cdns_sierra_phy *sp,
>   return 0;
>  }
>  
> +static int cdns_sierra_phy_get_clocks(struct cdns_sierra_phy *sp,
> +   struct device *dev)
> +{
> + struct clk *clk;
> + int ret;
> +
> + sp->clk = devm_clk_get_optional(dev, "phy_clk");
> + if (IS_ERR(sp->clk)) {
> + dev_err(dev, "failed to get clock phy_clk\n");
> + return PTR_ERR(sp->clk);
> + }
> +
> + sp->phy_rst = devm_reset_control_get(dev, "sierra_reset");

While you're at it, please use devm_reset_control_get_exclusive() here
and ...

> + if (IS_ERR(sp->phy_rst)) {
> + dev_err(dev, "failed to get reset\n");
> + return PTR_ERR(sp->phy_rst);
> + }
> +
> + sp->apb_rst = devm_reset_control_get_optional(dev, "sierra_apb");

... devm_reset_control_get_optional_exclusive() here.

regards
Philipp

Re: [PATCH 2/4] drm/imx: tve remove extraneous type qualifier

[Philipp Zabel]

On Mon, 2020-10-26 at 20:41 +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann 
> 
> clang warns about functions returning a 'const int' result:
> 
> drivers/gpu/drm/imx/imx-tve.c:487:8: warning: type qualifiers ignored on 
> function return type [-Wignored-qualifiers]
> 
> Remove the extraneous 'const' qualifier here. I would guess that the
> function was intended to be marked __attribute__((const)) instead,
> but that would also be wrong since it call other functions without
> that attribute.
> 
> Fixes: fcbc51e54d2a ("staging: drm/imx: Add support for Television Encoder 
> (TVEv2)")
> Signed-off-by: Arnd Bergmann 

Thank you, applied to imx-drm/next with Nick's R-b.

regards
Philipp

Re: [PATCH 9/9] remoteproc/wkup_m3: Use reset control driver if available

[Philipp Zabel]

Hi Tony,

On Mon, 2020-10-26 at 13:10 +0200, Tony Lindgren wrote:
> In order to move wkup_m3 to probe without platform data, let's add
> support for using optional reset control driver if configured in the
> dts. With this change and the related dts change, we can start
> dropping the platform data for am335x.
> 
> And once wkup_m3 no longer needs platform data, we can simply drop the
> related legacy reset platform data callbacks from wkup_m3 driver later
> on after also am437x no longer depends on it.
> 
> Cc: linux-remotep...@vger.kernel.org
> Cc: Bjorn Andersson 
> Cc: Dave Gerlach 
> Cc: Philipp Zabel 
> Cc: Suman Anna 
> Signed-off-by: Tony Lindgren 
> ---
> 
> Please review and ack if no issues. If you guys instead want to set up an
> immutable remoteproc branch with just this patch in it against v5.10-rc1
> that works too :)
> 
> ---
>  drivers/remoteproc/wkup_m3_rproc.c | 28 +---
>  1 file changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/remoteproc/wkup_m3_rproc.c 
> b/drivers/remoteproc/wkup_m3_rproc.c
> --- a/drivers/remoteproc/wkup_m3_rproc.c
> +++ b/drivers/remoteproc/wkup_m3_rproc.c
> @@ -17,6 +17,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  
> @@ -43,11 +44,13 @@ struct wkup_m3_mem {
>   * @rproc: rproc handle
>   * @pdev: pointer to platform device
>   * @mem: WkupM3 memory information
> + * @rsts: reset control
>   */
>  struct wkup_m3_rproc {
>   struct rproc *rproc;
>   struct platform_device *pdev;
>   struct wkup_m3_mem mem[WKUPM3_MEM_MAX];
> + struct reset_control *rsts;
>  };
>  
>  static int wkup_m3_rproc_start(struct rproc *rproc)
> @@ -57,6 +60,9 @@ static int wkup_m3_rproc_start(struct rproc *rproc)
>   struct device *dev = >dev;
>   struct wkup_m3_platform_data *pdata = dev_get_platdata(dev);
>  
> + if (wkupm3->rsts)

No need for this check, reset_control_deassert() just returns 0 if the
rstc parameter is NULL.

> + return reset_control_deassert(wkupm3->rsts);
> +
>   if (pdata->deassert_reset(pdev, pdata->reset_name)) {
>   dev_err(dev, "Unable to reset wkup_m3!\n");
>   return -ENODEV;
> @@ -72,6 +78,9 @@ static int wkup_m3_rproc_stop(struct rproc *rproc)
>   struct device *dev = >dev;
>   struct wkup_m3_platform_data *pdata = dev_get_platdata(dev);
>  
> + if (wkupm3->rsts)

Same as above.

> + return reset_control_assert(wkupm3->rsts);
> +
>   if (pdata->assert_reset(pdev, pdata->reset_name)) {
>   dev_err(dev, "Unable to assert reset of wkup_m3!\n");
>   return -ENODEV;
> @@ -132,12 +141,6 @@ static int wkup_m3_rproc_probe(struct platform_device 
> *pdev)
>   int ret;
>   int i;
>  
> - if (!(pdata && pdata->deassert_reset && pdata->assert_reset &&
> -   pdata->reset_name)) {
> - dev_err(dev, "Platform data missing!\n");
> - return -ENODEV;
> - }
> -
>   ret = of_property_read_string(dev->of_node, "ti,pm-firmware",
> _name);
>   if (ret) {
> @@ -165,6 +168,17 @@ static int wkup_m3_rproc_probe(struct platform_device 
> *pdev)
>   wkupm3->rproc = rproc;
>   wkupm3->pdev = pdev;
>  
> + wkupm3->rsts = devm_reset_control_get_optional_shared(dev, "rstctrl");
> + if (PTR_ERR_OR_ZERO(wkupm3->rsts)) {

Please properly return errors. rsts will be NULL if the optional rstctrl
reset is not specified:

if (IS_ERR(wkump3->rsts))
return PTR_ERR(wkump3->rsts);

if (!wkump3->rsts) {
> + if (!(pdata && pdata->deassert_reset && pdata->assert_reset &&
> +   pdata->reset_name)) {
> + dev_err(dev, "Platform data missing!\n");
> + ret = -ENODEV;
> + goto err_put_rproc;
> + }
> + wkupm3->rsts = NULL;

I assume this will later be dropped with the platform data support?

> + }
> +
>   for (i = 0; i < ARRAY_SIZE(mem_names); i++) {
>   res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>  mem_names[i]);
> @@ -173,7 +187,7 @@ static int wkup_m3_rproc_probe(struct platform_device 
> *pdev)
>   dev_err(>dev, "devm_ioremap_resource failed for 
> resource %d\n",
>   i);
>   ret = PTR_ERR(wkupm3->mem[i].cpu_addr);
> - goto err;
> + goto err_put_rproc;
>   }
>   wkupm3->mem[i].bus_addr = res->start;
>   wkupm3->mem[i].size = resource_size(res);

regards
Philipp

Re: [PATCH v2] reset: meson: make it possible to build as a module

[Philipp Zabel]

Hi Neil,

thank you for the patch.

On Mon, 2020-10-19 at 16:48 +0200, Neil Armstrong wrote:
> In order to reduce the kernel Image size on multi-platform distributions,
> make it possible to build the reset controller driver as a module.
> 
> This partially reverts 8290924e ("reset: meson: make it explicitly 
> non-modular")

I've changed this to:

This partially reverts commit 8290924e6878 ("reset: meson: make it
explicitly non-modular").

> Signed-off-by: Neil Armstrong 
> Reviewed-by: Kevin Hilman 
> Tested-by: Kevin Hilman 

Applied to reset/next with Martin's R-b.

regards
Philipp

Re: [PATCH v4 08/15] Documentation: of: Convert graph bindings to json-schema

[Philipp Zabel]

Hi Sameer, Rob,

On Mon, 2020-10-19 at 16:56 -0500, Rob Herring wrote:
> On Fri, Oct 16, 2020 at 08:12:55PM +0530, Sameer Pujar wrote:
> > Convert device tree bindings of graph to YAML format.
> 
> Thanks for doing this.

Seconded.

> > Signed-off-by: Sameer Pujar 
> > Cc: Philipp Zabel 
> > ---
> >  Documentation/devicetree/bindings/graph.txt  | 128 

The removed Documentation/devicetree/bindings/graph.txt is referenced by
a lot of files, tree-wide. Should the references be updated in the same
series?

> >  Documentation/devicetree/bindings/graph.yaml | 170 
> > +++
> >  2 files changed, 170 insertions(+), 128 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/graph.txt
> >  create mode 100644 Documentation/devicetree/bindings/graph.yaml
> 
> I'd like to move this to the dtschema repository instead.
> 
> > diff --git a/Documentation/devicetree/bindings/graph.yaml 
> > b/Documentation/devicetree/bindings/graph.yaml
> > new file mode 100644
> > index 000..67804c1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/graph.yaml
> > @@ -0,0 +1,170 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> 
> As the original text defaulted to GPL2, this needs Philipp's permission 
> to re-license.

Acked-by: Philipp Zabel 

> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/graph.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Common bindings for device graphs
> > +
> > +description: |
> > +  The hierarchical organisation of the device tree is well suited to 
> > describe
> > +  control flow to devices, but there can be more complex connections 
> > between
> > +  devices that work together to form a logical compound device, following 
> > an
> > +  arbitrarily complex graph.
> > +  There already is a simple directed graph between devices tree nodes using
> > +  phandle properties pointing to other nodes to describe connections that
> > +  can not be inferred from device tree parent-child relationships. The 
> > device
> > +  tree graph bindings described herein abstract more complex devices that 
> > can
> > +  have multiple specifiable ports, each of which can be linked to one or 
> > more
> > +  ports of other devices.
> > +
> > +  These common bindings do not contain any information about the direction 
> > or
> > +  type of the connections, they just map their existence. Specific 
> > properties
> > +  may be described by specialized bindings depending on the type of 
> > connection.
> > +
> > +  To see how this binding applies to video pipelines, for example, see
> > +  Documentation/devicetree/bindings/media/video-interfaces.txt.
> > +  Here the ports describe data interfaces, and the links between them are
> > +  the connecting data buses. A single port with multiple connections can
> > +  correspond to multiple devices being connected to the same physical bus.
> > +
> > +maintainers:
> > +  - Philipp Zabel 
> > +
> > +definitions:
> > +
> > +  port:
> > +type: object
> > +description: |
> > +  If there is more than one 'port' or more than one 'endpoint' node
> > +  or 'reg' property present in the port and/or endpoint nodes then
> > +  '#address-cells' and '#size-cells' properties are required in 
> > relevant
> > +  parent node.
> 
> reg property.

What about #address-cells and #size-cells in port and ports nodes?
These must either be #address-cells = <1>, #size-cells = <0>, or they
can be absent if the parent node already has the same, or if a port node
only contains a single endpoint.

> > +
> > +patternProperties:
> > +  "^endpoint(@[0-9a-f]+)?$":
> > +type: object
> > +properties:
> 
> reg?
> 
> > +  remote-endpoint:
> > +description: |
> > +  phandle to an 'endpoint' subnode of a remote device node.
> > +$ref: /schemas/types.yaml#/definitions/phandle
> > +
> > +  ports:
> > +type: object
> > +patternProperties:
> > +  "^port(@[0-9a-f]+)?$":
> > +$ref: "#/definitions/port"
> 
> No reason for this to be under 'definitions'. Just move down.
> 
> > +
> > +properties:
> > +  ports:
> > +$ref: "#/definitions/ports"
> > +
> > +patternProperties:
> > +  "^port(@[0-9a-f]+)?$":
> > +$ref: &quo

Re: [PATCH 04/18] media: hantro: add reset controller support

[Philipp Zabel]

Hi Adrian,

On Mon, 2020-10-12 at 23:59 +0300, Adrian Ratiu wrote:
> Some SoCs might have a reset controller which disables clocks
> by default in reset state which then drivers need to unreset
> before being able to ungate a specific clock.
> 
> In this specific case, the hantro driver needs to ensure the
> peripheral clock can be properly ungated otherwise MMIO reg
> values can't be accessed.
> 
> If the SoC has no reset controller or there is no "resets" DT
> property defined, this new code will have no effect.
> 
> Signed-off-by: Adrian Ratiu 
> Signed-off-by: Ezequiel Garcia 
> ---
>  drivers/staging/media/hantro/hantro.h | 1 +
>  drivers/staging/media/hantro/hantro_drv.c | 8 
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/staging/media/hantro/hantro.h 
> b/drivers/staging/media/hantro/hantro.h
> index 65f9f7ea7dcf..bb442eb1974e 100644
> --- a/drivers/staging/media/hantro/hantro.h
> +++ b/drivers/staging/media/hantro/hantro.h
> @@ -183,6 +183,7 @@ struct hantro_dev {
>   struct platform_device *pdev;
>   struct device *dev;
>   struct clk_bulk_data *clocks;
> + struct reset_control *reset;
>   void __iomem **reg_bases;
>   void __iomem *enc_base;
>   void __iomem *dec_base;
> diff --git a/drivers/staging/media/hantro/hantro_drv.c 
> b/drivers/staging/media/hantro/hantro_drv.c
> index 3cd00cc0a364..c2ea54552ce9 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -17,6 +17,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -747,6 +748,13 @@ static int hantro_probe(struct platform_device *pdev)
>  
>   INIT_DELAYED_WORK(>watchdog_work, hantro_watchdog);
>  
> + vpu->reset = devm_reset_control_get_optional_exclusive(>dev,
> +NULL);
> + if (IS_ERR(vpu->reset))
> + vpu->reset = NULL;

Please return the error. If the optional reset is missing from the
device tree, devm_reset_control_get_optional_exclusive() returns NULL
already.

regards
Philipp

Re: [PATCH] reset: Add reset controller API

[Philipp Zabel]

Hi Amjad,

Thank you for the patch, comments below:

On Thu, 2020-10-01 at 15:55 +0200, Amjad Ouled-Ameur wrote:
> An update on the patch title, since we don't add an API but extend it,
> The title should rather be: Add a new call to the reset framework

I think it should even say what functionality is added, for example

"reset: make shared pulsed reset controls re-triggerable"

> Le jeu. 1 oct. 2020 à 15:28, Amjad Ouled-Ameur
>  a écrit :
> > The current reset framework API does not allow to release what is done by
> > reset_control_reset(), IOW decrement triggered_count. Add the new
> > reset_control_resettable() call to do so.
> > 
> > When reset_control_reset() has been called once, the counter
> > triggered_count, in the reset framework, is incremented i.e the resource
> > under the reset is in-use and the reset should not be done again.
> > reset_control_resettable() would be the way to state that the resource is
> > no longer used and, that from the caller's perspective, the reset can be
> > fired again if necessary.
> > 
> > This patch will fix a usb suspend warning seen on the libretech-cc
> > related to the shared reset line. This warning was addressed by the
> > previously reverted commit 7a410953d1fb ("usb: dwc3: meson-g12a: fix shared
> > reset control use")
> > 
> > Signed-off-by: Amjad Ouled-Ameur 
> > Reported-by: Jerome Brunet 
> > ---
> >  drivers/reset/core.c  | 57 +++
> >  include/linux/reset.h |  1 +
> >  2 files changed, 58 insertions(+)
> > 
> > diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> > index 01c0c7aa835c..53653d4b55c4 100644
> > --- a/drivers/reset/core.c
> > +++ b/drivers/reset/core.c
> > @@ -207,6 +207,19 @@ static int reset_control_array_reset(struct 
> > reset_control_array *resets)
> > return 0;
> >  }
> > 
> > +static int reset_control_array_resettable(struct reset_control_array 
> > *resets)
> > +{
> > +   int ret, i;
> > +
> > +   for (i = 0; i < resets->num_rstcs; i++) {
> > +   ret = reset_control_resettable(resets->rstc[i]);
> > +   if (ret)
> > +   return ret;
> > +   }

This is tricky, as we can't really roll back decrementing
triggered_count in case just one of those fails.

I think reset_control_array_resettable has to be open coded to first
check for errors and only then loop through all controls and decrement
their triggered_count.

> > +
> > +   return 0;
> > +}
> > +
> >  static int reset_control_array_assert(struct reset_control_array *resets)
> >  {
> > int ret, i;
> > @@ -324,6 +337,50 @@ int reset_control_reset(struct reset_control *rstc)
> >  }
> >  EXPORT_SYMBOL_GPL(reset_control_reset);
> > 
> > +/**
> > + * reset_control_resettable - decrements triggered_count of the controlled 
> > device
> > + * @rstc: reset controller

It is more important to document the purpose of the function than the
mechanism by which it is achieved. triggered_count is an implementation
detail.

Maybe "allow shared reset line to be triggered again" or similar. 

> > + *
> > + * On a shared reset line the actual reset pulse is only triggered once 
> > for the
> > + * lifetime of the reset_control instance, except if this function is used.
> > + * In fact, It decrements triggered_count that makes sure of not allowing
> > + * a reset if triggered_count is not null.
> > + *
> > + * This is a no-op in case triggered_count is already null i.e shared 
> > reset line
> > + * is ready to be triggered.

This is not a good idea IMHO. It would be better to document that calls
to this function must be balanced with calls to reset_control_reset, and
then throw a big warning below in case deassert_count ever dips below 0.

Otherwise nothing stops drivers from silently decrementing other
driver's trigger count by accidentally calling this multiple times.

> > + *
> > + * Consumers must not use reset_control_(de)assert on shared reset lines 
> > when
> > + * reset_control_reset has been used.
> > + *
> > + * If rstc is NULL it is an optional clear and the function will just
> > + * return 0.
> > + */
> > +int reset_control_resettable(struct reset_control *rstc)
> > +{
> > +   if (!rstc)
> > +   return 0;
> > +
> > +   if (WARN_ON(IS_ERR(rstc)))
> > +   return -EINVAL;
> > +
> > +   if (reset_control_is_array(rstc))
> > +   return reset_control_array_resettable(rstc_to_array(rstc));
> > +
> > +   if (rstc->shared) {
> > +   if (WARN_ON(atomic_read(>deassert_count) != 0))
> > +   return -EINVAL;
> > +
> > +   if (atomic_read(>triggered_count) > 0)
> > +   atomic_dec(>triggered_count);

I think this should be

WARN_ON(atomic_dec_return(>triggered_count) < 0);

regards
Philipp

Re: [PATCH] ARM: prima2: Constify static sirfsoc_rstc_ops

[Philipp Zabel]

Hi Rikard,

On Tue, 2020-09-29 at 22:17 +0200, Rikard Falkeborn wrote:
> The only usage of sirfsoc_rstc_ops is to assign its address to the ops
> field in the reset_controller_dev struct, which is a const pointer. Make
> it const to allow the compiler to put it in read-only memory.
> 
> Signed-off-by: Rikard Falkeborn 

Reviewed-by: Philipp Zabel 

> ---
>  arch/arm/mach-prima2/rstc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-prima2/rstc.c b/arch/arm/mach-prima2/rstc.c
> index 9d56606ac87f..1ee405e2dde9 100644
> --- a/arch/arm/mach-prima2/rstc.c
> +++ b/arch/arm/mach-prima2/rstc.c
> @@ -53,7 +53,7 @@ static int sirfsoc_reset_module(struct reset_controller_dev 
> *rcdev,
>   return 0;
>  }
>  
> -static struct reset_control_ops sirfsoc_rstc_ops = {
> +static const struct reset_control_ops sirfsoc_rstc_ops = {
>   .reset = sirfsoc_reset_module,
>  };
>  

regards
Philipp

Re: [PATCH 01/20] media: coda: use semicolons rather than commas to separate statements

[Philipp Zabel]

On Tue, 2020-09-29 at 15:14 +0200, Julia Lawall wrote:
> Replace commas with semicolons.  Commas introduce unnecessary
> variability in the code structure and are hard to see.  What is done
> is essentially described by the following Coccinelle semantic patch
> (http://coccinelle.lip6.fr/):
> 
> // 
> @@ expression e1,e2; @@
> e1
> -,
> +;
> e2
> ... when any
> // 
> 
> Signed-off-by: Julia Lawall 
> 
> ---
>  drivers/media/platform/coda/coda-common.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/coda/coda-common.c 
> b/drivers/media/platform/coda/coda-common.c
> index eeba6c060981..1bb16cc0a823 100644
> --- a/drivers/media/platform/coda/coda-common.c
> +++ b/drivers/media/platform/coda/coda-common.c
> @@ -2861,7 +2861,7 @@ static int coda_register_device(struct coda_dev *dev, 
> int i)
>   strscpy(vfd->name, dev->devtype->vdevs[i]->name, sizeof(vfd->name));
>   vfd->fops   = _fops;
>   vfd->ioctl_ops  = _ioctl_ops;
> - vfd->release= video_device_release_empty,
> + vfd->release= video_device_release_empty;
>   vfd->lock   = >dev_mutex;
>   vfd->v4l2_dev   = >v4l2_dev;
>   vfd->vfl_dir= VFL_DIR_M2M;
> 
> 

Thank you,

Reviewed-by: Philipp Zabel 

regards
Philipp

Re: [v3,2/3] PCI: mediatek: Add new generation controller support

[Philipp Zabel]

Hi Jianjun,

On Sun, 2020-09-27 at 15:45 +0800, Jianjun Wang wrote:
> MediaTek's PCIe host controller has three generation HWs, the new
> generation HW is an individual bridge, it supoorts Gen3 speed and
> up to 256 MSI interrupt numbers for multi-function devices.
> 
> Add support for new Gen3 controller which can be found on MT8192.
> 
> Signed-off-by: Jianjun Wang 
> Acked-by: Ryder Lee 
> ---
>  drivers/pci/controller/Kconfig  |   14 +
>  drivers/pci/controller/Makefile |1 +
>  drivers/pci/controller/pcie-mediatek-gen3.c | 1024 +++
>  3 files changed, 1039 insertions(+)
>  create mode 100644 drivers/pci/controller/pcie-mediatek-gen3.c
> 
[...]
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c 
> b/drivers/pci/controller/pcie-mediatek-gen3.c
> new file mode 100644
> index ..ad69c789b24d
> --- /dev/null
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> @@ -0,0 +1,1024 @@
[...]
> +static int mtk_pcie_power_up(struct mtk_pcie_port *port)
> +{
> + struct device *dev = port->dev;
> + int err;
> +
> + port->phy_reset = devm_reset_control_get_optional_exclusive(dev,
> + "phy-rst");
> + if (IS_ERR(port->phy_reset))
> + return PTR_ERR(port->phy_reset);
> +
> + reset_control_deassert(port->phy_reset);

In general, it is better to request all required resources before
starting to activate the hardware.

> +
> + /* PHY power on and enable pipe clock */
> + port->phy = devm_phy_optional_get(dev, "pcie-phy");
> + if (IS_ERR(port->phy))
> + return PTR_ERR(port->phy);

For example, if the PHY driver is not loaded yet and this returns
-EPROBE_DEFER, it was not useful to take the PHY out of reset above.
Also, phy-rst is kept deasserted if this fails.

> +
> + err = phy_init(port->phy);
> + if (err) {
> + dev_notice(dev, "failed to initialize pcie phy\n");
> + return err;

phy-rst is kept deasserted if this fails.

> + }
> +
> + err = phy_power_on(port->phy);
> + if (err) {
> + dev_notice(dev, "failed to power on pcie phy\n");
> + goto err_phy_on;
> + }
> +
> + port->mac_reset = devm_reset_control_get_optional_exclusive(dev,
> + "mac-rst");
> + if (IS_ERR(port->mac_reset))
> + return PTR_ERR(port->mac_reset);

The PHY is not powered down if this fails.

> +
> + reset_control_deassert(port->mac_reset);
> +
> + /* MAC power on and enable transaction layer clocks */
> + pm_runtime_enable(dev);
> + pm_runtime_get_sync(dev);
> +
> + err = mtk_pcie_clk_init(port);
> + if (err) {
> + dev_notice(dev, "clock init failed\n");
> + goto err_clk_init;
> + }
> +
> + return 0;
> +
> +err_clk_init:
> + pm_runtime_put_sync(dev);
> + pm_runtime_disable(dev);
> + reset_control_assert(port->mac_reset);
> + phy_power_off(port->phy);
> +err_phy_on:
> + phy_exit(port->phy);
> + reset_control_assert(port->phy_reset);
> +
> + return -EBUSY;
> +}
> +
> +static void mtk_pcie_power_down(struct mtk_pcie_port *port)
> +{
> + phy_power_off(port->phy);
> + phy_exit(port->phy);
> +
> + clk_bulk_disable_unprepare(port->num_clks, port->clks);

In the power-up sequence clocks are enabled last, but here they are not
disabled before the PHY is powered off. Is this on purpose?

> +
> + pm_runtime_put_sync(port->dev);
> + pm_runtime_disable(port->dev);

In the power-up error path, PHY and controller resets are asserted
again, but here they are kept deasserted. Should they be asserted here
as well?

regards
Philipp

Re: [v3,1/3] dt-bindings: PCI: mediatek: Add YAML schema

[Philipp Zabel]

On Sun, 2020-09-27 at 15:45 +0800, Jianjun Wang wrote:
> Add YAML schemas documentation for Gen3 PCIe controller on
> MediaTek SoCs.
> 
> Signed-off-by: Jianjun Wang 
> Acked-by: Ryder Lee 
> ---
>  .../bindings/pci/mediatek-pcie-gen3.yaml  | 126 ++
>  1 file changed, 126 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml 
> b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> new file mode 100644
> index ..c7b5dd132ebd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/mediatek-pcie-gen3.yaml
> @@ -0,0 +1,126 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/mediatek-pcie-gen3.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Gen3 PCIe controller on MediaTek SoCs
> +
> +maintainers:
> +  - Jianjun Wang 
> +
> +allOf:
> +  - $ref: /schemas/pci/pci-bus.yaml#
> +
> +properties:
> +  compatible:
> +oneOf:
> +  - const: mediatek,mt8192-pcie
> +
> +  reg:
> +maxItems: 1
> +
> +  interrupts:
> +maxItems: 1
> +
> +  ranges:
> +minItems: 1
> +maxItems: 8
> +
> +  resets:
> +minItems: 1
> +maxItems: 2
> +
> +  reset-names:
> +anyOf:
> +  - const: mac-rst
> +  - const: phy-rst

The "-rst" suffix seems redundant. Unless these are exactly the names
used in the documentation, I'd just call them "mac" and "phy".

regards
Philipp

Re: [PATCH] reset: sti: reset-syscfg: fix struct description warnings

[Philipp Zabel]

Hi Alain,

On Mon, 2020-08-31 at 22:38 +0200, Alain Volmat wrote:
> Fix formating of struct description to avoid warning highlighted
> by W=1 compilation.
> 
> Fixes: e5d76075d930 ("drivers: reset: STi SoC system configuration reset 
> controller support")
> Signed-off-by: Alain Volmat 

Thank you, applied to reset/next.

regards
Philipp

Re: [PATCH v2 1/2] ASoC: fsl_xcvr: Add XCVR ASoC CPU DAI driver

[Philipp Zabel]

On Mon, Sep 21, 2020 at 10:08:11PM +0300, Viorel Suman (OSS) wrote:
> From: Viorel Suman 
> 
> XCVR (Audio Transceiver) is a on-chip functional module found
> on i.MX8MP. It support HDMI2.1 eARC, HDMI1.4 ARC and SPDIF.
> 
> Signed-off-by: Viorel Suman 
> ---
>  sound/soc/fsl/Kconfig|   10 +
>  sound/soc/fsl/Makefile   |2 +
>  sound/soc/fsl/fsl_xcvr.c | 1343 
> ++
>  sound/soc/fsl/fsl_xcvr.h |  266 +
>  4 files changed, 1621 insertions(+)
>  create mode 100644 sound/soc/fsl/fsl_xcvr.c
>  create mode 100644 sound/soc/fsl/fsl_xcvr.h
> 
> diff --git a/sound/soc/fsl/Kconfig b/sound/soc/fsl/Kconfig
> index 3f76ff7..d04b64d 100644
> --- a/sound/soc/fsl/Kconfig
> +++ b/sound/soc/fsl/Kconfig
> @@ -95,6 +95,16 @@ config SND_SOC_FSL_EASRC
> destination sample rate. It is a new design module compare with the
> old ASRC.
>  
> +config SND_SOC_FSL_XCVR
> + tristate "NXP Audio Transceiver (XCVR) module support"
> + select REGMAP_MMIO
> + select SND_SOC_IMX_PCM_DMA if SND_IMX_SOC != n
> + select SND_SOC_GENERIC_DMAENGINE_PCM
> + help
> +   Say Y if you want to add Audio Transceiver (XCVR) support for NXP
> +   iMX CPUs. XCVR is a digital module that supports HDMI2.1 eARC,
> +   HDMI1.4 ARC and SPDIF.
> +
>  config SND_SOC_FSL_UTILS
>   tristate
>  
> diff --git a/sound/soc/fsl/Makefile b/sound/soc/fsl/Makefile
> index b835eeb..1d2231f 100644
> --- a/sound/soc/fsl/Makefile
> +++ b/sound/soc/fsl/Makefile
> @@ -25,6 +25,7 @@ snd-soc-fsl-utils-objs := fsl_utils.o
>  snd-soc-fsl-dma-objs := fsl_dma.o
>  snd-soc-fsl-mqs-objs := fsl_mqs.o
>  snd-soc-fsl-easrc-objs := fsl_easrc.o
> +snd-soc-fsl-xcvr-objs := fsl_xcvr.o
>  
>  obj-$(CONFIG_SND_SOC_FSL_AUDMIX) += snd-soc-fsl-audmix.o
>  obj-$(CONFIG_SND_SOC_FSL_ASOC_CARD) += snd-soc-fsl-asoc-card.o
> @@ -38,6 +39,7 @@ obj-$(CONFIG_SND_SOC_FSL_UTILS) += snd-soc-fsl-utils.o
>  obj-$(CONFIG_SND_SOC_FSL_MQS) += snd-soc-fsl-mqs.o
>  obj-$(CONFIG_SND_SOC_FSL_EASRC) += snd-soc-fsl-easrc.o
>  obj-$(CONFIG_SND_SOC_POWERPC_DMA) += snd-soc-fsl-dma.o
> +obj-$(CONFIG_SND_SOC_FSL_XCVR) += snd-soc-fsl-xcvr.o
>  
>  # MPC5200 Platform Support
>  obj-$(CONFIG_SND_MPC52xx_DMA) += mpc5200_dma.o
> diff --git a/sound/soc/fsl/fsl_xcvr.c b/sound/soc/fsl/fsl_xcvr.c
> new file mode 100644
> index ..7391bca
> --- /dev/null
> +++ b/sound/soc/fsl/fsl_xcvr.c
> @@ -0,0 +1,1343 @@
[...]
> +static int fsl_xcvr_probe(struct platform_device *pdev)
> +{
> + struct device *dev = >dev;
> + struct device_node *np = dev->of_node;
> + const struct of_device_id *of_id;
> + struct fsl_xcvr *xcvr;
> + struct resource *ram_res, *regs_res, *rx_res, *tx_res;
> + void __iomem *regs;
> + int ret, irq;
> +
> + of_id = of_match_device(fsl_xcvr_dt_ids, dev);
> + if (!of_id)
> + return -EINVAL;
> +
> + xcvr = devm_kzalloc(dev, sizeof(*xcvr), GFP_KERNEL);
> + if (!xcvr)
> + return -ENOMEM;
> +
> + xcvr->pdev = pdev;
> + xcvr->ipg_clk = devm_clk_get(dev, "ipg");
> + if (IS_ERR(xcvr->ipg_clk)) {
> + dev_err(dev, "failed to get ipg clock\n");
> + return PTR_ERR(xcvr->ipg_clk);
> + }
> +
> + xcvr->phy_clk = devm_clk_get(dev, "phy");
> + if (IS_ERR(xcvr->phy_clk)) {
> + dev_err(dev, "failed to get phy clock\n");
> + return PTR_ERR(xcvr->phy_clk);
> + }
> +
> + xcvr->spba_clk = devm_clk_get(dev, "spba");
> + if (IS_ERR(xcvr->spba_clk)) {
> + dev_err(dev, "failed to get spba clock\n");
> + return PTR_ERR(xcvr->spba_clk);
> + }
> +
> + xcvr->pll_ipg_clk = devm_clk_get(dev, "pll_ipg");
> + if (IS_ERR(xcvr->pll_ipg_clk)) {
> + dev_err(dev, "failed to get pll_ipg clock\n");
> + return PTR_ERR(xcvr->pll_ipg_clk);
> + }
> +
> + ram_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ram");
> + xcvr->ram_addr = devm_ioremap_resource(dev, ram_res);
> + if (IS_ERR(xcvr->ram_addr))
> + return PTR_ERR(xcvr->ram_addr);
> +
> + regs_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "regs");
> + regs = devm_ioremap_resource(dev, regs_res);
> + if (IS_ERR(regs))
> + return PTR_ERR(regs);
> +
> + xcvr->regmap = devm_regmap_init_mmio_clk(dev, NULL, regs,
> +  _xcvr_regmap_cfg);
> + if (IS_ERR(xcvr->regmap)) {
> + dev_err(dev, "failed to init XCVR regmap: %ld\n",
> + PTR_ERR(xcvr->regmap));
> + return PTR_ERR(xcvr->regmap);
> + }
> +
> + xcvr->reset = of_reset_control_get(np, NULL);

Please use devm_reset_control_get_exclusive().

[...]
> +static __maybe_unused int fsl_xcvr_runtime_resume(struct device *dev)
> +{
> + struct fsl_xcvr *xcvr = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = clk_prepare_enable(xcvr->ipg_clk);
> + if (ret) {
> +   

Re: [PATCH V2 2/2] ata: ahci: ceva: Update the driver to support xilinx GT phy

[Philipp Zabel]

On Tue, 2020-09-22 at 15:45 +0530, Piyush Mehta wrote:
> SATA controller used in Xilinx ZynqMP platform uses xilinx GT phy
> which has 4 GT lanes and can used by 4 peripherals at a time.
> SATA controller uses 1 GT phy lane among the 4 GT lanes. To configure
> the GT lane for SATA controller, the below sequence is expected.
> 
> 1. Assert the SATA controller reset.
> 2. Configure the xilinx GT phy lane for SATA controller (phy_init).
> 3. De-assert the SATA controller reset.
> 4. Wait for PLL of the GT lane used by SATA to be locked (phy_power_on).
> 
> The ahci_platform_enable_resources() by default does the phy_init()
> and phy_power_on() but the default sequence doesn't work with Xilinx
> platforms. Because of this reason, updated the driver to support the
> new sequence.
> 
> Added is_rst_ctrl flag, for backward compatibility with the older
> sequence. If the reset controller is not available, then the SATA
> controller will configure with the older sequences.
> 
> Signed-off-by: Piyush Mehta 
> ---
>  drivers/ata/ahci_ceva.c | 39 +--
>  1 file changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/ahci_ceva.c b/drivers/ata/ahci_ceva.c
> index b10fd4c..c704906 100644
> --- a/drivers/ata/ahci_ceva.c
> +++ b/drivers/ata/ahci_ceva.c
> @@ -12,6 +12,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include "ahci.h"
>  
>  /* Vendor Specific Register Offsets */
> @@ -87,6 +88,7 @@ struct ceva_ahci_priv {
>   u32 axicc;
>   bool is_cci_enabled;
>   int flags;
> + struct reset_control *rst;
>  };
>  
>  static unsigned int ceva_ahci_read_id(struct ata_device *dev,
> @@ -194,7 +196,7 @@ static int ceva_ahci_probe(struct platform_device *pdev)
>   struct ahci_host_priv *hpriv;
>   struct ceva_ahci_priv *cevapriv;
>   enum dev_dma_attr attr;
> - int rc;
> + int rc, i, is_rst_ctrl = 1;
>  
>   cevapriv = devm_kzalloc(dev, sizeof(*cevapriv), GFP_KERNEL);
>   if (!cevapriv)
> @@ -202,14 +204,47 @@ static int ceva_ahci_probe(struct platform_device *pdev)
>  
>   cevapriv->ahci_pdev = pdev;
>  
> + cevapriv->rst = devm_reset_control_get(>dev, NULL);

Please use devm_reset_control_get_optional_exclusive()

> + if (IS_ERR(cevapriv->rst)) {
> + if (PTR_ERR(cevapriv->rst) != -EPROBE_DEFER)
> + dev_err(>dev, "failed to get reset: %ld\n",
> + PTR_ERR(cevapriv->rst));
> + is_rst_ctrl = 0;

is_rst_ctrl will not be required then.

> + }
> +
>   hpriv = ahci_platform_get_resources(pdev, 0);
>   if (IS_ERR(hpriv))
>   return PTR_ERR(hpriv);
> + if (is_rst_ctrl)
> + rc = ahci_platform_enable_clks(hpriv);
> + else
> + rc = ahci_platform_enable_resources(hpriv);
>  
> - rc = ahci_platform_enable_resources(hpriv);
>   if (rc)
>   return rc;
>  
> + if (is_rst_ctrl) {

This can just be "if (cevapriv->rst)"

> + /* Assert the controller reset */
> + reset_control_assert(cevapriv->rst);
> +
> + for (i = 0; i < hpriv->nports; i++) {
> + rc = phy_init(hpriv->phys[i]);
> + if (rc)
> + return rc;
> + }
> +
> + /* De-assert the controller reset */
> + reset_control_deassert(cevapriv->rst);
> +
> + for (i = 0; i < hpriv->nports; i++) {
> + rc = phy_power_on(hpriv->phys[i]);
> + if (rc) {
> + phy_exit(hpriv->phys[i]);
> + return rc;
> + }
> + }
> + }
> +
>   if (of_property_read_bool(np, "ceva,broken-gen2"))
>   cevapriv->flags = CEVA_FLAG_BROKEN_GEN2;
>  

regards
Philipp

Re: [PATCH -next] drm/panfrost: simplify the return expression of cz_ih_hw_init()

[Philipp Zabel]

On Mon, 2020-09-21 at 21:10 +0800, Qinglang Miao wrote:
> Simplify the return expression.
> 
> Signed-off-by: Qinglang Miao 
> ---
>  drivers/gpu/drm/panfrost/panfrost_device.c | 8 +---
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c 
> b/drivers/gpu/drm/panfrost/panfrost_device.c
> index e68967338..ea8d31863 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -18,19 +18,13 @@
>  
>  static int panfrost_reset_init(struct panfrost_device *pfdev)
>  {
> - int err;
> -
>   pfdev->rstc = devm_reset_control_array_get(pfdev->dev, false, true);
>   if (IS_ERR(pfdev->rstc)) {
>   dev_err(pfdev->dev, "get reset failed %ld\n", 
> PTR_ERR(pfdev->rstc));
>   return PTR_ERR(pfdev->rstc);
>   }
>  
> - err = reset_control_deassert(pfdev->rstc);
> - if (err)
> - return err;
> -
> - return 0;
> + return reset_control_deassert(pfdev->rstc);
>  }
>  
>  static void panfrost_reset_fini(struct panfrost_device *pfdev)

Reviewed-by: Philipp Zabel 

regards
Philipp

Re: [Re-send][PATCH] gpu/ipu-v3:reduce protected code area in ipu idmac get/put

[Philipp Zabel]

Hi Bernard,

On Mon, 2020-09-21 at 19:11 +0800, Bernard wrote:
> This change will speed-up a bit these ipu_idmac_get &
> ipu_idmac_put processing and there is no need to protect
> kzalloc & kfree.

I don't think that will be measurable, the channel lock is very unlikely
to be contended. It might make more sense to replace the list walk with
a bitfield.

> Signed-off-by: Bernard Zhao 
> ---
>  drivers/gpu/ipu-v3/ipu-common.c | 24 +---
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/ipu-v3/ipu-common.c b/drivers/gpu/ipu-v3/ipu-common.c
> index b3dae9ec1a38..8b3a57864c2e 100644
> --- a/drivers/gpu/ipu-v3/ipu-common.c
> +++ b/drivers/gpu/ipu-v3/ipu-common.c
> @@ -267,29 +267,30 @@ EXPORT_SYMBOL_GPL(ipu_rot_mode_to_degrees);
>  struct ipuv3_channel *ipu_idmac_get(struct ipu_soc *ipu, unsigned num)
>  {
>   struct ipuv3_channel *channel;
> + struct ipuv3_channel *entry;
>  
>   dev_dbg(ipu->dev, "%s %d\n", __func__, num);
>  
>   if (num > 63)
>   return ERR_PTR(-ENODEV);
>  
> + channel = kzalloc(sizeof(*channel), GFP_KERNEL);
> + if (!channel)
> + return ERR_PTR(-ENOMEM);
> +
> + channel->num = num;
> + channel->ipu = ipu;
> +
>   mutex_lock(>channel_lock);
>  
> - list_for_each_entry(channel, >channels, list) {
> - if (channel->num == num) {
> + list_for_each_entry(entry, >channels, list) {
> + if (entry->num == num) {
> + kfree(channel);
>   channel = ERR_PTR(-EBUSY);
>   goto out;

This leaks the channel memory allocated above.

>   }
>   }
> 
> - channel = kzalloc(sizeof(*channel), GFP_KERNEL);
> - if (!channel) {
> - channel = ERR_PTR(-ENOMEM);
> - goto out;
> - }
> -
> - channel->num = num;
> - channel->ipu = ipu;
>   list_add(>list, >channels);
>  
>  out:
> @@ -308,9 +309,10 @@ void ipu_idmac_put(struct ipuv3_channel *channel)
>   mutex_lock(>channel_lock);
>  
>   list_del(>list);
> - kfree(channel);
>  
>   mutex_unlock(>channel_lock);
> +
> + kfree(channel);
>  }
>  EXPORT_SYMBOL_GPL(ipu_idmac_put);

regards
Philipp