Re: [PATCH v11 1/9] ARM: OMAP2+: use common l2cache initialization code

[Tomasz Figa]

2015-01-06 5:25 GMT+09:00 Arnd Bergmann a...@arndb.de:
 On Monday 05 January 2015 13:19:00 Marek Szyprowski wrote:
  DT_MACHINE_START(OMAP4_DT, Generic OMAP4 (Flattened Device Tree))
 +   .l2c_aux_val= OMAP_L2C_AUX_CTRL,
 +   .l2c_aux_mask   = 0xcf9f,
 +   .l2c_write_sec  = omap4_l2c310_write_sec,
 .reserve= omap_reserve,
 .smp= smp_ops(omap4_smp_ops),
 .map_io = omap4_map_io,


 Could we also get those values into the dts files? Clearly we
 can't remove them here without breaking compatibility with old
 dtbs, but it would be nice to have all new dtbs do the right thing.

Sounds like a good next step after merging this series. :)

Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 2/2] ARM: l2c: AM437x: Introduce support for cache filter programming

[Tomasz Figa]

2015-01-04 0:34 GMT+09:00 Nishanth Menon n...@ti.com:
 On 15:40-20150103, Tomasz Figa wrote:
 Hi Nishanth,

 2015-01-03 2:43 GMT+09:00 Nishanth Menon n...@ti.com:
  AM437x generation of processors support programming the PL310 L2Cache
  controller's address filter start and end registers using a secure
  montior service.

 typo: s/montior/monitor/

 [snip]

 Uggh.. yes indeed. I will post a v3 updating the comments. If the
 following is ok.

  +   base = omap4_get_l2cache_base();
  +   filter_start = (reg == L310_ADDR_FILTER_START) ? val :
  +  readl_relaxed(base + 
  L310_ADDR_FILTER_START);
  +   filter_end = (reg == L310_ADDR_FILTER_END) ? val :
  +  readl_relaxed(base + L310_ADDR_FILTER_END);
  +   omap_smc1_2(AM43X_MON_L2X0_SETFILTER_INDEX, filter_start,
  +   filter_end);
  +   return;

 I don't have any significant comments about this patch in particular,
 but just noticed that you need to do read-backs here (and the typo
 thanks to the spell checker of my mailing app). Maybe you should
 consider switching to the .configure() API I introduced in my series?
 This would let you get rid of the hardcoded static mapping.

 Yeah, I have two choices there.. Either I provide the fundamental
 write function for the generic l2c code to use OR I provide a
 duplicate of resultant l2c_configure(aux write) + l2c310_configure.

 To allow for reuse of improvements or anything like errata
 implementations in the future, OMAP L2C implementation has chosen to provide 
 the
 low level code and allow the higherlevel configure/write/whatever of the
 future to stay in arch/arm/mm/cache-l2x0.c. The write_sec operation is
 not too complicated enough to warrant a replication of l2c310_configure.

 So, I prefer the current implementation than providing a .configure
 handler for outer_cache.configure from SoC level.

 Let me know if anyone has a strong objection to this.

Well, what l2c310_configure() does after my series is just writing the
registers. If they cannot be written normally (without some tricks
such as reading back other registers) then IMHO a separate function
should be provided.

This is becomes possible after patch 3/8 (ARM: l2c: Add interface to
ask hypervisor to configure L2C) and what is used on Exynos which also
updates multiple registers in single SMC calls. You can find an
example of use in patch 6/8 (ARM: EXYNOS: Add .write_sec outer cache
callback for L2C-310). What's even more interesting is that approaches
similar to the one currently used on OMAP had been NAKed, when
proposed for Exynos and this is why we have the solution proposed by
my patches.

Note that .write_sec() callback is still used for L2X0_CTRL and
L2X0_DEBUG_CTRL registers, because there might be a need to write them
separately (e.g. to disable the controller and to perform debug
operations/workarounds when the controller is already enabled).

Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 2/2] ARM: l2c: AM437x: Introduce support for cache filter programming

[Tomasz Figa]

2015-01-04 1:45 GMT+09:00 Nishanth Menon n...@ti.com:
 On 01/03/2015 10:16 AM, Tomasz Figa wrote:

 2015-01-04 0:34 GMT+09:00 Nishanth Menon n...@ti.com:

 On 15:40-20150103, Tomasz Figa wrote:

 Hi Nishanth,

 2015-01-03 2:43 GMT+09:00 Nishanth Menon n...@ti.com:

 AM437x generation of processors support programming the PL310 L2Cache
 controller's address filter start and end registers using a secure
 montior service.


 typo: s/montior/monitor/

 [snip]


 Uggh.. yes indeed. I will post a v3 updating the comments. If the
 following is ok.


 +   base = omap4_get_l2cache_base();
 +   filter_start = (reg == L310_ADDR_FILTER_START) ? val :
 +  readl_relaxed(base +
 L310_ADDR_FILTER_START);
 +   filter_end = (reg == L310_ADDR_FILTER_END) ? val :
 +  readl_relaxed(base +
 L310_ADDR_FILTER_END);
 +   omap_smc1_2(AM43X_MON_L2X0_SETFILTER_INDEX,
 filter_start,
 +   filter_end);
 +   return;


 I don't have any significant comments about this patch in particular,
 but just noticed that you need to do read-backs here (and the typo
 thanks to the spell checker of my mailing app). Maybe you should
 consider switching to the .configure() API I introduced in my series?
 This would let you get rid of the hardcoded static mapping.


 Yeah, I have two choices there.. Either I provide the fundamental
 write function for the generic l2c code to use OR I provide a
 duplicate of resultant l2c_configure(aux write) + l2c310_configure.

 To allow for reuse of improvements or anything like errata
 implementations in the future, OMAP L2C implementation has chosen to
 provide the
 low level code and allow the higherlevel configure/write/whatever of the
 future to stay in arch/arm/mm/cache-l2x0.c. The write_sec operation is
 not too complicated enough to warrant a replication of l2c310_configure.

 So, I prefer the current implementation than providing a .configure
 handler for outer_cache.configure from SoC level.

 Let me know if anyone has a strong objection to this.


 Well, what l2c310_configure() does after my series is just writing the
 registers. If they cannot be written normally (without some tricks
 such as reading back other registers) then IMHO a separate function
 should be provided.

 This is becomes possible after patch 3/8 (ARM: l2c: Add interface to
 ask hypervisor to configure L2C) and what is used on Exynos which also
 updates multiple registers in single SMC calls. You can find an
 example of use in patch 6/8 (ARM: EXYNOS: Add .write_sec outer cache
 callback for L2C-310). What's even more interesting is that approaches
 similar to the one currently used on OMAP had been NAKed, when
 proposed for Exynos and this is why we have the solution proposed by
 my patches.

 Note that .write_sec() callback is still used for L2X0_CTRL and
 L2X0_DEBUG_CTRL registers, because there might be a need to write them
 separately (e.g. to disable the controller and to perform debug
 operations/workarounds when the controller is already enabled).



 we dont have a machine descriptor for configure instead we overide the
 logic(in you case after firmware load, in OMAP case, I need to figure out).
 my point being unlike the exynos configure code, OMAP code will look exactly
 like current pl310_configure-2 lines of code which really does not benefit
 anything.


 Thinking again, in fact, i'd rather drop this series than have to do a
 duplicated configure code(and force a resultant maintenance for the future)
 in OMAP code since none of OMAP4 or AM437x series need these patches.
 Interest for this series was non-mandatory, but just to be complete from SoC
 definition point of view.

 Let me know which way you guys want me to go.

Right, dropping this series would definitely solve the the read-back issue. :)

After all, if you don't need to override the latencies in the kernel
(i.e. have sane firmware, unlike certain Exynos boards ;)), then I
don't see a point of having this feature.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v10 2/8] ARM: l2c: Refactor the driver to use commit-like interface

[Tomasz Figa]

On 30.12.2014 03:23, Nishanth Menon wrote:

On 12/23/2014 04:48 AM, Marek Szyprowski wrote:


-static void l2c310_resume(void)
+static void l2c310_configure(void __iomem *base)
  {
-   void __iomem *base = l2x0_base;
+   unsigned revision;

-   if (!(readl_relaxed(base + L2X0_CTRL)  L2X0_CTRL_EN)) {
-   unsigned revision;
-
-   /* restore pl310 setup */
-   writel_relaxed(l2x0_saved_regs.tag_latency,
-  base + L310_TAG_LATENCY_CTRL);
-   writel_relaxed(l2x0_saved_regs.data_latency,
-  base + L310_DATA_LATENCY_CTRL);
-   writel_relaxed(l2x0_saved_regs.filter_end,
-  base + L310_ADDR_FILTER_END);
-   writel_relaxed(l2x0_saved_regs.filter_start,
-  base + L310_ADDR_FILTER_START);
-
-   revision = readl_relaxed(base + L2X0_CACHE_ID) 
-   L2X0_CACHE_ID_RTL_MASK;
-
-   if (revision = L310_CACHE_ID_RTL_R2P0)
-   l2c_write_sec(l2x0_saved_regs.prefetch_ctrl, base,
- L310_PREFETCH_CTRL);
-   if (revision = L310_CACHE_ID_RTL_R3P0)
-   l2c_write_sec(l2x0_saved_regs.pwr_ctrl, base,
- L310_POWER_CTRL);
-
-   l2c_enable(base, l2x0_saved_regs.aux_ctrl, 8);
-
-   /* Re-enable full-line-of-zeros for Cortex-A9 */
-   if (l2x0_saved_regs.aux_ctrl  L310_AUX_CTRL_FULL_LINE_ZERO)
-   set_auxcr(get_auxcr() | BIT(3) | BIT(2) | BIT(1));
-   }
+   /* restore pl310 setup */
+   writel_relaxed(l2x0_saved_regs.tag_latency,
+  base + L310_TAG_LATENCY_CTRL);
+   writel_relaxed(l2x0_saved_regs.data_latency,
+  base + L310_DATA_LATENCY_CTRL);
+   writel_relaxed(l2x0_saved_regs.filter_end,
+  base + L310_ADDR_FILTER_END);
+   writel_relaxed(l2x0_saved_regs.filter_start,
+  base + L310_ADDR_FILTER_START);
+


^^ The above change broke AM437xx. Looks like the change causes the
following behavior difference on AM437x. For some reason, touching any
of the above 4 registers(even with the values read from the same
registers) causes AM437x to go beserk. Comment the 4 writes and we
reach shell. looks like l2c310_resume is not invoked prior to this
series. :(.. now that we reuse that logic to actually do programming,
we start to see the problem.


OK, I probably have answer for this. Apparently all four register above 
cannot be written in non-secure mode and they should go through 
l2c_write_sec(). More on this can be found in CoreLink Level 2 Cache 
Controller L2C-310 Technical Reference Manual, 3.2. Register summary, 
table 3.1. I have checked the TRM for r3p3, but I guess this should be 
uniform for all revisions.


Why this worked before? The registers were not written unless respective 
properties in DT were present and OMAP do not have them in DT. Current 
code always writes them, which should not really matter if the code is 
correct. (But it isn't - writel_relaxed() can't be used directly for 
those registers.)


Could you check if replacing those four writel_relaxed() with 
l2c_write_sec() does the thing?


Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v10 2/8] ARM: l2c: Refactor the driver to use commit-like interface

[Tomasz Figa]

On 30.12.2014 23:51, Nishanth Menon wrote:

Looks like the following also need addressing:
data-save is called twice (once more after l2cof_init)
l2c310_init_fns also needs l2c310_configure
will be nice to use l2x0_data only after we kmemdup data in __l2c_init


I'll check this.

Thanks.



Apparently the second save in __l2c_init() is not needed and it should 
have been removed. However it might be a good idea to actually do second 
save in l2c_enable() after l2c_configure() so that the values actually 
permitted by hardware and/or secure firmware are stored.


l2c310_init_fns needs to be updated indeed.

However I'm not sure about your concern about using l2x0_data before 
kmemdup(). I don't see any code potentially doing this.


Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v10 2/8] ARM: l2c: Refactor the driver to use commit-like interface

[Tomasz Figa]

On 02.01.2015 18:13, Tomasz Figa wrote:

On 30.12.2014 23:51, Nishanth Menon wrote:

Looks like the following also need addressing:
data-save is called twice (once more after l2cof_init)
l2c310_init_fns also needs l2c310_configure
will be nice to use l2x0_data only after we kmemdup data in __l2c_init


I'll check this.

Thanks.



Apparently the second save in __l2c_init() is not needed and it should
have been removed. However it might be a good idea to actually do second
save in l2c_enable() after l2c_configure() so that the values actually
permitted by hardware and/or secure firmware are stored.

l2c310_init_fns needs to be updated indeed.


Hmm, apparently current patch already adds this (and I missed it reading 
it at first), so I'm not sure what's your concern about it.


Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 0/2] ARM: l2c: OMAP4/AM437x: Additional register programming support.

[Tomasz Figa]

Hi Tony,

2015-01-03 9:23 GMT+09:00 Tony Lindgren t...@atomide.com:
 * Nishanth Menon n...@ti.com [150102 11:50]:
 On 01/02/2015 12:46 PM, santosh.shilim...@oracle.com wrote:
  On 1/2/15 9:43 AM, Nishanth Menon wrote:
  Hi,
  OMAP4 and AM437x ROM code provides services to program PL310's latency
  registers and AM437x provides service for programming Address filter
  registers.
 
  Provide support in the kernel for the same.
 
  V2 of the series contains documentation update and a bug fix due to a
  typo introduced during patch split :(
 
  Nishanth Menon (2):
 ARM: l2c: OMAP4/AM437x: Introduce support for cache latency
   programming
 ARM: l2c: AM437x: Introduce support for cache filter programming
 
  Looks fine to me ...
  Feel free to add my ack if you need one ...
 
  Minor: The subject looks like I2C though it is L2C ;-)
 
 Yeah, the thought did occur to me, but decided instead to go with the
 existing $subject conventions of arch/arm/mach-omap2/omap4-common.c
 ARM: l2c: omap2+: get rid of init call
 ARM: l2c: omap2+: get rid of redundant cache replacement policy setting
 ARM: l2c: omap2: remove explicit non-secure access bits
 ARM: l2c: omap2: remove cache size override
 ARM: l2c: omap2: remove explicit SMI calls to enable L2 cache
 ARM: l2c: omap2: implement new write_sec method
 ARM: l2c: remove platforms/SoCs setting early BRESP
 ARM: l2c: fix register naming
 ARM: l2c: omap2: remove ES1.0 support

 ..

 If folks feel strongly about this, I can capitalize the same and post
 a v3 to help confusing fonts on certain mail clients and terminals.
 let me know if folks want me to.

 I guess no need to :)

 Looks like these still won't fix the issue we found in the
 series posted by Tomasz though. At least I'm still getting errors
 on am437x with these and the patches from Tomasz applied.

Indeed, as I figured out in the original thread about this issue,
additional patch fixing code unaffected by my series (besides changing
the condition which triggers calling it) is necessary. Namely, the
affected 4 registers need to be written using the write_sec wrapper,
instead of using writel*() directly.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 2/2] ARM: l2c: AM437x: Introduce support for cache filter programming

[Tomasz Figa]

Hi Nishanth,

2015-01-03 2:43 GMT+09:00 Nishanth Menon n...@ti.com:
 AM437x generation of processors support programming the PL310 L2Cache
 controller's address filter start and end registers using a secure
 montior service.

typo: s/montior/monitor/

[snip]

 +   base = omap4_get_l2cache_base();
 +   filter_start = (reg == L310_ADDR_FILTER_START) ? val :
 +  readl_relaxed(base + L310_ADDR_FILTER_START);
 +   filter_end = (reg == L310_ADDR_FILTER_END) ? val :
 +  readl_relaxed(base + L310_ADDR_FILTER_END);
 +   omap_smc1_2(AM43X_MON_L2X0_SETFILTER_INDEX, filter_start,
 +   filter_end);
 +   return;

I don't have any significant comments about this patch in particular,
but just noticed that you need to do read-backs here (and the typo
thanks to the spell checker of my mailing app). Maybe you should
consider switching to the .configure() API I introduced in my series?
This would let you get rid of the hardcoded static mapping.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v10 2/8] ARM: l2c: Refactor the driver to use commit-like interface

[Tomasz Figa]

Thanks a lot for investigating this, even before I could look into
splitting this.

2014-12-30 3:23 GMT+09:00 Nishanth Menon n...@ti.com:
 On 12/23/2014 04:48 AM, Marek Szyprowski wrote:

 -static void l2c310_resume(void)
 +static void l2c310_configure(void __iomem *base)
  {
 - void __iomem *base = l2x0_base;
 + unsigned revision;

 - if (!(readl_relaxed(base + L2X0_CTRL)  L2X0_CTRL_EN)) {
 - unsigned revision;
 -
 - /* restore pl310 setup */
 - writel_relaxed(l2x0_saved_regs.tag_latency,
 -base + L310_TAG_LATENCY_CTRL);
 - writel_relaxed(l2x0_saved_regs.data_latency,
 -base + L310_DATA_LATENCY_CTRL);
 - writel_relaxed(l2x0_saved_regs.filter_end,
 -base + L310_ADDR_FILTER_END);
 - writel_relaxed(l2x0_saved_regs.filter_start,
 -base + L310_ADDR_FILTER_START);
 -
 - revision = readl_relaxed(base + L2X0_CACHE_ID) 
 - L2X0_CACHE_ID_RTL_MASK;
 -
 - if (revision = L310_CACHE_ID_RTL_R2P0)
 - l2c_write_sec(l2x0_saved_regs.prefetch_ctrl, base,
 -   L310_PREFETCH_CTRL);
 - if (revision = L310_CACHE_ID_RTL_R3P0)
 - l2c_write_sec(l2x0_saved_regs.pwr_ctrl, base,
 -   L310_POWER_CTRL);
 -
 - l2c_enable(base, l2x0_saved_regs.aux_ctrl, 8);
 -
 - /* Re-enable full-line-of-zeros for Cortex-A9 */
 - if (l2x0_saved_regs.aux_ctrl  L310_AUX_CTRL_FULL_LINE_ZERO)
 - set_auxcr(get_auxcr() | BIT(3) | BIT(2) | BIT(1));
 - }
 + /* restore pl310 setup */
 + writel_relaxed(l2x0_saved_regs.tag_latency,
 +base + L310_TAG_LATENCY_CTRL);
 + writel_relaxed(l2x0_saved_regs.data_latency,
 +base + L310_DATA_LATENCY_CTRL);
 + writel_relaxed(l2x0_saved_regs.filter_end,
 +base + L310_ADDR_FILTER_END);
 + writel_relaxed(l2x0_saved_regs.filter_start,
 +base + L310_ADDR_FILTER_START);
 +

 ^^ The above change broke AM437xx. Looks like the change causes the
 following behavior difference on AM437x. For some reason, touching any
 of the above 4 registers(even with the values read from the same
 registers) causes AM437x to go beserk. Comment the 4 writes and we
 reach shell. looks like l2c310_resume is not invoked prior to this
 series. :(.. now that we reuse that logic to actually do programming,
 we start to see the problem.

Hmm, but the thing is that .configure() should not be called if the
controller is already configured, i.e. L2X0_CTRL_EN in L2X0_CTRL is
set. Maybe I missed some check somewhere. Let me reread my code I
wrote quite a long time ago and make sure.


 one option might be to write only those registers that differ from
 saved_registers (example: unmodified values dont need reprogramming).

 Looks like the following also need addressing:
 data-save is called twice (once more after l2cof_init)
 l2c310_init_fns also needs l2c310_configure
 will be nice to use l2x0_data only after we kmemdup data in __l2c_init

I'll check this.


 if you'd like to split this up in pieces, [1] might be nice - will go
 good to change the pl310, aurora etc in each chunks to enable better
 review.

Thanks a lot, the split up version will be definitely useful. Just to
make sure, the parts look quite bisectable, but have you verified that
applying the changes one by one leave the L2 cache working on OMAP?


 [1]
 https://github.com/nmenon/linux-2.6-playground/commits/temp/l2c-patch2-splitup

Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v10 2/8] ARM: l2c: Refactor the driver to use commit-like interface

[Tomasz Figa]

Nishanth, Tony,

On 24.12.2014 02:13, Nishanth Menon wrote:

On 12/23/2014 11:06 AM, Tony Lindgren wrote:

* Marek Szyprowski m.szyprow...@samsung.com [141223 02:51]:

From: Tomasz Figa t.f...@samsung.com

Certain implementations of secure hypervisors (namely the one found on
Samsung Exynos-based boards) do not provide access to individual L2C
registers. This makes the .write_sec()-based interface insufficient and
provoking ugly hacks.

This patch is first step to make the driver not rely on availability of
writes to individual registers. This is achieved by refactoring the
driver to use a commit-like operation scheme: all register values are
prepared first and stored in an instance of l2x0_regs struct and then a
single callback is responsible to flush those values to the hardware.


The first patch of the series applied things boot with no problem.
But after applying this one I get the following on am437x:

Unhandled fault: imprecise external abort (0xc06) at 0xb6f33884

Probably the same issue Nishanth mentioned.



yep - just finished the bisect... came to the same conclusion..

c8c3a07fa6a8e9b27a1658e0d305b6f7e0fa068f is the first bad commit
commit c8c3a07fa6a8e9b27a1658e0d305b6f7e0fa068f
Author: Tomasz Figa t.f...@samsung.com
Date:   Tue Dec 23 11:48:30 2014 +0100

 ARM: l2c: Refactor the driver to use commit-like interface

 Certain implementations of secure hypervisors (namely the one found on
 Samsung Exynos-based boards) do not provide access to individual L2C
 registers. This makes the .write_sec()-based interface
insufficient and
 provoking ugly hacks.

 This patch is first step to make the driver not rely on
availability of
 writes to individual registers. This is achieved by refactoring the
 driver to use a commit-like operation scheme: all register values are
 prepared first and stored in an instance of l2x0_regs struct and
then a
 single callback is responsible to flush those values to the hardware.

 Signed-off-by: Tomasz Figa t.f...@samsung.com
 Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com

:04 04 74c6c74a0dc0612d124cd759951adf2a1e4124ee
8082aabb474f8659231de744d87cd8dbd6dd79bb M  arch


$ git bisect log
git bisect start
# good: [97bf6af1f928216fd6c5a66e8a57bfa95a659672] Linux 3.19-rc1
git bisect good 97bf6af1f928216fd6c5a66e8a57bfa95a659672
# bad: [9afe195db6558621bd8bac379ed65ef121930684] ARM: dts: exynos4:
Add nodes for L2 cache controller
git bisect bad 9afe195db6558621bd8bac379ed65ef121930684
# bad: [0a89ef4dd870bbf692e30fef6c8182d7b8b42e17] ARM: l2c: Get outer
cache .write_sec callback from mach_desc only if not NULL
git bisect bad 0a89ef4dd870bbf692e30fef6c8182d7b8b42e17
# bad: [c8c3a07fa6a8e9b27a1658e0d305b6f7e0fa068f] ARM: l2c: Refactor
the driver to use commit-like interface
git bisect bad c8c3a07fa6a8e9b27a1658e0d305b6f7e0fa068f
# good: [080ab387c653b8655dc1ee790658b618399db2aa] ARM: OMAP2+: use
common l2cache initialization code
git bisect good 080ab387c653b8655dc1ee790658b618399db2aa




May I ask you (or anyone else working on OMAP) to try to figure out what 
the issue is? It is stopping L2 cache support for Exynos4 being merged 
and Exynos people don't have access to any of affected boards to do 
anything about it. After all, this is generic code, so I believe 
community should cooperate with pushing it forward. (Of course I 
understand it is a holiday season at the moment, so I don't expect any 
solution right at this moment :))


Apparently patch 1/8 solved problems with some of the boards. Could you 
check how those boards differ and look for potential causes?


Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: regression: OMAP4 (next-20141204) (bisect to: ARM: 8208/1: l2c: Refactor the driver to use commit-like)

[Tomasz Figa]

2014-12-06 1:23 GMT+09:00 Russell King - ARM Linux li...@arm.linux.org.uk:
 On Fri, Dec 05, 2014 at 10:13:51AM -0600, Nishanth Menon wrote:
 On 12/05/2014 10:10 AM, Nishanth Menon wrote:
  Case #2: Reverting the following allows boot.
 
  From next-20141204
  10df7d5 ARM: 8211/1: l2c: Add support for overriding prefetch settings
  revert this  - boot still fails
 
  d42ced0 ARM: 8210/1: l2c: Get outer cache .write_sec callback from
  mach_desc only if not NULL
  revert this  - boot still fails
 
  46b9af8 ARM: 8209/1: l2c: Add interface to ask hypervisor to configure L2C
  revert this  - boot still fails
 
  c94e325 ARM: 8208/1: l2c: Refactor the driver to use commit-like
  revert this  - boot passed (first bad commit).
 
 

 + linux-samsung soc and updated Thomaz's mail ID (gmail now).

 Given where we are in the cycle (-final likely this weekend) the only
 thing we can do right now is to drop the patch set; exynos (and mvebu)
 will have to wait another cycle until this patch set (hopefully in a
 revised form) can be merged.

Or a fix could be queued on top of this. Since (I believe) this series
has been queued for 3.19, we have 6 or 7 RC releases ahead, which
could be used for the purpose of fixing things (as they are supposed
to?).


 I think we need 8208/1 split up into smaller changes so that the cause
 of this regression can be found.

I'm afraid we need more than that.

First of all, this series has been in the wild for more than 3 months
already, without any serious changes. Need not to mention that mailing
lists and maintainers of all potentially affected platforms had been
added. It had been noted in cover letter that the only platform I (and
Marek later) could test on was Exynos and that it would be good if
somebody working with other platforms could test the patches.

Looks like nobody cared back then, so why should we care that much
now, especially that we have several RC releases ahead and we can
still fix this?

Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/4] dwc3: exynos: Add support for SCLK present on Exynos7

[Tomasz Figa]

Hi Anton,

On 13.10.2014 06:54, Anton Tikhomirov wrote:
 Hi Vivek,
 
 Exynos7 also has a separate special gate clock going to the IP
 apart from the usual AHB clock. So add support for the same.
 
 As we discussed before, Exynos7 SoCs have 7 clocks to be controlled
 by the driver. Adding only sclk is not enough. 
 

I'm quite interested in this discussion. Has it happened on mailing lists?

In general, previous SoCs also gave the possibility of controlling all
the bus clocks separately, in addition to bulk gates, but there was no
real advantage in using those, while burdening the clock tree with
numerous clocks. Isn't Exynos7 similar in this aspect?

Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 4/7] ARM: l2c: Add support for overriding prefetch settings

[Tomasz Figa]

On 24.09.2014 13:14, Mark Rutland wrote:
 On Wed, Sep 24, 2014 at 12:05:38PM +0100, Marek Szyprowski wrote:
 From: Tomasz Figa t.f...@samsung.com

 Firmware on certain boards (e.g. ODROID-U3) can leave incorrect L2C prefetch
 settings configured in registers leading to crashes if L2C is enabled
 without overriding them. This patch introduces bindings to enable
 prefetch settings to be specified from DT and necessary support in the
 driver.

 Signed-off-by: Tomasz Figa t.f...@samsung.com
 Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
 ---
  Documentation/devicetree/bindings/arm/l2cc.txt | 10 +++
  arch/arm/mm/cache-l2x0.c   | 39 
 ++
  2 files changed, 49 insertions(+)

 diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt 
 b/Documentation/devicetree/bindings/arm/l2cc.txt
 index af527ee111c2..3443d2d76788 100644
 --- a/Documentation/devicetree/bindings/arm/l2cc.txt
 +++ b/Documentation/devicetree/bindings/arm/l2cc.txt
 @@ -47,6 +47,16 @@ Optional properties:
  - cache-id-part: cache id part number to be used if it is not present
on hardware
  - wt-override: If present then L2 is forced to Write through mode
 +- arm,double-linefill : Override double linefill enable setting. Enable if
 +  non-zero, disable if zero.
 +- arm,double-linefill-incr : Override double linefill on INCR read. Enable
 +  if non-zero, disable if zero.
 +- arm,double-linefill-wrap : Override double linefill on WRAP read. Enable
 +  if non-zero, disable if zero.
 +- arm,prefetch-drop : Override prefetch drop enable setting. Enable if 
 non-zero,
 +  disable if zero.
 
 I'm not too keen on tristate properties. Is this level of flexibility
 really required?
 
 What exact overrides do you need for boards you know of? Why do these
 cause crashes if not overridden?

Well, this is all thanks to broken firmware, which doesn't initialize
those values properly on certain systems and they must be overridden. On
the other side, there are systems with firmware that does it correctly
and for those the boot defaults should be preferred. I don't see any
other reasonable option of handling this.

As for why they cause crashes, I'm not an expert if it is about L2C
internals, so I can't really tell, but apparently the cache can work
correctly only on certain settings.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 5/7] ARM: EXYNOS: Add .write_sec outer cache callback for L2C-310

[Tomasz Figa]

 +static void exynos_l2_write_sec(unsigned long val, unsigned reg)
 +{
 +switch (reg) {
 +case L2X0_CTRL:
 +if (val  L2X0_CTRL_EN)
 +exynos_smc(SMC_CMD_L2X0INVALL, 0, 0, 0);
 
 If we're calling this with the cache already enabled, presumably you're
 doing this to cover the case where we're disabling the cache.

Can we ever call this with L2X0_CTRL_EN set in val, while the cache is
already enabled?

Anyway, calling of this firmware operation is necessary before enabling
the cache and this code is here to cover this requirement. Whether this
function simply invalidates the cache or does something else is unknown
to me, as all the information I got is that this needs to be done.

 
 1. Do you really want to *invalidate* the L2 cache, discarding its
contents?
 2. Don't you think that... if you needed something like this here, then
it could be a defficiency in the common code?
 
 If (2) doesn't apply, then should be a comment here why this is needed.
 

This is a quirk specific to Exynos firmware and I suspect it doesn't
even have anything to do with cache invalidation, but rather some
internal logic inside the firmware.

I agree, though, that a comment might be useful here.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 6/7] ARM: EXYNOS: Add support for non-secure L2X0 resume

[Tomasz Figa]

On 15.09.2014 11:03, Russell King - ARM Linux wrote:
 diff --git a/arch/arm/mach-exynos/firmware.c 
 b/arch/arm/mach-exynos/firmware.c
 index 554b350..71bcfbd 100644
 --- a/arch/arm/mach-exynos/firmware.c
 +++ b/arch/arm/mach-exynos/firmware.c
 @@ -102,7 +102,9 @@ static int exynos_suspend(void)
  writel(EXYNOS_SLEEP_MAGIC, sysram_ns_base_addr + EXYNOS_BOOT_FLAG);
  writel(virt_to_phys(exynos_cpu_resume_ns),
  sysram_ns_base_addr + EXYNOS_BOOT_ADDR);
 -
 +#ifdef CONFIG_CACHE_L2X0
 +l2x0_regs_phys = virt_to_phys(l2x0_saved_regs);
 +#endif
 
 NAK.  Please look at how arch/arm/mm/l2c-l2x0-resume.S gets the address
 of this structure in assembly code.  The name of this variable is crap
 in any case.  It's not the registers, it's the saved registers.  So even
 more reason to kill this abomination, which incidentally, I've already
 killed off once before in the exynos code.
 

Right. The way l2c-l2x0-resume.S does this is much better. Somehow I
overlooked it when implementing this.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 0/7] Enable L2 cache support on Exynos4210/4x12 SoCs

