| Message ID | 1447672194-483-2-git-send-email-jszhang@marvell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 16.11.2015 12:09, Jisheng Zhang wrote: > The Marvell Berlin BG2Q has 3 watchdogs which are compatible with the > snps,dw-wdt driver sit in the sysmgr domain. This patch adds the > corresponding device tree nodes. > > Signed-off-by: Jisheng Zhang <jszhang@marvell.com> > --- > arch/arm/boot/dts/berlin2q.dtsi | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/arch/arm/boot/dts/berlin2q.dtsi b/arch/arm/boot/dts/berlin2q.dtsi > index a3ecde5..fac4315 100644 > --- a/arch/arm/boot/dts/berlin2q.dtsi > +++ b/arch/arm/boot/dts/berlin2q.dtsi > @@ -483,6 +483,30 @@ > ranges = <0 0xfc0000 0x10000>; > interrupt-parent = <&sic>; > > + wdt0: watchdog@1000 { > + compatible = "snps,dw-wdt"; > + reg = <0x1000 0x100>; > + clocks = <&refclk>; > + interrupts = <0>; > + status = "disabled"; Jisheng, as the watchdogs are internal and cannot be clock gated at all, how about we remove the status = "disabled" and make them always available? I have applied patches 1-4 with the status property removed. This also renders patches 5-8 useless. So, for now tentatively Appled to berlin/dt and berlin64/dt respectivly with status property removed. Sebastian
On Thu, 19 Nov 2015 21:47:05 +0100 Sebastian Hesselbarth wrote: > On 16.11.2015 12:09, Jisheng Zhang wrote: > > The Marvell Berlin BG2Q has 3 watchdogs which are compatible with the > > snps,dw-wdt driver sit in the sysmgr domain. This patch adds the > > corresponding device tree nodes. > > > > Signed-off-by: Jisheng Zhang <jszhang@marvell.com> > > --- > > arch/arm/boot/dts/berlin2q.dtsi | 24 ++++++++++++++++++++++++ > > 1 file changed, 24 insertions(+) > > > > diff --git a/arch/arm/boot/dts/berlin2q.dtsi b/arch/arm/boot/dts/berlin2q.dtsi > > index a3ecde5..fac4315 100644 > > --- a/arch/arm/boot/dts/berlin2q.dtsi > > +++ b/arch/arm/boot/dts/berlin2q.dtsi > > @@ -483,6 +483,30 @@ > > ranges = <0 0xfc0000 0x10000>; > > interrupt-parent = <&sic>; > > > > + wdt0: watchdog@1000 { > > + compatible = "snps,dw-wdt"; > > + reg = <0x1000 0x100>; > > + clocks = <&refclk>; > > + interrupts = <0>; > > + status = "disabled"; > > Jisheng, > > as the watchdogs are internal and cannot be clock gated > at all, how about we remove the status = "disabled" and > make them always available? there are two issues here: 1. the dw-wdt can't support multiple variants now. I have rewrite the driver with watchdog core supplied framework, but the patch isn't sent out and may be need time to clean up and review. 2. not all dw-wdt devices are available and functional. This depends on board design and configuration. So IMHO status=disabled and patch5-8 is necessary, what do you think? Thanks a lot for review, Jisheng > > I have applied patches 1-4 with the status property removed. > This also renders patches 5-8 useless. > > So, for now tentatively > > Appled to berlin/dt and berlin64/dt respectivly > > with status property removed. > > Sebastian >
On 20.11.2015 04:34, Jisheng Zhang wrote: > On Thu, 19 Nov 2015 21:47:05 +0100 > Sebastian Hesselbarth wrote: >> On 16.11.2015 12:09, Jisheng Zhang wrote: >>> The Marvell Berlin BG2Q has 3 watchdogs which are compatible with the >>> snps,dw-wdt driver sit in the sysmgr domain. This patch adds the >>> corresponding device tree nodes. >>> >>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com> >>> --- >>> arch/arm/boot/dts/berlin2q.dtsi | 24 ++++++++++++++++++++++++ >>> 1 file changed, 24 insertions(+) >>> >>> diff --git a/arch/arm/boot/dts/berlin2q.dtsi b/arch/arm/boot/dts/berlin2q.dtsi >>> index a3ecde5..fac4315 100644 >>> --- a/arch/arm/boot/dts/berlin2q.dtsi >>> +++ b/arch/arm/boot/dts/berlin2q.dtsi >>> @@ -483,6 +483,30 @@ >>> ranges = <0 0xfc0000 0x10000>; >>> interrupt-parent = <&sic>; >>> >>> + wdt0: watchdog@1000 { >>> + compatible = "snps,dw-wdt"; >>> + reg = <0x1000 0x100>; >>> + clocks = <&refclk>; >>> + interrupts = <0>; >>> + status = "disabled"; >> >> as the watchdogs are internal and cannot be clock gated >> at all, how about we remove the status = "disabled" and >> make them always available? > > there are two issues here: > > 1. the dw-wdt can't support multiple variants now. I have rewrite the driver > with watchdog core supplied framework, but the patch isn't sent out and > may be need time to clean up and review. Ok. > 2. not all dw-wdt devices are available and functional. This depends on > board design and configuration. I understand that "board design and configuration" may hinder the wdt to issue a hard reset. But all others are able to issue a soft reset or just an interrupt, right? So, I still don't see why we should disable wdt nodes by default except for the driver issue above. > So IMHO status=disabled and patch5-8 is necessary, what do you think? No. I'd agree to enable wdt0 by default and leave wdt[1,2] disabled because of the driver issue. Patches 5-8 only enable wdt0 anyway. As soon as the driver issue is resolved, we enable all wdt nodes unconditionally. Sebastian >> I have applied patches 1-4 with the status property removed. >> This also renders patches 5-8 useless. >> >> So, for now tentatively >> >> Appled to berlin/dt and berlin64/dt respectivly >> >> with status property removed. >> >> Sebastian >> >
On Fri, 20 Nov 2015 21:19:46 +0100 Sebastian Hesselbarth wrote: > On 20.11.2015 04:34, Jisheng Zhang wrote: > > On Thu, 19 Nov 2015 21:47:05 +0100 > > Sebastian Hesselbarth wrote: > >> On 16.11.2015 12:09, Jisheng Zhang wrote: > >>> The Marvell Berlin BG2Q has 3 watchdogs which are compatible with the > >>> snps,dw-wdt driver sit in the sysmgr domain. This patch adds the > >>> corresponding device tree nodes. > >>> > >>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com> > >>> --- > >>> arch/arm/boot/dts/berlin2q.dtsi | 24 ++++++++++++++++++++++++ > >>> 1 file changed, 24 insertions(+) > >>> > >>> diff --git a/arch/arm/boot/dts/berlin2q.dtsi b/arch/arm/boot/dts/berlin2q.dtsi > >>> index a3ecde5..fac4315 100644 > >>> --- a/arch/arm/boot/dts/berlin2q.dtsi > >>> +++ b/arch/arm/boot/dts/berlin2q.dtsi > >>> @@ -483,6 +483,30 @@ > >>> ranges = <0 0xfc0000 0x10000>; > >>> interrupt-parent = <&sic>; > >>> > >>> + wdt0: watchdog@1000 { > >>> + compatible = "snps,dw-wdt"; > >>> + reg = <0x1000 0x100>; > >>> + clocks = <&refclk>; > >>> + interrupts = <0>; > >>> + status = "disabled"; > >> > >> as the watchdogs are internal and cannot be clock gated > >> at all, how about we remove the status = "disabled" and > >> make them always available? > > > > there are two issues here: > > > > 1. the dw-wdt can't support multiple variants now. I have rewrite the driver > > with watchdog core supplied framework, but the patch isn't sent out and > > may be need time to clean up and review. > > Ok. > > > 2. not all dw-wdt devices are available and functional. This depends on > > board design and configuration. > > I understand that "board design and configuration" may hinder the wdt > to issue a hard reset. But all others are able to issue a soft reset > or just an interrupt, right? > > So, I still don't see why we should disable wdt nodes by default > except for the driver issue above. > > > So IMHO status=disabled and patch5-8 is necessary, what do you think? > > No. I'd agree to enable wdt0 by default and leave wdt[1,2] disabled > because of the driver issue. Patches 5-8 only enable wdt0 anyway. That's fine. > > As soon as the driver issue is resolved, we enable all wdt nodes > unconditionally. I will submit patch for the wdt driver and hope it will be merged in v4.5. Thanks, Jisheng > > Sebastian > > >> I have applied patches 1-4 with the status property removed. > >> This also renders patches 5-8 useless. > >> > >> So, for now tentatively > >> > >> Appled to berlin/dt and berlin64/dt respectivly > >> > >> with status property removed. > >> > >> Sebastian > >> > > >
On 23.11.2015 05:59, Jisheng Zhang wrote: > On Fri, 20 Nov 2015 21:19:46 +0100 > Sebastian Hesselbarth wrote: >> On 20.11.2015 04:34, Jisheng Zhang wrote: >>> On Thu, 19 Nov 2015 21:47:05 +0100 >>> Sebastian Hesselbarth wrote: >>>> On 16.11.2015 12:09, Jisheng Zhang wrote: >>>>> The Marvell Berlin BG2Q has 3 watchdogs which are compatible with the >>>>> snps,dw-wdt driver sit in the sysmgr domain. This patch adds the >>>>> corresponding device tree nodes. >>>>> >>>>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com> >>>>> --- [...] >>>>> + wdt0: watchdog@1000 { >>>>> + compatible = "snps,dw-wdt"; >>>>> + reg = <0x1000 0x100>; >>>>> + clocks = <&refclk>; >>>>> + interrupts = <0>; >>>>> + status = "disabled"; >>>> >>>> as the watchdogs are internal and cannot be clock gated >>>> at all, how about we remove the status = "disabled" and >>>> make them always available? >>> >>> there are two issues here: >>> >>> 1. the dw-wdt can't support multiple variants now. I have rewrite the driver >>> with watchdog core supplied framework, but the patch isn't sent out and >>> may be need time to clean up and review. >> >> Ok. >> >>> 2. not all dw-wdt devices are available and functional. This depends on >>> board design and configuration. >> >> I understand that "board design and configuration" may hinder the wdt >> to issue a hard reset. But all others are able to issue a soft reset >> or just an interrupt, right? >> >> So, I still don't see why we should disable wdt nodes by default >> except for the driver issue above. >> >>> So IMHO status=disabled and patch5-8 is necessary, what do you think? >> >> No. I'd agree to enable wdt0 by default and leave wdt[1,2] disabled >> because of the driver issue. Patches 5-8 only enable wdt0 anyway. > > That's fine. Jisheng, I amended your SoC dtsi watchdog patches accordingly. wdt0 is now always enabled, while the others are disabled. So, with the changes Patches 1-4 applied to berlin/dt and berlin64/dt respectively. Patches 5-8 dropped. >> As soon as the driver issue is resolved, we enable all wdt nodes >> unconditionally. > > I will submit patch for the wdt driver and hope it will be merged > in v4.5. Ok. Feel free to add a patch that removes the status disabled properties again if berlin[64]/dt has already hit mainline in the meantime. If not, keep me posted on the DW wdt patch outcome. Thanks, Sebastian
On Sat, 28 Nov 2015 12:36:16 +0100 Sebastian Hesselbarth wrote: > On 23.11.2015 05:59, Jisheng Zhang wrote: > > On Fri, 20 Nov 2015 21:19:46 +0100 > > Sebastian Hesselbarth wrote: > >> On 20.11.2015 04:34, Jisheng Zhang wrote: > >>> On Thu, 19 Nov 2015 21:47:05 +0100 > >>> Sebastian Hesselbarth wrote: > >>>> On 16.11.2015 12:09, Jisheng Zhang wrote: > >>>>> The Marvell Berlin BG2Q has 3 watchdogs which are compatible with the > >>>>> snps,dw-wdt driver sit in the sysmgr domain. This patch adds the > >>>>> corresponding device tree nodes. > >>>>> > >>>>> Signed-off-by: Jisheng Zhang <jszhang@marvell.com> > >>>>> --- > [...] > >>>>> + wdt0: watchdog@1000 { > >>>>> + compatible = "snps,dw-wdt"; > >>>>> + reg = <0x1000 0x100>; > >>>>> + clocks = <&refclk>; > >>>>> + interrupts = <0>; > >>>>> + status = "disabled"; > >>>> > >>>> as the watchdogs are internal and cannot be clock gated > >>>> at all, how about we remove the status = "disabled" and > >>>> make them always available? > >>> > >>> there are two issues here: > >>> > >>> 1. the dw-wdt can't support multiple variants now. I have rewrite the driver > >>> with watchdog core supplied framework, but the patch isn't sent out and > >>> may be need time to clean up and review. > >> > >> Ok. > >> > >>> 2. not all dw-wdt devices are available and functional. This depends on > >>> board design and configuration. > >> > >> I understand that "board design and configuration" may hinder the wdt > >> to issue a hard reset. But all others are able to issue a soft reset > >> or just an interrupt, right? > >> > >> So, I still don't see why we should disable wdt nodes by default > >> except for the driver issue above. > >> > >>> So IMHO status=disabled and patch5-8 is necessary, what do you think? > >> > >> No. I'd agree to enable wdt0 by default and leave wdt[1,2] disabled > >> because of the driver issue. Patches 5-8 only enable wdt0 anyway. > > > > That's fine. > > Jisheng, > > I amended your SoC dtsi watchdog patches accordingly. wdt0 is > now always enabled, while the others are disabled. > > So, with the changes Patches 1-4 applied to berlin/dt and berlin64/dt > respectively. Patches 5-8 dropped. > > >> As soon as the driver issue is resolved, we enable all wdt nodes > >> unconditionally. > > > > I will submit patch for the wdt driver and hope it will be merged > > in v4.5. > > Ok. Feel free to add a patch that removes the status disabled properties > again if berlin[64]/dt has already hit mainline in the meantime. Got it. No problems. > > If not, keep me posted on the DW wdt patch outcome. Will do. These days, I'm focusing on clk patches and some trivial dw-apb-timer performance improvement patches. I will loop you in the mail when send out dw wdt driver rewrite patch. Thanks a lot, Jisheng
diff --git a/arch/arm/boot/dts/berlin2q.dtsi b/arch/arm/boot/dts/berlin2q.dtsi index a3ecde5..fac4315 100644 --- a/arch/arm/boot/dts/berlin2q.dtsi +++ b/arch/arm/boot/dts/berlin2q.dtsi @@ -483,6 +483,30 @@ ranges = <0 0xfc0000 0x10000>; interrupt-parent = <&sic>; + wdt0: watchdog@1000 { + compatible = "snps,dw-wdt"; + reg = <0x1000 0x100>; + clocks = <&refclk>; + interrupts = <0>; + status = "disabled"; + }; + + wdt1: watchdog@2000 { + compatible = "snps,dw-wdt"; + reg = <0x2000 0x100>; + clocks = <&refclk>; + interrupts = <1>; + status = "disabled"; + }; + + wdt2: watchdog@3000 { + compatible = "snps,dw-wdt"; + reg = <0x3000 0x100>; + clocks = <&refclk>; + interrupts = <2>; + status = "disabled"; + }; + sm_gpio1: gpio@5000 { compatible = "snps,dw-apb-gpio"; reg = <0x5000 0x400>;
The Marvell Berlin BG2Q has 3 watchdogs which are compatible with the snps,dw-wdt driver sit in the sysmgr domain. This patch adds the corresponding device tree nodes. Signed-off-by: Jisheng Zhang <jszhang@marvell.com> --- arch/arm/boot/dts/berlin2q.dtsi | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)