[PATCH 0/5] spmi:msm: Several fixes

[PATCH 0/5] spmi:msm: Several fixes

[Alexey Minnekhanov]

In the process of porting my board to u-boot I've noticed incorrect
behaviour, some things that clearly look wrong, and other strange
things. Here go several fixes to MSM SPMI driver, mostly related
to newer platforms support.

Alexey Minnekhanov (5):
  spmi: msm: Remove wrong and unused code
  spmi: msm: Fix parsing FDT and reading ARB version
  doc: spmi-msm: Update docs to reflect current state
  arm64: dts: qcom: Fix SPMI arbiter regs and reg-names
  spmi: msm: Fix up msm_spmi_write() for ARB V5

 arch/arm/dts/qcs404-evb.dts                |  7 ++--
 arch/arm/dts/sdm845.dtsi                   |  8 ++---
 doc/device-tree-bindings/spmi/spmi-msm.txt | 21 +++++++----
 drivers/spmi/spmi-msm.c                    | 41 +++++++++-------------
 4 files changed, 39 insertions(+), 38 deletions(-)

-- 
2.38.2

[PATCH 1/5] spmi: msm: Remove wrong and unused code

[Alexey Minnekhanov]

Variable err is never initialized and therefore not needed,
as well as the whole error handler block; the mentioned
"APID->PPID mapping table" is never read in the code anyways.

Signed-off-by: Alexey Minnekhanov <alexeymin@postmarketos.org>
---
 drivers/spmi/spmi-msm.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/spmi/spmi-msm.c b/drivers/spmi/spmi-msm.c
index 27a035c0a595..a9dcf5ab7f91 100644
--- a/drivers/spmi/spmi-msm.c
+++ b/drivers/spmi/spmi-msm.c
@@ -190,7 +190,6 @@ static int msm_spmi_probe(struct udevice *dev)
 	u32 hw_ver;
 	u32 version;
 	int i;
-	int err;
 
 	config_addr = dev_read_addr_index(dev, 0);
 	priv->spmi_core = dev_read_addr_index(dev, 1);
@@ -210,11 +209,6 @@ static int msm_spmi_probe(struct udevice *dev)
 		priv->arb_ver = V5;
 		version = 5;
 		priv->arb_chnl = config_addr + APID_MAP_OFFSET_V5;
-
-		if (err) {
-			dev_err(dev, "could not read APID->PPID mapping table, rc= %d\n", err);
-			return -1;
-		}
 	}
 
 	dev_dbg(dev, "PMIC Arb Version-%d (0x%x)\n", version, hw_ver);
-- 
2.38.2

Re: [PATCH 1/5] spmi: msm: Remove wrong and unused code

[Sumit Garg]

On Mon, 16 Jan 2023 at 06:03, Alexey Minnekhanov
<alexeymin@postmarketos.org> wrote:
>
> Variable err is never initialized and therefore not needed,
> as well as the whole error handler block; the mentioned
> "APID->PPID mapping table" is never read in the code anyways.
>
> Signed-off-by: Alexey Minnekhanov <alexeymin@postmarketos.org>
> ---
>  drivers/spmi/spmi-msm.c | 6 ------
>  1 file changed, 6 deletions(-)
>

Reviewed-by: Sumit Garg <sumit.garg@linaro.org>

> diff --git a/drivers/spmi/spmi-msm.c b/drivers/spmi/spmi-msm.c
> index 27a035c0a595..a9dcf5ab7f91 100644
> --- a/drivers/spmi/spmi-msm.c
> +++ b/drivers/spmi/spmi-msm.c
> @@ -190,7 +190,6 @@ static int msm_spmi_probe(struct udevice *dev)
>         u32 hw_ver;
>         u32 version;
>         int i;
> -       int err;
>
>         config_addr = dev_read_addr_index(dev, 0);
>         priv->spmi_core = dev_read_addr_index(dev, 1);
> @@ -210,11 +209,6 @@ static int msm_spmi_probe(struct udevice *dev)
>                 priv->arb_ver = V5;
>                 version = 5;
>                 priv->arb_chnl = config_addr + APID_MAP_OFFSET_V5;
> -
> -               if (err) {
> -                       dev_err(dev, "could not read APID->PPID mapping table, rc= %d\n", err);
> -                       return -1;
> -               }
>         }
>
>         dev_dbg(dev, "PMIC Arb Version-%d (0x%x)\n", version, hw_ver);
> --
> 2.38.2
>