[Tomasz Figa]

Russell, Olof, Kukjin,

On 26.08.2014 16:17, Tomasz Figa wrote:
 This series intends to add support for L2 cache on Exynos4 SoCs on boards
 running under secure firmware, which requires certain initialization steps
 to be done with help of firmware, as selected registers are writable only
 from secure mode.

I assume that since there has not been any input for almost 3 weeks,
this series can be merged. How should we proceed with it? Note that it
touches both core ARM and Exynos-specific areas and depends on another
Exynos-specific series [1].

[1] [PATCH v3 0/5] Firmware-assisted suspend/resume of Exynos SoCs
(https://lkml.org/lkml/2014/8/26/445)

Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] clk: don't use __initconst for non-const arrays

[Tomasz Figa]

[adding Sylwester and removing my samsung.com e-mail which is no longer
valid]

On 11.09.2014 23:04, Uwe Kleine-König wrote:
 The statement
 
   static const char *name[];
 
 defines a modifiable array of pointers to constant chars. That is
 
   *name[0] = 'f';
 
 is forbidden, but
 
   name[0] = f;
 
 is not. So marking an array that is defined as above with __initconst is
 wrong. Either an additional const must be added such that the whole
 definition reads:
 
   static const char *const name[] __initconst;
 
 or where this is not possible __initdata must be used.
 
 Signed-off-by: Uwe Kleine-König u.kleine-koe...@pengutronix.de
 ---
  drivers/clk/hisilicon/clk-hix5hd2.c |  6 ++--
  drivers/clk/mxs/clk-imx23.c | 12 
  drivers/clk/mxs/clk-imx28.c | 18 ++--
  drivers/clk/rockchip/clk.h  |  2 +-
  drivers/clk/samsung/clk-s5pv210.c   | 56 
 ++---

For drivers/clk/samsung/*

Acked-by: Tomasz Figa tomasz.f...@gmail.com

Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 3/7] ARM: l2c: Get outer cache .write_sec callback from mach_desc only if not NULL

[Tomasz Figa]

Certain platforms (i.e. Exynos) might need to set .write_sec callback
from firmware initialization which is happenning in .init_early callback
of machine descriptor. However current code will overwrite the pointer
with whatever is present in machine descriptor, even though it can be
already set earlier. This patch fixes this by making the assignment
conditional, depending on whether current .write_sec callback is NULL.

Signed-off-by: Tomasz Figa t.f...@samsung.com
---
 arch/arm/kernel/irq.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kernel/irq.c b/arch/arm/kernel/irq.c
index 2c42576..e7383b9 100644
--- a/arch/arm/kernel/irq.c
+++ b/arch/arm/kernel/irq.c
@@ -125,7 +125,8 @@ void __init init_IRQ(void)
 
if (IS_ENABLED(CONFIG_OF)  IS_ENABLED(CONFIG_CACHE_L2X0) 
(machine_desc-l2c_aux_mask || machine_desc-l2c_aux_val)) {
-   outer_cache.write_sec = machine_desc-l2c_write_sec;
+   if (!outer_cache.write_sec)
+   outer_cache.write_sec = machine_desc-l2c_write_sec;
ret = l2x0_of_init(machine_desc-l2c_aux_val,
   machine_desc-l2c_aux_mask);
if (ret)
-- 
2.0.4

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 0/7] Enable L2 cache support on Exynos4210/4x12 SoCs

[Tomasz Figa]

This series intends to add support for L2 cache on Exynos4 SoCs on boards
running under secure firmware, which requires certain initialization steps
to be done with help of firmware, as selected registers are writable only
from secure mode.

First four patches extend existing support for secure write in L2C driver
to account for design of secure firmware running on Exynos. Namely:
 1) direct read access to certain registers is needed on Exynos, because
secure firmware calls set several registers at once,
 2) not all boards are running secure firmware, so .write_sec callback
needs to be installed in Exynos firmware ops initialization code,
 3) write access to {DATA,TAG}_LATENCY_CTRL registers fron non-secure world
is not allowed and so must use l2c_write_sec as well,
 4) on certain boards, default value of prefetch register is incorrect
and must be overridden at L2C initialization.
For boards running with firmware that provides access to individual
L2C registers this series should introduce no functional changes. However
since the driver is widely used on other platforms I'd like to kindly ask
any interested people for testing.

Further three patches add implementation of .write_sec and .configure
callbacks for Exynos secure firmware and necessary DT nodes to enable
L2 cache.

Changes in this version tested on Exynos4412-based TRATS2 board (with secure
firmware). There should be no functional change for Exynos boards running
without secure firmware. I do not have access to affected non-Exynos boards,
so I could not test on them.

Depends on:
 - [PATCH v3 0/5] Firmware-assisted suspend/resume of Exynos SoCs
   (https://lkml.org/lkml/2014/8/26/445)

Changes since v3:
(https://lkml.org/lkml/2014/7/17/600)
 - fixed issues with references to initdata on resume path by creating
   a copy of affected structure (pointed out by Russell King),
 - fixed unnecessary full reconfiguration of L2C controller on resume
   (configuration is already determined after initialization, so the
only thing to do is to push those values to the controller),
 - rebased on next-20140717 tag of linux-next tree and last versions
   of dependencies.

Changes since v2:
(https://lkml.org/lkml/2014/6/25/416)
 - refactored L2C driver to use commit-like interface and make it no longer
   depend on availability of writes to individual registers,
 - moved L2C resume to assembly code, because doing it later makes some
   systems unstable - this is also needed for deeper cpuidle modes,
 - dropped unnecessary patch hacking around the .write_sec interface,
 - dropped patch making the driver use l2c_write_sec() for LATENCY_CTRL
   registers as Exynos is no longer affected and I'm not aware of any
   reports that this is also needed on other platforms (can be applied
   separately if it turns out to be so),
 - rebased onto next-20140717 tag of linux-next tree.

Changes since v1:
(https://www.mail-archive.com/linux-omap@vger.kernel.org/msg106323.html)
 - rebased onto for-next branch of linux-samsung tree,
 - changed argument order of outer_cache.write_sec() callback to match
   l2c_write_sec() function in cache-l2x0.c,
 - added support of overriding of prefetch settings to work around incorrect
   default settings on certain Exynos4x12-based boards,
 - added call to firmware to invalidate whole L2 cache before setting enable
   bit in L2C control register (required by Exynos secure firmware).

Tomasz Figa (7):
  ARM: l2c: Refactor the driver to use commit-like interface
  ARM: l2c: Add interface to ask hypervisor to configure L2C
  ARM: l2c: Get outer cache .write_sec callback from mach_desc only if
not NULL
  ARM: l2c: Add support for overriding prefetch settings
  ARM: EXYNOS: Add .write_sec outer cache callback for L2C-310
  ARM: EXYNOS: Add support for non-secure L2X0 resume
  ARM: dts: exynos4: Add nodes for L2 cache controller

 Documentation/devicetree/bindings/arm/l2cc.txt |  10 +
 arch/arm/boot/dts/exynos4210.dtsi  |   9 +
 arch/arm/boot/dts/exynos4x12.dtsi  |  14 ++
 arch/arm/include/asm/outercache.h  |   3 +
 arch/arm/kernel/irq.c  |   3 +-
 arch/arm/mach-exynos/common.h  |   1 +
 arch/arm/mach-exynos/firmware.c|  42 +++-
 arch/arm/mach-exynos/sleep.S   |  41 
 arch/arm/mm/cache-l2x0.c   | 255 -
 9 files changed, 281 insertions(+), 97 deletions(-)

-- 
2.0.4

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 6/7] ARM: EXYNOS: Add support for non-secure L2X0 resume

[Tomasz Figa]

On Exynos SoCs it is necessary to resume operation of L2C early in
assembly code, because otherwise certain systems will crash. This patch
adds necessary code to non-secure resume handler.

Signed-off-by: Tomasz Figa t.f...@samsung.com
---
 arch/arm/mach-exynos/common.h   |  1 +
 arch/arm/mach-exynos/firmware.c |  4 +++-
 arch/arm/mach-exynos/sleep.S| 41 +
 3 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-exynos/common.h b/arch/arm/mach-exynos/common.h
index c218200..e88c0f9 100644
--- a/arch/arm/mach-exynos/common.h
+++ b/arch/arm/mach-exynos/common.h
@@ -113,6 +113,7 @@ IS_SAMSUNG_CPU(exynos5800, EXYNOS5800_SOC_ID, 
EXYNOS5_SOC_MASK)
 
 extern u32 cp15_save_diag;
 extern u32 cp15_save_power;
+extern unsigned long l2x0_regs_phys;
 
 extern void __iomem *sysram_ns_base_addr;
 extern void __iomem *sysram_base_addr;
diff --git a/arch/arm/mach-exynos/firmware.c b/arch/arm/mach-exynos/firmware.c
index 554b350..71bcfbd 100644
--- a/arch/arm/mach-exynos/firmware.c
+++ b/arch/arm/mach-exynos/firmware.c
@@ -102,7 +102,9 @@ static int exynos_suspend(void)
writel(EXYNOS_SLEEP_MAGIC, sysram_ns_base_addr + EXYNOS_BOOT_FLAG);
writel(virt_to_phys(exynos_cpu_resume_ns),
sysram_ns_base_addr + EXYNOS_BOOT_ADDR);
-
+#ifdef CONFIG_CACHE_L2X0
+   l2x0_regs_phys = virt_to_phys(l2x0_saved_regs);
+#endif
return cpu_suspend(0, exynos_cpu_suspend);
 }
 
diff --git a/arch/arm/mach-exynos/sleep.S b/arch/arm/mach-exynos/sleep.S
index e3c3730..b8ce8f0 100644
--- a/arch/arm/mach-exynos/sleep.S
+++ b/arch/arm/mach-exynos/sleep.S
@@ -16,6 +16,8 @@
  */
 
 #include linux/linkage.h
+#include asm/asm-offsets.h
+#include asm/hardware/cache-l2x0.h
 #include smc.h
 
 #define CPU_MASK   0xff00
@@ -74,6 +76,40 @@ ENTRY(exynos_cpu_resume_ns)
mov r0, #SMC_CMD_C15RESUME
dsb
smc #0
+#ifdef CONFIG_CACHE_L2X0
+   adr r0, l2x0_regs_phys
+   ldr r0, [r0]
+   cmp r0, #0
+   beq skip_l2x0
+
+   ldr r1, [r0, #L2X0_R_PHY_BASE]
+   ldr r2, [r1, #L2X0_CTRL]
+   tst r2, #0x1
+   bne skip_l2x0
+
+   ldr r1, [r0, #L2X0_R_TAG_LATENCY]
+   ldr r2, [r0, #L2X0_R_DATA_LATENCY]
+   ldr r3, [r0, #L2X0_R_PREFETCH_CTRL]
+   mov r0, #SMC_CMD_L2X0SETUP1
+   smc #0
+
+   /* Reload saved regs pointer because smc corrupts registers. */
+   adr r0, l2x0_regs_phys
+   ldr r0, [r0]
+
+   ldr r1, [r0, #L2X0_R_PWR_CTRL]
+   ldr r2, [r0, #L2X0_R_AUX_CTRL]
+   mov r0, #SMC_CMD_L2X0SETUP2
+   smc #0
+
+   mov r0, #SMC_CMD_L2X0INVALL
+   smc #0
+
+   mov r1, #1
+   mov r0, #SMC_CMD_L2X0CTRL
+   smc #0
+skip_l2x0:
+#endif /* CONFIG_CACHE_L2X0 */
 skip_cp15:
b   cpu_resume
 ENDPROC(exynos_cpu_resume_ns)
@@ -83,3 +119,8 @@ cp15_save_diag:
.globl cp15_save_power
 cp15_save_power:
.long   0   @ cp15 power control
+#ifdef CONFIG_CACHE_L2X0
+   .globl l2x0_regs_phys
+l2x0_regs_phys:
+   .long   0   @ phys address of l2x0 save struct
+#endif /* CONFIG_CACHE_L2X0 */
-- 
2.0.4

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 1/7] ARM: l2c: Refactor the driver to use commit-like interface

[Tomasz Figa]

Certain implementations of secure hypervisors (namely the one found on
Samsung Exynos-based boards) do not provide access to individual L2C
registers. This makes the .write_sec()-based interface insufficient and
provoking ugly hacks.

This patch is first step to make the driver not rely on availability of
writes to individual registers. This is achieved by refactoring the
driver to use a commit-like operation scheme: all register values are
prepared first and stored in an instance of l2x0_regs struct and then a
single callback is responsible to flush those values to the hardware.

Signed-off-by: Tomasz Figa t.f...@samsung.com
---
 arch/arm/mm/cache-l2x0.c | 210 ++-
 1 file changed, 115 insertions(+), 95 deletions(-)

diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 5f2c988..b073563 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -40,12 +40,14 @@ struct l2c_init_data {
void (*enable)(void __iomem *, u32, unsigned);
void (*fixup)(void __iomem *, u32, struct outer_cache_fns *);
void (*save)(void __iomem *);
+   void (*configure)(void __iomem *);
struct outer_cache_fns outer_cache;
 };
 
 #define CACHE_LINE_SIZE32
 
 static void __iomem *l2x0_base;
+static const struct l2c_init_data *l2x0_data;
 static DEFINE_RAW_SPINLOCK(l2x0_lock);
 static u32 l2x0_way_mask;  /* Bitmask of active ways */
 static u32 l2x0_size;
@@ -105,6 +107,14 @@ static inline void l2c_unlock(void __iomem *base, unsigned 
num)
}
 }
 
+static void l2c_configure(void __iomem *base)
+{
+   if (l2x0_data-configure)
+   l2x0_data-configure(base);
+
+   l2c_write_sec(l2x0_saved_regs.aux_ctrl, base, L2X0_AUX_CTRL);
+}
+
 /*
  * Enable the L2 cache controller.  This function must only be
  * called when the cache controller is known to be disabled.
@@ -113,7 +123,12 @@ static void l2c_enable(void __iomem *base, u32 aux, 
unsigned num_lock)
 {
unsigned long flags;
 
-   l2c_write_sec(aux, base, L2X0_AUX_CTRL);
+   /* Do not touch the controller if already enabled. */
+   if (readl_relaxed(base + L2X0_CTRL)  L2X0_CTRL_EN)
+   return;
+
+   l2x0_saved_regs.aux_ctrl = aux;
+   l2c_configure(base);
 
l2c_unlock(base, num_lock);
 
@@ -207,6 +222,11 @@ static void l2c_save(void __iomem *base)
l2x0_saved_regs.aux_ctrl = readl_relaxed(l2x0_base + L2X0_AUX_CTRL);
 }
 
+static void l2c_resume(void)
+{
+   l2c_enable(l2x0_base, l2x0_saved_regs.aux_ctrl, l2x0_data-num_lock);
+}
+
 /*
  * L2C-210 specific code.
  *
@@ -287,14 +307,6 @@ static void l2c210_sync(void)
__l2c210_cache_sync(l2x0_base);
 }
 
-static void l2c210_resume(void)
-{
-   void __iomem *base = l2x0_base;
-
-   if (!(readl_relaxed(base + L2X0_CTRL)  L2X0_CTRL_EN))
-   l2c_enable(base, l2x0_saved_regs.aux_ctrl, 1);
-}
-
 static const struct l2c_init_data l2c210_data __initconst = {
.type = L2C-210,
.way_size_0 = SZ_8K,
@@ -308,7 +320,7 @@ static const struct l2c_init_data l2c210_data __initconst = 
{
.flush_all = l2c210_flush_all,
.disable = l2c_disable,
.sync = l2c210_sync,
-   .resume = l2c210_resume,
+   .resume = l2c_resume,
},
 };
 
@@ -465,7 +477,7 @@ static const struct l2c_init_data l2c220_data = {
.flush_all = l2c220_flush_all,
.disable = l2c_disable,
.sync = l2c220_sync,
-   .resume = l2c210_resume,
+   .resume = l2c_resume,
},
 };
 
@@ -614,39 +626,29 @@ static void __init l2c310_save(void __iomem *base)
L310_POWER_CTRL);
 }
 
-static void l2c310_resume(void)
+static void l2c310_configure(void __iomem *base)
 {
-   void __iomem *base = l2x0_base;
+   unsigned revision;
 
-   if (!(readl_relaxed(base + L2X0_CTRL)  L2X0_CTRL_EN)) {
-   unsigned revision;
-
-   /* restore pl310 setup */
-   writel_relaxed(l2x0_saved_regs.tag_latency,
-  base + L310_TAG_LATENCY_CTRL);
-   writel_relaxed(l2x0_saved_regs.data_latency,
-  base + L310_DATA_LATENCY_CTRL);
-   writel_relaxed(l2x0_saved_regs.filter_end,
-  base + L310_ADDR_FILTER_END);
-   writel_relaxed(l2x0_saved_regs.filter_start,
-  base + L310_ADDR_FILTER_START);
-
-   revision = readl_relaxed(base + L2X0_CACHE_ID) 
-   L2X0_CACHE_ID_RTL_MASK;
-
-   if (revision = L310_CACHE_ID_RTL_R2P0)
-   l2c_write_sec(l2x0_saved_regs.prefetch_ctrl, base,
- L310_PREFETCH_CTRL);
-   if (revision = L310_CACHE_ID_RTL_R3P0)
-   l2c_write_sec

[PATCH v4 7/7] ARM: dts: exynos4: Add nodes for L2 cache controller

[Tomasz Figa]

This patch adds device tree nodes for L2 cache controller present on
Exynos4 SoCs.

Signed-off-by: Tomasz Figa t.f...@samsung.com
---
 arch/arm/boot/dts/exynos4210.dtsi |  9 +
 arch/arm/boot/dts/exynos4x12.dtsi | 14 ++
 2 files changed, 23 insertions(+)

diff --git a/arch/arm/boot/dts/exynos4210.dtsi 
b/arch/arm/boot/dts/exynos4210.dtsi
index 807bb5b..8a182c4 100644
--- a/arch/arm/boot/dts/exynos4210.dtsi
+++ b/arch/arm/boot/dts/exynos4210.dtsi
@@ -64,6 +64,15 @@
reg = 0x10023CA0 0x20;
};
 
+   l2c: l2-cache-controller@10502000 {
+   compatible = arm,pl310-cache;
+   reg = 0x10502000 0x1000;
+   cache-unified;
+   cache-level = 2;
+   arm,tag-latency = 2 2 1;
+   arm,data-latency = 2 2 1;
+   };
+
gic: interrupt-controller@1049 {
cpu-offset = 0x8000;
};
diff --git a/arch/arm/boot/dts/exynos4x12.dtsi 
b/arch/arm/boot/dts/exynos4x12.dtsi
index 861bb91..c7adfd6 100644
--- a/arch/arm/boot/dts/exynos4x12.dtsi
+++ b/arch/arm/boot/dts/exynos4x12.dtsi
@@ -54,6 +54,20 @@
reg = 0x10023CA0 0x20;
};
 
+   l2c: l2-cache-controller@10502000 {
+   compatible = arm,pl310-cache;
+   reg = 0x10502000 0x1000;
+   cache-unified;
+   cache-level = 2;
+   arm,tag-latency = 2 2 1;
+   arm,data-latency = 3 2 1;
+   arm,double-linefill = 1;
+   arm,double-linefill-incr = 0;
+   arm,double-linefill-wrap = 1;
+   arm,prefetch-drop = 1;
+   arm,prefetch-offset = 7;
+   };
+
clock: clock-controller@1003 {
compatible = samsung,exynos4412-clock;
reg = 0x1003 0x2;
-- 
2.0.4

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 5/7] ARM: EXYNOS: Add .write_sec outer cache callback for L2C-310

[Tomasz Figa]

Exynos4 SoCs equipped with an L2C-310 cache controller and running under
secure firmware require certain registers of aforementioned IP to be
accessed only from secure mode. This means that SMC calls are required
for certain register writes. To handle this, an implementation of
.write_sec and .configure callbacks is provided by this patch.

Signed-off-by: Tomasz Figa t.f...@samsung.com
---
 arch/arm/mach-exynos/firmware.c | 38 ++
 1 file changed, 38 insertions(+)

diff --git a/arch/arm/mach-exynos/firmware.c b/arch/arm/mach-exynos/firmware.c
index f5e626d..554b350 100644
--- a/arch/arm/mach-exynos/firmware.c
+++ b/arch/arm/mach-exynos/firmware.c
@@ -17,6 +17,7 @@
 #include asm/cacheflush.h
 #include asm/cputype.h
 #include asm/firmware.h
+#include asm/hardware/cache-l2x0.h
 #include asm/suspend.h
 
 #include mach/map.h
@@ -120,6 +121,31 @@ static const struct firmware_ops exynos_firmware_ops = {
.resume = exynos_resume,
 };
 
+static void exynos_l2_write_sec(unsigned long val, unsigned reg)
+{
+   switch (reg) {
+   case L2X0_CTRL:
+   if (val  L2X0_CTRL_EN)
+   exynos_smc(SMC_CMD_L2X0INVALL, 0, 0, 0);
+   exynos_smc(SMC_CMD_L2X0CTRL, val, 0, 0);
+   break;
+
+   case L2X0_DEBUG_CTRL:
+   exynos_smc(SMC_CMD_L2X0DEBUG, val, 0, 0);
+   break;
+
+   default:
+   WARN_ONCE(1, %s: ignoring write to reg 0x%x\n, __func__, reg);
+   }
+}
+
+static void exynos_l2_configure(const struct l2x0_regs *regs)
+{
+   exynos_smc(SMC_CMD_L2X0SETUP1, regs-tag_latency, regs-data_latency,
+   regs-prefetch_ctrl);
+   exynos_smc(SMC_CMD_L2X0SETUP2, regs-pwr_ctrl, regs-aux_ctrl, 0);
+}
+
 void __init exynos_firmware_init(void)
 {
struct device_node *nd;
@@ -139,4 +165,16 @@ void __init exynos_firmware_init(void)
pr_info(Running under secure firmware.\n);
 
register_firmware_ops(exynos_firmware_ops);
+
+   /*
+* Exynos 4 SoCs (based on Cortex A9 and equipped with L2C-310),
+* running under secure firmware, require certain registers of L2
+* cache controller to be written in secure mode. Here .write_sec
+* callback is provided to perform necessary SMC calls.
+*/
+   if (IS_ENABLED(CONFIG_CACHE_L2X0)
+read_cpuid_part() == ARM_CPU_PART_CORTEX_A9) {
+   outer_cache.write_sec = exynos_l2_write_sec;
+   outer_cache.configure = exynos_l2_configure;
+   }
 }
-- 
2.0.4

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 4/7] ARM: l2c: Add support for overriding prefetch settings

[Tomasz Figa]

Firmware on certain boards (e.g. ODROID-U3) can leave incorrect L2C prefetch
settings configured in registers leading to crashes if L2C is enabled
without overriding them. This patch introduces bindings to enable
prefetch settings to be specified from DT and necessary support in the
driver.

Signed-off-by: Tomasz Figa t.f...@samsung.com
---
 Documentation/devicetree/bindings/arm/l2cc.txt | 10 +++
 arch/arm/mm/cache-l2x0.c   | 39 ++
 2 files changed, 49 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt 
b/Documentation/devicetree/bindings/arm/l2cc.txt
index af527ee..3443d2d 100644
--- a/Documentation/devicetree/bindings/arm/l2cc.txt
+++ b/Documentation/devicetree/bindings/arm/l2cc.txt
@@ -47,6 +47,16 @@ Optional properties:
 - cache-id-part: cache id part number to be used if it is not present
   on hardware
 - wt-override: If present then L2 is forced to Write through mode
+- arm,double-linefill : Override double linefill enable setting. Enable if
+  non-zero, disable if zero.
+- arm,double-linefill-incr : Override double linefill on INCR read. Enable
+  if non-zero, disable if zero.
+- arm,double-linefill-wrap : Override double linefill on WRAP read. Enable
+  if non-zero, disable if zero.
+- arm,prefetch-drop : Override prefetch drop enable setting. Enable if 
non-zero,
+  disable if zero.
+- arm,prefetch-offset : Override prefetch offset value. Valid values are
+  0-7, 15, 23, and 31.
 
 Example:
 
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 84c6c55..af90a6f 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -1059,6 +1059,8 @@ static void __init l2c310_of_parse(const struct 
device_node *np,
u32 data[3] = { 0, 0, 0 };
u32 tag[3] = { 0, 0, 0 };
u32 filter[2] = { 0, 0 };
+   u32 prefetch;
+   u32 val;
 
of_property_read_u32_array(np, arm,tag-latency, tag, ARRAY_SIZE(tag));
if (tag[0]  tag[1]  tag[2])
@@ -1083,6 +1085,43 @@ static void __init l2c310_of_parse(const struct 
device_node *np,
l2x0_saved_regs.filter_start = (filter[0]  ~(SZ_1M - 1))
| L310_ADDR_FILTER_EN;
}
+
+   prefetch = l2x0_saved_regs.prefetch_ctrl;
+
+   if (!of_property_read_u32(np, arm,double-linefill, val)) {
+   if (val)
+   prefetch |= L310_PREFETCH_CTRL_DBL_LINEFILL;
+   else
+   prefetch = ~L310_PREFETCH_CTRL_DBL_LINEFILL;
+   }
+
+   if (!of_property_read_u32(np, arm,double-linefill-incr, val)) {
+   if (val)
+   prefetch |= L310_PREFETCH_CTRL_DBL_LINEFILL_INCR;
+   else
+   prefetch = ~L310_PREFETCH_CTRL_DBL_LINEFILL_INCR;
+   }
+
+   if (!of_property_read_u32(np, arm,double-linefill-wrap, val)) {
+   if (!val)
+   prefetch |= L310_PREFETCH_CTRL_DBL_LINEFILL_WRAP;
+   else
+   prefetch = ~L310_PREFETCH_CTRL_DBL_LINEFILL_WRAP;
+   }
+
+   if (!of_property_read_u32(np, arm,prefetch-drop, val)) {
+   if (val)
+   prefetch |= L310_PREFETCH_CTRL_PREFETCH_DROP;
+   else
+   prefetch = ~L310_PREFETCH_CTRL_PREFETCH_DROP;
+   }
+
+   if (!of_property_read_u32(np, arm,prefetch-offset, val)) {
+   prefetch = ~L310_PREFETCH_CTRL_OFFSET_MASK;
+   prefetch |= val  L310_PREFETCH_CTRL_OFFSET_MASK;
+   }
+
+   l2x0_saved_regs.prefetch_ctrl = prefetch;
 }
 
 static const struct l2c_init_data of_l2c310_data __initconst = {
-- 
2.0.4

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 2/7] ARM: l2c: Add interface to ask hypervisor to configure L2C

[Tomasz Figa]

Because certain secure hypervisor do not allow writes to individual L2C
registers, but rather expect set of parameters to be passed as argument
to secure monitor calls, there is a need to provide an interface for the
L2C driver to ask the firmware to configure the hardware according to
specified parameters. This patch adds such.

Signed-off-by: Tomasz Figa t.f...@samsung.com
---
 arch/arm/include/asm/outercache.h | 3 +++
 arch/arm/mm/cache-l2x0.c  | 6 ++
 2 files changed, 9 insertions(+)

diff --git a/arch/arm/include/asm/outercache.h 
b/arch/arm/include/asm/outercache.h
index 891a56b..563b92f 100644
--- a/arch/arm/include/asm/outercache.h
+++ b/arch/arm/include/asm/outercache.h
@@ -23,6 +23,8 @@
 
 #include linux/types.h
 
+struct l2x0_regs;
+
 struct outer_cache_fns {
void (*inv_range)(unsigned long, unsigned long);
void (*clean_range)(unsigned long, unsigned long);
@@ -36,6 +38,7 @@ struct outer_cache_fns {
 
/* This is an ARM L2C thing */
void (*write_sec)(unsigned long, unsigned);
+   void (*configure)(const struct l2x0_regs *);
 };
 
 extern struct outer_cache_fns outer_cache;
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index b073563..84c6c55 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -109,6 +109,11 @@ static inline void l2c_unlock(void __iomem *base, unsigned 
num)
 
 static void l2c_configure(void __iomem *base)
 {
+   if (outer_cache.configure) {
+   outer_cache.configure(l2x0_saved_regs);
+   return;
+   }
+
if (l2x0_data-configure)
l2x0_data-configure(base);
 
@@ -909,6 +914,7 @@ static int __init __l2c_init(const struct l2c_init_data 
*data,
 
fns = data-outer_cache;
fns.write_sec = outer_cache.write_sec;
+   fns.configure = outer_cache.configure;
if (data-fixup)
data-fixup(l2x0_base, cache_id, fns);
 
-- 
2.0.4

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/7] ARM: l2c: Refactor the driver to use commit-like interface

[Tomasz Figa]

On 03.08.2014 00:09, Russell King - ARM Linux wrote:
 On Thu, Jul 17, 2014 at 06:38:56PM +0200, Tomasz Figa wrote:
 Certain implementations of secure hypervisors (namely the one found on
 Samsung Exynos-based boards) do not provide access to individual L2C
 registers. This makes the .write_sec()-based interface insufficient and
 provoking ugly hacks.

 This patch is first step to make the driver not rely on availability of
 writes to individual registers. This is achieved by refactoring the
 driver to use a commit-like operation scheme: all register values are
 prepared first and stored in an instance of l2x0_regs struct and then a
 single callback is responsible to flush those values to the hardware.
 
 This isn't going to work very well...
 
 +static const struct l2c_init_data *l2x0_data;
 
 So you keep a pointer to the init data...
 
 +static void l2c_resume(void)
 +{
 +l2x0_data-enable(l2x0_base, l2x0_saved_regs.aux_ctrl,
 +l2x0_data-num_lock);
 
 which you dereference at resume time...
 
  static const struct l2c_init_data l2c210_data __initconst = {
 
 but the structures which get assigned to the pointer are marked __initconst.

Good catch. The code was tested on Exynos which requires the cache to be
resumed from early assembly code and so I did not hit issues caused by this.

Proposed solution: kmemdup() in init, so that only used data remain in
memory and the structures can be kept __initconst.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 0/7] Enable L2 cache support on Exynos4210/4x12 SoCs

[Tomasz Figa]

On 17.07.2014 18:38, Tomasz Figa wrote:
 This series intends to add support for L2 cache on Exynos4 SoCs on boards
 running under secure firmware, which requires certain initialization steps
 to be done with help of firmware, as selected registers are writable only
 from secure mode.
 
 First four patches extend existing support for secure write in L2C driver
 to account for design of secure firmware running on Exynos. Namely:
  1) direct read access to certain registers is needed on Exynos, because
 secure firmware calls set several registers at once,
  2) not all boards are running secure firmware, so .write_sec callback
 needs to be installed in Exynos firmware ops initialization code,
  3) write access to {DATA,TAG}_LATENCY_CTRL registers fron non-secure world
 is not allowed and so must use l2c_write_sec as well,
  4) on certain boards, default value of prefetch register is incorrect
 and must be overridden at L2C initialization.
 For boards running with firmware that provides access to individual
 L2C registers this series should introduce no functional changes. However
 since the driver is widely used on other platforms I'd like to kindly ask
 any interested people for testing.
 
 Further two patches add impelmentation of .write_sec for Exynos secure
 firmware and necessary DT nodes to enable L2 cache.
 
 Tested on Exynos4210-based Universal C210 and Trats (both without secure
 firmware) and Exynos4412-based TRATS2 and ODROID-U3 boards (both with secure
 firmware).
 
 Depends on:
  - [PATCH] ARM: make it easier to check the CPU part number correctly
