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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c30a6f1b-b1a9-40db-b673-e6fc47bdb224@gmail.com>
Date: Wed, 8 Jan 2025 11:45:02 +0200
From: Ivaylo Ivanov <ivo.ivanov.ivanov1@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Rob Herring <robh@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
 Alim Akhtar <alim.akhtar@...sung.com>,
 Sam Protsenko <semen.protsenko@...aro.org>,
 Peter Griffin <peter.griffin@...aro.org>, devicetree@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-samsung-soc@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/3] soc: samsung: usi: implement support for USIv1 and
 exynos8895

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@...il.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?

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