Re: [PATCH 2/2] media:imx274 V4l2 driver for Sony imx274 CMOS sensor

[Sören Brinkmann]

On Mon, 2017-08-28 at 14:22:03 -0400, Nicolas Dufresne wrote:
> Le lundi 28 août 2017 à 08:15 -0700, Soren Brinkmann a écrit :
[...]
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index 94153895fcd4..4e8b64575b2a 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -547,16 +547,12 @@ config VIDEO_APTINA_PLL
> >  config VIDEO_SMIAPP_PLL
> > tristate
> >  
> > -config VIDEO_OV2640
> > -   tristate "OmniVision OV2640 sensor support"
> > -   depends on VIDEO_V4L2 && I2C
> > -   depends on MEDIA_CAMERA_SUPPORT
> > -   help
> > - This is a Video4Linux2 sensor-level driver for the OmniVision
> > - OV2640 camera.
> > -
> > - To compile this driver as a module, choose M here: the
> > - module will be called ov2640.
> 
> Is this removal of another sensor intentional ?

Oops, no, some rebase gone wrong, I guess. I'll put that on the list to
fix for v2.

Thanks,
Sören

Re: [PATCH 2/2] media:imx274 V4l2 driver for Sony imx274 CMOS sensor

[Sören Brinkmann]

On Mon, 2017-08-28 at 14:22:03 -0400, Nicolas Dufresne wrote:
> Le lundi 28 août 2017 à 08:15 -0700, Soren Brinkmann a écrit :
[...]
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index 94153895fcd4..4e8b64575b2a 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -547,16 +547,12 @@ config VIDEO_APTINA_PLL
> >  config VIDEO_SMIAPP_PLL
> > tristate
> >  
> > -config VIDEO_OV2640
> > -   tristate "OmniVision OV2640 sensor support"
> > -   depends on VIDEO_V4L2 && I2C
> > -   depends on MEDIA_CAMERA_SUPPORT
> > -   help
> > - This is a Video4Linux2 sensor-level driver for the OmniVision
> > - OV2640 camera.
> > -
> > - To compile this driver as a module, choose M here: the
> > - module will be called ov2640.
> 
> Is this removal of another sensor intentional ?

Oops, no, some rebase gone wrong, I guess. I'll put that on the list to
fix for v2.

Thanks,
Sören

Re: [RFC PATCH 1/4] serial: uartps: Remove console_initcall from the driver

[Sören Brinkmann]

On Fri, 2017-07-21 at 11:32:24 +0200, Michal Simek wrote:
> register_console() is called from
> uart_add_one_port()->uart_configure_port()
> that's why register_console() is called twice.
> 
> This patch remove console_initcall to call register_console() only from
> one location.
> 
> Also based on my tests cdns_uart_console_setup() is not called
> from the first register_console() call.
> 
> Signed-off-by: Michal Simek 
> ---
> 
> I am not quite sure about this because console_initcall is called
> early but I can see any difference in usage.
> pl011 is not calling this but others are doing it.

Doesn't this break early console/printk? I would expect that the UART
initialization may happen later than console init.

Sören

Re: [RFC PATCH 1/4] serial: uartps: Remove console_initcall from the driver

[Sören Brinkmann]

On Fri, 2017-07-21 at 11:32:24 +0200, Michal Simek wrote:
> register_console() is called from
> uart_add_one_port()->uart_configure_port()
> that's why register_console() is called twice.
> 
> This patch remove console_initcall to call register_console() only from
> one location.
> 
> Also based on my tests cdns_uart_console_setup() is not called
> from the first register_console() call.
> 
> Signed-off-by: Michal Simek 
> ---
> 
> I am not quite sure about this because console_initcall is called
> early but I can see any difference in usage.
> pl011 is not calling this but others are doing it.

Doesn't this break early console/printk? I would expect that the UART
initialization may happen later than console init.

Sören

Re: [PATCH] dmaengine: zynqmp_dma: fix error return code in zynqmp_dma_chan_probe()

[Sören Brinkmann]

On Fri, 2017-06-30 at 02:42:47 -0500, Gustavo A. R. Silva wrote:
> Print error message and propagate the return value of
> platform_get_irq on failure.
> 
> Signed-off-by: Gustavo A. R. Silva <garsi...@embeddedor.com>
Reviewed-by: Sören Brinkmann <soren.brinkm...@xilinx.com>

Sören

Re: [PATCH] dmaengine: zynqmp_dma: fix error return code in zynqmp_dma_chan_probe()

[Sören Brinkmann]

On Fri, 2017-06-30 at 02:42:47 -0500, Gustavo A. R. Silva wrote:
> Print error message and propagate the return value of
> platform_get_irq on failure.
> 
> Signed-off-by: Gustavo A. R. Silva 
Reviewed-by: Sören Brinkmann 

Sören

Re: [PATCH] usb: gadget: udc-xilinx: remove useless variable assignment in xudc_read_fifo()

[Sören Brinkmann]

On Tue, 2017-06-27 at 16:30:30 -0500, Gustavo A. R. Silva wrote:
> Value assigned to variable bufferspace at line 637 is overwritten
> at line 613, before it can be used. This makes such variable
> assignment useless.
> 
> Addresses-Coverity-ID: 1397677
> Signed-off-by: Gustavo A. R. Silva <garsi...@embeddedor.com>
Reviewed-by: Sören Brinkmann <soren.brinkm...@xilinx.com>

Sören

Re: [PATCH] usb: gadget: udc-xilinx: remove useless variable assignment in xudc_read_fifo()

[Sören Brinkmann]

On Tue, 2017-06-27 at 16:30:30 -0500, Gustavo A. R. Silva wrote:
> Value assigned to variable bufferspace at line 637 is overwritten
> at line 613, before it can be used. This makes such variable
> assignment useless.
> 
> Addresses-Coverity-ID: 1397677
> Signed-off-by: Gustavo A. R. Silva 
Reviewed-by: Sören Brinkmann 

Sören

Re: [PATCH] gpio: zynq: remove unneeded (void *) casts in of_match_table

[Sören Brinkmann]

On Sat, 2017-05-13 at 01:18:45 +0900, Masahiro Yamada wrote:
> of_device_id::data is an opaque pointer.  No explicit cast is needed.
> 
> Signed-off-by: Masahiro Yamada <yamada.masah...@socionext.com>
Acked-by: Sören Brinkmann <soren.brinkm...@xilinx.com>

Sören

Re: [PATCH] gpio: zynq: remove unneeded (void *) casts in of_match_table

[Sören Brinkmann]

On Sat, 2017-05-13 at 01:18:45 +0900, Masahiro Yamada wrote:
> of_device_id::data is an opaque pointer.  No explicit cast is needed.
> 
> Signed-off-by: Masahiro Yamada 
Acked-by: Sören Brinkmann 

Sören

Re: [PATCH] usb: gadget: udc-xilinx: clean up a variable name

[Sören Brinkmann]

On Thu, 2017-04-27 at 12:11:18 +0300, Dan Carpenter wrote:
> "ep->udc->lock" and "udc->lock" are the same thing.  It confuses Smatch
> if we don't use the same name consistently.
> 
> Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
Reviewed-by: Sören Brinkmann <soren.brinkm...@xilinx.com>

Sören

Re: [PATCH] usb: gadget: udc-xilinx: clean up a variable name

[Sören Brinkmann]

On Thu, 2017-04-27 at 12:11:18 +0300, Dan Carpenter wrote:
> "ep->udc->lock" and "udc->lock" are the same thing.  It confuses Smatch
> if we don't use the same name consistently.
> 
> Signed-off-by: Dan Carpenter 
Reviewed-by: Sören Brinkmann 

Sören

Re: [PATCH v2] ARM64: zynqmp: Fix i2c node's compatible string

[Sören Brinkmann]

On Thu, 2016-12-22 at 09:19:25 -0800, m...@kernel.org wrote:
> From: Moritz Fischer <m...@kernel.org>
> 
> The Zynq Ultrascale MP uses version 1.4 of the Cadence IP core
> which fixes some silicon bugs that needed software workarounds
> in Version 1.0 that was used on Zynq systems.
> 
> Signed-off-by: Moritz Fischer <m...@kernel.org>
> Cc: Michal Simek <michal.si...@xilinx.com>
> Cc: Sören Brinkmann <soren.brinkm...@xilinx.com>
> Cc: Rob Herring <robh...@kernel.org>
Acked-by: Sören Brinkmann <soren.brinkm...@xilinx.com>

Sören

Re: [PATCH v2] ARM64: zynqmp: Fix i2c node's compatible string

[Sören Brinkmann]

On Thu, 2016-12-22 at 09:19:25 -0800, m...@kernel.org wrote:
> From: Moritz Fischer 
> 
> The Zynq Ultrascale MP uses version 1.4 of the Cadence IP core
> which fixes some silicon bugs that needed software workarounds
> in Version 1.0 that was used on Zynq systems.
> 
> Signed-off-by: Moritz Fischer 
> Cc: Michal Simek 
> Cc: Sören Brinkmann 
> Cc: Rob Herring 
Acked-by: Sören Brinkmann 

Sören

Re: [PATCH 3/4] fpga mgr: zynq: Add support for encrypted bitstreams

[Sören Brinkmann]