(http://thread.gmane.org/gmane.linux.ports.arm.kernel/335126
 already in linux-next)
  - [PATCH v2 0/2] Firmware-assisted suspend/resume of Exynos SoCs
(https://lkml.org/lkml/2014/7/17/431)
 
 Changes since v2:
 (https://lkml.org/lkml/2014/6/25/416)
  - refactored L2C driver to use commit-like interface and make it no longer
depend on availability of writes to individual registers,
  - moved L2C resume to assembly code, because doing it later makes some
systems unstable - this is also needed for deeper cpuidle modes,
  - dropped unnecessary patch hacking around the .write_sec interface,
  - dropped patch making the driver use l2c_write_sec() for LATENCY_CTRL
registers as Exynos is no longer affected and I'm not aware of any
reports that this is also needed on other platforms (can be applied
separately if it turns out to be so),
  - rebased onto next-20140717 tag of linux-next tree.
 
 Changes since v1:
 (https://www.mail-archive.com/linux-omap@vger.kernel.org/msg106323.html)
  - rebased onto for-next branch of linux-samsung tree,
  - changed argument order of outer_cache.write_sec() callback to match
l2c_write_sec() function in cache-l2x0.c,
  - added support of overriding of prefetch settings to work around incorrect
default settings on certain Exynos4x12-based boards,
  - added call to firmware to invalidate whole L2 cache before setting enable
bit in L2C control register (required by Exynos secure firmware).
 
 Tomasz Figa (7):
   ARM: l2c: Refactor the driver to use commit-like interface
   ARM: l2c: Add interface to ask hypervisor to configure L2C
   ARM: l2c: Get outer cache .write_sec callback from mach_desc only if
 not NULL
   ARM: l2c: Add support for overriding prefetch settings
   ARM: EXYNOS: Add .write_sec outer cache callback for L2C-310
   ARM: EXYNOS: Add support for non-secure L2X0 resume
   ARM: dts: exynos4: Add nodes for L2 cache controller
 
  Documentation/devicetree/bindings/arm/l2cc.txt |  10 +
  arch/arm/boot/dts/exynos4210.dtsi  |   9 +
  arch/arm/boot/dts/exynos4x12.dtsi  |  14 ++
  arch/arm/include/asm/outercache.h  |   3 +
  arch/arm/kernel/irq.c  |   3 +-
  arch/arm/mach-exynos/common.h  |   1 +
  arch/arm/mach-exynos/firmware.c|  40 
  arch/arm/mach-exynos/sleep.S   |  41 +
  arch/arm/mm/cache-l2x0.c   | 245 
 -
  9 files changed, 274 insertions(+), 92 deletions(-)
 

Any comments for this series?

Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 0/7] Enable L2 cache support on Exynos4210/4x12 SoCs

[Tomasz Figa]

This series intends to add support for L2 cache on Exynos4 SoCs on boards
running under secure firmware, which requires certain initialization steps
to be done with help of firmware, as selected registers are writable only
from secure mode.

First four patches extend existing support for secure write in L2C driver
to account for design of secure firmware running on Exynos. Namely:
 1) direct read access to certain registers is needed on Exynos, because
secure firmware calls set several registers at once,
 2) not all boards are running secure firmware, so .write_sec callback
needs to be installed in Exynos firmware ops initialization code,
 3) write access to {DATA,TAG}_LATENCY_CTRL registers fron non-secure world
is not allowed and so must use l2c_write_sec as well,
 4) on certain boards, default value of prefetch register is incorrect
and must be overridden at L2C initialization.
For boards running with firmware that provides access to individual
L2C registers this series should introduce no functional changes. However
since the driver is widely used on other platforms I'd like to kindly ask
any interested people for testing.

Further two patches add impelmentation of .write_sec for Exynos secure
firmware and necessary DT nodes to enable L2 cache.

Tested on Exynos4210-based Universal C210 and Trats (both without secure
firmware) and Exynos4412-based TRATS2 and ODROID-U3 boards (both with secure
firmware).

Depends on:
 - [PATCH] ARM: make it easier to check the CPU part number correctly
   (http://thread.gmane.org/gmane.linux.ports.arm.kernel/335126
already in linux-next)
 - [PATCH v2 0/2] Firmware-assisted suspend/resume of Exynos SoCs
   (https://lkml.org/lkml/2014/7/17/431)

Changes since v2:
(https://lkml.org/lkml/2014/6/25/416)
 - refactored L2C driver to use commit-like interface and make it no longer
   depend on availability of writes to individual registers,
 - moved L2C resume to assembly code, because doing it later makes some
   systems unstable - this is also needed for deeper cpuidle modes,
 - dropped unnecessary patch hacking around the .write_sec interface,
 - dropped patch making the driver use l2c_write_sec() for LATENCY_CTRL
   registers as Exynos is no longer affected and I'm not aware of any
   reports that this is also needed on other platforms (can be applied
   separately if it turns out to be so),
 - rebased onto next-20140717 tag of linux-next tree.

Changes since v1:
(https://www.mail-archive.com/linux-omap@vger.kernel.org/msg106323.html)
 - rebased onto for-next branch of linux-samsung tree,
 - changed argument order of outer_cache.write_sec() callback to match
   l2c_write_sec() function in cache-l2x0.c,
 - added support of overriding of prefetch settings to work around incorrect
   default settings on certain Exynos4x12-based boards,
 - added call to firmware to invalidate whole L2 cache before setting enable
   bit in L2C control register (required by Exynos secure firmware).

Tomasz Figa (7):
  ARM: l2c: Refactor the driver to use commit-like interface
  ARM: l2c: Add interface to ask hypervisor to configure L2C
  ARM: l2c: Get outer cache .write_sec callback from mach_desc only if
not NULL
  ARM: l2c: Add support for overriding prefetch settings
  ARM: EXYNOS: Add .write_sec outer cache callback for L2C-310
  ARM: EXYNOS: Add support for non-secure L2X0 resume
  ARM: dts: exynos4: Add nodes for L2 cache controller

 Documentation/devicetree/bindings/arm/l2cc.txt |  10 +
 arch/arm/boot/dts/exynos4210.dtsi  |   9 +
 arch/arm/boot/dts/exynos4x12.dtsi  |  14 ++
 arch/arm/include/asm/outercache.h  |   3 +
 arch/arm/kernel/irq.c  |   3 +-
 arch/arm/mach-exynos/common.h  |   1 +
 arch/arm/mach-exynos/firmware.c|  40 
 arch/arm/mach-exynos/sleep.S   |  41 +
 arch/arm/mm/cache-l2x0.c   | 245 -
 9 files changed, 274 insertions(+), 92 deletions(-)

-- 
1.9.3

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 6/7] ARM: EXYNOS: Add support for non-secure L2X0 resume

[Tomasz Figa]

On Exynos SoCs it is necessary to resume operation of L2C early in
assembly code, because otherwise certain systems will crash. This patch
adds necessary code to non-secure resume handler.

Signed-off-by: Tomasz Figa t.f...@samsung.com
---
 arch/arm/mach-exynos/common.h   |  1 +
 arch/arm/mach-exynos/firmware.c |  2 ++
 arch/arm/mach-exynos/sleep.S| 41 +
 3 files changed, 44 insertions(+)

diff --git a/arch/arm/mach-exynos/common.h b/arch/arm/mach-exynos/common.h
index a3f3061..2540827 100644
--- a/arch/arm/mach-exynos/common.h
+++ b/arch/arm/mach-exynos/common.h
@@ -113,6 +113,7 @@ IS_SAMSUNG_CPU(exynos5800, EXYNOS5800_SOC_ID, 
EXYNOS5_SOC_MASK)
 
 extern u32 cp15_save_diag;
 extern u32 cp15_save_power;
+extern unsigned long l2x0_regs_phys;
 
 extern void __iomem *sysram_ns_base_addr;
 extern void __iomem *sysram_base_addr;
diff --git a/arch/arm/mach-exynos/firmware.c b/arch/arm/mach-exynos/firmware.c
index 554b350..09131d3 100644
--- a/arch/arm/mach-exynos/firmware.c
+++ b/arch/arm/mach-exynos/firmware.c
@@ -103,6 +103,8 @@ static int exynos_suspend(void)
writel(virt_to_phys(exynos_cpu_resume_ns),
sysram_ns_base_addr + EXYNOS_BOOT_ADDR);
 
+   l2x0_regs_phys = virt_to_phys(l2x0_saved_regs);
+
return cpu_suspend(0, exynos_cpu_suspend);
 }
 
diff --git a/arch/arm/mach-exynos/sleep.S b/arch/arm/mach-exynos/sleep.S
index e3c3730..b8ce8f0 100644
--- a/arch/arm/mach-exynos/sleep.S
+++ b/arch/arm/mach-exynos/sleep.S
@@ -16,6 +16,8 @@
  */
 
 #include linux/linkage.h
+#include asm/asm-offsets.h
+#include asm/hardware/cache-l2x0.h
 #include smc.h
 
 #define CPU_MASK   0xff00
@@ -74,6 +76,40 @@ ENTRY(exynos_cpu_resume_ns)
mov r0, #SMC_CMD_C15RESUME
dsb
smc #0
+#ifdef CONFIG_CACHE_L2X0
+   adr r0, l2x0_regs_phys
+   ldr r0, [r0]
+   cmp r0, #0
+   beq skip_l2x0
+
+   ldr r1, [r0, #L2X0_R_PHY_BASE]
+   ldr r2, [r1, #L2X0_CTRL]
+   tst r2, #0x1
+   bne skip_l2x0
+
+   ldr r1, [r0, #L2X0_R_TAG_LATENCY]
+   ldr r2, [r0, #L2X0_R_DATA_LATENCY]
+   ldr r3, [r0, #L2X0_R_PREFETCH_CTRL]
+   mov r0, #SMC_CMD_L2X0SETUP1
+   smc #0
+
+   /* Reload saved regs pointer because smc corrupts registers. */
+   adr r0, l2x0_regs_phys
+   ldr r0, [r0]
+
+   ldr r1, [r0, #L2X0_R_PWR_CTRL]
+   ldr r2, [r0, #L2X0_R_AUX_CTRL]
+   mov r0, #SMC_CMD_L2X0SETUP2
+   smc #0
+
+   mov r0, #SMC_CMD_L2X0INVALL
+   smc #0
+
+   mov r1, #1
+   mov r0, #SMC_CMD_L2X0CTRL
+   smc #0
+skip_l2x0:
+#endif /* CONFIG_CACHE_L2X0 */
 skip_cp15:
b   cpu_resume
 ENDPROC(exynos_cpu_resume_ns)
@@ -83,3 +119,8 @@ cp15_save_diag:
.globl cp15_save_power
 cp15_save_power:
.long   0   @ cp15 power control
+#ifdef CONFIG_CACHE_L2X0
+   .globl l2x0_regs_phys
+l2x0_regs_phys:
+   .long   0   @ phys address of l2x0 save struct
+#endif /* CONFIG_CACHE_L2X0 */
-- 
1.9.3

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 7/7] ARM: dts: exynos4: Add nodes for L2 cache controller

[Tomasz Figa]

This patch adds device tree nodes for L2 cache controller present on
Exynos4 SoCs.

Signed-off-by: Tomasz Figa t.f...@samsung.com
---
 arch/arm/boot/dts/exynos4210.dtsi |  9 +
 arch/arm/boot/dts/exynos4x12.dtsi | 14 ++
 2 files changed, 23 insertions(+)

diff --git a/arch/arm/boot/dts/exynos4210.dtsi 
b/arch/arm/boot/dts/exynos4210.dtsi
index ee3001f..99970ab 100644
--- a/arch/arm/boot/dts/exynos4210.dtsi
+++ b/arch/arm/boot/dts/exynos4210.dtsi
@@ -54,6 +54,15 @@
reg = 0x10023CA0 0x20;
};
 
+   l2c: l2-cache-controller@10502000 {
+   compatible = arm,pl310-cache;
+   reg = 0x10502000 0x1000;
+   cache-unified;
+   cache-level = 2;
+   arm,tag-latency = 2 2 1;
+   arm,data-latency = 2 2 1;
+   };
+
gic: interrupt-controller@1049 {
cpu-offset = 0x8000;
};
diff --git a/arch/arm/boot/dts/exynos4x12.dtsi 
b/arch/arm/boot/dts/exynos4x12.dtsi
index c5a943d..ddffefe 100644
--- a/arch/arm/boot/dts/exynos4x12.dtsi
+++ b/arch/arm/boot/dts/exynos4x12.dtsi
@@ -60,6 +60,20 @@
reg = 0x10023CA0 0x20;
};
 
+   l2c: l2-cache-controller@10502000 {
+   compatible = arm,pl310-cache;
+   reg = 0x10502000 0x1000;
+   cache-unified;
+   cache-level = 2;
+   arm,tag-latency = 2 2 1;
+   arm,data-latency = 3 2 1;
+   arm,double-linefill = 1;
+   arm,double-linefill-incr = 0;
+   arm,double-linefill-wrap = 1;
+   arm,prefetch-drop = 1;
+   arm,prefetch-offset = 7;
+   };
+
clock: clock-controller@1003 {
compatible = samsung,exynos4412-clock;
reg = 0x1003 0x2;
-- 
1.9.3

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/7] ARM: l2c: Refactor the driver to use commit-like interface

[Tomasz Figa]

Certain implementations of secure hypervisors (namely the one found on
Samsung Exynos-based boards) do not provide access to individual L2C
registers. This makes the .write_sec()-based interface insufficient and
provoking ugly hacks.

This patch is first step to make the driver not rely on availability of
writes to individual registers. This is achieved by refactoring the
driver to use a commit-like operation scheme: all register values are
prepared first and stored in an instance of l2x0_regs struct and then a
single callback is responsible to flush those values to the hardware.

Signed-off-by: Tomasz Figa t.f...@samsung.com
---
 arch/arm/mm/cache-l2x0.c | 201 ++-
 1 file changed, 110 insertions(+), 91 deletions(-)

diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 5f2c988..385c047 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -40,12 +40,14 @@ struct l2c_init_data {
void (*enable)(void __iomem *, u32, unsigned);
void (*fixup)(void __iomem *, u32, struct outer_cache_fns *);
void (*save)(void __iomem *);
+   void (*configure)(void __iomem *);
struct outer_cache_fns outer_cache;
 };
 
 #define CACHE_LINE_SIZE32
 
 static void __iomem *l2x0_base;
+static const struct l2c_init_data *l2x0_data;
 static DEFINE_RAW_SPINLOCK(l2x0_lock);
 static u32 l2x0_way_mask;  /* Bitmask of active ways */
 static u32 l2x0_size;
@@ -105,6 +107,14 @@ static inline void l2c_unlock(void __iomem *base, unsigned 
num)
}
 }
 
+static void l2c_configure(void __iomem *base)
+{
+   if (l2x0_data-configure)
+   l2x0_data-configure(base);
+
+   l2c_write_sec(l2x0_saved_regs.aux_ctrl, base, L2X0_AUX_CTRL);
+}
+
 /*
  * Enable the L2 cache controller.  This function must only be
  * called when the cache controller is known to be disabled.
@@ -113,7 +123,12 @@ static void l2c_enable(void __iomem *base, u32 aux, 
unsigned num_lock)
 {
unsigned long flags;
 
-   l2c_write_sec(aux, base, L2X0_AUX_CTRL);
+   /* Do not touch the controller if already enabled. */
+   if (readl_relaxed(base + L2X0_CTRL)  L2X0_CTRL_EN)
+   return;
+
+   l2x0_saved_regs.aux_ctrl = aux;
+   l2c_configure(base);
 
l2c_unlock(base, num_lock);
 
@@ -207,6 +222,12 @@ static void l2c_save(void __iomem *base)
l2x0_saved_regs.aux_ctrl = readl_relaxed(l2x0_base + L2X0_AUX_CTRL);
 }
 
+static void l2c_resume(void)
+{
+   l2x0_data-enable(l2x0_base, l2x0_saved_regs.aux_ctrl,
+   l2x0_data-num_lock);
+}
+
 /*
  * L2C-210 specific code.
  *
@@ -287,14 +308,6 @@ static void l2c210_sync(void)
__l2c210_cache_sync(l2x0_base);
 }
 
-static void l2c210_resume(void)
-{
-   void __iomem *base = l2x0_base;
-
-   if (!(readl_relaxed(base + L2X0_CTRL)  L2X0_CTRL_EN))
-   l2c_enable(base, l2x0_saved_regs.aux_ctrl, 1);
-}
-
 static const struct l2c_init_data l2c210_data __initconst = {
.type = L2C-210,
.way_size_0 = SZ_8K,
@@ -308,7 +321,7 @@ static const struct l2c_init_data l2c210_data __initconst = 
{
.flush_all = l2c210_flush_all,
.disable = l2c_disable,
.sync = l2c210_sync,
-   .resume = l2c210_resume,
+   .resume = l2c_resume,
},
 };
 
@@ -465,7 +478,7 @@ static const struct l2c_init_data l2c220_data = {
.flush_all = l2c220_flush_all,
.disable = l2c_disable,
.sync = l2c220_sync,
-   .resume = l2c210_resume,
+   .resume = l2c_resume,
},
 };
 
@@ -614,39 +627,29 @@ static void __init l2c310_save(void __iomem *base)
L310_POWER_CTRL);
 }
 
-static void l2c310_resume(void)
+static void l2c310_configure(void __iomem *base)
 {
-   void __iomem *base = l2x0_base;
+   unsigned revision;
 
-   if (!(readl_relaxed(base + L2X0_CTRL)  L2X0_CTRL_EN)) {
-   unsigned revision;
-
-   /* restore pl310 setup */
-   writel_relaxed(l2x0_saved_regs.tag_latency,
-  base + L310_TAG_LATENCY_CTRL);
-   writel_relaxed(l2x0_saved_regs.data_latency,
-  base + L310_DATA_LATENCY_CTRL);
-   writel_relaxed(l2x0_saved_regs.filter_end,
-  base + L310_ADDR_FILTER_END);
-   writel_relaxed(l2x0_saved_regs.filter_start,
-  base + L310_ADDR_FILTER_START);
-
-   revision = readl_relaxed(base + L2X0_CACHE_ID) 
-   L2X0_CACHE_ID_RTL_MASK;
-
-   if (revision = L310_CACHE_ID_RTL_R2P0)
-   l2c_write_sec(l2x0_saved_regs.prefetch_ctrl, base,
- L310_PREFETCH_CTRL);
-   if (revision = L310_CACHE_ID_RTL_R3P0

[PATCH v3 4/7] ARM: l2c: Add support for overriding prefetch settings

[Tomasz Figa]

Firmware on certain boards (e.g. ODROID-U3) can leave incorrect L2C prefetch
settings configured in registers leading to crashes if L2C is enabled
without overriding them. This patch introduces bindings to enable
prefetch settings to be specified from DT and necessary support in the
driver.

Signed-off-by: Tomasz Figa t.f...@samsung.com
---
 Documentation/devicetree/bindings/arm/l2cc.txt | 10 +++
 arch/arm/mm/cache-l2x0.c   | 39 ++
 2 files changed, 49 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt 
b/Documentation/devicetree/bindings/arm/l2cc.txt
index af527ee..3443d2d 100644
--- a/Documentation/devicetree/bindings/arm/l2cc.txt
+++ b/Documentation/devicetree/bindings/arm/l2cc.txt
@@ -47,6 +47,16 @@ Optional properties:
 - cache-id-part: cache id part number to be used if it is not present
   on hardware
 - wt-override: If present then L2 is forced to Write through mode
+- arm,double-linefill : Override double linefill enable setting. Enable if
+  non-zero, disable if zero.
+- arm,double-linefill-incr : Override double linefill on INCR read. Enable
+  if non-zero, disable if zero.
+- arm,double-linefill-wrap : Override double linefill on WRAP read. Enable
+  if non-zero, disable if zero.
+- arm,prefetch-drop : Override prefetch drop enable setting. Enable if 
non-zero,
+  disable if zero.
+- arm,prefetch-offset : Override prefetch offset value. Valid values are
+  0-7, 15, 23, and 31.
 
 Example:
 
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 21a625a0..6274803 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -1055,6 +1055,8 @@ static void __init l2c310_of_parse(const struct 
device_node *np,
u32 data[3] = { 0, 0, 0 };
u32 tag[3] = { 0, 0, 0 };
u32 filter[2] = { 0, 0 };
+   u32 prefetch;
+   u32 val;
 
of_property_read_u32_array(np, arm,tag-latency, tag, ARRAY_SIZE(tag));
if (tag[0]  tag[1]  tag[2])
@@ -1079,6 +1081,43 @@ static void __init l2c310_of_parse(const struct 
device_node *np,
l2x0_saved_regs.filter_start = (filter[0]  ~(SZ_1M - 1))
| L310_ADDR_FILTER_EN;
}
+
+   prefetch = l2x0_saved_regs.prefetch_ctrl;
+
+   if (!of_property_read_u32(np, arm,double-linefill, val)) {
+   if (val)
+   prefetch |= L310_PREFETCH_CTRL_DBL_LINEFILL;
+   else
+   prefetch = ~L310_PREFETCH_CTRL_DBL_LINEFILL;
+   }
+
+   if (!of_property_read_u32(np, arm,double-linefill-incr, val)) {
+   if (val)
+   prefetch |= L310_PREFETCH_CTRL_DBL_LINEFILL_INCR;
+   else
+   prefetch = ~L310_PREFETCH_CTRL_DBL_LINEFILL_INCR;
+   }
+
+   if (!of_property_read_u32(np, arm,double-linefill-wrap, val)) {
+   if (!val)
+   prefetch |= L310_PREFETCH_CTRL_DBL_LINEFILL_WRAP;
+   else
+   prefetch = ~L310_PREFETCH_CTRL_DBL_LINEFILL_WRAP;
+   }
+
+   if (!of_property_read_u32(np, arm,prefetch-drop, val)) {
+   if (val)
+   prefetch |= L310_PREFETCH_CTRL_PREFETCH_DROP;
+   else
+   prefetch = ~L310_PREFETCH_CTRL_PREFETCH_DROP;
+   }
+
+   if (!of_property_read_u32(np, arm,prefetch-offset, val)) {
+   prefetch = ~L310_PREFETCH_CTRL_OFFSET_MASK;
+   prefetch |= val  L310_PREFETCH_CTRL_OFFSET_MASK;
+   }
+
+   l2x0_saved_regs.prefetch_ctrl = prefetch;
 }
 
 static const struct l2c_init_data of_l2c310_data __initconst = {
-- 
1.9.3

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 5/7] ARM: EXYNOS: Add .write_sec outer cache callback for L2C-310

[Tomasz Figa]

Exynos4 SoCs equipped with an L2C-310 cache controller and running under
secure firmware require certain registers of aforementioned IP to be
accessed only from secure mode. This means that SMC calls are required
for certain register writes. To handle this, an implementation of
.write_sec and .configure callbacks is provided by this patch.

Signed-off-by: Tomasz Figa t.f...@samsung.com
---
 arch/arm/mach-exynos/firmware.c | 38 ++
 1 file changed, 38 insertions(+)

diff --git a/arch/arm/mach-exynos/firmware.c b/arch/arm/mach-exynos/firmware.c
index f5e626d..554b350 100644
--- a/arch/arm/mach-exynos/firmware.c
+++ b/arch/arm/mach-exynos/firmware.c
@@ -17,6 +17,7 @@
 #include asm/cacheflush.h
 #include asm/cputype.h
 #include asm/firmware.h
+#include asm/hardware/cache-l2x0.h
 #include asm/suspend.h
 
 #include mach/map.h
@@ -120,6 +121,31 @@ static const struct firmware_ops exynos_firmware_ops = {
.resume = exynos_resume,
 };
 
+static void exynos_l2_write_sec(unsigned long val, unsigned reg)
+{
+   switch (reg) {
+   case L2X0_CTRL:
+   if (val  L2X0_CTRL_EN)
+   exynos_smc(SMC_CMD_L2X0INVALL, 0, 0, 0);
+   exynos_smc(SMC_CMD_L2X0CTRL, val, 0, 0);
+   break;
+
+   case L2X0_DEBUG_CTRL:
+   exynos_smc(SMC_CMD_L2X0DEBUG, val, 0, 0);
+   break;
+
+   default:
+   WARN_ONCE(1, %s: ignoring write to reg 0x%x\n, __func__, reg);
+   }
+}
+
+static void exynos_l2_configure(const struct l2x0_regs *regs)
+{
+   exynos_smc(SMC_CMD_L2X0SETUP1, regs-tag_latency, regs-data_latency,
+   regs-prefetch_ctrl);
+   exynos_smc(SMC_CMD_L2X0SETUP2, regs-pwr_ctrl, regs-aux_ctrl, 0);
+}
+
 void __init exynos_firmware_init(void)
 {
struct device_node *nd;
@@ -139,4 +165,16 @@ void __init exynos_firmware_init(void)
pr_info(Running under secure firmware.\n);
 
register_firmware_ops(exynos_firmware_ops);
+
+   /*
+* Exynos 4 SoCs (based on Cortex A9 and equipped with L2C-310),
+* running under secure firmware, require certain registers of L2
+* cache controller to be written in secure mode. Here .write_sec
+* callback is provided to perform necessary SMC calls.
+*/
+   if (IS_ENABLED(CONFIG_CACHE_L2X0)
+read_cpuid_part() == ARM_CPU_PART_CORTEX_A9) {
+   outer_cache.write_sec = exynos_l2_write_sec;
+   outer_cache.configure = exynos_l2_configure;
+   }
 }
-- 
1.9.3

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/7] ARM: l2c: Add interface to ask hypervisor to configure L2C

[Tomasz Figa]

Because certain secure hypervisor do not allow writes to individual L2C
registers, but rather expect set of parameters to be passed as argument
to secure monitor calls, there is a need to provide an interface for the
L2C driver to ask the firmware to configure the hardware according to
specified parameters. This patch adds such.

Signed-off-by: Tomasz Figa t.f...@samsung.com
---
 arch/arm/include/asm/outercache.h | 3 +++
 arch/arm/mm/cache-l2x0.c  | 5 +
 2 files changed, 8 insertions(+)

diff --git a/arch/arm/include/asm/outercache.h 
b/arch/arm/include/asm/outercache.h
index 891a56b..563b92f 100644
--- a/arch/arm/include/asm/outercache.h
+++ b/arch/arm/include/asm/outercache.h
@@ -23,6 +23,8 @@
 
 #include linux/types.h
 
+struct l2x0_regs;
+
 struct outer_cache_fns {
void (*inv_range)(unsigned long, unsigned long);
void (*clean_range)(unsigned long, unsigned long);
@@ -36,6 +38,7 @@ struct outer_cache_fns {
 
/* This is an ARM L2C thing */
void (*write_sec)(unsigned long, unsigned);
+   void (*configure)(const struct l2x0_regs *);
 };
 
 extern struct outer_cache_fns outer_cache;
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 385c047..21a625a0 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -109,6 +109,11 @@ static inline void l2c_unlock(void __iomem *base, unsigned 
num)
 
 static void l2c_configure(void __iomem *base)
 {
+   if (outer_cache.configure) {
+   outer_cache.configure(l2x0_saved_regs);
+   return;
+   }
+
if (l2x0_data-configure)
l2x0_data-configure(base);
 
-- 
1.9.3

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 3/7] ARM: l2c: Get outer cache .write_sec callback from mach_desc only if not NULL

[Tomasz Figa]

Certain platforms (i.e. Exynos) might need to set .write_sec callback
from firmware initialization which is happenning in .init_early callback
of machine descriptor. However current code will overwrite the pointer
with whatever is present in machine descriptor, even though it can be
already set earlier. This patch fixes this by making the assignment
conditional, depending on whether current .write_sec callback is NULL.

Signed-off-by: Tomasz Figa t.f...@samsung.com
---
 arch/arm/kernel/irq.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kernel/irq.c b/arch/arm/kernel/irq.c
index 2c42576..e7383b9 100644
--- a/arch/arm/kernel/irq.c
+++ b/arch/arm/kernel/irq.c
@@ -125,7 +125,8 @@ void __init init_IRQ(void)
 
if (IS_ENABLED(CONFIG_OF)  IS_ENABLED(CONFIG_CACHE_L2X0) 
(machine_desc-l2c_aux_mask || machine_desc-l2c_aux_val)) {
-   outer_cache.write_sec = machine_desc-l2c_write_sec;
+   if (!outer_cache.write_sec)
+   outer_cache.write_sec = machine_desc-l2c_write_sec;
ret = l2x0_of_init(machine_desc-l2c_aux_val,
   machine_desc-l2c_aux_mask);
if (ret)
-- 
1.9.3

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM: convert all mov.* pc, reg to bx reg for ARMv6+ (part1)

[Tomasz Figa]

Hi Russell,

On 01.07.2014 18:19, Russell King wrote:
 ARMv6 and greater introduced a new instruction (bx) which can be used
 to return from function calls.  Recent CPUs perform better when the
 bx lr instruction is used rather than the mov pc, lr instruction,
 and this sequence is strongly recommended to be used by the ARM
 architecture manual (section A.4.1.1).
 
 We provide a new macro ret with all its variants for the condition
 code which will resolve to the appropriate instruction.
 
 Rather than doing this piecemeal, and miss some instances, change all
 the mov pc instances to use the new macro, with the exception of
 the movs instruction and the kprobes code.  This allows us to detect
 the mov pc, lr case and fix it up - and also gives us the possibility
 of deploying this for other registers depending on the CPU selection.
 
 Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk

Build- and boot-tested on mach-s3c64xx.

Tested-by: Tomasz Figa tomasz.f...@gmail.com

Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 0/6] Enable L2 cache support on Exynos4210/4x12 SoCs

[Tomasz Figa]

This series intends to add support for L2 cache on Exynos4 SoCs on boards
running under secure firmware, which requires certain initialization steps
to be done with help of firmware, as selected registers are writable only
from secure mode.

First four patches extend existing support for secure write in L2C driver
to account for design of secure firmware running on Exynos. Namely:
 1) direct read access to certain registers is needed on Exynos, because
secure firmware calls set several registers at once,
 2) not all boards are running secure firmware, so .write_sec callback
needs to be installed in Exynos firmware ops initialization code,
 3) write access to {DATA,TAG}_LATENCY_CTRL registers fron non-secure world
is not allowed and so must use l2c_write_sec as well,
 4) on certain boards, default value of prefetch register is incorrect
and must be overridden at L2C initialization.
Patches 1-3 might affect other platforms using .write_sec callback, so
I'd like to kindly ask any interested people for testing.

Further two patches add impelmentation of .write_sec for Exynos secure
firmware and necessary DT nodes to enable L2 cache.

Tested on Exynos4210-based Universal C210 and Trats (both without secure
firmware) and Exynos4412-based TRATS2 and ODROID-U3 boards (both with secure
firmware).

