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