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

[Ivaylo Ivanov]

On 1/8/25 11:26, Krzysztof Kozlowski wrote:
> 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@xxxxxxxxx>
>>>> ---
>>>>  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?

No, not really.

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

What about right after the USIv1 support? It would be less messy in my
opinion.

>
> Best regards,
> Krzysztof