Changes since v1:
(https://www.mail-archive.com/linux-omap@vger.kernel.org/msg106323.html)
 - rebased onto for-next branch of linux-samsung tree,
 - changed argument order of outer_cache.write_sec() callback to match
   l2c_write_sec() function in cache-l2x0.c,
 - added support of overriding of prefetch settings to work around incorrect
   default settings on certain Exynos4x12-based boards,
 - added call to firmware to invalidate whole L2 cache before setting enable
   bit in L2C control register (required by Exynos secure firmware).

Tomasz Figa (6):
  ARM: mm: cache-l2x0: Add base address argument to write_sec callback
  ARM: Get outer cache .write_sec callback from mach_desc only if not
NULL
  ARM: mm: cache-l2x0: Use l2c_write_sec() for LATENCY_CTRL registers
  ARM: mm: l2x0: Add support for overriding prefetch settings
  ARM: EXYNOS: Add .write_sec outer cache callback for L2C-310
  ARM: dts: exynos4: Add nodes for L2 cache controller

 Documentation/devicetree/bindings/arm/l2cc.txt | 10 
 arch/arm/boot/dts/exynos4210.dtsi  |  9 
 arch/arm/boot/dts/exynos4x12.dtsi  | 14 ++
 arch/arm/include/asm/mach/arch.h   |  3 +-
 arch/arm/include/asm/outercache.h  |  2 +-
 arch/arm/kernel/irq.c  |  3 +-
 arch/arm/mach-exynos/firmware.c| 63 +
 arch/arm/mach-highbank/highbank.c  |  3 +-
 arch/arm/mach-omap2/omap4-common.c |  3 +-
 arch/arm/mach-ux500/cache-l2x0.c   |  3 +-
 arch/arm/mm/cache-l2x0.c   | 64 ++
 11 files changed, 162 insertions(+), 15 deletions(-)

-- 
1.9.3

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 4/6] ARM: mm: l2x0: Add support for overriding prefetch settings

[Tomasz Figa]

Signed-off-by: Tomasz Figa t.f...@samsung.com
---
 Documentation/devicetree/bindings/arm/l2cc.txt | 10 ++
 arch/arm/mm/cache-l2x0.c   | 46 ++
 2 files changed, 56 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt 
b/Documentation/devicetree/bindings/arm/l2cc.txt
index b513cb8..8096fcd 100644
--- a/Documentation/devicetree/bindings/arm/l2cc.txt
+++ b/Documentation/devicetree/bindings/arm/l2cc.txt
@@ -44,6 +44,16 @@ Optional properties:
 - cache-id-part: cache id part number to be used if it is not present
   on hardware
 - wt-override: If present then L2 is forced to Write through mode
+- arm,double-linefill : Override double linefill enable setting. Enable if
+  non-zero, disable if zero.
+- arm,double-linefill-incr : Override double linefill on INCR read. Enable
+  if non-zero, disable if zero.
+- arm,double-linefill-wrap : Override double linefill on WRAP read. Enable
+  if non-zero, disable if zero.
+- arm,prefetch-drop : Override prefetch drop enable setting. Enable if 
non-zero,
+  disable if zero.
+- arm,prefetch-offset : Override prefetch offset value. Valid values are
+  0-7, 15, 23, and 31.
 
 Example:
 
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 6600fd9..fd23cce 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -1018,9 +1018,12 @@ static const struct l2c_init_data of_l2c220_data 
__initconst = {
 static void __init l2c310_of_parse(const struct device_node *np,
u32 *aux_val, u32 *aux_mask)
 {
+   bool set_prefetch = false;
u32 data[3] = { 0, 0, 0 };
u32 tag[3] = { 0, 0, 0 };
u32 filter[2] = { 0, 0 };
+   u32 prefetch;
+   u32 val;
 
of_property_read_u32_array(np, arm,tag-latency, tag, ARRAY_SIZE(tag));
if (tag[0]  tag[1]  tag[2])
@@ -1047,6 +1050,49 @@ static void __init l2c310_of_parse(const struct 
device_node *np,
writel_relaxed((filter[0]  ~(SZ_1M - 1)) | L310_ADDR_FILTER_EN,
   l2x0_base + L310_ADDR_FILTER_START);
}
+
+   prefetch = readl_relaxed(l2x0_base + L310_PREFETCH_CTRL);
+
+   if (!of_property_read_u32(np, arm,double-linefill, val)) {
+   if (val)
+   prefetch |= L310_PREFETCH_CTRL_DBL_LINEFILL;
+   else
+   prefetch = ~L310_PREFETCH_CTRL_DBL_LINEFILL;
+   set_prefetch = true;
+   }
+
+   if (!of_property_read_u32(np, arm,double-linefill-incr, val)) {
+   if (val)
+   prefetch |= L310_PREFETCH_CTRL_DBL_LINEFILL_INCR;
+   else
+   prefetch = ~L310_PREFETCH_CTRL_DBL_LINEFILL_INCR;
+   set_prefetch = true;
+   }
+
+   if (!of_property_read_u32(np, arm,double-linefill-wrap, val)) {
+   if (!val)
+   prefetch |= L310_PREFETCH_CTRL_DBL_LINEFILL_WRAP;
+   else
+   prefetch = ~L310_PREFETCH_CTRL_DBL_LINEFILL_WRAP;
+   set_prefetch = true;
+   }
+
+   if (!of_property_read_u32(np, arm,prefetch-drop, val)) {
+   if (val)
+   prefetch |= L310_PREFETCH_CTRL_PREFETCH_DROP;
+   else
+   prefetch = ~L310_PREFETCH_CTRL_PREFETCH_DROP;
+   set_prefetch = true;
+   }
+
+   if (!of_property_read_u32(np, arm,prefetch-offset, val)) {
+   prefetch = ~L310_PREFETCH_CTRL_OFFSET_MASK;
+   prefetch |= val  L310_PREFETCH_CTRL_OFFSET_MASK;
+   set_prefetch = true;
+   }
+
+   if (set_prefetch)
+   l2c_write_sec(prefetch, l2x0_base, L310_PREFETCH_CTRL);
 }
 
 static const struct l2c_init_data of_l2c310_data __initconst = {
-- 
1.9.3

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/6] ARM: mm: cache-l2x0: Use l2c_write_sec() for LATENCY_CTRL registers

[Tomasz Figa]

According to the documentation, TAG_LATENCY_CTRL and DATA_LATENCY_CTRL
registers of L2C-310 can be written only in secure mode, so
l2c_write_sec() should be used to change them, instead of plain
writel_relaxed().

Signed-off-by: Tomasz Figa t.f...@samsung.com
---
 arch/arm/mm/cache-l2x0.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 478566b..6600fd9 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -622,10 +622,10 @@ static void l2c310_resume(void)
unsigned revision;
 
/* restore pl310 setup */
-   writel_relaxed(l2x0_saved_regs.tag_latency,
-  base + L310_TAG_LATENCY_CTRL);
-   writel_relaxed(l2x0_saved_regs.data_latency,
-  base + L310_DATA_LATENCY_CTRL);
+   l2c_write_sec(l2x0_saved_regs.tag_latency,
+  base, L310_TAG_LATENCY_CTRL);
+   l2c_write_sec(l2x0_saved_regs.data_latency,
+  base, L310_DATA_LATENCY_CTRL);
writel_relaxed(l2x0_saved_regs.filter_end,
   base + L310_ADDR_FILTER_END);
writel_relaxed(l2x0_saved_regs.filter_start,
@@ -1024,20 +1024,20 @@ static void __init l2c310_of_parse(const struct 
device_node *np,
 
of_property_read_u32_array(np, arm,tag-latency, tag, ARRAY_SIZE(tag));
if (tag[0]  tag[1]  tag[2])
-   writel_relaxed(
+   l2c_write_sec(
L310_LATENCY_CTRL_RD(tag[0] - 1) |
L310_LATENCY_CTRL_WR(tag[1] - 1) |
L310_LATENCY_CTRL_SETUP(tag[2] - 1),
-   l2x0_base + L310_TAG_LATENCY_CTRL);
+   l2x0_base, L310_TAG_LATENCY_CTRL);
 
of_property_read_u32_array(np, arm,data-latency,
   data, ARRAY_SIZE(data));
if (data[0]  data[1]  data[2])
-   writel_relaxed(
+   l2c_write_sec(
L310_LATENCY_CTRL_RD(data[0] - 1) |
L310_LATENCY_CTRL_WR(data[1] - 1) |
L310_LATENCY_CTRL_SETUP(data[2] - 1),
-   l2x0_base + L310_DATA_LATENCY_CTRL);
+   l2x0_base, L310_DATA_LATENCY_CTRL);
 
of_property_read_u32_array(np, arm,filter-ranges,
   filter, ARRAY_SIZE(filter));
-- 
1.9.3

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 6/6] ARM: dts: exynos4: Add nodes for L2 cache controller

[Tomasz Figa]

This patch adds device tree nodes for L2 cache controller present on
Exynos4 SoCs.

Signed-off-by: Tomasz Figa t.f...@samsung.com
---
 arch/arm/boot/dts/exynos4210.dtsi |  9 +
 arch/arm/boot/dts/exynos4x12.dtsi | 14 ++
 2 files changed, 23 insertions(+)

diff --git a/arch/arm/boot/dts/exynos4210.dtsi 
b/arch/arm/boot/dts/exynos4210.dtsi
index ee3001f..99970ab 100644
--- a/arch/arm/boot/dts/exynos4210.dtsi
+++ b/arch/arm/boot/dts/exynos4210.dtsi
@@ -54,6 +54,15 @@
reg = 0x10023CA0 0x20;
};
 
+   l2c: l2-cache-controller@10502000 {
+   compatible = arm,pl310-cache;
+   reg = 0x10502000 0x1000;
+   cache-unified;
+   cache-level = 2;
+   arm,tag-latency = 2 2 1;
+   arm,data-latency = 2 2 1;
+   };
+
gic: interrupt-controller@1049 {
cpu-offset = 0x8000;
};
diff --git a/arch/arm/boot/dts/exynos4x12.dtsi 
b/arch/arm/boot/dts/exynos4x12.dtsi
index c5a943d..ddffefe 100644
--- a/arch/arm/boot/dts/exynos4x12.dtsi
+++ b/arch/arm/boot/dts/exynos4x12.dtsi
@@ -60,6 +60,20 @@
reg = 0x10023CA0 0x20;
};
 
+   l2c: l2-cache-controller@10502000 {
+   compatible = arm,pl310-cache;
+   reg = 0x10502000 0x1000;
+   cache-unified;
+   cache-level = 2;
+   arm,tag-latency = 2 2 1;
+   arm,data-latency = 3 2 1;
+   arm,double-linefill = 1;
+   arm,double-linefill-incr = 0;
+   arm,double-linefill-wrap = 1;
+   arm,prefetch-drop = 1;
+   arm,prefetch-offset = 7;
+   };
+
clock: clock-controller@1003 {
compatible = samsung,exynos4412-clock;
reg = 0x1003 0x2;
-- 
1.9.3

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/6] ARM: mm: cache-l2x0: Add base address argument to write_sec callback

[Tomasz Figa]

For certain platforms (e.g. Exynos) it is necessary to read back some
values from registers before they can be written (i.e. SMC calls that
set multiple registers per call), so base address of L2C controller is
needed for .write_sec operation. This patch adds base argument to
.write_sec callback so that its implementation can also access registers
directly.

Signed-off-by: Tomasz Figa t.f...@samsung.com
---
 arch/arm/include/asm/mach/arch.h   | 3 ++-
 arch/arm/include/asm/outercache.h  | 2 +-
 arch/arm/mach-highbank/highbank.c  | 3 ++-
 arch/arm/mach-omap2/omap4-common.c | 3 ++-
 arch/arm/mach-ux500/cache-l2x0.c   | 3 ++-
 arch/arm/mm/cache-l2x0.c   | 2 +-
 6 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h
index 060a75e..fefff4d 100644
--- a/arch/arm/include/asm/mach/arch.h
+++ b/arch/arm/include/asm/mach/arch.h
@@ -46,7 +46,8 @@ struct machine_desc {
enum reboot_modereboot_mode;/* default restart mode */
unsignedl2c_aux_val;/* L2 cache aux value   */
unsignedl2c_aux_mask;   /* L2 cache aux mask*/
-   void(*l2c_write_sec)(unsigned long, unsigned);
+   void(*l2c_write_sec)(unsigned long,
+void __iomem *, unsigned);
struct smp_operations   *smp;   /* SMP operations   */
bool(*smp_init)(void);
void(*fixup)(struct tag *, char **);
diff --git a/arch/arm/include/asm/outercache.h 
b/arch/arm/include/asm/outercache.h
index 891a56b..25b70b5 100644
--- a/arch/arm/include/asm/outercache.h
+++ b/arch/arm/include/asm/outercache.h
@@ -35,7 +35,7 @@ struct outer_cache_fns {
void (*resume)(void);
 
/* This is an ARM L2C thing */
-   void (*write_sec)(unsigned long, unsigned);
+   void (*write_sec)(unsigned long, void __iomem *, unsigned);
 };
 
 extern struct outer_cache_fns outer_cache;
diff --git a/arch/arm/mach-highbank/highbank.c 
b/arch/arm/mach-highbank/highbank.c
index 8c35ae4..20cf160 100644
--- a/arch/arm/mach-highbank/highbank.c
+++ b/arch/arm/mach-highbank/highbank.c
@@ -51,7 +51,8 @@ static void __init highbank_scu_map_io(void)
 }
 
 
-static void highbank_l2c310_write_sec(unsigned long val, unsigned reg)
+static void highbank_l2c310_write_sec(unsigned long val, void __iomem *base,
+ unsigned reg)
 {
if (reg == L2X0_CTRL)
highbank_smc1(0x102, val);
diff --git a/arch/arm/mach-omap2/omap4-common.c 
b/arch/arm/mach-omap2/omap4-common.c
index 326cd98..ce04afa 100644
--- a/arch/arm/mach-omap2/omap4-common.c
+++ b/arch/arm/mach-omap2/omap4-common.c
@@ -167,7 +167,8 @@ void __iomem *omap4_get_l2cache_base(void)
return l2cache_base;
 }
 
-static void omap4_l2c310_write_sec(unsigned long val, unsigned reg)
+static void omap4_l2c310_write_sec(unsigned long val, void __iomem *base,
+  unsigned reg)
 {
unsigned smc_op;
 
diff --git a/arch/arm/mach-ux500/cache-l2x0.c b/arch/arm/mach-ux500/cache-l2x0.c
index 842ebed..bd87108 100644
--- a/arch/arm/mach-ux500/cache-l2x0.c
+++ b/arch/arm/mach-ux500/cache-l2x0.c
@@ -35,7 +35,8 @@ static int __init ux500_l2x0_unlock(void)
return 0;
 }
 
-static void ux500_l2c310_write_sec(unsigned long val, unsigned reg)
+static void ux500_l2c310_write_sec(unsigned long val, void __iomem *base,
+  unsigned reg)
 {
/*
 * We can't write to secure registers as we are in non-secure
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index efc5cab..478566b 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -72,7 +72,7 @@ static void l2c_write_sec(unsigned long val, void __iomem 
*base, unsigned reg)
if (val == readl_relaxed(base + reg))
return;
if (outer_cache.write_sec)
-   outer_cache.write_sec(val, reg);
+   outer_cache.write_sec(val, base, reg);
else
writel_relaxed(val, base + reg);
 }
-- 
1.9.3

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 5/6] ARM: EXYNOS: Add .write_sec outer cache callback for L2C-310

[Tomasz Figa]

Exynos4 SoCs equipped with an L2C-310 cache controller and running under
secure firmware require certain registers of aforementioned IP to be
accessed only from secure mode. This means that SMC calls are required
for certain register writes. To handle this, an implementation of
.write_sec callback is provided by this patch.

Signed-off-by: Tomasz Figa t.f...@samsung.com
---
 arch/arm/mach-exynos/firmware.c | 63 +
 1 file changed, 63 insertions(+)

diff --git a/arch/arm/mach-exynos/firmware.c b/arch/arm/mach-exynos/firmware.c
index eb91d23..def7bb4 100644
--- a/arch/arm/mach-exynos/firmware.c
+++ b/arch/arm/mach-exynos/firmware.c
@@ -14,7 +14,9 @@
 #include linux/of.h
 #include linux/of_address.h
 
+#include asm/cputype.h
 #include asm/firmware.h
+#include asm/hardware/cache-l2x0.h
 
 #include mach/map.h
 
@@ -70,6 +72,57 @@ static const struct firmware_ops exynos_firmware_ops = {
.cpu_boot   = exynos_cpu_boot,
 };
 
+static void exynos_l2_write_sec(unsigned long val, void __iomem *base,
+   unsigned reg)
+{
+   switch (reg) {
+   case L2X0_CTRL:
+   if (val  L2X0_CTRL_EN)
+   exynos_smc(SMC_CMD_L2X0INVALL, 0, 0, 0);
+   exynos_smc(SMC_CMD_L2X0CTRL, val, 0, 0);
+   break;
+
+   case L2X0_AUX_CTRL:
+   exynos_smc(SMC_CMD_L2X0SETUP2,
+   readl_relaxed(base + L310_POWER_CTRL),
+   val, 0);
+   break;
+
+   case L2X0_DEBUG_CTRL:
+   exynos_smc(SMC_CMD_L2X0DEBUG, val, 0, 0);
+   break;
+
+   case L310_TAG_LATENCY_CTRL:
+   exynos_smc(SMC_CMD_L2X0SETUP1,
+   val,
+   readl_relaxed(base + L310_DATA_LATENCY_CTRL),
+   readl_relaxed(base + L310_PREFETCH_CTRL));
+   break;
+
+   case L310_DATA_LATENCY_CTRL:
+   exynos_smc(SMC_CMD_L2X0SETUP1,
+   readl_relaxed(base + L310_TAG_LATENCY_CTRL),
+   val,
+   readl_relaxed(base + L310_PREFETCH_CTRL));
+   break;
+
+   case L310_PREFETCH_CTRL:
+   exynos_smc(SMC_CMD_L2X0SETUP1,
+   readl_relaxed(base + L310_TAG_LATENCY_CTRL),
+   readl_relaxed(base + L310_DATA_LATENCY_CTRL),
+   val);
+   break;
+
+   case L310_POWER_CTRL:
+   exynos_smc(SMC_CMD_L2X0SETUP2, val,
+   readl_relaxed(base + L2X0_AUX_CTRL), 0);
+   break;
+
+   default:
+   WARN_ONCE(1, %s: ignoring write to reg 0x%x\n, __func__, reg);
+   }
+}
+
 void __init exynos_firmware_init(void)
 {
struct device_node *nd;
@@ -89,4 +142,14 @@ void __init exynos_firmware_init(void)
pr_info(Running under secure firmware.\n);
 
register_firmware_ops(exynos_firmware_ops);
+
+   /*
+* Exynos 4 SoCs (based on Cortex A9 and equipped with L2C-310),
+* running under secure firmware, require certain registers of L2
+* cache controller to be written in secure mode. Here .write_sec
+* callback is provided to perform necessary SMC calls.
+*/
+   if (IS_ENABLED(CONFIG_CACHE_L2X0)
+read_cpuid_part_number() == ARM_CPU_PART_CORTEX_A9)
+   outer_cache.write_sec = exynos_l2_write_sec;
 }
-- 
1.9.3

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/6] ARM: Get outer cache .write_sec callback from mach_desc only if not NULL

[Tomasz Figa]

Certain platforms (i.e. Exynos) might need to set .write_sec callback
from firmware initialization which is happenning in .init_early callback
of machine descriptor. However current code will overwrite the pointer
with whatever is present in machine descriptor, even though it can be
already set earlier. This patch fixes this by making the assignment
conditional, depending on whether current .write_sec callback is NULL.

Signed-off-by: Tomasz Figa t.f...@samsung.com
---
 arch/arm/kernel/irq.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kernel/irq.c b/arch/arm/kernel/irq.c
index 2c42576..e7383b9 100644
--- a/arch/arm/kernel/irq.c
+++ b/arch/arm/kernel/irq.c
@@ -125,7 +125,8 @@ void __init init_IRQ(void)
 
if (IS_ENABLED(CONFIG_OF)  IS_ENABLED(CONFIG_CACHE_L2X0) 
(machine_desc-l2c_aux_mask || machine_desc-l2c_aux_val)) {
-   outer_cache.write_sec = machine_desc-l2c_write_sec;
+   if (!outer_cache.write_sec)
+   outer_cache.write_sec = machine_desc-l2c_write_sec;
ret = l2x0_of_init(machine_desc-l2c_aux_val,
   machine_desc-l2c_aux_mask);
if (ret)
-- 
1.9.3

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/6] Enable L2 cache support on Exynos4210/4x12 SoCs

[Tomasz Figa]

On 25.06.2014 15:50, Russell King - ARM Linux wrote:
 On Wed, Jun 25, 2014 at 03:37:25PM +0200, Tomasz Figa wrote:
 This series intends to add support for L2 cache on Exynos4 SoCs on boards
 running under secure firmware, which requires certain initialization steps
 to be done with help of firmware, as selected registers are writable only
 from secure mode.
 
 What I said in my message on June 12th applies to this series.  I'm
 not having the virtual address exposed via the write_sec call.
 
 Yes, you need to read other registers in order to use your secure
 firmware implementation.  Let's fix that by providing a better write_sec
 interface so you don't have to read back these registers, rather than
 working around this short-coming.

Do you have anything in particular in mind? I would be glad to implement
it and send patches.

 
 That's exactly what I meant when I talked on June 12th about turning
 cache-l2x0.c back into a pile of crap.  You're working around problems
 rather than fixing the underlying issue, as seems to be standard
 platform maintainer behaviour when things like core ARM code is
 concerned.  This is why things devolve over time into piles of crap,
 because platforms just hack around problems rather than fixing the
 root cause of the problem.

I'm not sure what part of my patches exactly is turning cache-l2x0.c
into a pile of crap. On the contrary, I believe that working around the
firmware brokenness on platform level, while keeping the core code
simple does the opposite.

However, I'll be happy to rework my series if you have some more
specific suggestions.

 So... I'm NAKing the entire series.

Your opinion is always appreciated, thanks.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/6] Enable L2 cache support on Exynos4210/4x12 SoCs

[Tomasz Figa]

On 25.06.2014 16:37, Russell King - ARM Linux wrote:
 On Wed, Jun 25, 2014 at 04:13:16PM +0200, Tomasz Figa wrote:
 On 25.06.2014 15:50, Russell King - ARM Linux wrote:
 On Wed, Jun 25, 2014 at 03:37:25PM +0200, Tomasz Figa wrote:
 This series intends to add support for L2 cache on Exynos4 SoCs on boards
 running under secure firmware, which requires certain initialization steps
 to be done with help of firmware, as selected registers are writable only
 from secure mode.

 What I said in my message on June 12th applies to this series.  I'm
 not having the virtual address exposed via the write_sec call.

 Yes, you need to read other registers in order to use your secure
 firmware implementation.  Let's fix that by providing a better write_sec
 interface so you don't have to read back these registers, rather than
 working around this short-coming.

 Do you have anything in particular in mind? I would be glad to implement
 it and send patches.
 
 As I've already said, you are not the only ones who need fuller information
 to make your secure monitor calls.  So, what that implies is that rather
 than the interface being please write register X with value V, and then
 having platforms work-around that by reading various registers, we need
 a more flexible interface which passes the desired state.
 

So it's still not clear to me how this should be done correctly.

One thing that comes to my mind is precomputing register values to some
kind of structure, then calling some kind of magical platform-specific
.enable() or .configure() callback, which takes the structure and, in
one shot, configures the L2C according to firmware requirements. Then
the generic code would read back those values to verify the final
configuration (as it does right now) and rest of the operation would be
identical.

Is this something you would deem acceptable?

Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/5] Handle non-secure L2C initialization on Exynos4

[Tomasz Figa]

Hi Daniel,

I have attached, three patches which make the kernel boot fine with L2
cache enabled on ODROID-U3. Could you test them on your setup to verify
that they indeed fix the issue?

Best regards,
Tomasz

On 12.06.2014 15:38, Daniel Drake wrote:
 Hi Tomasz,
 
 Thanks for working on this!
 
 I have just tried this, against Linus master
 64b2d1fbbfda07765dae3f601862796a61b2c451.
 Added patch ARM: dts: Initial ODROID U2 support and booted on
 ODROID-U2. I believe this board has the security enabled.
 
 Unfortunately, it hangs during early boot. With earlyprintk the last
 messages seen are:
 
 L2C: platform modifies aux control register: 0x0207 - 0x3e470001
 L2C: platform provided aux values permit register corruption.
 L2C: DT/platform modifies aux control register: 0x0207 - 0x3e470001
 L2C-310 enabling early BRESP for Cortex-A9
 L2C-310: enabling full line of zeros but not enabled in Cortex-A9
 L2C-310 ID prefetch enabled, offset 1 lines
 L2C-310 dynamic clock gating enabled, standby mode enabled
 L2C-310 cache controller enabled, 16 ways, 1024 kB
 L2C-310: CACHE_ID 0x4100c4c8, AUX_CTRL 0x7e470001
 
 I then tried to go back to the earlier patch ARM: EXYNOS: Add secure
 firmware support for l2x0 init (attached, needed a rebase) but that
 one also now hangs at:
 
 L2C: platform modifies aux control register: 0x0207 - 0x3e470001
 
 It did work on 3.14 though. Looking at the changelogs, many changes
 have been made to l2x0 recently. Can you confirm that you have tested
 your patches against a kernel with all of Russell King's recent
 changes?
 
 Thanks
 Daniel
 
From b574212db2c1c226212c74b475acceb7fa507c27 Mon Sep 17 00:00:00 2001
From: Tomasz Figa t.f...@samsung.com
Date: Fri, 13 Jun 2014 16:40:29 +0200
Subject: [PATCH 1/3] ARM: EXYNOS: Invalidate L2 cache with SMC command before
 enabling

Signed-off-by: Tomasz Figa t.f...@samsung.com
---
 arch/arm/mach-exynos/firmware.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/mach-exynos/firmware.c b/arch/arm/mach-exynos/firmware.c
index d8544537..a688757 100644
--- a/arch/arm/mach-exynos/firmware.c
+++ b/arch/arm/mach-exynos/firmware.c
@@ -110,6 +110,8 @@ static void exynos_l2_write_sec(unsigned long val, void __iomem *base,
 {
 	switch (reg) {
 	case L2X0_CTRL:
+		if (val  L2X0_CTRL_EN)
+			exynos_smc(SMC_CMD_L2X0INVALL, 0, 0, 0);
 		exynos_smc(SMC_CMD_L2X0CTRL, val, 0, 0);
 		break;
 
-- 
1.9.3

From 0803df887262849ab8ef905f15fdbe2b34598dde Mon Sep 17 00:00:00 2001
From: Tomasz Figa t.f...@samsung.com
Date: Fri, 13 Jun 2014 16:48:47 +0200
Subject: [PATCH 2/3] ARM: mm: l2x0: Add support for overriding prefetch
 settings

Signed-off-by: Tomasz Figa t.f...@samsung.com
---
 Documentation/devicetree/bindings/arm/l2cc.txt | 10 ++
 arch/arm/mm/cache-l2x0.c   | 46 ++
 2 files changed, 56 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/l2cc.txt b/Documentation/devicetree/bindings/arm/l2cc.txt
index b513cb8..8096fcd 100644
--- a/Documentation/devicetree/bindings/arm/l2cc.txt
+++ b/Documentation/devicetree/bindings/arm/l2cc.txt
@@ -44,6 +44,16 @@ Optional properties:
 - cache-id-part: cache id part number to be used if it is not present
   on hardware
 - wt-override: If present then L2 is forced to Write through mode
+- arm,double-linefill : Override double linefill enable setting. Enable if
+  non-zero, disable if zero.
+- arm,double-linefill-incr : Override double linefill on INCR read. Enable
+  if non-zero, disable if zero.
+- arm,double-linefill-wrap : Override double linefill on WRAP read. Enable
+  if non-zero, disable if zero.
+- arm,prefetch-drop : Override prefetch drop enable setting. Enable if non-zero,
+  disable if zero.
+- arm,prefetch-offset : Override prefetch offset value. Valid values are
+  0-7, 15, 23, and 31.
 
 Example:
 
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index c25cc13..de39865 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -1018,9 +1018,12 @@ static const struct l2c_init_data of_l2c220_data __initconst = {
 static void __init l2c310_of_parse(const struct device_node *np,
 	u32 *aux_val, u32 *aux_mask)
 {
+	bool set_prefetch = false;
 	u32 data[3] = { 0, 0, 0 };
 	u32 tag[3] = { 0, 0, 0 };
 	u32 filter[2] = { 0, 0 };
+	u32 prefetch;
+	u32 val;
 
 	of_property_read_u32_array(np, arm,tag-latency, tag, ARRAY_SIZE(tag));
 	if (tag[0]  tag[1]  tag[2])
@@ -1047,6 +1050,49 @@ static void __init l2c310_of_parse(const struct device_node *np,
 		writel_relaxed((filter[0]  ~(SZ_1M - 1)) | L310_ADDR_FILTER_EN,
 			   l2x0_base + L310_ADDR_FILTER_START);
 	}
+
+	prefetch = readl_relaxed(l2x0_base + L310_PREFETCH_CTRL);
+
+	if (!of_property_read_u32(np, arm,double-linefill, val)) {
+		if (val)
+			prefetch |= L310_PREFETCH_CTRL_DBL_LINEFILL;
+		else
+			prefetch = ~L310_PREFETCH_CTRL_DBL_LINEFILL;
+		set_prefetch = true;
+	}
+
+	if (!of_property_read_u32(np, arm,double-linefill-incr, val)) {
+		if (val

Re: [PATCH 0/5] Handle non-secure L2C initialization on Exynos4

[Tomasz Figa]

Hi Daniel,

On 12.06.2014 15:38, Daniel Drake wrote:
 Hi Tomasz,
 
 Thanks for working on this!
 
 I have just tried this, against Linus master
 64b2d1fbbfda07765dae3f601862796a61b2c451.
 Added patch ARM: dts: Initial ODROID U2 support and booted on
 ODROID-U2. I believe this board has the security enabled.
 
 Unfortunately, it hangs during early boot. With earlyprintk the last
 messages seen are:
 
 L2C: platform modifies aux control register: 0x0207 - 0x3e470001
 L2C: platform provided aux values permit register corruption.
 L2C: DT/platform modifies aux control register: 0x0207 - 0x3e470001
 L2C-310 enabling early BRESP for Cortex-A9
 L2C-310: enabling full line of zeros but not enabled in Cortex-A9
 L2C-310 ID prefetch enabled, offset 1 lines
 L2C-310 dynamic clock gating enabled, standby mode enabled
 L2C-310 cache controller enabled, 16 ways, 1024 kB
 L2C-310: CACHE_ID 0x4100c4c8, AUX_CTRL 0x7e470001
 

Thanks for testing. We have some ODROIDs U2 and U3 here too so I'll try
to reproduce and investigate the issue.

 I then tried to go back to the earlier patch ARM: EXYNOS: Add secure
 firmware support for l2x0 init (attached, needed a rebase) but that
 one also now hangs at:
 
 L2C: platform modifies aux control register: 0x0207 - 0x3e470001
 
 It did work on 3.14 though. Looking at the changelogs, many changes
 have been made to l2x0 recently. Can you confirm that you have tested
 your patches against a kernel with all of Russell King's recent
 changes?

I'm usually working on latest linux-next, so I'm pretty sure all the
mentioned patches were already in my tree. The base for this series was
next-20140610 tag of linux-next tree.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/5] Handle non-secure L2C initialization on Exynos4

[Tomasz Figa]

Hi Russell,

On 12.06.2014 18:20, Russell King - ARM Linux wrote:
 On Thu, Jun 12, 2014 at 02:38:49PM +0100, Daniel Drake wrote:
 From 2e67231f10ed0b05c2bacfdd05774fe21315d6da Mon Sep 17 00:00:00 2001
 From: Gu1 g...@aeroxteam.fr
 Date: Mon, 21 Jan 2013 04:13:56 +0100
 Subject: [PATCH] ARM: EXYNOS: Add secure firmware support for l2x0 init

 Conflicts:
  arch/arm/mm/cache-l2x0.c
 
 For this patch... it's a big NAK because it's taking us right back down
 the route of a totally fucked up cache-l2x0.c driver.

This is just an old, internal patch that was not going to be upstreamed.
I just posted another series yesterday[1], hopefully doing it the right
way. Looking forward for review comments.

[1] http://thread.gmane.org/gmane.linux.kernel/1722989

 
 Why can't you people write stuff properly?  There's already another set
 of patches on this mailing list which want to pass the virtual base
 address of the l2x0 controller to l2c_write_sec() so that various
 registers can be read back, because the platform's secure API can
 only update several registers at the same time.

Probably the series you mention is [1]? :)

 
 This is the same pattern that is revealed in this patch.  So, what
 this means is that the l2c_write_sec API is wrong.  We need to come
 up with a *replacement* API which allows the platforms to do this
 kind of setup in a *clean* way, and stop creating rotten hacks like
 this which just makes long term maintanence a nightmare.
 
 So... please start doing stuff properly.  If you don't, you're going
 to be getting more flames from me, especially if you start doing this
 kind of hackery on code that I've been cleaning up to get rid of such
 crap.
 

Yes, I fully agree. Fortunately that was not supposed to hit upstream.

You've done a lot of great work to refactor this driver (thanks!), which
made it possible to support Exynos secure firmware in a sane way and
this is how it should be done.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/5] Handle non-secure L2C initialization on Exynos4

[Tomasz Figa]

This series intends to add support for L2 cache on Exynos4 SoCs on boards
running under secure firmware, which requires certain initialization steps
to be done with help of firmware, as selected registers are writable only
from secure mode.

First three patches extend existing support for secure write in L2C driver
to account for design of secure firmware running on Exynos. Namely:
 1) direct read access to certain registers is needed on Exynos, because
secure firmware calls set several registers at once,
 2) not all boards are running secure firmware, so .write_sec callback
needs to be installed in Exynos firmware ops initialization code,
 3) write access to {DATA,TAG}_LATENCY_CTRL registers fron non-secure world
is not allowed and so must use l2c_write_sec as well.
Those patches might affect other platforms using .write_sec callback, so
I'd like to kindly ask any interested people for testing.

Further two patches add impelmentation of .write_sec for Exynos secure
firmware and necessary DT nodes to enable L2 cache.

Tested on Exynos4210-based Universal C210 board (without secure firmware)
and Exynos4412-based TRATS2 board (with secure firmware).

Tomasz Figa (5):
  ARM: mm: cache-l2x0: Add base address argument to write_sec callback
  ARM: Get outer cache .write_sec callback from mach_desc only if not
NULL
  ARM: mm: cache-l2x0: Use l2c_write_sec() for LATENCY_CTRL registers
  ARM: EXYNOS: Add .write_sec outer cache callback for L2C-310
  ARM: dts: exynos4: Add nodes for L2 cache controller

 arch/arm/boot/dts/exynos4210.dtsi  |  9 ++
 arch/arm/boot/dts/exynos4x12.dtsi  |  9 ++
 arch/arm/include/asm/mach/arch.h   |  3 +-
 arch/arm/include/asm/outercache.h  |  2 +-
 arch/arm/kernel/irq.c  |  3 +-
 arch/arm/mach-exynos/firmware.c| 61 ++
 arch/arm/mach-highbank/highbank.c  |  3 +-
 arch/arm/mach-omap2/omap4-common.c |  3 +-
 arch/arm/mach-ux500/cache-l2x0.c   |  3 +-
 arch/arm/mm/cache-l2x0.c   | 10 +++
 10 files changed, 95 insertions(+), 11 deletions(-)

-- 
1.9.3

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5] ARM: mm: cache-l2x0: Add base address argument to write_sec callback