On Sun, 2016-11-06 at 17:13:25 -0700, Moritz Fischer wrote:
> Add new flag FPGA_MGR_DECRYPT_BISTREAM as well as a matching
> capability FPGA_MGR_CAP_DECRYPT to allow for on-the-fly
> decryption of an encrypted bitstream.
> 
> If the system is not booted in secure mode AES & HMAC units
> are disabled by the boot ROM, therefore the capability
> is not available.
> 
> Signed-off-by: Moritz Fischer <moritz.fisc...@ettus.com>
> Cc: Alan Tull <at...@opensource.altera.com>
> Cc: Michal Simek <michal.si...@xilinx.com>
> Cc: Sören Brinkmann <soren.brinkm...@xilinx.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-arm-ker...@lists.infradead.org
> ---
>  drivers/fpga/fpga-mgr.c   |  7 +++
>  drivers/fpga/zynq-fpga.c  | 21 +++--
>  include/linux/fpga/fpga-mgr.h |  2 ++
>  3 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index 98230b7..e4d08e1 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -61,6 +61,12 @@ int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, 
> const char *buf,
>   return -ENOTSUPP;
>   }
>  
> + if (flags & FPGA_MGR_DECRYPT_BITSTREAM &&
> + !fpga_mgr_has_cap(FPGA_MGR_CAP_DECRYPT, mgr->caps)) {
> + dev_err(dev, "Bitstream decryption not supported\n");
> + return -ENOTSUPP;
> + }
> +
>   /*
>* Call the low level driver's write_init function.  This will do the
>* device-specific things to get the FPGA into the state where it is
> @@ -170,6 +176,7 @@ static const char * const state_str[] = {
>  static const char * const cap_str[] = {
>   [FPGA_MGR_CAP_FULL_RECONF] = "Full reconfiguration",
>   [FPGA_MGR_CAP_PARTIAL_RECONF] = "Partial reconfiguration",
> + [FPGA_MGR_CAP_DECRYPT] = "Decrypt bitstream on the fly",
>  };
>  
>  static ssize_t name_show(struct device *dev,
> diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
> index 1d37ff0..0aa4705 100644
> --- a/drivers/fpga/zynq-fpga.c
> +++ b/drivers/fpga/zynq-fpga.c
> @@ -71,6 +71,10 @@
>  #define CTRL_PCAP_PR_MASKBIT(27)
>  /* Enable PCAP */
>  #define CTRL_PCAP_MODE_MASK  BIT(26)
> +/* Needed to reduce clock rate for secure config */
> +#define CTRL_PCAP_RATE_EN_MASK   BIT(25)
> +/* System booted in secure mode */
> +#define CTRL_SEC_EN_MASK BIT(7)
>  
>  /* Miscellaneous Control Register bit definitions */
>  /* Internal PCAP loopback */
> @@ -252,12 +256,20 @@ static int zynq_fpga_ops_write_init(struct fpga_manager 
> *mgr, u32 flags,
>  
>   /* set configuration register with following options:
>* - enable PCAP interface
> -  * - set throughput for maximum speed
> +  * - set throughput for maximum speed (if we're not decrypting)
>* - set CPU in user mode
>*/
>   ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
> - zynq_fpga_write(priv, CTRL_OFFSET,
> + if (flags & FPGA_MGR_DECRYPT_BITSTREAM) {
> + zynq_fpga_write(priv, CTRL_OFFSET,
> + (CTRL_PCAP_PR_MASK | CTRL_PCAP_MODE_MASK |
> +  CTRL_PCAP_RATE_EN_MASK | ctrl));
> +
> + } else {
> + ctrl &= ~CTRL_PCAP_RATE_EN_MASK;
> + zynq_fpga_write(priv, CTRL_OFFSET,
>   (CTRL_PCAP_PR_MASK | CTRL_PCAP_MODE_MASK | ctrl));
> + }

Minor nit:
Assuming that there may be more caps to check to come, wouldn't it be
slightly easier to write this in a way like?:
  if (flags & SOME_FLAG)
 ctrl |= FOO;
  if (flags & SOME_OTHER_FLAG)
 ctrl |= BAR;
  zynq_fpga_write(priv, CTRL_OFFSET, ctrl);

i.e. moving the fpga_write outside of the conditionals.

Sören

Re: [PATCH 3/4] fpga mgr: zynq: Add support for encrypted bitstreams

[Sören Brinkmann]

On Sun, 2016-11-06 at 17:13:25 -0700, Moritz Fischer wrote:
> Add new flag FPGA_MGR_DECRYPT_BISTREAM as well as a matching
> capability FPGA_MGR_CAP_DECRYPT to allow for on-the-fly
> decryption of an encrypted bitstream.
> 
> If the system is not booted in secure mode AES & HMAC units
> are disabled by the boot ROM, therefore the capability
> is not available.
> 
> Signed-off-by: Moritz Fischer 
> Cc: Alan Tull 
> Cc: Michal Simek 
> Cc: Sören Brinkmann 
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-arm-ker...@lists.infradead.org
> ---
>  drivers/fpga/fpga-mgr.c   |  7 +++
>  drivers/fpga/zynq-fpga.c  | 21 +++--
>  include/linux/fpga/fpga-mgr.h |  2 ++
>  3 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> index 98230b7..e4d08e1 100644
> --- a/drivers/fpga/fpga-mgr.c
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -61,6 +61,12 @@ int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, 
> const char *buf,
>   return -ENOTSUPP;
>   }
>  
> + if (flags & FPGA_MGR_DECRYPT_BITSTREAM &&
> + !fpga_mgr_has_cap(FPGA_MGR_CAP_DECRYPT, mgr->caps)) {
> + dev_err(dev, "Bitstream decryption not supported\n");
> + return -ENOTSUPP;
> + }
> +
>   /*
>* Call the low level driver's write_init function.  This will do the
>* device-specific things to get the FPGA into the state where it is
> @@ -170,6 +176,7 @@ static const char * const state_str[] = {
>  static const char * const cap_str[] = {
>   [FPGA_MGR_CAP_FULL_RECONF] = "Full reconfiguration",
>   [FPGA_MGR_CAP_PARTIAL_RECONF] = "Partial reconfiguration",
> + [FPGA_MGR_CAP_DECRYPT] = "Decrypt bitstream on the fly",
>  };
>  
>  static ssize_t name_show(struct device *dev,
> diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
> index 1d37ff0..0aa4705 100644
> --- a/drivers/fpga/zynq-fpga.c
> +++ b/drivers/fpga/zynq-fpga.c
> @@ -71,6 +71,10 @@
>  #define CTRL_PCAP_PR_MASKBIT(27)
>  /* Enable PCAP */
>  #define CTRL_PCAP_MODE_MASK  BIT(26)
> +/* Needed to reduce clock rate for secure config */
> +#define CTRL_PCAP_RATE_EN_MASK   BIT(25)
> +/* System booted in secure mode */
> +#define CTRL_SEC_EN_MASK BIT(7)
>  
>  /* Miscellaneous Control Register bit definitions */
>  /* Internal PCAP loopback */
> @@ -252,12 +256,20 @@ static int zynq_fpga_ops_write_init(struct fpga_manager 
> *mgr, u32 flags,
>  
>   /* set configuration register with following options:
>* - enable PCAP interface
> -  * - set throughput for maximum speed
> +  * - set throughput for maximum speed (if we're not decrypting)
>* - set CPU in user mode
>*/
>   ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
> - zynq_fpga_write(priv, CTRL_OFFSET,
> + if (flags & FPGA_MGR_DECRYPT_BITSTREAM) {
> + zynq_fpga_write(priv, CTRL_OFFSET,
> + (CTRL_PCAP_PR_MASK | CTRL_PCAP_MODE_MASK |
> +  CTRL_PCAP_RATE_EN_MASK | ctrl));
> +
> + } else {
> + ctrl &= ~CTRL_PCAP_RATE_EN_MASK;
> + zynq_fpga_write(priv, CTRL_OFFSET,
>   (CTRL_PCAP_PR_MASK | CTRL_PCAP_MODE_MASK | ctrl));
> + }

Minor nit:
Assuming that there may be more caps to check to come, wouldn't it be
slightly easier to write this in a way like?:
  if (flags & SOME_FLAG)
 ctrl |= FOO;
  if (flags & SOME_OTHER_FLAG)
 ctrl |= BAR;
  zynq_fpga_write(priv, CTRL_OFFSET, ctrl);

i.e. moving the fpga_write outside of the conditionals.

Sören

Re: [Patch v5 04/12] irqchip: xilinx: Add support for parent intc

[Sören Brinkmann]

On Tue, 2016-10-25 at 12:49:33 +0200, Thomas Gleixner wrote:
> On Tue, 25 Oct 2016, Zubair Lutfullah Kakakhel wrote:
> > On 10/21/2016 10:48 AM, Marc Zyngier wrote:
> > > Shouldn't you return an error if irq is zero?
> > > 
> > 
> > I'll add the following for the error case
> > 
> > pr_err("%s: Parent exists but interrupts property not defined\n" ,
> > __func__);
> 
> Please do not use this silly __func__ stuff. It's not giving any value to
> the printout.
> 
> Set a proper prefix for your pr_* stuff, so the string is prefixed with
> 'irq-xilinx:' or whatever you think is appropriate. Then the string itself
> is good enough to find from which place this printk comes.

Haven't looked at the real code, but is there probably a way to get a
struct device pointer and use dev_err?

Sören

Re: [Patch v5 04/12] irqchip: xilinx: Add support for parent intc

[Sören Brinkmann]

On Tue, 2016-10-25 at 12:49:33 +0200, Thomas Gleixner wrote:
> On Tue, 25 Oct 2016, Zubair Lutfullah Kakakhel wrote:
> > On 10/21/2016 10:48 AM, Marc Zyngier wrote:
> > > Shouldn't you return an error if irq is zero?
> > > 
> > 
> > I'll add the following for the error case
> > 
> > pr_err("%s: Parent exists but interrupts property not defined\n" ,
> > __func__);
> 
> Please do not use this silly __func__ stuff. It's not giving any value to
> the printout.
> 
> Set a proper prefix for your pr_* stuff, so the string is prefixed with
> 'irq-xilinx:' or whatever you think is appropriate. Then the string itself
> is good enough to find from which place this printk comes.

Haven't looked at the real code, but is there probably a way to get a
struct device pointer and use dev_err?

Sören

Re: [PATCH] Adding Support for Coresight Components on Zynq 7000.

[Sören Brinkmann]

Hi Muhammad,

On Mon, 2016-10-03 at 14:56:16 +0200, Muhammad Abdul WAHAB wrote:
> Hi Sören,
> 
> > I tried to refresh my Zynq knowledge a bit. The clkc provides the
> > dbg_trc clock, and that is the clock you need (not fclk). I couldn't
> > find it in the binding (I guess I messed that up), but apparently,
> > you can provide a 'trace_emio_clk' as input to the clkc node in the
> > Zynq DT. Then, with the muxes correctly configured (FSBL should do
> > that if you select the EMIO trace clock in Vivado), the dbg_trc
> > output of the clkc should be that EMIO clock. And the dbg_trc output
> > of the clkc is what should be consumed by the tpiu node. Though, as
> > I see it the binding/driver for the TPIU do not support that.
> >
> > I.e.
> > In the clkc description you'd have to add 'trace_emio_clk' to the
> > clock-names property together with a matching reference in the 'clocks'
> > property. As this change would be specific to local setups, this is not
> > really appropriate for upstream.
> >
> > Then, for the trace clock, ideally the TPIU would consume and enable it
> > as needed.
> 
> Thank you very much for this. I will have a look into it.
> Below is the patch without TPIU, is it possible to submit it ? I will submit
> the TPIU part very soon once I manage to get it working.

Sounds good. AFAICT, the change below should be OK. Probably some
stylistic changes to make it blend in with the rest of the DT (e.g.
use lower case characters in the address parts of the node name).

> 
> > Unfortunately, this is not how it works. The DT bindings are not a
> > recommendation. The DT description must follow the binding, otherwise
> > drivers will not work correctly, or best case, just ignore what you put
> > there.
> 
> Thanks. The idea of including TPIU part was to get feedback as I am far from
> being an expert on DT.
> 
> > As I don't see this in the coresight binding, I doubt that it has any
> > effect or should be here.
> 
> That's what I was thinking also. I will re-look into TPIU part and send it
> soon. Besides, if I want to ask you a question regarding TPIU or DT, can I
> contact you alone or should I keep sending it to all the CS/DT maintainers ?

I'd say that depends on what it is about. If it is about DT and the TPIU
Linux driver, I'd say, keep it on list and probably even include the
authors of that driver (the folks the get_maintainers script is
identifying for that driver).

If it's specific to Zynq, the Xilinx forums can be quite helpful as
there are a lot of people familiar with the device
(https://forums.xilinx.com/t5/Embedded-Linux/bd-p/ELINUX).

But when in doubt, feel free to reach out to me directly.

Sören

Re: [PATCH] Adding Support for Coresight Components on Zynq 7000.

[Sören Brinkmann]

Hi Muhammad,

On Mon, 2016-10-03 at 14:56:16 +0200, Muhammad Abdul WAHAB wrote:
> Hi Sören,
> 
> > I tried to refresh my Zynq knowledge a bit. The clkc provides the
> > dbg_trc clock, and that is the clock you need (not fclk). I couldn't
> > find it in the binding (I guess I messed that up), but apparently,
> > you can provide a 'trace_emio_clk' as input to the clkc node in the
> > Zynq DT. Then, with the muxes correctly configured (FSBL should do
> > that if you select the EMIO trace clock in Vivado), the dbg_trc
> > output of the clkc should be that EMIO clock. And the dbg_trc output
> > of the clkc is what should be consumed by the tpiu node. Though, as
> > I see it the binding/driver for the TPIU do not support that.
> >
> > I.e.
> > In the clkc description you'd have to add 'trace_emio_clk' to the
> > clock-names property together with a matching reference in the 'clocks'
> > property. As this change would be specific to local setups, this is not
> > really appropriate for upstream.
> >
> > Then, for the trace clock, ideally the TPIU would consume and enable it
> > as needed.
> 
> Thank you very much for this. I will have a look into it.
> Below is the patch without TPIU, is it possible to submit it ? I will submit
> the TPIU part very soon once I manage to get it working.

Sounds good. AFAICT, the change below should be OK. Probably some
stylistic changes to make it blend in with the rest of the DT (e.g.
use lower case characters in the address parts of the node name).

> 
> > Unfortunately, this is not how it works. The DT bindings are not a
> > recommendation. The DT description must follow the binding, otherwise
> > drivers will not work correctly, or best case, just ignore what you put
> > there.
> 
> Thanks. The idea of including TPIU part was to get feedback as I am far from
> being an expert on DT.
> 
> > As I don't see this in the coresight binding, I doubt that it has any
> > effect or should be here.
> 
> That's what I was thinking also. I will re-look into TPIU part and send it
> soon. Besides, if I want to ask you a question regarding TPIU or DT, can I
> contact you alone or should I keep sending it to all the CS/DT maintainers ?

I'd say that depends on what it is about. If it is about DT and the TPIU
Linux driver, I'd say, keep it on list and probably even include the
authors of that driver (the folks the get_maintainers script is
identifying for that driver).

If it's specific to Zynq, the Xilinx forums can be quite helpful as
there are a lot of people familiar with the device
(https://forums.xilinx.com/t5/Embedded-Linux/bd-p/ELINUX).

But when in doubt, feel free to reach out to me directly.

Sören

Re: [PATCH] Adding Support for Coresight Components on Zynq 7000.

[Sören Brinkmann]

Hi Muhammad,

On Fri, 2016-09-30 at 09:39:51 +0200, Muhammad Abdul WAHAB wrote:
> Hi Sören,
> 
> Thank you for your remarks. I corrected a few things as you suggested.
> 
[...]
> >> +tpiu@F8803000 {
> >>>
> >>> +compatible = "arm,coresight-tpiu", "arm,primecell";
> >>> +reg = <0xf8803000 0x1000>;
> >>> +clocks = < 47>, < 16>;
> >>
> >
> > I'm not sure this is correct for every setup. Sorry, that I don't recall
> > all the details, I haven't used tracing in a long time. But I guess this
> > clock is configurable as you're referring an fclk here. The other thing
> > that makes me a little suspicious is, that nothing in here uses the
> > 'dbg_trc' clock that the clock controller provides.
> 
> The TPIU setup included in my patch is specific to my configuration of TPIU.
> The second clock is configurable but I didn't know how to do so. As in
> Vivado setup I chose "fclk1" as the clock for TPIU, I decided to put fclk1.
> But, I am not sure about this. I am having some problems when I recover the
> trace from TPIU: it is not the same as in ETB even though it resembles a
> lot.
> I don't know if the problem is coming from the device tree or from the
> driver.
> I will have a look at 'dbg_trc' clock.

I tried to refresh my Zynq knowledge a bit. The clkc provides the
dbg_trc clock, and that is the clock you need (not fclk). I couldn't
find it in the binding (I guess I messed that up), but apparently,
you can provide a 'trace_emio_clk' as input to the clkc node in the
Zynq DT. Then, with the muxes correctly configured (FSBL should do
that if you select the EMIO trace clock in Vivado), the dbg_trc
output of the clkc should be that EMIO clock. And the dbg_trc output
of the clkc is what should be consumed by the tpiu node. Though, as
I see it the binding/driver for the TPIU do not support that.

I.e.
In the clkc description you'd have to add 'trace_emio_clk' to the
clock-names property together with a matching reference in the 'clocks'
property. As this change would be specific to local setups, this is not
really appropriate for upstream.

Then, for the trace clock, ideally the TPIU would consume and enable it
as needed.

> 
> >>
> >> +clock-names = "apb_pclk", "fclk1";
> >>
> > Those names (at least fclk1) is not a good name for tpiu to identify
> > it's input. fclk1 is a zynq-specific clock, and as mentioned above, it
> > seems likely that this could easily become a different one. The
> > clock-names are meant to identify an input from the consumer's
> > perspective. The correct names should be documented in the DT binding.
> 
> The first name was chosen as "apb_pclk" because it was recommended in
> Documentation. On the second name, I agree with you but again for TPIU DT
> entry, I am not sure how to make this clock configurable. So, that's why
> I put the same clock name as I had in my Vivado setup. If you have any
> leads on how to make it configurable, I will be happy to take a look
> into it.

Unfortunately, this is not how it works. The DT bindings are not a
recommendation. The DT description must follow the binding, otherwise
drivers will not work correctly, or best case, just ignore what you put
there.

> 
> >> +clock-frequency=<0xee6b280>;
> >>
> > I cannot find this property in the binding.
> >
> 
> This property was described in clock binding (in an example [2]) and the
> value here (250MHz) corresponds to the value I had chosen in Vivado for
> TPIU input clock.

As I don't see this in the coresight binding, I doubt that it has any
effect or should be here.

Sören

Re: [PATCH] Adding Support for Coresight Components on Zynq 7000.

[Sören Brinkmann]

Hi Muhammad,

On Fri, 2016-09-30 at 09:39:51 +0200, Muhammad Abdul WAHAB wrote:
> Hi Sören,
> 
> Thank you for your remarks. I corrected a few things as you suggested.
> 
[...]
> >> +tpiu@F8803000 {
> >>>
> >>> +compatible = "arm,coresight-tpiu", "arm,primecell";
> >>> +reg = <0xf8803000 0x1000>;
> >>> +clocks = < 47>, < 16>;
> >>
> >
> > I'm not sure this is correct for every setup. Sorry, that I don't recall
> > all the details, I haven't used tracing in a long time. But I guess this
> > clock is configurable as you're referring an fclk here. The other thing
> > that makes me a little suspicious is, that nothing in here uses the
> > 'dbg_trc' clock that the clock controller provides.
> 
> The TPIU setup included in my patch is specific to my configuration of TPIU.
> The second clock is configurable but I didn't know how to do so. As in
> Vivado setup I chose "fclk1" as the clock for TPIU, I decided to put fclk1.
> But, I am not sure about this. I am having some problems when I recover the
> trace from TPIU: it is not the same as in ETB even though it resembles a
> lot.
> I don't know if the problem is coming from the device tree or from the
> driver.
> I will have a look at 'dbg_trc' clock.

I tried to refresh my Zynq knowledge a bit. The clkc provides the
dbg_trc clock, and that is the clock you need (not fclk). I couldn't
find it in the binding (I guess I messed that up), but apparently,
you can provide a 'trace_emio_clk' as input to the clkc node in the
Zynq DT. Then, with the muxes correctly configured (FSBL should do
that if you select the EMIO trace clock in Vivado), the dbg_trc
output of the clkc should be that EMIO clock. And the dbg_trc output
of the clkc is what should be consumed by the tpiu node. Though, as
I see it the binding/driver for the TPIU do not support that.

I.e.
In the clkc description you'd have to add 'trace_emio_clk' to the
clock-names property together with a matching reference in the 'clocks'
property. As this change would be specific to local setups, this is not
really appropriate for upstream.

Then, for the trace clock, ideally the TPIU would consume and enable it
as needed.

> 
> >>
> >> +clock-names = "apb_pclk", "fclk1";
> >>
> > Those names (at least fclk1) is not a good name for tpiu to identify
> > it's input. fclk1 is a zynq-specific clock, and as mentioned above, it
> > seems likely that this could easily become a different one. The
> > clock-names are meant to identify an input from the consumer's
> > perspective. The correct names should be documented in the DT binding.
> 
> The first name was chosen as "apb_pclk" because it was recommended in
> Documentation. On the second name, I agree with you but again for TPIU DT
> entry, I am not sure how to make this clock configurable. So, that's why
> I put the same clock name as I had in my Vivado setup. If you have any
> leads on how to make it configurable, I will be happy to take a look
> into it.

Unfortunately, this is not how it works. The DT bindings are not a
recommendation. The DT description must follow the binding, otherwise
drivers will not work correctly, or best case, just ignore what you put
there.

> 
> >> +clock-frequency=<0xee6b280>;
> >>
> > I cannot find this property in the binding.
> >
> 
> This property was described in clock binding (in an example [2]) and the
> value here (250MHz) corresponds to the value I had chosen in Vivado for
> TPIU input clock.

As I don't see this in the coresight binding, I doubt that it has any
effect or should be here.

Sören

Re: [PATCH 2/2] mmc: sdhci-of-arasan: mark sdhci_arasan_reset() static

[Sören Brinkmann]

On Fri, 2016-09-30 at 09:37:39 +0800, Baoyou Xie wrote:
> We get 1 warning when building kernel with W=1:
> drivers/mmc/host/sdhci-of-arasan.c:253:6: warning: no previous prototype for 
> 'sdhci_arasan_reset' [-Wmissing-prototypes]
> 
> In fact, this function is only used in the file in which it is
> declared and don't need a declaration, but can be made static.
> So this patch marks it 'static'.
> 
> Signed-off-by: Baoyou Xie 

There is already a patch for this:
https://patchwork.kernel.org/patch/9349805/

Sören

Re: [PATCH 2/2] mmc: sdhci-of-arasan: mark sdhci_arasan_reset() static

[Sören Brinkmann]

On Fri, 2016-09-30 at 09:37:39 +0800, Baoyou Xie wrote:
> We get 1 warning when building kernel with W=1:
> drivers/mmc/host/sdhci-of-arasan.c:253:6: warning: no previous prototype for 
> 'sdhci_arasan_reset' [-Wmissing-prototypes]
> 
> In fact, this function is only used in the file in which it is
> declared and don't need a declaration, but can be made static.
> So this patch marks it 'static'.
> 
> Signed-off-by: Baoyou Xie 

There is already a patch for this:
https://patchwork.kernel.org/patch/9349805/

Sören

Re: [PATCH] Adding Support for Coresight Components on Zynq 7000.

[Sören Brinkmann]

Hi Muhammad,

On Thu, 2016-09-29 at 12:26:13 +0200, Muhammad Abdul WAHAB wrote:
> The Coresight components are present on the Zynq SoC but the corresponding
> device tree entries are missing. This patch adds device tree entries for
> coresight components while explaining how it was done in order to allow
> porting towards other boards easily.
> 
> By adding the entries for Coresight components in the device tree: if no
> files are created in sysfile system, you need to contact the board designer
> to sort out the problem. On some boards, Coresight components are not
> powered on boot.
> 
> Signed-off-by: Muhammad Abdul Wahab 
> ---
> The documentation file was very helpful
> (Documentation/devicetree/bindings/arm/coresight.txt). However, few details
> can be added to make it more clear for beginners.
> 
> Things to modify in device tree when changing the board are mainly:
> 
> - address
> - `clocks` field
> - some references in other entries may be missing (e.g. for `CPU` field in
>   ETM/PTM component, references need to be created)
> 
> Furthermore, the `reg` field should be adapted according to
> `#address-cells` and `#size-cells`. It may appear obvious, not for
> beginners.
> 
> ## Testing
> 
> The trace sink components need to be enabled by accessing through sysfile
> system.
> 
> echo 1 > /sys/bus/coresight/devices/@addr.etb/enable\_sink
> 
> Then enable the CS source component:
> 
> echo 1 > /sys/bus/coresight/devices/@addr.ptm/enable\_source
> 
> By default, CS Source components are configured to trace the kernel.
> 
> Then the trace can be read by dumping ETB.
> 
> dd if=/dev/@addr.etb of=trace_kernel.bin
> 
> The trace can be visualized by:
> 
> hexdump -C trace_kernel.bin
> 
> Or stored using:
> 
> hexdump -C trace_kernel.bin > trace_kernel.txt
> 
> The trace need to be decoded to be readable. All these above steps can now
> be performed with Perf Library which was not available at the time I was
> playing with DT entries.

I'm curious, did you test that with external debug tools. I have the
feeling the kernel using the debug HW could interfere with JTAG
debuggers, external trace tools, etc.

> 
> --- linux-4.7/arch/arm/boot/dts/zynq-7000.dtsi.orig 2016-07-24
> 21:23:50.0 +0200
> +++ linux-4.7/arch/arm/boot/dts/zynq-7000.dtsi2016-09-28
> 19:13:52.651881000 +0200
> @@ -363,5 +363,159 @@
>  reg = <0xf8005000 0x1000>;
>  timeout-sec = <10>;
>  };
> +
> +etb@F8801000 {
> +compatible = "arm,coresight-etb10", "arm,primecell";
> +reg = <0xf8801000 0x1000>;
> +coresight-default-sink;
> +clocks = < 47>;
> +clock-names = "apb_pclk";
> +
> +port {
> +
> +endpoint@0 {
> +slave-mode;
> +remote-endpoint = <0x8>;

Use labels please.

> +linux,phandle = <0xd>;
> +phandle = <0xd>;

Do these phandle properties need to be here?

> +};
> +};
> +};
> +
> +tpiu@F8803000 {
> +compatible = "arm,coresight-tpiu", "arm,primecell";
> +reg = <0xf8803000 0x1000>;
> +clocks = < 47>, < 16>;

I'm not sure this is correct for every setup. Sorry, that I don't recall
all the details, I haven't used tracing in a long time. But I guess this
clock is configurable as you're referring an fclk here. The other thing
that makes me a little suspicious is, that nothing in here uses the
'dbg_trc' clock that the clock controller provides.

> +clock-names = "apb_pclk", "fclk1";

Those names (at least fclk1) is not a good name for tpiu to identify
it's input. fclk1 is a zynq-specific clock, and as mentioned above, it
seems likely that this could easily become a different one. The
clock-names are meant to identify an input from the consumer's
perspective. The correct names should be documented in the DT binding.

> +clock-frequency=<0xee6b280>;

I cannot find this property in the binding.

> +
> +port {
> +
> +endpoint@0 {
> +slave-mode;
> +remote-endpoint = <0x9>;
> +linux,phandle = <0xe>;
> +phandle = <0xe>;
> +};
> +};
> +};
> +
> +funnel@F8804000 {
> +compatible = "arm,coresight-funnel", "arm,primecell";
> +reg = <0xf8804000 0x1000>;
> +clocks = < 47>;
> +clock-names = "apb_pclk";
> +
> +ports {
> +#address-cells = <0x1>;
> +#size-cells = <0x0>;
> +
> +port@0 {
> +reg = <0x0>;
> +
> +endpoint {
> +remote-endpoint = <0xa>;
> +linux,phandle = <0xf>;
> +phandle = <0xf>;
> +};
> 

Re: [PATCH] Adding Support for Coresight Components on Zynq 7000.

[Sören Brinkmann]

Hi Muhammad,

On Thu, 2016-09-29 at 12:26:13 +0200, Muhammad Abdul WAHAB wrote:
> The Coresight components are present on the Zynq SoC but the corresponding
> device tree entries are missing. This patch adds device tree entries for
> coresight components while explaining how it was done in order to allow
> porting towards other boards easily.
> 
> By adding the entries for Coresight components in the device tree: if no
> files are created in sysfile system, you need to contact the board designer
> to sort out the problem. On some boards, Coresight components are not
> powered on boot.
> 
> Signed-off-by: Muhammad Abdul Wahab 
> ---
> The documentation file was very helpful
> (Documentation/devicetree/bindings/arm/coresight.txt). However, few details
> can be added to make it more clear for beginners.
> 
> Things to modify in device tree when changing the board are mainly:
> 
> - address
> - `clocks` field
> - some references in other entries may be missing (e.g. for `CPU` field in
>   ETM/PTM component, references need to be created)
> 
> Furthermore, the `reg` field should be adapted according to
> `#address-cells` and `#size-cells`. It may appear obvious, not for
> beginners.
> 
> ## Testing
> 
> The trace sink components need to be enabled by accessing through sysfile
> system.
> 
> echo 1 > /sys/bus/coresight/devices/@addr.etb/enable\_sink
> 
> Then enable the CS source component:
> 
> echo 1 > /sys/bus/coresight/devices/@addr.ptm/enable\_source
> 
> By default, CS Source components are configured to trace the kernel.
> 
> Then the trace can be read by dumping ETB.
> 
> dd if=/dev/@addr.etb of=trace_kernel.bin
> 
> The trace can be visualized by:
> 
> hexdump -C trace_kernel.bin
> 
> Or stored using:
> 
> hexdump -C trace_kernel.bin > trace_kernel.txt
> 
> The trace need to be decoded to be readable. All these above steps can now
> be performed with Perf Library which was not available at the time I was
> playing with DT entries.

I'm curious, did you test that with external debug tools. I have the
feeling the kernel using the debug HW could interfere with JTAG
debuggers, external trace tools, etc.

> 
> --- linux-4.7/arch/arm/boot/dts/zynq-7000.dtsi.orig 2016-07-24
> 21:23:50.0 +0200
> +++ linux-4.7/arch/arm/boot/dts/zynq-7000.dtsi2016-09-28
> 19:13:52.651881000 +0200
> @@ -363,5 +363,159 @@
>  reg = <0xf8005000 0x1000>;
>  timeout-sec = <10>;
>  };
> +
> +etb@F8801000 {
> +compatible = "arm,coresight-etb10", "arm,primecell";
> +reg = <0xf8801000 0x1000>;
> +coresight-default-sink;
> +clocks = < 47>;
> +clock-names = "apb_pclk";
> +
> +port {
> +
> +endpoint@0 {
> +slave-mode;
> +remote-endpoint = <0x8>;

Use labels please.

> +linux,phandle = <0xd>;
> +phandle = <0xd>;

Do these phandle properties need to be here?

> +};
> +};
> +};
> +
> +tpiu@F8803000 {
> +compatible = "arm,coresight-tpiu", "arm,primecell";
> +reg = <0xf8803000 0x1000>;
> +clocks = < 47>, < 16>;

I'm not sure this is correct for every setup. Sorry, that I don't recall
all the details, I haven't used tracing in a long time. But I guess this
clock is configurable as you're referring an fclk here. The other thing
that makes me a little suspicious is, that nothing in here uses the
'dbg_trc' clock that the clock controller provides.

> +clock-names = "apb_pclk", "fclk1";

Those names (at least fclk1) is not a good name for tpiu to identify
it's input. fclk1 is a zynq-specific clock, and as mentioned above, it
seems likely that this could easily become a different one. The
clock-names are meant to identify an input from the consumer's
perspective. The correct names should be documented in the DT binding.

> +clock-frequency=<0xee6b280>;

I cannot find this property in the binding.

> +
> +port {
> +
> +endpoint@0 {
> +slave-mode;
> +remote-endpoint = <0x9>;
> +linux,phandle = <0xe>;
> +phandle = <0xe>;
> +};
> +};
> +};
> +
> +funnel@F8804000 {
> +compatible = "arm,coresight-funnel", "arm,primecell";
> +reg = <0xf8804000 0x1000>;
> +clocks = < 47>;
> +clock-names = "apb_pclk";
> +
> +ports {
> +#address-cells = <0x1>;
> +#size-cells = <0x0>;
> +
> +port@0 {
> +reg = <0x0>;
> +
> +endpoint {
> +remote-endpoint = <0xa>;
> +linux,phandle = <0xf>;
> +phandle = <0xf>;
> +};
> +};
> +
> +  

Re: [PATCH v2] arm: dts: zynq: Add MicroZed board support

[Sören Brinkmann]

On Thu, 2016-09-22 at 18:51:29 +0530, Jagan Teki wrote:
> From: Jagan Teki 
> 
> Added basic dts support for MicroZed board.
> 
> - UART
> - SDHCI
> - Ethernet
> 
> Cc: Soren Brinkmann 
> Cc: Michal Simek 
> Signed-off-by: Jagan Teki 
> ---
> Changes for v2:
>   - Add SDHCI
>   - Add Ethernet
> 
>  arch/arm/boot/dts/Makefile  |  1 +
>  arch/arm/boot/dts/zynq-microzed.dts | 95 
> +
>  2 files changed, 96 insertions(+)
>  create mode 100644 arch/arm/boot/dts/zynq-microzed.dts
> 
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index faacd52..4d7b858 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -862,6 +862,7 @@ dtb-$(CONFIG_ARCH_VT8500) += \
>   wm8750-apc8750.dtb \
>   wm8850-w70v2.dtb
>  dtb-$(CONFIG_ARCH_ZYNQ) += \
> + zynq-microzed.dtb \
>   zynq-parallella.dtb \
>   zynq-zc702.dtb \
>   zynq-zc706.dtb \
> diff --git a/arch/arm/boot/dts/zynq-microzed.dts 
> b/arch/arm/boot/dts/zynq-microzed.dts
> new file mode 100644
> index 000..9e64496
> --- /dev/null
> +++ b/arch/arm/boot/dts/zynq-microzed.dts
> @@ -0,0 +1,95 @@
> +/*
> + * Copyright (C) 2015 Jagan Teki 
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * 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.
> + */
> +/dts-v1/;
> +/include/ "zynq-7000.dtsi"
> +
> +/ {
> + model = "Zynq MicroZED Development Board";
> + compatible = "xlnx,zynq-microzed", "xlnx,zynq-7000";
> +
> + aliases {
> + ethernet0 = 
> + serial0 = 
> + };
> +
> + memory {
> + device_type = "memory";
> + reg = <0x0 0x4000>;
> + };
> +
> + chosen {
> + bootargs = "earlycon";
> + stdout-path = "serial0:115200n8";
> + };
> +
> + usb_phy0: phy0 {
> + compatible = "usb-nop-xceiv";
> + #phy-cells = <0>;
> + };
> +};
> +
> + {
> + ps-clk-frequency = <>;
> +};
> +
> + {
> + status = "okay";
> + phy-mode = "rgmii-id";
> + phy-handle = <_phy>;
> +
> + ethernet_phy: ethernet-phy@0 {
> + reg = <0>;
> + };
> +};
> +
> + {
> + status = "okay";
> +};
> +
> + {
> + status = "okay";
> +};
> +
> + {
> + status = "okay";
> + dr_mode = "host";
> + usb-phy = <_phy0>;
> + pinctrl-names = "default";
> + pinctrl-0 = <_usb0_default>;
> +};
> +
> + {
> + pinctrl_usb0_default: usb0-default {
> + mux {
> + groups = "usb0_0_grp";
> + function = "usb0";
> + };
> +
> + conf {
> + groups = "usb0_0_grp";
> + slew-rate = <0>;
> + io-standard = <1>;
> + };
> +
> + conf-rx {
> + pins = "MIO29", "MIO31", "MIO36";
> + bias-high-impedance;
> + };
> +
> + conf-tx {
> + pins = "MIO28", "MIO30", "MIO32", "MIO33", "MIO34",
> +"MIO35", "MIO37", "MIO38", "MIO39";
> + bias-disable;
> + };
> + };
> +};

I guess it's not strictly required, but shouldn't there be pinctrl
descriptions for all devices?

Sören

Re: [PATCH v2] arm: dts: zynq: Add MicroZed board support

[Sören Brinkmann]

On Thu, 2016-09-22 at 18:51:29 +0530, Jagan Teki wrote:
> From: Jagan Teki 
> 
> Added basic dts support for MicroZed board.
> 
> - UART
> - SDHCI
> - Ethernet
> 
> Cc: Soren Brinkmann 
> Cc: Michal Simek 
> Signed-off-by: Jagan Teki 
> ---
> Changes for v2:
>   - Add SDHCI
>   - Add Ethernet
> 
>  arch/arm/boot/dts/Makefile  |  1 +
>  arch/arm/boot/dts/zynq-microzed.dts | 95 
> +
>  2 files changed, 96 insertions(+)
>  create mode 100644 arch/arm/boot/dts/zynq-microzed.dts
> 
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index faacd52..4d7b858 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -862,6 +862,7 @@ dtb-$(CONFIG_ARCH_VT8500) += \
>   wm8750-apc8750.dtb \
>   wm8850-w70v2.dtb
>  dtb-$(CONFIG_ARCH_ZYNQ) += \
> + zynq-microzed.dtb \
>   zynq-parallella.dtb \
>   zynq-zc702.dtb \
>   zynq-zc706.dtb \
> diff --git a/arch/arm/boot/dts/zynq-microzed.dts 
> b/arch/arm/boot/dts/zynq-microzed.dts
> new file mode 100644
> index 000..9e64496
> --- /dev/null
> +++ b/arch/arm/boot/dts/zynq-microzed.dts
> @@ -0,0 +1,95 @@
> +/*
> + * Copyright (C) 2015 Jagan Teki 
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * 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.
> + */
> +/dts-v1/;
> +/include/ "zynq-7000.dtsi"
> +
> +/ {
> + model = "Zynq MicroZED Development Board";
> + compatible = "xlnx,zynq-microzed", "xlnx,zynq-7000";
> +
> + aliases {
> + ethernet0 = 
> + serial0 = 
> + };
> +
> + memory {
> + device_type = "memory";
> + reg = <0x0 0x4000>;
> + };
> +
> + chosen {
> + bootargs = "earlycon";
> + stdout-path = "serial0:115200n8";
> + };
> +
> + usb_phy0: phy0 {
> + compatible = "usb-nop-xceiv";
> + #phy-cells = <0>;
> + };
> +};
> +
> + {
> + ps-clk-frequency = <>;
> +};
> +
> + {
> + status = "okay";
> + phy-mode = "rgmii-id";
> + phy-handle = <_phy>;
> +
> + ethernet_phy: ethernet-phy@0 {
> + reg = <0>;
> + };
> +};
> +
> + {
> + status = "okay";
> +};
> +
> + {
> + status = "okay";
> +};
> +
> + {
> + status = "okay";
> + dr_mode = "host";
> + usb-phy = <_phy0>;
> + pinctrl-names = "default";
> + pinctrl-0 = <_usb0_default>;
> +};
> +
> + {
> + pinctrl_usb0_default: usb0-default {
> + mux {
> + groups = "usb0_0_grp";
> + function = "usb0";
> + };
> +
> + conf {
> + groups = "usb0_0_grp";
> + slew-rate = <0>;
> + io-standard = <1>;
> + };
> +
> + conf-rx {
> + pins = "MIO29", "MIO31", "MIO36";
> + bias-high-impedance;
> + };
> +
> + conf-tx {
> + pins = "MIO28", "MIO30", "MIO32", "MIO33", "MIO34",
> +"MIO35", "MIO37", "MIO38", "MIO39";
> + bias-disable;
> + };
> + };
> +};

I guess it's not strictly required, but shouldn't there be pinctrl
descriptions for all devices?

Sören

Re: [PATCH] gpio: Added zynq specific check for special pins on bank zero

[Sören Brinkmann]

On Tue, 2016-09-20 at 14:02:04 +0530, Nava kishore Manne wrote:
> From: Nava kishore Manne <nava.ma...@xilinx.com>
> 
> This patch adds zynq specific check for bank 0 pins 7 and 8
> are special and cannot be used as inputs
> 
> Signed-off-by: Nava kishore Manne <na...@xilinx.com>
> ---
>  drivers/gpio/gpio-zynq.c | 17 +++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
> index e72794e..eae9d24 100644
> --- a/drivers/gpio/gpio-zynq.c
> +++ b/drivers/gpio/gpio-zynq.c
> @@ -96,6 +96,10 @@
>  /* GPIO upper 16 bit mask */
>  #define ZYNQ_GPIO_UPPER_MASK 0x
>  
> +/* For GPIO quirks */
> +#define ZYNQ_GPIOBIT(0)
> +#define ZYNQMP_GPIO  BIT(1)

I'd make sure all quirks are easily identifiable and call them something
like 'ZYNQ_GPIO_QUIRK_FOO'

Apart from that:
Acked-by: Sören Brinkmann <soren.brinkm...@xilinx.com>

Sören

Re: [PATCH] gpio: Added zynq specific check for special pins on bank zero

[Sören Brinkmann]

On Tue, 2016-09-20 at 14:02:04 +0530, Nava kishore Manne wrote:
> From: Nava kishore Manne 
> 
> This patch adds zynq specific check for bank 0 pins 7 and 8
> are special and cannot be used as inputs
> 
> Signed-off-by: Nava kishore Manne 
> ---
>  drivers/gpio/gpio-zynq.c | 17 +++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
> index e72794e..eae9d24 100644
> --- a/drivers/gpio/gpio-zynq.c
> +++ b/drivers/gpio/gpio-zynq.c
> @@ -96,6 +96,10 @@
>  /* GPIO upper 16 bit mask */
>  #define ZYNQ_GPIO_UPPER_MASK 0x
>  
> +/* For GPIO quirks */
> +#define ZYNQ_GPIOBIT(0)
> +#define ZYNQMP_GPIO  BIT(1)

I'd make sure all quirks are easily identifiable and call them something
like 'ZYNQ_GPIO_QUIRK_FOO'

Apart from that:
Acked-by: Sören Brinkmann 

Sören

Re: [PATCH v5] Axi-usb: Add support for 64-bit addressing.

[Sören Brinkmann]

On Thu, 2016-09-01 at 14:22:56 +0530, Nava kishore Manne wrote:
> This patch updates the driver to support 64-bit DMA addressing.
> 
> Signed-off-by: Nava kishore Manne 
> Acked-by: Rob Herring 
> ---
> Changes for v5:
> -None.
> 
> Changes for v4:
> -Used boolen property insted of addrwith property in the DT
>  as suggested by Arnd Bergmann.
> -Adopt the DT relevant changes into the driver.
> 
> Changes for v3:
> -Added new compatable string for 5.00 IP version as suggested 
> by
>  Arnd Bergmann.
> -Used write_fn() insted of lo_hi_writeq() as suggested by
>  Arnd Bergmann.
> Changes for v2:
> -Added dma-ranges property in device tree as suggested by
>  Arnd Bergmann.
> -Modified the driver code based on the xlnx,addrwidth.
> 
>  .../devicetree/bindings/usb/udc-xilinx.txt |  5 ++-
>  drivers/usb/gadget/udc/udc-xilinx.c| 52 
> +-
>  2 files changed, 54 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/udc-xilinx.txt 
> b/Documentation/devicetree/bindings/usb/udc-xilinx.txt
> index 47b4e39..d08d972 100644
> --- a/Documentation/devicetree/bindings/usb/udc-xilinx.txt
> +++ b/Documentation/devicetree/bindings/usb/udc-xilinx.txt
> @@ -1,12 +1,14 @@
>  Xilinx USB2 device controller
>  
>  Required properties:
> -- compatible : Should be "xlnx,usb2-device-4.00.a"
> +- compatible : Should be "xlnx,usb2-device-4.00.a" or
> +   "xlnx,usb2-device-5.00"
>  - reg: Physical base address and size of the USB2
> device registers map.
>  - interrupts : Should contain single irq line of USB2 device
> controller
>  - xlnx,has-builtin-dma   : if DMA is included
> +- xlnx,has-64bit-dma : if DMA is included 64-bit addressing support.

We add these properties to describe the IP configuration + we have the
version indicator in the compatible string. Do we really need both? It
seems that all versions use the same driver and differentiation is made
through the DT properties that describe the IP configuration parameters.
Wouldn't it be better to just have one generic 'xlnx,axi-usb2' (or
whatever) compatible string instead of patching the driver for each new
version without really needing to differentiate the IP versions through
it?

Sören

Re: [PATCH v5] Axi-usb: Add support for 64-bit addressing.

[Sören Brinkmann]

On Thu, 2016-09-01 at 14:22:56 +0530, Nava kishore Manne wrote:
> This patch updates the driver to support 64-bit DMA addressing.
> 
> Signed-off-by: Nava kishore Manne 
> Acked-by: Rob Herring 
> ---
> Changes for v5:
> -None.
> 
> Changes for v4:
> -Used boolen property insted of addrwith property in the DT
>  as suggested by Arnd Bergmann.
> -Adopt the DT relevant changes into the driver.
> 
> Changes for v3:
> -Added new compatable string for 5.00 IP version as suggested 
> by
>  Arnd Bergmann.
> -Used write_fn() insted of lo_hi_writeq() as suggested by
>  Arnd Bergmann.
> Changes for v2:
> -Added dma-ranges property in device tree as suggested by
>  Arnd Bergmann.
> -Modified the driver code based on the xlnx,addrwidth.
> 
>  .../devicetree/bindings/usb/udc-xilinx.txt |  5 ++-
>  drivers/usb/gadget/udc/udc-xilinx.c| 52 
> +-
>  2 files changed, 54 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/udc-xilinx.txt 
> b/Documentation/devicetree/bindings/usb/udc-xilinx.txt
> index 47b4e39..d08d972 100644
> --- a/Documentation/devicetree/bindings/usb/udc-xilinx.txt
> +++ b/Documentation/devicetree/bindings/usb/udc-xilinx.txt
> @@ -1,12 +1,14 @@
>  Xilinx USB2 device controller
>  
>  Required properties:
> -- compatible : Should be "xlnx,usb2-device-4.00.a"
> +- compatible : Should be "xlnx,usb2-device-4.00.a" or
> +   "xlnx,usb2-device-5.00"
>  - reg: Physical base address and size of the USB2
> device registers map.
>  - interrupts : Should contain single irq line of USB2 device
> controller
>  - xlnx,has-builtin-dma   : if DMA is included
> +- xlnx,has-64bit-dma : if DMA is included 64-bit addressing support.

We add these properties to describe the IP configuration + we have the
version indicator in the compatible string. Do we really need both? It
seems that all versions use the same driver and differentiation is made
through the DT properties that describe the IP configuration parameters.
Wouldn't it be better to just have one generic 'xlnx,axi-usb2' (or
whatever) compatible string instead of patching the driver for each new
version without really needing to differentiate the IP versions through
it?

Sören

Re: [PATCH 1/2] sdhci-of-arasan: Add quirk and device tree parameter to fake CD bit

[Sören Brinkmann]

On Thu, 2016-08-25 at 17:23:47 +0200, Lars-Peter Clausen wrote:
> On 08/25/2016 05:10 PM, Sören Brinkmann wrote:
> > On Wed, 2016-08-24 at 18:23:03 -0500, Zach Brown wrote:
> >> The sdhci controller on xilinx zynq devices will not function unless
> >> the cd bit is provided. http://www.xilinx.com/support/answers/61064.html
> >> In cases where it is impossible to provide the cd bit in hardware,
> >> setting the controller to test mode and then setting inserted to true
> >> will get the controller to function with out the cd bit.
> >>
> >> The device property "fake-cd" will let the arasan driver know it needs
> >> to fake the cd bit for the controller inorder for the controller to
> >> function with a SD card that does not provide the CD bit.
> > 
> > I thought the CD is, if not pinned out, tied off to some valid logic
> > level. Isn't it enough to specify cd-inverted if needed to make it work
> > in those cases?
> 
> It is always brought out to some pin, that is the problem on the Zynq. This
> means you'd have to set at least one pin aside as dummy CD or WP pin. Which
> is not always possible when you are tight on available pins.

I have to admit that I haven't looked at Vivado for quite a while. Is it
possible to select EMIO for those pins? If those are not routed anything
they should be tied to some logic level, I believe.
If they are always forced to be on a physical pin, do you let that pin
just float? Otherwise, the logic level should also be defined, give and
take a logic inversion.

Sören

Re: [PATCH 1/2] sdhci-of-arasan: Add quirk and device tree parameter to fake CD bit

[Sören Brinkmann]

On Thu, 2016-08-25 at 17:23:47 +0200, Lars-Peter Clausen wrote:
> On 08/25/2016 05:10 PM, Sören Brinkmann wrote:
> > On Wed, 2016-08-24 at 18:23:03 -0500, Zach Brown wrote:
> >> The sdhci controller on xilinx zynq devices will not function unless
> >> the cd bit is provided. http://www.xilinx.com/support/answers/61064.html
> >> In cases where it is impossible to provide the cd bit in hardware,
> >> setting the controller to test mode and then setting inserted to true
> >> will get the controller to function with out the cd bit.
> >>
> >> The device property "fake-cd" will let the arasan driver know it needs
> >> to fake the cd bit for the controller inorder for the controller to
> >> function with a SD card that does not provide the CD bit.
> > 
> > I thought the CD is, if not pinned out, tied off to some valid logic
> > level. Isn't it enough to specify cd-inverted if needed to make it work
> > in those cases?
> 
> It is always brought out to some pin, that is the problem on the Zynq. This
> means you'd have to set at least one pin aside as dummy CD or WP pin. Which
> is not always possible when you are tight on available pins.

I have to admit that I haven't looked at Vivado for quite a while. Is it
possible to select EMIO for those pins? If those are not routed anything
they should be tied to some logic level, I believe.
If they are always forced to be on a physical pin, do you let that pin
just float? Otherwise, the logic level should also be defined, give and
take a logic inversion.

Sören

Re: [PATCH 1/2] sdhci-of-arasan: Add quirk and device tree parameter to fake CD bit

[Sören Brinkmann]

On Wed, 2016-08-24 at 18:23:03 -0500, Zach Brown wrote:
> The sdhci controller on xilinx zynq devices will not function unless
> the cd bit is provided. http://www.xilinx.com/support/answers/61064.html
> In cases where it is impossible to provide the cd bit in hardware,
> setting the controller to test mode and then setting inserted to true
> will get the controller to function with out the cd bit.
> 
> The device property "fake-cd" will let the arasan driver know it needs
> to fake the cd bit for the controller inorder for the controller to
> function with a SD card that does not provide the CD bit.

I thought the CD is, if not pinned out, tied off to some valid logic
level. Isn't it enough to specify cd-inverted if needed to make it work
in those cases?

Sören

Re: [PATCH 1/2] sdhci-of-arasan: Add quirk and device tree parameter to fake CD bit

[Sören Brinkmann]

On Wed, 2016-08-24 at 18:23:03 -0500, Zach Brown wrote:
> The sdhci controller on xilinx zynq devices will not function unless
> the cd bit is provided. http://www.xilinx.com/support/answers/61064.html
> In cases where it is impossible to provide the cd bit in hardware,
> setting the controller to test mode and then setting inserted to true
> will get the controller to function with out the cd bit.
> 
> The device property "fake-cd" will let the arasan driver know it needs
> to fake the cd bit for the controller inorder for the controller to
> function with a SD card that does not provide the CD bit.

I thought the CD is, if not pinned out, tied off to some valid logic
level. Isn't it enough to specify cd-inverted if needed to make it work
in those cases?

Sören

Re: SOC-specific action for irq_set_wake

[Sören Brinkmann]

On Wed, 2016-07-20 at 14:36:12 +0100, Marc Zyngier wrote:
> Hi Sören,
> 
> On 20/07/16 14:16, Sören Brinkmann wrote:
> > Hi Marc,
> 
> >>>>> Does anybody have similar problems and probably already solved it?
> >>>>> Any other suggestions for approaching the problem? Any preferred
> >>>>> solution?
> >>>>
> >>>> I think we have the same problem.  Can you provide more detail on the 
> >>>> hardware
> >>>> implementation of your wake irq controller?  I presume you have some set 
> >>>> of
> >>>> registers, an irq maybe, and some other stuff?  And how does it fit into 
> >>>> the
> >>>> overall architecture from a hardware perspective?
> >>>
> >>> We have essentially a whole second interrupt controller. All IRQs are
> >>> connected to the A53 GIC and this second interrupt controller that is
> >>> controlled by the companion core. The companion core is supposed to be
> >>> informed about what source the A53 needs to wake up on and will program
> >>> this second IRQ controller, etc.
> >>
> >> So your "special case" is exactly like everyone else's. Implement it as
> >> a hierarchical chip on top of the GIC, just like Tegra, OMAP, iMX6,
> >> Exynos and a few others. Unless you implement PSCI.
> > 
> > I didn't really think that our case is unique. I was just looking for
> > some pointers into the right direction as the extension mechanism that I
> > remembered disappeared and I haven't been following the development
> > closely enough to just know what alternatives are available.
> > So, you say the approach of letting the secure monitor infer the wake
> > IRQ by reading the GIC config is preferred over handling it as
> > hierarchical chip within Linux?
> 
> The in-kernel approach is a consequence of the firmware-less 32bit
> configuration. Hopefully, we won't see anything like that anymore.
> Fingers crossed.
> 
> So the firmware approach is clearly the preferred one on arm64, as it
> simplifies absolutely everything (and your power management has to know
> about all of this anyway).

Thanks for your input. We'll do it this way then. Thanks, everybody.

Sören

Re: SOC-specific action for irq_set_wake

[Sören Brinkmann]

On Wed, 2016-07-20 at 14:36:12 +0100, Marc Zyngier wrote:
> Hi Sören,
> 
> On 20/07/16 14:16, Sören Brinkmann wrote:
> > Hi Marc,
> 
> >>>>> Does anybody have similar problems and probably already solved it?
> >>>>> Any other suggestions for approaching the problem? Any preferred
> >>>>> solution?
> >>>>
> >>>> I think we have the same problem.  Can you provide more detail on the 
> >>>> hardware
> >>>> implementation of your wake irq controller?  I presume you have some set 
> >>>> of
> >>>> registers, an irq maybe, and some other stuff?  And how does it fit into 
> >>>> the
> >>>> overall architecture from a hardware perspective?
> >>>
> >>> We have essentially a whole second interrupt controller. All IRQs are
> >>> connected to the A53 GIC and this second interrupt controller that is
> >>> controlled by the companion core. The companion core is supposed to be
> >>> informed about what source the A53 needs to wake up on and will program
> >>> this second IRQ controller, etc.
> >>
> >> So your "special case" is exactly like everyone else's. Implement it as
> >> a hierarchical chip on top of the GIC, just like Tegra, OMAP, iMX6,
> >> Exynos and a few others. Unless you implement PSCI.
> > 
> > I didn't really think that our case is unique. I was just looking for
> > some pointers into the right direction as the extension mechanism that I
> > remembered disappeared and I haven't been following the development
> > closely enough to just know what alternatives are available.
> > So, you say the approach of letting the secure monitor infer the wake
> > IRQ by reading the GIC config is preferred over handling it as
> > hierarchical chip within Linux?
> 
> The in-kernel approach is a consequence of the firmware-less 32bit
> configuration. Hopefully, we won't see anything like that anymore.
> Fingers crossed.
> 
> So the firmware approach is clearly the preferred one on arm64, as it
> simplifies absolutely everything (and your power management has to know
> about all of this anyway).

Thanks for your input. We'll do it this way then. Thanks, everybody.

Sören

Re: SOC-specific action for irq_set_wake

[Sören Brinkmann]

Hi Thomas,

On Wed, 2016-07-20 at 08:28:35 +0200, Thomas Gleixner wrote:
> On Tue, 19 Jul 2016, Sören Brinkmann wrote:
> > we are working on the PM solution for Zynq MPSOC and ran into some
> > problem when setting the wake source.
> > 
> > The situation is that when the A53 is in suspend, the GIC(v2) may be
> > powered down. In that state a companion core is handling wake
> > events/IRQs, but we expect the OS/Linux to notify the companion core
> > about what device/IRQ is a wake up source. Hence, my idea was to capture
> > enabling/disabling wake IRQs in our platform PM code and then
> > communicate with the FW as needed during suspend operations. The problem
> > is: I don't see a good way to notify the platform code about these
> > events.
> > 
> > My ideas were:
> > 1. Use the irq_chip irq_set_wake function
> >   My thought was to implement the irq_set_wake function in a
> >   SOC-specific way (could even be generic and call some notifier chain or
> 
> Don't even think about notifier chains.
> 
> >   similar) to notify the platform PM code when a device/IRQ is
> >   enabled/disabled as wake up source.
> >   My problem is that the SKIP_IRQ_SET_WAKE flag is set in the generic
> >   driver (drivers/irqchip/irq-gic.c) and platforms cannot implement
> >   irq_set_wake without changes in the common code.
> 
> So and because it requires changes in the common code you think aboiut
> notifiers and other absurdities. Come on, common code is not a sacred cow. It
> can be modified and if you need for your particular platform that
> SKIP_IRQ_SET_WAKE is cleared, then there are a gazillion of sane ways to do
> that.

I'm not afraid of changing it, but I was hoping to get an idea of what
an acceptable solution would look like, as I don't want to run of into
any of the directions that are not sane and have it shoot down in the
end (like my notifier approach apparently would). I remembered the mentioned
extensions mechanism which disappeared. Hence, I thought it might be a
better idea to check beforehand why and what happened and what
alternative approach may be an acceptable solution for that problem.

Thanks,
Sören

Re: SOC-specific action for irq_set_wake

[Sören Brinkmann]

Hi Thomas,

On Wed, 2016-07-20 at 08:28:35 +0200, Thomas Gleixner wrote:
> On Tue, 19 Jul 2016, Sören Brinkmann wrote:
> > we are working on the PM solution for Zynq MPSOC and ran into some
> > problem when setting the wake source.
> > 
> > The situation is that when the A53 is in suspend, the GIC(v2) may be
> > powered down. In that state a companion core is handling wake
> > events/IRQs, but we expect the OS/Linux to notify the companion core
> > about what device/IRQ is a wake up source. Hence, my idea was to capture
> > enabling/disabling wake IRQs in our platform PM code and then
> > communicate with the FW as needed during suspend operations. The problem
> > is: I don't see a good way to notify the platform code about these
> > events.
> > 
> > My ideas were:
> > 1. Use the irq_chip irq_set_wake function
> >   My thought was to implement the irq_set_wake function in a
> >   SOC-specific way (could even be generic and call some notifier chain or
> 
> Don't even think about notifier chains.
> 
> >   similar) to notify the platform PM code when a device/IRQ is
> >   enabled/disabled as wake up source.
> >   My problem is that the SKIP_IRQ_SET_WAKE flag is set in the generic
> >   driver (drivers/irqchip/irq-gic.c) and platforms cannot implement
> >   irq_set_wake without changes in the common code.
> 
> So and because it requires changes in the common code you think aboiut
> notifiers and other absurdities. Come on, common code is not a sacred cow. It
> can be modified and if you need for your particular platform that
> SKIP_IRQ_SET_WAKE is cleared, then there are a gazillion of sane ways to do
> that.

I'm not afraid of changing it, but I was hoping to get an idea of what
an acceptable solution would look like, as I don't want to run of into
any of the directions that are not sane and have it shoot down in the
end (like my notifier approach apparently would). I remembered the mentioned
extensions mechanism which disappeared. Hence, I thought it might be a
better idea to check beforehand why and what happened and what
alternative approach may be an acceptable solution for that problem.

Thanks,
Sören

Re: SOC-specific action for irq_set_wake

[Sören Brinkmann]

Hi Marc,

On Wed, 2016-07-20 at 09:17:16 +0100, Marc Zyngier wrote:
> On 20/07/16 00:34, Sören Brinkmann wrote:
> > Hi Andy,
> > 
> > On Tue, 2016-07-19 at 17:47:13 -0500, Andy Gross wrote:
> >> On Tue, Jul 19, 2016 at 11:18:04AM -0700, Sören Brinkmann wrote:
> >>> Hi,
> >>>
> >>> we are working on the PM solution for Zynq MPSOC and ran into some
> >>> problem when setting the wake source.
> >>>
> >>> The situation is that when the A53 is in suspend, the GIC(v2) may be
> >>> powered down. In that state a companion core is handling wake
> >>> events/IRQs, but we expect the OS/Linux to notify the companion core
> >>> about what device/IRQ is a wake up source. Hence, my idea was to capture
> >>> enabling/disabling wake IRQs in our platform PM code and then
> >>> communicate with the FW as needed during suspend operations. The problem
> >>> is: I don't see a good way to notify the platform code about these
> >>> events.
> >>
> >> On Qualcomm platforms there is something similar.  When the system is power
> >> collapsed, the GIC loses context and can no longer handle/generate IRQs.  
> >> There
> >> is a separate block called the MPM that provides some registers that the 
> >> user
> >> can program.  These registers allow for up to 64 wake IRQs, each of which 
> >> can
> >> map to either a GPIO pin or an GIC IRQ.  The MPM also has a timer that can
> >> generate a wake IRQ so that the system can wake up and maintain the time.
> > 
> > We have a full companion core running firmware. As in your case,
> > we can use any IRQ source and some additional IOs as wake event. So,
> > it's fundamentally very similar. Main difference is probably the
> > interface. In our case, we have some platform-specific FW interfaces
> > that we need to call. Basically, Linux doing an SMC call and then the
> > secure monitor handling the rest. And for that we may also have to do
> > some additional mapping of the IRQ number to something the FW interface
> > understands.
> > 
> >>
> >>>
> >>> My ideas were:
> >>> 1. Use the irq_chip irq_set_wake function
> >>>   My thought was to implement the irq_set_wake function in a
> >>>   SOC-specific way (could even be generic and call some notifier chain or
> >>>   similar) to notify the platform PM code when a device/IRQ is
> >>>   enabled/disabled as wake up source.
> >>>   My problem is that the SKIP_IRQ_SET_WAKE flag is set in the generic
> >>>   driver (drivers/irqchip/irq-gic.c) and platforms cannot implement
> >>>   irq_set_wake without changes in the common code.
> >>
> >> So not too long ago there were irq extensions that you could add in to an 
> >> irq
> >> controller that would allow for intercepting the set wake calls.  This 
> >> went away
> >> and was replaced by the hierarchical irq design.
> > 
> > Yeah, I though I remembered that and wanted to use it now and was slightly
> > disappointed when I couldn't find the code anymore.
> 
> It was slaughtered for a good reason: It was unmaintainable, had
> horrible performance issues, wasn't described in any of DT or ACPI, and
> didn't fit any of the core code idioms. It has been replaced by
> something we that is supported in the code code and that is working for
> all current users of the GIC that have similar requirements to yours.
> 
> >> The way I am currently implementing the MPM for Qualcomm, I am defining 
> >> the MPM
> >> as a IRQ controller and creating relationships between the MPM and the GPIO
> >> controller (source of irqs) and the GIC.  As irqs are set wake and cleared 
> >> the
> >> calls go up through the hierarchy and they call each irqchips functions 
> >> (if you
> >> have it setup to do that).
> > 
> > I suspected that some sort of dummy interrupt controller might be one
> > way to do it. I was hoping to find a simpler way, but maybe this is the
> > way to go. One concern could be that this might require adding something
> > into the DT that doesn't really exist.
> 
> It is not "dummy". It drives an actual piece of HW (the wake up
> interrupts) and a piece of ware (the secure monitor side). And since
> when the DT has become something that we cannot change? Last time I
> checked, people were still adding 20 new bindings per merge window.

I'm not afraid of changing the DT. But the described hierarchy etc.
would not reflect the ac

Re: SOC-specific action for irq_set_wake

[Sören Brinkmann]

Hi Marc,

On Wed, 2016-07-20 at 09:17:16 +0100, Marc Zyngier wrote:
> On 20/07/16 00:34, Sören Brinkmann wrote:
> > Hi Andy,
> > 
> > On Tue, 2016-07-19 at 17:47:13 -0500, Andy Gross wrote:
> >> On Tue, Jul 19, 2016 at 11:18:04AM -0700, Sören Brinkmann wrote:
> >>> Hi,
> >>>
> >>> we are working on the PM solution for Zynq MPSOC and ran into some
> >>> problem when setting the wake source.
> >>>
> >>> The situation is that when the A53 is in suspend, the GIC(v2) may be
> >>> powered down. In that state a companion core is handling wake
> >>> events/IRQs, but we expect the OS/Linux to notify the companion core
> >>> about what device/IRQ is a wake up source. Hence, my idea was to capture
> >>> enabling/disabling wake IRQs in our platform PM code and then
> >>> communicate with the FW as needed during suspend operations. The problem
> >>> is: I don't see a good way to notify the platform code about these
> >>> events.
> >>
> >> On Qualcomm platforms there is something similar.  When the system is power
> >> collapsed, the GIC loses context and can no longer handle/generate IRQs.  
> >> There
> >> is a separate block called the MPM that provides some registers that the 
> >> user
> >> can program.  These registers allow for up to 64 wake IRQs, each of which 
> >> can
> >> map to either a GPIO pin or an GIC IRQ.  The MPM also has a timer that can
> >> generate a wake IRQ so that the system can wake up and maintain the time.
> > 
> > We have a full companion core running firmware. As in your case,
> > we can use any IRQ source and some additional IOs as wake event. So,
> > it's fundamentally very similar. Main difference is probably the
> > interface. In our case, we have some platform-specific FW interfaces
> > that we need to call. Basically, Linux doing an SMC call and then the
> > secure monitor handling the rest. And for that we may also have to do
> > some additional mapping of the IRQ number to something the FW interface
> > understands.
> > 
> >>
> >>>
> >>> My ideas were:
> >>> 1. Use the irq_chip irq_set_wake function
> >>>   My thought was to implement the irq_set_wake function in a
> >>>   SOC-specific way (could even be generic and call some notifier chain or
> >>>   similar) to notify the platform PM code when a device/IRQ is
> >>>   enabled/disabled as wake up source.
> >>>   My problem is that the SKIP_IRQ_SET_WAKE flag is set in the generic
> >>>   driver (drivers/irqchip/irq-gic.c) and platforms cannot implement
> >>>   irq_set_wake without changes in the common code.
> >>
> >> So not too long ago there were irq extensions that you could add in to an 
> >> irq
> >> controller that would allow for intercepting the set wake calls.  This 
> >> went away
> >> and was replaced by the hierarchical irq design.
> > 
> > Yeah, I though I remembered that and wanted to use it now and was slightly
> > disappointed when I couldn't find the code anymore.
> 
> It was slaughtered for a good reason: It was unmaintainable, had
> horrible performance issues, wasn't described in any of DT or ACPI, and
> didn't fit any of the core code idioms. It has been replaced by
> something we that is supported in the code code and that is working for
> all current users of the GIC that have similar requirements to yours.
> 
> >> The way I am currently implementing the MPM for Qualcomm, I am defining 
> >> the MPM
> >> as a IRQ controller and creating relationships between the MPM and the GPIO
> >> controller (source of irqs) and the GIC.  As irqs are set wake and cleared 
> >> the
> >> calls go up through the hierarchy and they call each irqchips functions 
> >> (if you
> >> have it setup to do that).
> > 
> > I suspected that some sort of dummy interrupt controller might be one
> > way to do it. I was hoping to find a simpler way, but maybe this is the
> > way to go. One concern could be that this might require adding something
> > into the DT that doesn't really exist.
> 
> It is not "dummy". It drives an actual piece of HW (the wake up
> interrupts) and a piece of ware (the secure monitor side). And since
> when the DT has become something that we cannot change? Last time I
> checked, people were still adding 20 new bindings per merge window.

I'm not afraid of changing the DT. But the described hierarchy etc.
would not reflect the ac

Re: SOC-specific action for irq_set_wake

[Sören Brinkmann]

Hi Andy,

On Tue, 2016-07-19 at 17:47:13 -0500, Andy Gross wrote:
> On Tue, Jul 19, 2016 at 11:18:04AM -0700, Sören Brinkmann wrote:
> > Hi,
> > 
> > we are working on the PM solution for Zynq MPSOC and ran into some
> > problem when setting the wake source.
> > 
> > The situation is that when the A53 is in suspend, the GIC(v2) may be
> > powered down. In that state a companion core is handling wake
> > events/IRQs, but we expect the OS/Linux to notify the companion core
> > about what device/IRQ is a wake up source. Hence, my idea was to capture
> > enabling/disabling wake IRQs in our platform PM code and then
> > communicate with the FW as needed during suspend operations. The problem
> > is: I don't see a good way to notify the platform code about these
> > events.
> 
> On Qualcomm platforms there is something similar.  When the system is power
> collapsed, the GIC loses context and can no longer handle/generate IRQs.  
> There
> is a separate block called the MPM that provides some registers that the user
> can program.  These registers allow for up to 64 wake IRQs, each of which can
> map to either a GPIO pin or an GIC IRQ.  The MPM also has a timer that can
> generate a wake IRQ so that the system can wake up and maintain the time.

We have a full companion core running firmware. As in your case,
we can use any IRQ source and some additional IOs as wake event. So,
it's fundamentally very similar. Main difference is probably the
interface. In our case, we have some platform-specific FW interfaces
that we need to call. Basically, Linux doing an SMC call and then the
secure monitor handling the rest. And for that we may also have to do
some additional mapping of the IRQ number to something the FW interface
understands.

> 
> > 
> > My ideas were:
> > 1. Use the irq_chip irq_set_wake function
> >   My thought was to implement the irq_set_wake function in a
> >   SOC-specific way (could even be generic and call some notifier chain or
> >   similar) to notify the platform PM code when a device/IRQ is
> >   enabled/disabled as wake up source.
> >   My problem is that the SKIP_IRQ_SET_WAKE flag is set in the generic
> >   driver (drivers/irqchip/irq-gic.c) and platforms cannot implement
> >   irq_set_wake without changes in the common code.
> 
> So not too long ago there were irq extensions that you could add in to an irq
> controller that would allow for intercepting the set wake calls.  This went 
> away
> and was replaced by the hierarchical irq design.

Yeah, I though I remembered that and wanted to use it now and was slightly
disappointed when I couldn't find the code anymore.

> 
> The way I am currently implementing the MPM for Qualcomm, I am defining the 
> MPM
> as a IRQ controller and creating relationships between the MPM and the GPIO
> controller (source of irqs) and the GIC.  As irqs are set wake and cleared the
> calls go up through the hierarchy and they call each irqchips functions (if 
> you
> have it setup to do that).

I suspected that some sort of dummy interrupt controller might be one
way to do it. I was hoping to find a simpler way, but maybe this is the
way to go. One concern could be that this might require adding something
into the DT that doesn't really exist.

> 
> 
> > 2. Stuff things into the secure monitor
> >   I guess we could read the GIC registers once we enter the secure
> >   monitor and do the communication with the companion core from there by
> >   identifying unmasked IRQs as wake IRQs. My concern here is that it
> >   might introduce additional hardcoded mappings that are much more
> >   dynamic in Linux thanks to the DT description.
> 
> This could work, but you'd have to have control of the secure monitor code.  
> In
> Qualcomm's case, that wouldn't work or couldn't because they won't change that
> code.  And you'd have to have calls into the monitor to turn on/off the lines.

We fully control our secure monitor. That should not be an issue for us.
What do you mean we'd need to call into the monitor? Linux masks all wake
IRQs when going into suspend. Everything unmasked should be a wake IRQ,
or am I missing something? I thought we could simply read the
enable/disable status of all IRQs in the secure monitor suspend path and
do the needed communication with the companion core.

> 
> > 
> > Does anybody have similar problems and probably already solved it?
> > Any other suggestions for approaching the problem? Any preferred
> > solution?
> 
> I think we have the same problem.  Can you provide more detail on the hardware
> implementation of your wake irq controller?  I presume you have some set of
> registers, an irq maybe, and some other stuff?  And how does it 

Re: SOC-specific action for irq_set_wake

[Sören Brinkmann]

Hi Andy,

On Tue, 2016-07-19 at 17:47:13 -0500, Andy Gross wrote:
> On Tue, Jul 19, 2016 at 11:18:04AM -0700, Sören Brinkmann wrote:
> > Hi,
> > 
> > we are working on the PM solution for Zynq MPSOC and ran into some
> > problem when setting the wake source.
> > 
> > The situation is that when the A53 is in suspend, the GIC(v2) may be
> > powered down. In that state a companion core is handling wake
> > events/IRQs, but we expect the OS/Linux to notify the companion core
> > about what device/IRQ is a wake up source. Hence, my idea was to capture
> > enabling/disabling wake IRQs in our platform PM code and then
> > communicate with the FW as needed during suspend operations. The problem
> > is: I don't see a good way to notify the platform code about these
> > events.
> 
> On Qualcomm platforms there is something similar.  When the system is power
> collapsed, the GIC loses context and can no longer handle/generate IRQs.  
> There
> is a separate block called the MPM that provides some registers that the user
> can program.  These registers allow for up to 64 wake IRQs, each of which can
> map to either a GPIO pin or an GIC IRQ.  The MPM also has a timer that can
> generate a wake IRQ so that the system can wake up and maintain the time.

We have a full companion core running firmware. As in your case,
we can use any IRQ source and some additional IOs as wake event. So,
it's fundamentally very similar. Main difference is probably the
interface. In our case, we have some platform-specific FW interfaces
that we need to call. Basically, Linux doing an SMC call and then the
secure monitor handling the rest. And for that we may also have to do
some additional mapping of the IRQ number to something the FW interface
understands.

> 
> > 
> > My ideas were:
> > 1. Use the irq_chip irq_set_wake function
> >   My thought was to implement the irq_set_wake function in a
> >   SOC-specific way (could even be generic and call some notifier chain or
> >   similar) to notify the platform PM code when a device/IRQ is
> >   enabled/disabled as wake up source.
> >   My problem is that the SKIP_IRQ_SET_WAKE flag is set in the generic
> >   driver (drivers/irqchip/irq-gic.c) and platforms cannot implement
> >   irq_set_wake without changes in the common code.
> 
> So not too long ago there were irq extensions that you could add in to an irq
> controller that would allow for intercepting the set wake calls.  This went 
> away
> and was replaced by the hierarchical irq design.

Yeah, I though I remembered that and wanted to use it now and was slightly
disappointed when I couldn't find the code anymore.

> 
> The way I am currently implementing the MPM for Qualcomm, I am defining the 
> MPM
> as a IRQ controller and creating relationships between the MPM and the GPIO
> controller (source of irqs) and the GIC.  As irqs are set wake and cleared the
> calls go up through the hierarchy and they call each irqchips functions (if 
> you
> have it setup to do that).

I suspected that some sort of dummy interrupt controller might be one
way to do it. I was hoping to find a simpler way, but maybe this is the
way to go. One concern could be that this might require adding something
into the DT that doesn't really exist.

> 
> 
> > 2. Stuff things into the secure monitor
> >   I guess we could read the GIC registers once we enter the secure
> >   monitor and do the communication with the companion core from there by
> >   identifying unmasked IRQs as wake IRQs. My concern here is that it
> >   might introduce additional hardcoded mappings that are much more
> >   dynamic in Linux thanks to the DT description.
> 
> This could work, but you'd have to have control of the secure monitor code.  
> In
> Qualcomm's case, that wouldn't work or couldn't because they won't change that
> code.  And you'd have to have calls into the monitor to turn on/off the lines.

We fully control our secure monitor. That should not be an issue for us.
What do you mean we'd need to call into the monitor? Linux masks all wake
IRQs when going into suspend. Everything unmasked should be a wake IRQ,
or am I missing something? I thought we could simply read the
enable/disable status of all IRQs in the secure monitor suspend path and
do the needed communication with the companion core.

> 
> > 
> > Does anybody have similar problems and probably already solved it?
> > Any other suggestions for approaching the problem? Any preferred
> > solution?
> 
> I think we have the same problem.  Can you provide more detail on the hardware
> implementation of your wake irq controller?  I presume you have some set of
> registers, an irq maybe, and some other stuff?  And how does it 

SOC-specific action for irq_set_wake

[Sören Brinkmann]

Hi,

we are working on the PM solution for Zynq MPSOC and ran into some
problem when setting the wake source.

The situation is that when the A53 is in suspend, the GIC(v2) may be
powered down. In that state a companion core is handling wake
events/IRQs, but we expect the OS/Linux to notify the companion core
about what device/IRQ is a wake up source. Hence, my idea was to capture
enabling/disabling wake IRQs in our platform PM code and then
communicate with the FW as needed during suspend operations. The problem
is: I don't see a good way to notify the platform code about these
events.

My ideas were:
1. Use the irq_chip irq_set_wake function
  My thought was to implement the irq_set_wake function in a
  SOC-specific way (could even be generic and call some notifier chain or
  similar) to notify the platform PM code when a device/IRQ is
  enabled/disabled as wake up source.
  My problem is that the SKIP_IRQ_SET_WAKE flag is set in the generic
  driver (drivers/irqchip/irq-gic.c) and platforms cannot implement
  irq_set_wake without changes in the common code.
2. Stuff things into the secure monitor
  I guess we could read the GIC registers once we enter the secure
  monitor and do the communication with the companion core from there by
  identifying unmasked IRQs as wake IRQs. My concern here is that it
  might introduce additional hardcoded mappings that are much more
  dynamic in Linux thanks to the DT description.

Does anybody have similar problems and probably already solved it?
Any other suggestions for approaching the problem? Any preferred
solution?

Thanks,
Sören

SOC-specific action for irq_set_wake

[Sören Brinkmann]

Hi,

we are working on the PM solution for Zynq MPSOC and ran into some
problem when setting the wake source.

The situation is that when the A53 is in suspend, the GIC(v2) may be
powered down. In that state a companion core is handling wake
events/IRQs, but we expect the OS/Linux to notify the companion core
about what device/IRQ is a wake up source. Hence, my idea was to capture
enabling/disabling wake IRQs in our platform PM code and then
communicate with the FW as needed during suspend operations. The problem
is: I don't see a good way to notify the platform code about these
events.

My ideas were:
1. Use the irq_chip irq_set_wake function
  My thought was to implement the irq_set_wake function in a
  SOC-specific way (could even be generic and call some notifier chain or
  similar) to notify the platform PM code when a device/IRQ is
  enabled/disabled as wake up source.
  My problem is that the SKIP_IRQ_SET_WAKE flag is set in the generic
  driver (drivers/irqchip/irq-gic.c) and platforms cannot implement
  irq_set_wake without changes in the common code.
2. Stuff things into the secure monitor
  I guess we could read the GIC registers once we enter the secure
  monitor and do the communication with the companion core from there by
  identifying unmasked IRQs as wake IRQs. My concern here is that it
  might introduce additional hardcoded mappings that are much more
  dynamic in Linux thanks to the DT description.

Does anybody have similar problems and probably already solved it?
Any other suggestions for approaching the problem? Any preferred
solution?

Thanks,
Sören

Re: [PATCH] clk: zynq: avoid retrieving clock names from DT property

[Sören Brinkmann]

On Sun, 2016-07-17 at 09:11:10 -0700, Sören Brinkmann wrote:
> On Sun, 2016-07-17 at 00:15:23 +0900, Masahiro Yamada wrote:
> > The "clock-output-names" property is useful for generic clock
> > providers such as fixed-clock, fixed-factor-clock, etc.
> > 
> > On the other hand, it should not be used for really SoC-specific
> > clock providers like this one.  As you see in "enum zynq_clk" in
> > this driver, it is written as if it already knows all the clock
> > names.  Besides, this is instantiated only once, so no clock name
> > conflict would happen even if the clock names are hard-coded in the
> > driver.
> > 
> > The device tree (arch/arm/boot/dts/zynq-7000.dtsi) will be fixed
> > later.
> > 
> > Signed-off-by: Masahiro Yamada <yamada.masah...@socionext.com>
> 
> IIRC, this was the only way to allow circular clock routing. E.g. there
> are use-cases that route an FCLK back into the PS to use it as input for
> the GEM. With the introduction of deferred probing this might all be
> possible now but it would be good to verify that this kind of circular
> clock routing still works with this change.

Yep, this breaks this kind of routing. E.g. if you remove the
clk-output-names from the DT and add an EMIO clock input like this:
clocks = < 15>;
clock-names = "gem1_emio_clk";
the code will no longer be able to correctly obtain the clock name for that 
input.

Sören

Re: [PATCH] clk: zynq: avoid retrieving clock names from DT property

[Sören Brinkmann]

On Sun, 2016-07-17 at 09:11:10 -0700, Sören Brinkmann wrote:
> On Sun, 2016-07-17 at 00:15:23 +0900, Masahiro Yamada wrote:
> > The "clock-output-names" property is useful for generic clock
> > providers such as fixed-clock, fixed-factor-clock, etc.
> > 
> > On the other hand, it should not be used for really SoC-specific
> > clock providers like this one.  As you see in "enum zynq_clk" in
> > this driver, it is written as if it already knows all the clock
> > names.  Besides, this is instantiated only once, so no clock name
> > conflict would happen even if the clock names are hard-coded in the
> > driver.
> > 
> > The device tree (arch/arm/boot/dts/zynq-7000.dtsi) will be fixed
> > later.
> > 
> > Signed-off-by: Masahiro Yamada 
> 
> IIRC, this was the only way to allow circular clock routing. E.g. there
> are use-cases that route an FCLK back into the PS to use it as input for
> the GEM. With the introduction of deferred probing this might all be
> possible now but it would be good to verify that this kind of circular
> clock routing still works with this change.

Yep, this breaks this kind of routing. E.g. if you remove the
clk-output-names from the DT and add an EMIO clock input like this:
clocks = < 15>;
clock-names = "gem1_emio_clk";
the code will no longer be able to correctly obtain the clock name for that 
input.

Sören

Re: [PATCH] clk: zynq: avoid retrieving clock names from DT property

[Sören Brinkmann]

On Sun, 2016-07-17 at 00:15:23 +0900, Masahiro Yamada wrote:
> The "clock-output-names" property is useful for generic clock
> providers such as fixed-clock, fixed-factor-clock, etc.
> 
> On the other hand, it should not be used for really SoC-specific
> clock providers like this one.  As you see in "enum zynq_clk" in
> this driver, it is written as if it already knows all the clock
> names.  Besides, this is instantiated only once, so no clock name
> conflict would happen even if the clock names are hard-coded in the
> driver.
> 
> The device tree (arch/arm/boot/dts/zynq-7000.dtsi) will be fixed
> later.
> 
> Signed-off-by: Masahiro Yamada 

IIRC, this was the only way to allow circular clock routing. E.g. there
are use-cases that route an FCLK back into the PS to use it as input for
the GEM. With the introduction of deferred probing this might all be
possible now but it would be good to verify that this kind of circular
clock routing still works with this change.

Sören

Re: [PATCH] clk: zynq: avoid retrieving clock names from DT property

[Sören Brinkmann]

On Sun, 2016-07-17 at 00:15:23 +0900, Masahiro Yamada wrote:
> The "clock-output-names" property is useful for generic clock
> providers such as fixed-clock, fixed-factor-clock, etc.
> 
> On the other hand, it should not be used for really SoC-specific
> clock providers like this one.  As you see in "enum zynq_clk" in
> this driver, it is written as if it already knows all the clock
> names.  Besides, this is instantiated only once, so no clock name
> conflict would happen even if the clock names are hard-coded in the
> driver.
> 
> The device tree (arch/arm/boot/dts/zynq-7000.dtsi) will be fixed
> later.
> 
> Signed-off-by: Masahiro Yamada 

IIRC, this was the only way to allow circular clock routing. E.g. there
are use-cases that route an FCLK back into the PS to use it as input for
the GEM. With the introduction of deferred probing this might all be
possible now but it would be good to verify that this kind of circular
clock routing still works with this change.

Sören

Re: [PATCH] PCI: xilinx: Fix return value in case of error

[Sören Brinkmann]

On Thu, 2016-07-14 at 12:10:46 +0200, Christophe JAILLET wrote:
> In function 'xilinx_pcie_init_irq_domain', the pattern used to check and
> return error is:
> 
>if (!var) {
>   dev_err(...);
>   return PTR_ERR(var);
>}
> 
> So the returned value in case of error is always 0, which means 'success'.
> Change it to return -ENODEV instead.
> 
> Signed-off-by: Christophe JAILLET <christophe.jail...@wanadoo.fr>
Acked-by: Sören Brinkmann <soren.brinkm...@xilinx.com>

Sören

Re: [PATCH] PCI: xilinx: Fix return value in case of error

[Sören Brinkmann]

On Thu, 2016-07-14 at 12:10:46 +0200, Christophe JAILLET wrote:
> In function 'xilinx_pcie_init_irq_domain', the pattern used to check and
> return error is:
> 
>if (!var) {
>   dev_err(...);
>   return PTR_ERR(var);
>}
> 
> So the returned value in case of error is always 0, which means 'success'.
> Change it to return -ENODEV instead.
> 
> Signed-off-by: Christophe JAILLET 
Acked-by: Sören Brinkmann 

Sören

Re: [PATCH] clocksource: cadence_ttc: fix a return value in case of error

[Sören Brinkmann]

On Wed, 2016-07-06 at 07:35:23 +0200, Christophe JAILLET wrote:
> IS_ERR and PTR_ERR should use the same variable, clk_ce in this case.
> 
> Fixes: 4de1eb07c47f (Convert init function to return error)
> 
> Signed-off-by: Christophe JAILLET <christophe.jail...@wanadoo.fr>
Acked-by: Sören Brinkmann <soren.brinkm...@xilinx.com>

Sören

Re: [PATCH] clocksource: cadence_ttc: fix a return value in case of error

[Sören Brinkmann]

On Wed, 2016-07-06 at 07:35:23 +0200, Christophe JAILLET wrote:
> IS_ERR and PTR_ERR should use the same variable, clk_ce in this case.
> 
> Fixes: 4de1eb07c47f (Convert init function to return error)
> 
> Signed-off-by: Christophe JAILLET 
Acked-by: Sören Brinkmann 

Sören

Re: [PATCH V2 07/63] clocksource/drivers/cadence_ttc: Convert init function to return error

[Sören Brinkmann]

On Thu, 2016-06-16 at 23:26:26 +0200, Daniel Lezcano wrote:
> The init functions do not return any error. They behave as the following:
> 
>  - panic, thus leading to a kernel crash while another timer may work and
>make the system boot up correctly
> 
>  or
> 
>  - print an error and let the caller unaware if the state of the system
> 
> Change that by converting the init functions to return an error conforming
> to the CLOCKSOURCE_OF_RET prototype.
> 
> Proper error handling (rollback, errno value) will be changed later case
> by case, thus this change just return back an error or success in the init
> function.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezc...@linaro.org>
Acked-by: Sören Brinkmann <soren.brinkm...@xilinx.com>

Sören

Re: [PATCH V2 07/63] clocksource/drivers/cadence_ttc: Convert init function to return error

[Sören Brinkmann]

On Thu, 2016-06-16 at 23:26:26 +0200, Daniel Lezcano wrote:
> The init functions do not return any error. They behave as the following:
> 
>  - panic, thus leading to a kernel crash while another timer may work and
>make the system boot up correctly
> 
>  or
> 
>  - print an error and let the caller unaware if the state of the system
> 
> Change that by converting the init functions to return an error conforming
> to the CLOCKSOURCE_OF_RET prototype.
> 
> Proper error handling (rollback, errno value) will be changed later case
> by case, thus this change just return back an error or success in the init
> function.
> 
> Signed-off-by: Daniel Lezcano 
Acked-by: Sören Brinkmann 

Sören

Re: [PATCH 7/9] pinctrl: zynq: make it explicitly non-modular

[Sören Brinkmann]

On Mon, 2016-06-06 at 22:43:06 -0400, Paul Gortmaker wrote:
> The Kconfig currently controlling compilation of this code is:
> 
> config PINCTRL_ZYNQ
> bool "Pinctrl driver for Xilinx Zynq"
> 
> ...meaning that it currently is not being built as a module by anyone.
> 
> Lets remove the modular code that is essentially orphaned, so that
> when reading the driver there is no doubt it is builtin-only.
> 
> Since module_platform_driver() uses the same init level priority as
> builtin_platform_driver() the init ordering remains unchanged with
> this commit.
> 
> Also note that MODULE_DEVICE_TABLE is a no-op for non-modular code.
> 
> We also delete the MODULE_LICENSE tag etc. since all that information
> is already contained at the top of the file in the comments.
> 
> Cc: Linus Walleij <linus.wall...@linaro.org>
> Cc: Michal Simek <michal.si...@xilinx.com>
> Cc: "Sören Brinkmann" <soren.brinkm...@xilinx.com>
> Cc: linux-g...@vger.kernel.org
> Signed-off-by: Paul Gortmaker <paul.gortma...@windriver.com>
Acked-by: "Sören Brinkmann" <soren.brinkm...@xilinx.com>

Sören

Re: [PATCH 7/9] pinctrl: zynq: make it explicitly non-modular

[Sören Brinkmann]

On Mon, 2016-06-06 at 22:43:06 -0400, Paul Gortmaker wrote:
> The Kconfig currently controlling compilation of this code is:
> 
> config PINCTRL_ZYNQ
> bool "Pinctrl driver for Xilinx Zynq"
> 
> ...meaning that it currently is not being built as a module by anyone.
> 
> Lets remove the modular code that is essentially orphaned, so that
> when reading the driver there is no doubt it is builtin-only.
> 
> Since module_platform_driver() uses the same init level priority as
> builtin_platform_driver() the init ordering remains unchanged with
> this commit.
> 
> Also note that MODULE_DEVICE_TABLE is a no-op for non-modular code.
> 
> We also delete the MODULE_LICENSE tag etc. since all that information
> is already contained at the top of the file in the comments.
> 
> Cc: Linus Walleij 
> Cc: Michal Simek 
> Cc: "Sören Brinkmann" 
> Cc: linux-g...@vger.kernel.org
> Signed-off-by: Paul Gortmaker 
Acked-by: "Sören Brinkmann" 

Sören

Re: [PATCH 7/9] clocksource/drivers/cadence_ttc: Convert init function to return error

[Sören Brinkmann]

Hi Daniel,

On Wed, 2016-06-01 at 10:34:50 +0200, Daniel Lezcano wrote:
> The init functions do not return any error. They behave as the following:
> 
>  - panic, thus leading to a kernel crash while another timer may work and
>make the system boot up correctly
> 
>  or
> 
>  - print an error and let the caller unaware if the state of the system
> 
> Change that by converting the init functions to return an error conforming
> to the CLOCKSOURCE_OF_RET prototype.
> 
> Proper error handling (rollback, errno value) will be changed later case
> by case, thus this change just return back an error or success in the init
> function.
> 
> Signed-off-by: Daniel Lezcano 
> ---
>  drivers/clocksource/cadence_ttc_timer.c | 82 
> +++--
>  1 file changed, 48 insertions(+), 34 deletions(-)
> 
[...]
>   ttcce->ttc.clk_rate_change_nb.notifier_call =
>   ttc_rate_change_clockevent_cb;
>   ttcce->ttc.clk_rate_change_nb.next = NULL;
> - if (clk_notifier_register(ttcce->ttc.clk,
> - >ttc.clk_rate_change_nb))
> +
> + err = clk_notifier_register(ttcce->ttc.clk,
> + >ttc.clk_rate_change_nb);
> + if (err) {
>   pr_warn("Unable to register clock notifier.\n");
> + return err;

So far we handle this as warning only and move on, as the notifier is
only needed when frequency scaling is enabled. And even then, the effect
is usually just that timing is off.

> + }
> +
>   ttcce->ttc.freq = clk_get_rate(ttcce->ttc.clk);
>  
>   ttcce->ttc.base_addr = base;
> @@ -451,13 +462,15 @@ static void __init ttc_setup_clockevent(struct clk *clk,
>  
>   err = request_irq(irq, ttc_clock_event_interrupt,
> IRQF_TIMER, ttcce->ce.name, ttcce);
> - if (WARN_ON(err)) {
> + if (err) {
>   kfree(ttcce);
> - return;
> + return err;
>   }
>  
>   clockevents_config_and_register(>ce,
>   ttcce->ttc.freq / PRESCALE, 1, 0xfffe);
> +
> + return 0;
>  }
>  
>  /**
> @@ -466,20 +479,14 @@ static void __init ttc_setup_clockevent(struct clk *clk,
>   * Initializes the timer hardware and register the clock source and clock 
> event
>   * timers with Linux kernal timer framework
>   */
> -static void __init ttc_timer_init(struct device_node *timer)
> +static int __init ttc_timer_init(struct device_node *timer)
>  {
>   unsigned int irq;
>   void __iomem *timer_baseaddr;
>   struct clk *clk_cs, *clk_ce;
> - static int initialized;
> - int clksel;
> + int clksel, ret;
>   u32 timer_width = 16;
>  
> - if (initialized)
> - return;
> -
> - initialized = 1;
> -

This also changes behavior. We have multiple of these timer modules in
our HW and we don't want them all to be used for time keeping. This
construct made sure that we only use the first timer for which init is
called leaving the others for non-OS purposes.

Sören

Re: [PATCH 7/9] clocksource/drivers/cadence_ttc: Convert init function to return error

[Sören Brinkmann]

Hi Daniel,

On Wed, 2016-06-01 at 10:34:50 +0200, Daniel Lezcano wrote:
> The init functions do not return any error. They behave as the following:
> 
>  - panic, thus leading to a kernel crash while another timer may work and
>make the system boot up correctly
> 
>  or
> 
>  - print an error and let the caller unaware if the state of the system
> 
> Change that by converting the init functions to return an error conforming
> to the CLOCKSOURCE_OF_RET prototype.
> 
> Proper error handling (rollback, errno value) will be changed later case
> by case, thus this change just return back an error or success in the init
> function.
> 
> Signed-off-by: Daniel Lezcano 
> ---
>  drivers/clocksource/cadence_ttc_timer.c | 82 
> +++--
>  1 file changed, 48 insertions(+), 34 deletions(-)
> 
[...]
>   ttcce->ttc.clk_rate_change_nb.notifier_call =
>   ttc_rate_change_clockevent_cb;
>   ttcce->ttc.clk_rate_change_nb.next = NULL;
> - if (clk_notifier_register(ttcce->ttc.clk,
> - >ttc.clk_rate_change_nb))
> +
> + err = clk_notifier_register(ttcce->ttc.clk,
> + >ttc.clk_rate_change_nb);
> + if (err) {
>   pr_warn("Unable to register clock notifier.\n");
> + return err;

So far we handle this as warning only and move on, as the notifier is
only needed when frequency scaling is enabled. And even then, the effect
is usually just that timing is off.

> + }
> +
>   ttcce->ttc.freq = clk_get_rate(ttcce->ttc.clk);
>  
>   ttcce->ttc.base_addr = base;
> @@ -451,13 +462,15 @@ static void __init ttc_setup_clockevent(struct clk *clk,
>  
>   err = request_irq(irq, ttc_clock_event_interrupt,
> IRQF_TIMER, ttcce->ce.name, ttcce);
> - if (WARN_ON(err)) {
> + if (err) {
>   kfree(ttcce);
> - return;
> + return err;
>   }
>  
>   clockevents_config_and_register(>ce,
>   ttcce->ttc.freq / PRESCALE, 1, 0xfffe);
> +
> + return 0;
>  }
>  
>  /**
> @@ -466,20 +479,14 @@ static void __init ttc_setup_clockevent(struct clk *clk,
>   * Initializes the timer hardware and register the clock source and clock 
> event
>   * timers with Linux kernal timer framework
>   */
> -static void __init ttc_timer_init(struct device_node *timer)
> +static int __init ttc_timer_init(struct device_node *timer)
>  {
>   unsigned int irq;
>   void __iomem *timer_baseaddr;
>   struct clk *clk_cs, *clk_ce;
> - static int initialized;
> - int clksel;
> + int clksel, ret;
>   u32 timer_width = 16;
>  
> - if (initialized)
> - return;
> -
> - initialized = 1;
> -

This also changes behavior. We have multiple of these timer modules in
our HW and we don't want them all to be used for time keeping. This
construct made sure that we only use the first timer for which init is
called leaving the others for non-OS purposes.

Sören

Re: [PATCH 2/5] dmaengine: vdma: Use dma_pool_zalloc

[Sören Brinkmann]

On Fri, 2016-04-29 at 22:09:09 +0200, Julia Lawall wrote:
> Dma_pool_zalloc combines dma_pool_alloc and memset 0.  The semantic patch
> that makes this transformation is as follows: (http://coccinelle.lip6.fr/)
> 
> // 
> @@
> expression d,e;
> statement S;
> @@
> 
> d =
> -dma_pool_alloc
> +dma_pool_zalloc
>  (...);
> if (!d) S
> -   memset(d, 0, sizeof(*d));
> // 
> 
> Signed-off-by: Julia Lawall <julia.law...@lip6.fr>
Acked-by: Sören Brinkmann <soren.brinkm...@xilinx.com>

Sören

Re: [PATCH 2/5] dmaengine: vdma: Use dma_pool_zalloc

[Sören Brinkmann]

On Fri, 2016-04-29 at 22:09:09 +0200, Julia Lawall wrote:
> Dma_pool_zalloc combines dma_pool_alloc and memset 0.  The semantic patch
> that makes this transformation is as follows: (http://coccinelle.lip6.fr/)
> 
> // 
> @@
> expression d,e;
> statement S;
> @@
> 
> d =
> -dma_pool_alloc
> +dma_pool_zalloc
>  (...);
> if (!d) S
> -   memset(d, 0, sizeof(*d));
> // 
> 
> Signed-off-by: Julia Lawall 
Acked-by: Sören Brinkmann 

Sören

Re: [PATCH v2 6/6] mmc: sdhci-of-arasan: overwrite enhanced strobe callback

[Sören Brinkmann]

On Fri, 2016-04-29 at 10:48:34 +0800, Shawn Lin wrote:
> Currently sdhci-arasan 5.1 can support enhanced strobe function,
> and we now limit it just for "arasan,sdhci-5.1". Add
> mmc-hs400-enhanced-strobe in DT to enable the function if we'r sure
> our controller can support it.
> 
> Signed-off-by: Shawn Lin <shawn@rock-chips.com>
> ---
> 
> Changes in v2: None
> 
>  drivers/mmc/host/sdhci-of-arasan.c | 22 ++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c 
> b/drivers/mmc/host/sdhci-of-arasan.c
> index 20b859e..a0e4536 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -25,7 +25,9 @@
>  #include "sdhci-pltfm.h"
>  
>  #define SDHCI_ARASAN_CLK_CTRL_OFFSET 0x2c
> +#define SDHCI_ARASAN_VENDOR_REGISTER 0x78

Just as a note: This register doesn't seem to exist in the IP version
Zynq is using. So, as long as we exclude that version from accessing it,
things should be fine (IIUC, that is the case).

Acked-by: Sören Brinkmann <soren.brinkm...@xilinx.com>

Sören

Re: [PATCH v2 6/6] mmc: sdhci-of-arasan: overwrite enhanced strobe callback

[Sören Brinkmann]

On Fri, 2016-04-29 at 10:48:34 +0800, Shawn Lin wrote:
> Currently sdhci-arasan 5.1 can support enhanced strobe function,
> and we now limit it just for "arasan,sdhci-5.1". Add
> mmc-hs400-enhanced-strobe in DT to enable the function if we'r sure
> our controller can support it.
> 
> Signed-off-by: Shawn Lin 
> ---
> 
> Changes in v2: None
> 
>  drivers/mmc/host/sdhci-of-arasan.c | 22 ++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c 
> b/drivers/mmc/host/sdhci-of-arasan.c
> index 20b859e..a0e4536 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -25,7 +25,9 @@
>  #include "sdhci-pltfm.h"
>  
>  #define SDHCI_ARASAN_CLK_CTRL_OFFSET 0x2c
> +#define SDHCI_ARASAN_VENDOR_REGISTER 0x78

Just as a note: This register doesn't seem to exist in the IP version
Zynq is using. So, as long as we exclude that version from accessing it,
things should be fine (IIUC, that is the case).

Acked-by: Sören Brinkmann 

Sören

Re: [PATCH v3 3/3] dmaengine: vdma: Add clock support

[Sören Brinkmann]

On Thu, 2016-04-21 at 09:32:44 -0700, Appana Durga Kedareswara Rao wrote:
> Hi Soren,
> 
> > -Original Message-
> > From: Sören Brinkmann [mailto:soren.brinkm...@xilinx.com]
> > Sent: Thursday, April 21, 2016 9:52 PM
> > To: Appana Durga Kedareswara Rao <appa...@xilinx.com>
> > Cc: robh...@kernel.org; pawel.m...@arm.com; mark.rutl...@arm.com;
> > ijc+devicet...@hellion.org.uk; ga...@codeaurora.org; Michal Simek
> > <mich...@xilinx.com>; vinod.k...@intel.com; dan.j.willi...@intel.com;
> > Appana Durga Kedareswara Rao <appa...@xilinx.com>;
> > moritz.fisc...@ettus.com; laurent.pinch...@ideasonboard.com;
> > l...@debethencourt.com; Anirudha Sarangi <anir...@xilinx.com>; Punnaiah
> > Choudary Kalluri <punn...@xilinx.com>; Shubhrajyoti Datta
> > <shubh...@xilinx.com>; devicet...@vger.kernel.org; linux-arm-
> > ker...@lists.infradead.org; linux-kernel@vger.kernel.org;
> > dmaeng...@vger.kernel.org
> > Subject: Re: [PATCH v3 3/3] dmaengine: vdma: Add clock support
> > 
> > On Thu, 2016-04-21 at 16:08:38 +0530, Kedareswara rao Appana wrote:
[...]
> > > @@ -1757,6 +1767,200 @@ static void xilinx_dma_chan_remove(struct
> > xilinx_dma_chan *chan)
> > >   list_del(>common.device_node);
> > >  }
> > >
> > > +static int axidma_clk_init(struct platform_device *pdev, struct clk 
> > > **axi_clk,
> > > + struct clk **tx_clk, struct clk **rx_clk,
> > > + struct clk **sg_clk, struct clk **tmp_clk) {
> > > + int err;
> > > +
> > > + *tmp_clk = NULL;
> > > +
> > > + *axi_clk = devm_clk_get(>dev, "s_axi_lite_aclk");
> > > + if (IS_ERR(*axi_clk)) {
> > > + err = PTR_ERR(*axi_clk);
> > > + dev_err(>dev, "failed to get axi_aclk (%u)\n", err);
> > > + return err;
> > > + }
> > > +
> > > + *tx_clk = devm_clk_get(>dev, "m_axi_mm2s_aclk");
> > > + if (IS_ERR(*tx_clk))
> > > + *tx_clk = NULL;
> > > +
> > > + *rx_clk = devm_clk_get(>dev, "m_axi_s2mm_aclk");
> > > + if (IS_ERR(*rx_clk))
> > > + *rx_clk = NULL;
> > > +
> > > + *sg_clk = devm_clk_get(>dev, "m_axi_sg_aclk");
> > > + if (IS_ERR(*sg_clk))
> > > + *sg_clk = NULL;
> > > +
> > > +
> > > + err = clk_prepare_enable(*axi_clk);
> > 
> > Should this be called if you know that the pointer might be NULL?
> 
> It is a mandatory clock and if this clk is not there in DT I am already 
> returning error...
> I didn't get your question could you please elaborate???

But for all the optional clocks. They could all be NULL and you're calling
clk_prepare_enable with a NULL pointer. That function is nice enough to
do a NULL check for you, but I wonder whether these calls should happen at
all when you already know that the pointer is not a valid clock.

Sören

Re: [PATCH v3 3/3] dmaengine: vdma: Add clock support

[Sören Brinkmann]

On Thu, 2016-04-21 at 09:32:44 -0700, Appana Durga Kedareswara Rao wrote:
> Hi Soren,
> 
> > -Original Message-
> > From: Sören Brinkmann [mailto:soren.brinkm...@xilinx.com]
> > Sent: Thursday, April 21, 2016 9:52 PM
> > To: Appana Durga Kedareswara Rao 
> > Cc: robh...@kernel.org; pawel.m...@arm.com; mark.rutl...@arm.com;
> > ijc+devicet...@hellion.org.uk; ga...@codeaurora.org; Michal Simek
> > ; vinod.k...@intel.com; dan.j.willi...@intel.com;
> > Appana Durga Kedareswara Rao ;
> > moritz.fisc...@ettus.com; laurent.pinch...@ideasonboard.com;
> > l...@debethencourt.com; Anirudha Sarangi ; Punnaiah
> > Choudary Kalluri ; Shubhrajyoti Datta
> > ; devicet...@vger.kernel.org; linux-arm-
> > ker...@lists.infradead.org; linux-kernel@vger.kernel.org;
> > dmaeng...@vger.kernel.org
> > Subject: Re: [PATCH v3 3/3] dmaengine: vdma: Add clock support
> > 
> > On Thu, 2016-04-21 at 16:08:38 +0530, Kedareswara rao Appana wrote:
[...]
> > > @@ -1757,6 +1767,200 @@ static void xilinx_dma_chan_remove(struct
> > xilinx_dma_chan *chan)
> > >   list_del(>common.device_node);
> > >  }
> > >
> > > +static int axidma_clk_init(struct platform_device *pdev, struct clk 
> > > **axi_clk,
> > > + struct clk **tx_clk, struct clk **rx_clk,
> > > + struct clk **sg_clk, struct clk **tmp_clk) {
> > > + int err;
> > > +
> > > + *tmp_clk = NULL;
> > > +
> > > + *axi_clk = devm_clk_get(>dev, "s_axi_lite_aclk");
> > > + if (IS_ERR(*axi_clk)) {
> > > + err = PTR_ERR(*axi_clk);
> > > + dev_err(>dev, "failed to get axi_aclk (%u)\n", err);
> > > + return err;
> > > + }
> > > +
> > > + *tx_clk = devm_clk_get(>dev, "m_axi_mm2s_aclk");
> > > + if (IS_ERR(*tx_clk))
> > > + *tx_clk = NULL;
> > > +
> > > + *rx_clk = devm_clk_get(>dev, "m_axi_s2mm_aclk");
> > > + if (IS_ERR(*rx_clk))
> > > + *rx_clk = NULL;
> > > +
> > > + *sg_clk = devm_clk_get(>dev, "m_axi_sg_aclk");
> > > + if (IS_ERR(*sg_clk))
> > > + *sg_clk = NULL;
> > > +
> > > +
> > > + err = clk_prepare_enable(*axi_clk);
> > 
> > Should this be called if you know that the pointer might be NULL?
> 
> It is a mandatory clock and if this clk is not there in DT I am already 
> returning error...
> I didn't get your question could you please elaborate???

But for all the optional clocks. They could all be NULL and you're calling
clk_prepare_enable with a NULL pointer. That function is nice enough to
do a NULL check for you, but I wonder whether these calls should happen at
all when you already know that the pointer is not a valid clock.

Sören

Re: [PATCH v3 3/3] dmaengine: vdma: Add clock support

[Sören Brinkmann]

On Thu, 2016-04-21 at 16:08:38 +0530, Kedareswara rao Appana wrote:
> Added basic clock support for axi dma's.
> The clocks are requested at probe and released at remove.
> 
> Reviewed-by: Shubhrajyoti Datta 
> Signed-off-by: Kedareswara rao Appana 
> ---
> Changes for v3:
> ---> Added clock support for all the AXI DMA's.
> ---> Fixed clk_unprepare leak during probe fail as suggested by Moritz.
> ---> Fixed comment driver specifically asks for the clocks it needs and return
> an error if a mandatory clock is missing as suggested by soren.
> Changes for v2:
> ---> None.
> 
>  drivers/dma/xilinx/xilinx_vdma.c | 225 
> ++-
>  1 file changed, 223 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/xilinx/xilinx_vdma.c 
> b/drivers/dma/xilinx/xilinx_vdma.c
> index 5bfaa32..41bb5b3 100644
> --- a/drivers/dma/xilinx/xilinx_vdma.c
> +++ b/drivers/dma/xilinx/xilinx_vdma.c
> @@ -44,6 +44,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "../dmaengine.h"
>  
> @@ -344,6 +345,9 @@ struct xilinx_dma_chan {
>  
>  struct dma_config {
>   enum xdma_ip_type dmatype;
> + int (*clk_init)(struct platform_device *pdev, struct clk **axi_clk,
> + struct clk **tx_clk, struct clk **txs_clk,
> + struct clk **rx_clk, struct clk **rxs_clk);
>  };
>  
>  /**
> @@ -365,7 +369,13 @@ struct xilinx_dma_device {
>   bool has_sg;
>   u32 flush_on_fsync;
>   bool ext_addr;
> + struct platform_device  *pdev;
>   const struct dma_config *dma_config;
> + struct clk *axi_clk;
> + struct clk *tx_clk;
> + struct clk *txs_clk;
> + struct clk *rx_clk;
> + struct clk *rxs_clk;
>  };

the struct members could be documented

>  
>  /* Macros */
> @@ -1757,6 +1767,200 @@ static void xilinx_dma_chan_remove(struct 
> xilinx_dma_chan *chan)
>   list_del(>common.device_node);
>  }
>  
> +static int axidma_clk_init(struct platform_device *pdev, struct clk 
> **axi_clk,
> + struct clk **tx_clk, struct clk **rx_clk,
> + struct clk **sg_clk, struct clk **tmp_clk)
> +{
> + int err;
> +
> + *tmp_clk = NULL;
> +
> + *axi_clk = devm_clk_get(>dev, "s_axi_lite_aclk");
> + if (IS_ERR(*axi_clk)) {
> + err = PTR_ERR(*axi_clk);
> + dev_err(>dev, "failed to get axi_aclk (%u)\n", err);
> + return err;
> + }
> +
> + *tx_clk = devm_clk_get(>dev, "m_axi_mm2s_aclk");
> + if (IS_ERR(*tx_clk))
> + *tx_clk = NULL;
> +
> + *rx_clk = devm_clk_get(>dev, "m_axi_s2mm_aclk");
> + if (IS_ERR(*rx_clk))
> + *rx_clk = NULL;
> +
> + *sg_clk = devm_clk_get(>dev, "m_axi_sg_aclk");
> + if (IS_ERR(*sg_clk))
> + *sg_clk = NULL;
> +
> +
> + err = clk_prepare_enable(*axi_clk);

Should this be called if you know that the pointer might be NULL?

> + if (err) {
> + dev_err(>dev, "failed to enable axi_clk (%u)\n", err);
> + return err;
> + }
> +
> + err = clk_prepare_enable(*tx_clk);
> + if (err) {
> + dev_err(>dev, "failed to enable tx_clk (%u)\n", err);
> + goto err_disable_axiclk;
> + }
> +
> + err = clk_prepare_enable(*rx_clk);
> + if (err) {
> + dev_err(>dev, "failed to enable txs_clk (%u)\n", err);
> + goto err_disable_txclk;
> + }
> +
> + err = clk_prepare_enable(*sg_clk);
> + if (err) {
> + dev_err(>dev, "failed to enable rxs_clk (%u)\n", err);
> + goto err_disable_rxclk;
> + }
> +
> + return 0;
> +
> +err_disable_rxclk:
> + clk_disable_unprepare(*rx_clk);
> +err_disable_txclk:
> + clk_disable_unprepare(*tx_clk);
> +err_disable_axiclk:
> + clk_disable_unprepare(*axi_clk);
> +
> + return err;
> +}
> +
> +static int axicdma_clk_init(struct platform_device *pdev, struct clk 
> **axi_clk,
> + struct clk **dev_clk, struct clk **tmp_clk,
> + struct clk **tmp1_clk, struct clk **tmp2_clk)
> +{
> + int err;
> +
> + *tmp_clk = NULL;
> + *tmp1_clk = NULL;
> + *tmp2_clk = NULL;
> +
> + *axi_clk = devm_clk_get(>dev, "s_axi_lite_aclk");
> + if (IS_ERR(*axi_clk)) {
> + err = PTR_ERR(*axi_clk);
> + dev_err(>dev, "failed to get axi_aclk (%u)\n", err);
> + return err;
> + }
> +
> + *dev_clk = devm_clk_get(>dev, "m_axi_aclk");
> + if (IS_ERR(*dev_clk))
> + *dev_clk = NULL;

This is a required clock according to binding but a failure is ignored
here.

> +
> +
> + err = clk_prepare_enable(*axi_clk);
> + if (err) {
> + dev_err(>dev, "failed to enable axi_clk (%u)\n", err);
> + return err;
> + }
> +
> + err = clk_prepare_enable(*dev_clk);
> + if (err) {
> + dev_err(>dev, "failed to enable tx_clk (%u)\n", err);
> 

Re: [PATCH v3 3/3] dmaengine: vdma: Add clock support

[Sören Brinkmann]

On Thu, 2016-04-21 at 16:08:38 +0530, Kedareswara rao Appana wrote:
> Added basic clock support for axi dma's.
> The clocks are requested at probe and released at remove.
> 
> Reviewed-by: Shubhrajyoti Datta 
> Signed-off-by: Kedareswara rao Appana 
> ---
> Changes for v3:
> ---> Added clock support for all the AXI DMA's.
> ---> Fixed clk_unprepare leak during probe fail as suggested by Moritz.
> ---> Fixed comment driver specifically asks for the clocks it needs and return
> an error if a mandatory clock is missing as suggested by soren.
> Changes for v2:
> ---> None.
> 
>  drivers/dma/xilinx/xilinx_vdma.c | 225 
> ++-
>  1 file changed, 223 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/xilinx/xilinx_vdma.c 
> b/drivers/dma/xilinx/xilinx_vdma.c
> index 5bfaa32..41bb5b3 100644
> --- a/drivers/dma/xilinx/xilinx_vdma.c
> +++ b/drivers/dma/xilinx/xilinx_vdma.c
> @@ -44,6 +44,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "../dmaengine.h"
>  
> @@ -344,6 +345,9 @@ struct xilinx_dma_chan {
>  
>  struct dma_config {
>   enum xdma_ip_type dmatype;
> + int (*clk_init)(struct platform_device *pdev, struct clk **axi_clk,
> + struct clk **tx_clk, struct clk **txs_clk,
> + struct clk **rx_clk, struct clk **rxs_clk);
>  };
>  
>  /**
> @@ -365,7 +369,13 @@ struct xilinx_dma_device {
>   bool has_sg;
>   u32 flush_on_fsync;
>   bool ext_addr;
> + struct platform_device  *pdev;
>   const struct dma_config *dma_config;
> + struct clk *axi_clk;
> + struct clk *tx_clk;
> + struct clk *txs_clk;
> + struct clk *rx_clk;
> + struct clk *rxs_clk;
>  };

the struct members could be documented

>  
>  /* Macros */
> @@ -1757,6 +1767,200 @@ static void xilinx_dma_chan_remove(struct 
> xilinx_dma_chan *chan)
>   list_del(>common.device_node);
>  }
>  
> +static int axidma_clk_init(struct platform_device *pdev, struct clk 
> **axi_clk,
> + struct clk **tx_clk, struct clk **rx_clk,
> + struct clk **sg_clk, struct clk **tmp_clk)
> +{
> + int err;
> +
> + *tmp_clk = NULL;
> +
> + *axi_clk = devm_clk_get(>dev, "s_axi_lite_aclk");
> + if (IS_ERR(*axi_clk)) {
> + err = PTR_ERR(*axi_clk);
> + dev_err(>dev, "failed to get axi_aclk (%u)\n", err);
> + return err;
> + }
> +
> + *tx_clk = devm_clk_get(>dev, "m_axi_mm2s_aclk");
> + if (IS_ERR(*tx_clk))
> + *tx_clk = NULL;
> +
> + *rx_clk = devm_clk_get(>dev, "m_axi_s2mm_aclk");
> + if (IS_ERR(*rx_clk))
> + *rx_clk = NULL;
> +
> + *sg_clk = devm_clk_get(>dev, "m_axi_sg_aclk");
> + if (IS_ERR(*sg_clk))
> + *sg_clk = NULL;
> +
> +
> + err = clk_prepare_enable(*axi_clk);

Should this be called if you know that the pointer might be NULL?

> + if (err) {
> + dev_err(>dev, "failed to enable axi_clk (%u)\n", err);
> + return err;
> + }
> +
> + err = clk_prepare_enable(*tx_clk);
> + if (err) {
> + dev_err(>dev, "failed to enable tx_clk (%u)\n", err);
> + goto err_disable_axiclk;
> + }
> +
> + err = clk_prepare_enable(*rx_clk);
> + if (err) {
> + dev_err(>dev, "failed to enable txs_clk (%u)\n", err);
> + goto err_disable_txclk;
> + }
> +
> + err = clk_prepare_enable(*sg_clk);
> + if (err) {
> + dev_err(>dev, "failed to enable rxs_clk (%u)\n", err);
> + goto err_disable_rxclk;
> + }
> +
> + return 0;
> +
> +err_disable_rxclk:
> + clk_disable_unprepare(*rx_clk);
> +err_disable_txclk:
> + clk_disable_unprepare(*tx_clk);
> +err_disable_axiclk:
> + clk_disable_unprepare(*axi_clk);
> +
> + return err;
> +}
> +
> +static int axicdma_clk_init(struct platform_device *pdev, struct clk 
> **axi_clk,
> + struct clk **dev_clk, struct clk **tmp_clk,
> + struct clk **tmp1_clk, struct clk **tmp2_clk)
> +{
> + int err;
> +
> + *tmp_clk = NULL;
> + *tmp1_clk = NULL;
> + *tmp2_clk = NULL;
> +
> + *axi_clk = devm_clk_get(>dev, "s_axi_lite_aclk");
> + if (IS_ERR(*axi_clk)) {
> + err = PTR_ERR(*axi_clk);
> + dev_err(>dev, "failed to get axi_aclk (%u)\n", err);
> + return err;
> + }
> +
> + *dev_clk = devm_clk_get(>dev, "m_axi_aclk");
> + if (IS_ERR(*dev_clk))
> + *dev_clk = NULL;

This is a required clock according to binding but a failure is ignored
here.

> +
> +
> + err = clk_prepare_enable(*axi_clk);
> + if (err) {
> + dev_err(>dev, "failed to enable axi_clk (%u)\n", err);
> + return err;
> + }
> +
> + err = clk_prepare_enable(*dev_clk);
> + if (err) {
> + dev_err(>dev, "failed to enable tx_clk (%u)\n", err);
> + goto err_disable_axiclk;
> 

Re: [PATCH v2 2/2] dmaengine: vdma: Add clock support

[Sören Brinkmann]

On Wed, 2016-04-20 at 07:55:27 -0700, Appana Durga Kedareswara Rao wrote:
> Hi Soren,
> 
> > -Original Message-
> > From: Sören Brinkmann [mailto:soren.brinkm...@xilinx.com]
> > Sent: Wednesday, April 20, 2016 8:06 PM
> > To: Appana Durga Kedareswara Rao <appa...@xilinx.com>
> > Cc: robh...@kernel.org; pawel.m...@arm.com; mark.rutl...@arm.com;
> > ijc+devicet...@hellion.org.uk; ga...@codeaurora.org; Michal Simek
> > <mich...@xilinx.com>; vinod.k...@intel.com; dan.j.willi...@intel.com;
> > Appana Durga Kedareswara Rao <appa...@xilinx.com>;
> > moritz.fisc...@ettus.com; laurent.pinch...@ideasonboard.com;
> > l...@debethencourt.com; Anirudha Sarangi <anir...@xilinx.com>; Punnaiah
> > Choudary Kalluri <punn...@xilinx.com>; Shubhrajyoti Datta
> > <shubh...@xilinx.com>; devicet...@vger.kernel.org; linux-arm-
> > ker...@lists.infradead.org; linux-kernel@vger.kernel.org;
> > dmaeng...@vger.kernel.org
> > Subject: Re: [PATCH v2 2/2] dmaengine: vdma: Add clock support
> > 
> > On Wed, 2016-04-20 at 17:13:19 +0530, Kedareswara rao Appana wrote:
> > > Added basic clock support. The clocks are requested at probe and
> > > released at remove.
> > >
> > > Signed-off-by: Kedareswara rao Appana <appa...@xilinx.com>
> > > ---
> > > Changes for v2:
> > > --> None.
> > >
> > >  drivers/dma/xilinx/xilinx_vdma.c | 56
> > > 
> > >  1 file changed, 56 insertions(+)
> > >
> > > diff --git a/drivers/dma/xilinx/xilinx_vdma.c
> > > b/drivers/dma/xilinx/xilinx_vdma.c
> > > index 70caea6..d526029 100644
> > > --- a/drivers/dma/xilinx/xilinx_vdma.c
> > > +++ b/drivers/dma/xilinx/xilinx_vdma.c
> > > @@ -44,6 +44,7 @@
> > >  #include 
> > >  #include 
> > >  #include 
> > > +#include 
> > >
> > >  #include "../dmaengine.h"
> > >
> > > @@ -352,6 +353,8 @@ struct xilinx_dma_chan {
> > >   * @flush_on_fsync: Flush on frame sync
> > >   * @ext_addr: Indicates 64 bit addressing is supported by dma device
> > >   * @dmatype: DMA ip type
> > > + * @clks:pointer to array of clocks
> > > + * @numclks: number of clocks available
> > >   */
> > >  struct xilinx_dma_device {
> > >   void __iomem *regs;
> > > @@ -362,6 +365,8 @@ struct xilinx_dma_device {
> > >   u32 flush_on_fsync;
> > >   bool ext_addr;
> > >   enum xdma_ip_type dmatype;
> > > + struct clk **clks;
> > > + int numclks;
> > >  };
> > >
> > >  /* Macros */
> > > @@ -1731,6 +1736,26 @@ int xilinx_vdma_channel_set_config(struct
> > > dma_chan *dchan,  }  EXPORT_SYMBOL(xilinx_vdma_channel_set_config);
> > >
> > > +static int xdma_clk_init(struct xilinx_dma_device *xdev, bool enable)
> > > +{
> > > + int i = 0, ret;
> > > +
> > > + for (i = 0; i < xdev->numclks; i++) {
> > > + if (enable) {
> > > + ret = clk_prepare_enable(xdev->clks[i]);
> > > + if (ret) {
> > > + dev_err(xdev->dev,
> > > + "failed to enable the axidma clock\n");
> > > + return ret;
> > > + }
> > > + } else {
> > > + clk_disable_unprepare(xdev->clks[i]);
> > > + }
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > >  /* 
> > > -
> > >   * Probe and remove
> > >   */
> > > @@ -1919,6 +1944,7 @@ static int xilinx_dma_probe(struct platform_device
> > *pdev)
> > >   struct resource *io;
> > >   u32 num_frames, addr_width;
> > >   int i, err;
> > > + const char *str;
> > >
> > >   /* Allocate and initialize the DMA engine structure */
> > >   xdev = devm_kzalloc(>dev, sizeof(*xdev), GFP_KERNEL); @@
> > > -1965,6 +1991,32 @@ static int xilinx_dma_probe(struct platform_device
> > *pdev)
> > >   /* Set the dma mask bits */
> > >   dma_set_mask(xdev->dev, DMA_BIT_MASK(addr_width));
> > >
> > > + xdev->numclks = of_property_count_strings(pdev->dev.of_node,
> > > +   "clock-names");
> > > + if (xdev-

Re: [PATCH v2 2/2] dmaengine: vdma: Add clock support

[Sören Brinkmann]

On Wed, 2016-04-20 at 07:55:27 -0700, Appana Durga Kedareswara Rao wrote:
> Hi Soren,
> 
> > -Original Message-
> > From: Sören Brinkmann [mailto:soren.brinkm...@xilinx.com]
> > Sent: Wednesday, April 20, 2016 8:06 PM
> > To: Appana Durga Kedareswara Rao 
> > Cc: robh...@kernel.org; pawel.m...@arm.com; mark.rutl...@arm.com;
> > ijc+devicet...@hellion.org.uk; ga...@codeaurora.org; Michal Simek
> > ; vinod.k...@intel.com; dan.j.willi...@intel.com;
> > Appana Durga Kedareswara Rao ;
> > moritz.fisc...@ettus.com; laurent.pinch...@ideasonboard.com;
> > l...@debethencourt.com; Anirudha Sarangi ; Punnaiah
> > Choudary Kalluri ; Shubhrajyoti Datta
> > ; devicet...@vger.kernel.org; linux-arm-
> > ker...@lists.infradead.org; linux-kernel@vger.kernel.org;
> > dmaeng...@vger.kernel.org
> > Subject: Re: [PATCH v2 2/2] dmaengine: vdma: Add clock support
> > 
> > On Wed, 2016-04-20 at 17:13:19 +0530, Kedareswara rao Appana wrote:
> > > Added basic clock support. The clocks are requested at probe and
> > > released at remove.
> > >
> > > Signed-off-by: Kedareswara rao Appana 
> > > ---
> > > Changes for v2:
> > > --> None.
> > >
> > >  drivers/dma/xilinx/xilinx_vdma.c | 56
> > > 
> > >  1 file changed, 56 insertions(+)
> > >
> > > diff --git a/drivers/dma/xilinx/xilinx_vdma.c
> > > b/drivers/dma/xilinx/xilinx_vdma.c
> > > index 70caea6..d526029 100644
> > > --- a/drivers/dma/xilinx/xilinx_vdma.c
> > > +++ b/drivers/dma/xilinx/xilinx_vdma.c
> > > @@ -44,6 +44,7 @@
> > >  #include 
> > >  #include 
> > >  #include 
> > > +#include 
> > >
> > >  #include "../dmaengine.h"
> > >
> > > @@ -352,6 +353,8 @@ struct xilinx_dma_chan {
> > >   * @flush_on_fsync: Flush on frame sync
> > >   * @ext_addr: Indicates 64 bit addressing is supported by dma device
> > >   * @dmatype: DMA ip type
> > > + * @clks:pointer to array of clocks
> > > + * @numclks: number of clocks available
> > >   */
> > >  struct xilinx_dma_device {
> > >   void __iomem *regs;
> > > @@ -362,6 +365,8 @@ struct xilinx_dma_device {
> > >   u32 flush_on_fsync;
> > >   bool ext_addr;
> > >   enum xdma_ip_type dmatype;
> > > + struct clk **clks;
> > > + int numclks;
> > >  };
> > >
> > >  /* Macros */
> > > @@ -1731,6 +1736,26 @@ int xilinx_vdma_channel_set_config(struct
> > > dma_chan *dchan,  }  EXPORT_SYMBOL(xilinx_vdma_channel_set_config);
> > >
> > > +static int xdma_clk_init(struct xilinx_dma_device *xdev, bool enable)
> > > +{
> > > + int i = 0, ret;
> > > +
> > > + for (i = 0; i < xdev->numclks; i++) {
> > > + if (enable) {
> > > + ret = clk_prepare_enable(xdev->clks[i]);
> > > + if (ret) {
> > > + dev_err(xdev->dev,
> > > + "failed to enable the axidma clock\n");
> > > + return ret;
> > > + }
> > > + } else {
> > > + clk_disable_unprepare(xdev->clks[i]);
> > > + }
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > >  /* 
> > > -
> > >   * Probe and remove
> > >   */
> > > @@ -1919,6 +1944,7 @@ static int xilinx_dma_probe(struct platform_device
> > *pdev)
> > >   struct resource *io;
> > >   u32 num_frames, addr_width;
> > >   int i, err;
> > > + const char *str;
> > >
> > >   /* Allocate and initialize the DMA engine structure */
> > >   xdev = devm_kzalloc(>dev, sizeof(*xdev), GFP_KERNEL); @@
> > > -1965,6 +1991,32 @@ static int xilinx_dma_probe(struct platform_device
> > *pdev)
> > >   /* Set the dma mask bits */
> > >   dma_set_mask(xdev->dev, DMA_BIT_MASK(addr_width));
> > >
> > > + xdev->numclks = of_property_count_strings(pdev->dev.of_node,
> > > +   "clock-names");
> > > + if (xdev->numclks > 0) {
> > > + xdev->clks = devm_kmalloc_array(>dev, xdev->numclks,
> > > + sizeof(struct clk *),
> > >

Re: [PATCH v2 2/2] dmaengine: vdma: Add clock support

[Sören Brinkmann]

On Wed, 2016-04-20 at 17:13:19 +0530, Kedareswara rao Appana wrote:
> Added basic clock support. The clocks are requested at probe
> and released at remove.
> 
> Signed-off-by: Kedareswara rao Appana 
> ---
> Changes for v2:
> --> None.
> 
>  drivers/dma/xilinx/xilinx_vdma.c | 56 
> 
>  1 file changed, 56 insertions(+)
> 
> diff --git a/drivers/dma/xilinx/xilinx_vdma.c 
> b/drivers/dma/xilinx/xilinx_vdma.c
> index 70caea6..d526029 100644
> --- a/drivers/dma/xilinx/xilinx_vdma.c
> +++ b/drivers/dma/xilinx/xilinx_vdma.c
> @@ -44,6 +44,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "../dmaengine.h"
>  
> @@ -352,6 +353,8 @@ struct xilinx_dma_chan {
>   * @flush_on_fsync: Flush on frame sync
>   * @ext_addr: Indicates 64 bit addressing is supported by dma device
>   * @dmatype: DMA ip type
> + * @clks:pointer to array of clocks
> + * @numclks: number of clocks available
>   */
>  struct xilinx_dma_device {
>   void __iomem *regs;
> @@ -362,6 +365,8 @@ struct xilinx_dma_device {
>   u32 flush_on_fsync;
>   bool ext_addr;
>   enum xdma_ip_type dmatype;
> + struct clk **clks;
> + int numclks;
>  };
>  
>  /* Macros */
> @@ -1731,6 +1736,26 @@ int xilinx_vdma_channel_set_config(struct dma_chan 
> *dchan,
>  }
>  EXPORT_SYMBOL(xilinx_vdma_channel_set_config);
>  
> +static int xdma_clk_init(struct xilinx_dma_device *xdev, bool enable)
> +{
> + int i = 0, ret;
> +
> + for (i = 0; i < xdev->numclks; i++) {
> + if (enable) {
> + ret = clk_prepare_enable(xdev->clks[i]);
> + if (ret) {
> + dev_err(xdev->dev,
> + "failed to enable the axidma clock\n");
> + return ret;
> + }
> + } else {
> + clk_disable_unprepare(xdev->clks[i]);
> + }
> + }
> +
> + return 0;
> +}
> +
>  /* 
> -
>   * Probe and remove
>   */
> @@ -1919,6 +1944,7 @@ static int xilinx_dma_probe(struct platform_device 
> *pdev)
>   struct resource *io;
>   u32 num_frames, addr_width;
>   int i, err;
> + const char *str;
>  
>   /* Allocate and initialize the DMA engine structure */
>   xdev = devm_kzalloc(>dev, sizeof(*xdev), GFP_KERNEL);
> @@ -1965,6 +1991,32 @@ static int xilinx_dma_probe(struct platform_device 
> *pdev)
>   /* Set the dma mask bits */
>   dma_set_mask(xdev->dev, DMA_BIT_MASK(addr_width));
>  
> + xdev->numclks = of_property_count_strings(pdev->dev.of_node,
> +   "clock-names");
> + if (xdev->numclks > 0) {
> + xdev->clks = devm_kmalloc_array(>dev, xdev->numclks,
> + sizeof(struct clk *),
> + GFP_KERNEL);
> + if (!xdev->clks)
> + return -ENOMEM;
> +
> + for (i = 0; i < xdev->numclks; i++) {
> + of_property_read_string_index(pdev->dev.of_node,
> +   "clock-names", i, );
> + xdev->clks[i] = devm_clk_get(xdev->dev, str);
> + if (IS_ERR(xdev->clks[i])) {
> + if (PTR_ERR(xdev->clks[i]) == -ENOENT)
> + xdev->clks[i] = NULL;
> + else
> + return PTR_ERR(xdev->clks[i]);
> + }
> + }
> +
> + err = xdma_clk_init(xdev, true);
> + if (err)
> + return err;
> + }

I guess this works, but the relation to the binding is a little loose,
IMHO. Instead of using the clock names from the binding this is just
using whatever is provided in DT and enabling it. Also, all the clocks
here are handled as optional feature, while binding - and I guess
reality too - have them as mandatory.
It would be nicer if the driver specifically asks for the clocks it
needs and return an error if a mandatory clock is missing.

Sören

Re: [PATCH v2 2/2] dmaengine: vdma: Add clock support

[Sören Brinkmann]

On Wed, 2016-04-20 at 17:13:19 +0530, Kedareswara rao Appana wrote:
> Added basic clock support. The clocks are requested at probe
> and released at remove.
> 
> Signed-off-by: Kedareswara rao Appana 
> ---
> Changes for v2:
> --> None.
> 
>  drivers/dma/xilinx/xilinx_vdma.c | 56 
> 
>  1 file changed, 56 insertions(+)
> 
> diff --git a/drivers/dma/xilinx/xilinx_vdma.c 
> b/drivers/dma/xilinx/xilinx_vdma.c
> index 70caea6..d526029 100644
> --- a/drivers/dma/xilinx/xilinx_vdma.c
> +++ b/drivers/dma/xilinx/xilinx_vdma.c
> @@ -44,6 +44,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include "../dmaengine.h"
>  
> @@ -352,6 +353,8 @@ struct xilinx_dma_chan {
>   * @flush_on_fsync: Flush on frame sync
>   * @ext_addr: Indicates 64 bit addressing is supported by dma device
>   * @dmatype: DMA ip type
> + * @clks:pointer to array of clocks
> + * @numclks: number of clocks available
>   */
>  struct xilinx_dma_device {
>   void __iomem *regs;
> @@ -362,6 +365,8 @@ struct xilinx_dma_device {
>   u32 flush_on_fsync;
>   bool ext_addr;
>   enum xdma_ip_type dmatype;
> + struct clk **clks;
> + int numclks;
>  };
>  
>  /* Macros */
> @@ -1731,6 +1736,26 @@ int xilinx_vdma_channel_set_config(struct dma_chan 
> *dchan,
>  }
>  EXPORT_SYMBOL(xilinx_vdma_channel_set_config);
>  
> +static int xdma_clk_init(struct xilinx_dma_device *xdev, bool enable)
> +{
> + int i = 0, ret;
> +
> + for (i = 0; i < xdev->numclks; i++) {
> + if (enable) {
> + ret = clk_prepare_enable(xdev->clks[i]);
> + if (ret) {
> + dev_err(xdev->dev,
> + "failed to enable the axidma clock\n");
> + return ret;
> + }
> + } else {
> + clk_disable_unprepare(xdev->clks[i]);
> + }
> + }
> +
> + return 0;
> +}
> +
>  /* 
> -
>   * Probe and remove
>   */
> @@ -1919,6 +1944,7 @@ static int xilinx_dma_probe(struct platform_device 
> *pdev)
>   struct resource *io;
>   u32 num_frames, addr_width;
>   int i, err;
> + const char *str;
>  
>   /* Allocate and initialize the DMA engine structure */
>   xdev = devm_kzalloc(>dev, sizeof(*xdev), GFP_KERNEL);
> @@ -1965,6 +1991,32 @@ static int xilinx_dma_probe(struct platform_device 
> *pdev)
>   /* Set the dma mask bits */
>   dma_set_mask(xdev->dev, DMA_BIT_MASK(addr_width));
>  
> + xdev->numclks = of_property_count_strings(pdev->dev.of_node,
> +   "clock-names");
> + if (xdev->numclks > 0) {
> + xdev->clks = devm_kmalloc_array(>dev, xdev->numclks,
> + sizeof(struct clk *),
> + GFP_KERNEL);
> + if (!xdev->clks)
> + return -ENOMEM;
> +
> + for (i = 0; i < xdev->numclks; i++) {
> + of_property_read_string_index(pdev->dev.of_node,
> +   "clock-names", i, );
> + xdev->clks[i] = devm_clk_get(xdev->dev, str);
> + if (IS_ERR(xdev->clks[i])) {
> + if (PTR_ERR(xdev->clks[i]) == -ENOENT)
> + xdev->clks[i] = NULL;
> + else
> + return PTR_ERR(xdev->clks[i]);
> + }
> + }
> +
> + err = xdma_clk_init(xdev, true);
> + if (err)
> + return err;
> + }

I guess this works, but the relation to the binding is a little loose,
IMHO. Instead of using the clock names from the binding this is just
using whatever is provided in DT and enabling it. Also, all the clocks
here are handled as optional feature, while binding - and I guess
reality too - have them as mandatory.
It would be nicer if the driver specifically asks for the clocks it
needs and return an error if a mandatory clock is missing.

Sören

Re: [PATCH v2 1/2] Documentation: DT: vdma: Add clock support for vdma

[Sören Brinkmann]

On Wed, 2016-04-20 at 17:13:18 +0530, Kedareswara rao Appana wrote:
> This patch updates the binding doc with clock description
> for vdma.
> 
> Signed-off-by: Kedareswara rao Appana <appa...@xilinx.com>
> ---
> Changes for v2:
> --> Listed down all the clocks supported by the h/w
> as suggested by the Datta.
> --> Used IP clock names instead of shortcut clock names.
> 
>  Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt 
> b/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> index fcc2b65..afe9eb7 100644
> --- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> +++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> @@ -21,6 +21,11 @@ Required properties:
>  - dma-channel child node: Should have at least one channel and can have up to
>   two channels per device. This node specifies the properties of each
>   DMA channel (see child node properties below).
> +- clocks: Input clock specifier. Refer to common clock bindings.
> +- clock-names: List of input clocks "s_axi_lite_aclk", "m_axi_mm2s_aclk"
> +"m_axi_s2mm_aclk", "m_axis_mm2s_aclk", "s_axis_s2mm_aclk"
> +(list of input cloks may vary based on the ip configuration.

s/cloks/clocks

Acked-by: Sören Brinkmann <soren.brinkm...@xilinx.com>

Sören

Re: [PATCH v2 1/2] Documentation: DT: vdma: Add clock support for vdma

[Sören Brinkmann]

On Wed, 2016-04-20 at 17:13:18 +0530, Kedareswara rao Appana wrote:
> This patch updates the binding doc with clock description
> for vdma.
> 
> Signed-off-by: Kedareswara rao Appana 
> ---
> Changes for v2:
> --> Listed down all the clocks supported by the h/w
> as suggested by the Datta.
> --> Used IP clock names instead of shortcut clock names.
> 
>  Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt 
> b/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> index fcc2b65..afe9eb7 100644
> --- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> +++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> @@ -21,6 +21,11 @@ Required properties:
>  - dma-channel child node: Should have at least one channel and can have up to
>   two channels per device. This node specifies the properties of each
>   DMA channel (see child node properties below).
> +- clocks: Input clock specifier. Refer to common clock bindings.
> +- clock-names: List of input clocks "s_axi_lite_aclk", "m_axi_mm2s_aclk"
> +"m_axi_s2mm_aclk", "m_axis_mm2s_aclk", "s_axis_s2mm_aclk"
> +(list of input cloks may vary based on the ip configuration.

s/cloks/clocks

Acked-by: Sören Brinkmann 

Sören

Re: [PATCH v3 1/5] Documentation: DT: vdma: Rename vdma-chan prefix to dma-chan

[Sören Brinkmann]

On Wed, 2016-04-06 at 22:13:59 +, Koul, Vinod wrote:
> On Wed, 2016-04-06 at 23:16 +0200, Lars-Peter Clausen wrote:
> > On 04/06/2016 06:25 PM, Appana Durga Kedareswara Rao wrote:
> > > a/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> > > > > +++
> > > > > b/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> > > > > @@ -24,8 +24,8 @@ Optional properties:
> > > > >   {3}, flush s2mm channel
> > > > > 
> > > > >  Required child node properties:
> > > > > -- compatible: It should be either "xlnx,axi-vdma-mm2s-channel"
> > > > > or
> > > > > - "xlnx,axi-vdma-s2mm-channel".
> > > > > +- compatible: It should be either "xlnx,axi-dma-mm2s-channel"
> > > > > or
> > > > > + "xlnx,axi-dma-s2mm-channel".
> > > > 
> > > > This change is not backwards compatible and breaks every user of
> > > > the current
> > > > binding.
> > > 
> > > This commit http://git.kernel.org/cgit/linux/kernel/git/vkoul/slave-
> > > dma.git/commit/?h=next=8e66e7d682b04f7141f8ae666908c8dcd7fc0bfa 
> > > Renames xilinx_vdma_ prefix to xilinx_dma which includes renaming of
> > > the above properties.
> 
> That patch changes driver from vdma to dma. It does not change property
> name!

It does. Unfortunately there are no line numbers on that website, hence here an 
excerpt
from the commit you mention:
@@ -1220,26 +1220,26 @@ static int xilinx_vdma_chan_probe(struct 
xilinx_vdma_device *xdev,
if (!has_dre)
xdev->common.copy_align = fls(width - 1);
 
-   if (of_device_is_compatible(node, "xlnx,axi-vdma-mm2s-channel")) {
+   if (of_device_is_compatible(node, "xlnx,axi-dma-mm2s-channel")) {
chan->direction = DMA_MEM_TO_DEV;
chan->id = 0;

Sören

Re: [PATCH v3 1/5] Documentation: DT: vdma: Rename vdma-chan prefix to dma-chan

[Sören Brinkmann]

On Wed, 2016-04-06 at 22:13:59 +, Koul, Vinod wrote:
> On Wed, 2016-04-06 at 23:16 +0200, Lars-Peter Clausen wrote:
> > On 04/06/2016 06:25 PM, Appana Durga Kedareswara Rao wrote:
> > > a/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> > > > > +++
> > > > > b/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> > > > > @@ -24,8 +24,8 @@ Optional properties:
> > > > >   {3}, flush s2mm channel
> > > > > 
> > > > >  Required child node properties:
> > > > > -- compatible: It should be either "xlnx,axi-vdma-mm2s-channel"
> > > > > or
> > > > > - "xlnx,axi-vdma-s2mm-channel".
> > > > > +- compatible: It should be either "xlnx,axi-dma-mm2s-channel"
> > > > > or
> > > > > + "xlnx,axi-dma-s2mm-channel".
> > > > 
> > > > This change is not backwards compatible and breaks every user of
> > > > the current
> > > > binding.
> > > 
> > > This commit http://git.kernel.org/cgit/linux/kernel/git/vkoul/slave-
> > > dma.git/commit/?h=next=8e66e7d682b04f7141f8ae666908c8dcd7fc0bfa 
> > > Renames xilinx_vdma_ prefix to xilinx_dma which includes renaming of
> > > the above properties.
> 
> That patch changes driver from vdma to dma. It does not change property
> name!

It does. Unfortunately there are no line numbers on that website, hence here an 
excerpt
from the commit you mention:
@@ -1220,26 +1220,26 @@ static int xilinx_vdma_chan_probe(struct 
xilinx_vdma_device *xdev,
if (!has_dre)
xdev->common.copy_align = fls(width - 1);
 
-   if (of_device_is_compatible(node, "xlnx,axi-vdma-mm2s-channel")) {
+   if (of_device_is_compatible(node, "xlnx,axi-dma-mm2s-channel")) {
chan->direction = DMA_MEM_TO_DEV;
chan->id = 0;

Sören

Re: [PATCH v3 1/5] Documentation: DT: vdma: Rename vdma-chan prefix to dma-chan

[Sören Brinkmann]

On Wed, 2016-04-06 at 21:45:31 +0530, Kedareswara rao Appana wrote:
> This patch renames the vdma-mm2s-channel/vdma-s2mm-channel
> property with dma-mm2s-channel/dma-s2mm-channel to sync with
> the driver.
> 
> Signed-off-by: Kedareswara rao Appana 
> ---
> Changes for v3:
> ---> New patch.
> 
>  Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt 
> b/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> index a86737c..762938f 100644
> --- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> +++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> @@ -24,8 +24,8 @@ Optional properties:
>   {3}, flush s2mm channel
>  
>  Required child node properties:
> -- compatible: It should be either "xlnx,axi-vdma-mm2s-channel" or
> - "xlnx,axi-vdma-s2mm-channel".
> +- compatible: It should be either "xlnx,axi-dma-mm2s-channel" or
> + "xlnx,axi-dma-s2mm-channel".

This change is not backwards compatible and breaks every user of the
current binding.

Sören

Re: [PATCH v3 1/5] Documentation: DT: vdma: Rename vdma-chan prefix to dma-chan

[Sören Brinkmann]

On Wed, 2016-04-06 at 21:45:31 +0530, Kedareswara rao Appana wrote:
> This patch renames the vdma-mm2s-channel/vdma-s2mm-channel
> property with dma-mm2s-channel/dma-s2mm-channel to sync with
> the driver.
> 
> Signed-off-by: Kedareswara rao Appana 
> ---
> Changes for v3:
> ---> New patch.
> 
>  Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt 
> b/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> index a86737c..762938f 100644
> --- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> +++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> @@ -24,8 +24,8 @@ Optional properties:
>   {3}, flush s2mm channel
>  
>  Required child node properties:
> -- compatible: It should be either "xlnx,axi-vdma-mm2s-channel" or
> - "xlnx,axi-vdma-s2mm-channel".
> +- compatible: It should be either "xlnx,axi-dma-mm2s-channel" or
> + "xlnx,axi-dma-s2mm-channel".

This change is not backwards compatible and breaks every user of the
current binding.

Sören

Re: [PATCH v2 5/6] Documentation: DT: vdma: update binding doc for AXI CDMA

[Sören Brinkmann]

On Sun, 2016-03-27 at 23:36:06 +0530, Kedareswara rao Appana wrote:
> This patch updates the device-tree binding doc for
> adding support for AXI CDMA.
> 
> Signed-off-by: Kedareswara rao Appana 
> ---
> ---> Modified commit message as suggested by Vinod.
> ---> Moved the patch to forward in the series as suggested by vinod.
> 
>  .../devicetree/bindings/dma/xilinx/xilinx_vdma.txt | 18 
> +-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt 
> b/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> index 5841421..2b0c12b 100644
> --- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> +++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> @@ -8,8 +8,12 @@ target devices. It can be configured to have one channel or 
> two channels.
>  If configured as two channels, one is to transmit to the device and another
>  is to receive from the device.
>  
> +Xilinx AXI CDMA engine, it does transfers between memory-mapped source
> +address and a memory-mapped destination address.
> +
>  Required properties:
> -- compatible: Should be "xlnx,axi-vdma-1.00.a" or "xlnx,axi-dma-1.00.a"
> +- compatible: Should be "xlnx,axi-vdma-1.00.a" or "xlnx,axi-dma-1.00.a" or
> +   "xlnx,axi-cdma-1.00.a""
>  - #dma-cells: Should be <1>, see "dmas" property below
>  - reg: Should contain VDMA registers location and length.
>  - xlnx,num-fstores: Should be the number of framebuffers as configured in 
> h/w.
> @@ -80,6 +84,18 @@ axi_dma_0: axidma@4040 {
>   } ;
>  } ;
>  
> +axi_cdma_0: axicdma@7e20 {
> +   compatible = "xlnx,axi-cdma-1.00.a";
> +   #dma-cells = <1>;
> +   reg = < 0x7e20 0x1 >;
> +   xlnx,addrwidth = <0x20>;
> +   dma-channel@7e20 {
> +   compatible = "xlnx,axi-dma-mm2s-channel";
> +   interrupts = < 0 55 4 >;
> +   xlnx,datawidth = <0x40>;
> +   } ;
> +} ;

As in the other patch, the node name should be 'dma-controller@...' and
the inconsistend spacing could be fixed.

Also, it seems this adds pretty much identical examples that just differ
in the compat string. Is that really needed?

Sören

Re: [PATCH v2 5/6] Documentation: DT: vdma: update binding doc for AXI CDMA

[Sören Brinkmann]

On Sun, 2016-03-27 at 23:36:06 +0530, Kedareswara rao Appana wrote:
> This patch updates the device-tree binding doc for
> adding support for AXI CDMA.
> 
> Signed-off-by: Kedareswara rao Appana 
> ---
> ---> Modified commit message as suggested by Vinod.
> ---> Moved the patch to forward in the series as suggested by vinod.
> 
>  .../devicetree/bindings/dma/xilinx/xilinx_vdma.txt | 18 
> +-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt 
> b/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> index 5841421..2b0c12b 100644
> --- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> +++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> @@ -8,8 +8,12 @@ target devices. It can be configured to have one channel or 
> two channels.
>  If configured as two channels, one is to transmit to the device and another
>  is to receive from the device.
>  
> +Xilinx AXI CDMA engine, it does transfers between memory-mapped source
> +address and a memory-mapped destination address.
> +
>  Required properties:
> -- compatible: Should be "xlnx,axi-vdma-1.00.a" or "xlnx,axi-dma-1.00.a"
> +- compatible: Should be "xlnx,axi-vdma-1.00.a" or "xlnx,axi-dma-1.00.a" or
> +   "xlnx,axi-cdma-1.00.a""
>  - #dma-cells: Should be <1>, see "dmas" property below
>  - reg: Should contain VDMA registers location and length.
>  - xlnx,num-fstores: Should be the number of framebuffers as configured in 
> h/w.
> @@ -80,6 +84,18 @@ axi_dma_0: axidma@4040 {
>   } ;
>  } ;
>  
> +axi_cdma_0: axicdma@7e20 {
> +   compatible = "xlnx,axi-cdma-1.00.a";
> +   #dma-cells = <1>;
> +   reg = < 0x7e20 0x1 >;
> +   xlnx,addrwidth = <0x20>;
> +   dma-channel@7e20 {
> +   compatible = "xlnx,axi-dma-mm2s-channel";
> +   interrupts = < 0 55 4 >;
> +   xlnx,datawidth = <0x40>;
> +   } ;
> +} ;

As in the other patch, the node name should be 'dma-controller@...' and
the inconsistend spacing could be fixed.

Also, it seems this adds pretty much identical examples that just differ
in the compat string. Is that really needed?

Sören

Re: [PATCH v2 3/6] Documentation: DT: vdma: update binding doc for AXI DMA

[Sören Brinkmann]

On Sun, 2016-03-27 at 23:36:05 +0530, Kedareswara rao Appana wrote:
> This patch updates the device-tree binding doc for
> adding support for AXI DMA.
> 
> Signed-off-by: Kedareswara rao Appana 
> ---
> Changes for v2:
> ---> Modified commit message as suggested by Vinod.
> ---> Moved the patch to forward in the series as suggested by vinod.
> 
>  .../devicetree/bindings/dma/xilinx/xilinx_vdma.txt | 22 
> +-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt 
> b/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> index a86737c..5841421 100644
> --- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> +++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> @@ -3,8 +3,13 @@ It can be configured to have one channel or two channels. If 
> configured
>  as two channels, one is to transmit to the video device and another is
>  to receive from the video device.
>  
> +Xilinx AXI DMA engine, it does transfers between memory and AXI4 stream
> +target devices. It can be configured to have one channel or two channels.
> +If configured as two channels, one is to transmit to the device and another
> +is to receive from the device.
> +
>  Required properties:
> -- compatible: Should be "xlnx,axi-vdma-1.00.a"
> +- compatible: Should be "xlnx,axi-vdma-1.00.a" or "xlnx,axi-dma-1.00.a"
>  - #dma-cells: Should be <1>, see "dmas" property below
>  - reg: Should contain VDMA registers location and length.
>  - xlnx,num-fstores: Should be the number of framebuffers as configured in 
> h/w.
> @@ -59,6 +64,21 @@ axi_vdma_0: axivdma@4003 {
>   } ;
>  } ;
>  
> +axi_dma_0: axidma@4040 {

The node names should follow the generic names, hence
'dma-controller@...'.

> + compatible = "xlnx,axi-dma-1.00.a";
> + #dma-cells = <1>;
> + reg = < 0x4040 0x1 >;
> + dma-channel@4040 {
> + compatible = "xlnx,axi-dma-mm2s-channel";
> + interrupts = < 0 59 4 >;
> + xlnx,datawidth = <0x40>;
> + } ;
> + dma-channel@40400030 {
> + compatible = "xlnx,axi-dma-s2mm-channel";
> + interrupts = < 0 58 4 >;
> + xlnx,datawidth = <0x40>;

Nit: The spacing around the '<' '>' is inconsistent. I'd just remove all
redundant spaces.

Sören

Re: [PATCH v2 3/6] Documentation: DT: vdma: update binding doc for AXI DMA

[Sören Brinkmann]

On Sun, 2016-03-27 at 23:36:05 +0530, Kedareswara rao Appana wrote:
> This patch updates the device-tree binding doc for
> adding support for AXI DMA.
> 
> Signed-off-by: Kedareswara rao Appana 
> ---
> Changes for v2:
> ---> Modified commit message as suggested by Vinod.
> ---> Moved the patch to forward in the series as suggested by vinod.
> 
>  .../devicetree/bindings/dma/xilinx/xilinx_vdma.txt | 22 
> +-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt 
> b/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> index a86737c..5841421 100644
> --- a/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> +++ b/Documentation/devicetree/bindings/dma/xilinx/xilinx_vdma.txt
> @@ -3,8 +3,13 @@ It can be configured to have one channel or two channels. If 
> configured
>  as two channels, one is to transmit to the video device and another is
>  to receive from the video device.
>  
> +Xilinx AXI DMA engine, it does transfers between memory and AXI4 stream
> +target devices. It can be configured to have one channel or two channels.
> +If configured as two channels, one is to transmit to the device and another
> +is to receive from the device.
> +
>  Required properties:
> -- compatible: Should be "xlnx,axi-vdma-1.00.a"
> +- compatible: Should be "xlnx,axi-vdma-1.00.a" or "xlnx,axi-dma-1.00.a"
>  - #dma-cells: Should be <1>, see "dmas" property below
>  - reg: Should contain VDMA registers location and length.
>  - xlnx,num-fstores: Should be the number of framebuffers as configured in 
> h/w.
> @@ -59,6 +64,21 @@ axi_vdma_0: axivdma@4003 {
>   } ;
>  } ;
>  
> +axi_dma_0: axidma@4040 {

The node names should follow the generic names, hence
'dma-controller@...'.

> + compatible = "xlnx,axi-dma-1.00.a";
> + #dma-cells = <1>;
> + reg = < 0x4040 0x1 >;
> + dma-channel@4040 {
> + compatible = "xlnx,axi-dma-mm2s-channel";
> + interrupts = < 0 59 4 >;
> + xlnx,datawidth = <0x40>;
> + } ;
> + dma-channel@40400030 {
> + compatible = "xlnx,axi-dma-s2mm-channel";
> + interrupts = < 0 58 4 >;
> + xlnx,datawidth = <0x40>;

Nit: The spacing around the '<' '>' is inconsistent. I'd just remove all
redundant spaces.

Sören

Re: [PATCH 38/41] clk: si5{14,351,70}: Remove CLK_IS_ROOT

[Sören Brinkmann]

On Tue, 2016-03-01 at 11:00:23 -0800, Stephen Boyd wrote:
> This flag is a no-op now. Remove usage of the flag.
> 
> Cc: Sebastian Hesselbarth <sebastian.hesselba...@gmail.com>
> Cc: Guenter Roeck <li...@roeck-us.net>
> Cc: Sören Brinkmann <soren.brinkm...@xilinx.com>
> Cc: Mike Looijmans <mike.looijm...@topic.nl>
> Signed-off-by: Stephen Boyd <sb...@codeaurora.org>
Reviewed-by: Sören Brinkmann <soren.brinkm...@xilinx.com>

Thanks,
Sören

Re: [PATCH 38/41] clk: si5{14,351,70}: Remove CLK_IS_ROOT

[Sören Brinkmann]

On Tue, 2016-03-01 at 11:00:23 -0800, Stephen Boyd wrote:
> This flag is a no-op now. Remove usage of the flag.
> 
> Cc: Sebastian Hesselbarth 
> Cc: Guenter Roeck 
> Cc: Sören Brinkmann 
> Cc: Mike Looijmans 
> Signed-off-by: Stephen Boyd 
Reviewed-by: Sören Brinkmann 

Thanks,
Sören

Re: [PATCH 24/41] clk: zynq: Remove CLK_IS_ROOT

[Sören Brinkmann]

On Tue, 2016-03-01 at 11:00:09 -0800, Stephen Boyd wrote:
> This flag is a no-op now. Remove usage of the flag.
> 
> Cc: Sören Brinkmann <soren.brinkm...@xilinx.com>
> Signed-off-by: Stephen Boyd <sb...@codeaurora.org>
Reviewed-by: Sören Brinkmann <soren.brinkm...@xilinx.com>

Thanks,
Sören

Re: [PATCH 24/41] clk: zynq: Remove CLK_IS_ROOT

[Sören Brinkmann]

On Tue, 2016-03-01 at 11:00:09 -0800, Stephen Boyd wrote:
> This flag is a no-op now. Remove usage of the flag.
> 
> Cc: Sören Brinkmann 
> Signed-off-by: Stephen Boyd 
Reviewed-by: Sören Brinkmann 

Thanks,
Sören

Re: [RFC PATCH] ARM: clocksource: make ARM_GLOBAL_TIMER selectable

[Sören Brinkmann]

On Fri, 2016-02-26 at 15:03:19 +0200, Grygorii Strashko wrote:
> On 02/05/2016 01:39 AM, Sören Brinkmann wrote:
> > On Thu, 2016-02-04 at 15:14:47 -0800, Moritz Fischer wrote:
> >> Hi Soeren,
> >>
> >> On Thu, Feb 4, 2016 at 2:41 PM, Sören Brinkmann
> >> <soren.brinkm...@xilinx.com> wrote:
> >>
> >>> But with this change the 'if !CPU_FREQ' becomes obsolete.
> >> I'm confused, could you explain that statement? You don't want people
> >> accidentally running with GT when CPU_FREQ is on, right?
> > 
> > Correct. But with this Kconfig rework you can just deselect it in
> > Kconfig. The generic HAVE_GT could always be selected.
> > 
> 
> 
> Don't know whom should i ask - but what will be the final conclusion here?
> Can it be merged?

I think we don't break anything either way. Would just be some
additional clean up to get rid of that mentioned constraint (which
doesn't really work well anyway in the multi-arch kernel). So, no real
objections to merging it from my side.

Thanks,
Sören

Re: [RFC PATCH] ARM: clocksource: make ARM_GLOBAL_TIMER selectable

[Sören Brinkmann]

On Fri, 2016-02-26 at 15:03:19 +0200, Grygorii Strashko wrote:
> On 02/05/2016 01:39 AM, Sören Brinkmann wrote:
> > On Thu, 2016-02-04 at 15:14:47 -0800, Moritz Fischer wrote:
> >> Hi Soeren,
> >>
> >> On Thu, Feb 4, 2016 at 2:41 PM, Sören Brinkmann
> >>  wrote:
> >>
> >>> But with this change the 'if !CPU_FREQ' becomes obsolete.
> >> I'm confused, could you explain that statement? You don't want people
> >> accidentally running with GT when CPU_FREQ is on, right?
> > 
> > Correct. But with this Kconfig rework you can just deselect it in
> > Kconfig. The generic HAVE_GT could always be selected.
> > 
> 
> 
> Don't know whom should i ask - but what will be the final conclusion here?
> Can it be merged?

I think we don't break anything either way. Would just be some
additional clean up to get rid of that mentioned constraint (which
doesn't really work well anyway in the multi-arch kernel). So, no real
objections to merging it from my side.

Thanks,
Sören

Re: [PATCH 50/50] pinctrl: zynq: Use devm_pinctrl_register() for pinctrl registration

[Sören Brinkmann]

On Wed, 2016-02-24 at 18:46:15 +0530, Laxman Dewangan wrote:
> Use devm_pinctrl_register() for pin control registration and
> remove the need of .remove callback.
> 
> Signed-off-by: Laxman Dewangan <ldewan...@nvidia.com>
> Cc: Michal Simek <michal.si...@xilinx.com>
> Cc: Sören Brinkmann <soren.brinkm...@xilinx.com>
> Cc: linux-g...@vger.kernel.org
> Cc: linux-arm-ker...@lists.infradead.org
Acked-by: Sören Brinkmann <soren.brinkm...@xilinx.com>

Sören

Re: [PATCH 50/50] pinctrl: zynq: Use devm_pinctrl_register() for pinctrl registration

[Sören Brinkmann]

On Wed, 2016-02-24 at 18:46:15 +0530, Laxman Dewangan wrote:
> Use devm_pinctrl_register() for pin control registration and
> remove the need of .remove callback.
> 
> Signed-off-by: Laxman Dewangan 
> Cc: Michal Simek 
> Cc: Sören Brinkmann 
> Cc: linux-g...@vger.kernel.org
> Cc: linux-arm-ker...@lists.infradead.org
Acked-by: Sören Brinkmann 

Sören

Re: [RFC PATCH] ARM: clocksource: make ARM_GLOBAL_TIMER selectable

[Sören Brinkmann]

On Thu, 2016-02-04 at 15:14:47 -0800, Moritz Fischer wrote:
> Hi Soeren,
> 
> On Thu, Feb 4, 2016 at 2:41 PM, Sören Brinkmann
>  wrote:
> 
> > But with this change the 'if !CPU_FREQ' becomes obsolete.
> I'm confused, could you explain that statement? You don't want people
> accidentally running with GT when CPU_FREQ is on, right?

Correct. But with this Kconfig rework you can just deselect it in
Kconfig. The generic HAVE_GT could always be selected.

Sören

Re: [RFC PATCH] ARM: clocksource: make ARM_GLOBAL_TIMER selectable

[Sören Brinkmann]

On Thu, 2016-02-04 at 20:20:17 +0200, Grygorii Strashko wrote:
> This patch intended to fix following cases: 
> - SoC-A has ARM GT, defines DT node for ARM GT and selects ARM_GLOBAL_TIMER
> statically in Kconfig file. SoC-B has ARM GT and defines DT node for ARM GT,
> but do not selects ARM_GLOBAL_TIMER statically in Kconfig file. In case of
> multiplatform build ARM GT will be implicitly enabled for SoC-B.
> 
> - There is no way to disable ARM GT without modifying Kconfig file,
> once ARM_GLOBAL_TIMER is selected statically in Kconfig file.
> 
> Hence, fix above case by defining both HAVE_ARM_GLOBAL_TIMER and
> ARM_GLOBAL_TIMER as recommended by 'Adding common features and make
> the usage configurable' section in kconfig-language.txt. All places in
> ARM folder where ARM_GLOBAL_TIMER was used now replaced on
> HAVE_ARM_GLOBAL_TIMER.
> 
> Cc: Florian Fainelli 
> Cc: Russell King 
> Cc: Wei Xu 
> Cc: Shawn Guo 
> Cc: Sascha Hauer 
> Cc: Srinivas Kandagatla 
> Cc: Maxime Coquelin 
> Cc: Masahiro Yamada 
> Cc: Liviu Dudau 
> Cc: Sudeep Holla 
> Cc: Jun Nie 
> Cc: Michal Simek 
> Cc: "Sören Brinkmann" 
> Cc: Daniel Lezcano 
> 
> Signed-off-by: Grygorii Strashko 
> ---
[...]
> diff --git a/arch/arm/mach-zynq/Kconfig b/arch/arm/mach-zynq/Kconfig
> index fd0aeeb..3165720 100644
> --- a/arch/arm/mach-zynq/Kconfig
> +++ b/arch/arm/mach-zynq/Kconfig
> @@ -5,7 +5,7 @@ config ARCH_ZYNQ
>   select ARCH_SUPPORTS_BIG_ENDIAN
>   select ARM_AMBA
>   select ARM_GIC
> - select ARM_GLOBAL_TIMER if !CPU_FREQ
> + select HAVE_ARM_GLOBAL_TIMER if !CPU_FREQ

We actually have this issue, as we don't want to use GT when CPU_FREQ is
enabled. But with this change the 'if !CPU_FREQ' becomes obsolete.

Acked-by: Sören Brinkmann 

Sören

Re: [RFC PATCH] ARM: clocksource: make ARM_GLOBAL_TIMER selectable

[Sören Brinkmann]

On Thu, 2016-02-04 at 15:14:47 -0800, Moritz Fischer wrote:
> Hi Soeren,
> 
> On Thu, Feb 4, 2016 at 2:41 PM, Sören Brinkmann
> <soren.brinkm...@xilinx.com> wrote:
> 
> > But with this change the 'if !CPU_FREQ' becomes obsolete.
> I'm confused, could you explain that statement? You don't want people
> accidentally running with GT when CPU_FREQ is on, right?

Correct. But with this Kconfig rework you can just deselect it in
Kconfig. The generic HAVE_GT could always be selected.

Sören

Re: [RFC PATCH] ARM: clocksource: make ARM_GLOBAL_TIMER selectable

[Sören Brinkmann]

On Thu, 2016-02-04 at 20:20:17 +0200, Grygorii Strashko wrote:
> This patch intended to fix following cases: 
> - SoC-A has ARM GT, defines DT node for ARM GT and selects ARM_GLOBAL_TIMER
> statically in Kconfig file. SoC-B has ARM GT and defines DT node for ARM GT,
> but do not selects ARM_GLOBAL_TIMER statically in Kconfig file. In case of
> multiplatform build ARM GT will be implicitly enabled for SoC-B.
> 
> - There is no way to disable ARM GT without modifying Kconfig file,
> once ARM_GLOBAL_TIMER is selected statically in Kconfig file.
> 
> Hence, fix above case by defining both HAVE_ARM_GLOBAL_TIMER and
> ARM_GLOBAL_TIMER as recommended by 'Adding common features and make
> the usage configurable' section in kconfig-language.txt. All places in
> ARM folder where ARM_GLOBAL_TIMER was used now replaced on
> HAVE_ARM_GLOBAL_TIMER.
> 
> Cc: Florian Fainelli <f.faine...@gmail.com>
> Cc: Russell King <li...@arm.linux.org.uk>
> Cc: Wei Xu <xuw...@hisilicon.com>
> Cc: Shawn Guo <shawn...@kernel.org>
> Cc: Sascha Hauer <ker...@pengutronix.de>
> Cc: Srinivas Kandagatla <srinivas.kandaga...@gmail.com>
> Cc: Maxime Coquelin <maxime.coque...@st.com>
> Cc: Masahiro Yamada <yamada.masah...@socionext.com>
> Cc: Liviu Dudau <liviu.du...@arm.com>
> Cc: Sudeep Holla <sudeep.ho...@arm.com>
> Cc: Jun Nie <jun@linaro.org>
> Cc: Michal Simek <michal.si...@xilinx.com>
> Cc: "Sören Brinkmann" <soren.brinkm...@xilinx.com>
> Cc: Daniel Lezcano <daniel.lezc...@linaro.org>
> 
> Signed-off-by: Grygorii Strashko <grygorii.stras...@ti.com>
> ---
[...]
> diff --git a/arch/arm/mach-zynq/Kconfig b/arch/arm/mach-zynq/Kconfig
> index fd0aeeb..3165720 100644
> --- a/arch/arm/mach-zynq/Kconfig
> +++ b/arch/arm/mach-zynq/Kconfig
> @@ -5,7 +5,7 @@ config ARCH_ZYNQ
>   select ARCH_SUPPORTS_BIG_ENDIAN
>   select ARM_AMBA
>   select ARM_GIC
> - select ARM_GLOBAL_TIMER if !CPU_FREQ
> + select HAVE_ARM_GLOBAL_TIMER if !CPU_FREQ

We actually have this issue, as we don't want to use GT when CPU_FREQ is
enabled. But with this change the 'if !CPU_FREQ' becomes obsolete.

Acked-by: Sören Brinkmann <soren.brinkm...@xilinx.com>

Sören

Re: [PATCH LINUX 0/6] Second part of xuartps fixes

[Sören Brinkmann]

ping? Any comments?

Thanks,
Sören

On Mon, 2016-01-11 at 17:41:35 -0800, Soren Brinkmann wrote:
> Hi,
> 
> this is the second part of fixes for xuartps that evolved from this
> series: https://lkml.org/lkml/2015/12/26/26.
> 
> This series, obviously, depends on the patches mentioned above. It
> includes several minor improvements and refactoring and the refactoring
> of the IRQ management, which prevents lock ups that could happen when
> RX-related IRQs fired while the receiver was disabled.
> 
>   Thanks,
>   Sören
> 
> Sören Brinkmann (6):
>   tty: xuartps: Move request_irq to after setting up the HW
>   tty: xuartps: Refactor IRQ handling
>   tty: xuartps: Cleanup: Reformat if-else
>   tty: xuartps: Improve sysrq handling
>   tty: xuartps: Remove '_OFFSET' suffix from #defines
>   tty: xuartps: Consolidate TX handling
> 
>  drivers/tty/serial/xilinx_uartps.c | 460 
> +
>  1 file changed, 216 insertions(+), 244 deletions(-)
> 
> -- 
> 2.7.0.3.g497ea1e
> 

Re: [PATCH LINUX 0/6] Second part of xuartps fixes

[Sören Brinkmann]

ping? Any comments?

Thanks,
Sören

On Mon, 2016-01-11 at 17:41:35 -0800, Soren Brinkmann wrote:
> Hi,
> 
> this is the second part of fixes for xuartps that evolved from this
> series: https://lkml.org/lkml/2015/12/26/26.
> 
> This series, obviously, depends on the patches mentioned above. It
> includes several minor improvements and refactoring and the refactoring
> of the IRQ management, which prevents lock ups that could happen when
> RX-related IRQs fired while the receiver was disabled.
> 
>   Thanks,
>   Sören
> 
> Sören Brinkmann (6):
>   tty: xuartps: Move request_irq to after setting up the HW
>   tty: xuartps: Refactor IRQ handling
>   tty: xuartps: Cleanup: Reformat if-else
>   tty: xuartps: Improve sysrq handling
>   tty: xuartps: Remove '_OFFSET' suffix from #defines
>   tty: xuartps: Consolidate TX handling
> 
>  drivers/tty/serial/xilinx_uartps.c | 460 
> +
>  1 file changed, 216 insertions(+), 244 deletions(-)
> 
> -- 
> 2.7.0.3.g497ea1e
> 

Re: [PATCH v4 05/13] mmc: sdhci-of-arasan: fix clk issue in sdhci_arasan_remove()

[Sören Brinkmann]

On Tue, 2016-01-26 at 06:15PM +0800, Jisheng Zhang wrote:
> sdhci_pltfm_unregister() could operate host's registers, it will cause
> problems if the clk is already disabled and unprepared. Fix this issue
> by moving the clk_disable_unprepare() call to the end of remove
> function.
> 
> Signed-off-by: Jisheng Zhang 
Acked-by: Sören Brinkmann 

Sören

Re: [PATCH v4 05/13] mmc: sdhci-of-arasan: fix clk issue in sdhci_arasan_remove()

[Sören Brinkmann]

On Tue, 2016-01-26 at 06:15PM +0800, Jisheng Zhang wrote:
> sdhci_pltfm_unregister() could operate host's registers, it will cause
> problems if the clk is already disabled and unprepared. Fix this issue
> by moving the clk_disable_unprepare() call to the end of remove
> function.
> 
> Signed-off-by: Jisheng Zhang <jszh...@marvell.com>
Acked-by: Sören Brinkmann <soren.brinkm...@xilinx.com>

Sören

Re: [RFC LINUX PATCH] dt: xilinx: xadc: provision to control clock frequency

[Sören Brinkmann]

On Wed, 2015-12-23 at 03:58PM +0530, Ranjit Waghmode wrote:
> This patch adds parameter to the xilinx-xadc node for controlling
> clock frequency.
> 
> Following are the possible options for user to control the frequency:
>   * 00 : 1/2 of clock frequency
>   * 01 : 1/4 of clock frequency
>   * 10 : 1/8 of clock frequency
>   * 11 : 1/16 of clock frequency

How is this chosen? Are these just arbitrary values that you need for
some use-case? Or are these the options HW allows?

> 
> So this patch adds parameter tck-rate to set user defined values from
> above pool to control the clock frequency.

This is no longer describing HW, but configuring driver/HW. Why does
this have to be in DT? Why can't the driver request/set the frequency as
operation requires it?

Thanks,
Sören
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC LINUX PATCH] dt: xilinx: xadc: provision to control clock frequency

[Sören Brinkmann]

On Wed, 2015-12-23 at 03:58PM +0530, Ranjit Waghmode wrote:
> This patch adds parameter to the xilinx-xadc node for controlling
> clock frequency.
> 
> Following are the possible options for user to control the frequency:
>   * 00 : 1/2 of clock frequency
>   * 01 : 1/4 of clock frequency
>   * 10 : 1/8 of clock frequency
>   * 11 : 1/16 of clock frequency

How is this chosen? Are these just arbitrary values that you need for
some use-case? Or are these the options HW allows?

> 
> So this patch adds parameter tck-rate to set user defined values from
> above pool to control the clock frequency.

This is no longer describing HW, but configuring driver/HW. Why does
this have to be in DT? Why can't the driver request/set the frequency as
operation requires it?

Thanks,
Sören
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/