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