[Tomasz Figa]

For certain platforms (e.g. Exynos) it is necessary to read back some
values from registers before they can be written (i.e. SMC calls that
set multiple registers per call), so base address of L2C controller is
needed for .write_sec operation. This patch adds base argument to
.write_sec callback so that its implementation can also access registers
directly.

Signed-off-by: Tomasz Figa t.f...@samsung.com
---
 arch/arm/include/asm/mach/arch.h   | 3 ++-
 arch/arm/include/asm/outercache.h  | 2 +-
 arch/arm/mach-highbank/highbank.c  | 3 ++-
 arch/arm/mach-omap2/omap4-common.c | 3 ++-
 arch/arm/mach-ux500/cache-l2x0.c   | 3 ++-
 arch/arm/mm/cache-l2x0.c   | 2 +-
 6 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h
index 060a75e..ddaebcd 100644
--- a/arch/arm/include/asm/mach/arch.h
+++ b/arch/arm/include/asm/mach/arch.h
@@ -46,7 +46,8 @@ struct machine_desc {
enum reboot_modereboot_mode;/* default restart mode */
unsignedl2c_aux_val;/* L2 cache aux value   */
unsignedl2c_aux_mask;   /* L2 cache aux mask*/
-   void(*l2c_write_sec)(unsigned long, unsigned);
+   void(*l2c_write_sec)(void __iomem *,
+unsigned long, unsigned);
struct smp_operations   *smp;   /* SMP operations   */
bool(*smp_init)(void);
void(*fixup)(struct tag *, char **);
diff --git a/arch/arm/include/asm/outercache.h 
b/arch/arm/include/asm/outercache.h
index 891a56b..5cc703b 100644
--- a/arch/arm/include/asm/outercache.h
+++ b/arch/arm/include/asm/outercache.h
@@ -35,7 +35,7 @@ struct outer_cache_fns {
void (*resume)(void);
 
/* This is an ARM L2C thing */
-   void (*write_sec)(unsigned long, unsigned);
+   void (*write_sec)(void __iomem *, unsigned long, unsigned);
 };
 
 extern struct outer_cache_fns outer_cache;
diff --git a/arch/arm/mach-highbank/highbank.c 
b/arch/arm/mach-highbank/highbank.c
index 8c35ae4..2bd3243 100644
--- a/arch/arm/mach-highbank/highbank.c
+++ b/arch/arm/mach-highbank/highbank.c
@@ -51,7 +51,8 @@ static void __init highbank_scu_map_io(void)
 }
 
 
-static void highbank_l2c310_write_sec(unsigned long val, unsigned reg)
+static void highbank_l2c310_write_sec(void __iomem *base,
+ unsigned long val, unsigned reg)
 {
if (reg == L2X0_CTRL)
highbank_smc1(0x102, val);
diff --git a/arch/arm/mach-omap2/omap4-common.c 
b/arch/arm/mach-omap2/omap4-common.c
index 326cd98..bdbe658 100644
--- a/arch/arm/mach-omap2/omap4-common.c
+++ b/arch/arm/mach-omap2/omap4-common.c
@@ -167,7 +167,8 @@ void __iomem *omap4_get_l2cache_base(void)
return l2cache_base;
 }
 
-static void omap4_l2c310_write_sec(unsigned long val, unsigned reg)
+static void omap4_l2c310_write_sec(void __iomem *base,
+  unsigned long val, unsigned reg)
 {
unsigned smc_op;
 
diff --git a/arch/arm/mach-ux500/cache-l2x0.c b/arch/arm/mach-ux500/cache-l2x0.c
index 842ebed..35c2623 100644
--- a/arch/arm/mach-ux500/cache-l2x0.c
+++ b/arch/arm/mach-ux500/cache-l2x0.c
@@ -35,7 +35,8 @@ static int __init ux500_l2x0_unlock(void)
return 0;
 }
 
-static void ux500_l2c310_write_sec(unsigned long val, unsigned reg)
+static void ux500_l2c310_write_sec(void __iomem *base,
+  unsigned long val, unsigned reg)
 {
/*
 * We can't write to secure registers as we are in non-secure
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index efc5cab..1695eab 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -72,7 +72,7 @@ static void l2c_write_sec(unsigned long val, void __iomem 
*base, unsigned reg)
if (val == readl_relaxed(base + reg))
return;
if (outer_cache.write_sec)
-   outer_cache.write_sec(val, reg);
+   outer_cache.write_sec(base, val, reg);
else
writel_relaxed(val, base + reg);
 }
-- 
1.9.3

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/5] ARM: dts: exynos4: Add nodes for L2 cache controller

[Tomasz Figa]

This patch adds device tree nodes for L2 cache controller present on
Exynos4 SoCs.

Signed-off-by: Tomasz Figa t.f...@samsung.com
---
 arch/arm/boot/dts/exynos4210.dtsi | 9 +
 arch/arm/boot/dts/exynos4x12.dtsi | 9 +
 2 files changed, 18 insertions(+)

diff --git a/arch/arm/boot/dts/exynos4210.dtsi 
b/arch/arm/boot/dts/exynos4210.dtsi
index ee3001f..99970ab 100644
--- a/arch/arm/boot/dts/exynos4210.dtsi
+++ b/arch/arm/boot/dts/exynos4210.dtsi
@@ -54,6 +54,15 @@
reg = 0x10023CA0 0x20;
};
 
+   l2c: l2-cache-controller@10502000 {
+   compatible = arm,pl310-cache;
+   reg = 0x10502000 0x1000;
+   cache-unified;
+   cache-level = 2;
+   arm,tag-latency = 2 2 1;
+   arm,data-latency = 2 2 1;
+   };
+
gic: interrupt-controller@1049 {
cpu-offset = 0x8000;
};
diff --git a/arch/arm/boot/dts/exynos4x12.dtsi 
b/arch/arm/boot/dts/exynos4x12.dtsi
index c5a943d..9487f9c 100644
--- a/arch/arm/boot/dts/exynos4x12.dtsi
+++ b/arch/arm/boot/dts/exynos4x12.dtsi
@@ -60,6 +60,15 @@
reg = 0x10023CA0 0x20;
};
 
+   l2c: l2-cache-controller@10502000 {
+   compatible = arm,pl310-cache;
+   reg = 0x10502000 0x1000;
+   cache-unified;
+   cache-level = 2;
+   arm,tag-latency = 2 2 1;
+   arm,data-latency = 3 2 1;
+   };
+
clock: clock-controller@1003 {
compatible = samsung,exynos4412-clock;
reg = 0x1003 0x2;
-- 
1.9.3

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/5] ARM: mm: cache-l2x0: Use l2c_write_sec() for LATENCY_CTRL registers

[Tomasz Figa]

According to the documentation, TAG_LATENCY_CTRL and DATA_LATENCY_CTRL
registers of L2C-310 can be written only in secure mode, so
l2c_write_sec() should be used to change them, instead of plain
writel_relaxed().

Signed-off-by: Tomasz Figa t.f...@samsung.com
---
 arch/arm/mm/cache-l2x0.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 1695eab..0180eb7 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -1024,20 +1024,20 @@ static void __init l2c310_of_parse(const struct 
device_node *np,
 
of_property_read_u32_array(np, arm,tag-latency, tag, ARRAY_SIZE(tag));
if (tag[0]  tag[1]  tag[2])
-   writel_relaxed(
+   l2c_write_sec(
L310_LATENCY_CTRL_RD(tag[0] - 1) |
L310_LATENCY_CTRL_WR(tag[1] - 1) |
L310_LATENCY_CTRL_SETUP(tag[2] - 1),
-   l2x0_base + L310_TAG_LATENCY_CTRL);
+   l2x0_base, L310_TAG_LATENCY_CTRL);
 
of_property_read_u32_array(np, arm,data-latency,
   data, ARRAY_SIZE(data));
if (data[0]  data[1]  data[2])
-   writel_relaxed(
+   l2c_write_sec(
L310_LATENCY_CTRL_RD(data[0] - 1) |
L310_LATENCY_CTRL_WR(data[1] - 1) |
L310_LATENCY_CTRL_SETUP(data[2] - 1),
-   l2x0_base + L310_DATA_LATENCY_CTRL);
+   l2x0_base, L310_DATA_LATENCY_CTRL);
 
of_property_read_u32_array(np, arm,filter-ranges,
   filter, ARRAY_SIZE(filter));
-- 
1.9.3

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/5] ARM: Get outer cache .write_sec callback from mach_desc only if not NULL

[Tomasz Figa]

Certain platforms (i.e. Exynos) might need to set .write_sec callback
from firmware initialization which is happenning in .init_early callback
of machine descriptor. However current code will overwrite the pointer
with whatever is present in machine descriptor, even though it can be
already set earlier. This patch fixes this by making the assignment
conditional, depending on whether current .write_sec callback is NULL.

Signed-off-by: Tomasz Figa t.f...@samsung.com
---
 arch/arm/kernel/irq.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kernel/irq.c b/arch/arm/kernel/irq.c
index 2c42576..e7383b9 100644
--- a/arch/arm/kernel/irq.c
+++ b/arch/arm/kernel/irq.c
@@ -125,7 +125,8 @@ void __init init_IRQ(void)
 
if (IS_ENABLED(CONFIG_OF)  IS_ENABLED(CONFIG_CACHE_L2X0) 
(machine_desc-l2c_aux_mask || machine_desc-l2c_aux_val)) {
-   outer_cache.write_sec = machine_desc-l2c_write_sec;
+   if (!outer_cache.write_sec)
+   outer_cache.write_sec = machine_desc-l2c_write_sec;
ret = l2x0_of_init(machine_desc-l2c_aux_val,
   machine_desc-l2c_aux_mask);
if (ret)
-- 
1.9.3

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/5] ARM: EXYNOS: Add .write_sec outer cache callback for L2C-310

[Tomasz Figa]

Exynos4 SoCs equipped with an L2C-310 cache controller and running under
secure firmware require certain registers of aforementioned IP to be
accessed only from secure mode. This means that SMC calls are required
for certain register writes. To handle this, an implementation of
.write_sec callback is provided by this patch.

Signed-off-by: Tomasz Figa t.f...@samsung.com
---
 arch/arm/mach-exynos/firmware.c | 61 +
 1 file changed, 61 insertions(+)

diff --git a/arch/arm/mach-exynos/firmware.c b/arch/arm/mach-exynos/firmware.c
index eb91d23..34f7798 100644
--- a/arch/arm/mach-exynos/firmware.c
+++ b/arch/arm/mach-exynos/firmware.c
@@ -14,7 +14,9 @@
 #include linux/of.h
 #include linux/of_address.h
 
+#include asm/cputype.h
 #include asm/firmware.h
+#include asm/hardware/cache-l2x0.h
 
 #include mach/map.h
 
@@ -70,6 +72,55 @@ static const struct firmware_ops exynos_firmware_ops = {
.cpu_boot   = exynos_cpu_boot,
 };
 
+static void exynos_l2_write_sec(void __iomem *base, unsigned long val,
+   unsigned reg)
+{
+   switch (reg) {
+   case L2X0_CTRL:
+   exynos_smc(SMC_CMD_L2X0CTRL, val, 0, 0);
+   break;
+
+   case L2X0_AUX_CTRL:
+   exynos_smc(SMC_CMD_L2X0SETUP2,
+   readl_relaxed(base + L310_POWER_CTRL),
+   val, 0);
+   break;
+
+   case L2X0_DEBUG_CTRL:
+   exynos_smc(SMC_CMD_L2X0DEBUG, val, 0, 0);
+   break;
+
+   case L310_TAG_LATENCY_CTRL:
+   exynos_smc(SMC_CMD_L2X0SETUP1,
+   val,
+   readl_relaxed(base + L310_DATA_LATENCY_CTRL),
+   readl_relaxed(base + L310_PREFETCH_CTRL));
+   break;
+
+   case L310_DATA_LATENCY_CTRL:
+   exynos_smc(SMC_CMD_L2X0SETUP1,
+   readl_relaxed(base + L310_TAG_LATENCY_CTRL),
+   val,
+   readl_relaxed(base + L310_PREFETCH_CTRL));
+   break;
+
+   case L310_PREFETCH_CTRL:
+   exynos_smc(SMC_CMD_L2X0SETUP1,
+   readl_relaxed(base + L310_TAG_LATENCY_CTRL),
+   readl_relaxed(base + L310_DATA_LATENCY_CTRL),
+   val);
+   break;
+
+   case L310_POWER_CTRL:
+   exynos_smc(SMC_CMD_L2X0SETUP2, val,
+   readl_relaxed(base + L2X0_AUX_CTRL), 0);
+   break;
+
+   default:
+   WARN_ONCE(1, %s: ignoring write to reg 0x%x\n, __func__, reg);
+   }
+}
+
 void __init exynos_firmware_init(void)
 {
struct device_node *nd;
@@ -89,4 +140,14 @@ void __init exynos_firmware_init(void)
pr_info(Running under secure firmware.\n);
 
register_firmware_ops(exynos_firmware_ops);
+
+   /*
+* Exynos 4 SoCs (based on Cortex A9 and equipped with L2C-310),
+* running under secure firmware, require certain registers of L2
+* cache controller to be written in secure mode. Here .write_sec
+* callback is provided to perform necessary SMC calls.
+*/
+   if (IS_ENABLED(CONFIG_CACHE_L2X0)
+read_cpuid_part_number() == ARM_CPU_PART_CORTEX_A9)
+   outer_cache.write_sec = exynos_l2_write_sec;
 }
-- 
1.9.3

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] ARM: mm: cache-l2x0: Add base address argument to write_sec callback

[Tomasz Figa]

On 11.06.2014 18:00, Jon Loeliger wrote:
 diff --git a/arch/arm/include/asm/mach/arch.h 
 b/arch/arm/include/asm/mach/arch.h
 index 060a75e..ddaebcd 100644
 --- a/arch/arm/include/asm/mach/arch.h
 +++ b/arch/arm/include/asm/mach/arch.h
 @@ -46,7 +46,8 @@ struct machine_desc {
 enum reboot_modereboot_mode;/* default restart mode */
 unsignedl2c_aux_val;/* L2 cache aux value   */
 unsignedl2c_aux_mask;   /* L2 cache aux mask*/
 -   void(*l2c_write_sec)(unsigned long, unsigned);
 +   void(*l2c_write_sec)(void __iomem *,
 +unsigned long, unsigned);
 struct smp_operations   *smp;   /* SMP operations   */
 bool(*smp_init)(void);
 void(*fixup)(struct tag *, char **);
 
 diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
 index efc5cab..1695eab 100644
 --- a/arch/arm/mm/cache-l2x0.c
 +++ b/arch/arm/mm/cache-l2x0.c
 @@ -72,7 +72,7 @@ static void l2c_write_sec(unsigned long val, void __iomem 
 *base, unsigned reg)
 if (val == readl_relaxed(base + reg))
 return;
 if (outer_cache.write_sec)
 -   outer_cache.write_sec(val, reg);
 +   outer_cache.write_sec(base, val, reg);
 else
 writel_relaxed(val, base + reg);
  }
 
 The parameter order (base, val, reg) seems very non-intuitive.
 Are you matching some existing prototype or adhering to some
 backwards compatibility issue?  If not wouldn't, say, (base, reg, val)
 or (val, base, reg) be more intuitive?

Hmm, I didn't think too much about this, so this order is just whatever
first came to my mind, probably because I'm used to xxx_write(ctx, val,
reg) accessors found in many drivers.

Anyway, l2c_write_sec() in arm/mm/cache-l2x0.c, which calls
outer_cache.write_sec(), already uses (val, base, reg) convention, so
probably this one would be most suitable. I'll change in v2.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 3/4] of/clk: Register clocks suitable for Runtime PM with the PM core

[Tomasz Figa]

On 24.04.2014 15:11, Ulf Hansson wrote:

On 24 April 2014 12:13, Geert Uytterhoeven geert+rene...@glider.be wrote:

When adding a device from DT, check if its clocks are suitable for Runtime
PM, and register them with the PM core.
If Runtime PM is disabled, just enable the clock.

This allows the PM core to automatically manage gate clocks of devices for
Runtime PM.


Normally I don't think it's a good idea to automatically manage
clocks from PM core or any other place but from the driver (and
possibly the subsystem).

The reason is simply that we hide things that normally is supposed to
be handled by the driver. Typically a cross SOC driver should work
fine both with and without a pm_domain. It should also not rely on
CONFIG_PM_RUNTIME.



Signed-off-by: Geert Uytterhoeven geert+rene...@glider.be
---
  drivers/of/Makefile|1 +
  drivers/of/of_clk.c|  103 
  drivers/of/platform.c  |3 ++
  include/linux/of_clk.h |   18 +
  4 files changed, 125 insertions(+)
  create mode 100644 drivers/of/of_clk.c
  create mode 100644 include/linux/of_clk.h

diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index ed9660adad77..49bcd413906f 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -10,3 +10,4 @@ obj-$(CONFIG_OF_PCI)  += of_pci.o
  obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
  obj-$(CONFIG_OF_MTD)   += of_mtd.o
  obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
+obj-$(CONFIG_COMMON_CLK) += of_clk.o
diff --git a/drivers/of/of_clk.c b/drivers/of/of_clk.c
new file mode 100644
index ..35f5e9f3dd42
--- /dev/null
+++ b/drivers/of/of_clk.c
@@ -0,0 +1,103 @@
+/*
+ *  Copyright (C) 2014 Glider bvba
+ */
+
+#include linux/clk.h
+#include linux/err.h
+#include linux/of.h
+#include linux/of_clk.h
+#include linux/platform_device.h
+#include linux/pm_clock.h
+#include linux/pm_runtime.h
+
+
+#ifdef CONFIG_PM_RUNTIME
+
+static int of_clk_pm_runtime_suspend(struct device *dev)
+{
+   int ret;
+
+   ret = pm_generic_runtime_suspend(dev);
+   if (ret)
+   return ret;
+
+   ret = pm_clk_suspend(dev);
+   if (ret) {
+   pm_generic_runtime_resume(dev);
+   return ret;
+   }
+
+   return 0;
+}
+
+static int of_clk_pm_runtime_resume(struct device *dev)
+{
+   pm_clk_resume(dev);
+   return pm_generic_runtime_resume(dev);
+}
+
+static struct dev_pm_domain of_clk_pm_domain = {
+   .ops = {
+   .runtime_suspend = of_clk_pm_runtime_suspend,
+   .runtime_resume = of_clk_pm_runtime_resume,
+   USE_PLATFORM_PM_SLEEP_OPS
+   },
+};
+
+static int of_clk_register(struct device *dev, struct clk *clk)
+{
+   int error;
+
+   if (!dev-pm_domain) {
+   error = pm_clk_create(dev);
+   if (error)
+   return error;
+
+   dev-pm_domain = of_clk_pm_domain;


I am concerned about how this will work in conjunction with the
generic power domain.

A device can't reside in more than one pm_domain; thus I think it
would be better to always use the generic power domain and not have a
specific one for clocks. Typically the genpd should invoke
pm_clk_resume|suspend from it's runtime PM callbacks.


I'm not sure about this. A typical use case would be to gate clocks ASAP 
and then wait until device is idle long enough to consider turning off 
the power domain worthwhile. Also sometimes we may want to gate the 
clocks, but prevent power domain from being powered off to retain 
hardware state (e.g. because there is no way to read it and restore later).


I believe, though, that for devices that are not inside a controllable 
power domain, this might be a good solution.


Best regards,
Tomasz
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv9 01/43] clk: Add support for regmap register read/write

[Tomasz Figa]

Hi Tero,

On Friday 25 of October 2013 18:56:55 Tero Kristo wrote:
 Previously, only direct register read/write was supported. Now a
 per-clock regmap can be provided for same purpose, which allows the
 clock drivers to access clock registers behind e.g. I2C bus.
 
 Signed-off-by: Tero Kristo t-kri...@ti.com
 ---
  drivers/clk/clk-divider.c|6 +++---
  drivers/clk/clk-gate.c   |6 +++---
  drivers/clk/clk-mux.c|6 +++---
  include/linux/clk-provider.h |   23 +++
  4 files changed, 28 insertions(+), 13 deletions(-)

The idea itself sounds not bad, but patch description fails to specify 
what is the need for this. I mean, if you know that this will have some 
actual users, it is nice to specify them.

Also IMHO the variant with clk_reg_ops (but with regmap used) would look a 
bit better in terms of readability and extensibility, but I'm leaving this 
completely to Mike's decision.

(OR maybe you could migrate this fully to regmap, which can do MMIO 
accesses as well, without the need of having separate code paths for both 
in clock code?)

Best regards,
Tomasz

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv9 05/43] CLK: TI: add DT alias clock registration mechanism

[Tomasz Figa]

Hi Matt,

On Tuesday 29 of October 2013 12:50:43 Matt Sealey wrote:
 On Fri, Oct 25, 2013 at 10:56 AM, Tero Kristo t-kri...@ti.com wrote:
  Some devices require their clocks to be available with a specific
  dev-id con-id mapping. With DT, the clocks can be found by default
  only with their name, or alternatively through the device node of
  the consumer. With drivers, that don't support DT fully yet, add
  mechanism to register specific clock names.
  
  Signed-off-by: Tero Kristo t-kri...@ti.com
  Tested-by: Nishanth Menon n...@ti.com
  Acked-by: Tony Lindgren t...@atomide.com
 
 You guys are wonderful, by the way ;)
 
  +void __init ti_dt_clocks_register(struct ti_dt_clk oclks[])
  +{
  +   struct ti_dt_clk *c;
  +   struct device_node *node;
  +   struct clk *clk;
  +   struct of_phandle_args clkspec;
  +
  +   for (c = oclks; c-node_name != NULL; c++) {
  +   node = of_find_node_by_name(NULL, c-node_name);
  +   clkspec.np = node;
  +   clk = of_clk_get_from_provider(clkspec);
  +
  +   if (!IS_ERR(clk)) {
  +   c-lk.clk = clk;
  +   clkdev_add(c-lk);
  +   } else {
  +   pr_warn(%s: failed to lookup clock node
  %s\n,
  +   __func__, c-node_name);
  +   }
  +   }
  +}
 
 I have one question here, what makes this part of the patch
 TI-specific at all except the definition of the structure ti_dt_clk?
 Mapping DT clocks to generic clkdev legacy namings seems like it would
 be a necessary evil across *all* SoC platforms.

SoC platforms that are fully converted to Device Tree do not need any 
legacy clkdev namings, because they have all clkdev look-ups performed 
using data from DT (see generic clock bindings).

So this would be not *all* SoC platforms, but instead SoC platforms, 
which are currently undergoing conversion to DT or conversion of which is 
yet to be started.

Still this might be a good enough justification for having a generic 
solution for this problem, but I would wait with this until the real need 
on some of such platforms shows up. I don't see anything wrong in 
implementing this first as TI-specific quirk and then possibly making it 
generic if such need shows up.

By the way, we already have something like this in clk/samsung/clk.c . 
This is called aliases there and allows registering clkdev look-ups for 
particular clocks. However this is specific to our clock drivers that 
register all clocks inside a single clock privder, based on static ID 
assignment and all clocks statically listed in clock driver - see 
clk/samsung/clk-s3c64xx.c for an example.

Best regards,
Tomasz

 I would say there is a good case for this being generic and used by
 all platforms waiting for con-id/dev-id to be moved to DT-awareness,
 the structure itself is very generic within clkdev as long as the
 drivers conformed anyway (and if they didn't, none of this helps) and
 I don't think it really needs to be a TI-only thing. The next thing
 that's going to happen when another platform arrives that wants to do
 the same thing is duplication or consolidation - so it would probably
 be a good idea to make this generic to start.
 
 While there the names could be a bit more descriptive -
 dt_clk_name_lookup_map_register(), struct dt_clk_name_lookup_map,
 DT_CLK_LOOKUP_MAP() ..? The current namespace makes it look like this
 is the absolute correct thing to do under all circumstances, but in
 fact this is purely for mapping the old way to the new, better way..
 in the event a brand new platform arrives, with all new drivers (or
 only compliant drivers), none of this code should be implemented for
 it and it should be presented this way.
 
 Thanks,
 Matt Sealey n...@bakuhatsu.net
 
 ___
 linux-arm-kernel mailing list
 linux-arm-ker...@lists.infradead.org
 http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] wl1251: spi: add device tree support

[Tomasz Figa]

On Monday 28 of October 2013 01:37:34 Kumar Gala wrote:
 On Oct 27, 2013, at 11:14 AM, Sebastian Reichel wrote:
  Add device tree support for the spi variant of wl1251
  and document the binding.
  
  Signed-off-by: Sebastian Reichel s...@debian.org
  ---
  .../devicetree/bindings/net/wireless/ti,wl1251.txt | 36
  ++ drivers/net/wireless/ti/wl1251/spi.c  
  | 23 ++ 2 files changed, 53 insertions(+), 6
  deletions(-)
  create mode 100644
  Documentation/devicetree/bindings/net/wireless/ti,wl1251.txt
  
  diff --git
  a/Documentation/devicetree/bindings/net/wireless/ti,wl1251.txt
  b/Documentation/devicetree/bindings/net/wireless/ti,wl1251.txt new
  file mode 100644
  index 000..5f8a154
  --- /dev/null
  +++ b/Documentation/devicetree/bindings/net/wireless/ti,wl1251.txt
  @@ -0,0 +1,36 @@
  +* Texas Instruments wl1251 controller
  +
  +The wl1251 chip can be connected via SPI or via SDIO. The linux
  +kernel currently only supports device tree for the SPI variant.
  +
 
 From the binding I have no idea what this chip actually does, also we
 don't normally reference linux kernel support in bindings specs (so
 please remove it).
 
 However, what would expect the SDIO binding to look like?  Or more
 specifically, how would you distinguish the SPI vs SDIO
 binding/connection?  I'm wondering if the compatible should be
 something like ti,wl1251-spi and than the sdio can be
 ti,wl1251-sdio

Well, you can easily distinguish an SDIO device from an SPI device by its 
parent node, but...

The binding for SDIO might require different set of properties (other than 
ones inherited from generic SDIO or SPI bindings) than one for SPI. So 
probably different compatible values might be justified.

Did we already have such case before? (maybe some I2C + SPI devices?)

  +Required properties:
  +- compatible : Should be ti,wl1251
 
 reg is not listed as a required prop.

It is implied by SPI bindings, but it might be a good idea to have this 
stated here as well.

 
  +- interrupts : Should contain interrupt line
  +- interrupt-parent : Should be the phandle for the interrupt
  +  controller that services interrupts for this device
  +- vio-supply : phandle to regulator providing VIO
  +- power-gpio : GPIO connected to chip's PMEN pin
 
 should be vendor prefixed: ti,power-gpio

Hmm, out of curiosity, is it a rule for this kind of properties? I can see 
both cases with and without prefixes when grepping for -gpios in 
Documentation/devicetree/bindings. We should really have such things 
written down somewhere.

 
  +- For additional required properties on SPI, please consult
  +  Documentation/devicetree/bindings/spi/spi-bus.txt
  +
  +Optional properties:
  +- ti,use-eeprom : If found, configuration will be loaded from eeprom.
 
 can you be a bit more specific on what cfg will be loaded.  Also, is
 this property a boolean, if so how do I know which eeprom the cfg is
 loaded from (is it one that is directly connected to the wl1251?

Maybe one from ti,has-eeprom or ti,config-eeprom would be better name for 
this property?

Best regards,
Tomasz

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] gpio: pcf857x: Add OF support

