| Message ID | 20210611165624.30749-4-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | Add RZ/G2L I2C support | expand |
Hi Biju, On Fri, Jun 11, 2021 at 6:56 PM Biju Das <biju.das.jz@bp.renesas.com> wrote: > RZ/G2L i2c controller is compatible with RZ/A i2c controller. > By default IP is in reset state, so need to perform release > reset before accessing any register. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Thanks for your patch! > --- a/drivers/i2c/busses/Kconfig > +++ b/drivers/i2c/busses/Kconfig > @@ -941,6 +941,7 @@ config I2C_QUP > config I2C_RIIC > tristate "Renesas RIIC adapter" > depends on ARCH_RENESAS || COMPILE_TEST > + select RESET_CONTROLLER if ARCH_R9A07G044? > --- a/drivers/i2c/busses/i2c-riic.c > +++ b/drivers/i2c/busses/i2c-riic.c _kzalloc(&pdev->dev, sizeof(*riic), GFP_KERNEL); > if (!riic) > @@ -412,6 +421,17 @@ static int riic_i2c_probe(struct platform_device *pdev) > return PTR_ERR(riic->clk); > } > > + type = (enum riic_type)of_device_get_match_data(&pdev->dev); > + if (type == RIIC_RZ_G2L) { > + rstc = devm_reset_control_get(&pdev->dev, NULL); > + if (IS_ERR(rstc)) { > + dev_err(&pdev->dev, "Error: missing reset ctrl\n"); > + return PTR_ERR(rstc); > + } > + > + reset_control_deassert(rstc); Just wondering: does it harm if the driver is unloaded or unbounded, and rebound while the I2C controller is not in reset state? > + } > + > for (i = 0; i < ARRAY_SIZE(riic_irqs); i++) { > res = platform_get_resource(pdev, IORESOURCE_IRQ, riic_irqs[i].res_num); > if (!res) > @@ -472,6 +492,7 @@ static int riic_i2c_remove(struct platform_device *pdev) > } > > static const struct of_device_id riic_i2c_dt_ids[] = { > + { .compatible = "renesas,riic-r9a07g044", .data = (void *)RIIC_RZ_G2L }, > { .compatible = "renesas,riic-rz" }, Please fill in .data, to avoid relying implicitly on RIIC_RZ_A being zero. > { /* Sentinel */ }, > }; Gr{oetje,eeting}s, Geert
Hi Biju, On Fri, 2021-06-11 at 17:56 +0100, Biju Das wrote: > RZ/G2L i2c controller is compatible with RZ/A i2c controller. > By default IP is in reset state, so need to perform release > reset before accessing any register. > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > drivers/i2c/busses/Kconfig | 1 + > drivers/i2c/busses/i2c-riic.c | 21 +++++++++++++++++++++ > 2 files changed, 22 insertions(+) > > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > index 281a65d9b44b..5da09288b461 100644 > --- a/drivers/i2c/busses/Kconfig > +++ b/drivers/i2c/busses/Kconfig > @@ -941,6 +941,7 @@ config I2C_QUP > config I2C_RIIC > tristate "Renesas RIIC adapter" > depends on ARCH_RENESAS || COMPILE_TEST > + select RESET_CONTROLLER There's no need for this. The reset API defines inline stubs so this can be compiled without RESET_CONTROLLER enabled. > help > If you say yes to this option, support will be included for the > Renesas RIIC I2C interface. > diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c > index 4eccc0f69861..a3860631c6db 100644 > --- a/drivers/i2c/busses/i2c-riic.c > +++ b/drivers/i2c/busses/i2c-riic.c > @@ -42,8 +42,10 @@ > #include <linux/io.h> > #include <linux/module.h> > #include <linux/of.h> > +#include <linux/of_device.h> > #include <linux/platform_device.h> > #include <linux/pm_runtime.h> > +#include <linux/reset.h> > > #define RIIC_ICCR1 0x00 > #define RIIC_ICCR2 0x04 > @@ -86,6 +88,11 @@ > > #define RIIC_INIT_MSG -1 > > +enum riic_type { > + RIIC_RZ_A, > + RIIC_RZ_G2L, > +}; > + > struct riic_dev { > void __iomem *base; > u8 *buf; > @@ -395,7 +402,9 @@ static int riic_i2c_probe(struct platform_device *pdev) > struct i2c_adapter *adap; > struct resource *res; > struct i2c_timings i2c_t; > + struct reset_control *rstc; > int i, ret; > + enum riic_type type; > > riic = devm_kzalloc(&pdev->dev, sizeof(*riic), GFP_KERNEL); > if (!riic) > @@ -412,6 +421,17 @@ static int riic_i2c_probe(struct platform_device *pdev) > return PTR_ERR(riic->clk); > } > > + type = (enum riic_type)of_device_get_match_data(&pdev->dev); > + if (type == RIIC_RZ_G2L) { > + rstc = devm_reset_control_get(&pdev->dev, NULL); Please use devm_reset_control_get_exclusive(). > + if (IS_ERR(rstc)) { > + dev_err(&pdev->dev, "Error: missing reset ctrl\n"); > + return PTR_ERR(rstc); > + } > + > + reset_control_deassert(rstc); > + } > + > for (i = 0; i < ARRAY_SIZE(riic_irqs); i++) { > res = platform_get_resource(pdev, IORESOURCE_IRQ, riic_irqs[i].res_num); > if (!res) > @@ -472,6 +492,7 @@ static int riic_i2c_remove(struct platform_device *pdev) > } > > static const struct of_device_id riic_i2c_dt_ids[] = { > + { .compatible = "renesas,riic-r9a07g044", .data = (void *)RIIC_RZ_G2L }, > { .compatible = "renesas,riic-rz" }, > { /* Sentinel */ }, > }; regards Philipp
Hi Philipp, On Mon, Jun 14, 2021 at 3:27 PM Philipp Zabel <p.zabel@pengutronix.de> wrote: > On Fri, 2021-06-11 at 17:56 +0100, Biju Das wrote: > > RZ/G2L i2c controller is compatible with RZ/A i2c controller. > > By default IP is in reset state, so need to perform release > > reset before accessing any register. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > drivers/i2c/busses/Kconfig | 1 + > > drivers/i2c/busses/i2c-riic.c | 21 +++++++++++++++++++++ > > 2 files changed, 22 insertions(+) > > > > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > > index 281a65d9b44b..5da09288b461 100644 > > --- a/drivers/i2c/busses/Kconfig > > +++ b/drivers/i2c/busses/Kconfig > > @@ -941,6 +941,7 @@ config I2C_QUP > > config I2C_RIIC > > tristate "Renesas RIIC adapter" > > depends on ARCH_RENESAS || COMPILE_TEST > > + select RESET_CONTROLLER > > There's no need for this. The reset API defines inline stubs so this can > be compiled without RESET_CONTROLLER enabled. AFAIK, the issue is that RIIC on RZ/G2L requires reset support, so it must be enabled when building a kernel for RZ/G2L. As RZ/A does not need or use it, and may run from SRAM, I'd like to leave it disabled when building a kernel not including RZ/G2L support. Gr{oetje,eeting}s, Geert
Hi Geert, Thanks for the feedback. > -----Original Message----- > Subject: Re: [PATCH 3/5] i2c: riic: Add RZ/G2L support > > Hi Biju, > > On Fri, Jun 11, 2021 at 6:56 PM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > RZ/G2L i2c controller is compatible with RZ/A i2c controller. > > By default IP is in reset state, so need to perform release reset > > before accessing any register. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Thanks for your patch! > > > --- a/drivers/i2c/busses/Kconfig > > +++ b/drivers/i2c/busses/Kconfig > > @@ -941,6 +941,7 @@ config I2C_QUP > > config I2C_RIIC > > tristate "Renesas RIIC adapter" > > depends on ARCH_RENESAS || COMPILE_TEST > > + select RESET_CONTROLLER > > if ARCH_R9A07G044? OK,Agreed. > > > --- a/drivers/i2c/busses/i2c-riic.c > > +++ b/drivers/i2c/busses/i2c-riic.c > _kzalloc(&pdev->dev, sizeof(*riic), GFP_KERNEL); > > if (!riic) > > @@ -412,6 +421,17 @@ static int riic_i2c_probe(struct platform_device > *pdev) > > return PTR_ERR(riic->clk); > > } > > > > + type = (enum riic_type)of_device_get_match_data(&pdev->dev); > > + if (type == RIIC_RZ_G2L) { > > + rstc = devm_reset_control_get(&pdev->dev, NULL); > > + if (IS_ERR(rstc)) { > > + dev_err(&pdev->dev, "Error: missing reset > ctrl\n"); > > + return PTR_ERR(rstc); > > + } > > + > > + reset_control_deassert(rstc); > > Just wondering: does it harm if the driver is unloaded or unbounded, and > rebound while the I2C controller is not in reset state? I have tested and it works after unloading and reloading it again. Cheers, Biju > > > + } > > + > > for (i = 0; i < ARRAY_SIZE(riic_irqs); i++) { > > res = platform_get_resource(pdev, IORESOURCE_IRQ, > riic_irqs[i].res_num); > > if (!res) > > @@ -472,6 +492,7 @@ static int riic_i2c_remove(struct platform_device > > *pdev) } > > > > static const struct of_device_id riic_i2c_dt_ids[] = { > > + { .compatible = "renesas,riic-r9a07g044", .data = (void > > + *)RIIC_RZ_G2L }, > > { .compatible = "renesas,riic-rz" }, > > Please fill in .data, to avoid relying implicitly on RIIC_RZ_A being zero. > > > { /* Sentinel */ }, > > }; > > 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 Philipp, Thanks for the review. > Subject: Re: [PATCH 3/5] i2c: riic: Add RZ/G2L support > > Hi Biju, > > On Fri, 2021-06-11 at 17:56 +0100, Biju Das wrote: > > RZ/G2L i2c controller is compatible with RZ/A i2c controller. > > By default IP is in reset state, so need to perform release reset > > before accessing any register. > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > drivers/i2c/busses/Kconfig | 1 + > > drivers/i2c/busses/i2c-riic.c | 21 +++++++++++++++++++++ > > 2 files changed, 22 insertions(+) > > > > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > > index 281a65d9b44b..5da09288b461 100644 > > --- a/drivers/i2c/busses/Kconfig > > +++ b/drivers/i2c/busses/Kconfig > > @@ -941,6 +941,7 @@ config I2C_QUP > > config I2C_RIIC > > tristate "Renesas RIIC adapter" > > depends on ARCH_RENESAS || COMPILE_TEST > > + select RESET_CONTROLLER > > There's no need for this. The reset API defines inline stubs so this can > be compiled without RESET_CONTROLLER enabled. Hope you are ok with Geert's suggestion, as RZ/A boards are resource constrained. So it is not good to enable for them. > > > help > > If you say yes to this option, support will be included for the > > Renesas RIIC I2C interface. > > diff --git a/drivers/i2c/busses/i2c-riic.c > > b/drivers/i2c/busses/i2c-riic.c index 4eccc0f69861..a3860631c6db > > 100644 > > --- a/drivers/i2c/busses/i2c-riic.c > > +++ b/drivers/i2c/busses/i2c-riic.c > > @@ -42,8 +42,10 @@ > > #include <linux/io.h> > > #include <linux/module.h> > > #include <linux/of.h> > > +#include <linux/of_device.h> > > #include <linux/platform_device.h> > > #include <linux/pm_runtime.h> > > +#include <linux/reset.h> > > > > #define RIIC_ICCR1 0x00 > > #define RIIC_ICCR2 0x04 > > @@ -86,6 +88,11 @@ > > > > #define RIIC_INIT_MSG -1 > > > > +enum riic_type { > > + RIIC_RZ_A, > > + RIIC_RZ_G2L, > > +}; > > + > > struct riic_dev { > > void __iomem *base; > > u8 *buf; > > @@ -395,7 +402,9 @@ static int riic_i2c_probe(struct platform_device > *pdev) > > struct i2c_adapter *adap; > > struct resource *res; > > struct i2c_timings i2c_t; > > + struct reset_control *rstc; > > int i, ret; > > + enum riic_type type; > > > > riic = devm_kzalloc(&pdev->dev, sizeof(*riic), GFP_KERNEL); > > if (!riic) > > @@ -412,6 +421,17 @@ static int riic_i2c_probe(struct platform_device > *pdev) > > return PTR_ERR(riic->clk); > > } > > > > + type = (enum riic_type)of_device_get_match_data(&pdev->dev); > > + if (type == RIIC_RZ_G2L) { > > + rstc = devm_reset_control_get(&pdev->dev, NULL); > > Please use devm_reset_control_get_exclusive(). OK. Will do. Regards, Biju > > > + if (IS_ERR(rstc)) { > > + dev_err(&pdev->dev, "Error: missing reset ctrl\n"); > > + return PTR_ERR(rstc); > > + } > > + > > + reset_control_deassert(rstc); > > + } > > + > > for (i = 0; i < ARRAY_SIZE(riic_irqs); i++) { > > res = platform_get_resource(pdev, IORESOURCE_IRQ, > riic_irqs[i].res_num); > > if (!res) > > @@ -472,6 +492,7 @@ static int riic_i2c_remove(struct platform_device > > *pdev) } > > > > static const struct of_device_id riic_i2c_dt_ids[] = { > > + { .compatible = "renesas,riic-r9a07g044", .data = (void > > +*)RIIC_RZ_G2L }, > > { .compatible = "renesas,riic-rz" }, > > { /* Sentinel */ }, > > }; > > regards > Philipp
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 281a65d9b44b..5da09288b461 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -941,6 +941,7 @@ config I2C_QUP config I2C_RIIC tristate "Renesas RIIC adapter" depends on ARCH_RENESAS || COMPILE_TEST + select RESET_CONTROLLER help If you say yes to this option, support will be included for the Renesas RIIC I2C interface. diff --git a/drivers/i2c/busses/i2c-riic.c b/drivers/i2c/busses/i2c-riic.c index 4eccc0f69861..a3860631c6db 100644 --- a/drivers/i2c/busses/i2c-riic.c +++ b/drivers/i2c/busses/i2c-riic.c @@ -42,8 +42,10 @@ #include <linux/io.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/of_device.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> +#include <linux/reset.h> #define RIIC_ICCR1 0x00 #define RIIC_ICCR2 0x04 @@ -86,6 +88,11 @@ #define RIIC_INIT_MSG -1 +enum riic_type { + RIIC_RZ_A, + RIIC_RZ_G2L, +}; + struct riic_dev { void __iomem *base; u8 *buf; @@ -395,7 +402,9 @@ static int riic_i2c_probe(struct platform_device *pdev) struct i2c_adapter *adap; struct resource *res; struct i2c_timings i2c_t; + struct reset_control *rstc; int i, ret; + enum riic_type type; riic = devm_kzalloc(&pdev->dev, sizeof(*riic), GFP_KERNEL); if (!riic) @@ -412,6 +421,17 @@ static int riic_i2c_probe(struct platform_device *pdev) return PTR_ERR(riic->clk); } + type = (enum riic_type)of_device_get_match_data(&pdev->dev); + if (type == RIIC_RZ_G2L) { + rstc = devm_reset_control_get(&pdev->dev, NULL); + if (IS_ERR(rstc)) { + dev_err(&pdev->dev, "Error: missing reset ctrl\n"); + return PTR_ERR(rstc); + } + + reset_control_deassert(rstc); + } + for (i = 0; i < ARRAY_SIZE(riic_irqs); i++) { res = platform_get_resource(pdev, IORESOURCE_IRQ, riic_irqs[i].res_num); if (!res) @@ -472,6 +492,7 @@ static int riic_i2c_remove(struct platform_device *pdev) } static const struct of_device_id riic_i2c_dt_ids[] = { + { .compatible = "renesas,riic-r9a07g044", .data = (void *)RIIC_RZ_G2L }, { .compatible = "renesas,riic-rz" }, { /* Sentinel */ }, };