| Message ID | 20250107113512.525001-3-ivo.ivanov.ivanov1@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | soc: samsung: usi: implement support for USIv1 | expand |
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. > + 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
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. > >> + 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). Alright. > >> + >> + 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. Best regards, Ivaylo >> + 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 >
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
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@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? 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
On 08/01/2025 10:45, Ivaylo Ivanov wrote: >>>>> + >>>>> + 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. Fixes should be before new features and why messy? You add devm cleanup handler for unconfigure and in next commit you grow it with clk disable for USIv1? Best regards, Krzysztof
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, + 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, }, + { + .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); + +fail_probe: + if (usi->data->ver != USI_VER2) + clk_bulk_disable_unprepare(usi->data->num_clks, usi->clks); + + 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); + else + clk_bulk_disable_unprepare(usi->data->num_clks, usi->clks); } static int __maybe_unused exynos_usi_resume_noirq(struct device *dev) @@ -277,6 +358,7 @@ static struct platform_driver exynos_usi_driver = { .of_match_table = exynos_usi_dt_match, }, .probe = exynos_usi_probe, + .remove = exynos_usi_remove, }; module_platform_driver(exynos_usi_driver);
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(-)