Hi Geert, Thank you for the review. On Tue, Mar 19, 2024 at 8:33 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > Thanks for your patch! > > On Mon, Mar 18, 2024 at 5:08 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > RZ/V2H(P) (R9A09G057) SoC has Generic Timer Module(a.k.a OSTM) which > > needs to deassert the reset line before accessing any registers just > > like the RZ/G2L SoC. > > > > Enable the entry point for RZ/V2H(P) SoC so that we can deassert > > the reset line in probe callback. > > This is not really what is happening. > As OSTM on RZ/V2H has a reset specified, the early call to ostm_init() > through TIMER_OF_DECLARE() always fails with -EPROBE_DEFER, as resets > are not available that early in the boot process. Hence the driver > needs to be reprobed later through the platform driver probe. > Thank you for clarification. Ill update the commit description as above. > > While at it use IS_ENABLED() macro instead of open coding. > > I don't see how the code was open-coding IS_ENABLED()? > Ahh.. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > --- a/drivers/clocksource/renesas-ostm.c > > +++ b/drivers/clocksource/renesas-ostm.c > > @@ -224,7 +224,7 @@ static int __init ostm_init(struct device_node *np) > > > > TIMER_OF_DECLARE(ostm, "renesas,ostm", ostm_init); > > > > -#ifdef CONFIG_ARCH_RZG2L > > +#if IS_ENABLED(CONFIG_ARCH_RZG2L) || IS_ENABLED(CONFIG_ARCH_R9A09G057) > > I think you want to use "defined()" instead of "IS_ENABLED()"? > OK, I will use defined(). Cheers, Prabhakar > > static int __init ostm_probe(struct platform_device *pdev) > > { > > struct device *dev = &pdev->dev; > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
Hi Geert, Thanks for the feedback. > -----Original Message----- > From: Geert Uytterhoeven <geert@linux-m68k.org> > Sent: Tuesday, March 19, 2024 8:44 AM > Subject: Re: [PATCH v4 4/6] usb: renesas_usbhs: Update usbhs pipe configuration for RZ/G2L family > > Hi Biju, > > On Mon, Mar 18, 2024 at 6:33 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > The RZ/G2L family SoCs has 10 pipe buffers compared to 16 pipe buffers > > on RZ/A2M. Update the pipe configuration for RZ/G2L family SoCs and > > use family SoC specific compatible to handle this difference. > > > > The pipe configuration of RZ/G2L is same as > > usbhsc_rzg2l_default_pipe[], so select the default pipe configuration > > for RZ/G2L SoCs by setting .has_new_pipe_configs to zero. > > > > Add SoC specific compatible to OF table to avoid ABI breakage with old > > DTB. To optimize memory usage the SoC specific compatible will be > > removed later. > > > > Based on the patch in BSP by Huy Nguyen <huy.nguyen.wh@renesas.com> > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > --- > > v3->v4: > > * Credit Huy Nguyen's work in the commit message and dropped his name > > from Signed-off-by tag. > > * Selection of usbhsc_rzg2l_default_pipe[] by setting the variable > > has_new_pipe_configs to zero. > > * Updated commit description. > > Thanks for the update! > > > * Dropped the check 'priv->dparam.pipe_configs' as it is same as > > checking !has_new_pipe_configs. > > I disagree: it is used to specify the pipe config through platform data on non-DT platforms. There > don't seem to be any upstream SH platforms doing that, though. You are correct. > > > --- a/drivers/usb/renesas_usbhs/common.c > > +++ b/drivers/usb/renesas_usbhs/common.c > > > @@ -642,7 +658,7 @@ static int usbhs_probe(struct platform_device *pdev) > > if (usbhs_get_dparam(priv, has_new_pipe_configs)) { > > priv->dparam.pipe_configs = usbhsc_new_pipe; > > priv->dparam.pipe_size = ARRAY_SIZE(usbhsc_new_pipe); > > - } else if (!priv->dparam.pipe_configs) { > > + } else { > > Hence I'd rather drop this check. Ok, will restore the check. Cheers, Biju > > > priv->dparam.pipe_configs = usbhsc_default_pipe; > > priv->dparam.pipe_size = ARRAY_SIZE(usbhsc_default_pipe); > > }
Hi Biju, On Mon, Mar 18, 2024 at 6:33 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > The RZ/G2L family SoCs has 10 pipe buffers compared to 16 pipe buffers on > RZ/A2M. Update the pipe configuration for RZ/G2L family SoCs and use > family SoC specific compatible to handle this difference. > > The pipe configuration of RZ/G2L is same as usbhsc_rzg2l_default_pipe[], > so select the default pipe configuration for RZ/G2L SoCs by setting > .has_new_pipe_configs to zero. > > Add SoC specific compatible to OF table to avoid ABI breakage with old > DTB. To optimize memory usage the SoC specific compatible will be removed > later. > > Based on the patch in BSP by Huy Nguyen <huy.nguyen.wh@renesas.com> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > v3->v4: > * Credit Huy Nguyen's work in the commit message and dropped his name > from Signed-off-by tag. > * Selection of usbhsc_rzg2l_default_pipe[] by setting the variable > has_new_pipe_configs to zero. > * Updated commit description. Thanks for the update! > * Dropped the check 'priv->dparam.pipe_configs' as it is same as > checking !has_new_pipe_configs. I disagree: it is used to specify the pipe config through platform data on non-DT platforms. There don't seem to be any upstream SH platforms doing that, though. > --- a/drivers/usb/renesas_usbhs/common.c > +++ b/drivers/usb/renesas_usbhs/common.c > @@ -642,7 +658,7 @@ static int usbhs_probe(struct platform_device *pdev) > if (usbhs_get_dparam(priv, has_new_pipe_configs)) { > priv->dparam.pipe_configs = usbhsc_new_pipe; > priv->dparam.pipe_size = ARRAY_SIZE(usbhsc_new_pipe); > - } else if (!priv->dparam.pipe_configs) { > + } else { Hence I'd rather drop this check. > priv->dparam.pipe_configs = usbhsc_default_pipe; > priv->dparam.pipe_size = ARRAY_SIZE(usbhsc_default_pipe); > } The rest LGTM. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Prabhakar, Thanks for your patch! On Mon, Mar 18, 2024 at 5:08 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > RZ/V2H(P) (R9A09G057) SoC has Generic Timer Module(a.k.a OSTM) which > needs to deassert the reset line before accessing any registers just > like the RZ/G2L SoC. > > Enable the entry point for RZ/V2H(P) SoC so that we can deassert > the reset line in probe callback. This is not really what is happening. As OSTM on RZ/V2H has a reset specified, the early call to ostm_init() through TIMER_OF_DECLARE() always fails with -EPROBE_DEFER, as resets are not available that early in the boot process. Hence the driver needs to be reprobed later through the platform driver probe. > While at it use IS_ENABLED() macro instead of open coding. I don't see how the code was open-coding IS_ENABLED()? > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- a/drivers/clocksource/renesas-ostm.c > +++ b/drivers/clocksource/renesas-ostm.c > @@ -224,7 +224,7 @@ static int __init ostm_init(struct device_node *np) > > TIMER_OF_DECLARE(ostm, "renesas,ostm", ostm_init); > > -#ifdef CONFIG_ARCH_RZG2L > +#if IS_ENABLED(CONFIG_ARCH_RZG2L) || IS_ENABLED(CONFIG_ARCH_R9A09G057) I think you want to use "defined()" instead of "IS_ENABLED()"? > static int __init ostm_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Mon, Mar 18, 2024 at 5:08 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Document the General Timer Module (a.k.a OSTM) block on Renesas RZ/V2H(P)
> ("R9A09G057") SoC, which is identical to the one found on the RZ/A1H and
> RZ/G2L SoCs. Add the "renesas,r9a09g057-ostm" compatible string for the
> RZ/V2H(P) SoC.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Prabhakar, On Mon, Mar 18, 2024 at 6:22 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Add serial support for RZ/V2H(P) SoC with earlycon. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > v2 - > v3 > - new patch Thanks for your patch! > --- a/drivers/tty/serial/sh-sci.c > +++ b/drivers/tty/serial/sh-sci.c > @@ -290,7 +290,7 @@ static const struct sci_port_params sci_port_params[SCIx_NR_REGTYPES] = { > }, > > /* > - * The "SCIFA" that is in RZ/A2, RZ/G2L and RZ/T. > + * The "SCIFA" that is in RZ/A2, RZ/G2L, RZ/T and RZ/V2H. > * It looks like a normal SCIF with FIFO data, but with a > * compressed address space. Also, the break out of interrupts > * are different: ERI/BRI, RXI, TXI, TEI, DRI. and RZ/V2H has more interrupts than RZ/A1, RZ/G2L and RZ/T... In addition, RZ/V2H does not support synchronous mode (does not matter for the driver) and modem control signals. Currently, sci_init_pins() does write ones to the SCPTR bits that are reserved and marked as "write zero" on RZ/V2H. I am not sure how bad that is. You could avoid that by adding a check for .hasrtscts, but that may have impact on other SoCs/boards, where currently e.g. RTS# is always programmed for output and active low... So if you really need to avoid writing to these bits, the only safe way may be to add a new SCIF type. But perhaps I'm over-cautious? ;-) > @@ -3224,6 +3224,10 @@ static const struct of_device_id of_sci_match[] __maybe_unused = { > .compatible = "renesas,scif-r9a07g044", > .data = SCI_OF_DATA(PORT_SCIF, SCIx_RZ_SCIFA_REGTYPE), > }, > + { > + .compatible = "renesas,scif-r9a09g057", > + .data = SCI_OF_DATA(PORT_SCIF, SCIx_RZ_SCIFA_REGTYPE), > + }, > /* Family-specific types */ > { > .compatible = "renesas,rcar-gen1-scif", > @@ -3554,6 +3558,7 @@ OF_EARLYCON_DECLARE(sci, "renesas,sci", sci_early_console_setup); > OF_EARLYCON_DECLARE(scif, "renesas,scif", scif_early_console_setup); > OF_EARLYCON_DECLARE(scif, "renesas,scif-r7s9210", rzscifa_early_console_setup); > OF_EARLYCON_DECLARE(scif, "renesas,scif-r9a07g044", rzscifa_early_console_setup); > +OF_EARLYCON_DECLARE(scif, "renesas,scif-r9a09g057", rzscifa_early_console_setup); > OF_EARLYCON_DECLARE(scifa, "renesas,scifa", scifa_early_console_setup); > OF_EARLYCON_DECLARE(scifb, "renesas,scifb", scifb_early_console_setup); > OF_EARLYCON_DECLARE(hscif, "renesas,hscif", hscif_early_console_setup); Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Prabhakar, On Mon, Mar 18, 2024 at 6:22 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Document support for the Serial Communication Interface with FIFO (SCIF) > available in the Renesas RZ/V2H(P) (R9A09G057) SoC. The SCIF interface in > the Renesas RZ/V2H(P) is similar to that available in the RZ/G2L > (R9A07G044) SoC, with the only difference being that the RZ/V2H(P) SoC has > three additional interrupts: one for Tx end/Rx ready and the other two for > Rx and Tx buffer full, which are edge-triggered. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > v2->v3 > - Added SoC specific compat string Thanks for the update! > --- a/Documentation/devicetree/bindings/serial/renesas,scif.yaml > +++ b/Documentation/devicetree/bindings/serial/renesas,scif.yaml > @@ -79,6 +79,8 @@ properties: > - renesas,scif-r9a08g045 # RZ/G3S > - const: renesas,scif-r9a07g044 # RZ/G2{L,LC} fallback > > + - const: renesas,scif-r9a09g057 # RZ/V2H(P) > + > reg: > maxItems: 1 > > @@ -204,6 +206,37 @@ allOf: > - const: dri > - const: tei > > + - if: > + properties: > + compatible: > + contains: > + const: renesas,scif-r9a09g057 > + then: > + properties: > + interrupts: > + items: > + - description: Error interrupt [...] These descriptions should be at the top level. The SoC-specific rules should just restrict the lower limit (interrupts/minItems). > + > + interrupt-names: > + items: > + - const: eri [...] Likewise. In addition, you should restrict clocks/maxItems to 1, as on RZ/V2H only the UART functional clock is supported. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 19/03/2024 07:19, Krzysztof Kozlowski wrote: > On 18/03/2024 18:21, Prabhakar wrote: >> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> >> >> Add support to validate the 'interrupts' and 'interrupt-names' properties >> for every supported SoC. This ensures proper handling and configuration of >> interrupt-related properties across supported platforms. >> >> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> >> --- >> v2->v3 >> - Listed interrupts and interrupt-names for every SoC in if check >> --- >> .../bindings/serial/renesas,scif.yaml | 95 ++++++++++++------- >> 1 file changed, 63 insertions(+), 32 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/serial/renesas,scif.yaml b/Documentation/devicetree/bindings/serial/renesas,scif.yaml >> index af72c3420453..53f18e9810fd 100644 >> --- a/Documentation/devicetree/bindings/serial/renesas,scif.yaml >> +++ b/Documentation/devicetree/bindings/serial/renesas,scif.yaml >> @@ -82,38 +82,6 @@ properties: >> reg: >> maxItems: 1 >> >> - interrupts: > > I don't understand what is happening with this patchset. Interrupts must > stay here. Where did you receive any different feedback? Look how it is done: https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L44 Best regards, Krzysztof
On 18/03/2024 18:21, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Add support to validate the 'interrupts' and 'interrupt-names' properties
> for every supported SoC. This ensures proper handling and configuration of
> interrupt-related properties across supported platforms.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> v2->v3
> - Listed interrupts and interrupt-names for every SoC in if check
> ---
> .../bindings/serial/renesas,scif.yaml | 95 ++++++++++++-------
> 1 file changed, 63 insertions(+), 32 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/serial/renesas,scif.yaml b/Documentation/devicetree/bindings/serial/renesas,scif.yaml
> index af72c3420453..53f18e9810fd 100644
> --- a/Documentation/devicetree/bindings/serial/renesas,scif.yaml
> +++ b/Documentation/devicetree/bindings/serial/renesas,scif.yaml
> @@ -82,38 +82,6 @@ properties:
> reg:
> maxItems: 1
>
> - interrupts:
I don't understand what is happening with this patchset. Interrupts must
stay here. Where did you receive any different feedback?
Best regards,
Krzysztof
Hi Biju, Thank you for the patch. On Mon, Mar 18, 2024 at 11:08:41AM +0000, Biju Das wrote: > The clk_disable_unprepare() doesn't guarantee that a clock is gated after > the execution as it is driver dependent. The Renesas and most of the other > platforms don't wait until clock is stopped because of performance reason. Is that really the case for most platforms ? I've never seen that being an issue in practice. > But these platforms wait while turning on the clock. > > The normal case for shutting down the clock is unbind/close/suspend or > error paths in the driver. Not waiting for the shutting down the clock > will improve the suspend time. > > But on RZ/G2L Camera Data Receiving Unit (CRU) IP, initially the vclk is > on. Before enabling link reception, we need to wait for vclk to be off > and after enabling reception, we need to turn the vlck on. Special cases > like this requires a sync API for clock gating. > > Add clk_poll_disable_unprepare() to poll the clock gate operation that > guarantees gating of clk after the execution. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > v2->v3: > * Added WARN_ON(enable count non-zero) and return an error code (-EBUSY), > if the user is not the sole user of the clock and the enable count is > non-zero. > * Returned an error if there's no is_enabled() callback. > RFC->v2: > * Renamed clk_disable_unprepare_sync()-->clk_poll_disable_unprepare() > * Redesigned to make use of __clk_is_enabled() to poll the clock gating. > --- > drivers/clk/clk.c | 29 ++++++++++++++++++++++++++++ > include/linux/clk.h | 46 +++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 75 insertions(+) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index f5fa91a339d7..e10bb14c904d 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -13,6 +13,7 @@ > #include <linux/mutex.h> > #include <linux/spinlock.h> > #include <linux/err.h> > +#include <linux/iopoll.h> > #include <linux/list.h> > #include <linux/slab.h> > #include <linux/of.h> > @@ -1160,6 +1161,34 @@ void clk_disable(struct clk *clk) > } > EXPORT_SYMBOL_GPL(clk_disable); > > +/** > + * clk_poll_disabled - poll for clock gating. > + * @clk: the clk that is going to stop > + * @sleep_us: Maximum time to sleep between reads in us (0 > + * tight-loops). Should be less than ~20ms since usleep_range > + * is used (see Documentation/timers/timers-howto.rst). > + * @timeout_us: Timeout in us, 0 means never timeout > + * > + * It polls for a clk to be stopped. > + */ > +int clk_poll_disabled(struct clk *clk, unsigned long sleep_us, u64 timeout_us) > +{ > + bool status; > + > + if (IS_ERR_OR_NULL(clk)) > + return 0; > + > + if (!clk->core->ops->is_enabled) > + return -EOPNOTSUPP; > + > + if (WARN(__clk_get_enable_count(clk), "clk is in use\n")) > + return -EBUSY; A WARN() is fairly bad, given how easy it could be to this this condition. To make it safe in the genral case for drivers to use this API, we may need a way to acquire a clock exclusively, which would complexify the API quite a bit. > + > + return read_poll_timeout(__clk_is_enabled, status, !status, sleep_us, > + timeout_us, false, clk); > +} > +EXPORT_SYMBOL_GPL(clk_poll_disabled); > + > static int clk_core_enable(struct clk_core *core) > { > int ret = 0; > diff --git a/include/linux/clk.h b/include/linux/clk.h > index 84b02518791f..7f714ecce0eb 100644 > --- a/include/linux/clk.h > +++ b/include/linux/clk.h > @@ -693,6 +693,20 @@ int __must_check clk_bulk_enable(int num_clks, > */ > void clk_disable(struct clk *clk); > > +/** > + * clk_poll_disabled - inform the system whether the clock source is stopped. > + * @clk: clock source > + * @sleep_us: Maximum time to sleep between reads in us (0 > + * tight-loops). Should be less than ~20ms since usleep_range > + * is used (see Documentation/timers/timers-howto.rst). > + * @timeout_us: Timeout in us, 0 means never timeout > + * > + * Poll for clock gating and Inform the system about it's status. > + * > + * Context: May sleep. > + */ > +int clk_poll_disabled(struct clk *clk, unsigned long sleep_us, u64 timeout_us); > + > /** > * clk_bulk_disable - inform the system when the set of clks is no > * longer required. > @@ -1030,6 +1044,11 @@ static inline int __must_check clk_bulk_enable(int num_clks, > > static inline void clk_disable(struct clk *clk) {} > > +static inline int clk_poll_disabled(struct clk *clk, unsigned long sleep_us, > + u64 timeout_us) > +{ > + return 0; > +} > > static inline void clk_bulk_disable(int num_clks, > const struct clk_bulk_data *clks) {} > @@ -1121,6 +1140,33 @@ static inline void clk_disable_unprepare(struct clk *clk) > clk_unprepare(clk); > } > > +/** > + * clk_poll_disable_unprepare - Poll clk_disable_unprepare > + * @clk: clock source > + * @sleep_us: Maximum time to sleep between reads in us (0 > + * tight-loops). Should be less than ~20ms since usleep_range > + * is used (see Documentation/timers/timers-howto.rst). > + * @timeout_us: Timeout in us, 0 means never timeout > + * > + * Context: May sleep. > + * > + * This function polls until the clock has stopped. > + * > + * Returns success (0) or negative errno. > + */ > +static inline int clk_poll_disable_unprepare(struct clk *clk, > + unsigned long sleep_us, > + u64 timeout_us) > +{ > + int ret; > + > + clk_disable(clk); > + ret = clk_poll_disabled(clk, sleep_us, timeout_us); > + clk_unprepare(clk); What happens in the clk_disable_unprepare() case, if the clk_unprepare() function is called on a clock that hasn't been synchronously disabled ? This is ill-defined, a clock provider driver that implements .disable() asynchronously would see its .unprepare() operation called with different clock states. That behaviour is error-prone, especially given that it could be difficult to test clock provider drivers to ensure that handle both cases correctly. One option could be to turn the .unprepare() operation into a synchronization point, requiring drivers that implement .disable() asynchronously to implement synchronization in .unprepare(). That way you wouldn't need a new API function for clock consumers. The downside is that consumers that call clk_disable_unprepare() will never benefit from the .disable() operation being asynchronous, which may defeat the whole point of this exercise. I'm starting to wonder if the simplest option in your case wouldn't be to make your clock provider synchronous for the vclk... > + > + return ret; > +} > + > static inline int __must_check > clk_bulk_prepare_enable(int num_clks, const struct clk_bulk_data *clks) > { -- Regards, Laurent Pinchart
Hi Biju, Thank you for the patch. On Mon, Mar 18, 2024 at 11:08:40AM +0000, Biju Das wrote: > > The API's related to clk disable operation does not explicitly > states the synchoronous or asynchrous behaviour as it is driver s/synchoronous/synchronous/ > dependent. So make this part clear in API documentation. You need to explain the rationale here, why asynchronous behaviour is preferred. > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > v2->v3: > * No change. > v2: > * New patch. > --- > drivers/clk/clk.c | 3 ++- > include/linux/clk-provider.h | 3 ++- > include/linux/clk.h | 3 ++- > 3 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 25371c91a58f..f5fa91a339d7 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -1010,7 +1010,8 @@ static void clk_core_unprepare_lock(struct clk_core *core) > * if the operation may sleep. One example is a clk which is accessed over > * I2c. In the complex case a clk gate operation may require a fast and a slow > * part. It is this reason that clk_unprepare and clk_disable are not mutually > - * exclusive. In fact clk_disable must be called before clk_unprepare. > + * exclusive. In fact clk_disable must be called before clk_unprepare. The > + * synchronous or asynchronous clock gating operation is driver dependent. If synchronous operation is not guaranteed, then it's asynchonous. Asynchronous doesn't mean slow, even an asynchronous provider can complete the disable operation before the function returns to the caller. All it means is that there's no guarantee of synchronous operation. I would document it as such: * This function is asynchronous, if may return before the clock provider * completes the unprepare operation. However, below you're addressing the disable operation. Did you mean to patch the documentation for clk_prepare() instead ? Making clk_unprepare() asynchronous seems a bit weird, given that the function may sleep and is expected to take more time. > */ > void clk_unprepare(struct clk *clk) > { > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > index 4a537260f655..5b493024e1ec 100644 > --- a/include/linux/clk-provider.h > +++ b/include/linux/clk-provider.h > @@ -113,7 +113,8 @@ struct clk_duty { > * sleep. > * > * @disable: Disable the clock atomically. Called with enable_lock held. > - * This function must not sleep. > + * This function must not sleep. The synchronous or asynchronous > + * disabling of the clock is driver dependent. As this is the documentation that targets clock providers, I would expand it and explain why a provider may want to make the disable operation not synchronous. > * > * @is_enabled: Queries the hardware to determine if the clock is enabled. > * This function must not sleep. Optional, if this op is not .is_enabled() should become mandatory if .disable() is not synchronous. The relationship between the two operations should be better explained. > diff --git a/include/linux/clk.h b/include/linux/clk.h > index 00623f4de5e1..84b02518791f 100644 > --- a/include/linux/clk.h > +++ b/include/linux/clk.h > @@ -681,7 +681,8 @@ int __must_check clk_bulk_enable(int num_clks, > * @clk: clock source > * > * Inform the system that a clock source is no longer required by > - * a driver and may be shut down. > + * a driver and may be shut down. It is not guaranteed to ever actually > + * be stopped, that will be driver dependent. This is the documentation of clk_bulk_disable(), you should address clk_disable() too. I've just noticed that both functions are documented in two places, in include/linux/clk.h, and in drivers/clk/. I wonder why that is. It sounds like it should be fixed, or you'll have to patch both documentation blocks. There's another issue that I'll raise in the review of 2/3. > * > * May be called from atomic contexts. > * -- Regards, Laurent Pinchart
Hi Biju, On Mon, Mar 18, 2024 at 11:08:40AM +0000, Biju Das wrote: > > The API's related to clk disable operation does not explicitly > states the synchoronous or asynchrous behaviour as it is driver > dependent. So make this part clear in API documentation. > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > v2->v3: > * No change. > v2: > * New patch. > --- > drivers/clk/clk.c | 3 ++- > include/linux/clk-provider.h | 3 ++- > include/linux/clk.h | 3 ++- > 3 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index 25371c91a58f..f5fa91a339d7 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -1010,7 +1010,8 @@ static void clk_core_unprepare_lock(struct clk_core *core) > * if the operation may sleep. One example is a clk which is accessed over > * I2c. In the complex case a clk gate operation may require a fast and a slow > * part. It is this reason that clk_unprepare and clk_disable are not mutually > - * exclusive. In fact clk_disable must be called before clk_unprepare. > + * exclusive. In fact clk_disable must be called before clk_unprepare. The > + * synchronous or asynchronous clock gating operation is driver dependent. > */ > void clk_unprepare(struct clk *clk) > { > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > index 4a537260f655..5b493024e1ec 100644 > --- a/include/linux/clk-provider.h > +++ b/include/linux/clk-provider.h > @@ -113,7 +113,8 @@ struct clk_duty { > * sleep. > * > * @disable: Disable the clock atomically. Called with enable_lock held. > - * This function must not sleep. > + * This function must not sleep. The synchronous or asynchronous > + * disabling of the clock is driver dependent. s/driver\K/ and hardware/ Same in the first chunk actually. > * > * @is_enabled: Queries the hardware to determine if the clock is enabled. > * This function must not sleep. Optional, if this op is not > diff --git a/include/linux/clk.h b/include/linux/clk.h > index 00623f4de5e1..84b02518791f 100644 > --- a/include/linux/clk.h > +++ b/include/linux/clk.h > @@ -681,7 +681,8 @@ int __must_check clk_bulk_enable(int num_clks, > * @clk: clock source > * > * Inform the system that a clock source is no longer required by > - * a driver and may be shut down. > + * a driver and may be shut down. It is not guaranteed to ever actually > + * be stopped, that will be driver dependent. I'd rephrase this, taking other users into account: There's no guarantee that the clock stops within a particular time window or at all, depending on other users of the clock as well as the driver and hardware implementation. > * > * May be called from atomic contexts. > * -- Kind regards, Sakari Ailus
Hi Biju, Thanks for the update. On Mon, Mar 18, 2024 at 11:08:41AM +0000, Biju Das wrote: > The clk_disable_unprepare() doesn't guarantee that a clock is gated after > the execution as it is driver dependent. The Renesas and most of the other > platforms don't wait until clock is stopped because of performance reason. > But these platforms wait while turning on the clock. > > The normal case for shutting down the clock is unbind/close/suspend or > error paths in the driver. Not waiting for the shutting down the clock > will improve the suspend time. > > But on RZ/G2L Camera Data Receiving Unit (CRU) IP, initially the vclk is > on. Before enabling link reception, we need to wait for vclk to be off > and after enabling reception, we need to turn the vlck on. Special cases > like this requires a sync API for clock gating. > > Add clk_poll_disable_unprepare() to poll the clock gate operation that > guarantees gating of clk after the execution. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > --- > v2->v3: > * Added WARN_ON(enable count non-zero) and return an error code (-EBUSY), > if the user is not the sole user of the clock and the enable count is > non-zero. > * Returned an error if there's no is_enabled() callback. > RFC->v2: > * Renamed clk_disable_unprepare_sync()-->clk_poll_disable_unprepare() > * Redesigned to make use of __clk_is_enabled() to poll the clock gating. > --- > drivers/clk/clk.c | 29 ++++++++++++++++++++++++++++ > include/linux/clk.h | 46 +++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 75 insertions(+) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index f5fa91a339d7..e10bb14c904d 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -13,6 +13,7 @@ > #include <linux/mutex.h> > #include <linux/spinlock.h> > #include <linux/err.h> > +#include <linux/iopoll.h> > #include <linux/list.h> > #include <linux/slab.h> > #include <linux/of.h> > @@ -1160,6 +1161,34 @@ void clk_disable(struct clk *clk) > } > EXPORT_SYMBOL_GPL(clk_disable); > > +/** > + * clk_poll_disabled - poll for clock gating. > + * @clk: the clk that is going to stop > + * @sleep_us: Maximum time to sleep between reads in us (0 > + * tight-loops). Should be less than ~20ms since usleep_range > + * is used (see Documentation/timers/timers-howto.rst). > + * @timeout_us: Timeout in us, 0 means never timeout > + * > + * It polls for a clk to be stopped. > + */ We should have documentation either in the header or here, not both. I'd drop this. > +int clk_poll_disabled(struct clk *clk, unsigned long sleep_us, u64 timeout_us) > +{ > + bool status; > + > + if (IS_ERR_OR_NULL(clk)) > + return 0; > + > + if (!clk->core->ops->is_enabled) > + return -EOPNOTSUPP; > + > + if (WARN(__clk_get_enable_count(clk), "clk is in use\n")) > + return -EBUSY; > + > + return read_poll_timeout(__clk_is_enabled, status, !status, sleep_us, > + timeout_us, false, clk); > +} > +EXPORT_SYMBOL_GPL(clk_poll_disabled); > + > static int clk_core_enable(struct clk_core *core) > { > int ret = 0; > diff --git a/include/linux/clk.h b/include/linux/clk.h > index 84b02518791f..7f714ecce0eb 100644 > --- a/include/linux/clk.h > +++ b/include/linux/clk.h > @@ -693,6 +693,20 @@ int __must_check clk_bulk_enable(int num_clks, > */ > void clk_disable(struct clk *clk); > > +/** > + * clk_poll_disabled - inform the system whether the clock source is stopped. > + * @clk: clock source > + * @sleep_us: Maximum time to sleep between reads in us (0 > + * tight-loops). Should be less than ~20ms since usleep_range > + * is used (see Documentation/timers/timers-howto.rst). > + * @timeout_us: Timeout in us, 0 means never timeout > + * > + * Poll for clock gating and Inform the system about it's status. How about, instead: Poll for clock gating and return when either there's a timeout or the clock has been gated. Returns: 0 if the clock is successfully gated, error otherwise. Please run scripts/kerneldoc -Wall on this. > + * > + * Context: May sleep. > + */ > +int clk_poll_disabled(struct clk *clk, unsigned long sleep_us, u64 timeout_us); > + > /** > * clk_bulk_disable - inform the system when the set of clks is no > * longer required. > @@ -1030,6 +1044,11 @@ static inline int __must_check clk_bulk_enable(int num_clks, > > static inline void clk_disable(struct clk *clk) {} > > +static inline int clk_poll_disabled(struct clk *clk, unsigned long sleep_us, > + u64 timeout_us) > +{ > + return 0; > +} > > static inline void clk_bulk_disable(int num_clks, > const struct clk_bulk_data *clks) {} > @@ -1121,6 +1140,33 @@ static inline void clk_disable_unprepare(struct clk *clk) > clk_unprepare(clk); > } > > +/** > + * clk_poll_disable_unprepare - Poll clk_disable_unprepare How about calling this clk_disable_sync_unprepare? I'm not sure if a special function is needed for something needed so rarely as you can already call clk_poll_disabled(). Maybe others have an opinion on this, too. > + * @clk: clock source > + * @sleep_us: Maximum time to sleep between reads in us (0 > + * tight-loops). Should be less than ~20ms since usleep_range > + * is used (see Documentation/timers/timers-howto.rst). > + * @timeout_us: Timeout in us, 0 means never timeout > + * > + * Context: May sleep. > + * > + * This function polls until the clock has stopped. > + * > + * Returns success (0) or negative errno. > + */ > +static inline int clk_poll_disable_unprepare(struct clk *clk, > + unsigned long sleep_us, > + u64 timeout_us) > +{ > + int ret; > + > + clk_disable(clk); > + ret = clk_poll_disabled(clk, sleep_us, timeout_us); > + clk_unprepare(clk); > + > + return ret; > +} > + > static inline int __must_check > clk_bulk_prepare_enable(int num_clks, const struct clk_bulk_data *clks) > { -- Kind regards, Sakari Ailus
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Add 'interrupt-names' property into SCIF nodes for clarity. This allows us to update the DT binding to mark 'interrupt-names' property as required for all the SoCs which have multiple interrupts and also allow us to validate the DT binding doc using dtbs_check. Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> --- Note, all the SoCs with multiple interrupts using renesas,scif.yaml already have interrupt-names property apart from this SoC. This change allows us to validate the DT. --- arch/arm/boot/dts/renesas/r7s72100.dtsi | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/arch/arm/boot/dts/renesas/r7s72100.dtsi b/arch/arm/boot/dts/renesas/r7s72100.dtsi index e6d8da6faffb..08ea4c551ed0 100644 --- a/arch/arm/boot/dts/renesas/r7s72100.dtsi +++ b/arch/arm/boot/dts/renesas/r7s72100.dtsi @@ -125,6 +125,7 @@ scif0: serial@e8007000 { <GIC_SPI 191 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 189 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "eri", "rxi", "txi", "bri"; clocks = <&mstp4_clks R7S72100_CLK_SCIF0>; clock-names = "fck"; power-domains = <&cpg_clocks>; @@ -138,6 +139,7 @@ scif1: serial@e8007800 { <GIC_SPI 195 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 196 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 193 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "eri", "rxi", "txi", "bri"; clocks = <&mstp4_clks R7S72100_CLK_SCIF1>; clock-names = "fck"; power-domains = <&cpg_clocks>; @@ -151,6 +153,7 @@ scif2: serial@e8008000 { <GIC_SPI 199 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 200 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 197 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "eri", "rxi", "txi", "bri"; clocks = <&mstp4_clks R7S72100_CLK_SCIF2>; clock-names = "fck"; power-domains = <&cpg_clocks>; @@ -164,6 +167,7 @@ scif3: serial@e8008800 { <GIC_SPI 203 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 204 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 201 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "eri", "rxi", "txi", "bri"; clocks = <&mstp4_clks R7S72100_CLK_SCIF3>; clock-names = "fck"; power-domains = <&cpg_clocks>; @@ -177,6 +181,7 @@ scif4: serial@e8009000 { <GIC_SPI 207 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 208 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 205 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "eri", "rxi", "txi", "bri"; clocks = <&mstp4_clks R7S72100_CLK_SCIF4>; clock-names = "fck"; power-domains = <&cpg_clocks>; @@ -190,6 +195,7 @@ scif5: serial@e8009800 { <GIC_SPI 211 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 212 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 209 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "eri", "rxi", "txi", "bri"; clocks = <&mstp4_clks R7S72100_CLK_SCIF5>; clock-names = "fck"; power-domains = <&cpg_clocks>; @@ -203,6 +209,7 @@ scif6: serial@e800a000 { <GIC_SPI 215 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 216 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 213 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "eri", "rxi", "txi", "bri"; clocks = <&mstp4_clks R7S72100_CLK_SCIF6>; clock-names = "fck"; power-domains = <&cpg_clocks>; @@ -216,6 +223,7 @@ scif7: serial@e800a800 { <GIC_SPI 219 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 220 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 217 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "eri", "rxi", "txi", "bri"; clocks = <&mstp4_clks R7S72100_CLK_SCIF7>; clock-names = "fck"; power-domains = <&cpg_clocks>; -- 2.34.1
The number of pipe buffers on RZ/G2L family SoCs is 10, whereas on RZ/A2M it is 16. Replace 'renesas,rza2m-usbhs->renesas,rzg2l-usbhs' as family SoC compatible to handle this difference and use the SoC specific compatible in driver to avoid the ABI breakage with older DTB. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Acked-by: Geert Uytterhoeven <geert+renesas@glider.be> --- v3->v4: * No change. v2->v3: * Added tags from Geert. v1->v2: * Updated commit description about ABI breakage. * Updated commit header as it is RZ/G2L specific. --- arch/arm64/boot/dts/renesas/r9a07g043.dtsi | 2 +- arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 2 +- arch/arm64/boot/dts/renesas/r9a07g054.dtsi | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/arch/arm64/boot/dts/renesas/r9a07g043.dtsi b/arch/arm64/boot/dts/renesas/r9a07g043.dtsi index 8721f4c9fa0f..766c54b91acc 100644 --- a/arch/arm64/boot/dts/renesas/r9a07g043.dtsi +++ b/arch/arm64/boot/dts/renesas/r9a07g043.dtsi @@ -812,7 +812,7 @@ usb2_phy1: usb-phy@11c70200 { hsusb: usb@11c60000 { compatible = "renesas,usbhs-r9a07g043", - "renesas,rza2-usbhs"; + "renesas,rzg2l-usbhs"; reg = <0 0x11c60000 0 0x10000>; interrupts = <SOC_PERIPHERAL_IRQ(100) IRQ_TYPE_EDGE_RISING>, <SOC_PERIPHERAL_IRQ(101) IRQ_TYPE_LEVEL_HIGH>, diff --git a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi index 9f00b75d2bd0..88634ae43287 100644 --- a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi +++ b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi @@ -1217,7 +1217,7 @@ usb2_phy1: usb-phy@11c70200 { hsusb: usb@11c60000 { compatible = "renesas,usbhs-r9a07g044", - "renesas,rza2-usbhs"; + "renesas,rzg2l-usbhs"; reg = <0 0x11c60000 0 0x10000>; interrupts = <GIC_SPI 100 IRQ_TYPE_EDGE_RISING>, <GIC_SPI 101 IRQ_TYPE_LEVEL_HIGH>, diff --git a/arch/arm64/boot/dts/renesas/r9a07g054.dtsi b/arch/arm64/boot/dts/renesas/r9a07g054.dtsi index 53d8905f367a..e89bfe4085f5 100644 --- a/arch/arm64/boot/dts/renesas/r9a07g054.dtsi +++ b/arch/arm64/boot/dts/renesas/r9a07g054.dtsi @@ -1225,7 +1225,7 @@ usb2_phy1: usb-phy@11c70200 { hsusb: usb@11c60000 { compatible = "renesas,usbhs-r9a07g054", - "renesas,rza2-usbhs"; + "renesas,rzg2l-usbhs"; reg = <0 0x11c60000 0 0x10000>; interrupts = <GIC_SPI 100 IRQ_TYPE_EDGE_RISING>, <GIC_SPI 101 IRQ_TYPE_LEVEL_HIGH>, -- 2.25.1
Remove the trailing comma in the terminator entry for the OF table making code robust against (theoretical) misrebases or other similar things where the new entry goes _after_ the termination without the compiler noticing. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> --- v3->v4: * No change. v2->v3: * No change. v1->v2: * Added Rb tag from Geert. --- drivers/usb/renesas_usbhs/common.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/renesas_usbhs/common.c b/drivers/usb/renesas_usbhs/common.c index df94375f6c23..7ec1ab90fef3 100644 --- a/drivers/usb/renesas_usbhs/common.c +++ b/drivers/usb/renesas_usbhs/common.c @@ -597,7 +597,7 @@ static const struct of_device_id usbhs_of_match[] = { .compatible = "renesas,rzg2l-usbhs", .data = &usbhs_rzg2l_plat_info, }, - { }, + { } }; MODULE_DEVICE_TABLE(of, usbhs_of_match); -- 2.25.1
As per the hardware manual, double buffer setting results in fewer interrupts for high-speed data transfers. Improve usbhsc_default_pipe[] for isochronous transfers by updating the table from single->double buffering and update the pipe number accordingly. Suggested-by: Geert Uytterhoeven <geert+renesas@glider.be> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> --- v3->v4: * Added Rbtag from Geert. v3: * New patch --- drivers/usb/renesas_usbhs/common.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/usb/renesas_usbhs/common.c b/drivers/usb/renesas_usbhs/common.c index 0c62e4c6c88d..177fa3144a47 100644 --- a/drivers/usb/renesas_usbhs/common.c +++ b/drivers/usb/renesas_usbhs/common.c @@ -366,11 +366,11 @@ static void usbhsc_clk_disable_unprepare(struct usbhs_priv *priv) /* commonly used on old SH-Mobile SoCs */ static struct renesas_usbhs_driver_pipe_config usbhsc_default_pipe[] = { RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_CONTROL, 64, 0x00, false), - RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_ISOC, 1024, 0x08, false), - RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_ISOC, 1024, 0x18, false), - RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_BULK, 512, 0x28, true), - RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_BULK, 512, 0x38, true), + RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_ISOC, 1024, 0x08, true), + RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_ISOC, 1024, 0x28, true), RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_BULK, 512, 0x48, true), + RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_BULK, 512, 0x58, true), + RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_BULK, 512, 0x68, true), RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_INT, 64, 0x04, false), RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_INT, 64, 0x05, false), RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_INT, 64, 0x06, false), -- 2.25.1
The RZ/G2L family SoCs has 10 pipe buffers compared to 16 pipe buffers on RZ/A2M. Update the pipe configuration for RZ/G2L family SoCs and use family SoC specific compatible to handle this difference. The pipe configuration of RZ/G2L is same as usbhsc_rzg2l_default_pipe[], so select the default pipe configuration for RZ/G2L SoCs by setting .has_new_pipe_configs to zero. Add SoC specific compatible to OF table to avoid ABI breakage with old DTB. To optimize memory usage the SoC specific compatible will be removed later. Based on the patch in BSP by Huy Nguyen <huy.nguyen.wh@renesas.com> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- v3->v4: * Credit Huy Nguyen's work in the commit message and dropped his name from Signed-off-by tag. * Selection of usbhsc_rzg2l_default_pipe[] by setting the variable has_new_pipe_configs to zero. * Updated commit description. * Dropped the check 'priv->dparam.pipe_configs' as it is same as checking !has_new_pipe_configs. v2->v3: * Updated commit description * Dropped usbhsc_rzg2l_pipe[] and reusing the default_pipe[]. v1->v2: * Dropped using of_device_is_compatible() in probe. * Added usbhs_rzg2l_plat_info and replaced the device data for RZ/G2L from usbhs_rza2_plat_info->usbhs_rzg2l_plat_info. * Moved usbhsc_rzg2l_pipe table near to the user. * Updated commit description. --- drivers/usb/renesas_usbhs/common.c | 20 ++++++++++++++++++-- drivers/usb/renesas_usbhs/rza.h | 1 + drivers/usb/renesas_usbhs/rza2.c | 13 +++++++++++++ 3 files changed, 32 insertions(+), 2 deletions(-) diff --git a/drivers/usb/renesas_usbhs/common.c b/drivers/usb/renesas_usbhs/common.c index 177fa3144a47..df94375f6c23 100644 --- a/drivers/usb/renesas_usbhs/common.c +++ b/drivers/usb/renesas_usbhs/common.c @@ -363,7 +363,7 @@ static void usbhsc_clk_disable_unprepare(struct usbhs_priv *priv) * platform default param */ -/* commonly used on old SH-Mobile SoCs */ +/* commonly used on old SH-Mobile and RZ/G2L family SoCs */ static struct renesas_usbhs_driver_pipe_config usbhsc_default_pipe[] = { RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_CONTROL, 64, 0x00, false), RENESAS_USBHS_PIPE(USB_ENDPOINT_XFER_ISOC, 1024, 0x08, true), @@ -565,6 +565,18 @@ static const struct of_device_id usbhs_of_match[] = { .compatible = "renesas,usbhs-r8a77995", .data = &usbhs_rcar_gen3_with_pll_plat_info, }, + { + .compatible = "renesas,usbhs-r9a07g043", + .data = &usbhs_rzg2l_plat_info, + }, + { + .compatible = "renesas,usbhs-r9a07g044", + .data = &usbhs_rzg2l_plat_info, + }, + { + .compatible = "renesas,usbhs-r9a07g054", + .data = &usbhs_rzg2l_plat_info, + }, { .compatible = "renesas,rcar-gen2-usbhs", .data = &usbhs_rcar_gen2_plat_info, @@ -581,6 +593,10 @@ static const struct of_device_id usbhs_of_match[] = { .compatible = "renesas,rza2-usbhs", .data = &usbhs_rza2_plat_info, }, + { + .compatible = "renesas,rzg2l-usbhs", + .data = &usbhs_rzg2l_plat_info, + }, { }, }; MODULE_DEVICE_TABLE(of, usbhs_of_match); @@ -642,7 +658,7 @@ static int usbhs_probe(struct platform_device *pdev) if (usbhs_get_dparam(priv, has_new_pipe_configs)) { priv->dparam.pipe_configs = usbhsc_new_pipe; priv->dparam.pipe_size = ARRAY_SIZE(usbhsc_new_pipe); - } else if (!priv->dparam.pipe_configs) { + } else { priv->dparam.pipe_configs = usbhsc_default_pipe; priv->dparam.pipe_size = ARRAY_SIZE(usbhsc_default_pipe); } diff --git a/drivers/usb/renesas_usbhs/rza.h b/drivers/usb/renesas_usbhs/rza.h index a29b75fef057..8b879aa34a20 100644 --- a/drivers/usb/renesas_usbhs/rza.h +++ b/drivers/usb/renesas_usbhs/rza.h @@ -3,3 +3,4 @@ extern const struct renesas_usbhs_platform_info usbhs_rza1_plat_info; extern const struct renesas_usbhs_platform_info usbhs_rza2_plat_info; +extern const struct renesas_usbhs_platform_info usbhs_rzg2l_plat_info; diff --git a/drivers/usb/renesas_usbhs/rza2.c b/drivers/usb/renesas_usbhs/rza2.c index f079817250bb..b83699eab373 100644 --- a/drivers/usb/renesas_usbhs/rza2.c +++ b/drivers/usb/renesas_usbhs/rza2.c @@ -71,3 +71,16 @@ const struct renesas_usbhs_platform_info usbhs_rza2_plat_info = { .has_new_pipe_configs = 1, }, }; + +const struct renesas_usbhs_platform_info usbhs_rzg2l_plat_info = { + .platform_callback = { + .hardware_init = usbhs_rza2_hardware_init, + .hardware_exit = usbhs_rza2_hardware_exit, + .power_ctrl = usbhs_rza2_power_ctrl, + .get_id = usbhs_get_id_as_gadget, + }, + .driver_param = { + .has_cnen = 1, + .cfifo_byte_addr = 1, + }, +}; -- 2.25.1
Simplify probe() by removing redundant dev->of_node check. While at it, replace dev_err->dev_err_probe for error path. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> --- v3->v4: * No change. v2->v3: * Added Rb tag from Geert. v2: * New patch. --- drivers/usb/renesas_usbhs/common.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/drivers/usb/renesas_usbhs/common.c b/drivers/usb/renesas_usbhs/common.c index dd1c17542439..0c62e4c6c88d 100644 --- a/drivers/usb/renesas_usbhs/common.c +++ b/drivers/usb/renesas_usbhs/common.c @@ -595,16 +595,11 @@ static int usbhs_probe(struct platform_device *pdev) u32 tmp; int irq; - /* check device node */ - if (dev_of_node(dev)) - info = of_device_get_match_data(dev); - else - info = renesas_usbhs_get_info(pdev); - - /* check platform information */ + info = of_device_get_match_data(dev); if (!info) { - dev_err(dev, "no platform information\n"); - return -EINVAL; + info = renesas_usbhs_get_info(pdev); + if (!info) + return dev_err_probe(dev, -EINVAL, "no platform info\n"); } /* platform data */ -- 2.25.1
The USBHS IP found on RZ/G2L SoCs only has 10 pipe buffers compared to 16 pipe buffers on RZ/A2M. Document renesas,rzg2l-usbhs family compatible to handle this difference for RZ/G2L family SoCs. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> --- v3->v4: * No change. v2->v3: * Added Rb tag from Geert. v1->v2: * Added Ack from Krzysztof Kozlowski. --- Documentation/devicetree/bindings/usb/renesas,usbhs.yaml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/usb/renesas,usbhs.yaml b/Documentation/devicetree/bindings/usb/renesas,usbhs.yaml index 40ada78f2328..c63db3ebd07b 100644 --- a/Documentation/devicetree/bindings/usb/renesas,usbhs.yaml +++ b/Documentation/devicetree/bindings/usb/renesas,usbhs.yaml @@ -19,10 +19,14 @@ properties: - items: - enum: - renesas,usbhs-r7s9210 # RZ/A2 + - const: renesas,rza2-usbhs + + - items: + - enum: - renesas,usbhs-r9a07g043 # RZ/G2UL and RZ/Five - renesas,usbhs-r9a07g044 # RZ/G2{L,LC} - renesas,usbhs-r9a07g054 # RZ/V2L - - const: renesas,rza2-usbhs + - const: renesas,rzg2l-usbhs - items: - enum: -- 2.25.1
The USBHS IP found on RZ/G2L SoCs only has 10 pipe buffers compared to 16 pipe buffers on RZ/A2M. Document renesas,rzg2l-usbhs family compatible to handle this difference for RZ/G2L family SoCs. This patch series aims to fix the USB pipe configuration for RZ/G2L family SoCs. For the backward compatibility SoC specific compatible is used and will be removed the same after few kernel releases. As the DTS update has a hard dependency on the driver fix, Got ack from Geert for patch#6 to apply the DTS update together with the driver fix. v3->v4: * Added Rbtag from Geert for patch#3. * Dropped patch#4 * Credit Huy Nguyen's work in the commit message for patch#4 and dropped his name from Signed-off-by tag. * Selection of usbhsc_rzg2l_default_pipe[] by setting the variable has_new_pipe_configs to zero. * Updated commit description for patch#4. * Dropped the check 'priv->dparam.pipe_configs' as it is same as checking !has_new_pipe_configs. v2->v3: * Added Rb tag from Geert for patch#1,#2 and #7 * Added Ack tag from Geert for patch#7. * Added patch#3 for improving usbhsc_default_pipe[] for isochronous transfers * Added patch#4 for dropping has_new_pipe_configs from struct renesas_usbhs_driver_param * Updated commit description for patch#5 * Dropped usbhsc_rzg2l_pipe[] and reusing the default_pipe[]. v1->v2: * Added Ack from Krzysztof Kozlowski for patch#1. * Added patch#2 for simplify obtaining device data. * Dropped using of_device_is_compatible() in probe. * Added usbhs_rzg2l_plat_info and replaced the device data for RZ/G2L from usbhs_rza2_plat_info->usbhs_rzg2l_plat_info. * Moved usbhsc_rzg2l_pipe table near to the user. * Updated commit description in patch#3 * Added Rb tag from Geert for patch#4. * Updated commit description about ABI breakage in patch#5. * Updated commit header in patch#5 as it is RZ/G2L specific. Biju Das (6): dt-bindings: usb: renesas,usbhs: Document RZ/G2L family compatible usb: renesas_usbhs: Simplify obtaining device data usb: renesas_usbhs: Improve usbhsc_default_pipe[] for isochronous transfers usb: renesas_usbhs: Update usbhs pipe configuration for RZ/G2L family usb: renesas_usbhs: Remove trailing comma in the terminator entry for OF table arm64: dts: renesas: r9a07g0{43,44,54}: Update RZ/G2L family compatible .../bindings/usb/renesas,usbhs.yaml | 6 ++- arch/arm64/boot/dts/renesas/r9a07g043.dtsi | 2 +- arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 2 +- arch/arm64/boot/dts/renesas/r9a07g054.dtsi | 2 +- drivers/usb/renesas_usbhs/common.c | 43 ++++++++++++------- drivers/usb/renesas_usbhs/rza.h | 1 + drivers/usb/renesas_usbhs/rza2.c | 13 ++++++ 7 files changed, 49 insertions(+), 20 deletions(-) -- 2.25.1
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Add serial support for RZ/V2H(P) SoC with earlycon. Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> --- v2 - > v3 - new patch --- drivers/tty/serial/sh-sci.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c index a85e7b9a2e49..4a60d77257d6 100644 --- a/drivers/tty/serial/sh-sci.c +++ b/drivers/tty/serial/sh-sci.c @@ -290,7 +290,7 @@ static const struct sci_port_params sci_port_params[SCIx_NR_REGTYPES] = { }, /* - * The "SCIFA" that is in RZ/A2, RZ/G2L and RZ/T. + * The "SCIFA" that is in RZ/A2, RZ/G2L, RZ/T and RZ/V2H. * It looks like a normal SCIF with FIFO data, but with a * compressed address space. Also, the break out of interrupts * are different: ERI/BRI, RXI, TXI, TEI, DRI. @@ -3224,6 +3224,10 @@ static const struct of_device_id of_sci_match[] __maybe_unused = { .compatible = "renesas,scif-r9a07g044", .data = SCI_OF_DATA(PORT_SCIF, SCIx_RZ_SCIFA_REGTYPE), }, + { + .compatible = "renesas,scif-r9a09g057", + .data = SCI_OF_DATA(PORT_SCIF, SCIx_RZ_SCIFA_REGTYPE), + }, /* Family-specific types */ { .compatible = "renesas,rcar-gen1-scif", @@ -3554,6 +3558,7 @@ OF_EARLYCON_DECLARE(sci, "renesas,sci", sci_early_console_setup); OF_EARLYCON_DECLARE(scif, "renesas,scif", scif_early_console_setup); OF_EARLYCON_DECLARE(scif, "renesas,scif-r7s9210", rzscifa_early_console_setup); OF_EARLYCON_DECLARE(scif, "renesas,scif-r9a07g044", rzscifa_early_console_setup); +OF_EARLYCON_DECLARE(scif, "renesas,scif-r9a09g057", rzscifa_early_console_setup); OF_EARLYCON_DECLARE(scifa, "renesas,scifa", scifa_early_console_setup); OF_EARLYCON_DECLARE(scifb, "renesas,scifb", scifb_early_console_setup); OF_EARLYCON_DECLARE(hscif, "renesas,hscif", hscif_early_console_setup); -- 2.34.1
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Document support for the Serial Communication Interface with FIFO (SCIF) available in the Renesas RZ/V2H(P) (R9A09G057) SoC. The SCIF interface in the Renesas RZ/V2H(P) is similar to that available in the RZ/G2L (R9A07G044) SoC, with the only difference being that the RZ/V2H(P) SoC has three additional interrupts: one for Tx end/Rx ready and the other two for Rx and Tx buffer full, which are edge-triggered. Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> --- v2->v3 - Added SoC specific compat string --- .../bindings/serial/renesas,scif.yaml | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/Documentation/devicetree/bindings/serial/renesas,scif.yaml b/Documentation/devicetree/bindings/serial/renesas,scif.yaml index 53f18e9810fd..e4ce13e20cd7 100644 --- a/Documentation/devicetree/bindings/serial/renesas,scif.yaml +++ b/Documentation/devicetree/bindings/serial/renesas,scif.yaml @@ -79,6 +79,8 @@ properties: - renesas,scif-r9a08g045 # RZ/G3S - const: renesas,scif-r9a07g044 # RZ/G2{L,LC} fallback + - const: renesas,scif-r9a09g057 # RZ/V2H(P) + reg: maxItems: 1 @@ -204,6 +206,37 @@ allOf: - const: dri - const: tei + - if: + properties: + compatible: + contains: + const: renesas,scif-r9a09g057 + then: + properties: + interrupts: + items: + - description: Error interrupt + - description: Receive buffer full interrupt + - description: Transmit buffer empty interrupt + - description: Break interrupt + - description: Data Ready interrupt + - description: Transmit End interrupt + - description: Transmit End/Data Ready interrupt + - description: Receive buffer full interrupt (EDGE trigger) + - description: Transmit buffer empty interrupt (EDGE trigger) + + interrupt-names: + items: + - const: eri + - const: rxi + - const: txi + - const: bri + - const: dri + - const: tei + - const: tei-dri + - const: rxi-edge + - const: txi-edge + unevaluatedProperties: false examples: -- 2.34.1
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Add support to validate the 'interrupts' and 'interrupt-names' properties for every supported SoC. This ensures proper handling and configuration of interrupt-related properties across supported platforms. Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> --- v2->v3 - Listed interrupts and interrupt-names for every SoC in if check --- .../bindings/serial/renesas,scif.yaml | 95 ++++++++++++------- 1 file changed, 63 insertions(+), 32 deletions(-) diff --git a/Documentation/devicetree/bindings/serial/renesas,scif.yaml b/Documentation/devicetree/bindings/serial/renesas,scif.yaml index af72c3420453..53f18e9810fd 100644 --- a/Documentation/devicetree/bindings/serial/renesas,scif.yaml +++ b/Documentation/devicetree/bindings/serial/renesas,scif.yaml @@ -82,38 +82,6 @@ properties: reg: maxItems: 1 - interrupts: - oneOf: - - items: - - description: A combined interrupt - - items: - - description: Error interrupt - - description: Receive buffer full interrupt - - description: Transmit buffer empty interrupt - - description: Break interrupt - - items: - - description: Error interrupt - - description: Receive buffer full interrupt - - description: Transmit buffer empty interrupt - - description: Break interrupt - - description: Data Ready interrupt - - description: Transmit End interrupt - - interrupt-names: - oneOf: - - items: - - const: eri - - const: rxi - - const: txi - - const: bri - - items: - - const: eri - - const: rxi - - const: txi - - const: bri - - const: dri - - const: tei - clocks: minItems: 1 maxItems: 4 @@ -173,6 +141,69 @@ allOf: required: - resets + - if: + properties: + compatible: + contains: + enum: + - renesas,rcar-gen1-scif + - renesas,rcar-gen2-scif + - renesas,rcar-gen3-scif + - renesas,rcar-gen4-scif + then: + properties: + interrupts: + items: + - description: Single combined interrupt + + - if: + properties: + compatible: + contains: + const: renesas,scif-r7s72100 + then: + properties: + interrupts: + items: + - description: Error interrupt + - description: Receive buffer full interrupt + - description: Transmit buffer empty interrupt + - description: Break interrupt + + interrupt-names: + items: + - const: eri + - const: rxi + - const: txi + - const: bri + + - if: + properties: + compatible: + contains: + enum: + - renesas,scif-r7s9210 + - renesas,scif-r9a07g044 + then: + properties: + interrupts: + items: + - description: Error interrupt + - description: Receive buffer full interrupt + - description: Transmit buffer empty interrupt + - description: Break interrupt + - description: Data Ready interrupt + - description: Transmit End interrupt + + interrupt-names: + items: + - const: eri + - const: rxi + - const: txi + - const: bri + - const: dri + - const: tei + unevaluatedProperties: false examples: -- 2.34.1
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> In preparation for adding more validation checks move the ref for 'serial.yaml' to the end and also move reset check in 'allOf' block. Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> --- v2->v3 - no change --- .../bindings/serial/renesas,scif.yaml | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/Documentation/devicetree/bindings/serial/renesas,scif.yaml b/Documentation/devicetree/bindings/serial/renesas,scif.yaml index 4610a5bd580c..af72c3420453 100644 --- a/Documentation/devicetree/bindings/serial/renesas,scif.yaml +++ b/Documentation/devicetree/bindings/serial/renesas,scif.yaml @@ -9,9 +9,6 @@ title: Renesas Serial Communication Interface with FIFO (SCIF) maintainers: - Geert Uytterhoeven <geert+renesas@glider.be> -allOf: - - $ref: serial.yaml# - properties: compatible: oneOf: @@ -160,18 +157,21 @@ required: - clock-names - power-domains -if: - properties: - compatible: - contains: - enum: - - renesas,rcar-gen2-scif - - renesas,rcar-gen3-scif - - renesas,rcar-gen4-scif - - renesas,scif-r9a07g044 -then: - required: - - resets +allOf: + - $ref: serial.yaml# + + - if: + properties: + compatible: + contains: + enum: + - renesas,rcar-gen2-scif + - renesas,rcar-gen3-scif + - renesas,rcar-gen4-scif + - renesas,scif-r9a07g044 + then: + required: + - resets unevaluatedProperties: false -- 2.34.1