Re: [Patch v6 3/7] slimbus: qcom: Add Qualcomm Slimbus controller driver

[Srinivas Kandagatla]

Thanks for Review Comments.

On 18/10/17 08:27, Bjorn Andersson wrote:

On Fri 06 Oct 08:51 PDT 2017, srinivas.kandaga...@linaro.org wrote:


From: Sagar Dharia 

This controller driver programs manager, interface, and framer
devices for Qualcomm's slimbus HW block.
Manager component currently implements logical address setting,
and messaging interface.
Interface device reports bus synchronization information, and framer
device clocks the bus from the time it's woken up, until clock-pause
is executed by the manager device.

Signed-off-by: Sagar Dharia 
Signed-off-by: Srinivas Kandagatla 
---
  .../devicetree/bindings/slimbus/slim-qcom-ctrl.txt |  43 ++
  drivers/slimbus/Kconfig|   9 +
  drivers/slimbus/Makefile   |   3 +
  drivers/slimbus/slim-qcom-ctrl.c   | 594 +
  drivers/slimbus/slim-qcom.h|  63 +++
  5 files changed, 712 insertions(+)
  create mode 100644 
Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
  create mode 100644 drivers/slimbus/slim-qcom-ctrl.c
  create mode 100644 drivers/slimbus/slim-qcom.h

diff --git a/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt 
b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
new file mode 100644
index 000..081110d
--- /dev/null
+++ b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
@@ -0,0 +1,43 @@
+Qualcomm SLIMBUS controller
+This controller is used if applications processor driver controls slimbus
+master component.
+
+Required properties:
+
+ - #address-cells - refer to Documentation/devicetree/bindings/slimbus/bus.txt
+ - #size-cells - refer to Documentation/devicetree/bindings/slimbus/bus.txt
+
+ - reg : Offset and length of the register region(s) for the device
+ - reg-names : Register region name(s) referenced in reg above
+Required register resource entries are:
+"ctrl": Physical adderess of controller register blocks
+ - compatible : should be "qcom,-slim" for SOC specific compatible or


As you implementation is only compatible with "qcom,slim" this should be
"and" not "or".

yep,




+   "qcom,slim" if using generic qcom SLIM IP.
+ - interrupts : Interrupt number used by this controller
+ - clocks : Interface and core clocks used by this slimbus controller
+ - clock-names : Required clock-name entries are:
+   "iface_clk" : Interface clock for this controller
+   "core_clk" : Interrupt for controller core's BAM
+
+
+Optional property:
+ - reg entry for slew rate : If slew rate control register is provided, this
+   entry should be used.
+ - reg-name for slew rate: "slew"
+
+Example:
+   slim@2808 {
+   compatible = "qcom,slim";
+   #address-cells = <4>;
+   #size-cells = <0>;
+   reg = <0x2808 0x2000>, <0x80207C 4>;
+   reg-names = "ctrl", "slew";
+   interrupts = <0 33 0>;
+   clocks = < SLIMBUS_SRC>, < AUDIO_SLIMBUS_CLK>;
+   clock-names = "iface_clk", "core_clk";
+
+   codec: wcd9310@1{
+   compatible = "slim217,60";
+   reg = <1 0>;
+   };
+   };
diff --git a/drivers/slimbus/Kconfig b/drivers/slimbus/Kconfig
index f0b118a..438773e 100644
--- a/drivers/slimbus/Kconfig
+++ b/drivers/slimbus/Kconfig
@@ -9,3 +9,12 @@ menuconfig SLIMBUS
  
  	  If unsure, choose N.

yep.

  
+if SLIMBUS

+config SLIM_QCOM_CTRL
+   tristate "Qualcomm Slimbus Manager Component"
+   depends on SLIMBUS
+   help
+ Select driver if Qualcomm's Slimbus Manager Component is
+ programmed using Linux kernel.
+
+endif
diff --git a/drivers/slimbus/Makefile b/drivers/slimbus/Makefile
index bd1d35e..ed8521a 100644
--- a/drivers/slimbus/Makefile
+++ b/drivers/slimbus/Makefile
@@ -3,3 +3,6 @@
  #
  obj-$(CONFIG_SLIMBUS) += slimbus.o
  slimbus-y := slim-core.o slim-messaging.o
+
+#Controllers
+obj-$(CONFIG_SLIM_QCOM_CTRL)   += slim-qcom-ctrl.o
diff --git a/drivers/slimbus/slim-qcom-ctrl.c b/drivers/slimbus/slim-qcom-ctrl.c
new file mode 100644
index 000..d0574e1
--- /dev/null
+++ b/drivers/slimbus/slim-qcom-ctrl.c

..

+#include "slim-qcom.h"
+
+#define MSM_SLIM_NAME  "msm_slim_ctrl"


Just put this string in the driver struct.


yes..




+
+/* Manager registers */
+#defineMGR_CFG 0x200

...



+
+static int msm_slim_queue_tx(struct msm_slim_ctrl *dev, u32 *buf, u8 len,
+u32 tx_reg)


Use void * for buf.


okay.


