| Message ID | 20250109090325.595475-3-ivo.ivanov.ivanov1@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | soc: samsung: usi: implement support for USIv1 | expand |
On 09/01/2025 10:03, Ivaylo Ivanov wrote: > static int exynos_usi_parse_dt(struct device_node *np, struct exynos_usi *usi) > { > int ret; > @@ -251,6 +268,10 @@ static int exynos_usi_probe(struct platform_device *pdev) > return PTR_ERR(usi->regs); > } > > + ret = devm_add_action_or_reset(&pdev->dev, exynos_usi_unconfigure, usi); > + if (ret) > + return ret; You should not reverse the action before the action actually takes place. > + > ret = exynos_usi_configure(usi); > if (ret) > return ret; Registering cleanup should therefore happen here. Best regards, Krzysztof
On 09/01/2025 10:03, Ivaylo Ivanov wrote: > +static void exynos_usi_unconfigure(void *data) > +{ > + struct exynos_usi *usi = data; > + 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); Also shouldn't you enable clocks for accessing these? They are being disabled at the end of exynos_usi_enable(). Best regards, Krzysztof
On 1/14/25 20:03, Krzysztof Kozlowski wrote: > On 09/01/2025 10:03, Ivaylo Ivanov wrote: >> +static void exynos_usi_unconfigure(void *data) >> +{ >> + struct exynos_usi *usi = data; >> + 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); > Also shouldn't you enable clocks for accessing these? They are being > disabled at the end of exynos_usi_enable(). Hm, perhaps. But I find that weird, since I haven't seen it in the downstream driver implementation. Best regards, Ivaylo > > Best regards, > Krzysztof
On 1/14/25 19:59, Krzysztof Kozlowski wrote: > On 09/01/2025 10:03, Ivaylo Ivanov wrote: >> static int exynos_usi_parse_dt(struct device_node *np, struct exynos_usi *usi) >> { >> int ret; >> @@ -251,6 +268,10 @@ static int exynos_usi_probe(struct platform_device *pdev) >> return PTR_ERR(usi->regs); >> } >> >> + ret = devm_add_action_or_reset(&pdev->dev, exynos_usi_unconfigure, usi); >> + if (ret) >> + return ret; > You should not reverse the action before the action actually takes place. > >> + >> ret = exynos_usi_configure(usi); >> if (ret) >> return ret; > Registering cleanup should therefore happen here. Alright, will fix. I'll try to submit the next (and hopefully final, if that's all the critique you have) version in the following days, but I'm currently focused on working on exynos2200. Best regards, Ivaylo > > > Best regards, > Krzysztof
On 14/01/2025 22:57, Ivaylo Ivanov wrote: > On 1/14/25 20:03, Krzysztof Kozlowski wrote: >> On 09/01/2025 10:03, Ivaylo Ivanov wrote: >>> +static void exynos_usi_unconfigure(void *data) >>> +{ >>> + struct exynos_usi *usi = data; >>> + 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); >> Also shouldn't you enable clocks for accessing these? They are being >> disabled at the end of exynos_usi_enable(). > > Hm, perhaps. But I find that weird, since I haven't seen it in the downstream > driver implementation. So the downstream enables clocks when configured usi, but never when unconfiguring? I could believe that no one cared about remove/error paths, thus never tested. Best regards, Krzysztof
On 14/01/2025 22:59, Ivaylo Ivanov wrote: > On 1/14/25 19:59, Krzysztof Kozlowski wrote: >> On 09/01/2025 10:03, Ivaylo Ivanov wrote: >>> static int exynos_usi_parse_dt(struct device_node *np, struct exynos_usi *usi) >>> { >>> int ret; >>> @@ -251,6 +268,10 @@ static int exynos_usi_probe(struct platform_device *pdev) >>> return PTR_ERR(usi->regs); >>> } >>> >>> + ret = devm_add_action_or_reset(&pdev->dev, exynos_usi_unconfigure, usi); >>> + if (ret) >>> + return ret; >> You should not reverse the action before the action actually takes place. >> >>> + >>> ret = exynos_usi_configure(usi); >>> if (ret) >>> return ret; >> Registering cleanup should therefore happen here. > > Alright, will fix. I'll try to submit the next (and hopefully final, if that's > all the critique you have) version in the following days, but I'm currently > focused on working on exynos2200. Yeah, rest looks good. Anyway it is too late for this cycle, so I will pick this (and everything else not picked up) after the merge window. Best regards, Krzysztof
diff --git a/drivers/soc/samsung/exynos-usi.c b/drivers/soc/samsung/exynos-usi.c index 114352695..4b84f481b 100644 --- a/drivers/soc/samsung/exynos-usi.c +++ b/drivers/soc/samsung/exynos-usi.c @@ -174,6 +174,23 @@ static int exynos_usi_configure(struct exynos_usi *usi) return 0; } +static void exynos_usi_unconfigure(void *data) +{ + struct exynos_usi *usi = data; + 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_parse_dt(struct device_node *np, struct exynos_usi *usi) { int ret; @@ -251,6 +268,10 @@ static int exynos_usi_probe(struct platform_device *pdev) return PTR_ERR(usi->regs); } + ret = devm_add_action_or_reset(&pdev->dev, exynos_usi_unconfigure, usi); + if (ret) + return ret; + ret = exynos_usi_configure(usi); if (ret) return ret;
Add a devm_add_action_or_reset() routine for unconfiguring the USI IP block whenever the device gets removed. Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com> --- drivers/soc/samsung/exynos-usi.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)