| Message ID | 20211103173127.13701-1-prabhakar.mahadev-lad.rj@bp.renesas.com |
|---|---|
| Headers | show |
| Series | RZ/G2L SCI support and sh-sci driver update | expand |
Hi Prabhakar, On Wed, Nov 3, 2021 at 6:31 PM Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > On RZ/G2L devices should be explicitly pulled out of reset for it > to work. This patch adds support to read the "resets" property and > performs deassert/assert when required. > > Also, propagate the error to the caller of sci_parse_dt() instead of > returning NULL in case of failure. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> Thanks for your patch! > --- > Hi Geert, > For handling the resets I was in dual mind whether to perform > reset based on compatible strings or soc-id, let me know your > thoughts. Currently no SoC's use "renesas,sci" so using the same > for performing the reset operation for SCI. We do, on H8/300. > --- a/drivers/tty/serial/sh-sci.c > +++ b/drivers/tty/serial/sh-sci.c > @@ -3203,23 +3204,58 @@ static const struct of_device_id of_sci_match[] = { > }; > MODULE_DEVICE_TABLE(of, of_sci_match); > > +static void sci_reset_control_assert(void *data) > +{ > + reset_control_assert(data); > +} > + > static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev, > unsigned int *dev_id) > { > struct device_node *np = pdev->dev.of_node; > + const struct of_device_id *of_id; > struct plat_sci_port *p; > struct sci_port *sp; > const void *data; > int id; > > if (!IS_ENABLED(CONFIG_OF) || !np) > - return NULL; > + return ERR_PTR(-EINVAL); > + > + of_id = of_match_device(of_sci_match, &pdev->dev); > + if (!of_id) > + return ERR_PTR(-EINVAL); > > - data = of_device_get_match_data(&pdev->dev); > + if (!strcmp(of_id->compatible, "renesas,scif-r9a07g044") || > + !strcmp(of_id->compatible, "renesas,sci")) { This will match on H8/300, too, which doesn't have resets. Please match against "renesas,sci-r9a07g044" instead. Please don't use explicit strcmp() calls here, but add a flag to of_sci_match[].data. > + struct reset_control *rstc; > + int ret; > + > + rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL); > + if (IS_ERR(rstc)) { > + dev_err(&pdev->dev, "Error: missing reset ctrl\n"); > + return ERR_PTR(PTR_ERR(rstc)); > + } > + > + ret = reset_control_deassert(rstc); > + if (ret) { > + dev_err(&pdev->dev, "failed to deassert %d\n", ret); > + return ERR_PTR(ret); > + } > + > + ret = devm_add_action_or_reset(&pdev->dev, sci_reset_control_assert, rstc); > + if (ret) { > + dev_err(&pdev->dev, "failed to register assert devm action, %d\n", > + ret); > + return ERR_PTR(ret); > + } > + } > + > + data = of_id->data; > 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, Thank you for the review. On Mon, Nov 8, 2021 at 4:40 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Wed, Nov 3, 2021 at 6:31 PM Lad Prabhakar > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > On RZ/G2L devices should be explicitly pulled out of reset for it > > to work. This patch adds support to read the "resets" property and > > performs deassert/assert when required. > > > > Also, propagate the error to the caller of sci_parse_dt() instead of > > returning NULL in case of failure. > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> > > Thanks for your patch! > > > --- > > Hi Geert, > > For handling the resets I was in dual mind whether to perform > > reset based on compatible strings or soc-id, let me know your > > thoughts. Currently no SoC's use "renesas,sci" so using the same > > for performing the reset operation for SCI. > > We do, on H8/300. > Missed that. > > --- a/drivers/tty/serial/sh-sci.c > > +++ b/drivers/tty/serial/sh-sci.c > > @@ -3203,23 +3204,58 @@ static const struct of_device_id of_sci_match[] = { > > }; > > MODULE_DEVICE_TABLE(of, of_sci_match); > > > > +static void sci_reset_control_assert(void *data) > > +{ > > + reset_control_assert(data); > > +} > > + > > static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev, > > unsigned int *dev_id) > > { > > struct device_node *np = pdev->dev.of_node; > > + const struct of_device_id *of_id; > > struct plat_sci_port *p; > > struct sci_port *sp; > > const void *data; > > int id; > > > > if (!IS_ENABLED(CONFIG_OF) || !np) > > - return NULL; > > + return ERR_PTR(-EINVAL); > > + > > + of_id = of_match_device(of_sci_match, &pdev->dev); > > + if (!of_id) > > + return ERR_PTR(-EINVAL); > > > > - data = of_device_get_match_data(&pdev->dev); > > + if (!strcmp(of_id->compatible, "renesas,scif-r9a07g044") || > > + !strcmp(of_id->compatible, "renesas,sci")) { > > This will match on H8/300, too, which doesn't have resets. > Please match against "renesas,sci-r9a07g044" instead. > > Please don't use explicit strcmp() calls here, but add a flag to > of_sci_match[].data. > Agreed, does the below hunk look good for handling the reset? -#define SCI_OF_DATA(type, regtype) (void *)((type) << 16 | (regtype)) +#define SCIx_RESET BIT(31) +#define SCI_OF_DATA(type, regtype, reset) (void *)(reset | (type) << 16 | (regtype)) #define SCI_OF_TYPE(data) ((unsigned long)(data) >> 16) #define SCI_OF_REGTYPE(data) ((unsigned long)(data) & 0xffff) @@ -3169,7 +3170,11 @@ static const struct of_device_id of_sci_match[] = { }, { .compatible = "renesas,scif-r9a07g044", - .data = SCI_OF_DATA(PORT_SCIF, SCIx_RZ_SCIFA_REGTYPE), + .data = SCI_OF_DATA(PORT_SCIF, SCIx_RZ_SCIFA_REGTYPE, SCIx_RESET), + }, + { + .compatible = "renesas,sci-r9a07g044", + .data = SCI_OF_DATA(PORT_SCI, SCIx_SCI_REGTYPE, SCIx_RESET), }, Cheers, Prabhakar > > + struct reset_control *rstc; > > + int ret; > > + > > + rstc = devm_reset_control_get_exclusive(&pdev->dev, NULL); > > + if (IS_ERR(rstc)) { > > + dev_err(&pdev->dev, "Error: missing reset ctrl\n"); > > + return ERR_PTR(PTR_ERR(rstc)); > > + } > > + > > + ret = reset_control_deassert(rstc); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to deassert %d\n", ret); > > + return ERR_PTR(ret); > > + } > > + > > + ret = devm_add_action_or_reset(&pdev->dev, sci_reset_control_assert, rstc); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to register assert devm action, %d\n", > > + ret); > > + return ERR_PTR(ret); > > + } > > + } > > + > > + data = of_id->data; > > > > 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 Tue, Nov 9, 2021 at 1:34 AM Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote: > On Mon, Nov 8, 2021 at 4:40 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Wed, Nov 3, 2021 at 6:31 PM Lad Prabhakar > > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > On RZ/G2L devices should be explicitly pulled out of reset for it > > > to work. This patch adds support to read the "resets" property and > > > performs deassert/assert when required. > > > > > > Also, propagate the error to the caller of sci_parse_dt() instead of > > > returning NULL in case of failure. > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> > > > > Thanks for your patch! > > > > > --- > > > Hi Geert, > > > For handling the resets I was in dual mind whether to perform > > > reset based on compatible strings or soc-id, let me know your > > > thoughts. Currently no SoC's use "renesas,sci" so using the same > > > for performing the reset operation for SCI. > > > > We do, on H8/300. > > > Missed that. > > > > --- a/drivers/tty/serial/sh-sci.c > > > +++ b/drivers/tty/serial/sh-sci.c > > > @@ -3203,23 +3204,58 @@ static const struct of_device_id of_sci_match[] = { > > > }; > > > MODULE_DEVICE_TABLE(of, of_sci_match); > > > > > > +static void sci_reset_control_assert(void *data) > > > +{ > > > + reset_control_assert(data); > > > +} > > > + > > > static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev, > > > unsigned int *dev_id) > > > { > > > struct device_node *np = pdev->dev.of_node; > > > + const struct of_device_id *of_id; > > > struct plat_sci_port *p; > > > struct sci_port *sp; > > > const void *data; > > > int id; > > > > > > if (!IS_ENABLED(CONFIG_OF) || !np) > > > - return NULL; > > > + return ERR_PTR(-EINVAL); > > > + > > > + of_id = of_match_device(of_sci_match, &pdev->dev); > > > + if (!of_id) > > > + return ERR_PTR(-EINVAL); > > > > > > - data = of_device_get_match_data(&pdev->dev); > > > + if (!strcmp(of_id->compatible, "renesas,scif-r9a07g044") || > > > + !strcmp(of_id->compatible, "renesas,sci")) { > > > > This will match on H8/300, too, which doesn't have resets. > > Please match against "renesas,sci-r9a07g044" instead. > > > > Please don't use explicit strcmp() calls here, but add a flag to > > of_sci_match[].data. > > > Agreed, does the below hunk look good for handling the reset? > > -#define SCI_OF_DATA(type, regtype) (void *)((type) << 16 | (regtype)) > +#define SCIx_RESET BIT(31) > +#define SCI_OF_DATA(type, regtype, reset) (void *)(reset | > (type) << 16 | (regtype)) > #define SCI_OF_TYPE(data) ((unsigned long)(data) >> 16) > #define SCI_OF_REGTYPE(data) ((unsigned long)(data) & 0xffff) > > @@ -3169,7 +3170,11 @@ static const struct of_device_id of_sci_match[] = { > }, > { > .compatible = "renesas,scif-r9a07g044", > - .data = SCI_OF_DATA(PORT_SCIF, SCIx_RZ_SCIFA_REGTYPE), > + .data = SCI_OF_DATA(PORT_SCIF, SCIx_RZ_SCIFA_REGTYPE, > SCIx_RESET), > + }, > + { > + .compatible = "renesas,sci-r9a07g044", > + .data = SCI_OF_DATA(PORT_SCI, SCIx_SCI_REGTYPE, SCIx_RESET), > }, That's what I had in mind. But upon second thought, it might be better to just drop the check, and obtain an optional reset instead? After all the reset requirement is not a feature of this specific SCI(F) variant, but an SoC integration feature. And deasserting the reset on other SoCs that have a reset should be fine. 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, On Tue, Nov 9, 2021 at 7:50 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Prabhakar, > > On Tue, Nov 9, 2021 at 1:34 AM Lad, Prabhakar > <prabhakar.csengg@gmail.com> wrote: > > On Mon, Nov 8, 2021 at 4:40 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > On Wed, Nov 3, 2021 at 6:31 PM Lad Prabhakar > > > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > > On RZ/G2L devices should be explicitly pulled out of reset for it > > > > to work. This patch adds support to read the "resets" property and > > > > performs deassert/assert when required. > > > > > > > > Also, propagate the error to the caller of sci_parse_dt() instead of > > > > returning NULL in case of failure. > > > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com> > > > > > > Thanks for your patch! > > > > > > > --- > > > > Hi Geert, > > > > For handling the resets I was in dual mind whether to perform > > > > reset based on compatible strings or soc-id, let me know your > > > > thoughts. Currently no SoC's use "renesas,sci" so using the same > > > > for performing the reset operation for SCI. > > > > > > We do, on H8/300. > > > > > Missed that. > > > > > > --- a/drivers/tty/serial/sh-sci.c > > > > +++ b/drivers/tty/serial/sh-sci.c > > > > @@ -3203,23 +3204,58 @@ static const struct of_device_id of_sci_match[] = { > > > > }; > > > > MODULE_DEVICE_TABLE(of, of_sci_match); > > > > > > > > +static void sci_reset_control_assert(void *data) > > > > +{ > > > > + reset_control_assert(data); > > > > +} > > > > + > > > > static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev, > > > > unsigned int *dev_id) > > > > { > > > > struct device_node *np = pdev->dev.of_node; > > > > + const struct of_device_id *of_id; > > > > struct plat_sci_port *p; > > > > struct sci_port *sp; > > > > const void *data; > > > > int id; > > > > > > > > if (!IS_ENABLED(CONFIG_OF) || !np) > > > > - return NULL; > > > > + return ERR_PTR(-EINVAL); > > > > + > > > > + of_id = of_match_device(of_sci_match, &pdev->dev); > > > > + if (!of_id) > > > > + return ERR_PTR(-EINVAL); > > > > > > > > - data = of_device_get_match_data(&pdev->dev); > > > > + if (!strcmp(of_id->compatible, "renesas,scif-r9a07g044") || > > > > + !strcmp(of_id->compatible, "renesas,sci")) { > > > > > > This will match on H8/300, too, which doesn't have resets. > > > Please match against "renesas,sci-r9a07g044" instead. > > > > > > Please don't use explicit strcmp() calls here, but add a flag to > > > of_sci_match[].data. > > > > > Agreed, does the below hunk look good for handling the reset? > > > > -#define SCI_OF_DATA(type, regtype) (void *)((type) << 16 | (regtype)) > > +#define SCIx_RESET BIT(31) > > +#define SCI_OF_DATA(type, regtype, reset) (void *)(reset | > > (type) << 16 | (regtype)) > > #define SCI_OF_TYPE(data) ((unsigned long)(data) >> 16) > > #define SCI_OF_REGTYPE(data) ((unsigned long)(data) & 0xffff) > > > > @@ -3169,7 +3170,11 @@ static const struct of_device_id of_sci_match[] = { > > }, > > { > > .compatible = "renesas,scif-r9a07g044", > > - .data = SCI_OF_DATA(PORT_SCIF, SCIx_RZ_SCIFA_REGTYPE), > > + .data = SCI_OF_DATA(PORT_SCIF, SCIx_RZ_SCIFA_REGTYPE, > > SCIx_RESET), > > + }, > > + { > > + .compatible = "renesas,sci-r9a07g044", > > + .data = SCI_OF_DATA(PORT_SCI, SCIx_SCI_REGTYPE, SCIx_RESET), > > }, > > That's what I had in mind. > > But upon second thought, it might be better to just drop the check, > and obtain an optional reset instead? Agreed, will do that. > After all the reset requirement is not a feature of this specific > SCI(F) variant, but an SoC integration feature. And deasserting > the reset on other SoCs that have a reset should be fine. > Indeed. Cheers, Prabhakar > 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