Message ID | 20190423022507.34969-1-andy.tang@nxp.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v6] arm64: dts: ls1088a: add one more thermal zone node | expand |
On Tue, Apr 23, 2019 at 10:25:07AM +0800, Yuantian Tang wrote: > Ls1088a has 2 thermal sensors, core cluster and SoC platform. Core cluster > sensor is used to monitor the temperature of core and SoC platform is for > platform. The current dts only support the first sensor. > This patch adds the second sensor node to dts to enable it. > > Signed-off-by: Yuantian Tang <andy.tang@nxp.com> > --- > v6: > - add cooling device map to cpu0-7 in platform node. @Daniel, are you fine with this version? Shawn
> -----Original Message----- > From: Shawn Guo <shawnguo@kernel.org> > Sent: 2019年5月10日 11:14 > To: Andy Tang <andy.tang@nxp.com> > Cc: Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; > mark.rutland@arm.com; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > linux-pm@vger.kernel.org; daniel.lezcano@linaro.org; rui.zhang@intel.com; > edubezval@gmail.com > Subject: [EXT] Re: [PATCH v6] arm64: dts: ls1088a: add one more thermal > zone node > > Caution: EXT Email > > On Tue, Apr 23, 2019 at 10:25:07AM +0800, Yuantian Tang wrote: > > Ls1088a has 2 thermal sensors, core cluster and SoC platform. Core > > cluster sensor is used to monitor the temperature of core and SoC > > platform is for platform. The current dts only support the first sensor. > > This patch adds the second sensor node to dts to enable it. > > > > Signed-off-by: Yuantian Tang <andy.tang@nxp.com> > > --- > > v6: > > - add cooling device map to cpu0-7 in platform node. I like to explain a little. I think it makes sense that multiple thermal zone map to same cooling device. In this way, no matter which thermal zone raises a temp alarm, it can call cooling device to chill out. I also asked cpufreq maintainer about the cooling map issue, he think it would be fine. I have tested and no issue found. Daniel, what's your thought? Thanks, Andy > > @Daniel, are you fine with this version? > > Shawn WARNING: This email was created outside of NXP. DO NOT CLICK links or attachments unless you recognize the sender and know the content is safe. On 22-04-19, 07:09, Andy Tang wrote: > Hi Viresh, > > Sorry to bother you. I have a question, hope I can get you help. > Here it is: > > I want to add multiple "Thermal Zone" support in dts ( driver is ready). > The final dts looks like below: > > thermal-zones { > cpu_thermal: cpu-thermal { > polling-delay-passive = <1000>; > polling-delay = <5000>; > thermal-sensors = <&tmu 0>; > > trips { > ccu_alert: ccu-alert { > temperature = <85000>; > hysteresis = <2000>; > type = "passive"; > }; > ccu_crit: ccu-crit { > temperature = <95000>; > hysteresis = <2000>; > type = "critical"; > cooling-maps { > map0 { > trip = <&ccu_alert>; > cooling-device = > <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > <&cpu4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > <&cpu5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > <&cpu6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > <&cpu7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; > }; > }; > }; > platform { > polling-delay-passive = <1000>; > polling-delay = <5000>; > thermal-sensors = <&tmu 1>; > trips { > plt_alert: plt-alert { > temperature = <85000>; > hysteresis = <2000>; > type = "passive"; > }; > plt_crit: plt-crit { > temperature = <95000>; > hysteresis = <2000>; > type = "critical"; > }; > }; > cooling-maps { > map0 { > trip = <&plt_alert>; > cooling-device = > <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > <&cpu4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > <&cpu5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > <&cpu6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, > <&cpu7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; > } > } > > Here we have 2 thermal zones, but both map cpu0-7 as cooling device. I have tested and didn't find any issues. > My query is: is it allowed to map same device as "cooling device" to more than one "thermal zone"? That should be fine IMO. -- viresh
On 10/05/2019 05:40, Andy Tang wrote: >> -----Original Message----- >> From: Shawn Guo <shawnguo@kernel.org> >> Sent: 2019年5月10日 11:14 >> To: Andy Tang <andy.tang@nxp.com> >> Cc: Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; >> mark.rutland@arm.com; linux-arm-kernel@lists.infradead.org; >> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; >> linux-pm@vger.kernel.org; daniel.lezcano@linaro.org; rui.zhang@intel.com; >> edubezval@gmail.com >> Subject: [EXT] Re: [PATCH v6] arm64: dts: ls1088a: add one more thermal >> zone node >> >> Caution: EXT Email >> >> On Tue, Apr 23, 2019 at 10:25:07AM +0800, Yuantian Tang wrote: >>> Ls1088a has 2 thermal sensors, core cluster and SoC platform. Core >>> cluster sensor is used to monitor the temperature of core and SoC >>> platform is for platform. The current dts only support the first sensor. >>> This patch adds the second sensor node to dts to enable it. >>> >>> Signed-off-by: Yuantian Tang <andy.tang@nxp.com> >>> --- >>> v6: >>> - add cooling device map to cpu0-7 in platform node. > I like to explain a little. I think it makes sense that multiple thermal zone map to same cooling device. > In this way, no matter which thermal zone raises a temp alarm, it can call cooling device to chill out. > I also asked cpufreq maintainer about the cooling map issue, he think it would be fine. > I have tested and no issue found. > > Daniel, what's your thought? If there are multiple thermal zones, they will be managed by different instances of a thermal governor. Each instances will act on the shared cooling device and will collide in their decisions: - If the sensors are closed, their behavior will be similar regarding the temperature. The governors may take the same decision for the cooling device. But in such case having just one thermal zone managed is enough. - If the sensors are not closed, their behavior will be different regarding the temperature. The governors will take different decision regarding the cooling device (one will decrease the freq, other will increase the freq). As the thermal governors are not able to manage several thermal zones and there is one cooling device (the cpu cooling device), this setup won't work as expected IMO. The setup making sense is having a thermal zone per 'cluster' and a cooling device per 'cluster'. That means the platform has one clock line per 'cluster'. The thermal management happens in a self-contained thermal zone (one cooling device - one governor - one thermal zone). In the case of HMP, other combinations are possible to be optimal.
+ Viresh for help. > -----Original Message----- > From: Daniel Lezcano <daniel.lezcano@linaro.org> > Sent: 2019年5月10日 15:17 > To: Andy Tang <andy.tang@nxp.com>; Shawn Guo <shawnguo@kernel.org> > Cc: Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; > mark.rutland@arm.com; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > linux-pm@vger.kernel.org; rui.zhang@intel.com; edubezval@gmail.com > Subject: Re: [EXT] Re: [PATCH v6] arm64: dts: ls1088a: add one more thermal > zone node > > Caution: EXT Email > > On 10/05/2019 05:40, Andy Tang wrote: > >> -----Original Message----- > >> From: Shawn Guo <shawnguo@kernel.org> > >> Sent: 2019年5月10日 11:14 > >> To: Andy Tang <andy.tang@nxp.com> > >> Cc: Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; > >> mark.rutland@arm.com; linux-arm-kernel@lists.infradead.org; > >> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > >> linux-pm@vger.kernel.org; daniel.lezcano@linaro.org; > >> rui.zhang@intel.com; edubezval@gmail.com > >> Subject: [EXT] Re: [PATCH v6] arm64: dts: ls1088a: add one more > >> thermal zone node > >> > >> Caution: EXT Email > >> > >> On Tue, Apr 23, 2019 at 10:25:07AM +0800, Yuantian Tang wrote: > >>> Ls1088a has 2 thermal sensors, core cluster and SoC platform. Core > >>> cluster sensor is used to monitor the temperature of core and SoC > >>> platform is for platform. The current dts only support the first sensor. > >>> This patch adds the second sensor node to dts to enable it. > >>> > >>> Signed-off-by: Yuantian Tang <andy.tang@nxp.com> > >>> --- > >>> v6: > >>> - add cooling device map to cpu0-7 in platform node. > > I like to explain a little. I think it makes sense that multiple thermal zone > map to same cooling device. > > In this way, no matter which thermal zone raises a temp alarm, it can call > cooling device to chill out. > > I also asked cpufreq maintainer about the cooling map issue, he think it > would be fine. > > I have tested and no issue found. > > > > Daniel, what's your thought? > > If there are multiple thermal zones, they will be managed by different > instances of a thermal governor. Each instances will act on the shared cooling > device and will collide in their decisions: > > - If the sensors are closed, their behavior will be similar regarding the > temperature. The governors may take the same decision for the cooling > device. But in such case having just one thermal zone managed is enough. > > - If the sensors are not closed, their behavior will be different regarding the > temperature. The governors will take different decision regarding the cooling > device (one will decrease the freq, other will increase the freq). > > As the thermal governors are not able to manage several thermal zones and > there is one cooling device (the cpu cooling device), this setup won't work as > expected IMO. > > The setup making sense is having a thermal zone per 'cluster' and a cooling > device per 'cluster'. That means the platform has one clock line per 'cluster'. > The thermal management happens in a self-contained thermal zone (one > cooling device - one governor - one thermal zone). > > In the case of HMP, other combinations are possible to be optimal. Hi Viresh, I want to map multiple thermal zones to the same cooling device. The above is the discussion about it. It seems reasonable. But I am not expert on this. Could you please provide some thoughts? Thanks. BR, Andy > > > > -- > > <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww. > linaro.org%2F&data=02%7C01%7Candy.tang%40nxp.com%7C935b7a08 > 31cc466da40808d6d5176f8e%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0 > %7C0%7C636930693965540189&sdata=WK9NDBVwzhCC9WMkmjLObJ > OBQwRG%2Fboed%2FKx18xiBNQ%3D&reserved=0> Linaro.org │ Open > source software for ARM SoCs > > Follow Linaro: > <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww. > facebook.com%2Fpages%2FLinaro&data=02%7C01%7Candy.tang%40nx > p.com%7C935b7a0831cc466da40808d6d5176f8e%7C686ea1d3bc2b4c6fa92c > d99c5c301635%7C0%7C0%7C636930693965550202&sdata=O0mWO76 > 9sK2ZMGX9AxgLGYNVfkFHBD4ZIGCclttvyPI%3D&reserved=0> > Facebook | > <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Ftwitte > r.com%2F%23!%2Flinaroorg&data=02%7C01%7Candy.tang%40nxp.com > %7C935b7a0831cc466da40808d6d5176f8e%7C686ea1d3bc2b4c6fa92cd99c5 > c301635%7C0%7C0%7C636930693965550202&sdata=tV%2Bi8Bk3OqO > h%2FZHpBr2NQDACVvtGi8KNGQt5dZaTeyg%3D&reserved=0> Twitter | > <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww. > linaro.org%2Flinaro-blog%2F&data=02%7C01%7Candy.tang%40nxp.co > m%7C935b7a0831cc466da40808d6d5176f8e%7C686ea1d3bc2b4c6fa92cd99 > c5c301635%7C0%7C0%7C636930693965550202&sdata=0E8a938xEt7l2 > MBLnMETsCQKfhJMgmzFNtuCKPXf5SY%3D&reserved=0> Blog
On 10-05-19, 08:47, Andy Tang wrote: > + Viresh for help. > > > -----Original Message----- > > From: Daniel Lezcano <daniel.lezcano@linaro.org> > > Sent: 2019年5月10日 15:17 > > To: Andy Tang <andy.tang@nxp.com>; Shawn Guo <shawnguo@kernel.org> > > Cc: Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; > > mark.rutland@arm.com; linux-arm-kernel@lists.infradead.org; > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > > linux-pm@vger.kernel.org; rui.zhang@intel.com; edubezval@gmail.com > > Subject: Re: [EXT] Re: [PATCH v6] arm64: dts: ls1088a: add one more thermal > > zone node > > > > Caution: EXT Email > > > > On 10/05/2019 05:40, Andy Tang wrote: > > >> -----Original Message----- > > >> From: Shawn Guo <shawnguo@kernel.org> > > >> Sent: 2019年5月10日 11:14 > > >> To: Andy Tang <andy.tang@nxp.com> > > >> Cc: Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; > > >> mark.rutland@arm.com; linux-arm-kernel@lists.infradead.org; > > >> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > > >> linux-pm@vger.kernel.org; daniel.lezcano@linaro.org; > > >> rui.zhang@intel.com; edubezval@gmail.com > > >> Subject: [EXT] Re: [PATCH v6] arm64: dts: ls1088a: add one more > > >> thermal zone node > > >> > > >> Caution: EXT Email > > >> > > >> On Tue, Apr 23, 2019 at 10:25:07AM +0800, Yuantian Tang wrote: > > >>> Ls1088a has 2 thermal sensors, core cluster and SoC platform. Core > > >>> cluster sensor is used to monitor the temperature of core and SoC > > >>> platform is for platform. The current dts only support the first sensor. > > >>> This patch adds the second sensor node to dts to enable it. > > >>> > > >>> Signed-off-by: Yuantian Tang <andy.tang@nxp.com> > > >>> --- > > >>> v6: > > >>> - add cooling device map to cpu0-7 in platform node. > > > I like to explain a little. I think it makes sense that multiple thermal zone > > map to same cooling device. > > > In this way, no matter which thermal zone raises a temp alarm, it can call > > cooling device to chill out. > > > I also asked cpufreq maintainer about the cooling map issue, he think it > > would be fine. Yes, you asked me and I said it should be okay. > > > I have tested and no issue found. > > > > > > Daniel, what's your thought? > > > > If there are multiple thermal zones, they will be managed by different > > instances of a thermal governor. Each instances will act on the shared cooling > > device and will collide in their decisions: > > > > - If the sensors are closed, their behavior will be similar regarding the > > temperature. The governors may take the same decision for the cooling > > device. But in such case having just one thermal zone managed is enough. > > > > - If the sensors are not closed, their behavior will be different regarding the > > temperature. The governors will take different decision regarding the cooling > > device (one will decrease the freq, other will increase the freq). > > > > As the thermal governors are not able to manage several thermal zones and > > there is one cooling device (the cpu cooling device), this setup won't work as > > expected IMO. > > > > The setup making sense is having a thermal zone per 'cluster' and a cooling > > device per 'cluster'. That means the platform has one clock line per 'cluster'. > > The thermal management happens in a self-contained thermal zone (one > > cooling device - one governor - one thermal zone). > > > > In the case of HMP, other combinations are possible to be optimal. But not sure how I missed the obvious, though I do remember thinking about this. So the problem is that the cpu_cooling driver will get requests in parallel to set different max frequencies and the last call will always win and may result in undesired outcome. Sorry about creating the confusion.
Thanks Viresh for your explanation. BR, Andy > -----Original Message----- > From: Viresh Kumar <viresh.kumar@linaro.org> > Sent: 2019年5月10日 18:12 > To: Andy Tang <andy.tang@nxp.com> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org>; Shawn Guo > <shawnguo@kernel.org>; Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; > mark.rutland@arm.com; linux-arm-kernel@lists.infradead.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > linux-pm@vger.kernel.org; rui.zhang@intel.com; edubezval@gmail.com > Subject: Re: [EXT] Re: [PATCH v6] arm64: dts: ls1088a: add one more thermal > zone node > > Caution: EXT Email > > On 10-05-19, 08:47, Andy Tang wrote: > > + Viresh for help. > > > > > -----Original Message----- > > > From: Daniel Lezcano <daniel.lezcano@linaro.org> > > > Sent: 2019年5月10日 15:17 > > > To: Andy Tang <andy.tang@nxp.com>; Shawn Guo > <shawnguo@kernel.org> > > > Cc: Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; > > > mark.rutland@arm.com; linux-arm-kernel@lists.infradead.org; > > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > > > linux-pm@vger.kernel.org; rui.zhang@intel.com; edubezval@gmail.com > > > Subject: Re: [EXT] Re: [PATCH v6] arm64: dts: ls1088a: add one more > > > thermal zone node > > > > > > Caution: EXT Email > > > > > > On 10/05/2019 05:40, Andy Tang wrote: > > > >> -----Original Message----- > > > >> From: Shawn Guo <shawnguo@kernel.org> > > > >> Sent: 2019年5月10日 11:14 > > > >> To: Andy Tang <andy.tang@nxp.com> > > > >> Cc: Leo Li <leoyang.li@nxp.com>; robh+dt@kernel.org; > > > >> mark.rutland@arm.com; linux-arm-kernel@lists.infradead.org; > > > >> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > > > >> linux-pm@vger.kernel.org; daniel.lezcano@linaro.org; > > > >> rui.zhang@intel.com; edubezval@gmail.com > > > >> Subject: [EXT] Re: [PATCH v6] arm64: dts: ls1088a: add one more > > > >> thermal zone node > > > >> > > > >> Caution: EXT Email > > > >> > > > >> On Tue, Apr 23, 2019 at 10:25:07AM +0800, Yuantian Tang wrote: > > > >>> Ls1088a has 2 thermal sensors, core cluster and SoC platform. > > > >>> Core cluster sensor is used to monitor the temperature of core > > > >>> and SoC platform is for platform. The current dts only support the first > sensor. > > > >>> This patch adds the second sensor node to dts to enable it. > > > >>> > > > >>> Signed-off-by: Yuantian Tang <andy.tang@nxp.com> > > > >>> --- > > > >>> v6: > > > >>> - add cooling device map to cpu0-7 in platform node. > > > > I like to explain a little. I think it makes sense that multiple > > > > thermal zone > > > map to same cooling device. > > > > In this way, no matter which thermal zone raises a temp alarm, it > > > > can call > > > cooling device to chill out. > > > > I also asked cpufreq maintainer about the cooling map issue, he > > > > think it > > > would be fine. > > Yes, you asked me and I said it should be okay. > > > > > I have tested and no issue found. > > > > > > > > Daniel, what's your thought? > > > > > > If there are multiple thermal zones, they will be managed by > > > different instances of a thermal governor. Each instances will act > > > on the shared cooling device and will collide in their decisions: > > > > > > - If the sensors are closed, their behavior will be similar > > > regarding the temperature. The governors may take the same decision > > > for the cooling device. But in such case having just one thermal zone > managed is enough. > > > > > > - If the sensors are not closed, their behavior will be different > > > regarding the temperature. The governors will take different > > > decision regarding the cooling device (one will decrease the freq, other > will increase the freq). > > > > > > As the thermal governors are not able to manage several thermal > > > zones and there is one cooling device (the cpu cooling device), this > > > setup won't work as expected IMO. > > > > > > The setup making sense is having a thermal zone per 'cluster' and a > > > cooling device per 'cluster'. That means the platform has one clock line > per 'cluster'. > > > The thermal management happens in a self-contained thermal zone (one > > > cooling device - one governor - one thermal zone). > > > > > > In the case of HMP, other combinations are possible to be optimal. > > But not sure how I missed the obvious, though I do remember thinking about > this. > > So the problem is that the cpu_cooling driver will get requests in parallel to > set different max frequencies and the last call will always win and may result > in undesired outcome. > > Sorry about creating the confusion. > > -- > viresh
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi index 661137f..a697a82 100644 --- a/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi +++ b/arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi @@ -129,19 +129,19 @@ }; thermal-zones { - cpu_thermal: cpu-thermal { + core-cluster { polling-delay-passive = <1000>; polling-delay = <5000>; thermal-sensors = <&tmu 0>; trips { - cpu_alert: cpu-alert { + core_cluster_alert: core-cluster-alert { temperature = <85000>; hysteresis = <2000>; type = "passive"; }; - cpu_crit: cpu-crit { + core_cluster_crit: core-cluster-crit { temperature = <95000>; hysteresis = <2000>; type = "critical"; @@ -150,7 +150,42 @@ cooling-maps { map0 { - trip = <&cpu_alert>; + trip = <&core_cluster_alert>; + cooling-device = + <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu4 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu5 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, + <&cpu7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>; + }; + }; + }; + + platform { + polling-delay-passive = <1000>; + polling-delay = <5000>; + thermal-sensors = <&tmu 1>; + + trips { + platform_alert: platform-alert { + temperature = <85000>; + hysteresis = <2000>; + type = "passive"; + }; + + platform_crit: platform-crit { + temperature = <95000>; + hysteresis = <2000>; + type = "critical"; + }; + }; + + cooling-maps { + map0 { + trip = <&platform_alert>; cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>, <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
Ls1088a has 2 thermal sensors, core cluster and SoC platform. Core cluster sensor is used to monitor the temperature of core and SoC platform is for platform. The current dts only support the first sensor. This patch adds the second sensor node to dts to enable it. Signed-off-by: Yuantian Tang <andy.tang@nxp.com> --- v6: - add cooling device map to cpu0-7 in platform node. arch/arm64/boot/dts/freescale/fsl-ls1088a.dtsi | 43 +++++++++++++++++++++-- 1 files changed, 39 insertions(+), 4 deletions(-)