Message ID | fcb88cd21f31a467d2d49911c2505082837f72ea.1410323179.git.viresh.kumar@linaro.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Dear Viresh Kumar, On Wed, 10 Sep 2014 09:59:07 +0530, Viresh Kumar wrote: > Yesterday again I was pinged by Thomas about the status of this work and the > thread isn't going anywhere. He is blocked with just this to get his platform > running on mainline. > > Will it be possible to get this through as a temporary solution? A compile time conditional does not work, as it isn't compatible with multiplatform: what if in the same kernel you want to support two platforms that use cpufreq-dt, one having a single clock for all CPUs, and one having one clock per CPU. I'm still confused as to what is the problem with the DT binding surrounding this driver. Isn't it simply a matter of adding a clocks = <...> property to all CPUs, and that's it? Viresh told me there was an issue related to comparing the clocks in the driver to find out which CPUs share which clocks. But that is not a DT binding issue, it's an implementation detail. Since the driver is purely for DT platforms (as its name suggest), comparing phandles could work as a temporary solution. And since it's just an implementation detail that doesn't affect the DT binding, we could change the implementation later to use a proper clock API like clk_is_same(clk1, clk2) that would tell us if the clocks are the same. But I have probably missed some of the complexity of this. Thanks, Thomas
On 10 September 2014 11:59, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > A compile time conditional does not work, as it isn't compatible with > multiplatform: what if in the same kernel you want to support two > platforms that use cpufreq-dt, one having a single clock for all CPUs, > and one having one clock per CPU. Yeah, that's a known problem. Should have mentioned this in logs. > I'm still confused as to what is the problem with the DT binding > surrounding this driver. Isn't it simply a matter of adding a clocks = > <...> property to all CPUs, and that's it? > > Viresh told me there was an issue related to comparing the clocks in > the driver to find out which CPUs share which clocks. But that is not a > DT binding issue, it's an implementation detail. Since the driver is > purely for DT platforms (as its name suggest), comparing phandles could > work as a temporary solution. And since it's just an implementation > detail that doesn't affect the DT binding, we could change the > implementation later to use a proper clock API like clk_is_same(clk1, > clk2) that would tell us if the clocks are the same. > > But I have probably missed some of the complexity of this. (Couldn't give you these links yesterday as I was done for the day and was replying from my phone (In our private chat)) https://lkml.org/lkml/2014/6/26/577 https://lkml.org/lkml/2014/7/1/565 https://lkml.org/lkml/2014/7/3/813 http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/275520.html So these are few mails I could pick, specially from Mike, where he pointed out why getting a clk-API for this is a bad idea. -- viresh -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, Here is a proposal to solve the cpufreq-dt issue. We don't seem to be anywhere near reaching an agreement on a DT binding to represent the topology of CPU clocks in a system, because it's a complicated matter. Mike doesn't want to add a clock API that would allow the generic cpufreq driver to find out which CPUs share clocks, as he believes this is part of the hardware description and should therefore be described in the DT rather than "guessed" dynamically at boot time by looking at the clocks referenced by each CPU. And with Viresh not accepting any machine specific driver, it results in platforms like Armada XP having no solution to support cpufreq... So this proposal consists in adding a platform_data flag for the cpufreq-dt driver, which allows platform code to tell whether CPU clocks are shared or are independent. If you don't like platform_data, we can also register two different platform_driver for the two different cases, simply with different names. Another approach would be to lift the ban on machine-specific cpufreq drivers, since the generic driver is not capable of handling all situations. Thomas Thomas Petazzoni (4): cpufreq: allow driver-specific flags cpufreq: cpufreq-dt: extend with platform_data ARM: mvebu: use the cpufreq-dt platform_data for independent clocks cpufreq: cpufreq-dt: remove warning about regulators arch/arm/mach-mvebu/pmsu.c | 8 +++++++- drivers/cpufreq/cpufreq-dt.c | 23 ++++++++++++++++++----- drivers/cpufreq/cpufreq.c | 15 +++++++++++++++ include/linux/cpufreq-dt.h | 22 ++++++++++++++++++++++ include/linux/cpufreq.h | 5 ++++- 5 files changed, 66 insertions(+), 7 deletions(-) create mode 100644 include/linux/cpufreq-dt.h
On Wednesday 10 September 2014 11:41:48 Thomas Petazzoni wrote: > We don't seem to be anywhere near reaching an agreement on a DT > binding to represent the topology of CPU clocks in a system, because > it's a complicated matter. Mike doesn't want to add a clock API that > would allow the generic cpufreq driver to find out which CPUs share > clocks, as he believes this is part of the hardware description and > should therefore be described in the DT rather than "guessed" > dynamically at boot time by looking at the clocks referenced by each > CPU. And with Viresh not accepting any machine specific driver, it > results in platforms like Armada XP having no solution to support > cpufreq... > > So this proposal consists in adding a platform_data flag for the > cpufreq-dt driver, which allows platform code to tell whether CPU > clocks are shared or are independent. > > If you don't like platform_data, we can also register two different > platform_driver for the two different cases, simply with different > names. > > Another approach would be to lift the ban on machine-specific cpufreq > drivers, since the generic driver is not capable of handling all > situations. Using platform_data works nicely if you register the cpufreq device from the platform code, but that is a particular thing that has been bugging me for a long time and that I really want to get rid of: We don't want to introduce platform-specific files for ARM64, so we have to solve this anyway by just looking at DT, and whatever solution we end up with should work for both ARM32 and ARM64. Most of us will be at LCU next week, so I'd suggest we solve this problem using the 'lock everyone into one room without beer until we come up with a working approach' method. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Arnd, On Wed, 10 Sep 2014 11:53:27 +0200, Arnd Bergmann wrote: > > Another approach would be to lift the ban on machine-specific cpufreq > > drivers, since the generic driver is not capable of handling all > > situations. > > Using platform_data works nicely if you register the cpufreq device > from the platform code, but that is a particular thing that has > been bugging me for a long time and that I really want to get rid > of: We don't want to introduce platform-specific files for ARM64, > so we have to solve this anyway by just looking at DT, and whatever > solution we end up with should work for both ARM32 and ARM64. Right. Note that there is a similar issue with cpuidle. Maybe each cpufreq/cpuidle driver should simply list of top-level compatible strings it is compatible with, so that at boot time, each cpufreq/cpuidle driver gets a chance to init itself and see whether it is matching the currently running platform. > Most of us will be at LCU next week, so I'd suggest we solve this > problem using the 'lock everyone into one room without beer until > we come up with a working approach' method. That does seem like an interesting idea. However, I initially wanted to push cpufreq support for Armada XP in 3.17, and everything was pushed *except* the cpufreq driver bits. So I was hoping to at least get things working for 3.18. If the discussion takes place next week, and then the time to finally implement something, we'll also miss 3.18. In the mean time, would it be possible to realize that the existing generic cpufreq driver simply isn't generic enough, and that we should accept machine-specific cpufreq drivers for the time being? Thanks, Thomas
On Wednesday 10 September 2014 12:10:02 Thomas Petazzoni wrote: > On Wed, 10 Sep 2014 11:53:27 +0200, Arnd Bergmann wrote: > > > > Another approach would be to lift the ban on machine-specific cpufreq > > > drivers, since the generic driver is not capable of handling all > > > situations. > > > > Using platform_data works nicely if you register the cpufreq device > > from the platform code, but that is a particular thing that has > > been bugging me for a long time and that I really want to get rid > > of: We don't want to introduce platform-specific files for ARM64, > > so we have to solve this anyway by just looking at DT, and whatever > > solution we end up with should work for both ARM32 and ARM64. > > Right. Note that there is a similar issue with cpuidle. > > Maybe each cpufreq/cpuidle driver should simply list of top-level > compatible strings it is compatible with, so that at boot time, each > cpufreq/cpuidle driver gets a chance to init itself and see whether it > is matching the currently running platform. The main problem I see with that is that it requires updating the driver for every new platform we want to support. It would be better to find a way that lets you add new platforms using just new dts files that work with the existing driver. This is the main reason why we try to never match the root compatible property in any driver, and I don't think cpufreq should be an exception to this just because it can't easily be mapped to a particular register set. > > Most of us will be at LCU next week, so I'd suggest we solve this > > problem using the 'lock everyone into one room without beer until > > we come up with a working approach' method. > > That does seem like an interesting idea. However, I initially wanted to > push cpufreq support for Armada XP in 3.17, and everything was pushed > *except* the cpufreq driver bits. So I was hoping to at least get > things working for 3.18. If the discussion takes place next week, and > then the time to finally implement something, we'll also miss 3.18. I see. > In the mean time, would it be possible to realize that the existing > generic cpufreq driver simply isn't generic enough, and that we should > accept machine-specific cpufreq drivers for the time being? That would work for me, to get things going for 3.18, but it's really up to the cpufreq maintainers. Your platform_data based patch set seems like a better solution to me than separate drivers, but I haven't looked at the details. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Arnd, On Wed, 10 Sep 2014 12:19:41 +0200, Arnd Bergmann wrote: > the driver for every new platform we want to support. It would > be better to find a way that lets you add new platforms using just > new dts files that work with the existing driver. What do you call a "platform" ? A board or a SoC ? The top-level compatible string contain both references to the board and the SoC, and my idea was obviously that the cpufreq driver would match against the SoC compatible string, not the board one. This definitely allows to support additional boards by adding more dts without touching the code. Of course, it however means that when a new SoC shows up, you'll need to adjust the cpufreq driver. But you generally anyway need some kernel changes to support a new SoC: .dtsi changes are rarely sufficient to completely support a new chip. > > In the mean time, would it be possible to realize that the existing > > generic cpufreq driver simply isn't generic enough, and that we should > > accept machine-specific cpufreq drivers for the time being? > > That would work for me, to get things going for 3.18, but it's really > up to the cpufreq maintainers. Your platform_data based patch set > seems like a better solution to me than separate drivers, but I haven't > looked at the details. I personally don't really care: using platform_data, adding a machine-specific driver, having two platform_driver for cpufreq-dt with different names. Viresh ? Thomas
On 10 September 2014 15:11, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > And with Viresh not accepting any machine specific driver, it Its not that we aren't accepting any drivers at all, but if we can reuse existing infrastructure then we better use it for long term maintainability. But there is another view here which Mike objected to, sometime back. Its also not a good idea to push everything onto a virtual-cpu-clock driver.. I had something in mind to get that fixed, but this thread hasn't allowed me to work on that as it should be finished first. Probably we can add some callbacks to the cpufreq-cpu0 driver. For example if somebody wants to do something crazy enough in their ->target() routine then they can just register their callback and we will call it instead of cpufreq-dt's target() routine.. That way we can still reuse the common part.. > results in platforms like Armada XP having no solution to support > cpufreq... > > So this proposal consists in adding a platform_data flag for the > cpufreq-dt driver, which allows platform code to tell whether CPU > clocks are shared or are independent. > > If you don't like platform_data, we can also register two different > platform_driver for the two different cases, simply with different > names. > > Another approach would be to lift the ban on machine-specific cpufreq > drivers, since the generic driver is not capable of handling all > situations. The platform-data solution is better than any other here :), and that will get fixed once we get all this including which driver to probe from DT.. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dear Viresh Kumar, On Wed, 10 Sep 2014 16:08:51 +0530, Viresh Kumar wrote: > Its not that we aren't accepting any drivers at all, but if we can reuse > existing infrastructure then we better use it for long term maintainability. Sure, but when "existing infrastructure" isn't ready to support a particular use case, and there seems to be no solution on which people are agreeing? > But there is another view here which Mike objected to, sometime back. > Its also not a good idea to push everything onto a virtual-cpu-clock > driver.. > > I had something in mind to get that fixed, but this thread hasn't allowed > me to work on that as it should be finished first. > > Probably we can add some callbacks to the cpufreq-cpu0 driver. For > example if somebody wants to do something crazy enough in their > ->target() routine then they can just register their callback and we > will call it instead of cpufreq-dt's target() routine.. > > That way we can still reuse the common part.. I personally don't think it would be a very good idea: in this case, you'd better do a complete cpufreq driver on its own. Doing subsystems in subsystems just for the sake of factorizing 10 lines of initialization seems a bit silly to me. > The platform-data solution is better than any other here :), and that > will get fixed once we get all this including which driver to probe from > DT.. Ok, I'll respin with a different solution than the flags. Thanks, Thomas
Hello, Here is the v2 of the platform_data based proposal to make cpufreq-dt understand when it has a shared clock by all CPUs vs. independent clock for each CPU. Changes since v1: - Use a 'void *driver_data' in cpufreq_driver instead of extending the existing flags field. Suggested by Viresh. - Keep the regulator related warning in cpufreq-dt, but reword it, and move it to the dev_dbg() level instead of dev_warn(). Thanks, Thomas Thomas Petazzoni (4): cpufreq: allow driver-specific data cpufreq: cpufreq-dt: extend with platform_data ARM: mvebu: use the cpufreq-dt platform_data for independent clocks cpufreq: cpufreq-dt: adjust message related to regulators arch/arm/mach-mvebu/pmsu.c | 8 +++++++- drivers/cpufreq/cpufreq-dt.c | 21 +++++++++++++++++---- drivers/cpufreq/cpufreq.c | 15 +++++++++++++++ include/linux/cpufreq-dt.h | 22 ++++++++++++++++++++++ include/linux/cpufreq.h | 2 ++ 5 files changed, 63 insertions(+), 5 deletions(-) create mode 100644 include/linux/cpufreq-dt.h
On 10 September 2014 17:38, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Hello, > > Here is the v2 of the platform_data based proposal to make cpufreq-dt > understand when it has a shared clock by all CPUs vs. independent > clock for each CPU. > > Changes since v1: > > - Use a 'void *driver_data' in cpufreq_driver instead of extending > the existing flags field. Suggested by Viresh. > > - Keep the regulator related warning in cpufreq-dt, but reword it, > and move it to the dev_dbg() level instead of dev_warn(). > > Thanks, > > Thomas > > Thomas Petazzoni (4): > cpufreq: allow driver-specific data > cpufreq: cpufreq-dt: extend with platform_data > ARM: mvebu: use the cpufreq-dt platform_data for independent clocks > cpufreq: cpufreq-dt: adjust message related to regulators After Arnd's reply: Acked-by: Viresh Kumar <viresh.kumar@linaro.org> -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/10/14 02:53, Arnd Bergmann wrote: > > Most of us will be at LCU next week, so I'd suggest we solve this > problem using the 'lock everyone into one room without beer until > we come up with a working approach' method. > I'll be at connect Tuesday to mid-Thursday. I'd like to be included if such a discussion happens.
Hello, On Wed, 10 Sep 2014 14:08:24 +0200, Thomas Petazzoni wrote: > Thomas Petazzoni (4): > cpufreq: allow driver-specific data > cpufreq: cpufreq-dt: extend with platform_data > ARM: mvebu: use the cpufreq-dt platform_data for independent clocks > cpufreq: cpufreq-dt: adjust message related to regulators > > arch/arm/mach-mvebu/pmsu.c | 8 +++++++- > drivers/cpufreq/cpufreq-dt.c | 21 +++++++++++++++++---- > drivers/cpufreq/cpufreq.c | 15 +++++++++++++++ > include/linux/cpufreq-dt.h | 22 ++++++++++++++++++++++ > include/linux/cpufreq.h | 2 ++ > 5 files changed, 63 insertions(+), 5 deletions(-) > create mode 100644 include/linux/cpufreq-dt.h Any news on getting this merged for 3.18 ? Thanks, Thomas
Hello, On Wed, 10 Sep 2014 14:08:24 +0200, Thomas Petazzoni wrote: > Thomas Petazzoni (4): > cpufreq: allow driver-specific data > cpufreq: cpufreq-dt: extend with platform_data > ARM: mvebu: use the cpufreq-dt platform_data for independent clocks > cpufreq: cpufreq-dt: adjust message related to regulators > > arch/arm/mach-mvebu/pmsu.c | 8 +++++++- > drivers/cpufreq/cpufreq-dt.c | 21 +++++++++++++++++---- > drivers/cpufreq/cpufreq.c | 15 +++++++++++++++ > include/linux/cpufreq-dt.h | 22 ++++++++++++++++++++++ > include/linux/cpufreq.h | 2 ++ > 5 files changed, 63 insertions(+), 5 deletions(-) > create mode 100644 include/linux/cpufreq-dt.h Since this series was Acked-by Viresh, would it be possible to get it merged for 3.18 ? Thanks, Thomas
On Monday, October 06, 2014 09:19:05 AM Thomas Petazzoni wrote: > Hello, > > On Wed, 10 Sep 2014 14:08:24 +0200, Thomas Petazzoni wrote: > > > Thomas Petazzoni (4): > > cpufreq: allow driver-specific data > > cpufreq: cpufreq-dt: extend with platform_data > > ARM: mvebu: use the cpufreq-dt platform_data for independent clocks > > cpufreq: cpufreq-dt: adjust message related to regulators > > > > arch/arm/mach-mvebu/pmsu.c | 8 +++++++- > > drivers/cpufreq/cpufreq-dt.c | 21 +++++++++++++++++---- > > drivers/cpufreq/cpufreq.c | 15 +++++++++++++++ > > include/linux/cpufreq-dt.h | 22 ++++++++++++++++++++++ > > include/linux/cpufreq.h | 2 ++ > > 5 files changed, 63 insertions(+), 5 deletions(-) > > create mode 100644 include/linux/cpufreq-dt.h > > Since this series was Acked-by Viresh, would it be possible to get it > merged for 3.18 ? I'm still seeing discussion going on about patch [1/4], though. Is that irrelevant? Also I'd prefer Viresh to tell me that this is 3.18 material instead of sending ACKs in random replies and hoping that I'll notice them. I may not.
On 7 October 2014 04:23, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > I'm still seeing discussion going on about patch [1/4], though. That's V1's 1/4 and we finalized V2.. > Is that irrelevant? No. > Also I'd prefer Viresh to tell me that this is 3.18 material instead of > sending ACKs in random replies and hoping that I'll notice them. I may > not. Hmm, so yeah its for 3.18.. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, October 07, 2014 09:00:16 AM Viresh Kumar wrote: > On 7 October 2014 04:23, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > I'm still seeing discussion going on about patch [1/4], though. > > That's V1's 1/4 and we finalized V2.. > > > Is that irrelevant? > > No. > > > Also I'd prefer Viresh to tell me that this is 3.18 material instead of > > sending ACKs in random replies and hoping that I'll notice them. I may > > not. > > Hmm, so yeah its for 3.18.. OK So can you please send me links to the patches? I probably can find them, but honestly I have too little time for other stuff already.
On 8 October 2014 05:29, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > So can you please send me links to the patches? I probably can find them, > but honestly I have too little time for other stuff already. Sure: https://patchwork.kernel.org/patch/4876641/ https://patchwork.kernel.org/patch/4876651/ https://patchwork.kernel.org/patch/4876661/ https://patchwork.kernel.org/patch/4876671/ -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, October 08, 2014 11:21:42 AM Viresh Kumar wrote: > On 8 October 2014 05:29, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > So can you please send me links to the patches? I probably can find them, > > but honestly I have too little time for other stuff already. > > Sure: > > https://patchwork.kernel.org/patch/4876641/ > https://patchwork.kernel.org/patch/4876651/ > https://patchwork.kernel.org/patch/4876661/ > https://patchwork.kernel.org/patch/4876671/ They didn't apply cleanly for me and I didn't have the time to rebase them myself. Thomas, care to rebase the v2 on top of the current Linus' tree and resend?
Dear Rafael J. Wysocki, On Sun, 12 Oct 2014 22:25:02 +0200, Rafael J. Wysocki wrote: > Thomas, care to rebase the v2 on top of the current Linus' tree and > resend? I've just resent the patches. See: https://patchwork.kernel.org/patch/5100691/ https://patchwork.kernel.org/patch/5100731/ https://patchwork.kernel.org/patch/5100711/ There are only 3 patches in this version, compared to four in the previous version, because one of the patch touches arch/arm/mach-mvebu/, and should therefore go through the mvebu maintainers. Thanks, Thomas
diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig index 3489f8f..a755227 100644 --- a/drivers/cpufreq/Kconfig +++ b/drivers/cpufreq/Kconfig @@ -196,6 +196,15 @@ config CPUFREQ_DT If in doubt, say N. +config CPUFREQ_DT_INDEPENDENT_CLOCKS + bool "CPUs change clocks independently" + depends on CPUFREQ_DT + help + Selecting this will make cpufreq-dt driver believe that all CPUs have + independent clock lines and so all will have separate 'struct + cpufreq_policy' instances for them. This must be replaced by proper DT + bindings later on. + menu "x86 CPU frequency scaling drivers" depends on X86 source "drivers/cpufreq/Kconfig.x86" diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c index e002650..dbb6f22 100644 --- a/drivers/cpufreq/cpufreq-dt.c +++ b/drivers/cpufreq/cpufreq-dt.c @@ -266,9 +266,22 @@ static int cpufreq_init(struct cpufreq_policy *policy) policy->driver_data = priv; policy->clk = cpu_clk; - ret = cpufreq_generic_init(policy, freq_table, transition_latency); - if (ret) + ret = cpufreq_table_validate_and_show(policy, freq_table); + if (ret) { + dev_err(cpu_dev, "%s: invalid frequency table: %d\n", __func__, + ret); goto out_cooling_unregister; + } + + policy->cpuinfo.transition_latency = transition_latency; + +#ifndef CONFIG_CPUFREQ_DT_INDEPENDENT_CLOCKS + /* + * The driver only supports the SMP configuartion where all processors + * share the clock and voltage and clock. + */ + cpumask_setall(policy->cpus); +#endif return 0;
There has been lots of discussion over mailing lists about how to get CPU's clock sharing information for platforms as CPUFreq core depends on that. It doesn't look that there will be any immediate solution to that as there are multiple views over how to get that from DT. But there are platforms (already upstreamed) which depend on this requirement and are waiting for a solution to sail through. The common thing about them (Krait and Mvebu) is that all CPUs have independent clock lines and that makes things easier for us. So, this patch creates another Kconfig option to indicate if all CPUs share same clock line or all have independent. The complex cases of multiple clusters having separate clock lines, but having same for all CPUs within a cluster isn't addressed yet. This is a *short-term* solution for getting these platforms up an running and a DT based solution should be upstreamed soon. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- Yesterday again I was pinged by Thomas about the status of this work and the thread isn't going anywhere. He is blocked with just this to get his platform running on mainline. Will it be possible to get this through as a temporary solution? drivers/cpufreq/Kconfig | 9 +++++++++ drivers/cpufreq/cpufreq-dt.c | 17 +++++++++++++++-- 2 files changed, 24 insertions(+), 2 deletions(-)