[PATCH 2/5] spmi: msm: Fix parsing FDT and reading ARB version

[Alexey Minnekhanov]

First of all, use dev_read_addr_name() instead of
dev_read_addr_index() to avoid confusion: most dts files
have their regs specified in the wrong order, so driver
is reading config reg and using it instead of core reg.
Using names instead of indexes helps to avoid such errors.

Second, same as Linux driver, use core reg to read version
from [1]. This fixes reading incorrect arbiter version.

Third, print addresses in hex, so it can be visually
compared to values in DTS more easily.

[1]: https://elixir.bootlin.com/linux/v6.1.6/source/drivers/spmi/spmi-pmic-arb.c#L1339

Signed-off-by: Alexey Minnekhanov <alexeymin@postmarketos.org>
---
 drivers/spmi/spmi-msm.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/spmi/spmi-msm.c b/drivers/spmi/spmi-msm.c
index a9dcf5ab7f91..3df0f12c8b86 100644
--- a/drivers/spmi/spmi-msm.c
+++ b/drivers/spmi/spmi-msm.c
@@ -191,11 +191,12 @@ static int msm_spmi_probe(struct udevice *dev)
 	u32 version;
 	int i;
 
-	config_addr = dev_read_addr_index(dev, 0);
-	priv->spmi_core = dev_read_addr_index(dev, 1);
-	priv->spmi_obs = dev_read_addr_index(dev, 2);
+	/* DTS bindings: reg-names = "cnfg", "core", "obsrvr"; */
+	config_addr = dev_read_addr_name(dev, "cnfg");
+	priv->spmi_core = dev_read_addr_name(dev, "core");
+	priv->spmi_obs = dev_read_addr_name(dev, "obsrvr");
 
-	hw_ver = readl(config_addr + PMIC_ARB_VERSION);
+	hw_ver = readl(priv->spmi_core + PMIC_ARB_VERSION);
 
 	if (hw_ver < PMIC_ARB_VERSION_V3_MIN) {
 		priv->arb_ver = V2;
@@ -218,9 +219,10 @@ static int msm_spmi_probe(struct udevice *dev)
 	    priv->spmi_obs == FDT_ADDR_T_NONE)
 		return -EINVAL;
 
-	dev_dbg(dev, "priv->arb_chnl address (%llu)\n", priv->arb_chnl);
-	dev_dbg(dev, "priv->spmi_core address (%llu)\n", priv->spmi_core);
-	dev_dbg(dev, "priv->spmi_obs address (%llu)\n", priv->spmi_obs);
+	dev_dbg(dev, "priv->arb_chnl address (%llx)\n", priv->arb_chnl);
+	dev_dbg(dev, "priv->spmi_core address (%llx)\n", priv->spmi_core);
+	dev_dbg(dev, "priv->spmi_obs address (%llx)\n", priv->spmi_obs);
+
 	/* Scan peripherals connected to each SPMI channel */
 	for (i = 0; i < SPMI_MAX_PERIPH; i++) {
 		uint32_t periph = readl(priv->arb_chnl + ARB_CHANNEL_OFFSET(i));
-- 
2.38.2

Re: [PATCH 2/5] spmi: msm: Fix parsing FDT and reading ARB version

[Sumit Garg]

On Mon, 16 Jan 2023 at 06:03, Alexey Minnekhanov
<alexeymin@postmarketos.org> wrote:
>
> First of all, use dev_read_addr_name() instead of
> dev_read_addr_index() to avoid confusion: most dts files
> have their regs specified in the wrong order, so driver
> is reading config reg and using it instead of core reg.
> Using names instead of indexes helps to avoid such errors.
>
> Second, same as Linux driver, use core reg to read version
> from [1]. This fixes reading incorrect arbiter version.
>
> Third, print addresses in hex, so it can be visually
> compared to values in DTS more easily.
>
> [1]: https://elixir.bootlin.com/linux/v6.1.6/source/drivers/spmi/spmi-pmic-arb.c#L1339
>
> Signed-off-by: Alexey Minnekhanov <alexeymin@postmarketos.org>
> ---
>  drivers/spmi/spmi-msm.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)

Apart from nit below, feel free to add:

Reviewed-by: Sumit Garg <sumit.garg@linaro.org>