[Tomasz Figa]

Hi Laurent,

On Tuesday 27 of August 2013 10:02:39 Laurent Pinchart wrote:
 Add DT bindings for the pcf857x-compatible chips and parse the device
 tree node in the driver.
 
 Signed-off-by: Laurent Pinchart
 laurent.pinchart+rene...@ideasonboard.com ---
  .../devicetree/bindings/gpio/gpio-pcf857x.txt  | 71
 ++ drivers/gpio/gpio-pcf857x.c 
   | 44 +++--- 2 files changed, 107 insertions(+), 8
 deletions(-)
  create mode 100644
 Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt
 
 Changes since v4:
 
 - Don't try to get ngpio from of_device_id data, we already get it from
   i2c_device_id

Hmm, I'm not sure how this is supposed to work.

How does the I2C core resolve OF compatible name to particular entry in 
id_table? I believe it simply passes NULL as the second argument of 
.probe() if the device is instantiated based on OF compatible string and 
not one in the legacy ID table.

 
 Changes since v3:
 
 - Get rid of the #ifdef CONFIG_OF in the probe function
 - Give DT node priority over platform data
 
 Changes since v2:
 
 - Replace mention about interrupts software configuration in DT bindings
 documentation with an explanation of the hardware configuration.
 
 diff --git a/Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt
 b/Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt new file mode
 100644
 index 000..d261391
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt
 @@ -0,0 +1,71 @@
 +* PCF857x-compatible I/O expanders
 +
 +The PCF857x-compatible chips have quasi-bidirectional I/O pins that
 can be +driven high by a pull-up current source or driven low to
 ground. This combines +the direction and output level into a single bit
 per pin, which can't be read +back. We can't actually know at
 initialization time whether a pin is configured +(a) as output and
 driving the signal low/high, or (b) as input and reporting a +low/high
 value, without knowing the last value written since the chip came out
 +of reset (if any). The only reliable solution for setting up pin
 direction is +thus to do it explicitly.
 +
 +Required Properties:
 +
 +  - compatible: should be one of the following.
 +- maxim,max7328: For the Maxim MAX7378
 +- maxim,max7329: For the Maxim MAX7329
 +- nxp,pca8574: For the NXP PCA8574
 +- nxp,pca8575: For the NXP PCA8575
 +- nxp,pca9670: For the NXP PCA9670
 +- nxp,pca9671: For the NXP PCA9671
 +- nxp,pca9672: For the NXP PCA9672
 +- nxp,pca9673: For the NXP PCA9673
 +- nxp,pca9674: For the NXP PCA9674
 +- nxp,pca9675: For the NXP PCA9675
 +- nxp,pcf8574: For the NXP PCF8574
 +- nxp,pcf8574a: For the NXP PCF8574A
 +- nxp,pcf8575: For the NXP PCF8575
 +- ti,tca9554: For the TI TCA9554
 +
 +  - reg: I2C slave address.
 +
 +  - gpio-controller: Marks the device node as a gpio controller.
 +  - #gpio-cells: Should be 2. The first cell is the GPIO number and the
 second +cell specifies GPIO flags, as defined in
 dt-bindings/gpio/gpio.h. Only the +GPIO_ACTIVE_HIGH and
 GPIO_ACTIVE_LOW flags are supported. +
 +Optional Properties:
 +
 +  - pins-initial-state: Bitmask that specifies the initial state of
 each pin. +  When a bit is set to zero, the corresponding pin will be
 initialized to the +  input (pulled-up) state. When the  bit is set to
 one, the pin will be +  initialized the the low-level output state. If
 the property is not specified +  all pins will be initialized to the
 input state.
 +
 +  The I/O expander can detect input state changes, and thus optionally
 act as +  an interrupt controller. When the expander interrupt pin is
 connected all the +  following properties must be set. For more
 information please see the +  interrupt controller device tree bindings
 documentation available at + 
 Documentation/devicetree/bindings/interrupt-controller/interrupts.txt.
 +
 +  - interrupt-controller: Identifies the node as an interrupt
 controller. +  - #interrupt-cells: Number of cells to encode an
 interrupt source, shall be 2. +  - interrupt-parent: phandle of the
 parent interrupt controller. +  - interrupts: Interrupt specifier for
 the controllers interrupt. +
 +
 +Please refer to gpio.txt in this directory for details of the common
 GPIO +bindings used by client devices.
 +
 +Example: PCF8575 I/O expander node
 +
 + pcf8575: gpio@20 {
 + compatible = nxp,pcf8575;
 + reg = 0x20;
 + interrupt-parent = irqpin2;
 + interrupts = 3 0;
 + gpio-controller;
 + #gpio-cells = 2;
 + interrupt-controller;
 + #interrupt-cells = 2;
 + };
 diff --git a/drivers/gpio/gpio-pcf857x.c b/drivers/gpio/gpio-pcf857x.c
 index 9e61bb0..864dd8c 100644
 --- a/drivers/gpio/gpio-pcf857x.c
 +++ b/drivers/gpio/gpio-pcf857x.c
 @@ -26,6 +26,8 @@
  #include linux/irqdomain.h
  #include linux/kernel.h
  #include linux/module.h
 +#include linux/of.h
 

Re: [PATCH v5] gpio: pcf857x: Add OF support

[Tomasz Figa]

On Tuesday 27 of August 2013 14:00:24 Archit Taneja wrote:
 Hi,
 
 On Tuesday 27 August 2013 01:44 PM, Tomasz Figa wrote:
  Hi Laurent,
  
  On Tuesday 27 of August 2013 10:02:39 Laurent Pinchart wrote:
  Add DT bindings for the pcf857x-compatible chips and parse the device
  tree node in the driver.
  
  Signed-off-by: Laurent Pinchart
  laurent.pinchart+rene...@ideasonboard.com ---
  
.../devicetree/bindings/gpio/gpio-pcf857x.txt  | 71
  
  ++ drivers/gpio/gpio-pcf857x.c
  
 | 44 +++--- 2 files changed, 107 insertions(+), 8
  
  deletions(-)
  
create mode 100644
  
  Documentation/devicetree/bindings/gpio/gpio-pcf857x.txt
  
  Changes since v4:
  
  - Don't try to get ngpio from of_device_id data, we already get it
  from
  
 i2c_device_id
  
  Hmm, I'm not sure how this is supposed to work.
  
  How does the I2C core resolve OF compatible name to particular entry in
  id_table? I believe it simply passes NULL as the second argument of
  .probe() if the device is instantiated based on OF compatible string
  and
  not one in the legacy ID table.
 
 It doesn't pass the second argument as NULL. If you look at
 i2c_device_probe() in drivers/i2c/i2c-core.c, the second argument to
 probe is passed as: i2c_match_id(driver-id_table, client)
 
 This will extract the i2c_device_id pointer from the id_table.

Yes, there is a chance that it will not return NULL, but I think that 
relying on this is rather flawed.

If you look at the whole code path, you can see that it's only a 
coincidence that this works. See the execution flow:
 - I2C adapter driver calls of_i2c_register_devices(),
 - of_i2c_register_devices() calls of_modalias_node() for every device on 
this bus,
 - of_modalias_node() stores the second substring of compatible string 
separated by a comma, if there is one or the whole compatible otherwise to 
the output buffer (type field of i2c_board_info struct, as passed by 
of_i2c_register_devices()),
 - of_i2c_register_devices() then calls i2c_new_device() with the resulting 
info struct,
 - i2c_new_device() takes info-type and copies its contents to client-
name,
 - then a bit later, I2C core calls i2c_match_id(), which does matching of 
client-name against id_table of the driver and the resulting i2c_device_id 
entry (or NULL) is then passed to driver's .probe() callback.

So if it happens that compatible is not equal to vendor,ID from legacy 
I2C table, then the matching will fail and NULL will be passed.

[CCing Wolfram and Grant, as they should now more about this behavior and 
whether it's intentional or no]

Best regards,
Tomasz

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/15] drivers: phy: add generic PHY framework

[Tomasz Figa]

On Tuesday 13 of August 2013 16:14:44 Kishon Vijay Abraham I wrote:
 Hi,
 
 On Wednesday 31 July 2013 11:45 AM, Felipe Balbi wrote:
  Hi,
  
  On Wed, Jul 31, 2013 at 11:14:32AM +0530, Kishon Vijay Abraham I wrote:
  IMHO we need a lookup method for PHYs, just like for clocks,
  regulators, PWMs or even i2c busses because there are complex
  cases
  when passing just a name using platform data will not work. I
  would
  second what Stephen said [1] and define a structure doing things
  in a
  DT-like way.
  
  Example;
  
  [platform code]
  
  static const struct phy_lookup my_phy_lookup[] = {
  
PHY_LOOKUP(s3c-hsotg.0, otg, samsung-usbphy.1, phy.2),
  
  The only problem here is that if *PLATFORM_DEVID_AUTO* is used
  while
  creating the device, the ids in the device name would change and
  PHY_LOOKUP wont be useful.
  
  I don't think this is a problem. All the existing lookup methods
  already
  use ID to identify devices (see regulators, clkdev, PWMs, i2c,
  ...). You
  can simply add a requirement that the ID must be assigned manually,
  without using PLATFORM_DEVID_AUTO to use PHY lookup.
  
  And I'm saying that this idea, of using a specific name and id, is
  frought with fragility and will break in the future in various ways
  when
  devices get added to systems, making these strings constantly have
  to be
  kept up to date with different board configurations.
  
  People, NEVER, hardcode something like an id.  The fact that this
  happens today with the clock code, doesn't make it right, it makes
  the
  clock code wrong.  Others have already said that this is wrong there
  as
  well, as systems change and dynamic ids get used more and more.
  
  Let's not repeat the same mistakes of the past just because we
  refuse to
  learn from them...
  
  So again, the find a phy by a string functions should be removed,
  the
  device id should be automatically created by the phy core just to
  make
  things unique in sysfs, and no driver code should _ever_ be reliant
  on
  the number that is being created, and the pointer to the phy
  structure
  should be used everywhere instead.
  
  With those types of changes, I will consider merging this subsystem,
  but
  without them, sorry, I will not.
  
  I'll agree with Greg here, the very fact that we see people trying to
  add a requirement of *NOT* using PLATFORM_DEVID_AUTO already points
  to a
  big problem in the framework.
  
  The fact is that if we don't allow PLATFORM_DEVID_AUTO we will end up
  adding similar infrastructure to the driver themselves to make sure
  we
  don't end up with duplicate names in sysfs in case we have multiple
  instances of the same IP in the SoC (or several of the same PCIe
  card).
  I really don't want to go back to that.
  
  If we are using PLATFORM_DEVID_AUTO, then I dont see any way we can
  give the correct binding information to the PHY framework. I think we
  can drop having this non-dt support in PHY framework? I see only one
  platform (OMAP3) going to be needing this non-dt support and we can
  use the USB PHY library for it. 
  you shouldn't drop support for non-DT platform, in any case we lived
  without DT (and still do) for years. Gotta find a better way ;-)
 
 hmm..
 
 how about passing the device names of PHY in platform data of the
 controller? It should be deterministic as the PHY framework assigns its
 own id and we *don't* want to add any requirement that the ID must be
 assigned manually without using PLATFORM_DEVID_AUTO. We can get rid of
 *phy_init_data* in the v10 patch series.

What about slightly altering the concept of v9 to pass a pointer to struct 
device instead of device name inside phy_init_data?

Best regards,
Tomasz

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/15] drivers: phy: add generic PHY framework

[Tomasz Figa]

On Wednesday 14 of August 2013 00:19:28 Sylwester Nawrocki wrote:
 W dniu 2013-08-13 14:05, Kishon Vijay Abraham I pisze:
  On Tuesday 13 August 2013 05:07 PM, Tomasz Figa wrote:
  On Tuesday 13 of August 2013 16:14:44 Kishon Vijay Abraham I wrote:
  On Wednesday 31 July 2013 11:45 AM, Felipe Balbi wrote:
  On Wed, Jul 31, 2013 at 11:14:32AM +0530, Kishon Vijay Abraham I 
wrote:
  IMHO we need a lookup method for PHYs, just like for clocks,
  regulators, PWMs or even i2c busses because there are complex
  cases
  when passing just a name using platform data will not work. I
  would
  second what Stephen said [1] and define a structure doing
  things
  in a
  DT-like way.
  
  Example;
  
  [platform code]
  
  static const struct phy_lookup my_phy_lookup[] = {
  
 PHY_LOOKUP(s3c-hsotg.0, otg, samsung-usbphy.1,
 phy.2),
  
  The only problem here is that if *PLATFORM_DEVID_AUTO* is used
  while
  creating the device, the ids in the device name would change
  and
  PHY_LOOKUP wont be useful.
  
  I don't think this is a problem. All the existing lookup
  methods
  already
  use ID to identify devices (see regulators, clkdev, PWMs, i2c,
  ...). You
  can simply add a requirement that the ID must be assigned
  manually,
  without using PLATFORM_DEVID_AUTO to use PHY lookup.
  
  And I'm saying that this idea, of using a specific name and id,
  is
  frought with fragility and will break in the future in various
  ways
  when
  devices get added to systems, making these strings constantly
  have
  to be
  kept up to date with different board configurations.
  
  People, NEVER, hardcode something like an id.  The fact that
  this
  happens today with the clock code, doesn't make it right, it
  makes
  the
  clock code wrong.  Others have already said that this is wrong
  there
  as
  well, as systems change and dynamic ids get used more and more.
  
  Let's not repeat the same mistakes of the past just because we
  refuse to
  learn from them...
  
  So again, the find a phy by a string functions should be
  removed,
  the
  device id should be automatically created by the phy core just
  to
  make
  things unique in sysfs, and no driver code should _ever_ be
  reliant
  on
  the number that is being created, and the pointer to the phy
  structure
  should be used everywhere instead.
  
  With those types of changes, I will consider merging this
  subsystem,
  but
  without them, sorry, I will not.
  
  I'll agree with Greg here, the very fact that we see people
  trying to
  add a requirement of *NOT* using PLATFORM_DEVID_AUTO already
  points
  to a big problem in the framework.
  
  The fact is that if we don't allow PLATFORM_DEVID_AUTO we will
  end up
  adding similar infrastructure to the driver themselves to make
  sure
  we
  don't end up with duplicate names in sysfs in case we have
  multiple
  instances of the same IP in the SoC (or several of the same PCIe
  card).
  I really don't want to go back to that.
  
  If we are using PLATFORM_DEVID_AUTO, then I dont see any way we
  can
  give the correct binding information to the PHY framework. I think
  we
  can drop having this non-dt support in PHY framework? I see only
  one
  platform (OMAP3) going to be needing this non-dt support and we
  can
  use the USB PHY library for it.
  
  you shouldn't drop support for non-DT platform, in any case we
  lived
  without DT (and still do) for years. Gotta find a better way ;-)
  
  hmm..
  
  how about passing the device names of PHY in platform data of the
  controller? It should be deterministic as the PHY framework assigns
  its
  own id and we *don't* want to add any requirement that the ID must
  be
  assigned manually without using PLATFORM_DEVID_AUTO. We can get rid
  of
  *phy_init_data* in the v10 patch series.
 
 OK, so the PHY device name would have a fixed part, passed as
 platform data of the controller and a variable part appended
 by the PHY core, depending on the number of registered PHYs ?
 
 Then same PHY names would be passed as the PHY provider driver's
 platform data ?
 
 Then if there are 2 instances of the above (same names in platform
 data) how would be determined which PHY controller is linked to
 which PHY supplier ?
 
 I guess you want each device instance to have different PHY device
 names already in platform data ? That might work. We probably will
 be focused mostly on DT anyway. It seem without DT we are trying
 to find some layer that would allow us to couple relevant devices
 and overcome driver core inconvenience that it provides to means
 to identify specific devices in advance. :) Your proposal sounds
 reasonable, however I might be missing some details or corner cases.
 
  What about slightly altering the concept of v9 to pass a pointer to
  struct device instead of device name inside phy_init_data?
 
 As Felipe said, we don't want to pass pointers in platform_data
 to/from random subsystems. We pass data, passing pointers would
 be a total mess IMHO.

Well

Re: [PATCHv5 01/31] CLK: clkdev: add support for looking up clocks from DT

[Tomasz Figa]

Hi Tero,

On Friday 02 of August 2013 19:25:20 Tero Kristo wrote:
 clk_get_sys / clk_get can now find clocks from device-tree. If a DT
 clock is found, an entry is added to the clk_lookup list also for
 subsequent searches.
 
 Signed-off-by: Tero Kristo t-kri...@ti.com
 Cc: Russell King li...@arm.linux.org.uk
 ---
  drivers/clk/clkdev.c |   33 +
  1 file changed, 33 insertions(+)
 
 diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
 index 442a313..cbeb252 100644
 --- a/drivers/clk/clkdev.c
 +++ b/drivers/clk/clkdev.c
 @@ -93,6 +93,18 @@ struct clk *of_clk_get_by_name(struct device_node
 *np, const char *name) EXPORT_SYMBOL(of_clk_get_by_name);
  #endif
 
 +/**
 + * clkdev_add_nolock - add lookup entry for a clock
 + * @cl: pointer to new clock lookup entry
 + *
 + * Non-locking version, used internally by clk_find() to add DT based
 + * clock lookup entries.
 + */
 +static void clkdev_add_nolock(struct clk_lookup *cl)
 +{
 + list_add_tail(cl-node, clocks);
 +}
 +
  /*
   * Find the correct struct clk for the device and connection ID.
   * We do slightly fuzzy matching here:
 @@ -106,6 +118,9 @@ static struct clk_lookup *clk_find(const char
 *dev_id, const char *con_id) {
   struct clk_lookup *p, *cl = NULL;
   int match, best_found = 0, best_possible = 0;
 + struct device_node *node;
 + struct clk *clk;
 + struct of_phandle_args clkspec;
 
   if (dev_id)
   best_possible += 2;
 @@ -133,6 +148,24 @@ static struct clk_lookup *clk_find(const char
 *dev_id, const char *con_id) break;
   }
   }
 +
 + if (cl)
 + return cl;
 +
 + /* If clock was not found, attempt to look-up from DT */
 + node = of_find_node_by_name(NULL, con_id);

Why are we introducing the lookup by name brokenness to the yet (mostly) 
sane DT world?

We already have a good way of binding things together in DT, which is 
using phandles.

Not even saying that this (or something this patch relies on) breaks the 
ePAPR recommendation about node naming, which states that node names 
should not be used to convey platform-specific data, but instead should be 
as generic as possible to show what kind of hardware is represented by the 
node.

Best regards,
Tomasz

P.S. Added missing DT maintainers to CC.

 + clkspec.np = node;
 +
 + clk = of_clk_get_from_provider(clkspec);
 +
 + if (!IS_ERR(clk)) {
 + /* We found a clock, add node to clkdev */
 + cl = clkdev_alloc(clk, con_id, dev_id);
 + if (cl)
 + clkdev_add_nolock(cl);
 + }
 +
   return cl;
  }
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv5 06/31] ARM: dts: omap4 clock data

[Tomasz Figa]

On Friday 02 of August 2013 19:25:25 Tero Kristo wrote:
 This patch creates a unique node for each clock in the OMAP4 power,
 reset and clock manager (PRCM). OMAP443x and OMAP446x have slightly
 different clock tree which is taken into account in the data.
 
 Signed-off-by: Tero Kristo t-kri...@ti.com
 ---
  arch/arm/boot/dts/omap443x-clocks.dtsi |   17 +
  arch/arm/boot/dts/omap443x.dtsi|8 +
  arch/arm/boot/dts/omap4460.dtsi|8 +
  arch/arm/boot/dts/omap446x-clocks.dtsi |   27 +
  arch/arm/boot/dts/omap44xx-clocks.dtsi | 1648
  5 files changed, 1708 insertions(+)
  create mode 100644 arch/arm/boot/dts/omap443x-clocks.dtsi
  create mode 100644 arch/arm/boot/dts/omap446x-clocks.dtsi
  create mode 100644 arch/arm/boot/dts/omap44xx-clocks.dtsi
 
 diff --git a/arch/arm/boot/dts/omap443x-clocks.dtsi
 b/arch/arm/boot/dts/omap443x-clocks.dtsi new file mode 100644
 index 000..2bd82b2
 --- /dev/null
 +++ b/arch/arm/boot/dts/omap443x-clocks.dtsi
 @@ -0,0 +1,17 @@
 +/*
 + * Device Tree Source for OMAP443x clock data
 + *
 + * Copyright (C) 2013 Texas Instruments, Inc.
 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License version 2 as +
 * published by the Free Software Foundation.
 + */
 +
 +bandgap_fclk: bandgap_fclk@4a307888 {
 + #clock-cells = 0;
 + compatible = gate-clock;
 + clocks = sys_32k_ck;
 + bit-shift = 8;
 + reg = 0x4a307888 0x4;
 +};
 diff --git a/arch/arm/boot/dts/omap443x.dtsi
 b/arch/arm/boot/dts/omap443x.dtsi index bcf455e..dfd648c 100644
 --- a/arch/arm/boot/dts/omap443x.dtsi
 +++ b/arch/arm/boot/dts/omap443x.dtsi
 @@ -30,4 +30,12 @@
  0x4a00232C 0x4;
   compatible = ti,omap4430-bandgap;
   };
 +
 + clocks {
 + #address-cells = 1;
 + #size-cells = 1;
 + ranges;
 + /include/ omap44xx-clocks.dtsi
 + /include/ omap443x-clocks.dtsi
 + };
  };
 diff --git a/arch/arm/boot/dts/omap4460.dtsi
 b/arch/arm/boot/dts/omap4460.dtsi index c2f0f39..d9d00b2 100644
 --- a/arch/arm/boot/dts/omap4460.dtsi
 +++ b/arch/arm/boot/dts/omap4460.dtsi
 @@ -38,4 +38,12 @@
   interrupts = 0 126 IRQ_TYPE_LEVEL_HIGH; /* talert */
   gpios = gpio3 22 0; /* tshut */
   };
 +
 + clocks {
 + #address-cells = 1;
 + #size-cells = 1;
 + ranges;
 + /include/ omap44xx-clocks.dtsi
 + /include/ omap446x-clocks.dtsi
 + };
  };
 diff --git a/arch/arm/boot/dts/omap446x-clocks.dtsi
 b/arch/arm/boot/dts/omap446x-clocks.dtsi new file mode 100644
 index 000..86d0805
 --- /dev/null
 +++ b/arch/arm/boot/dts/omap446x-clocks.dtsi
 @@ -0,0 +1,27 @@
 +/*
 + * Device Tree Source for OMAP446x clock data
 + *
 + * Copyright (C) 2013 Texas Instruments, Inc.
 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License version 2 as +
 * published by the Free Software Foundation.
 + */
 +
 +div_ts_ck: div_ts_ck@4a307888 {
 + #clock-cells = 0;
 + compatible = divider-clock;
 + clocks = l4_wkup_clk_mux_ck;
 + bit-shift = 24;
 + reg = 0x4a307888 0x4;
 + table =  8 0 ,  16 1 ,  32 2 ;
 + bit-mask = 0x3;
 +};
 +
 +bandgap_ts_fclk: bandgap_ts_fclk@4a307888 {
 + #clock-cells = 0;
 + compatible = gate-clock;
 + clocks = div_ts_ck;
 + bit-shift = 8;
 + reg = 0x4a307888 0x4;
 +};
 diff --git a/arch/arm/boot/dts/omap44xx-clocks.dtsi
 b/arch/arm/boot/dts/omap44xx-clocks.dtsi new file mode 100644
 index 000..23f623c
 --- /dev/null
 +++ b/arch/arm/boot/dts/omap44xx-clocks.dtsi
 @@ -0,0 +1,1648 @@
 +/*
 + * Device Tree Source for OMAP4 clock data
 + *
 + * Copyright (C) 2013 Texas Instruments, Inc.
 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms of the GNU General Public License version 2 as +
 * published by the Free Software Foundation.
 + */
 +
 +extalt_clkin_ck: extalt_clkin_ck {
 + #clock-cells = 0;
 + compatible = fixed-clock;
 + clock-frequency = 5900;
 +};
 +
 +pad_clks_src_ck: pad_clks_src_ck {
 + #clock-cells = 0;
 + compatible = fixed-clock;
 + clock-frequency = 1200;
 +};
 +
 +pad_clks_ck: pad_clks_ck@4a004108 {
 + #clock-cells = 0;
 + compatible = gate-clock;
 + clocks = pad_clks_src_ck;
 + bit-shift = 8;
 + reg = 0x4a004108 0x4;
 +};
 +
 +pad_slimbus_core_clks_ck: pad_slimbus_core_clks_ck {
 + #clock-cells = 0;
 + compatible = fixed-clock;
 + clock-frequency = 1200;
 +};
 +
 +secure_32k_clk_src_ck: secure_32k_clk_src_ck {
 + #clock-cells = 0;
 + compatible = fixed-clock;
 + clock-frequency = 32768;
 +};
 +
 +slimbus_src_clk: slimbus_src_clk {
 + #clock-cells = 0;
 + compatible = fixed-clock;
 + clock-frequency = 1200;
 +};
 +
 

Re: [PATCHv5 01/31] CLK: clkdev: add support for looking up clocks from DT

[Tomasz Figa]

Hi Russell,

On Saturday 03 of August 2013 19:35:43 Russell King - ARM Linux wrote:
 On Sat, Aug 03, 2013 at 04:02:36PM +0200, Tomasz Figa wrote:
   + if (cl)
   + return cl;
   +
   + /* If clock was not found, attempt to look-up from DT */
   + node = of_find_node_by_name(NULL, con_id);
  
  Why are we introducing the lookup by name brokenness to the yet
  (mostly) sane DT world?
  
  We already have a good way of binding things together in DT, which is
  using phandles.
  
  Not even saying that this (or something this patch relies on) breaks
  the ePAPR recommendation about node naming, which states that node
  names should not be used to convey platform-specific data, but
  instead should be as generic as possible to show what kind of
  hardware is represented by the node.
 
 Tell me this: many devices name their clock inputs.  You can find these
 input names in data sheets and the like.  These are the names defined
 by the hardware.  What is wrong about using those names in DT?

Yes, clock inputs. But this is about lookup by clock name, not clock input 
name, as far as I can tell from the patch, based on looking for a node 
with given name over the whole DT.

 Remember that the second argument to clk_get() is the _clock_ _input_
 _name_ on the _device_ passed in the first argument.  It is not *ever*
 some random name of the clock producer unless someone's fscked up their
 use of this API (in which case they're the ones with the problem.)

This is perfectly fine and this is how the generic common clock framework 
DT bindings work - clock-names property is a list of clock input names and 
clocks property contain specifiers of respective clocks.

Best regards,
Tomasz

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv5 01/31] CLK: clkdev: add support for looking up clocks from DT

[Tomasz Figa]

On Saturday 03 of August 2013 19:48:05 Russell King - ARM Linux wrote:
 On Sat, Aug 03, 2013 at 08:39:34PM +0200, Tomasz Figa wrote:
  Hi Russell,
  
  On Saturday 03 of August 2013 19:35:43 Russell King - ARM Linux wrote:
   On Sat, Aug 03, 2013 at 04:02:36PM +0200, Tomasz Figa wrote:
 + if (cl)
 + return cl;
 +
 + /* If clock was not found, attempt to look-up from DT */
 + node = of_find_node_by_name(NULL, con_id);

Why are we introducing the lookup by name brokenness to the yet
(mostly) sane DT world?

We already have a good way of binding things together in DT, which
is
using phandles.

Not even saying that this (or something this patch relies on)
breaks
the ePAPR recommendation about node naming, which states that node
names should not be used to convey platform-specific data, but
instead should be as generic as possible to show what kind of
hardware is represented by the node.
   
   Tell me this: many devices name their clock inputs.  You can find
   these
   input names in data sheets and the like.  These are the names
   defined
   by the hardware.  What is wrong about using those names in DT?
  
  Yes, clock inputs. But this is about lookup by clock name, not clock
  input name, as far as I can tell from the patch, based on looking for
  a node with given name over the whole DT.
 
 con_id is supposed to be the clock input name when the clock API is
 used correctly.

Right. Still, the code is looking for a node with the same name as the 
supposed input name, which seems strange to me, because clock input names 
are supposed to be defined in consumer's node, inside clock-names 
property.

   Remember that the second argument to clk_get() is the _clock_
   _input_
   _name_ on the _device_ passed in the first argument.  It is not
   *ever*
   some random name of the clock producer unless someone's fscked up
   their
   use of this API (in which case they're the ones with the problem.)
  
  This is perfectly fine and this is how the generic common clock
  framework DT bindings work - clock-names property is a list of clock
  input names and clocks property contain specifiers of respective
  clocks.
 
 There seems to be a some pressure to do clk_get()-of_clk_get()
 conversions, and also to find ways to lookup clocks.

I don't really see any need for this kind of conversion. clk_get() already 
handles lookup using DT correctly and from drivers perspective nothing 
changes.

 It seems to me
 that no one understands how DT handles clocks.  Maybe that's where the
 problem is - lack of documentation about how DT clocks are handled.

Hmm, there is pretty much of description and examples in

Documentation/devicetree/bindings/clock/clock-bindings.txt

For me it was enough to learn how DT based clock lookup works, but I'm 
quite used to DT, so I'm not a good test sample.

Best regards,
Tomasz

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/15] drivers: phy: add generic PHY framework

[Tomasz Figa]

Hi Alan,

On Monday 22 of July 2013 10:44:39 Alan Stern wrote:
 On Mon, 22 Jul 2013, Kishon Vijay Abraham I wrote:
 The PHY and the controller it is attached to are both physical
 devices.
 
 The connection between them is hardwired by the system
 manufacturer and cannot be changed by software.
 
 PHYs are generally described by fixed system-specific board
 files or by Device Tree information.  Are they ever discovered
 dynamically?
  
  No. They are created just like any other platform devices are created.
 
 Okay.  Are PHYs _always_ platform devices?

They can be i2c, spi or any other device types as well.

 Is the same true for the controllers attached to the PHYs?
 If not -- if both a PHY and a controller are discovered
 dynamically -- how does the kernel know whether they are
 connected to each other?
  
  No differences here. Both PHY and controller will have dt information
  or hwmod data using which platform devices will be created.
  
 The kernel needs to know which controller is attached to which
 PHY.  Currently this information is represented by name or ID
 strings embedded in platform data.
  
  right. It's embedded in the platform data of the controller.
 
 It must also be embedded in the PHY's platform data somehow.
 Otherwise, how would the kernel know which PHY to use?

