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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 08/01/2025 10:17, Ivaylo Ivanov wrote:
> 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.

Is there any need for keeping it the same?

> 
>>
>>> +	USI_VER2,
>>>  };


...

>>
>>> +
>>> +	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.

Sure, but separate commit, please. Can be preceeding the USIv1 support.

Best regards,
Krzysztof