>
> diff --git a/drivers/spmi/spmi-msm.c b/drivers/spmi/spmi-msm.c
> index a9dcf5ab7f91..3df0f12c8b86 100644
> --- a/drivers/spmi/spmi-msm.c
> +++ b/drivers/spmi/spmi-msm.c
> @@ -191,11 +191,12 @@ static int msm_spmi_probe(struct udevice *dev)
>         u32 version;
>         int i;
>
> -       config_addr = dev_read_addr_index(dev, 0);
> -       priv->spmi_core = dev_read_addr_index(dev, 1);
> -       priv->spmi_obs = dev_read_addr_index(dev, 2);
> +       /* DTS bindings: reg-names = "cnfg", "core", "obsrvr"; */

nit: this comment is redundant as there is already DT bindings
documentation to look at.

-Sumit

> +       config_addr = dev_read_addr_name(dev, "cnfg");
> +       priv->spmi_core = dev_read_addr_name(dev, "core");
> +       priv->spmi_obs = dev_read_addr_name(dev, "obsrvr");
>
> -       hw_ver = readl(config_addr + PMIC_ARB_VERSION);
> +       hw_ver = readl(priv->spmi_core + PMIC_ARB_VERSION);
>
>         if (hw_ver < PMIC_ARB_VERSION_V3_MIN) {
>                 priv->arb_ver = V2;
> @@ -218,9 +219,10 @@ static int msm_spmi_probe(struct udevice *dev)
>             priv->spmi_obs == FDT_ADDR_T_NONE)
>                 return -EINVAL;
>
> -       dev_dbg(dev, "priv->arb_chnl address (%llu)\n", priv->arb_chnl);
> -       dev_dbg(dev, "priv->spmi_core address (%llu)\n", priv->spmi_core);
> -       dev_dbg(dev, "priv->spmi_obs address (%llu)\n", priv->spmi_obs);
> +       dev_dbg(dev, "priv->arb_chnl address (%llx)\n", priv->arb_chnl);
> +       dev_dbg(dev, "priv->spmi_core address (%llx)\n", priv->spmi_core);
> +       dev_dbg(dev, "priv->spmi_obs address (%llx)\n", priv->spmi_obs);
> +
>         /* Scan peripherals connected to each SPMI channel */
>         for (i = 0; i < SPMI_MAX_PERIPH; i++) {
>                 uint32_t periph = readl(priv->arb_chnl + ARB_CHANNEL_OFFSET(i));
> --
> 2.38.2
>

Re: [PATCH 2/5] spmi: msm: Fix parsing FDT and reading ARB version

[Alexey Minnekhanov]

On 2023-01-19 11:36, Sumit Garg wrote:
> On Mon, 16 Jan 2023 at 06:03, Alexey Minnekhanov
> <alexeymin@postmarketos.org> wrote:
...
>> -       config_addr = dev_read_addr_index(dev, 0);
>> -       priv->spmi_core = dev_read_addr_index(dev, 1);
>> -       priv->spmi_obs = dev_read_addr_index(dev, 2);
>> +       /* DTS bindings: reg-names = "cnfg", "core", "obsrvr"; */
> 
> nit: this comment is redundant as there is already DT bindings
> documentation to look at.
> 
> -Sumit
> 

Thanks, will remove it in v2.
--
Alexey Minnekhanov

[PATCH 3/5] doc: spmi-msm: Update docs to reflect current state

[Alexey Minnekhanov]

Update spmi-msm documentation and example to reflect the current
state of the driver.

Signed-off-by: Alexey Minnekhanov <alexeymin@postmarketos.org>
---
 doc/device-tree-bindings/spmi/spmi-msm.txt | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/doc/device-tree-bindings/spmi/spmi-msm.txt b/doc/device-tree-bindings/spmi/spmi-msm.txt
index ae47673b768b..cd0380b723b9 100644
--- a/doc/device-tree-bindings/spmi/spmi-msm.txt
+++ b/doc/device-tree-bindings/spmi/spmi-msm.txt
@@ -1,13 +1,17 @@
 Qualcomm SPMI arbiter/bus driver
 
 This is bus driver for Qualcomm chips that use SPMI to communicate with PMICs.
+The SPMI PMIC Arbiter is found on Snapdragon chipsets. It is an SPMI
+controller with wrapping arbitration logic to allow for multiple on-chip
+devices to control a single SPMI master.
 
 Required properties:
 - compatible: "qcom,spmi-pmic-arb"
 - reg: Register block adresses and sizes for various parts of device:
-   1) PMIC arbiter channel mapping base (PMIC_ARB_REG_CHNLn)
-   2) SPMI write command (master) registers (PMIC_ARB_CORE_SW_DEC_CHANNELS)
-   3) SPMI read command (observer) registers (PMIC_ARB_CORE_REGISTERS_OBS)
+   1) PMIC arbiter core registers
+   2) SPMI configuration registers
+   3) SPMI read command (observer) registers
+- reg-names: "core", "cnfg", "obsrvr" for corresponding reg values
 
 Optional properties (if not set by parent):
 - #address-cells: 0x1 - childs slave ID address