+{
+   int i;
+
+   for (i = 0; i < (len + 3) >> 2; i++) {


If len is in bytes then this looks like you will read outside the
buffer. If buf is guaranteed to be 4-byte aligned make "len" number of
32-bit entries in the array.


+   dev_dbg(dev->dev, "AHB TX data:0x%x\n", 

Re: [Patch v6 3/7] slimbus: qcom: Add Qualcomm Slimbus controller driver

[Srinivas Kandagatla]

Thanks for Review Comments.

On 18/10/17 08:27, Bjorn Andersson wrote:

On Fri 06 Oct 08:51 PDT 2017, srinivas.kandaga...@linaro.org wrote:


From: Sagar Dharia 

This controller driver programs manager, interface, and framer
devices for Qualcomm's slimbus HW block.
Manager component currently implements logical address setting,
and messaging interface.
Interface device reports bus synchronization information, and framer
device clocks the bus from the time it's woken up, until clock-pause
is executed by the manager device.

Signed-off-by: Sagar Dharia 
Signed-off-by: Srinivas Kandagatla 
---
  .../devicetree/bindings/slimbus/slim-qcom-ctrl.txt |  43 ++
  drivers/slimbus/Kconfig|   9 +
  drivers/slimbus/Makefile   |   3 +
  drivers/slimbus/slim-qcom-ctrl.c   | 594 +
  drivers/slimbus/slim-qcom.h|  63 +++
  5 files changed, 712 insertions(+)
  create mode 100644 
Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
  create mode 100644 drivers/slimbus/slim-qcom-ctrl.c
  create mode 100644 drivers/slimbus/slim-qcom.h

diff --git a/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt 
b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
new file mode 100644
index 000..081110d
--- /dev/null
+++ b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
@@ -0,0 +1,43 @@
+Qualcomm SLIMBUS controller
+This controller is used if applications processor driver controls slimbus
+master component.
+
+Required properties:
+
+ - #address-cells - refer to Documentation/devicetree/bindings/slimbus/bus.txt
+ - #size-cells - refer to Documentation/devicetree/bindings/slimbus/bus.txt
+
+ - reg : Offset and length of the register region(s) for the device
+ - reg-names : Register region name(s) referenced in reg above
+Required register resource entries are:
+"ctrl": Physical adderess of controller register blocks
+ - compatible : should be "qcom,-slim" for SOC specific compatible or


As you implementation is only compatible with "qcom,slim" this should be
"and" not "or".

yep,




+   "qcom,slim" if using generic qcom SLIM IP.
+ - interrupts : Interrupt number used by this controller
+ - clocks : Interface and core clocks used by this slimbus controller
+ - clock-names : Required clock-name entries are:
+   "iface_clk" : Interface clock for this controller
+   "core_clk" : Interrupt for controller core's BAM
+
+
+Optional property:
+ - reg entry for slew rate : If slew rate control register is provided, this
+   entry should be used.
+ - reg-name for slew rate: "slew"
+
+Example:
+   slim@2808 {
+   compatible = "qcom,slim";
+   #address-cells = <4>;
+   #size-cells = <0>;
+   reg = <0x2808 0x2000>, <0x80207C 4>;
+   reg-names = "ctrl", "slew";
+   interrupts = <0 33 0>;
+   clocks = < SLIMBUS_SRC>, < AUDIO_SLIMBUS_CLK>;
+   clock-names = "iface_clk", "core_clk";
+
+   codec: wcd9310@1{
+   compatible = "slim217,60";
+   reg = <1 0>;
+   };
+   };
diff --git a/drivers/slimbus/Kconfig b/drivers/slimbus/Kconfig
index f0b118a..438773e 100644
--- a/drivers/slimbus/Kconfig
+++ b/drivers/slimbus/Kconfig
@@ -9,3 +9,12 @@ menuconfig SLIMBUS
  
  	  If unsure, choose N.

yep.

  
+if SLIMBUS

+config SLIM_QCOM_CTRL
+   tristate "Qualcomm Slimbus Manager Component"
+   depends on SLIMBUS
+   help
+ Select driver if Qualcomm's Slimbus Manager Component is
+ programmed using Linux kernel.
+
+endif
diff --git a/drivers/slimbus/Makefile b/drivers/slimbus/Makefile
index bd1d35e..ed8521a 100644
--- a/drivers/slimbus/Makefile
+++ b/drivers/slimbus/Makefile
@@ -3,3 +3,6 @@
  #
  obj-$(CONFIG_SLIMBUS) += slimbus.o
  slimbus-y := slim-core.o slim-messaging.o
+
+#Controllers
+obj-$(CONFIG_SLIM_QCOM_CTRL)   += slim-qcom-ctrl.o
diff --git a/drivers/slimbus/slim-qcom-ctrl.c b/drivers/slimbus/slim-qcom-ctrl.c
new file mode 100644
index 000..d0574e1
--- /dev/null
+++ b/drivers/slimbus/slim-qcom-ctrl.c

..

+#include "slim-qcom.h"
+
+#define MSM_SLIM_NAME  "msm_slim_ctrl"


Just put this string in the driver struct.


yes..




+
+/* Manager registers */
+#defineMGR_CFG 0x200

...



+
+static int msm_slim_queue_tx(struct msm_slim_ctrl *dev, u32 *buf, u8 len,
+u32 tx_reg)


Use void * for buf.


okay.


+{
+   int i;
+
+   for (i = 0; i < (len + 3) >> 2; i++) {


If len is in bytes then this looks like you will read outside the
buffer. If buf is guaranteed to be 4-byte aligned make "len" number of
32-bit entries in the array.


+   dev_dbg(dev->dev, "AHB TX data:0x%x\n", buf[i]);


Drop the debug print, if anything use print_hex_dump to get the whole

Re: [Patch v6 3/7] slimbus: qcom: Add Qualcomm Slimbus controller driver

[Bjorn Andersson]

On Fri 06 Oct 08:51 PDT 2017, srinivas.kandaga...@linaro.org wrote:

> From: Sagar Dharia 
> 
> This controller driver programs manager, interface, and framer
> devices for Qualcomm's slimbus HW block.
> Manager component currently implements logical address setting,
> and messaging interface.
> Interface device reports bus synchronization information, and framer
> device clocks the bus from the time it's woken up, until clock-pause
> is executed by the manager device.
> 
> Signed-off-by: Sagar Dharia 
> Signed-off-by: Srinivas Kandagatla 
> ---
>  .../devicetree/bindings/slimbus/slim-qcom-ctrl.txt |  43 ++
>  drivers/slimbus/Kconfig|   9 +
>  drivers/slimbus/Makefile   |   3 +
>  drivers/slimbus/slim-qcom-ctrl.c   | 594 
> +
>  drivers/slimbus/slim-qcom.h|  63 +++
>  5 files changed, 712 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
>  create mode 100644 drivers/slimbus/slim-qcom-ctrl.c
>  create mode 100644 drivers/slimbus/slim-qcom.h
> 
> diff --git a/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt 
> b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
> new file mode 100644
> index 000..081110d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
> @@ -0,0 +1,43 @@
> +Qualcomm SLIMBUS controller
> +This controller is used if applications processor driver controls slimbus
> +master component.
> +
> +Required properties:
> +
> + - #address-cells - refer to 
> Documentation/devicetree/bindings/slimbus/bus.txt
> + - #size-cells   - refer to 
> Documentation/devicetree/bindings/slimbus/bus.txt
> +
> + - reg : Offset and length of the register region(s) for the device
> + - reg-names : Register region name(s) referenced in reg above
> +  Required register resource entries are:
> +  "ctrl": Physical adderess of controller register blocks
> + - compatible : should be "qcom,-slim" for SOC specific compatible 
> or

As you implementation is only compatible with "qcom,slim" this should be
"and" not "or".

> + "qcom,slim" if using generic qcom SLIM IP.
> + - interrupts : Interrupt number used by this controller
> + - clocks : Interface and core clocks used by this slimbus controller
> + - clock-names : Required clock-name entries are:
> + "iface_clk" : Interface clock for this controller
> + "core_clk" : Interrupt for controller core's BAM
> +
> +
> +Optional property:
> + - reg entry for slew rate : If slew rate control register is provided, this
> + entry should be used.
> + - reg-name for slew rate: "slew"
> +
> +Example:
> + slim@2808 {
> + compatible = "qcom,slim";
> + #address-cells = <4>;
> + #size-cells = <0>;
> + reg = <0x2808 0x2000>, <0x80207C 4>;
> + reg-names = "ctrl", "slew";
> + interrupts = <0 33 0>;
> + clocks = < SLIMBUS_SRC>, < AUDIO_SLIMBUS_CLK>;
> + clock-names = "iface_clk", "core_clk";
> +
> + codec: wcd9310@1{
> + compatible = "slim217,60";
> + reg = <1 0>;
> + };
> + };
> diff --git a/drivers/slimbus/Kconfig b/drivers/slimbus/Kconfig
> index f0b118a..438773e 100644
> --- a/drivers/slimbus/Kconfig
> +++ b/drivers/slimbus/Kconfig
> @@ -9,3 +9,12 @@ menuconfig SLIMBUS
>  
> If unsure, choose N.
>  
> +if SLIMBUS
> +config SLIM_QCOM_CTRL
> + tristate "Qualcomm Slimbus Manager Component"
> + depends on SLIMBUS
> + help
> +   Select driver if Qualcomm's Slimbus Manager Component is
> +   programmed using Linux kernel.
> +
> +endif
> diff --git a/drivers/slimbus/Makefile b/drivers/slimbus/Makefile
> index bd1d35e..ed8521a 100644
> --- a/drivers/slimbus/Makefile
> +++ b/drivers/slimbus/Makefile
> @@ -3,3 +3,6 @@
>  #
>  obj-$(CONFIG_SLIMBUS)+= slimbus.o
>  slimbus-y:= slim-core.o slim-messaging.o
> +
> +#Controllers
> +obj-$(CONFIG_SLIM_QCOM_CTRL) += slim-qcom-ctrl.o
> diff --git a/drivers/slimbus/slim-qcom-ctrl.c 
> b/drivers/slimbus/slim-qcom-ctrl.c
> new file mode 100644
> index 000..d0574e1
> --- /dev/null
> +++ b/drivers/slimbus/slim-qcom-ctrl.c
> @@ -0,0 +1,594 @@
> +/* Copyright (c) 2011-2016, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * 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 

Re: [Patch v6 3/7] slimbus: qcom: Add Qualcomm Slimbus controller driver

[Bjorn Andersson]

On Fri 06 Oct 08:51 PDT 2017, srinivas.kandaga...@linaro.org wrote:

> From: Sagar Dharia 
> 
> This controller driver programs manager, interface, and framer
> devices for Qualcomm's slimbus HW block.
> Manager component currently implements logical address setting,
> and messaging interface.
> Interface device reports bus synchronization information, and framer
> device clocks the bus from the time it's woken up, until clock-pause
> is executed by the manager device.
> 
> Signed-off-by: Sagar Dharia 
> Signed-off-by: Srinivas Kandagatla 
> ---
>  .../devicetree/bindings/slimbus/slim-qcom-ctrl.txt |  43 ++
>  drivers/slimbus/Kconfig|   9 +
>  drivers/slimbus/Makefile   |   3 +
>  drivers/slimbus/slim-qcom-ctrl.c   | 594 
> +
>  drivers/slimbus/slim-qcom.h|  63 +++
>  5 files changed, 712 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
>  create mode 100644 drivers/slimbus/slim-qcom-ctrl.c
>  create mode 100644 drivers/slimbus/slim-qcom.h
> 
> diff --git a/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt 
> b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
> new file mode 100644
> index 000..081110d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
> @@ -0,0 +1,43 @@
> +Qualcomm SLIMBUS controller
> +This controller is used if applications processor driver controls slimbus
> +master component.
> +
> +Required properties:
> +
> + - #address-cells - refer to 
> Documentation/devicetree/bindings/slimbus/bus.txt
> + - #size-cells   - refer to 
> Documentation/devicetree/bindings/slimbus/bus.txt
> +
> + - reg : Offset and length of the register region(s) for the device
> + - reg-names : Register region name(s) referenced in reg above
> +  Required register resource entries are:
> +  "ctrl": Physical adderess of controller register blocks
> + - compatible : should be "qcom,-slim" for SOC specific compatible 
> or

As you implementation is only compatible with "qcom,slim" this should be
"and" not "or".

> + "qcom,slim" if using generic qcom SLIM IP.
> + - interrupts : Interrupt number used by this controller
> + - clocks : Interface and core clocks used by this slimbus controller
> + - clock-names : Required clock-name entries are:
> + "iface_clk" : Interface clock for this controller
> + "core_clk" : Interrupt for controller core's BAM
> +
> +
> +Optional property:
> + - reg entry for slew rate : If slew rate control register is provided, this
> + entry should be used.
> + - reg-name for slew rate: "slew"
> +
> +Example:
> + slim@2808 {
> + compatible = "qcom,slim";
> + #address-cells = <4>;
> + #size-cells = <0>;
> + reg = <0x2808 0x2000>, <0x80207C 4>;
> + reg-names = "ctrl", "slew";
> + interrupts = <0 33 0>;
> + clocks = < SLIMBUS_SRC>, < AUDIO_SLIMBUS_CLK>;
> + clock-names = "iface_clk", "core_clk";
> +
> + codec: wcd9310@1{
> + compatible = "slim217,60";
> + reg = <1 0>;
> + };
> + };
> diff --git a/drivers/slimbus/Kconfig b/drivers/slimbus/Kconfig
> index f0b118a..438773e 100644
> --- a/drivers/slimbus/Kconfig
> +++ b/drivers/slimbus/Kconfig
> @@ -9,3 +9,12 @@ menuconfig SLIMBUS
>  
> If unsure, choose N.
>  
> +if SLIMBUS
> +config SLIM_QCOM_CTRL
> + tristate "Qualcomm Slimbus Manager Component"
> + depends on SLIMBUS
> + help
> +   Select driver if Qualcomm's Slimbus Manager Component is
> +   programmed using Linux kernel.
> +
> +endif
> diff --git a/drivers/slimbus/Makefile b/drivers/slimbus/Makefile
> index bd1d35e..ed8521a 100644
> --- a/drivers/slimbus/Makefile
> +++ b/drivers/slimbus/Makefile
> @@ -3,3 +3,6 @@
>  #
>  obj-$(CONFIG_SLIMBUS)+= slimbus.o
>  slimbus-y:= slim-core.o slim-messaging.o
> +
> +#Controllers
> +obj-$(CONFIG_SLIM_QCOM_CTRL) += slim-qcom-ctrl.o
> diff --git a/drivers/slimbus/slim-qcom-ctrl.c 
> b/drivers/slimbus/slim-qcom-ctrl.c
> new file mode 100644
> index 000..d0574e1
> --- /dev/null
> +++ b/drivers/slimbus/slim-qcom-ctrl.c
> @@ -0,0 +1,594 @@
> +/* Copyright (c) 2011-2016, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * 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.
> + */
> +#include 
> +#include 
> +#include 
> +#include 

Re: [Patch v6 3/7] slimbus: qcom: Add Qualcomm Slimbus controller driver

[Srinivas Kandagatla]

On 13/10/17 20:17, Rob Herring wrote:

On Fri, Oct 06, 2017 at 05:51:32PM +0200, srinivas.kandaga...@linaro.org wrote:

From: Sagar Dharia 

This controller driver programs manager, interface, and framer
devices for Qualcomm's slimbus HW block.
Manager component currently implements logical address setting,
and messaging interface.
Interface device reports bus synchronization information, and framer
device clocks the bus from the time it's woken up, until clock-pause
is executed by the manager device.

Signed-off-by: Sagar Dharia 
Signed-off-by: Srinivas Kandagatla 
---
  .../devicetree/bindings/slimbus/slim-qcom-ctrl.txt |  43 ++


Version 6 and this is the first I see it? Where's the version history?


Its been a very long time (18 months) from v5-v6, I will try see if I 
can pull up some version history from past few year patches :-) before i 
send next version. If it helps review process.




Please split to separate patch.


Will do that in next version.




  drivers/slimbus/Kconfig|   9 +
  drivers/slimbus/Makefile   |   3 +
  drivers/slimbus/slim-qcom-ctrl.c   | 594 +
  drivers/slimbus/slim-qcom.h|  63 +++
  5 files changed, 712 insertions(+)
  create mode 100644 
Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
  create mode 100644 drivers/slimbus/slim-qcom-ctrl.c
  create mode 100644 drivers/slimbus/slim-qcom.h

diff --git a/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt 
b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
new file mode 100644
index 000..081110d
--- /dev/null
+++ b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
@@ -0,0 +1,43 @@
+Qualcomm SLIMBUS controller
+This controller is used if applications processor driver controls slimbus
+master component.
+
+Required properties:
+
+ - #address-cells - refer to Documentation/devicetree/bindings/slimbus/bus.txt
+ - #size-cells - refer to Documentation/devicetree/bindings/slimbus/bus.txt
+
+ - reg : Offset and length of the register region(s) for the device
+ - reg-names : Register region name(s) referenced in reg above
+Required register resource entries are:
+"ctrl": Physical adderess of controller register blocks
+ - compatible : should be "qcom,-slim" for SOC specific compatible or
+   "qcom,slim" if using generic qcom SLIM IP.


No such thing as generic IP.Yep, i though I removed such instances, but it looks like there are 

still in more places, I will rescan for them before sending next version.


Fine as a fallback, but not by itself.


+ - interrupts : Interrupt number used by this controller
+ - clocks : Interface and core clocks used by this slimbus controller
+ - clock-names : Required clock-name entries are:
+   "iface_clk" : Interface clock for this controller
+   "core_clk" : Interrupt for controller core's BAM


_clk is redundant.

I agree, I will remove this suffixes in next version.




+
+
+Optional property:
+ - reg entry for slew rate : If slew rate control register is provided, this
+   entry should be used.
+ - reg-name for slew rate: "slew"


Pointless to have -name when there is only one.


I agree if it was just one.

Slew is optional resource, but as a whole there are 2 resources for the 
controller, so we need -name as there are 2 resources.



+
+Example:
+   slim@2808 {
+   compatible = "qcom,slim";
+   #address-cells = <4>;
+   #size-cells = <0>;
+   reg = <0x2808 0x2000>, <0x80207C 4>;
+   reg-names = "ctrl", "slew";
+   interrupts = <0 33 0>;
+   clocks = < SLIMBUS_SRC>, < AUDIO_SLIMBUS_CLK>;
+   clock-names = "iface_clk", "core_clk";
+
+   codec: wcd9310@1{
+   compatible = "slim217,60";
+   reg = <1 0>;


This would not compile. You don't have 4 cells here.


Yep, the example needs fixing.. address cells are actually 2.




+   };
+   };

Re: [Patch v6 3/7] slimbus: qcom: Add Qualcomm Slimbus controller driver

[Srinivas Kandagatla]

On 13/10/17 20:17, Rob Herring wrote:

On Fri, Oct 06, 2017 at 05:51:32PM +0200, srinivas.kandaga...@linaro.org wrote:

From: Sagar Dharia 

This controller driver programs manager, interface, and framer
devices for Qualcomm's slimbus HW block.
Manager component currently implements logical address setting,
and messaging interface.
Interface device reports bus synchronization information, and framer
device clocks the bus from the time it's woken up, until clock-pause
is executed by the manager device.

Signed-off-by: Sagar Dharia 
Signed-off-by: Srinivas Kandagatla 
---
  .../devicetree/bindings/slimbus/slim-qcom-ctrl.txt |  43 ++


Version 6 and this is the first I see it? Where's the version history?


Its been a very long time (18 months) from v5-v6, I will try see if I 
can pull up some version history from past few year patches :-) before i 
send next version. If it helps review process.




Please split to separate patch.


Will do that in next version.




  drivers/slimbus/Kconfig|   9 +
  drivers/slimbus/Makefile   |   3 +
  drivers/slimbus/slim-qcom-ctrl.c   | 594 +
  drivers/slimbus/slim-qcom.h|  63 +++
  5 files changed, 712 insertions(+)
  create mode 100644 
Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
  create mode 100644 drivers/slimbus/slim-qcom-ctrl.c
  create mode 100644 drivers/slimbus/slim-qcom.h

diff --git a/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt 
b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
new file mode 100644
index 000..081110d
--- /dev/null
+++ b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
@@ -0,0 +1,43 @@
+Qualcomm SLIMBUS controller
+This controller is used if applications processor driver controls slimbus
+master component.
+
+Required properties:
+
+ - #address-cells - refer to Documentation/devicetree/bindings/slimbus/bus.txt
+ - #size-cells - refer to Documentation/devicetree/bindings/slimbus/bus.txt
+
+ - reg : Offset and length of the register region(s) for the device
+ - reg-names : Register region name(s) referenced in reg above
+Required register resource entries are:
+"ctrl": Physical adderess of controller register blocks
+ - compatible : should be "qcom,-slim" for SOC specific compatible or
+   "qcom,slim" if using generic qcom SLIM IP.


No such thing as generic IP.Yep, i though I removed such instances, but it looks like there are 

still in more places, I will rescan for them before sending next version.


Fine as a fallback, but not by itself.


+ - interrupts : Interrupt number used by this controller
+ - clocks : Interface and core clocks used by this slimbus controller
+ - clock-names : Required clock-name entries are:
+   "iface_clk" : Interface clock for this controller
+   "core_clk" : Interrupt for controller core's BAM


_clk is redundant.

I agree, I will remove this suffixes in next version.




+
+
+Optional property:
+ - reg entry for slew rate : If slew rate control register is provided, this
+   entry should be used.
+ - reg-name for slew rate: "slew"


Pointless to have -name when there is only one.


I agree if it was just one.

Slew is optional resource, but as a whole there are 2 resources for the 
controller, so we need -name as there are 2 resources.



+
+Example:
+   slim@2808 {
+   compatible = "qcom,slim";
+   #address-cells = <4>;
+   #size-cells = <0>;
+   reg = <0x2808 0x2000>, <0x80207C 4>;
+   reg-names = "ctrl", "slew";
+   interrupts = <0 33 0>;
+   clocks = < SLIMBUS_SRC>, < AUDIO_SLIMBUS_CLK>;
+   clock-names = "iface_clk", "core_clk";
+
+   codec: wcd9310@1{
+   compatible = "slim217,60";
+   reg = <1 0>;


This would not compile. You don't have 4 cells here.


Yep, the example needs fixing.. address cells are actually 2.




+   };
+   };

Re: [Patch v6 3/7] slimbus: qcom: Add Qualcomm Slimbus controller driver

[Rob Herring]

On Fri, Oct 06, 2017 at 05:51:32PM +0200, srinivas.kandaga...@linaro.org wrote:
> From: Sagar Dharia 
> 
> This controller driver programs manager, interface, and framer
> devices for Qualcomm's slimbus HW block.
> Manager component currently implements logical address setting,
> and messaging interface.
> Interface device reports bus synchronization information, and framer
> device clocks the bus from the time it's woken up, until clock-pause
> is executed by the manager device.
> 
> Signed-off-by: Sagar Dharia 
> Signed-off-by: Srinivas Kandagatla 
> ---
>  .../devicetree/bindings/slimbus/slim-qcom-ctrl.txt |  43 ++

Version 6 and this is the first I see it? Where's the version history? 

Please split to separate patch.

>  drivers/slimbus/Kconfig|   9 +
>  drivers/slimbus/Makefile   |   3 +
>  drivers/slimbus/slim-qcom-ctrl.c   | 594 
> +
>  drivers/slimbus/slim-qcom.h|  63 +++
>  5 files changed, 712 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
>  create mode 100644 drivers/slimbus/slim-qcom-ctrl.c
>  create mode 100644 drivers/slimbus/slim-qcom.h
> 
> diff --git a/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt 
> b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
> new file mode 100644
> index 000..081110d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
> @@ -0,0 +1,43 @@
> +Qualcomm SLIMBUS controller
> +This controller is used if applications processor driver controls slimbus
> +master component.
> +
> +Required properties:
> +
> + - #address-cells - refer to 
> Documentation/devicetree/bindings/slimbus/bus.txt
> + - #size-cells   - refer to 
> Documentation/devicetree/bindings/slimbus/bus.txt
> +
> + - reg : Offset and length of the register region(s) for the device
> + - reg-names : Register region name(s) referenced in reg above
> +  Required register resource entries are:
> +  "ctrl": Physical adderess of controller register blocks
> + - compatible : should be "qcom,-slim" for SOC specific compatible 
> or
> + "qcom,slim" if using generic qcom SLIM IP.

No such thing as generic IP.

Fine as a fallback, but not by itself.

> + - interrupts : Interrupt number used by this controller
> + - clocks : Interface and core clocks used by this slimbus controller
> + - clock-names : Required clock-name entries are:
> + "iface_clk" : Interface clock for this controller
> + "core_clk" : Interrupt for controller core's BAM

_clk is redundant.

> +
> +
> +Optional property:
> + - reg entry for slew rate : If slew rate control register is provided, this
> + entry should be used.
> + - reg-name for slew rate: "slew"

Pointless to have -name when there is only one.

> +
> +Example:
> + slim@2808 {
> + compatible = "qcom,slim";
> + #address-cells = <4>;
> + #size-cells = <0>;
> + reg = <0x2808 0x2000>, <0x80207C 4>;
> + reg-names = "ctrl", "slew";
> + interrupts = <0 33 0>;
> + clocks = < SLIMBUS_SRC>, < AUDIO_SLIMBUS_CLK>;
> + clock-names = "iface_clk", "core_clk";
> +
> + codec: wcd9310@1{
> + compatible = "slim217,60";
> + reg = <1 0>;

This would not compile. You don't have 4 cells here.

> + };
> + };

Re: [Patch v6 3/7] slimbus: qcom: Add Qualcomm Slimbus controller driver

[Rob Herring]

On Fri, Oct 06, 2017 at 05:51:32PM +0200, srinivas.kandaga...@linaro.org wrote:
> From: Sagar Dharia 
> 
> This controller driver programs manager, interface, and framer
> devices for Qualcomm's slimbus HW block.
> Manager component currently implements logical address setting,
> and messaging interface.
> Interface device reports bus synchronization information, and framer
> device clocks the bus from the time it's woken up, until clock-pause
> is executed by the manager device.
> 
> Signed-off-by: Sagar Dharia 
> Signed-off-by: Srinivas Kandagatla 
> ---
>  .../devicetree/bindings/slimbus/slim-qcom-ctrl.txt |  43 ++

Version 6 and this is the first I see it? Where's the version history? 

Please split to separate patch.

>  drivers/slimbus/Kconfig|   9 +
>  drivers/slimbus/Makefile   |   3 +
>  drivers/slimbus/slim-qcom-ctrl.c   | 594 
> +
>  drivers/slimbus/slim-qcom.h|  63 +++
>  5 files changed, 712 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
>  create mode 100644 drivers/slimbus/slim-qcom-ctrl.c
>  create mode 100644 drivers/slimbus/slim-qcom.h
> 
> diff --git a/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt 
> b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
> new file mode 100644
> index 000..081110d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
> @@ -0,0 +1,43 @@
> +Qualcomm SLIMBUS controller
> +This controller is used if applications processor driver controls slimbus
> +master component.
> +
> +Required properties:
> +
> + - #address-cells - refer to 
> Documentation/devicetree/bindings/slimbus/bus.txt
> + - #size-cells   - refer to 
> Documentation/devicetree/bindings/slimbus/bus.txt
> +
> + - reg : Offset and length of the register region(s) for the device
> + - reg-names : Register region name(s) referenced in reg above
> +  Required register resource entries are:
> +  "ctrl": Physical adderess of controller register blocks
> + - compatible : should be "qcom,-slim" for SOC specific compatible 
> or
> + "qcom,slim" if using generic qcom SLIM IP.

No such thing as generic IP.

Fine as a fallback, but not by itself.

> + - interrupts : Interrupt number used by this controller
> + - clocks : Interface and core clocks used by this slimbus controller
> + - clock-names : Required clock-name entries are:
> + "iface_clk" : Interface clock for this controller
> + "core_clk" : Interrupt for controller core's BAM

_clk is redundant.

> +
> +
> +Optional property:
> + - reg entry for slew rate : If slew rate control register is provided, this
> + entry should be used.
> + - reg-name for slew rate: "slew"

Pointless to have -name when there is only one.

> +
> +Example:
> + slim@2808 {
> + compatible = "qcom,slim";
> + #address-cells = <4>;
> + #size-cells = <0>;
> + reg = <0x2808 0x2000>, <0x80207C 4>;
> + reg-names = "ctrl", "slew";
> + interrupts = <0 33 0>;
> + clocks = < SLIMBUS_SRC>, < AUDIO_SLIMBUS_CLK>;
> + clock-names = "iface_clk", "core_clk";
> +
> + codec: wcd9310@1{
> + compatible = "slim217,60";
> + reg = <1 0>;

This would not compile. You don't have 4 cells here.

> + };
> + };

Re: [Patch v6 3/7] slimbus: qcom: Add Qualcomm Slimbus controller driver

[Srinivas Kandagatla]

Thanks for the review comments

On 07/10/17 08:45, Jonathan Neuschäfer wrote:

Hi,

On Fri, Oct 06, 2017 at 05:51:32PM +0200, srinivas.kandaga...@linaro.org wrote:

From: Sagar Dharia 

This controller driver programs manager, interface, and framer
devices for Qualcomm's slimbus HW block.
Manager component currently implements logical address setting,
and messaging interface.
Interface device reports bus synchronization information, and framer
device clocks the bus from the time it's woken up, until clock-pause
is executed by the manager device.

Signed-off-by: Sagar Dharia 
Signed-off-by: Srinivas Kandagatla 
---
  .../devicetree/bindings/slimbus/slim-qcom-ctrl.txt |  43 ++
  drivers/slimbus/Kconfig|   9 +
  drivers/slimbus/Makefile   |   3 +
  drivers/slimbus/slim-qcom-ctrl.c   | 594 +
  drivers/slimbus/slim-qcom.h|  63 +++
  5 files changed, 712 insertions(+)
  create mode 100644 
Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
  create mode 100644 drivers/slimbus/slim-qcom-ctrl.c
  create mode 100644 drivers/slimbus/slim-qcom.h

diff --git a/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt 
b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
new file mode 100644
index 000..081110d
--- /dev/null
+++ b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
@@ -0,0 +1,43 @@
+Qualcomm SLIMBUS controller
+This controller is used if applications processor driver controls slimbus
+master component.
+
+Required properties:
+
+ - #address-cells - refer to Documentation/devicetree/bindings/slimbus/bus.txt
+ - #size-cells - refer to Documentation/devicetree/bindings/slimbus/bus.txt
+
+ - reg : Offset and length of the register region(s) for the device
+ - reg-names : Register region name(s) referenced in reg above
+Required register resource entries are:
+"ctrl": Physical adderess of controller register blocks


s/adderess/address/

Will fix this in next version.




+}

[...]

+static void msm_slim_prg_slew(struct platform_device *pdev,
+   struct msm_slim_ctrl *dev)
+{
+   void __iomem *slew_reg;
+
+   /* SLEW RATE register for this slimbus */
+   dev->slew_mem = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+"slew");
+   if (!dev->slew_mem) {
+   dev_warn(>dev, "no slimbus slew resource\n");
+   return;
+   }
+
+   slew_reg = devm_ioremap(>dev, dev->slew_mem->start,
+   resource_size(dev->slew_mem));


How often will the driver program a slew rate?


This should be programmed only once after power on.


If it's often, you'll have a "soft" memory leak over the life time of a
SLIM controller instance, because the mappings for slew_reg will
accumulate in the driver instance's devm area until they are all freed
in the end (If I'm reading the code correctly). I think you'll either
have to unmap slew_reg when this function returns (and not use devm), or
cache slew_reg so that subsequent calls to msm_slim_prg_slew won't
create more mappings.
Yep .. I revisit this part once again before sending next version to see 
if we can do any better!



+   if (!slew_reg) {
+   dev_err(dev->dev, "slew register mapping failed");
+   release_mem_region(dev->slew_mem->start,
+   resource_size(dev->slew_mem));
+   dev->slew_mem = NULL;
+   return;
+   }
+   writel_relaxed(1, slew_reg);
+   /* Make sure slimbus-slew rate enabling goes through */
+   wmb();
+}
+
+static int msm_slim_probe(struct platform_device *pdev)
+{
+   struct msm_slim_ctrl *dev;
+   struct slim_controller *ctrl;
+   struct resource *slim_mem;
+   struct resource *irq;
+   struct clk *hclk, *rclk;
+   int ret;
+
+   hclk = devm_clk_get(>dev, "iface_clk");
+   if (IS_ERR(hclk))
+   return PTR_ERR(hclk);
+
+   rclk = devm_clk_get(>dev, "core_clk");
+   if (IS_ERR(rclk)) {
+   /* unlikely that this is probe-defer */
+   dev_err(>dev, "rclk get failed:%ld\n", PTR_ERR(rclk));
+   return PTR_ERR(rclk);
+   }
+
+   ret = clk_set_rate(rclk, SLIM_ROOT_FREQ);
+   if (ret) {
+   dev_err(>dev, "ref-clock set-rate failed:%d\n", ret);
+   return ret;
+   }
+
+   slim_mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ctrl");
+   if (!slim_mem) {
+   dev_err(>dev, "no slimbus physical memory resource\n");
+   return -ENODEV;
+   }
+
+   irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+   if (!irq) {
+   dev_err(>dev, "no slimbus IRQ resource\n");
+   return 

Re: [Patch v6 3/7] slimbus: qcom: Add Qualcomm Slimbus controller driver

[Srinivas Kandagatla]

Thanks for the review comments

On 07/10/17 08:45, Jonathan Neuschäfer wrote:

Hi,

On Fri, Oct 06, 2017 at 05:51:32PM +0200, srinivas.kandaga...@linaro.org wrote:

From: Sagar Dharia 

This controller driver programs manager, interface, and framer
devices for Qualcomm's slimbus HW block.
Manager component currently implements logical address setting,
and messaging interface.
Interface device reports bus synchronization information, and framer
device clocks the bus from the time it's woken up, until clock-pause
is executed by the manager device.

Signed-off-by: Sagar Dharia 
Signed-off-by: Srinivas Kandagatla 
---
  .../devicetree/bindings/slimbus/slim-qcom-ctrl.txt |  43 ++
  drivers/slimbus/Kconfig|   9 +
  drivers/slimbus/Makefile   |   3 +
  drivers/slimbus/slim-qcom-ctrl.c   | 594 +
  drivers/slimbus/slim-qcom.h|  63 +++
  5 files changed, 712 insertions(+)
  create mode 100644 
Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
  create mode 100644 drivers/slimbus/slim-qcom-ctrl.c
  create mode 100644 drivers/slimbus/slim-qcom.h

diff --git a/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt 
b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
new file mode 100644
index 000..081110d
--- /dev/null
+++ b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
@@ -0,0 +1,43 @@
+Qualcomm SLIMBUS controller
+This controller is used if applications processor driver controls slimbus
+master component.
+
+Required properties:
+
+ - #address-cells - refer to Documentation/devicetree/bindings/slimbus/bus.txt
+ - #size-cells - refer to Documentation/devicetree/bindings/slimbus/bus.txt
+
+ - reg : Offset and length of the register region(s) for the device
+ - reg-names : Register region name(s) referenced in reg above
+Required register resource entries are:
+"ctrl": Physical adderess of controller register blocks


s/adderess/address/

Will fix this in next version.




+}

[...]

+static void msm_slim_prg_slew(struct platform_device *pdev,
+   struct msm_slim_ctrl *dev)
+{
+   void __iomem *slew_reg;
+
+   /* SLEW RATE register for this slimbus */
+   dev->slew_mem = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+"slew");
+   if (!dev->slew_mem) {
+   dev_warn(>dev, "no slimbus slew resource\n");
+   return;
+   }
+
+   slew_reg = devm_ioremap(>dev, dev->slew_mem->start,
+   resource_size(dev->slew_mem));


How often will the driver program a slew rate?


This should be programmed only once after power on.


If it's often, you'll have a "soft" memory leak over the life time of a
SLIM controller instance, because the mappings for slew_reg will
accumulate in the driver instance's devm area until they are all freed
in the end (If I'm reading the code correctly). I think you'll either
have to unmap slew_reg when this function returns (and not use devm), or
cache slew_reg so that subsequent calls to msm_slim_prg_slew won't
create more mappings.
Yep .. I revisit this part once again before sending next version to see 
if we can do any better!



+   if (!slew_reg) {
+   dev_err(dev->dev, "slew register mapping failed");
+   release_mem_region(dev->slew_mem->start,
+   resource_size(dev->slew_mem));
+   dev->slew_mem = NULL;
+   return;
+   }
+   writel_relaxed(1, slew_reg);
+   /* Make sure slimbus-slew rate enabling goes through */
+   wmb();
+}
+
+static int msm_slim_probe(struct platform_device *pdev)
+{
+   struct msm_slim_ctrl *dev;
+   struct slim_controller *ctrl;
+   struct resource *slim_mem;
+   struct resource *irq;
+   struct clk *hclk, *rclk;
+   int ret;
+
+   hclk = devm_clk_get(>dev, "iface_clk");
+   if (IS_ERR(hclk))
+   return PTR_ERR(hclk);
+
+   rclk = devm_clk_get(>dev, "core_clk");
+   if (IS_ERR(rclk)) {
+   /* unlikely that this is probe-defer */
+   dev_err(>dev, "rclk get failed:%ld\n", PTR_ERR(rclk));
+   return PTR_ERR(rclk);
+   }
+
+   ret = clk_set_rate(rclk, SLIM_ROOT_FREQ);
+   if (ret) {
+   dev_err(>dev, "ref-clock set-rate failed:%d\n", ret);
+   return ret;
+   }
+
+   slim_mem = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ctrl");
+   if (!slim_mem) {
+   dev_err(>dev, "no slimbus physical memory resource\n");
+   return -ENODEV;
+   }
+
+   irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+   if (!irq) {
+   dev_err(>dev, "no slimbus IRQ resource\n");
+   return -ENODEV;
+   }
+
+   dev = devm_kzalloc(>dev, sizeof(*dev), GFP_KERNEL);
+   

Re: [Patch v6 3/7] slimbus: qcom: Add Qualcomm Slimbus controller driver

[Jonathan Neuschäfer]

Hi,

On Fri, Oct 06, 2017 at 05:51:32PM +0200, srinivas.kandaga...@linaro.org wrote:
> From: Sagar Dharia 
> 
> This controller driver programs manager, interface, and framer
> devices for Qualcomm's slimbus HW block.
> Manager component currently implements logical address setting,
> and messaging interface.
> Interface device reports bus synchronization information, and framer
> device clocks the bus from the time it's woken up, until clock-pause
> is executed by the manager device.
> 
> Signed-off-by: Sagar Dharia 
> Signed-off-by: Srinivas Kandagatla 
> ---
>  .../devicetree/bindings/slimbus/slim-qcom-ctrl.txt |  43 ++
>  drivers/slimbus/Kconfig|   9 +
>  drivers/slimbus/Makefile   |   3 +
>  drivers/slimbus/slim-qcom-ctrl.c   | 594 
> +
>  drivers/slimbus/slim-qcom.h|  63 +++
>  5 files changed, 712 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
>  create mode 100644 drivers/slimbus/slim-qcom-ctrl.c
>  create mode 100644 drivers/slimbus/slim-qcom.h
> 
> diff --git a/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt 
> b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
> new file mode 100644
> index 000..081110d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
> @@ -0,0 +1,43 @@
> +Qualcomm SLIMBUS controller
> +This controller is used if applications processor driver controls slimbus
> +master component.
> +
> +Required properties:
> +
> + - #address-cells - refer to 
> Documentation/devicetree/bindings/slimbus/bus.txt
> + - #size-cells   - refer to 
> Documentation/devicetree/bindings/slimbus/bus.txt
> +
> + - reg : Offset and length of the register region(s) for the device
> + - reg-names : Register region name(s) referenced in reg above
> +  Required register resource entries are:
> +  "ctrl": Physical adderess of controller register blocks

s/adderess/address/

> + - compatible : should be "qcom,-slim" for SOC specific compatible 
> or
> + "qcom,slim" if using generic qcom SLIM IP.
> + - interrupts : Interrupt number used by this controller
> + - clocks : Interface and core clocks used by this slimbus controller
> + - clock-names : Required clock-name entries are:
> + "iface_clk" : Interface clock for this controller
> + "core_clk" : Interrupt for controller core's BAM
[...]

> +static irqreturn_t msm_slim_interrupt(int irq, void *d)
> +{
> + struct msm_slim_ctrl *dev = d;
> + u32 stat = readl_relaxed(dev->base + MGR_INT_STAT);
> + int err = 0, ret = IRQ_NONE;
> +
> + if (stat & MGR_INT_TX_MSG_SENT || stat & MGR_INT_TX_NACKED_2) {
> + if (stat & MGR_INT_TX_MSG_SENT)
> + writel_relaxed(MGR_INT_TX_MSG_SENT,
> +dev->base + MGR_INT_CLR);
> + if (stat & MGR_INT_TX_NACKED_2) {
> + u32 mgr_stat = readl_relaxed(dev->base + MGR_STATUS);
> + u32 mgr_ie_stat = readl_relaxed(dev->base +
> + MGR_IE_STAT);
> + u32 frm_stat = readl_relaxed(dev->base + FRM_STAT);
> + u32 frm_cfg = readl_relaxed(dev->base + FRM_CFG);
> + u32 frm_intr_stat = readl_relaxed(dev->base +
> +   FRM_INT_STAT);
> + u32 frm_ie_stat = readl_relaxed(dev->base +
> + FRM_IE_STAT);
> + u32 intf_stat = readl_relaxed(dev->base + INTF_STAT);
> + u32 intf_intr_stat = readl_relaxed(dev->base +
> +INTF_INT_STAT);
> + u32 intf_ie_stat = readl_relaxed(dev->base +
> +  INTF_IE_STAT);
> +
> + writel_relaxed(MGR_INT_TX_NACKED_2, dev->base +
> +MGR_INT_CLR);
> + dev_err(dev->dev, "TX Nack MGR:int:0x%x, stat:0x%x\n",
> + stat, mgr_stat);
> + dev_err(dev->dev, "TX Nack MGR:ie:0x%x\n", mgr_ie_stat);
> + dev_err(dev->dev, "TX Nack FRM:int:0x%x, stat:0x%x\n",
> + frm_intr_stat, frm_stat);
> + dev_err(dev->dev, "TX Nack FRM:cfg:0x%x, ie:0x%x\n",
> + frm_cfg, frm_ie_stat);
> + dev_err(dev->dev, "TX Nack INTF:intr:0x%x, stat:0x%x\n",
> + intf_intr_stat, intf_stat);
> + dev_err(dev->dev, "TX Nack INTF:ie:0x%x\n",
> + intf_ie_stat);
> + err = -ENOTCONN;
> + }
> 

Re: [Patch v6 3/7] slimbus: qcom: Add Qualcomm Slimbus controller driver

[Jonathan Neuschäfer]

Hi,

On Fri, Oct 06, 2017 at 05:51:32PM +0200, srinivas.kandaga...@linaro.org wrote:
> From: Sagar Dharia 
> 
> This controller driver programs manager, interface, and framer
> devices for Qualcomm's slimbus HW block.
> Manager component currently implements logical address setting,
> and messaging interface.
> Interface device reports bus synchronization information, and framer
> device clocks the bus from the time it's woken up, until clock-pause
> is executed by the manager device.
> 
> Signed-off-by: Sagar Dharia 
> Signed-off-by: Srinivas Kandagatla 
> ---
>  .../devicetree/bindings/slimbus/slim-qcom-ctrl.txt |  43 ++
>  drivers/slimbus/Kconfig|   9 +
>  drivers/slimbus/Makefile   |   3 +
>  drivers/slimbus/slim-qcom-ctrl.c   | 594 
> +
>  drivers/slimbus/slim-qcom.h|  63 +++
>  5 files changed, 712 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
>  create mode 100644 drivers/slimbus/slim-qcom-ctrl.c
>  create mode 100644 drivers/slimbus/slim-qcom.h
> 
> diff --git a/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt 
> b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
> new file mode 100644
> index 000..081110d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
> @@ -0,0 +1,43 @@
> +Qualcomm SLIMBUS controller
> +This controller is used if applications processor driver controls slimbus
> +master component.
> +
> +Required properties:
> +
> + - #address-cells - refer to 
> Documentation/devicetree/bindings/slimbus/bus.txt
> + - #size-cells   - refer to 
> Documentation/devicetree/bindings/slimbus/bus.txt
> +
> + - reg : Offset and length of the register region(s) for the device
> + - reg-names : Register region name(s) referenced in reg above
> +  Required register resource entries are:
> +  "ctrl": Physical adderess of controller register blocks

s/adderess/address/

> + - compatible : should be "qcom,-slim" for SOC specific compatible 
> or
> + "qcom,slim" if using generic qcom SLIM IP.
> + - interrupts : Interrupt number used by this controller
> + - clocks : Interface and core clocks used by this slimbus controller
> + - clock-names : Required clock-name entries are:
> + "iface_clk" : Interface clock for this controller
> + "core_clk" : Interrupt for controller core's BAM
[...]

> +static irqreturn_t msm_slim_interrupt(int irq, void *d)
> +{
> + struct msm_slim_ctrl *dev = d;
> + u32 stat = readl_relaxed(dev->base + MGR_INT_STAT);
> + int err = 0, ret = IRQ_NONE;
> +
> + if (stat & MGR_INT_TX_MSG_SENT || stat & MGR_INT_TX_NACKED_2) {
> + if (stat & MGR_INT_TX_MSG_SENT)
> + writel_relaxed(MGR_INT_TX_MSG_SENT,
> +dev->base + MGR_INT_CLR);
> + if (stat & MGR_INT_TX_NACKED_2) {
> + u32 mgr_stat = readl_relaxed(dev->base + MGR_STATUS);
> + u32 mgr_ie_stat = readl_relaxed(dev->base +
> + MGR_IE_STAT);
> + u32 frm_stat = readl_relaxed(dev->base + FRM_STAT);
> + u32 frm_cfg = readl_relaxed(dev->base + FRM_CFG);
> + u32 frm_intr_stat = readl_relaxed(dev->base +
> +   FRM_INT_STAT);
> + u32 frm_ie_stat = readl_relaxed(dev->base +
> + FRM_IE_STAT);
> + u32 intf_stat = readl_relaxed(dev->base + INTF_STAT);
> + u32 intf_intr_stat = readl_relaxed(dev->base +
> +INTF_INT_STAT);
> + u32 intf_ie_stat = readl_relaxed(dev->base +
> +  INTF_IE_STAT);
> +
> + writel_relaxed(MGR_INT_TX_NACKED_2, dev->base +
> +MGR_INT_CLR);
> + dev_err(dev->dev, "TX Nack MGR:int:0x%x, stat:0x%x\n",
> + stat, mgr_stat);
> + dev_err(dev->dev, "TX Nack MGR:ie:0x%x\n", mgr_ie_stat);
> + dev_err(dev->dev, "TX Nack FRM:int:0x%x, stat:0x%x\n",
> + frm_intr_stat, frm_stat);
> + dev_err(dev->dev, "TX Nack FRM:cfg:0x%x, ie:0x%x\n",
> + frm_cfg, frm_ie_stat);
> + dev_err(dev->dev, "TX Nack INTF:intr:0x%x, stat:0x%x\n",
> + intf_intr_stat, intf_stat);
> + dev_err(dev->dev, "TX Nack INTF:ie:0x%x\n",
> + intf_ie_stat);
> + err = -ENOTCONN;
> + }
> + /**

This isn't really a kerneldoc comment…

> +  * 

[Patch v6 3/7] slimbus: qcom: Add Qualcomm Slimbus controller driver

[srinivas . kandagatla]

From: Sagar Dharia 

This controller driver programs manager, interface, and framer
devices for Qualcomm's slimbus HW block.
Manager component currently implements logical address setting,
and messaging interface.
Interface device reports bus synchronization information, and framer
device clocks the bus from the time it's woken up, until clock-pause
is executed by the manager device.

Signed-off-by: Sagar Dharia 
Signed-off-by: Srinivas Kandagatla 
---
 .../devicetree/bindings/slimbus/slim-qcom-ctrl.txt |  43 ++
 drivers/slimbus/Kconfig|   9 +
 drivers/slimbus/Makefile   |   3 +
 drivers/slimbus/slim-qcom-ctrl.c   | 594 +
 drivers/slimbus/slim-qcom.h|  63 +++
 5 files changed, 712 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
 create mode 100644 drivers/slimbus/slim-qcom-ctrl.c
 create mode 100644 drivers/slimbus/slim-qcom.h

diff --git a/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt 
b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
new file mode 100644
index 000..081110d
--- /dev/null
+++ b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
@@ -0,0 +1,43 @@
+Qualcomm SLIMBUS controller
+This controller is used if applications processor driver controls slimbus
+master component.
+
+Required properties:
+
+ - #address-cells - refer to Documentation/devicetree/bindings/slimbus/bus.txt
+ - #size-cells - refer to Documentation/devicetree/bindings/slimbus/bus.txt
+
+ - reg : Offset and length of the register region(s) for the device
+ - reg-names : Register region name(s) referenced in reg above
+Required register resource entries are:
+"ctrl": Physical adderess of controller register blocks
+ - compatible : should be "qcom,-slim" for SOC specific compatible or
+   "qcom,slim" if using generic qcom SLIM IP.
+ - interrupts : Interrupt number used by this controller
+ - clocks : Interface and core clocks used by this slimbus controller
+ - clock-names : Required clock-name entries are:
+   "iface_clk" : Interface clock for this controller
+   "core_clk" : Interrupt for controller core's BAM
+
+
+Optional property:
+ - reg entry for slew rate : If slew rate control register is provided, this
+   entry should be used.
+ - reg-name for slew rate: "slew"
+
+Example:
+   slim@2808 {
+   compatible = "qcom,slim";
+   #address-cells = <4>;
+   #size-cells = <0>;
+   reg = <0x2808 0x2000>, <0x80207C 4>;
+   reg-names = "ctrl", "slew";
+   interrupts = <0 33 0>;
+   clocks = < SLIMBUS_SRC>, < AUDIO_SLIMBUS_CLK>;
+   clock-names = "iface_clk", "core_clk";
+
+   codec: wcd9310@1{
+   compatible = "slim217,60";
+   reg = <1 0>;
+   };
+   };
diff --git a/drivers/slimbus/Kconfig b/drivers/slimbus/Kconfig
index f0b118a..438773e 100644
--- a/drivers/slimbus/Kconfig
+++ b/drivers/slimbus/Kconfig
@@ -9,3 +9,12 @@ menuconfig SLIMBUS
 
  If unsure, choose N.
 
+if SLIMBUS
+config SLIM_QCOM_CTRL
+   tristate "Qualcomm Slimbus Manager Component"
+   depends on SLIMBUS
+   help
+ Select driver if Qualcomm's Slimbus Manager Component is
+ programmed using Linux kernel.
+
+endif
diff --git a/drivers/slimbus/Makefile b/drivers/slimbus/Makefile
index bd1d35e..ed8521a 100644
--- a/drivers/slimbus/Makefile
+++ b/drivers/slimbus/Makefile
@@ -3,3 +3,6 @@
 #
 obj-$(CONFIG_SLIMBUS)  += slimbus.o
 slimbus-y  := slim-core.o slim-messaging.o
+
+#Controllers
+obj-$(CONFIG_SLIM_QCOM_CTRL)   += slim-qcom-ctrl.o
diff --git a/drivers/slimbus/slim-qcom-ctrl.c b/drivers/slimbus/slim-qcom-ctrl.c
new file mode 100644
index 000..d0574e1
--- /dev/null
+++ b/drivers/slimbus/slim-qcom-ctrl.c
@@ -0,0 +1,594 @@
+/* Copyright (c) 2011-2016, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * 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.
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "slim-qcom.h"
+
+#define MSM_SLIM_NAME  "msm_slim_ctrl"
+
+/* Manager registers */
+#defineMGR_CFG 0x200
+#defineMGR_STATUS  0x204
+#defineMGR_INT_EN  0x210

[Patch v6 3/7] slimbus: qcom: Add Qualcomm Slimbus controller driver

[srinivas . kandagatla]

From: Sagar Dharia 

This controller driver programs manager, interface, and framer
devices for Qualcomm's slimbus HW block.
Manager component currently implements logical address setting,
and messaging interface.
Interface device reports bus synchronization information, and framer
device clocks the bus from the time it's woken up, until clock-pause
is executed by the manager device.

Signed-off-by: Sagar Dharia 
Signed-off-by: Srinivas Kandagatla 
---
 .../devicetree/bindings/slimbus/slim-qcom-ctrl.txt |  43 ++
 drivers/slimbus/Kconfig|   9 +
 drivers/slimbus/Makefile   |   3 +
 drivers/slimbus/slim-qcom-ctrl.c   | 594 +
 drivers/slimbus/slim-qcom.h|  63 +++
 5 files changed, 712 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
 create mode 100644 drivers/slimbus/slim-qcom-ctrl.c
 create mode 100644 drivers/slimbus/slim-qcom.h

diff --git a/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt 
b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
new file mode 100644
index 000..081110d
--- /dev/null
+++ b/Documentation/devicetree/bindings/slimbus/slim-qcom-ctrl.txt
@@ -0,0 +1,43 @@
+Qualcomm SLIMBUS controller
+This controller is used if applications processor driver controls slimbus
+master component.
+
+Required properties:
+
+ - #address-cells - refer to Documentation/devicetree/bindings/slimbus/bus.txt
+ - #size-cells - refer to Documentation/devicetree/bindings/slimbus/bus.txt
+
+ - reg : Offset and length of the register region(s) for the device
+ - reg-names : Register region name(s) referenced in reg above
+Required register resource entries are:
+"ctrl": Physical adderess of controller register blocks
+ - compatible : should be "qcom,-slim" for SOC specific compatible or
+   "qcom,slim" if using generic qcom SLIM IP.
+ - interrupts : Interrupt number used by this controller
+ - clocks : Interface and core clocks used by this slimbus controller
+ - clock-names : Required clock-name entries are:
+   "iface_clk" : Interface clock for this controller
+   "core_clk" : Interrupt for controller core's BAM
+
+
+Optional property:
+ - reg entry for slew rate : If slew rate control register is provided, this
+   entry should be used.
+ - reg-name for slew rate: "slew"
+
+Example:
+   slim@2808 {
+   compatible = "qcom,slim";
+   #address-cells = <4>;
+   #size-cells = <0>;
+   reg = <0x2808 0x2000>, <0x80207C 4>;
+   reg-names = "ctrl", "slew";
+   interrupts = <0 33 0>;
+   clocks = < SLIMBUS_SRC>, < AUDIO_SLIMBUS_CLK>;
+   clock-names = "iface_clk", "core_clk";
+
+   codec: wcd9310@1{
+   compatible = "slim217,60";
+   reg = <1 0>;
+   };
+   };
diff --git a/drivers/slimbus/Kconfig b/drivers/slimbus/Kconfig
index f0b118a..438773e 100644
--- a/drivers/slimbus/Kconfig
+++ b/drivers/slimbus/Kconfig
@@ -9,3 +9,12 @@ menuconfig SLIMBUS
 
  If unsure, choose N.
 
+if SLIMBUS
+config SLIM_QCOM_CTRL
+   tristate "Qualcomm Slimbus Manager Component"
+   depends on SLIMBUS
+   help
+ Select driver if Qualcomm's Slimbus Manager Component is
+ programmed using Linux kernel.
+
+endif
diff --git a/drivers/slimbus/Makefile b/drivers/slimbus/Makefile
index bd1d35e..ed8521a 100644
--- a/drivers/slimbus/Makefile
+++ b/drivers/slimbus/Makefile
@@ -3,3 +3,6 @@
 #
 obj-$(CONFIG_SLIMBUS)  += slimbus.o
 slimbus-y  := slim-core.o slim-messaging.o
+
+#Controllers
+obj-$(CONFIG_SLIM_QCOM_CTRL)   += slim-qcom-ctrl.o
diff --git a/drivers/slimbus/slim-qcom-ctrl.c b/drivers/slimbus/slim-qcom-ctrl.c
new file mode 100644
index 000..d0574e1
--- /dev/null
+++ b/drivers/slimbus/slim-qcom-ctrl.c
@@ -0,0 +1,594 @@
+/* Copyright (c) 2011-2016, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * 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.
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "slim-qcom.h"
+
+#define MSM_SLIM_NAME  "msm_slim_ctrl"
+
+/* Manager registers */
+#defineMGR_CFG 0x200
+#defineMGR_STATUS  0x204
+#defineMGR_INT_EN  0x210
+#defineMGR_INT_STAT0x214
+#defineMGR_INT_CLR 0x218