| Message ID | 20190723202758.21295-4-simon.k.r.goldschmidt@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Simon Goldschmidt |
| Headers | show |
| Series | arm: socfpga: gen5: DM improvements | expand |
On 7/23/19 10:27 PM, Simon Goldschmidt wrote: > To use this timer on socfpga as system tick, it needs to take itself out > of reset. > > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> > --- > > drivers/timer/dw-apb-timer.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/drivers/timer/dw-apb-timer.c b/drivers/timer/dw-apb-timer.c > index 86312b8dc7..fad22be8c9 100644 > --- a/drivers/timer/dw-apb-timer.c > +++ b/drivers/timer/dw-apb-timer.c > @@ -8,6 +8,7 @@ > #include <common.h> > #include <dm.h> > #include <clk.h> > +#include <reset.h> > #include <timer.h> > > #include <asm/io.h> > @@ -18,7 +19,8 @@ > #define DW_APB_CTRL 0x8 > > struct dw_apb_timer_priv { > - fdt_addr_t regs; > + fdt_addr_t regs; > + struct reset_ctl_bulk resets; > }; > > static int dw_apb_timer_get_count(struct udevice *dev, u64 *count) > @@ -42,6 +44,12 @@ static int dw_apb_timer_probe(struct udevice *dev) > struct clk clk; > int ret; > > + ret = reset_get_bulk(dev, &priv->resets); > + if (ret) > + dev_warn(dev, "Can't get reset: %d\n", ret); Shouldn't this be printed by the subsystem ? [...]
On Wed, Jul 24, 2019 at 9:31 AM Marek Vasut <marex@denx.de> wrote: > > On 7/23/19 10:27 PM, Simon Goldschmidt wrote: > > To use this timer on socfpga as system tick, it needs to take itself out > > of reset. > > > > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> > > --- > > > > drivers/timer/dw-apb-timer.c | 18 +++++++++++++++++- > > 1 file changed, 17 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/timer/dw-apb-timer.c b/drivers/timer/dw-apb-timer.c > > index 86312b8dc7..fad22be8c9 100644 > > --- a/drivers/timer/dw-apb-timer.c > > +++ b/drivers/timer/dw-apb-timer.c > > @@ -8,6 +8,7 @@ > > #include <common.h> > > #include <dm.h> > > #include <clk.h> > > +#include <reset.h> > > #include <timer.h> > > > > #include <asm/io.h> > > @@ -18,7 +19,8 @@ > > #define DW_APB_CTRL 0x8 > > > > struct dw_apb_timer_priv { > > - fdt_addr_t regs; > > + fdt_addr_t regs; > > + struct reset_ctl_bulk resets; > > }; > > > > static int dw_apb_timer_get_count(struct udevice *dev, u64 *count) > > @@ -42,6 +44,12 @@ static int dw_apb_timer_probe(struct udevice *dev) > > struct clk clk; > > int ret; > > > > + ret = reset_get_bulk(dev, &priv->resets); > > + if (ret) > > + dev_warn(dev, "Can't get reset: %d\n", ret); > > Shouldn't this be printed by the subsystem ? I don't think it's printed by the subsystem. Plus I don't know if it's a warning for all drivers? I also haven't really thought about that, as I just copied the reset handling code from another driver... Regards, Simon
On 7/24/19 9:43 AM, Simon Goldschmidt wrote: > On Wed, Jul 24, 2019 at 9:31 AM Marek Vasut <marex@denx.de> wrote: >> >> On 7/23/19 10:27 PM, Simon Goldschmidt wrote: >>> To use this timer on socfpga as system tick, it needs to take itself out >>> of reset. >>> >>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >>> --- >>> >>> drivers/timer/dw-apb-timer.c | 18 +++++++++++++++++- >>> 1 file changed, 17 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/timer/dw-apb-timer.c b/drivers/timer/dw-apb-timer.c >>> index 86312b8dc7..fad22be8c9 100644 >>> --- a/drivers/timer/dw-apb-timer.c >>> +++ b/drivers/timer/dw-apb-timer.c >>> @@ -8,6 +8,7 @@ >>> #include <common.h> >>> #include <dm.h> >>> #include <clk.h> >>> +#include <reset.h> >>> #include <timer.h> >>> >>> #include <asm/io.h> >>> @@ -18,7 +19,8 @@ >>> #define DW_APB_CTRL 0x8 >>> >>> struct dw_apb_timer_priv { >>> - fdt_addr_t regs; >>> + fdt_addr_t regs; >>> + struct reset_ctl_bulk resets; >>> }; >>> >>> static int dw_apb_timer_get_count(struct udevice *dev, u64 *count) >>> @@ -42,6 +44,12 @@ static int dw_apb_timer_probe(struct udevice *dev) >>> struct clk clk; >>> int ret; >>> >>> + ret = reset_get_bulk(dev, &priv->resets); >>> + if (ret) >>> + dev_warn(dev, "Can't get reset: %d\n", ret); >> >> Shouldn't this be printed by the subsystem ? > > I don't think it's printed by the subsystem. Plus I don't know if it's > a warning for all drivers? I also haven't really thought about that, as > I just copied the reset handling code from another driver... Hmmmm, I guess we cannot operate without clock anyway, so it should be dev_err(). And I guess it's indeed not a subsystem print afterall.
On Wed, Jul 24, 2019 at 9:48 AM Marek Vasut <marex@denx.de> wrote: > > On 7/24/19 9:43 AM, Simon Goldschmidt wrote: > > On Wed, Jul 24, 2019 at 9:31 AM Marek Vasut <marex@denx.de> wrote: > >> > >> On 7/23/19 10:27 PM, Simon Goldschmidt wrote: > >>> To use this timer on socfpga as system tick, it needs to take itself out > >>> of reset. > >>> > >>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> > >>> --- > >>> > >>> drivers/timer/dw-apb-timer.c | 18 +++++++++++++++++- > >>> 1 file changed, 17 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/timer/dw-apb-timer.c b/drivers/timer/dw-apb-timer.c > >>> index 86312b8dc7..fad22be8c9 100644 > >>> --- a/drivers/timer/dw-apb-timer.c > >>> +++ b/drivers/timer/dw-apb-timer.c > >>> @@ -8,6 +8,7 @@ > >>> #include <common.h> > >>> #include <dm.h> > >>> #include <clk.h> > >>> +#include <reset.h> > >>> #include <timer.h> > >>> > >>> #include <asm/io.h> > >>> @@ -18,7 +19,8 @@ > >>> #define DW_APB_CTRL 0x8 > >>> > >>> struct dw_apb_timer_priv { > >>> - fdt_addr_t regs; > >>> + fdt_addr_t regs; > >>> + struct reset_ctl_bulk resets; > >>> }; > >>> > >>> static int dw_apb_timer_get_count(struct udevice *dev, u64 *count) > >>> @@ -42,6 +44,12 @@ static int dw_apb_timer_probe(struct udevice *dev) > >>> struct clk clk; > >>> int ret; > >>> > >>> + ret = reset_get_bulk(dev, &priv->resets); > >>> + if (ret) > >>> + dev_warn(dev, "Can't get reset: %d\n", ret); > >> > >> Shouldn't this be printed by the subsystem ? > > > > I don't think it's printed by the subsystem. Plus I don't know if it's > > a warning for all drivers? I also haven't really thought about that, as > > I just copied the reset handling code from another driver... > > Hmmmm, I guess we cannot operate without clock anyway, so it should be > dev_err(). And I guess it's indeed not a subsystem print afterall. Well, for gen5, it would be dev_err. I'm not sure about arria10 and stratix10 though? I think at least stratix10 doesn't have a clock driver, yet. Regards, Simon
On 7/24/19 8:09 PM, Simon Goldschmidt wrote: > On Wed, Jul 24, 2019 at 9:48 AM Marek Vasut <marex@denx.de> wrote: >> >> On 7/24/19 9:43 AM, Simon Goldschmidt wrote: >>> On Wed, Jul 24, 2019 at 9:31 AM Marek Vasut <marex@denx.de> wrote: >>>> >>>> On 7/23/19 10:27 PM, Simon Goldschmidt wrote: >>>>> To use this timer on socfpga as system tick, it needs to take itself out >>>>> of reset. >>>>> >>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> >>>>> --- >>>>> >>>>> drivers/timer/dw-apb-timer.c | 18 +++++++++++++++++- >>>>> 1 file changed, 17 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/timer/dw-apb-timer.c b/drivers/timer/dw-apb-timer.c >>>>> index 86312b8dc7..fad22be8c9 100644 >>>>> --- a/drivers/timer/dw-apb-timer.c >>>>> +++ b/drivers/timer/dw-apb-timer.c >>>>> @@ -8,6 +8,7 @@ >>>>> #include <common.h> >>>>> #include <dm.h> >>>>> #include <clk.h> >>>>> +#include <reset.h> >>>>> #include <timer.h> >>>>> >>>>> #include <asm/io.h> >>>>> @@ -18,7 +19,8 @@ >>>>> #define DW_APB_CTRL 0x8 >>>>> >>>>> struct dw_apb_timer_priv { >>>>> - fdt_addr_t regs; >>>>> + fdt_addr_t regs; >>>>> + struct reset_ctl_bulk resets; >>>>> }; >>>>> >>>>> static int dw_apb_timer_get_count(struct udevice *dev, u64 *count) >>>>> @@ -42,6 +44,12 @@ static int dw_apb_timer_probe(struct udevice *dev) >>>>> struct clk clk; >>>>> int ret; >>>>> >>>>> + ret = reset_get_bulk(dev, &priv->resets); >>>>> + if (ret) >>>>> + dev_warn(dev, "Can't get reset: %d\n", ret); >>>> >>>> Shouldn't this be printed by the subsystem ? >>> >>> I don't think it's printed by the subsystem. Plus I don't know if it's >>> a warning for all drivers? I also haven't really thought about that, as >>> I just copied the reset handling code from another driver... >> >> Hmmmm, I guess we cannot operate without clock anyway, so it should be >> dev_err(). And I guess it's indeed not a subsystem print afterall. > > Well, for gen5, it would be dev_err. I'm not sure about arria10 and > stratix10 though? I think at least stratix10 doesn't have a clock driver, yet. But it does have reset driver, doesn't it ?
On Thu, Jul 25, 2019 at 1:08 PM Marek Vasut <marex@denx.de> wrote: > > On 7/24/19 8:09 PM, Simon Goldschmidt wrote: > > On Wed, Jul 24, 2019 at 9:48 AM Marek Vasut <marex@denx.de> wrote: > >> > >> On 7/24/19 9:43 AM, Simon Goldschmidt wrote: > >>> On Wed, Jul 24, 2019 at 9:31 AM Marek Vasut <marex@denx.de> wrote: > >>>> > >>>> On 7/23/19 10:27 PM, Simon Goldschmidt wrote: > >>>>> To use this timer on socfpga as system tick, it needs to take itself out > >>>>> of reset. > >>>>> > >>>>> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> > >>>>> --- > >>>>> > >>>>> drivers/timer/dw-apb-timer.c | 18 +++++++++++++++++- > >>>>> 1 file changed, 17 insertions(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/drivers/timer/dw-apb-timer.c b/drivers/timer/dw-apb-timer.c > >>>>> index 86312b8dc7..fad22be8c9 100644 > >>>>> --- a/drivers/timer/dw-apb-timer.c > >>>>> +++ b/drivers/timer/dw-apb-timer.c > >>>>> @@ -8,6 +8,7 @@ > >>>>> #include <common.h> > >>>>> #include <dm.h> > >>>>> #include <clk.h> > >>>>> +#include <reset.h> > >>>>> #include <timer.h> > >>>>> > >>>>> #include <asm/io.h> > >>>>> @@ -18,7 +19,8 @@ > >>>>> #define DW_APB_CTRL 0x8 > >>>>> > >>>>> struct dw_apb_timer_priv { > >>>>> - fdt_addr_t regs; > >>>>> + fdt_addr_t regs; > >>>>> + struct reset_ctl_bulk resets; > >>>>> }; > >>>>> > >>>>> static int dw_apb_timer_get_count(struct udevice *dev, u64 *count) > >>>>> @@ -42,6 +44,12 @@ static int dw_apb_timer_probe(struct udevice *dev) > >>>>> struct clk clk; > >>>>> int ret; > >>>>> > >>>>> + ret = reset_get_bulk(dev, &priv->resets); > >>>>> + if (ret) > >>>>> + dev_warn(dev, "Can't get reset: %d\n", ret); > >>>> > >>>> Shouldn't this be printed by the subsystem ? > >>> > >>> I don't think it's printed by the subsystem. Plus I don't know if it's > >>> a warning for all drivers? I also haven't really thought about that, as > >>> I just copied the reset handling code from another driver... > >> > >> Hmmmm, I guess we cannot operate without clock anyway, so it should be > >> dev_err(). And I guess it's indeed not a subsystem print afterall. > > > > Well, for gen5, it would be dev_err. I'm not sure about arria10 and > > stratix10 though? I think at least stratix10 doesn't have a clock driver, yet. > > But it does have reset driver, doesn't it ? Yes it does. You confused me with saying "we cannot operate without clock anyway", but this is about reset. So yes, you're right, dev_err() is better. Regards, Simon
diff --git a/drivers/timer/dw-apb-timer.c b/drivers/timer/dw-apb-timer.c index 86312b8dc7..fad22be8c9 100644 --- a/drivers/timer/dw-apb-timer.c +++ b/drivers/timer/dw-apb-timer.c @@ -8,6 +8,7 @@ #include <common.h> #include <dm.h> #include <clk.h> +#include <reset.h> #include <timer.h> #include <asm/io.h> @@ -18,7 +19,8 @@ #define DW_APB_CTRL 0x8 struct dw_apb_timer_priv { - fdt_addr_t regs; + fdt_addr_t regs; + struct reset_ctl_bulk resets; }; static int dw_apb_timer_get_count(struct udevice *dev, u64 *count) @@ -42,6 +44,12 @@ static int dw_apb_timer_probe(struct udevice *dev) struct clk clk; int ret; + ret = reset_get_bulk(dev, &priv->resets); + if (ret) + dev_warn(dev, "Can't get reset: %d\n", ret); + else + reset_deassert_bulk(&priv->resets); + ret = clk_get_by_index(dev, 0, &clk); if (ret) return ret; @@ -67,6 +75,13 @@ static int dw_apb_timer_ofdata_to_platdata(struct udevice *dev) return 0; } +static int dw_apb_timer_remove(struct udevice *dev) +{ + struct dw_apb_timer_priv *priv = dev_get_priv(dev); + + return reset_release_bulk(&priv->resets); +} + static const struct timer_ops dw_apb_timer_ops = { .get_count = dw_apb_timer_get_count, }; @@ -83,5 +98,6 @@ U_BOOT_DRIVER(dw_apb_timer) = { .probe = dw_apb_timer_probe, .of_match = dw_apb_timer_ids, .ofdata_to_platdata = dw_apb_timer_ofdata_to_platdata, + .remove = dw_apb_timer_remove, .priv_auto_alloc_size = sizeof(struct dw_apb_timer_priv), };
To use this timer on socfpga as system tick, it needs to take itself out of reset. Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com> --- drivers/timer/dw-apb-timer.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)