@@ -18,9 +22,12 @@ Automatic detection of childs is currently not supported.
 
 Example:
 
-spmi@200f000 {
+spmi@fc4cf000 {
 	compatible = "qcom,spmi-pmic-arb";
-	reg = <0x200f800 0x200 0x2400000 0x400000 0x2c00000 0x400000>;
-	#address-cells = <0x1>;
-	#size-cells = <0x1>;
+	reg = <0xfc4cf000 0x1000>,
+	      <0xfc4cb000 0x1000>,
+	      <0xfc4ca000 0x1000>;
+	reg-names = "core", "cnfg", "obsrvr";
+	#address-cells = <1>;
+	#size-cells = <1>;
 };
-- 
2.38.2

Re: [PATCH 3/5] doc: spmi-msm: Update docs to reflect current state

[Sumit Garg]

On Mon, 16 Jan 2023 at 06:04, Alexey Minnekhanov
<alexeymin@postmarketos.org> wrote:
>
> Update spmi-msm documentation and example to reflect the current
> state of the driver.
>
> Signed-off-by: Alexey Minnekhanov <alexeymin@postmarketos.org>
> ---
>  doc/device-tree-bindings/spmi/spmi-msm.txt | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
>

Since our long term goal is to align with Linux DT bindings and use
the DTS imported from Linux as is. So we should rather get rid of
these redundant bindings in u-boot unless there isn't any u-boot
specific separate bindings requirement which isn't the case here.

-Sumit

> diff --git a/doc/device-tree-bindings/spmi/spmi-msm.txt b/doc/device-tree-bindings/spmi/spmi-msm.txt
> index ae47673b768b..cd0380b723b9 100644
> --- a/doc/device-tree-bindings/spmi/spmi-msm.txt
> +++ b/doc/device-tree-bindings/spmi/spmi-msm.txt
> @@ -1,13 +1,17 @@
>  Qualcomm SPMI arbiter/bus driver
>
>  This is bus driver for Qualcomm chips that use SPMI to communicate with PMICs.
> +The SPMI PMIC Arbiter is found on Snapdragon chipsets. It is an SPMI
> +controller with wrapping arbitration logic to allow for multiple on-chip
> +devices to control a single SPMI master.
>
>  Required properties:
>  - compatible: "qcom,spmi-pmic-arb"
>  - reg: Register block adresses and sizes for various parts of device:
> -   1) PMIC arbiter channel mapping base (PMIC_ARB_REG_CHNLn)
> -   2) SPMI write command (master) registers (PMIC_ARB_CORE_SW_DEC_CHANNELS)
> -   3) SPMI read command (observer) registers (PMIC_ARB_CORE_REGISTERS_OBS)
> +   1) PMIC arbiter core registers
> +   2) SPMI configuration registers
> +   3) SPMI read command (observer) registers
> +- reg-names: "core", "cnfg", "obsrvr" for corresponding reg values
>
>  Optional properties (if not set by parent):
>  - #address-cells: 0x1 - childs slave ID address
> @@ -18,9 +22,12 @@ Automatic detection of childs is currently not supported.
>
>  Example:
>
> -spmi@200f000 {
> +spmi@fc4cf000 {
>         compatible = "qcom,spmi-pmic-arb";
> -       reg = <0x200f800 0x200 0x2400000 0x400000 0x2c00000 0x400000>;
> -       #address-cells = <0x1>;
> -       #size-cells = <0x1>;
> +       reg = <0xfc4cf000 0x1000>,
> +             <0xfc4cb000 0x1000>,
> +             <0xfc4ca000 0x1000>;
> +       reg-names = "core", "cnfg", "obsrvr";
> +       #address-cells = <1>;
> +       #size-cells = <1>;
>  };
> --
> 2.38.2
>

Re: [PATCH 3/5] doc: spmi-msm: Update docs to reflect current state

[Alexey Minnekhanov]

On 2023-01-19 11:44, Sumit Garg wrote:
> On Mon, 16 Jan 2023 at 06:04, Alexey Minnekhanov
> <alexeymin@postmarketos.org> wrote:
>>
>> Update spmi-msm documentation and example to reflect the current
>> state of the driver.
>>
> 
> Since our long term goal is to align with Linux DT bindings and use
> the DTS imported from Linux as is. So we should rather get rid of
> these redundant bindings in u-boot unless there isn't any u-boot
> specific separate bindings requirement which isn't the case here.
> 
> -Sumit
> 
So, you suggest I just remove these docs from u-boot completely instead 
of fixing them?

--
Alexey Minnekhanov

Re: [PATCH 3/5] doc: spmi-msm: Update docs to reflect current state

[Sumit Garg]

On Sat, 21 Jan 2023 at 08:26, Alexey Minnekhanov
<alexeymin@postmarketos.org> wrote:
>
> On 2023-01-19 11:44, Sumit Garg wrote:
> > On Mon, 16 Jan 2023 at 06:04, Alexey Minnekhanov
> > <alexeymin@postmarketos.org> wrote:
> >>
> >> Update spmi-msm documentation and example to reflect the current
> >> state of the driver.
> >>
> >
> > Since our long term goal is to align with Linux DT bindings and use
> > the DTS imported from Linux as is. So we should rather get rid of
> > these redundant bindings in u-boot unless there isn't any u-boot
> > specific separate bindings requirement which isn't the case here.
> >
> > -Sumit
> >
> So, you suggest I just remove these docs from u-boot completely instead
> of fixing them?

Yeah, IMO it adds no value to maintain redundant copies of DT bindings
in u-boot.

-Sumit

>
> --
> Alexey Minnekhanov

[PATCH 4/5] arm64: dts: qcom: Fix SPMI arbiter regs and reg-names

[Alexey Minnekhanov]

Now that reg-names is required, specify them, and use correct
addresses for SPMI arbiter regs, taken from Linux dts [1] [2].

[1] https://elixir.bootlin.com/linux/v6.1.5/source/arch/arm64/boot/dts/qcom/qcs404.dtsi#L739
[2] https://elixir.bootlin.com/linux/v6.1.5/source/arch/arm64/boot/dts/qcom/sdm845.dtsi#L4896

Signed-off-by: Alexey Minnekhanov <alexeymin@postmarketos.org>
---
 arch/arm/dts/qcs404-evb.dts | 7 ++++---
 arch/arm/dts/sdm845.dtsi    | 8 ++++----
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/arch/arm/dts/qcs404-evb.dts b/arch/arm/dts/qcs404-evb.dts
index 0639af8fe336..6bf6736139ed 100644
--- a/arch/arm/dts/qcs404-evb.dts
+++ b/arch/arm/dts/qcs404-evb.dts
@@ -171,9 +171,10 @@
 
 		spmi@200f000 {
 			compatible = "qcom,spmi-pmic-arb";
-			reg = <0x200f000 0x1000
-			       0x2400000 0x400000
-			       0x2c00000 0x400000>;
+			reg = <0x0200f000 0x1000>,
+			      <0x0200a000 0x2100>,
+			      <0x02c00000 0x800000>;
+			reg-names = "core", "cnfg", "obsrvr";
 			#address-cells = <0x1>;
 			#size-cells = <0x1>;
 
diff --git a/arch/arm/dts/sdm845.dtsi b/arch/arm/dts/sdm845.dtsi
index 607af277f8be..2bb0954133c5 100644
--- a/arch/arm/dts/sdm845.dtsi
+++ b/arch/arm/dts/sdm845.dtsi
@@ -65,10 +65,10 @@
 
 		spmi@c440000 {
 			compatible = "qcom,spmi-pmic-arb";
-			reg = <0xc440000 0x1100>,
-			      <0xc600000 0x2000000>,
-			      <0xe600000 0x100000>;
-			reg-names = "cnfg", "core", "obsrvr";
+			reg = <0x0c440000 0x1100>,
+			      <0x0c40a000 0x26000>,
+			      <0x0e600000 0x100000>;
+			reg-names = "core", "cnfg", "obsrvr";
 			#address-cells = <0x1>;
 			#size-cells = <0x1>;
 
-- 
2.38.2

Re: [PATCH 4/5] arm64: dts: qcom: Fix SPMI arbiter regs and reg-names

[Sumit Garg]

On Mon, 16 Jan 2023 at 06:04, Alexey Minnekhanov
<alexeymin@postmarketos.org> wrote:
>
> Now that reg-names is required, specify them, and use correct
> addresses for SPMI arbiter regs, taken from Linux dts [1] [2].
>
> [1] https://elixir.bootlin.com/linux/v6.1.5/source/arch/arm64/boot/dts/qcom/qcs404.dtsi#L739
> [2] https://elixir.bootlin.com/linux/v6.1.5/source/arch/arm64/boot/dts/qcom/sdm845.dtsi#L4896
>

Since you have already aligned the u-boot driver to work with Linux DT
binding, we should rather exactly match spmi DT nodes as in Linux dts
files.

-Sumit

> Signed-off-by: Alexey Minnekhanov <alexeymin@postmarketos.org>
> ---
>  arch/arm/dts/qcs404-evb.dts | 7 ++++---
>  arch/arm/dts/sdm845.dtsi    | 8 ++++----
>  2 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/arch/arm/dts/qcs404-evb.dts b/arch/arm/dts/qcs404-evb.dts
> index 0639af8fe336..6bf6736139ed 100644
> --- a/arch/arm/dts/qcs404-evb.dts
> +++ b/arch/arm/dts/qcs404-evb.dts
> @@ -171,9 +171,10 @@
>
>                 spmi@200f000 {
>                         compatible = "qcom,spmi-pmic-arb";
> -                       reg = <0x200f000 0x1000
> -                              0x2400000 0x400000
> -                              0x2c00000 0x400000>;
> +                       reg = <0x0200f000 0x1000>,
> +                             <0x0200a000 0x2100>,
> +                             <0x02c00000 0x800000>;
> +                       reg-names = "core", "cnfg", "obsrvr";
>                         #address-cells = <0x1>;
>                         #size-cells = <0x1>;
>
> diff --git a/arch/arm/dts/sdm845.dtsi b/arch/arm/dts/sdm845.dtsi
> index 607af277f8be..2bb0954133c5 100644
> --- a/arch/arm/dts/sdm845.dtsi
> +++ b/arch/arm/dts/sdm845.dtsi
> @@ -65,10 +65,10 @@
>
>                 spmi@c440000 {
>                         compatible = "qcom,spmi-pmic-arb";
> -                       reg = <0xc440000 0x1100>,
> -                             <0xc600000 0x2000000>,
> -                             <0xe600000 0x100000>;
> -                       reg-names = "cnfg", "core", "obsrvr";
> +                       reg = <0x0c440000 0x1100>,
> +                             <0x0c40a000 0x26000>,
> +                             <0x0e600000 0x100000>;
> +                       reg-names = "core", "cnfg", "obsrvr";
>                         #address-cells = <0x1>;
>                         #size-cells = <0x1>;
>
> --
> 2.38.2
>

Re: [PATCH 4/5] arm64: dts: qcom: Fix SPMI arbiter regs and reg-names

[Alexey Minnekhanov]

On 2023-01-19 11:47, Sumit Garg wrote:
> On Mon, 16 Jan 2023 at 06:04, Alexey Minnekhanov
> <alexeymin@postmarketos.org> wrote:
>>
>> Now that reg-names is required, specify them, and use correct
>> addresses for SPMI arbiter regs, taken from Linux dts [1] [2].
>>
>> [1] https://elixir.bootlin.com/linux/v6.1.5/source/arch/arm64/boot/dts/qcom/qcs404.dtsi#L739
>> [2] https://elixir.bootlin.com/linux/v6.1.5/source/arch/arm64/boot/dts/qcom/sdm845.dtsi#L4896
>>
> 
> Since you have already aligned the u-boot driver to work with Linux DT
> binding, we should rather exactly match spmi DT nodes as in Linux dts
> files.
> 
> -Sumit
> 

It can be done easier for sdm845, but there is no exact file named
qcs404-evb.dts in Linux with spmi node in it. File structure in Linux
is a bit different. There qcs404-evb.dtsi includes qcs404.dtsi.

I can take spmi node from Linux's qcs404.dtsi and put it here in 
qcs404-evb.dts, but it won't be 1:1 match to Linux DT.

If that would be OK, I can do it in v2?

--
Alexey Minnekhanov

Re: [PATCH 4/5] arm64: dts: qcom: Fix SPMI arbiter regs and reg-names

[Sumit Garg]

On Sat, 21 Jan 2023 at 08:38, Alexey Minnekhanov
<alexeymin@postmarketos.org> wrote:
>
> On 2023-01-19 11:47, Sumit Garg wrote:
> > On Mon, 16 Jan 2023 at 06:04, Alexey Minnekhanov
> > <alexeymin@postmarketos.org> wrote:
> >>
> >> Now that reg-names is required, specify them, and use correct
> >> addresses for SPMI arbiter regs, taken from Linux dts [1] [2].
> >>
> >> [1] https://elixir.bootlin.com/linux/v6.1.5/source/arch/arm64/boot/dts/qcom/qcs404.dtsi#L739
> >> [2] https://elixir.bootlin.com/linux/v6.1.5/source/arch/arm64/boot/dts/qcom/sdm845.dtsi#L4896
> >>
> >
> > Since you have already aligned the u-boot driver to work with Linux DT
> > binding, we should rather exactly match spmi DT nodes as in Linux dts
> > files.
> >
> > -Sumit
> >
>
> It can be done easier for sdm845, but there is no exact file named
> qcs404-evb.dts in Linux with spmi node in it. File structure in Linux
> is a bit different. There qcs404-evb.dtsi includes qcs404.dtsi.

Once we have all the u-boot drivers (pending ones are pinctrl, uart
etc.) aligned to Linux DT binding then we should be able to directly
import those dts/dtsi files.

>
> I can take spmi node from Linux's qcs404.dtsi and put it here in
> qcs404-evb.dts, but it won't be 1:1 match to Linux DT.
>
> If that would be OK, I can do it in v2?

Yeah that's fine with me.

-Sumit

>
> --
> Alexey Minnekhanov

[PATCH 5/5] spmi: msm: Fix up msm_spmi_write() for ARB V5

[Alexey Minnekhanov]

In commit f5a2d6b4b03a ("spmi: msm: add arbiter version 5 support")
support for arbiter V5 was introduced, and msm_spmi_read() was
correctly converted to use varying channel offset depending on
ARB version. But msm_spmi_write() was not fully converted.

Even though ch_offset variable was introduced, it was not used
in read/write operations in that function. Fix this inconsistency,
use calculated ch_offset instead of SPMI_CH_OFFSET(..).

Fixes: f5a2d6b4b03a ("spmi: msm: add arbiter version 5 support")

Signed-off-by: Alexey Minnekhanov <alexeymin@postmarketos.org>
---
 drivers/spmi/spmi-msm.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/spmi/spmi-msm.c b/drivers/spmi/spmi-msm.c
index 3df0f12c8b86..2174c10c920a 100644
--- a/drivers/spmi/spmi-msm.c
+++ b/drivers/spmi/spmi-msm.c
@@ -92,13 +92,16 @@ static int msm_spmi_write(struct udevice *dev, int usid, int pid, int off,
 		return -EIO;
 
 	channel = priv->channel_map[usid][pid];
+	if (priv->arb_ver == V5)
+		ch_offset = SPMI_V5_RW_CH_OFFSET(channel);
+	else
+		ch_offset = SPMI_CH_OFFSET(channel);
 
 	/* Disable IRQ mode for the current channel*/
-	writel(0x0,
-	       priv->spmi_core + SPMI_CH_OFFSET(channel) + SPMI_REG_CONFIG);
+	writel(0x0, priv->spmi_core + ch_offset + SPMI_REG_CONFIG);
 
 	/* Write single byte */
-	writel(val, priv->spmi_core + SPMI_CH_OFFSET(channel) + SPMI_REG_WDATA);
+	writel(val, priv->spmi_core + ch_offset + SPMI_REG_WDATA);
 
 	/* Prepare write command */
 	reg |= SPMI_CMD_EXT_REG_WRITE_LONG << SPMI_CMD_OPCODE_SHIFT;
@@ -107,19 +110,13 @@ static int msm_spmi_write(struct udevice *dev, int usid, int pid, int off,
 	reg |= (off << SPMI_CMD_ADDR_OFFSET_SHIFT);
 	reg |= 1; /* byte count */
 
-	if (priv->arb_ver == V5)
-		ch_offset = SPMI_V5_RW_CH_OFFSET(channel);
-	else
-		ch_offset = SPMI_CH_OFFSET(channel);
-
 	/* Send write command */
-	writel(reg, priv->spmi_core + SPMI_CH_OFFSET(channel) + SPMI_REG_CMD0);
+	writel(reg, priv->spmi_core + ch_offset + SPMI_REG_CMD0);
 
 	/* Wait till CMD DONE status */
 	reg = 0;
 	while (!reg) {
-		reg = readl(priv->spmi_core + SPMI_CH_OFFSET(channel) +
-			    SPMI_REG_STATUS);
+		reg = readl(priv->spmi_core + ch_offset + SPMI_REG_STATUS);
 	}
 
 	if (reg ^ SPMI_STATUS_DONE) {
-- 
2.38.2

Re: [PATCH 5/5] spmi: msm: Fix up msm_spmi_write() for ARB V5

[Sumit Garg]

On Mon, 16 Jan 2023 at 06:04, Alexey Minnekhanov
<alexeymin@postmarketos.org> wrote:
>
> In commit f5a2d6b4b03a ("spmi: msm: add arbiter version 5 support")
> support for arbiter V5 was introduced, and msm_spmi_read() was
> correctly converted to use varying channel offset depending on
> ARB version. But msm_spmi_write() was not fully converted.
>
> Even though ch_offset variable was introduced, it was not used
> in read/write operations in that function. Fix this inconsistency,
> use calculated ch_offset instead of SPMI_CH_OFFSET(..).
>
> Fixes: f5a2d6b4b03a ("spmi: msm: add arbiter version 5 support")
>
> Signed-off-by: Alexey Minnekhanov <alexeymin@postmarketos.org>
> ---
>  drivers/spmi/spmi-msm.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
>

Reviewed-by: Sumit Garg <sumit.garg@linaro.org>

-Sumit

> diff --git a/drivers/spmi/spmi-msm.c b/drivers/spmi/spmi-msm.c
> index 3df0f12c8b86..2174c10c920a 100644
> --- a/drivers/spmi/spmi-msm.c
> +++ b/drivers/spmi/spmi-msm.c
> @@ -92,13 +92,16 @@ static int msm_spmi_write(struct udevice *dev, int usid, int pid, int off,
>                 return -EIO;
>
>         channel = priv->channel_map[usid][pid];
> +       if (priv->arb_ver == V5)
> +               ch_offset = SPMI_V5_RW_CH_OFFSET(channel);
> +       else
> +               ch_offset = SPMI_CH_OFFSET(channel);
>
>         /* Disable IRQ mode for the current channel*/
> -       writel(0x0,
> -              priv->spmi_core + SPMI_CH_OFFSET(channel) + SPMI_REG_CONFIG);
> +       writel(0x0, priv->spmi_core + ch_offset + SPMI_REG_CONFIG);
>
>         /* Write single byte */
> -       writel(val, priv->spmi_core + SPMI_CH_OFFSET(channel) + SPMI_REG_WDATA);
> +       writel(val, priv->spmi_core + ch_offset + SPMI_REG_WDATA);
>
>         /* Prepare write command */
>         reg |= SPMI_CMD_EXT_REG_WRITE_LONG << SPMI_CMD_OPCODE_SHIFT;
> @@ -107,19 +110,13 @@ static int msm_spmi_write(struct udevice *dev, int usid, int pid, int off,
>         reg |= (off << SPMI_CMD_ADDR_OFFSET_SHIFT);
>         reg |= 1; /* byte count */
>
> -       if (priv->arb_ver == V5)
> -               ch_offset = SPMI_V5_RW_CH_OFFSET(channel);
> -       else
> -               ch_offset = SPMI_CH_OFFSET(channel);
> -
>         /* Send write command */
> -       writel(reg, priv->spmi_core + SPMI_CH_OFFSET(channel) + SPMI_REG_CMD0);
> +       writel(reg, priv->spmi_core + ch_offset + SPMI_REG_CMD0);
>
>         /* Wait till CMD DONE status */
>         reg = 0;
>         while (!reg) {
> -               reg = readl(priv->spmi_core + SPMI_CH_OFFSET(channel) +
> -                           SPMI_REG_STATUS);
> +               reg = readl(priv->spmi_core + ch_offset + SPMI_REG_STATUS);
>         }
>
>         if (reg ^ SPMI_STATUS_DONE) {
> --
> 2.38.2
>