By using a PHY lookup as Stephen and I suggested in our previous replies. 
Without any extra data in platform data. (I have even posted a code 
example.)

 The PHY's driver (the supplier) uses the platform data to
 construct a platform_device structure that represents the PHY.
  
  Currently the driver assigns static labels (corresponding to the label
  used in the platform data of the controller).
  
 Until this is done, the controller's driver (the client) cannot
 use the PHY.
  
  right.
  
 Since there is no parent-child relation between the PHY and the
 controller, there is no guarantee that the PHY's driver will be
 ready when the controller's driver wants to use it.  A deferred
 probe may be needed.
  
  right.
  
 The issue (or one of the issues) in this discussion is that
 Greg does not like the idea of using names or IDs to associate
 PHYs with controllers, because they are too prone to
 duplications or other errors.  Pointers are more reliable.
 
 But pointers to what?  Since the only data known to be
 available to both the PHY driver and controller driver is the
 platform data, the obvious answer is a pointer to platform data
 (either for the PHY or for the controller, or maybe both).
  
  hmm.. it's not going to be simple though as the platform device for
  the PHY and controller can be created in entirely different places.
  e.g., in some cases the PHY device is a child of some mfd core device
  (the device will be created in drivers/mfd) and the controller driver
  (usually) is created in board file. I guess then we have to come up
  with something to share a pointer in two different files.
 
 The ability for two different source files to share a pointer to a data
 item defined in a third source file has been around since long before
 the C language was invented.  :-)
 
 In this case, it doesn't matter where the platform_device structures
 are created or where the driver source code is.  Let's take a simple
 example.  Suppose the system design includes a PHY named foo.  Then
 the board file could contain:
 
 struct phy_info { ... } phy_foo;
 EXPORT_SYMBOL_GPL(phy_foo);
 
 and a header file would contain:
 
 extern struct phy_info phy_foo;
 
 The PHY supplier could then call phy_create(phy_foo), and the PHY
 client could call phy_find(phy_foo).  Or something like that; make up
 your own structure tags and function names.
 
 It's still possible to have conflicts, but now two PHYs with the same
 name (or a misspelled name somewhere) will cause an error at link time.

This is incorrect, sorry. First of all it's a layering violation - you 
export random driver-specific symbols from one driver to another. Then 
imagine 4 SoCs - A, B, C, D. There are two PHY types PHY1 and PHY2 and 
there are two types of consumer drivers (e.g. USB host controllers). Now 
consider following mapping:

SoC PHY consumer
A   PHY1HOST1
B   PHY1HOST2
C   PHY2HOST1
D   PHY2HOST2

So we have to be able to use any of the PHYs with any of the host drivers. 
This means you would have to export symbol with the same name from both 
PHY drivers, which obviously would not work in this case, because having 
both drivers enabled (in a multiplatform aware configuration) would lead 
to linking conflict.

Best regards,
Tomasz

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/15] drivers: phy: add generic PHY framework

[Tomasz Figa]

[Fixed address of devicetree mailing list and added more people on CC.]

For reference, full thread can be found under following link:
http://thread.gmane.org/gmane.linux.ports.arm.kernel/252813

Best regards,
Tomasz

On Tuesday 23 of July 2013 09:29:32 Tomasz Figa wrote:
 Hi Alan,
 
 On Monday 22 of July 2013 10:44:39 Alan Stern wrote:
  On Mon, 22 Jul 2013, Kishon Vijay Abraham I wrote:
The PHY and the controller it is attached to are both 
physical
devices.

The connection between them is hardwired by the system
manufacturer and cannot be changed by software.

PHYs are generally described by fixed system-specific 
board
files or by Device Tree information.  Are they ever 
discovered
dynamically?
   
   No. They are created just like any other platform devices are
   created.
  
  Okay.  Are PHYs _always_ platform devices?
 
 They can be i2c, spi or any other device types as well.
 
Is the same true for the controllers attached to the PHYs?
If not -- if both a PHY and a controller are discovered
dynamically -- how does the kernel know whether they are
connected to each other?
   
   No differences here. Both PHY and controller will have dt
   information
   or hwmod data using which platform devices will be created.
   
The kernel needs to know which controller is attached to 
which
PHY.  Currently this information is represented by name or 
ID
strings embedded in platform data.
   
   right. It's embedded in the platform data of the controller.
  
  It must also be embedded in the PHY's platform data somehow.
  Otherwise, how would the kernel know which PHY to use?
 
 By using a PHY lookup as Stephen and I suggested in our previous
 replies. Without any extra data in platform data. (I have even posted a
 code example.)
 
The PHY's driver (the supplier) uses the platform data to
construct a platform_device structure that represents the 
PHY.
   
   Currently the driver assigns static labels (corresponding to the
   label
   used in the platform data of the controller).
   
Until this is done, the controller's driver (the client) 
cannot
use the PHY.
   
   right.
   
Since there is no parent-child relation between the PHY 
and the
controller, there is no guarantee that the PHY's driver 
will be
ready when the controller's driver wants to use it.  A 
deferred
probe may be needed.
   
   right.
   
The issue (or one of the issues) in this discussion is 
that
Greg does not like the idea of using names or IDs to 
associate
PHYs with controllers, because they are too prone to
duplications or other errors.  Pointers are more reliable.

But pointers to what?  Since the only data known to be
available to both the PHY driver and controller driver is 
the
platform data, the obvious answer is a pointer to platform 
data
(either for the PHY or for the controller, or maybe both).
   
   hmm.. it's not going to be simple though as the platform device for
   the PHY and controller can be created in entirely different places.
   e.g., in some cases the PHY device is a child of some mfd core
   device
   (the device will be created in drivers/mfd) and the controller
   driver
   (usually) is created in board file. I guess then we have to come up
   with something to share a pointer in two different files.
  
  The ability for two different source files to share a pointer to a
  data
  item defined in a third source file has been around since long before
  the C language was invented.  :-)
  
  In this case, it doesn't matter where the platform_device structures
  are created or where the driver source code is.  Let's take a simple
  example.  Suppose the system design includes a PHY named foo.  Then
  the board file could contain:
  
  struct phy_info { ... } phy_foo;
  EXPORT_SYMBOL_GPL(phy_foo);
  
  and a header file would contain:
  
  extern struct phy_info phy_foo;
  
  The PHY supplier could then call phy_create(phy_foo), and the PHY
  client could call phy_find(phy_foo).  Or something like that; make up
  your own structure tags and function names.
  
  It's still possible to have conflicts, but now two PHYs with the same
  name (or a misspelled name somewhere) will cause an error at link
  time.
 
 This is incorrect, sorry. First of all it's a layering violation - you
 export random driver-specific symbols from one driver to another. Then
 imagine 4 SoCs - A, B, C, D. There are two PHY types PHY1 and PHY2 and
 there are two types of consumer drivers (e.g. USB host controllers). Now
 consider following mapping:
 
 SoC   PHY consumer
 A PHY1HOST1
 B PHY1HOST2
 C PHY2HOST1
 D PHY2HOST2

Re: [PATCH 01/15] drivers: phy: add generic PHY framework

[Tomasz Figa]

On Tuesday 23 of July 2013 10:37:05 Alan Stern wrote:
 On Tue, 23 Jul 2013, Tomasz Figa wrote:
  On Tuesday 23 of July 2013 09:29:32 Tomasz Figa wrote:
   Hi Alan,
 
 Thanks for helping to clarify the issues here.
 
Okay.  Are PHYs _always_ platform devices?
   
   They can be i2c, spi or any other device types as well.
 
 In those other cases, presumably there is no platform data associated
 with the PHY since it isn't a platform device.  Then how does the
 kernel know which controller is attached to the PHY?  Is this spelled
 out in platform data associated with the PHY's i2c/spi/whatever parent?
 
  PHY.  Currently this information is represented by name or
  
  ID
  
  strings embedded in platform data.
 
 right. It's embedded in the platform data of the controller.

It must also be embedded in the PHY's platform data somehow.
Otherwise, how would the kernel know which PHY to use?
   
   By using a PHY lookup as Stephen and I suggested in our previous
   replies. Without any extra data in platform data. (I have even posted
   a
   code example.)
 
 I don't understand, because I don't know what a PHY lookup does.

I have provided a code example in [1]. Feel free to ask questions about 
those code snippets.

[1] http://thread.gmane.org/gmane.linux.ports.arm.kernel/252813/focus=20889

In this case, it doesn't matter where the platform_device
structures
are created or where the driver source code is.  Let's take a
simple
example.  Suppose the system design includes a PHY named foo. 
Then
the board file could contain:

struct phy_info { ... } phy_foo;
EXPORT_SYMBOL_GPL(phy_foo);

and a header file would contain:

extern struct phy_info phy_foo;

The PHY supplier could then call phy_create(phy_foo), and the PHY
client could call phy_find(phy_foo).  Or something like that; make
up
your own structure tags and function names.

It's still possible to have conflicts, but now two PHYs with the
same
name (or a misspelled name somewhere) will cause an error at link
time.
   
   This is incorrect, sorry. First of all it's a layering violation -
   you
   export random driver-specific symbols from one driver to another.
   Then
 
 No, that's not what I said.  Neither the PHY driver nor the controller
 driver exports anything to the other.  Instead, both drivers use data
 exported by the board file.

It's still a random, driver-specific global symbol exported from board file 
to drivers.

   imagine 4 SoCs - A, B, C, D. There are two PHY types PHY1 and PHY2
   and
   there are two types of consumer drivers (e.g. USB host controllers).
   Now
   consider following mapping:
   
   SoC   PHY consumer
   A PHY1HOST1
   B PHY1HOST2
   C PHY2HOST1
   D PHY2HOST2
   
   So we have to be able to use any of the PHYs with any of the host
   drivers. This means you would have to export symbol with the same
   name
   from both PHY drivers, which obviously would not work in this case,
   because having both drivers enabled (in a multiplatform aware
   configuration) would lead to linking conflict.
 
 You're right; the scheme was too simple.  Instead, the board file must
 export two types of data structures, one for PHYs and one for
 controllers.  Like this:
 
 struct phy_info {
   /* Info for the controller attached to this PHY */
   struct controller_info  *hinfo;
 };
 
 struct controller_info {
   /* Info for the PHY which this controller is attached to */
   struct phy_info *pinfo;
 };
 
 The board file for SoC A would contain:
 
 struct phy_info phy1 = {host1);
 EXPORT_SYMBOL(phy1);
 struct controller_info host1 = {phy1};
 EXPORT_SYMBOL(host1);
 
 The board file for SoC B would contain:
 
 struct phy_info phy1 = {host2);
 EXPORT_SYMBOL(phy1);
 struct controller_info host2 = {phy1};
 EXPORT_SYMBOL(host2);
 
 And so on.  This explicitly gives the connection between PHYs and
 controllers.  The PHY providers would use phy1 or phy2, and the PHY
 consumers would use host1 or host2.

This could work assuming that only one SoC and one board is supported in 
single kernel image. However it's not the case.

We've used to support multiple boards since a long time already and now for 
selected platforms we even support multiplatform, i.e. multiple SoCs in 
single zImage. Such solution will not work.

Best regards,
Tomasz

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/15] drivers: phy: add generic PHY framework

[Tomasz Figa]

On Tuesday 23 of July 2013 09:18:46 Greg KH wrote:
 On Tue, Jul 23, 2013 at 08:48:24PM +0530, Kishon Vijay Abraham I wrote:
  Hi,
  
  On Tuesday 23 July 2013 08:07 PM, Alan Stern wrote:
   On Tue, 23 Jul 2013, Tomasz Figa wrote:
   On Tuesday 23 of July 2013 09:29:32 Tomasz Figa wrote:
   Hi Alan,
   
   Thanks for helping to clarify the issues here.
   
   Okay.  Are PHYs _always_ platform devices?
   
   They can be i2c, spi or any other device types as well.
   
   In those other cases, presumably there is no platform data associated
   with the PHY since it isn't a platform device.  Then how does the
   kernel know which controller is attached to the PHY?  Is this spelled
   out in platform data associated with the PHY's i2c/spi/whatever
   parent?
  
  Yes. I think we could use i2c_board_info for passing platform data.
  
PHY.  Currently this information is represented by name or
   
   ID
   
strings embedded in platform data.
   
   right. It's embedded in the platform data of the controller.
   
   It must also be embedded in the PHY's platform data somehow.
   Otherwise, how would the kernel know which PHY to use?
   
   By using a PHY lookup as Stephen and I suggested in our previous
   replies. Without any extra data in platform data. (I have even
   posted a
   code example.)
   
   I don't understand, because I don't know what a PHY lookup does.
  
  It is how the PHY framework finds a PHY, when the controller (say
  USB)requests a PHY from the PHY framework.
  
   In this case, it doesn't matter where the platform_device
   structures
   are created or where the driver source code is.  Let's take a
   simple
   example.  Suppose the system design includes a PHY named foo. 
   Then
   the board file could contain:
   
   struct phy_info { ... } phy_foo;
   EXPORT_SYMBOL_GPL(phy_foo);
   
   and a header file would contain:
   
   extern struct phy_info phy_foo;
   
   The PHY supplier could then call phy_create(phy_foo), and the PHY
   client could call phy_find(phy_foo).  Or something like that;
   make up
   your own structure tags and function names.
   
   It's still possible to have conflicts, but now two PHYs with the
   same
   name (or a misspelled name somewhere) will cause an error at link
   time.
   
   This is incorrect, sorry. First of all it's a layering violation -
   you
   export random driver-specific symbols from one driver to another.
   Then
   
   No, that's not what I said.  Neither the PHY driver nor the
   controller
   driver exports anything to the other.  Instead, both drivers use data
   exported by the board file.
  
  I think instead we can use the same data while creating the platform
  data of the controller and the PHY.
  The PHY driver while creating the PHY (using PHY framework) will also
  pass the *data* it actually got from the platform data to the
  framework. The PHY user driver (USB), while requesting for the PHY
  (from the PHY framework) will pass the *data* it got from its platform
  data.
  The PHY framework can do a comparison of the *data* pointers it has and
  return the appropriate PHY to the controller.
  
   imagine 4 SoCs - A, B, C, D. There are two PHY types PHY1 and PHY2
   and
   there are two types of consumer drivers (e.g. USB host
   controllers). Now
   consider following mapping:
   
   SoC PHY consumer
   A   PHY1HOST1
   B   PHY1HOST2
   C   PHY2HOST1
   D   PHY2HOST2
   
   So we have to be able to use any of the PHYs with any of the host
   drivers. This means you would have to export symbol with the same
   name
   from both PHY drivers, which obviously would not work in this case,
   because having both drivers enabled (in a multiplatform aware
   configuration) would lead to linking conflict.
   
   You're right; the scheme was too simple.  Instead, the board file
   must
   export two types of data structures, one for PHYs and one for
   controllers.  Like this:
   
   struct phy_info {
   
 /* Info for the controller attached to this PHY */
 struct controller_info  *hinfo;
   
   };
   
   struct controller_info {
   
 /* Info for the PHY which this controller is attached to */
 struct phy_info *pinfo;
   
   };
   
   The board file for SoC A would contain:
   
   struct phy_info phy1 = {host1);
   EXPORT_SYMBOL(phy1);
   struct controller_info host1 = {phy1};
   EXPORT_SYMBOL(host1);
   
   The board file for SoC B would contain:
   
   struct phy_info phy1 = {host2);
   EXPORT_SYMBOL(phy1);
   struct controller_info host2 = {phy1};
   EXPORT_SYMBOL(host2);
  
  I meant something like this
  struct phy_info {
  
  const char *name;
  
  };
  
  struct phy_platform_data {
  
  .
  .
  struct phy_info *info;
  
  };
  
  struct usb_controller_platform_data {
  
  .
  .
  struct phy_info *info;
  
  };
  
  struct phy_info phy_info;
  
  While creating the phy device
  
  struct phy_platform_data phy_data

Re: [PATCH 01/15] drivers: phy: add generic PHY framework

[Tomasz Figa]

On Tuesday 23 of July 2013 10:37:11 Greg KH wrote:
 On Tue, Jul 23, 2013 at 06:50:29PM +0200, Tomasz Figa wrote:
   Ick, no.  Why can't you just pass the pointer to the phy itself?  If
   you
   had a priv pointer to search from, then you could have just passed
   the
   original phy pointer in the first place, right?
  
  IMHO it would be better if you provided some code example, but let's
  try to check if I understood you correctly.
 
 It's not my code that I want to have added, so I don't have to write
 examples, I just get to complain about the existing stuff :)

Still, I think that some small code snippets illustrating the idea are 
really helpful.

  8
  
  
  [Board file]
  
  static struct phy my_phy;
  
  static struct platform_device phy_pdev = {
  
  /* ... */
  .platform_data = my_phy;
  /* ... */
  
  };
  
  static struct platform_device phy_pdev = {
  
  /* ... */
  .platform_data = my_phy;
  /* ... */
  
  };
  
  [Provider driver]
  
  struct phy *phy = pdev-dev.platform_data;
  
  ret = phy_create(phy);
  
  [Consumer driver]
  
  struct phy *phy = pdev-dev.platform_data;
  
  ret = phy_get(pdev-dev, phy);
  
  ---
  -8
  
  Is this what you mean?
 
 No.  Well, kind of.  What's wrong with using the platform data structure
 unique to the board to have the pointer?
 
 For example (just randomly picking one), the ata-pxa driver would change
 include/linux/platform_data/ata-pxa.h to have a phy pointer in it:
 
 struct phy;
 
 struct  pata_pxa_pdata {
   /* PXA DMA DREQ0:2 pin */
   uint32_tdma_dreq;
   /* Register shift */
   uint32_treg_shift;
   /* IRQ flags */
   uint32_tirq_flags;
   /* PHY */
   struct phy  *phy;
 };
 
 Then, when you create the platform, set the phy* pointer with a call to
 phy_create().  Then you can use that pointer wherever that plaform data
 is available (i.e. whereever platform_data is at).

Hmm? So, do you suggest to call phy_create() from board file? What phy_ops 
struct and other hardware parameters would it take?

   The issue is that a string name is not going to scale at all, as it
   requires hard-coded information that will change over time (as the
   existing clock interface is already showing.)
  
  I fully agree that a simple, single string will not scale even in some,
  not so uncommon cases, but there is already a lot of existing lookup
  solutions over the kernel and so there is no point in introducing
  another one.
 I'm trying to get _rid_ of lookup solutions and just use a real
 pointer, as you should.  I'll go tackle those other ones after this one
 is taken care of, to show how the others should be handled as well.

There was a reason for introducing lookup solutions. The reason was that in 
board file there is no way to get a pointer to something that is going to be 
created much later in time. We don't do time travel ;-).

   Please just pass the real phy pointer around, that's what it is
   there
   for.  Your board binding logic/code should be able to handle this,
   as
   it somehow was going to do the same thing with a name.
  
  It's technically correct, but quality of this solution isn't really
  nice, because it's a layering violation (at least if I understood what
  you mean). This is because you need to have full definition of struct
  phy in board file and a structure that is used as private data in PHY
  core comes from platform code.
 
 No, just a pointer, you don't need the full structure until you get to
 some .c code that actually manipulates the phy itself, for all other
 places, you are just dealing with a pointer and a structure you never
 reference.
 
 Does that make more sense?

Well, to the point that I think I now understood your suggestion. 
Unfortunately the suggestion alone isn't really something that can be done, 
considering how driver core and generic frameworks work.

Best regards,
Tomasz

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/15] drivers: phy: add generic PHY framework

[Tomasz Figa]

On Tuesday 23 of July 2013 12:44:23 Greg KH wrote:
 On Tue, Jul 23, 2013 at 08:31:05PM +0100, Mark Brown wrote:
   You don't know the id of the device you are looking up, due to
   multiple devices being in the system (dynamic ids, look back earlier
   in
   this thread for details about that.)
  
  I got copied in very late so don't have most of the thread I'm afraid,
  I did try looking at web archives but didn't see a clear problem
  statement.  In any case this is why the APIs doing lookups do the
  lookups in the context of the requesting device - devices ask for
  whatever name they use locally.
 
 What do you mean by locally?
 
 The problem with the api was that the phy core wanted a id and a name to
 create a phy, and then later other code was doing a lookup based on
 the name and id (mushed together), because it knew that this device
 was the one it wanted.
 
 Just like the clock api, which, for multiple devices, has proven to
 cause problems.  I don't want to see us accept an api that we know has
 issues in it now, I'd rather us fix it up properly.
 
 Subsystems should be able to create ids how ever they want to, and not
 rely on the code calling them to specify the names of the devices that
 way, otherwise the api is just too fragile.
 
 I think, that if you create a device, then just carry around the pointer
 to that device (in this case a phy) and pass it to whatever other code
 needs it.  No need to do lookups on known names or anything else,
 just normal pointers, with no problems for multiple devices, busses, or
 naming issues.

PHY object is not a device, it is something that a device driver creates 
(one or more instances of) when it is being probed. You don't have a clean 
way to export this PHY object to other driver, other than keeping this PHY 
on a list inside PHY core with some well-known ID (e.g. device name + 
consumer port name/index, like in regulator core) and then to use this 
well-known ID inside consumer driver as a lookup key passed to phy_get();

Actually I think for PHY case, exactly the same way as used for regulators 
might be completely fine:

1. Each PHY would have some kind of platform, non-unique name, that is 
just used to print some messages (like the platform/board name of a 
regulator).
2. Each PHY would have an array of consumers. Consumer specifier would 
consist of consumer device name and consumer port name - just like in 
regulator subsystem.
3. PHY driver receives an array of, let's say, phy_init_data inside its 
platform data that it would use to register its PHYs.
4. Consumer drivers would have constant consumer port names and wouldn't 
receive any information about PHYs from platform code.

Code example:

[Board file]

static const struct phy_consumer_data usb_20_phy0_consumers[] = {
{
.devname = foo-ehci,
.port = usbphy,
},
};

static const struct phy_consumer_data usb_20_phy1_consumers[] = {
{
.devname = foo-otg,
.port = otgphy,
},
};

static const struct phy_init_data my_phys[] = {
{
.name = USB 2.0 PHY 0,
.consumers = usb_20_phy0_consumers,
.num_consumers = ARRAY_SIZE(usb_20_phy0_consumers),
},
{
.name = USB 2.0 PHY 1,
.consumers = usb_20_phy1_consumers,
.num_consumers = ARRAY_SIZE(usb_20_phy1_consumers),
},
{ }
};

static const struct platform_device usb_phy_pdev = {
.name = foo-usbphy,
.id = -1,
.dev = {
.platform_data = my_phys,
},
};

[PHY driver]

static int foo_usbphy_probe(pdev)
{
struct foo_usbphy *foo;
struct phy_init_data *init_data = pdev-dev.platform_data;
/* ... */
// for each PHY in init_data {
phy_register(foo-phy[i], init_data[i]);
// }
/* ... */
}

[EHCI driver]

static int foo_ehci_probe(pdev)
{
struct phy *phy;
/* ... */
phy = phy_get(pdev-dev, usbphy);
/* ... */
}

[OTG driver]

static int foo_otg_probe(pdev)
{
struct phy *phy;
/* ... */
phy = phy_get(pdev-dev, otgphy);
/* ... */
}

Having to write platform data for everything gets old fast and the
code
duplication is pretty tedious...
   
   Adding a single pointer is tedious?  Where is the name that you
   are
   going to lookup going to come from?  That code doesn't write
   itself...
  
  It's adding platform data in the first place that gets tedious - and
  of
  course there's also DT and ACPI to worry about, it's not just a case
  of
  platform data and then you're done.  Pushing the lookup into library
  code means that drivers don't have to worry about any of this stuff.
 
 I agree, so just pass around the pointer to the phy and all is good.  No
 need to worry about DT or ACPI or anything else.

With Device Tree we don't have board files anymore. How would 

Re: [PATCH 01/15] drivers: phy: add generic PHY framework

[Tomasz Figa]

On Tuesday 23 of July 2013 15:36:00 Alan Stern wrote:
 On Tue, 23 Jul 2013, Tomasz Figa wrote:
  IMHO it would be better if you provided some code example, but let's
  try to check if I understood you correctly.
  
  8---
  -
  
  [Board file]
  
  static struct phy my_phy;
  
  static struct platform_device phy_pdev = {
  
  /* ... */
  .platform_data = my_phy;
  /* ... */
  
  };
  
  static struct platform_device phy_pdev = {
 
 This should be controller_pdev, not phy_pdev, yes?

Right. A copy-pasto.

 
  /* ... */
  .platform_data = my_phy;
  /* ... */
  
  };
  
  [Provider driver]
  
  struct phy *phy = pdev-dev.platform_data;
  
  ret = phy_create(phy);
  
  [Consumer driver]
  
  struct phy *phy = pdev-dev.platform_data;
  
  ret = phy_get(pdev-dev, phy);
 
 Or even just phy_get(pdev-dev), because phy_get() could be smart
 enough to to set phy = dev-platform_data.

Unless you need more than one PHY in this driver...

 
  --
  --8
  
  Is this what you mean?
 
 That's what I was going to suggest too.  The struct phy is defined in
 the board file, which already knows about all the PHYs that exist in
 the system.  (Or perhaps it is allocated dynamically, so that when many
 board files are present in the same kernel, only the entries listed in
 the board file for the current system get created.) 

Well, such dynamic allocation is a must. We don't accept non-multiplatform 
aware code anymore, not even saying about multiboard.

 Then the
 structure's address is stored in the platform data and made available
 to both the provider and the consumer.

Yes, technically this can work. You would still have to perform some kind 
of synchronization to make sure that the PHY bound to this structure is 
actually present. This is again technically doable (e.g. a list of 
registered struct phys inside PHY core).

 Even though the struct phy is defined (or allocated) in the board file,
 its contents don't get filled in until the PHY driver provides the
 details.

You can't assure this. Board file is free to do whatever it wants with 
this struct. A clean solution would prevent this.

  It's technically correct, but quality of this solution isn't really
  nice, because it's a layering violation (at least if I understood
  what you mean). This is because you need to have full definition of
  struct phy in board file and a structure that is used as private data
  in PHY core comes from platform code.
 
 You don't have to have a full definition in the board file.  Just a
 partial definition -- most of the contents can be filled in later, when
 the PHY driver is ready to store the private data.
 
 It's not a layering violation for one region of the kernel to store
 private data in a structure defined by another part of the kernel.
 This happens all the time (e.g., dev_set_drvdata).

Not really. The phy struct is something that _is_ private data of PHY 
subsystem, not something that can store private data of PHY subsystem 
(sure it can store private data of particular PHY driver, but that's 
another story) and only PHY subsystem should have access to its contents.

By the way, we need to consider other cases here as well, for example it 
would be nice to have a single phy_get() function that works for both non-
DT and DT cases to make the consumer driver not have to worry whether it's 
being probed from DT or not.

I'd suggest simply reusing the lookup method of regulator framework, just 
as I suggested here:

http://thread.gmane.org/gmane.linux.ports.arm.kernel/252813/focus=101661

Best regards,
Tomasz

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/15] drivers: phy: add generic PHY framework

[Tomasz Figa]

On Tuesday 23 of July 2013 11:04:14 Greg KH wrote:
 On Tue, Jul 23, 2013 at 07:48:11PM +0200, Tomasz Figa wrote:
  On Tuesday 23 of July 2013 10:37:11 Greg KH wrote:
   On Tue, Jul 23, 2013 at 06:50:29PM +0200, Tomasz Figa wrote:
 Ick, no.  Why can't you just pass the pointer to the phy itself?
  If
 you
 had a priv pointer to search from, then you could have just
 passed
 the
 original phy pointer in the first place, right?

IMHO it would be better if you provided some code example, but
let's
try to check if I understood you correctly.
   
   It's not my code that I want to have added, so I don't have to write
   examples, I just get to complain about the existing stuff :)
  
  Still, I think that some small code snippets illustrating the idea are
  really helpful.
  
8---
-


[Board file]

static struct phy my_phy;

static struct platform_device phy_pdev = {

/* ... */
.platform_data = my_phy;
/* ... */

};

static struct platform_device phy_pdev = {

/* ... */
.platform_data = my_phy;
/* ... */

};

[Provider driver]

struct phy *phy = pdev-dev.platform_data;

ret = phy_create(phy);

[Consumer driver]

struct phy *phy = pdev-dev.platform_data;

ret = phy_get(pdev-dev, phy);

--
-
-8

Is this what you mean?
   
   No.  Well, kind of.  What's wrong with using the platform data
   structure unique to the board to have the pointer?
   
   For example (just randomly picking one), the ata-pxa driver would
   change include/linux/platform_data/ata-pxa.h to have a phy pointer
   in it:
   
   struct phy;
   
   struct  pata_pxa_pdata {
   
 /* PXA DMA DREQ0:2 pin */
 uint32_tdma_dreq;
 /* Register shift */
 uint32_treg_shift;
 /* IRQ flags */
 uint32_tirq_flags;
 /* PHY */
 struct phy  *phy;
   
   };
   
   Then, when you create the platform, set the phy* pointer with a call
   to
   phy_create().  Then you can use that pointer wherever that plaform
   data
   is available (i.e. whereever platform_data is at).
  
  Hmm? So, do you suggest to call phy_create() from board file? What
  phy_ops struct and other hardware parameters would it take?
  
 The issue is that a string name is not going to scale at all,
 as it
 requires hard-coded information that will change over time (as
 the
 existing clock interface is already showing.)

I fully agree that a simple, single string will not scale even in
some,
not so uncommon cases, but there is already a lot of existing
lookup
solutions over the kernel and so there is no point in introducing
another one.
   
   I'm trying to get _rid_ of lookup solutions and just use a real
   pointer, as you should.  I'll go tackle those other ones after this
   one
   is taken care of, to show how the others should be handled as well.
  
  There was a reason for introducing lookup solutions. The reason was
  that in board file there is no way to get a pointer to something that
  is going to be created much later in time. We don't do time travel
  ;-).
  
 Please just pass the real phy pointer around, that's what it
 is
 there
 for.  Your board binding logic/code should be able to handle
 this,
 as
 it somehow was going to do the same thing with a name.

It's technically correct, but quality of this solution isn't
really
nice, because it's a layering violation (at least if I understood
what
you mean). This is because you need to have full definition of
struct
phy in board file and a structure that is used as private data in
PHY
core comes from platform code.
   
   No, just a pointer, you don't need the full structure until you
   get to some .c code that actually manipulates the phy itself, for
   all other places, you are just dealing with a pointer and a
   structure you never reference.
   
   Does that make more sense?
  
  Well, to the point that I think I now understood your suggestion.
  Unfortunately the suggestion alone isn't really something that can be
  done, considering how driver core and generic frameworks work.
 
 Ok, given that I seem to be totally confused as to exactly how the
 board-specific frameworks work, I'll take your word for it.

Well, they are working in a way that keeps separation of layers, making 
things clean. Platform code should not (well, there might exist some in 
tree hacks, but this should not be propagated) used to exchange data 
between drivers, but rather to specify board specific parameters for 
generic drivers. If drivers need to cooperate, there must be a dedicated 
interface

Re: [PATCH 01/15] drivers: phy: add generic PHY framework

[Tomasz Figa]

On Tuesday 23 of July 2013 16:53:55 Alan Stern wrote:
 On Tue, 23 Jul 2013, Tomasz Figa wrote:
   That's what I was going to suggest too.  The struct phy is defined
   in
   the board file, which already knows about all the PHYs that exist in
   the system.  (Or perhaps it is allocated dynamically, so that when
   many
   board files are present in the same kernel, only the entries listed
   in
   the board file for the current system get created.)
  
  Well, such dynamic allocation is a must. We don't accept
  non-multiplatform aware code anymore, not even saying about
  multiboard.
  
   Then the
   structure's address is stored in the platform data and made
   available
   to both the provider and the consumer.
  
  Yes, technically this can work. You would still have to perform some
  kind of synchronization to make sure that the PHY bound to this
  structure is actually present. This is again technically doable (e.g.
  a list of registered struct phys inside PHY core).
 
 The synchronization takes place inside phy_get.  If phy_create hasn't
 been called for this structure by the time phy_get runs, phy_get will
 return an error.

