Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1440669 > unrolled thread
| Started by | Abhilash Kesavan <kesavan.abhilash@gmail.com> |
|---|---|
| First post | 2016-07-11 16:50 +0200 |
| Last post | 2016-07-12 10:50 +0200 |
| Articles | 4 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH 2/2] soc: samsung: Add support for Exynos7 PMU Abhilash Kesavan <kesavan.abhilash@gmail.com> - 2016-07-11 16:50 +0200
Re: [PATCH 2/2] soc: samsung: Add support for Exynos7 PMU Sylwester Nawrocki <s.nawrocki@samsung.com> - 2016-07-11 18:40 +0200
Re: [PATCH 2/2] soc: samsung: Add support for Exynos7 PMU Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-07-12 11:00 +0200
Re: [PATCH 2/2] soc: samsung: Add support for Exynos7 PMU Krzysztof Kozlowski <k.kozlowski@samsung.com> - 2016-07-12 10:50 +0200
| From | Abhilash Kesavan <kesavan.abhilash@gmail.com> |
|---|---|
| Date | 2016-07-11 16:50 +0200 |
| Subject | Re: [PATCH 2/2] soc: samsung: Add support for Exynos7 PMU |
| Message-ID | <rTKH0-8jQ-11@gated-at.bofh.it> |
Hi Krzysztof,
[...]
>> diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
>> index 0acdfd8..7cda8fb 100644
>> --- a/drivers/soc/samsung/exynos-pmu.c
>> +++ b/drivers/soc/samsung/exynos-pmu.c
>> @@ -88,6 +88,9 @@ static const struct of_device_id exynos_pmu_of_device_ids[] = {
>> }, {
>> .compatible = "samsung,exynos5420-pmu",
>> .data = &exynos5420_pmu_data,
>> + }, {
>> + .compatible = "samsung,exynos7-pmu",
>> + .data = &exynos7_pmu_data,
>
> Hi,
>
> Thanks for the patch. Few comments:
Thanks for the review.
>
> You set here compatible for Exynos7. However there are at least three
> publicly known Exynos7 chipsets (7420, 7580, 7870 -
> https://en.wikipedia.org/wiki/Exynos). My questions are:
> 1. Are all of these share the same PMU configuration?
> 2. New different Exynos7 may be released, right?
Exynos7 is a Quad Core A57 based SoC that pre-dates all the above
mentioned SoCs. It is the closest to the exynos7420 in terms of the
IPs present. The PMU configuration between exynos7 and exynos7420 is
identical except for the extra A53 configuration required in case of
the 7420. The PMU configuration for 7580 and 7870 differ from that of
eynos7 and 7420 in terms of the registers offsets, number of
registers being configured and some extra configurations. So, while
sharing of some functions is possible across the SoCs, each SoC should
ideally have its own PMU file. The posted patch adds PMU support for
only the exynos7 SoC.
>
> The exynos7 compatible is already spread all over DTS... but probably
> it is safer to use a specific SoC revision. Unless you are sure that
> all Exynos7 SoCs will be 100% compatible here and there won't be
> another exynos7xxx-pmu.
Please let me know if I can continue to use the exynos7 compatible
since it is a distinct SoC and not indicative of a series. However, if
you feel strongly about it then I can change the compatible to use
7420 since they are quite similar.
>
>
>> },
>> { /*sentinel*/ },
>> };
[...]
>> +static void exynos7_pmu_init(void)
>> +{
>> + unsigned int cpu;
>> + unsigned int tmp, i;
>> + struct device_node *node;
>> + static void __iomem *atlas_cmu_base;
>
> 1. Why static?
> 2. This looks like used only in if() block below. Move it there so it
> will stay local to that block.
Will re-work as per your other comments below.
>
>> +
>> + /* Enable only SC_FEEDBACK for the register list */
>> + for (i = 0 ; i < ARRAY_SIZE(exynos7_list_feed) ; i++) {
>> + tmp = pmu_raw_readl(exynos7_list_feed[i]);
>> + tmp &= ~EXYNOS5_USE_SC_COUNTER;
>> + tmp |= EXYNOS5_USE_SC_FEEDBACK;
>> + pmu_raw_writel(tmp, exynos7_list_feed[i]);
>> + }
>> +
>> + /*
>> + * Disable automatic L2 flush, Disable L2 retention and
>> + * Enable STANDBYWFIL2, ACE/ACP
>> + */
>> + tmp = pmu_raw_readl(EXYNOS7_ATLAS_L2_OPTION);
>> + tmp &= ~(EXYNOS7_USE_AUTO_L2FLUSHREQ | EXYNOS7_USE_RETENTION);
>> + tmp |= (EXYNOS7_USE_STANDBYWFIL2 |
>> + EXYNOS7_USE_DEACTIVATE_ACE |
>> + EXYNOS7_USE_DEACTIVATE_ACP);
>> + pmu_raw_writel(tmp, EXYNOS7_ATLAS_L2_OPTION);
>> +
>> + /*
>> + * Enable both SC_COUNTER and SC_FEEDBACK for the CPUs
>> + * Use STANDBYWFI and SMPEN to indicate that core is ready to enter
>> + * low power mode
>> + */
>> + for (cpu = 0; cpu < 4; cpu++) {
>
> '4' shouldn't be hardcoded here. num_possible_cpus()?
Will change.
[...]
>> + /*
>> + * Set clock freeze cycle count to 0 before and after arm clamp or
>> + * reset signal transition
>> + */
>> + node = of_find_compatible_node(NULL, NULL,
>> + "samsung,exynos7-clock-atlas");
>> + if (node) {
>> + atlas_cmu_base = of_iomap(node, 0);
>> + if (!atlas_cmu_base)
>> + return;
>> +
>> + __raw_writel(0x0,
>> + atlas_cmu_base + EXYNOS7_CORE_ARMCLK_STOPCTRL);
>> + iounmap(atlas_cmu_base);
>
> Missing:
> of_node_put(node);
>
> ...but I think this creates unnecessary dependency on different
> compatible. I understand that disabling the EXTENDED_CLKSTOP is needed
> after configuring the PMU so this code belongs here. However
> everything you need is just a mapping of CMU address. The PMU driver
> should receive in bindings everything it needs to do its work. Either
> it is a phandle to something or an address for iomap. In this case the
> PMU should probably get two addresses: PMU and optionally CMU (part of
> CMU for example). Of course bindings would have to be updated.
I will add an optional CMU phandle to the PMU bindings.
>
>> + }
>> +
>> + pr_info("Exynos7 PMU has been initialized\n");
>> +}
>> +
>> +const struct exynos_pmu_data exynos7_pmu_data = {
>> + .pmu_config = exynos7_pmu_config,
>> + .pmu_init = exynos7_pmu_init,
>> + .pmu_config_extra = exynos7_pmu_config_extra,
>> + .powerdown_conf = exynos7_powerdown_conf,
>> +};
>> diff --git a/include/linux/soc/samsung/exynos-regs-pmu.h b/include/linux/soc/samsung/exynos-regs-pmu.h
>> index d30186e..2de1d94 100644
>> --- a/include/linux/soc/samsung/exynos-regs-pmu.h
>> +++ b/include/linux/soc/samsung/exynos-regs-pmu.h
>
> (...)
>
>> +/* EXYNOS7_PMU_DEBUG */
>> +#define EXYNOS7_CLKOUT_DISABLE (0x1 << 0)
>> +
>> +#define EXYNOS7_USE_SMPEN (0x1 << 28)
>> +#define EXYNOS7_USE_STANDBYWFE (0x1 << 24)
>> +#define EXYNOS7_USE_STANDBYWFI (0x1 << 16)
>> +#define EXYNOS7_USE_SC_FEEDBACK (0x1 << 1)
>> +#define EXYNOS7_USE_SC_COUNTER (0x1 << 0)
>> +
>> +/* EXYNOS7_PAD_RETENTION_AUD_OPTION */
>> +#define PAD_INITIATE_WAKEUP (0x1 << 28)
>> +
>> +#define EXYNOS7_DUR_WAIT_RESET (0xF << 20)
>> +#define EXYNOS7_DUR_SCALL (0xF << 4)
>> +#define EXYNOS7_DUR_SCALL_VALUE (0x1 << 4)
>> +
>> +#define EXYNOS7_SKIP_BLK_PWR_DOWN (0x1 << 8)
>> +#define EXYNOS7_ENABLE_ATLAS_CPU (0x1 << 0)
>> +
>> +#define EXYNOS7_PS_HOLD_OUTPUT (0x1 << 8)
>> +#define EXYNOS7_ENABLE_HW_TRIP (0x1 << 31)
>> +#define EXYNOS7_DBG_INITIATE_WAKEUP (0x3 << 16)
>> +
>> +#define EXYNOS7_CORE_ARMCLK_STOPCTRL (0x1000)
> Please add comment that this is from different block (CMU).
Will add a comment.
>> +
>> #endif /* __LINUX_SOC_EXYNOS_REGS_PMU_H */
>> --
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
[toc] | [next] | [standalone]
| From | Sylwester Nawrocki <s.nawrocki@samsung.com> |
|---|---|
| Date | 2016-07-11 18:40 +0200 |
| Message-ID | <rTMpr-19f-7@gated-at.bofh.it> |
| In reply to | #1440669 |
On 07/11/2016 04:44 PM, Abhilash Kesavan wrote:
>>> + /*
>>> >> + * Set clock freeze cycle count to 0 before and after arm clamp or
>>> >> + * reset signal transition
>>> >> + */
>>> >> + node = of_find_compatible_node(NULL, NULL,
>>> >> + "samsung,exynos7-clock-atlas");
>>> >> + if (node) {
>>> >> + atlas_cmu_base = of_iomap(node, 0);
>>> >> + if (!atlas_cmu_base)
>>> >> + return;
>>> >> +
>>> >> + __raw_writel(0x0,
>>> >> + atlas_cmu_base + EXYNOS7_CORE_ARMCLK_STOPCTRL);
>>> >> + iounmap(atlas_cmu_base);
>> >
>> > Missing:
>> > of_node_put(node);
>> >
>> > ...but I think this creates unnecessary dependency on different
>> > compatible. I understand that disabling the EXTENDED_CLKSTOP is needed
>> > after configuring the PMU so this code belongs here. However
>> > everything you need is just a mapping of CMU address. The PMU driver
>> > should receive in bindings everything it needs to do its work. Either
>> > it is a phandle to something or an address for iomap. In this case the
>> > PMU should probably get two addresses: PMU and optionally CMU (part of
>> > CMU for example). Of course bindings would have to be updated.
>
> I will add an optional CMU phandle to the PMU bindings.
We could additionally split the CMU_ATLAS region into 2 regions in DT
(derived from exynos7420 documentation):
reg = <0x11800000 0xF08>, // offsets 0x0000...0x0F04
<0x11801000 0x8C>, // offsets 0x1000...0x1088
so that the first can be mapped by the clk driver and the second by
the PMU driver? It seems the first region is strictly clock functionality
related, while the second contains power control related and other
registers.
However I'm not sure it is a good idea, for consistency this would need
to be done also for CMU_APOLLO, CMU_MIF{0...3}. All these CMUs don't have
DT bindings defined yet though and there is no corresponding dts entries.
--
Thanks,
Sylwester
[toc] | [prev] | [next] | [standalone]
| From | Krzysztof Kozlowski <k.kozlowski@samsung.com> |
|---|---|
| Date | 2016-07-12 11:00 +0200 |
| Message-ID | <rU1HP-2A1-11@gated-at.bofh.it> |
| In reply to | #1440751 |
On 07/11/2016 06:38 PM, Sylwester Nawrocki wrote:
> On 07/11/2016 04:44 PM, Abhilash Kesavan wrote:
>>>> + /*
>>>>>> + * Set clock freeze cycle count to 0 before and after arm clamp or
>>>>>> + * reset signal transition
>>>>>> + */
>>>>>> + node = of_find_compatible_node(NULL, NULL,
>>>>>> + "samsung,exynos7-clock-atlas");
>>>>>> + if (node) {
>>>>>> + atlas_cmu_base = of_iomap(node, 0);
>>>>>> + if (!atlas_cmu_base)
>>>>>> + return;
>>>>>> +
>>>>>> + __raw_writel(0x0,
>>>>>> + atlas_cmu_base + EXYNOS7_CORE_ARMCLK_STOPCTRL);
>>>>>> + iounmap(atlas_cmu_base);
>>>>
>>>> Missing:
>>>> of_node_put(node);
>>>>
>>>> ...but I think this creates unnecessary dependency on different
>>>> compatible. I understand that disabling the EXTENDED_CLKSTOP is needed
>>>> after configuring the PMU so this code belongs here. However
>>>> everything you need is just a mapping of CMU address. The PMU driver
>>>> should receive in bindings everything it needs to do its work. Either
>>>> it is a phandle to something or an address for iomap. In this case the
>>>> PMU should probably get two addresses: PMU and optionally CMU (part of
>>>> CMU for example). Of course bindings would have to be updated.
>>
>> I will add an optional CMU phandle to the PMU bindings.
>
> We could additionally split the CMU_ATLAS region into 2 regions in DT
> (derived from exynos7420 documentation):
>
> reg = <0x11800000 0xF08>, // offsets 0x0000...0x0F04
> <0x11801000 0x8C>, // offsets 0x1000...0x1088
>
> so that the first can be mapped by the clk driver and the second by
> the PMU driver? It seems the first region is strictly clock functionality
> related, while the second contains power control related and other
> registers.
Do the clk-exynos7 driver really needs the second region? If not then it
would be sufficient just to reduce the mapping range for CMU_ATLAS.
Best regards,
Krzysztof
>
> However I'm not sure it is a good idea, for consistency this would need
> to be done also for CMU_APOLLO, CMU_MIF{0...3}. All these CMUs don't have
> DT bindings defined yet though and there is no corresponding dts entries.
>
[toc] | [prev] | [next] | [standalone]
| From | Krzysztof Kozlowski <k.kozlowski@samsung.com> |
|---|---|
| Date | 2016-07-12 10:50 +0200 |
| Message-ID | <rU1ya-2wR-17@gated-at.bofh.it> |
| In reply to | #1440669 |
On 07/11/2016 04:44 PM, Abhilash Kesavan wrote:
> Hi Krzysztof,
>
> [...]
>>> diff --git a/drivers/soc/samsung/exynos-pmu.c b/drivers/soc/samsung/exynos-pmu.c
>>> index 0acdfd8..7cda8fb 100644
>>> --- a/drivers/soc/samsung/exynos-pmu.c
>>> +++ b/drivers/soc/samsung/exynos-pmu.c
>>> @@ -88,6 +88,9 @@ static const struct of_device_id exynos_pmu_of_device_ids[] = {
>>> }, {
>>> .compatible = "samsung,exynos5420-pmu",
>>> .data = &exynos5420_pmu_data,
>>> + }, {
>>> + .compatible = "samsung,exynos7-pmu",
>>> + .data = &exynos7_pmu_data,
>>
>> Hi,
>>
>> Thanks for the patch. Few comments:
>
> Thanks for the review.
>>
>> You set here compatible for Exynos7. However there are at least three
>> publicly known Exynos7 chipsets (7420, 7580, 7870 -
>> https://en.wikipedia.org/wiki/Exynos). My questions are:
>> 1. Are all of these share the same PMU configuration?
>> 2. New different Exynos7 may be released, right?
>
> Exynos7 is a Quad Core A57 based SoC that pre-dates all the above
> mentioned SoCs. It is the closest to the exynos7420 in terms of the
> IPs present.
Hmm, okay... It is confusing because Samsung Semiconductors calls both
7420 and 5433 as "Exynos 7 Octa":
http://www.samsung.com/semiconductor/minisite/Exynos/w/solution/mobile_ap/7420/
The marketing uses term "Exynos7" for a generation of SoCs.
However if there is really a design called Exynos7 and a board with it
(Espresso), then I don't mind. Let it be Exynos7 but keeping in mind
that this is a specific SoC, not a calls of products.
> The PMU configuration between exynos7 and exynos7420 is
> identical except for the extra A53 configuration required in case of
> the 7420.
That is additional argument in favor of "Exynos7" naming.
> The PMU configuration for 7580 and 7870 differ from that of
> eynos7 and 7420 in terms of the registers offsets, number of
> registers being configured and some extra configurations. So, while
> sharing of some functions is possible across the SoCs, each SoC should
> ideally have its own PMU file. The posted patch adds PMU support for
> only the exynos7 SoC.
Thanks for explanation.
>>
>> The exynos7 compatible is already spread all over DTS... but probably
>> it is safer to use a specific SoC revision. Unless you are sure that
>> all Exynos7 SoCs will be 100% compatible here and there won't be
>> another exynos7xxx-pmu.
> Please let me know if I can continue to use the exynos7 compatible
> since it is a distinct SoC and not indicative of a series. However, if
> you feel strongly about it then I can change the compatible to use
> 7420 since they are quite similar.
Exynos7 is fine. Thanks for the details!
Best regards,
Krzysztof
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web