Re: [PATCH v4 2/3] soc: samsung: usi: implement support for USIv1 and exynos8895

[Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>]

On 1/8/25 10:30, Krzysztof Kozlowski wrote:
> On Tue, Jan 07, 2025 at 01:35:11PM +0200, Ivaylo Ivanov wrote:
>> USIv1 IP-core is found on some ARM64 Exynos SoCs (like Exynos8895) and
>> provides selectable serial protocols (one of: HSI2C0, HSI2C1, HSI2C0_1,
>> SPI, UART, UART_HSI2C1).
>>
>> USIv1, unlike USIv2, doesn't have any known register map. Underlying
>> protocols that it implements have no offset, like with Exynos850.
>> Desired protocol can be chosen via SW_CONF register from System
>> Register block of the same domain as USI.
>>
>> In order to select a particular protocol, the protocol has to be
>> selected via the System Register. Unlike USIv2, there's no need for
>> any setup before the given protocol becomes accessible apart from
>> enabling the APB clock and the protocol operating clock.
>>
>> Modify the existing driver in order to allow USIv1 instances in
>> Exynos8895 to probe and set their protocol. While we're at it,
>> make use of the new mode constants in place of the old ones
>> and add a removal routine.
>>
>> Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>
>> ---
>>  drivers/soc/samsung/exynos-usi.c | 108 +++++++++++++++++++++++++++----
>>  1 file changed, 95 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/soc/samsung/exynos-usi.c b/drivers/soc/samsung/exynos-usi.c
>> index 114352695..43c17b100 100644
>> --- a/drivers/soc/samsung/exynos-usi.c
>> +++ b/drivers/soc/samsung/exynos-usi.c
>> @@ -16,6 +16,18 @@
>>  
>>  #include <dt-bindings/soc/samsung,exynos-usi.h>
>>  
>> +/* USIv1: System Register: SW_CONF register bits */
>> +#define USI_V1_SW_CONF_NONE		0x0
>> +#define USI_V1_SW_CONF_I2C0		0x1
>> +#define USI_V1_SW_CONF_I2C1		0x2
>> +#define USI_V1_SW_CONF_I2C0_1		0x3
>> +#define USI_V1_SW_CONF_SPI		0x4
>> +#define USI_V1_SW_CONF_UART		0x8
>> +#define USI_V1_SW_CONF_UART_I2C1	0xa
>> +#define USI_V1_SW_CONF_MASK		(USI_V1_SW_CONF_I2C0 | USI_V1_SW_CONF_I2C1 | \
>> +					 USI_V1_SW_CONF_I2C0_1 | USI_V1_SW_CONF_SPI | \
>> +					 USI_V1_SW_CONF_UART | USI_V1_SW_CONF_UART_I2C1)
>> +
>>  /* USIv2: System Register: SW_CONF register bits */
>>  #define USI_V2_SW_CONF_NONE	0x0
>>  #define USI_V2_SW_CONF_UART	BIT(0)
>> @@ -34,7 +46,8 @@
>>  #define USI_OPTION_CLKSTOP_ON	BIT(2)
>>  
>>  enum exynos_usi_ver {
>> -	USI_VER2 = 2,
>> +	USI_VER1 = 1,
> Is this assignment=1 actually now helping? Isn't it creating empty item
> in exynos_usi_modes array? Basically it wastes space in the array for
> no benefits.

I wanted to keep the USIv2 enum the same.

>
>> +	USI_VER2,
>>  };
>>  
>>  struct exynos_usi_variant {
>> @@ -66,19 +79,39 @@ struct exynos_usi_mode {
>>  	unsigned int val;		/* mode register value */
>>  };
>>  
>> -static const struct exynos_usi_mode exynos_usi_modes[] = {
>> -	[USI_V2_NONE] =	{ .name = "none", .val = USI_V2_SW_CONF_NONE },
>> -	[USI_V2_UART] =	{ .name = "uart", .val = USI_V2_SW_CONF_UART },
>> -	[USI_V2_SPI] =	{ .name = "spi",  .val = USI_V2_SW_CONF_SPI },
>> -	[USI_V2_I2C] =	{ .name = "i2c",  .val = USI_V2_SW_CONF_I2C },
>> +#define USI_MODES_MAX (USI_MODE_UART_I2C1 + 1)
>> +static const struct exynos_usi_mode exynos_usi_modes[][USI_MODES_MAX] = {
>> +	[USI_VER1] = {
>> +		[USI_MODE_NONE] =	{ .name = "none", .val = USI_V1_SW_CONF_NONE },
>> +		[USI_MODE_UART] =	{ .name = "uart", .val = USI_V1_SW_CONF_UART },
>> +		[USI_MODE_SPI] =	{ .name = "spi",  .val = USI_V1_SW_CONF_SPI },
>> +		[USI_MODE_I2C] =	{ .name = "i2c",  .val = USI_V1_SW_CONF_I2C0 },
>> +		[USI_MODE_I2C1] =	{ .name = "i2c1", .val = USI_V1_SW_CONF_I2C1 },
>> +		[USI_MODE_I2C0_1] =	{ .name = "i2c0_1", .val = USI_V1_SW_CONF_I2C0_1 },
>> +		[USI_MODE_UART_I2C1] =	{ .name = "uart_i2c1", .val = USI_V1_SW_CONF_UART_I2C1 },
>> +	}, [USI_VER2] = {
>> +		[USI_MODE_NONE] =	{ .name = "none", .val = USI_V2_SW_CONF_NONE },
>> +		[USI_MODE_UART] =	{ .name = "uart", .val = USI_V2_SW_CONF_UART },
>> +		[USI_MODE_SPI] =	{ .name = "spi",  .val = USI_V2_SW_CONF_SPI },
>> +		[USI_MODE_I2C] =	{ .name = "i2c",  .val = USI_V2_SW_CONF_I2C },
>> +	},
>>  };
>>  
>>  static const char * const exynos850_usi_clk_names[] = { "pclk", "ipclk" };
>>  static const struct exynos_usi_variant exynos850_usi_data = {
>>  	.ver		= USI_VER2,
>>  	.sw_conf_mask	= USI_V2_SW_CONF_MASK,
>> -	.min_mode	= USI_V2_NONE,
>> -	.max_mode	= USI_V2_I2C,
>> +	.min_mode	= USI_MODE_NONE,
>> +	.max_mode	= USI_MODE_I2C,
>> +	.num_clks	= ARRAY_SIZE(exynos850_usi_clk_names),
>> +	.clk_names	= exynos850_usi_clk_names,
>> +};
>> +
>> +static const struct exynos_usi_variant exynos8895_usi_data = {
>> +	.ver		= USI_VER1,
>> +	.sw_conf_mask	= USI_V1_SW_CONF_MASK,
>> +	.min_mode	= USI_MODE_NONE,
>> +	.max_mode	= USI_MODE_UART_I2C1,
>>  	.num_clks	= ARRAY_SIZE(exynos850_usi_clk_names),
>>  	.clk_names	= exynos850_usi_clk_names,
>>  };
>> @@ -88,6 +121,10 @@ static const struct of_device_id exynos_usi_dt_match[] = {
>>  		.compatible = "samsung,exynos850-usi",
>>  		.data = &exynos850_usi_data,
>>  	},
>> +	{
> These two are in oone line.
>
>> +		.compatible = "samsung,exynos8895-usi",
>> +		.data = &exynos8895_usi_data,
>> +	},
>>  	{ } /* sentinel */
>>  };
>>  MODULE_DEVICE_TABLE(of, exynos_usi_dt_match);
>> @@ -109,14 +146,15 @@ static int exynos_usi_set_sw_conf(struct exynos_usi *usi, size_t mode)
>>  	if (mode < usi->data->min_mode || mode > usi->data->max_mode)
>>  		return -EINVAL;
>>  
>> -	val = exynos_usi_modes[mode].val;
>> +	val = exynos_usi_modes[usi->data->ver][mode].val;
>>  	ret = regmap_update_bits(usi->sysreg, usi->sw_conf,
>>  				 usi->data->sw_conf_mask, val);
>>  	if (ret)
>>  		return ret;
>>  
>>  	usi->mode = mode;
>> -	dev_dbg(usi->dev, "protocol: %s\n", exynos_usi_modes[usi->mode].name);
>> +	dev_dbg(usi->dev, "protocol: %s\n",
>> +		exynos_usi_modes[usi->data->ver][usi->mode].name);
>>  
>>  	return 0;
>>  }
>> @@ -160,6 +198,30 @@ static int exynos_usi_enable(const struct exynos_usi *usi)
>>  	return ret;
>>  }
>>  
>> +/**
>> + * exynos_usi_disable - Disable USI block
>> + * @usi: USI driver object
>> + *
>> + * USI IP-core needs the reset flag cleared in order to function. This
>> + * routine disables the USI block by setting the reset flag. It also disables
>> + * HWACG behavior. It should be performed on removal of the device.
>> + */
>> +static void exynos_usi_disable(const struct exynos_usi *usi)
>> +{
>> +	u32 val;
>> +
>> +	/* Make sure that we've stopped providing the clock to USI IP */
>> +	val = readl(usi->regs + USI_OPTION);
>> +	val &= ~USI_OPTION_CLKREQ_ON;
>> +	val |= ~USI_OPTION_CLKSTOP_ON;
>> +	writel(val, usi->regs + USI_OPTION);
>> +
>> +	/* Set USI block state to reset */
>> +	val = readl(usi->regs + USI_CON);
>> +	val |= USI_CON_RESET;
>> +	writel(val, usi->regs + USI_CON);
>> +}
>> +
>>  static int exynos_usi_configure(struct exynos_usi *usi)
>>  {
>>  	int ret;
>> @@ -169,9 +231,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);
>>  
>> -	return 0;
>> +	return ret;
>>  }
>>  
>>  static int exynos_usi_parse_dt(struct device_node *np, struct exynos_usi *usi)
>> @@ -253,10 +318,26 @@ static int exynos_usi_probe(struct platform_device *pdev)
>>  
>>  	ret = exynos_usi_configure(usi);
>>  	if (ret)
>> -		return ret;
>> +		goto fail_probe;
>>  
>>  	/* Make it possible to embed protocol nodes into USI np */
>>  	return of_platform_populate(np, NULL, NULL, dev);
> This also needs error handling.
>
>> +
>> +fail_probe:
> err_unconfigure:
>
>> +	if (usi->data->ver != USI_VER2)
>> +		clk_bulk_disable_unprepare(usi->data->num_clks, usi->clks);
> Move it to its own callback exynos_usi_unconfigure(), so naming will be
> symmetric. The probe does not prepare clocks directly, so above code is
> not that readable. The most readable is to have symmetrics calls -
> configure+unconfigure (or whatever we name it).

Alright.

>
>> +
>> +	return ret;
>> +}
>> +
>> +static void exynos_usi_remove(struct platform_device *pdev)
>> +{
>> +	struct exynos_usi *usi = platform_get_drvdata(pdev);
>> +
>> +	if (usi->data->ver == USI_VER2)
>> +		exynos_usi_disable(usi);
> This is not related to the patch and should be separate patch, if at
> all.

Well I though that since didn't have any removal routine before it'd be good
to introduce that and not leave USIv2 with hwacg set.

Best regards,
Ivaylo

>> +	else
>> +		clk_bulk_disable_unprepare(usi->data->num_clks, usi->clks);
> So the easiest would be to add devm reset action and then no need for
> goto-err handling and remove() callback.
>
> Best regards,
> Krzysztof
>