Yes, this is the solution that I had in mind when saying that this is 
doable.

   Even though the struct phy is defined (or allocated) in the board
   file,
   its contents don't get filled in until the PHY driver provides the
   details.
  
  You can't assure this. Board file is free to do whatever it wants with
  this struct. A clean solution would prevent this.
 
 I'm not sure what you mean here.  Of course I can't prevent a board
 file from messing up a data structure.  I can't prevent it from causing
 memory access violations either; in fact, I can't prevent any bugs in
 other people's code.
 
 Besides, why do you say the board file is free to do whatever it wants
 with the struct phy?  Currently the struct phy is created by the PHY
 provider and the PHY core, right?  It's not even mentioned in the board
 file.

I mean, if you have a struct type of which full declaration is available 
for some code, this code can access any memeber of it without any hacks, 
which is not something that we want to have in board files. The phy struct 
should be opaque for them.

It's technically correct, but quality of this solution isn't
really
nice, because it's a layering violation (at least if I understood
what you mean). This is because you need to have full definition
of
struct phy in board file and a structure that is used as private
data
in PHY core comes from platform code.
   
   You don't have to have a full definition in the board file.  Just a
   partial definition -- most of the contents can be filled in later,
   when
   the PHY driver is ready to store the private data.
   
   It's not a layering violation for one region of the kernel to store
   private data in a structure defined by another part of the kernel.
   This happens all the time (e.g., dev_set_drvdata).
  
  Not really. The phy struct is something that _is_ private data of PHY
  subsystem, not something that can store private data of PHY subsystem
  (sure it can store private data of particular PHY driver, but that's
  another story) and only PHY subsystem should have access to its
  contents.
 If you want to keep the phy struct completely separate from the board
 file, there's an easy way to do it.  Let's say the board file knows
 about N different PHYs in the system.  Then you define an array of N
 pointers to phys:
 
 struct phy *(phy_address[N]);
 
 In the platform data for both PHY j and its controller, store
 phy_address[j].  The PHY provider passes this cookie to phy_create:
 
   cookie = pdev-dev.platform_data;
   ret = phy_create(phy, cookie);
 
 and phy_create simply stores: *cookie = phy.  The PHY consumer does
 much the same the same thing:
 
   cookie = pdev-dev.platform_data;
   phy = phy_get(cookie);
 
 phy_get returns *cookie if it isn't NULL, or an ERR_PTR otherwise.

OK, this can work. Again, just technically, because it's rather ugly.

  By the way, we need to consider other cases here as well, for example
  it would be nice to have a single phy_get() function that works for
  both non- DT and DT cases to make the consumer driver not have to
  worry whether it's being probed from DT or not.
 
 You ought to be able to adapt this scheme to work with DT.  Maybe by
 having multiple phy_address arrays.

Where would you want to have those phy_address arrays stored? There are no 
board files when booting with DT. Not even saying that you don't need to 
use any hacky schemes like this when you have DT that nicely specifies 
relations between devices.

Anyway, board file should not be considered as a method to exchange data 
between drivers. It should be used only to pass data from it to drivers, 
not the other way. Ideally all data in a board file should be marked as 
const and __init and dropped after system initialization.

Best regards,
Tomasz

Re: [PATCH 01/15] drivers: phy: add generic PHY framework

[Tomasz Figa]

On Tuesday 23 of July 2013 13:50:07 Greg KH wrote:
 On Tue, Jul 23, 2013 at 10:07:52PM +0200, Tomasz Figa wrote:
  On Tuesday 23 of July 2013 12:44:23 Greg KH wrote:
   On Tue, Jul 23, 2013 at 08:31:05PM +0100, Mark Brown wrote:
 You don't know the id of the device you are looking up, due to
 multiple devices being in the system (dynamic ids, look back
 earlier
 in
 this thread for details about that.)

I got copied in very late so don't have most of the thread I'm
afraid,
I did try looking at web archives but didn't see a clear problem
statement.  In any case this is why the APIs doing lookups do the
lookups in the context of the requesting device - devices ask for
whatever name they use locally.
   
   What do you mean by locally?
   
   The problem with the api was that the phy core wanted a id and a
   name to create a phy, and then later other code was doing a
   lookup based on the name and id (mushed together), because it
   knew that this device was the one it wanted.
   
   Just like the clock api, which, for multiple devices, has proven to
   cause problems.  I don't want to see us accept an api that we know
   has
   issues in it now, I'd rather us fix it up properly.
   
   Subsystems should be able to create ids how ever they want to, and
   not
   rely on the code calling them to specify the names of the devices
   that
   way, otherwise the api is just too fragile.
   
   I think, that if you create a device, then just carry around the
   pointer to that device (in this case a phy) and pass it to whatever
   other code needs it.  No need to do lookups on known names or
   anything else, just normal pointers, with no problems for multiple
   devices, busses, or naming issues.
  
  PHY object is not a device, it is something that a device driver
  creates (one or more instances of) when it is being probed.
 
 But you created a 'struct device' for it, so I think of it as a device
 be it virtual or real :)

Keep in mind that those virtual devices are created by PHY driver bound to 
a real device and one real device can have multiple virtual devices behind 
it.

  You don't have a clean way to export this PHY object to other driver,
  other than keeping this PHY on a list inside PHY core with some
  well-known ID (e.g. device name + consumer port name/index, like in
  regulator core) and then to use this well-known ID inside consumer
  driver as a lookup key passed to phy_get();
  
  Actually I think for PHY case, exactly the same way as used for
  regulators might be completely fine:
  
  1. Each PHY would have some kind of platform, non-unique name, that is
  just used to print some messages (like the platform/board name of a
  regulator).
  2. Each PHY would have an array of consumers. Consumer specifier would
  consist of consumer device name and consumer port name - just like in
  regulator subsystem.
  3. PHY driver receives an array of, let's say, phy_init_data inside
  its
  platform data that it would use to register its PHYs.
  4. Consumer drivers would have constant consumer port names and
  wouldn't receive any information about PHYs from platform code.
  
  Code example:
  
  [Board file]
  
  static const struct phy_consumer_data usb_20_phy0_consumers[] = {
  
  {
  
  .devname = foo-ehci,
  .port = usbphy,
  
  },
  
  };
  
  static const struct phy_consumer_data usb_20_phy1_consumers[] = {
  
  {
  
  .devname = foo-otg,
  .port = otgphy,
  
  },
  
  };
  
  static const struct phy_init_data my_phys[] = {
  
  {
  
  .name = USB 2.0 PHY 0,
  .consumers = usb_20_phy0_consumers,
  .num_consumers = ARRAY_SIZE(usb_20_phy0_consumers),
  
  },
  {
  
  .name = USB 2.0 PHY 1,
  .consumers = usb_20_phy1_consumers,
  .num_consumers = ARRAY_SIZE(usb_20_phy1_consumers),
  
  },
  { }
  
  };
  
  static const struct platform_device usb_phy_pdev = {
  
  .name = foo-usbphy,
  .id = -1,
  .dev = {
  
  .platform_data = my_phys,
  
  },
  
  };
  
  [PHY driver]
  
  static int foo_usbphy_probe(pdev)
  {
  
  struct foo_usbphy *foo;
  struct phy_init_data *init_data = pdev-dev.platform_data;
  /* ... */
  // for each PHY in init_data {
  
  phy_register(foo-phy[i], init_data[i]);
  
  // }
  /* ... */
  
  }
  
  [EHCI driver]
  
  static int foo_ehci_probe(pdev)
  {
  
  struct phy *phy;
  /* ... */
  phy = phy_get(pdev-dev, usbphy);
  /* ... */
  
  }
  
  [OTG driver]
  
  static int foo_otg_probe(pdev)
  {
  
  struct phy *phy;
  /* ... */
  phy = phy_get(pdev-dev, otgphy);
  /* ... */
  
  }
 
 That's not so bad, as long as you let the phy core use whatever name it
 wants for the device when it registers it with sysfs.

Yes, in regulator core

Re: [PATCH 01/15] drivers: phy: add generic PHY framework

[Tomasz Figa]

On Tuesday 23 of July 2013 17:14:20 Alan Stern wrote:
 On Tue, 23 Jul 2013, Tomasz Figa wrote:
   If you want to keep the phy struct completely separate from the
   board
   file, there's an easy way to do it.  Let's say the board file knows
   about N different PHYs in the system.  Then you define an array of N
   pointers to phys:
   
   struct phy *(phy_address[N]);
   
   In the platform data for both PHY j and its controller, store
   
   phy_address[j].  The PHY provider passes this cookie to phy_create:
 cookie = pdev-dev.platform_data;
 ret = phy_create(phy, cookie);
   
   and phy_create simply stores: *cookie = phy.  The PHY consumer does
   
   much the same the same thing:
 cookie = pdev-dev.platform_data;
 phy = phy_get(cookie);
   
   phy_get returns *cookie if it isn't NULL, or an ERR_PTR otherwise.
  
  OK, this can work. Again, just technically, because it's rather ugly.
 
 There's no reason the phy_address things have to be arrays.  A separate
 individual pointer for each PHY would work just as well.
 
  Where would you want to have those phy_address arrays stored? There
  are no board files when booting with DT. Not even saying that you
  don't need to use any hacky schemes like this when you have DT that
  nicely specifies relations between devices.
 
 If everybody agrees DT has a nice scheme for specifying relations
 between devices, why not use that same scheme in the PHY core?

It is already used, for cases when consumer device has a DT node attached. 
In non-DT case this kind lookup translates loosely to something that is 
being done in regulator framework - you can't bind devices by pointers, 
because you don't have those pointers, so you need to use device names.

  Anyway, board file should not be considered as a method to exchange
  data between drivers. It should be used only to pass data from it to
  drivers, not the other way. Ideally all data in a board file should
  be marked as const and __init and dropped after system
  initialization.
 
 The phy_address things don't have to be defined or allocated in the
 board file; they could be set up along with the platform data.

There is no platform data when booting with DT.

 In any case, this was simply meant to be a suggestion to show that it
 is relatively easy to do what you need without using name or ID
 strings.

Sure. It's good to have different options discussed as well.

Best regards,
Tomasz

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/15] drivers: phy: add generic PHY framework

[Tomasz Figa]

Hi,

On Saturday 20 of July 2013 19:59:10 Greg KH wrote:
 On Sat, Jul 20, 2013 at 10:32:26PM -0400, Alan Stern wrote:
  On Sat, 20 Jul 2013, Greg KH wrote:
That should be passed using platform data.

Ick, don't pass strings around, pass pointers.  If you have
platform
data you can get to, then put the pointer there, don't use a
name.

I don't think I understood you here :-s We wont have phy pointer
when we create the device for the controller no?(it'll be done in
board file). Probably I'm missing something.
   
   Why will you not have that pointer?  You can't rely on the name as
   the device id will not match up, so you should be able to rely on
   the pointer being in the structure that the board sets up, right?
   
   Don't use names, especially as ids can, and will, change, that is
   going
   to cause big problems.  Use pointers, this is C, we are supposed to
   be
   doing that :)
  
  Kishon, I think what Greg means is this:  The name you are using must
  be stored somewhere in a data structure constructed by the board file,
  right?  Or at least, associated with some data structure somehow.
  Otherwise the platform code wouldn't know which PHY hardware
  corresponded to a particular name.
  
  Greg's suggestion is that you store the address of that data structure
  in the platform data instead of storing the name string.  Have the
  consumer pass the data structure's address when it calls phy_create,
  instead of passing the name.  Then you don't have to worry about two
  PHYs accidentally ending up with the same name or any other similar
  problems.
 
 Close, but the issue is that whatever returns from phy_create() should
 then be used, no need to call any find functions, as you can just use
 the pointer that phy_create() returns.  Much like all other class api
 functions in the kernel work.

I think there is a confusion here about who registers the PHYs.

All platform code does is registering a platform/i2c/whatever device, 
which causes a driver (located in drivers/phy/) to be instantiated. Such 
drivers call phy_create(), usually in their probe() callbacks, so 
platform_code has no way (and should have no way, for the sake of 
layering) to get what phy_create() returns.

IMHO we need a lookup method for PHYs, just like for clocks, regulators, 
PWMs or even i2c busses because there are complex cases when passing just 
a name using platform data will not work. I would second what Stephen said 
[1] and define a structure doing things in a DT-like way.

Example;

[platform code]

static const struct phy_lookup my_phy_lookup[] = {
PHY_LOOKUP(s3c-hsotg.0, otg, samsung-usbphy.1, phy.2),
/* etc... */
};

static void my_machine_init(void)
{
/* ... */
phy_register_lookup(my_phy_lookup, ARRAY_SIZE(my_phy_lookup));
/* ... */
}

/* Notice nothing stuffed in platform data. */

[provider code - samsung-usbphy driver]

static int samsung_usbphy_probe(...)
{
/* ... */
for (i = 0; i  PHY_COUNT; ++i) {
usbphy-phy[i].name = phy;
usbphy-phy[i].id = i;
/* ... */
ret = phy_create(usbphy-phy);
/* err handling */
}
/* ... */
}

[consumer code - s3c-hsotg driver]

static int s3c_hsotg_probe(...)
{
/* ... */
phy = phy_get(pdev-dev, otg);
/* ... */
}

[1] http://thread.gmane.org/gmane.linux.ports.arm.kernel/252813

Best regards,
Tomasz

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/15] drivers: phy: add generic PHY framework

[Tomasz Figa]

On Sunday 21 of July 2013 16:37:33 Kishon Vijay Abraham I wrote:
 Hi,
 
 On Sunday 21 July 2013 04:01 PM, Tomasz Figa wrote:
  Hi,
  
  On Saturday 20 of July 2013 19:59:10 Greg KH wrote:
  On Sat, Jul 20, 2013 at 10:32:26PM -0400, Alan Stern wrote:
  On Sat, 20 Jul 2013, Greg KH wrote:
  That should be passed using platform data.
  
  Ick, don't pass strings around, pass pointers.  If you have
  platform
  data you can get to, then put the pointer there, don't use a
  name.
  
  I don't think I understood you here :-s We wont have phy pointer
  when we create the device for the controller no?(it'll be done in
  board file). Probably I'm missing something.
  
  Why will you not have that pointer?  You can't rely on the name
  as
  the device id will not match up, so you should be able to rely on
  the pointer being in the structure that the board sets up, right?
  
  Don't use names, especially as ids can, and will, change, that is
  going
  to cause big problems.  Use pointers, this is C, we are supposed to
  be
  doing that :)
  
  Kishon, I think what Greg means is this:  The name you are using
  must
  be stored somewhere in a data structure constructed by the board
  file,
  right?  Or at least, associated with some data structure somehow.
  Otherwise the platform code wouldn't know which PHY hardware
  corresponded to a particular name.
  
  Greg's suggestion is that you store the address of that data
  structure
  in the platform data instead of storing the name string.  Have the
  consumer pass the data structure's address when it calls phy_create,
  instead of passing the name.  Then you don't have to worry about two
  PHYs accidentally ending up with the same name or any other similar
  problems.
  
  Close, but the issue is that whatever returns from phy_create()
  should
  then be used, no need to call any find functions, as you can just
  use
  the pointer that phy_create() returns.  Much like all other class api
  functions in the kernel work.
  
  I think there is a confusion here about who registers the PHYs.
  
  All platform code does is registering a platform/i2c/whatever device,
  which causes a driver (located in drivers/phy/) to be instantiated.
  Such drivers call phy_create(), usually in their probe() callbacks,
  so platform_code has no way (and should have no way, for the sake of
  layering) to get what phy_create() returns.
 
 right.
 
  IMHO we need a lookup method for PHYs, just like for clocks,
  regulators, PWMs or even i2c busses because there are complex cases
  when passing just a name using platform data will not work. I would
  second what Stephen said [1] and define a structure doing things in a
  DT-like way.
  
  Example;
  
  [platform code]
  
  static const struct phy_lookup my_phy_lookup[] = {
  
  PHY_LOOKUP(s3c-hsotg.0, otg, samsung-usbphy.1, phy.2),
 
 The only problem here is that if *PLATFORM_DEVID_AUTO* is used while
 creating the device, the ids in the device name would change and
 PHY_LOOKUP wont be useful.

I don't think this is a problem. All the existing lookup methods already 
use ID to identify devices (see regulators, clkdev, PWMs, i2c, ...). You 
can simply add a requirement that the ID must be assigned manually, 
without using PLATFORM_DEVID_AUTO to use PHY lookup.

Best regards,
Tomasz

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] of/irq: store IRQ trigger/level in struct resource flags

[Tomasz Figa]

On Thursday 06 of June 2013 10:50:39 Jean-Christophe PLAGNIOL-VILLARD 
wrote:
 On 00:26 Thu 06 Jun , Grant Likely wrote:
  On Tue, 09 Apr 2013 00:44:05 +0200, Javier Martinez Canillas 
javier.marti...@collabora.co.uk wrote:
   On 04/09/2013 12:05 AM, Rob Herring wrote:
On 04/05/2013 02:48 AM, Javier Martinez Canillas wrote:
This means that drivers that need the IRQ type/level flags
defined in
the DT won't be able to get it.

But the interrupt controllers that need the information should be
able
to get to it via irqd_get_trigger_type. What problem exactly are
you
trying to fix? What driver would use this?
   
   Yes but this is not about the interrupt controller wanting this
   information but a device driver that is using the IORESOURCE_IRQ
   struct resource that has the information about the virtual IRQ
   associated with a GPIO-IRQ.
   
   The driver doesn't know neither care if its IRQ line is connected to
   a line of an real IRQ controller or to a GPIO controller that
   allows a GPIO line to be used as an IRQ.
   
My understanding of the IORESOURCE_IRQ_xxx (and DMA) bits are they
are
ISA specific and therefore should not be used on non-ISA buses.
   
   Many TI OMAP2+ SoC based boards have an SMSC LAN911x/912x controller
   (drivers/net/ethernet/smsc/smsc911x.c) that is connected to the OMAP
   processor through its General-Purpose Memory Controller (GPMC) and
   this LAN driver obtain its IRQ and I/O address space from a struct
   resource IORESOURCE_IRQ and IORESOURCE_MEM respectively, that is
   filled by the DeviceTree core.
   
   It does this:
   
   irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
   irq_flags = irq_res-flags  IRQF_TRIGGER_MASK;
   
   Since of_irq_to_resource() doesn't fill the trigger/level flags on
   the
   IORESOURCE_IRQ struct resource, irq_flags will always be 0 regarding
   the value specified on the second cell of the interrupts DT
   property.
 
 why do you need to known this in you driver this need to be handle in
 the irq chip

There are devices that support multiple interrupt trigger types, like 
Broadcom WLAN chips, and drivers must configure them properly for 
requested trigger type, obtaining the information from flags field of the 
IRQ resource.

Best regards,
Tomasz

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 1/5] ARM: cache-l2x0: add 'smc' identifier

[Tomasz Figa]

Hi Srinidhi,

On Tuesday 29 of January 2013 15:43:31 srinidhi kasagar wrote:
 Add 'smc' (Secure Monitor Call) identifier to differentiates
 the platforms which implements this.
 
 Signed-off-by: srinidhi kasagar srinidhi.kasa...@stericsson.com
 ---
  arch/arm/boot/dts/omap4.dtsi   |1 +
  arch/arm/include/asm/hardware/cache-l2x0.h |2 +-
  arch/arm/mach-cns3xxx/core.c   |4 ++--
  arch/arm/mach-exynos/common.c  |4 ++--
  arch/arm/mach-imx/mm-imx3.c|4 ++--
  arch/arm/mach-nomadik/cpu-8815.c   |4 ++--
  arch/arm/mach-omap2/omap4-common.c |2 +-
  arch/arm/mach-realview/realview_eb.c   |4 ++--
  arch/arm/mach-realview/realview_pb1176.c   |4 ++--
  arch/arm/mach-realview/realview_pb11mp.c   |4 ++--
  arch/arm/mach-realview/realview_pbx.c  |4 ++--
  arch/arm/mach-shmobile/board-ag5evm.c  |2 +-
  arch/arm/mach-shmobile/board-armadillo800eva.c |4 ++--
  arch/arm/mach-shmobile/board-bonito.c  |4 ++--
  arch/arm/mach-shmobile/board-kota2.c   |4 ++--
  arch/arm/mach-shmobile/board-kzm9g.c   |4 ++--
  arch/arm/mach-shmobile/setup-r8a7779.c |4 ++--
  arch/arm/mach-spear13xx/spear13xx.c|2 +-
  arch/arm/mach-ux500/cache-l2x0.c   |2 +-
  arch/arm/mach-vexpress/ct-ca9x4.c  |2 +-
  arch/arm/mm/cache-l2x0.c   |   18
 ++ 21 files changed, 47 insertions(+), 36 deletions(-)

I'd recommend you to take a look at this series:
http://thread.gmane.org/gmane.linux.ports.arm.kernel/202224/

It's already queued up for 3.9 in Samsung tree and most likely provides an 
interface needed for what you want to achieve.

Best regards,
-- 
Tomasz Figa
Samsung Poland RD Center
SW Solution Development, Linux Platform

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH 32/32] usb: gadget: drop now unnecessary flag

[Tomasz Figa]

Hi Felipe,

On Thursday 24 of January 2013 17:46:10 Felipe Balbi wrote:
 We don't need the -register_my_device flag
 anymore because all UDC drivers have been
 properly converted.
 
 Let's remove every history of it.
 
 Signed-off-by: Felipe Balbi ba...@ti.com
 ---
  drivers/usb/chipidea/udc.c | 1 -
  drivers/usb/dwc3/gadget.c  | 1 -
  drivers/usb/gadget/amd5536udc.c| 1 -
  drivers/usb/gadget/at91_udc.c  | 1 -
  drivers/usb/gadget/atmel_usba_udc.c| 1 -
  drivers/usb/gadget/bcm63xx_udc.c   | 1 -
  drivers/usb/gadget/dummy_hcd.c | 1 -
  drivers/usb/gadget/fsl_qe_udc.c| 1 -
  drivers/usb/gadget/fsl_udc_core.c  | 1 -
  drivers/usb/gadget/fusb300_udc.c   | 1 -
  drivers/usb/gadget/goku_udc.c  | 1 -
  drivers/usb/gadget/imx_udc.c   | 1 -
  drivers/usb/gadget/lpc32xx_udc.c   | 1 -
  drivers/usb/gadget/m66592-udc.c| 1 -
  drivers/usb/gadget/mv_u3d_core.c   | 1 -
  drivers/usb/gadget/mv_udc_core.c   | 1 -
  drivers/usb/gadget/net2272.c   | 1 -
  drivers/usb/gadget/net2280.c   | 1 -
  drivers/usb/gadget/omap_udc.c  | 1 -
  drivers/usb/gadget/pch_udc.c   | 1 -
  drivers/usb/gadget/pxa25x_udc.c| 1 -
  drivers/usb/gadget/pxa27x_udc.c| 1 -
  drivers/usb/gadget/r8a66597-udc.c  | 1 -
  drivers/usb/gadget/s3c-hsotg.c | 1 -
  drivers/usb/gadget/s3c-hsudc.c | 1 -
  drivers/usb/gadget/s3c2410_udc.c   | 1 -
  drivers/usb/gadget/udc-core.c  | 3 ---
  drivers/usb/musb/musb_gadget.c | 1 -
  drivers/usb/renesas_usbhs/mod_gadget.c | 1 -
  include/linux/usb/gadget.h | 4 
  30 files changed, 35 deletions(-)
 
[snip]
 diff --git a/drivers/usb/gadget/udc-core.c
 b/drivers/usb/gadget/udc-core.c index 9195054..4ee0efc 100644
 --- a/drivers/usb/gadget/udc-core.c
 +++ b/drivers/usb/gadget/udc-core.c
 @@ -173,7 +173,6 @@ int usb_add_gadget_udc(struct device *parent, struct
 usb_gadget *gadget) if (!udc)
   goto err1;
 
 - if (gadget-register_my_device) {
   dev_set_name(gadget-dev, gadget);
 
   ret = device_register(gadget-dev);
 @@ -211,7 +210,6 @@ err3:
   put_device(udc-dev);
 
  err2:
 - if (gadget-register_my_device)
   put_device(gadget-dev);
  err1:
   return ret;
 @@ -267,7 +265,6 @@ found:
   kobject_uevent(udc-dev.kobj, KOBJ_REMOVE);
   device_unregister(udc-dev);
 
 - if (gadget-register_my_device)
   device_unregister(gadget-dev);

Correct me if I am wrong, but doesn't this patch leave us with incorrect 
indentation?

Otherwise looks good.

Best regards,
Tomasz Figa

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH 01/32] usb: gadget: udc-core: allow udc class register gadget device

[Tomasz Figa]

Hi Felipe,

On Thursday 24 of January 2013 17:45:39 Felipe Balbi wrote:
 Currently all UDC drivers are calling
 device_register() before calling
 usb_add_gadget_udc(). In order to avoid
 code duplication, we can allow udc-core.c
 register that device.
 
 However that would become a really large patch,
 so to cope with the meanwhile and allow us
 to write bite-sized patches, we're adding
 a flag which will be set by UDC driver once
 it removes the code for registering the
 gadget device.
 
 Once all are converted, the new flag will
 be removed.
 
 Signed-off-by: Felipe Balbi ba...@ti.com
 ---
  drivers/usb/gadget/udc-core.c | 23 +++
  include/linux/usb/gadget.h|  4 
  2 files changed, 23 insertions(+), 4 deletions(-)
 
 diff --git a/drivers/usb/gadget/udc-core.c
 b/drivers/usb/gadget/udc-core.c index 2a9cd36..9195054 100644
 --- a/drivers/usb/gadget/udc-core.c
 +++ b/drivers/usb/gadget/udc-core.c
 @@ -173,6 +173,14 @@ int usb_add_gadget_udc(struct device *parent,
 struct usb_gadget *gadget) if (!udc)
   goto err1;
 
 + if (gadget-register_my_device) {
 + dev_set_name(gadget-dev, gadget);
 +
 + ret = device_register(gadget-dev);
 + if (ret)
 + goto err2;
 + }
 +
   device_initialize(udc-dev);
   udc-dev.release = usb_udc_release;
   udc-dev.class = udc_class;
 @@ -180,7 +188,7 @@ int usb_add_gadget_udc(struct device *parent, struct
 usb_gadget *gadget) udc-dev.parent = parent;
   ret = dev_set_name(udc-dev, %s, kobject_name(parent-kobj));
   if (ret)
 - goto err2;
 + goto err3;

Just a nitpick: If you are at changing labels of error path, wouldn't it 
be better to give them some meaningful names? Like err_del_unlock, 
err_put_udc, err_put_gadget, err_ret.

Otherwise looks good. Nice idea.

Reviewed-by: Tomasz Figa tomasz.f...@gmail.com

Best regards,
Tomasz Figa

   udc-gadget = gadget;
 
 @@ -189,18 +197,22 @@ int usb_add_gadget_udc(struct device *parent,
 struct usb_gadget *gadget)
 
   ret = device_add(udc-dev);
   if (ret)
 - goto err3;
 + goto err4;
 
   mutex_unlock(udc_lock);
 
   return 0;
 -err3:
 +
 +err4:
   list_del(udc-list);
   mutex_unlock(udc_lock);
 
 -err2:
 +err3:
   put_device(udc-dev);
 
 +err2:
 + if (gadget-register_my_device)
 + put_device(gadget-dev);
  err1:
   return ret;
  }
 @@ -254,6 +266,9 @@ found:
 
   kobject_uevent(udc-dev.kobj, KOBJ_REMOVE);
   device_unregister(udc-dev);
 +
 + if (gadget-register_my_device)
 + device_unregister(gadget-dev);
  }
  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
 
 diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
 index 2e297e8..fcd9ef8 100644
 --- a/include/linux/usb/gadget.h
 +++ b/include/linux/usb/gadget.h
 @@ -494,6 +494,9 @@ struct usb_gadget_ops {
   *   only supports HNP on a different root port.
   * @b_hnp_enable: OTG device feature flag, indicating that the A-Host
   *   enabled HNP support.
 + * @register_my_device: Flag telling udc-core that UDC driver didn't
 + *   register the gadget device to the driver model. Temporary until
 + *   all UDC drivers are fixed up properly.
   * @name: Identifies the controller hardware type.  Used in diagnostics
 * and sometimes configuration.
   * @dev: Driver model state for this abstract device.
 @@ -531,6 +534,7 @@ struct usb_gadget {
   unsignedb_hnp_enable:1;
   unsigneda_hnp_support:1;
   unsigneda_alt_hnp_support:1;
 + unsignedregister_my_device:1;
   const char  *name;
   struct device   dev;
   unsignedout_epnum;
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH 24/32] usb: gadget: s3c-hsotg: let udc-core manage gadget-dev

[Tomasz Figa]

Hi Felipe,

On Thursday 24 of January 2013 17:46:02 Felipe Balbi wrote:
 By simply setting a flag, we can drop some
 boilerplate code.
 
 Signed-off-by: Felipe Balbi ba...@ti.com
 ---
  drivers/usb/gadget/s3c-hsotg.c | 14 +-
  1 file changed, 1 insertion(+), 13 deletions(-)
 
 diff --git a/drivers/usb/gadget/s3c-hsotg.c
 b/drivers/usb/gadget/s3c-hsotg.c index 833d85b..bd8292d 100644
 --- a/drivers/usb/gadget/s3c-hsotg.c
 +++ b/drivers/usb/gadget/s3c-hsotg.c
 @@ -3552,17 +3552,13 @@ static int s3c_hsotg_probe(struct
 platform_device *pdev)
 
   dev_info(dev, regs %p, irq %d\n, hsotg-regs, hsotg-irq);
 
 - device_initialize(hsotg-gadget.dev);
 -
 - dev_set_name(hsotg-gadget.dev, gadget);
 -
   hsotg-gadget.max_speed = USB_SPEED_HIGH;
   hsotg-gadget.ops = s3c_hsotg_gadget_ops;
   hsotg-gadget.name = dev_name(dev);
 -
   hsotg-gadget.dev.parent = dev;
   hsotg-gadget.dev.dma_mask = dev-dma_mask;
   hsotg-gadget.dev.release = s3c_hsotg_release;
 + hsotg-gadget.register_my_device = true;
 
   /* reset the system */
 
 @@ -3643,12 +3639,6 @@ static int s3c_hsotg_probe(struct platform_device
 *pdev)
 
   s3c_hsotg_phy_disable(hsotg);
 
 - ret = device_add(hsotg-gadget.dev);
 - if (ret) {
 - put_device(hsotg-gadget.dev);
 - goto err_ep_mem;
 - }
 -
   ret = usb_add_gadget_udc(pdev-dev, hsotg-gadget);
   if (ret)
   goto err_ep_mem;
 @@ -3687,10 +3677,8 @@ static int s3c_hsotg_remove(struct
 platform_device *pdev) }
 
   s3c_hsotg_phy_disable(hsotg);
 -
   clk_disable_unprepare(hsotg-clk);
 
 - device_unregister(hsotg-gadget.dev);
   return 0;
  }

Looks good.

Reviewed-by: Tomasz Figa tomasz.f...@gmail.com

Best regards,
Tomasz Figa

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html