mbox series

[v3,0/4] soc: samsung: usi: implement support for USIv1

Message ID 20250105160346.418829-1-ivo.ivanov.ivanov1@gmail.com
Headers show
Series soc: samsung: usi: implement support for USIv1 | expand

Message

Ivaylo Ivanov Jan. 5, 2025, 4:03 p.m. UTC
Hey folks,

This patch series adds support for USIv1 in the existing exynos-usi driver,
as well as dedicated sysreg compatibles for exynos8895.

The USIv1 IP-core is found on some ARM64 Exynos SoCs (like Exynos8895).
It provides selectable serial protocols (one of: HSI2C0, HSI2C1, HSI2C0_1,
SPI, UART, UART_HSI2C1). It's a bit different from USIv2 as it doesn't
have any known MMIO register map and the serial protocols that it
implements share the same register base, hence why the way of modelling it
in device trees will be with ranges, like so:

usi1: usi@10460000 {
  compatible = "samsung,exynos8895-usi";
  ranges = <0x0 0x10460000 0x11000>;
  clocks = <1>, <2>;
  clock-names = "pclk", "ipclk";
  #address-cells = <1>;
  #size-cells = <1>;
  samsung,sysreg = <&syscon_peric0 0x1004>;
  status = "disabled";

  hsi2c_5: i2c@0 {
    compatible = "samsung,exynos8895-hsi2c";
    reg = <0x0 0x1000>;
    ...
  };
};

This patchset also assumes that [1] and [2] have been merged before it.

Best regards,
Ivaylo

[1]: https://lore.kernel.org/all/20241222145257.31451-1-krzysztof.kozlowski@linaro.org/
[2]: https://lore.kernel.org/all/20250103082549.19419-1-krzysztof.kozlowski@linaro.org/

Changes in v3:
  - drop the sysreg patch as it was applied
  - add a patch at the beginning of the series for renaming all USI_V2
    constants to USI_MODE_ and a patch at the end to rename them in dt
  - redo the usi binding support for 8895 to hopefully match all
    feedback from Krzysztof
  - change the description of the usiv1 and 8895 binding patch in order
    to account for the constants changes
  - change the subject and description of the usiv1 driver support patch
    because we're adding support for exynos8895 in the first place
  - make exynos_usi_modes a two dimensional array while also accounting
    for the merged usi mode constants

Changes in v2:
  - add r-b from Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
  - restrict the possible ids of samsung,mode with an allOf:if:then
  - set the properties samsung,clkreq-on and reg to false for non-usiv2
  - only make use of exynos_usi_modes
  - make sure pclk and ipclk are enabled

Ivaylo Ivanov (4):
  dt-bindings: soc: samsung: usi: replace USI_V2 in constants with
    USI_MODE
  dt-bindings: soc: samsung: usi: add USIv1 and samsung,exynos8895-usi
  soc: samsung: usi: implement support for USIv1 and exynos8895
  arm64: dts: exynos: update all samsung,mode constants

 .../bindings/soc/samsung/exynos-usi.yaml      | 101 ++++++++++++------
 arch/arm64/boot/dts/exynos/exynos850.dtsi     |  14 +--
 arch/arm64/boot/dts/exynos/exynosautov9.dtsi  |  48 ++++-----
 .../arm64/boot/dts/exynos/exynosautov920.dtsi |   2 +-
 .../boot/dts/exynos/google/gs101-oriole.dts   |   4 +-
 arch/arm64/boot/dts/exynos/google/gs101.dtsi  |   2 +-
 drivers/soc/samsung/exynos-usi.c              |  65 ++++++++---
 include/dt-bindings/soc/samsung,exynos-usi.h  |  11 +-
 8 files changed, 163 insertions(+), 84 deletions(-)

Comments

Krzysztof Kozlowski Jan. 6, 2025, 6:24 a.m. UTC | #1
On Sun, Jan 05, 2025 at 06:03:43PM +0200, Ivaylo Ivanov wrote:
> diff --git a/include/dt-bindings/soc/samsung,exynos-usi.h b/include/dt-bindings/soc/samsung,exynos-usi.h
> index a01af169d..b7c1406f3 100644
> --- a/include/dt-bindings/soc/samsung,exynos-usi.h
> +++ b/include/dt-bindings/soc/samsung,exynos-usi.h
> @@ -9,9 +9,9 @@
>  #ifndef __DT_BINDINGS_SAMSUNG_EXYNOS_USI_H
>  #define __DT_BINDINGS_SAMSUNG_EXYNOS_USI_H
>  
> -#define USI_V2_NONE		0
> -#define USI_V2_UART		1
> -#define USI_V2_SPI		2
> -#define USI_V2_I2C		3
> +#define USI_MODE_NONE		0
> +#define USI_MODE_UART		1
> +#define USI_MODE_SPI		2
> +#define USI_MODE_I2C		3

This breaks ABI and does not build. You still need also:
	#define USI_V2_NONE 		USI_MODE_NONE
and same for the rest.

Best regards,
Krzysztof
Krzysztof Kozlowski Jan. 6, 2025, 7:02 a.m. UTC | #2
On Sun, Jan 05, 2025 at 06:03:45PM +0200, Ivaylo Ivanov wrote:
> @@ -169,9 +207,12 @@ static int exynos_usi_configure(struct exynos_usi *usi)
>  		return ret;
>  
>  	if (usi->data->ver == USI_VER2)
> -		return exynos_usi_enable(usi);
> +		ret = exynos_usi_enable(usi);
> +	else
> +		ret = clk_bulk_prepare_enable(usi->data->num_clks,
> +					      usi->clks);

You need now exynos_usi_remove() callback and also error path after
populate at end of probe.

Best regards,
Krzysztof
Tudor Ambarus Jan. 6, 2025, 7:36 a.m. UTC | #3
Hiya,

On 1/5/25 4:03 PM, Ivaylo Ivanov wrote:
> +#define USI_MODE_NONE		0
> +#define USI_MODE_UART		1
> +#define USI_MODE_SPI		2
> +#define USI_MODE_I2C		3

USI_CONFIG register refers to the protocol selection with USI_I2C,
USI_SPI, USI_UART. How about getting rid of the MODE from the name?

Cheers,
ta
Krzysztof Kozlowski Jan. 6, 2025, 8:50 a.m. UTC | #4
On 06/01/2025 08:41, Ivaylo Ivanov wrote:
> On 1/6/25 09:36, Tudor Ambarus wrote:
>> Hiya,
>>
>> On 1/5/25 4:03 PM, Ivaylo Ivanov wrote:
>>> +#define USI_MODE_NONE		0
>>> +#define USI_MODE_UART		1
>>> +#define USI_MODE_SPI		2
>>> +#define USI_MODE_I2C		3
>> USI_CONFIG register refers to the protocol selection with USI_I2C,
>> USI_SPI, USI_UART. How about getting rid of the MODE from the name?
> 
> I thought about that too but I believe that mentioning that these constants
> are for mode selection in their name is generally a good practice. Let me know
> if dropping _MODE is really needed.

I am fine with both, MODE feels a bit more descriptive indicating how
the USI should be configured.

Best regards,
Krzysztof