[v3,01/12] clk: tegra: Implement memory-controller clock

Commit Message

Thierry Reding Oct. 13, 2014, 10:33 a.m. UTC

From: Thierry Reding <[e-mail]>

The memory controller clock runs either at half or the same frequency as
the EMC clock.

Signed-off-by: Thierry Reding <[e-mail]>
---
Changes in v3:
- split registration into a separate function that can be reused for all
  SoC generations, but pass in the name and parent parameters for
  clarity as well as the register address (in case it ever changes) and
  the EMC spin-lock since it isn't globally available

 drivers/clk/tegra/clk-divider.c          | 13 +++++++++++++
 drivers/clk/tegra/clk-tegra114.c         |  7 ++++++-
 drivers/clk/tegra/clk-tegra124.c         |  7 ++++++-
 drivers/clk/tegra/clk-tegra20.c          |  8 +++++++-
 drivers/clk/tegra/clk-tegra30.c          |  7 ++++++-
 drivers/clk/tegra/clk.h                  |  2 ++
 include/dt-bindings/clock/tegra114-car.h |  2 +-
 include/dt-bindings/clock/tegra124-car.h |  2 +-
 include/dt-bindings/clock/tegra20-car.h  |  2 +-
 9 files changed, 43 insertions(+), 7 deletions(-)

Comments

Olof Johansson Oct. 15, 2014, 10:05 p.m. UTC | #1

Hi,

On Mon, Oct 13, 2014 at 3:33 AM, Thierry Reding
<[e-mail]> wrote:
[...]
> diff --git a/drivers/memory/tegra/tegra-mc.c b/drivers/memory/tegra/tegra-mc.c
> new file mode 100644
> index 000000000000..0f0c8be096d0
> --- /dev/null
> +++ b/drivers/memory/tegra/tegra-mc.c
> @@ -0,0 +1,1061 @@
> +/*
> + * Copyright (C) 2014 NVIDIA CORPORATION.  All rights reserved.
> + *
> + * 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.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/iommu.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>

You need a linux/mm.h in here (on 64-bit).

> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>


[...]


> diff --git a/drivers/memory/tegra/tegra124-mc.c b/drivers/memory/tegra/tegra124-mc.c
> new file mode 100644
> index 000000000000..db31c96fc288
> --- /dev/null
> +++ b/drivers/memory/tegra/tegra124-mc.c

[...]


> @@ -0,0 +1,1028 @@
> +/*
> + * Copyright (C) 2014 NVIDIA CORPORATION.  All rights reserved.
> + *
> + * 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.
> + */
> +
> +#include <linux/of.h>
> +#include <linux/mm.h>
> +
> +#include <asm/cacheflush.h>
> +
> +#include <dt-bindings/memory/tegra124-mc.h>
> +
> +#include "tegra-mc.h"
> +
> +static const struct tegra_mc_client tegra124_mc_clients[] = {
> +       {
> +               .id = 0x00,
> +               .name = "ptcr",
> +               .swgroup = TEGRA_SWGROUP_PTC,
> +       }, {
> +               .id = 0x01,
> +               .name = "display0a",
> +               .swgroup = TEGRA_SWGROUP_DC,
> +               .smmu = {
> +                       .reg = 0x228,
> +                       .bit = 1,
> +               },
> +               .latency = {
> +                       .reg = 0x2e8,
> +                       .shift = 0,
> +                       .mask = 0xff,
> +                       .def = 0xc2,
> +               },
> +       }, {

These are very verbose tables. Having a macro for the initializers
could help density a lot.



> +#ifdef CONFIG_ARCH_TEGRA_132_SOC
> +static void tegra132_flush_dcache(struct page *page, unsigned long offset,
> +                                 size_t size)
> +{
> +       void *virt = page_address(page) + offset;
> +
> +       __flush_dcache_area(virt, size);
> +}
> +
> +static const struct tegra_smmu_ops tegra132_smmu_ops = {
> +       .flush_dcache = tegra132_flush_dcache,
> +};
> +
> +static const struct tegra_smmu_soc tegra132_smmu_soc = {
> +       .groups = tegra124_smmu_groups,
> +       .num_groups = ARRAY_SIZE(tegra124_smmu_groups),
> +       .clients = tegra124_mc_clients,
> +       .num_clients = ARRAY_SIZE(tegra124_mc_clients),
> +       .swgroups = tegra124_swgroups,
> +       .num_swgroups = ARRAY_SIZE(tegra124_swgroups),
> +       .supports_round_robin_arbitration = true,
> +       .supports_request_limit = true,
> +       .num_address_bits = 34,
> +       .num_asids = 128,
> +       .ops = &tegra132_smmu_ops,
> +};
> +
> +const struct tegra_mc_soc tegra132_mc_soc = {
> +       .clients = tegra124_mc_clients,
> +       .num_clients = ARRAY_SIZE(tegra124_mc_clients),
> +       .atom_size = 32,
> +       .smmu = &tegra132_smmu_soc,
> +};
> +#endif /* CONFIG_ARCH_TEGRA_132_SOC */


This won't compile -- several of the tegra132_smmu_soc members are no
longer valid. In particular:

groups
num_groups
supports_round_robin_arbitration
supports_request_limit


-Olof

Olof Johansson Oct. 15, 2014, 10:09 p.m. UTC | #2

Hi,

Oh, a few more comments:

On Mon, Oct 13, 2014 at 3:33 AM, Thierry Reding
<[e-mail]> wrote:

> diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
> index c32d31981be3..1c932e7e7b8d 100644
> --- a/drivers/memory/Makefile
> +++ b/drivers/memory/Makefile
> @@ -12,4 +12,5 @@ obj-$(CONFIG_FSL_CORENET_CF)  += fsl-corenet-cf.o
>  obj-$(CONFIG_FSL_IFC)          += fsl_ifc.o
>  obj-$(CONFIG_MVEBU_DEVBUS)     += mvebu-devbus.o
>  obj-$(CONFIG_TEGRA20_MC)       += tegra20-mc.o
> -obj-$(CONFIG_TEGRA30_MC)       += tegra30-mc.o
> +
> +obj-$(CONFIG_ARCH_TEGRA)       += tegra/
> diff --git a/drivers/memory/tegra/Makefile b/drivers/memory/tegra/Makefile
> new file mode 100644
> index 000000000000..51b9e8fcde1b
> --- /dev/null
> +++ b/drivers/memory/tegra/Makefile
> @@ -0,0 +1,5 @@
> +obj-y                                   = tegra-mc.o
> +obj-$(CONFIG_ARCH_TEGRA_3x_SOC)                += tegra30-mc.o
> +obj-$(CONFIG_ARCH_TEGRA_114_SOC)       += tegra114-mc.o
> +obj-$(CONFIG_ARCH_TEGRA_124_SOC)       += tegra124-mc.o
> +obj-$(CONFIG_ARCH_TEGRA_132_SOC)       += tegra124-mc.o

You'll need a Kconfig and not just a makefile -- there are definitely
dependencies on this driver (IOMMU in particular).


Also, the problem of having a global enable bit that is only under
control of TrustZone FW is a big problem -- if the bit is not set, the
driver will not work (and the machine will crash).

I think you'll need to come up with a way to detect that in the
driver. I don't have a good idea of how it can be done though.


-Olof

Tomeu Vizoso Oct. 20, 2014, 11:02 a.m. UTC | #3

On 13 October 2014 12:33, Thierry Reding <[e-mail]> wrote:
> From: Thierry Reding <[e-mail]>
>
> The memory controller clock runs either at half or the same frequency as
> the EMC clock.
>
> Signed-off-by: Thierry Reding <[e-mail]>
> ---
> Changes in v3:
> - split registration into a separate function that can be reused for all
>   SoC generations, but pass in the name and parent parameters for
>   clarity as well as the register address (in case it ever changes) and
>   the EMC spin-lock since it isn't globally available
>
>  drivers/clk/tegra/clk-divider.c          | 13 +++++++++++++
>  drivers/clk/tegra/clk-tegra114.c         |  7 ++++++-
>  drivers/clk/tegra/clk-tegra124.c         |  7 ++++++-
>  drivers/clk/tegra/clk-tegra20.c          |  8 +++++++-
>  drivers/clk/tegra/clk-tegra30.c          |  7 ++++++-
>  drivers/clk/tegra/clk.h                  |  2 ++
>  include/dt-bindings/clock/tegra114-car.h |  2 +-
>  include/dt-bindings/clock/tegra124-car.h |  2 +-
>  include/dt-bindings/clock/tegra20-car.h  |  2 +-
>  9 files changed, 43 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/clk/tegra/clk-divider.c b/drivers/clk/tegra/clk-divider.c
> index 290f9c1a3749..84e1b3c1fb2a 100644
> --- a/drivers/clk/tegra/clk-divider.c
> +++ b/drivers/clk/tegra/clk-divider.c
> @@ -185,3 +185,16 @@ struct clk *tegra_clk_register_divider(const char *name,
>
>         return clk;
>  }
> +
> +static const struct clk_div_table mc_div_table[] = {
> +       { .val = 0, .div = 2 },
> +       { .val = 1, .div = 1 },
> +       { .val = 0, .div = 0 },
> +};
> +
> +struct clk *tegra_clk_register_mc(const char *name, const char *parent_name,
> +                                 void __iomem *reg, spinlock_t *lock)
> +{
> +       return clk_register_divider_table(NULL, "mc", "emc_mux", 0, reg,

Should take name and parent_name from the passed args?

With that change:

Reviewed-By: Tomeu Vizoso <[e-mail]>

Cheers,

Tomeu

> +                                         16, 1, 0, mc_div_table, lock);
> +}
> diff --git a/drivers/clk/tegra/clk-tegra114.c b/drivers/clk/tegra/clk-tegra114.c
> index f760f31d05c4..0b03d2cf7264 100644
> --- a/drivers/clk/tegra/clk-tegra114.c
> +++ b/drivers/clk/tegra/clk-tegra114.c
> @@ -173,6 +173,7 @@ static DEFINE_SPINLOCK(pll_d_lock);
>  static DEFINE_SPINLOCK(pll_d2_lock);
>  static DEFINE_SPINLOCK(pll_u_lock);
>  static DEFINE_SPINLOCK(pll_re_lock);
> +static DEFINE_SPINLOCK(emc_lock);
>
>  static struct div_nmp pllxc_nmp = {
>         .divm_shift = 0,
> @@ -1228,7 +1229,11 @@ static __init void tegra114_periph_clk_init(void __iomem *clk_base,
>                                ARRAY_SIZE(mux_pllmcp_clkm),
>                                CLK_SET_RATE_NO_REPARENT,
>                                clk_base + CLK_SOURCE_EMC,
> -                              29, 3, 0, NULL);
> +                              29, 3, 0, &emc_lock);
> +
> +       clk = tegra_clk_register_mc("mc", "emc_mux", clk_base + CLK_SOURCE_EMC,
> +                                   &emc_lock);
> +       clks[TEGRA114_CLK_MC] = clk;
>
>         for (i = 0; i < ARRAY_SIZE(tegra_periph_clk_list); i++) {
>                 data = &tegra_periph_clk_list[i];
> diff --git a/drivers/clk/tegra/clk-tegra124.c b/drivers/clk/tegra/clk-tegra124.c
> index e3a85842ce0c..f5f9baca7bb6 100644
> --- a/drivers/clk/tegra/clk-tegra124.c
> +++ b/drivers/clk/tegra/clk-tegra124.c
> @@ -132,6 +132,7 @@ static DEFINE_SPINLOCK(pll_d2_lock);
>  static DEFINE_SPINLOCK(pll_e_lock);
>  static DEFINE_SPINLOCK(pll_re_lock);
>  static DEFINE_SPINLOCK(pll_u_lock);
> +static DEFINE_SPINLOCK(emc_lock);
>
>  /* possible OSC frequencies in Hz */
>  static unsigned long tegra124_input_freq[] = {
> @@ -1127,7 +1128,11 @@ static __init void tegra124_periph_clk_init(void __iomem *clk_base,
>         clk = clk_register_mux(NULL, "emc_mux", mux_pllmcp_clkm,
>                                ARRAY_SIZE(mux_pllmcp_clkm), 0,
>                                clk_base + CLK_SOURCE_EMC,
> -                              29, 3, 0, NULL);
> +                              29, 3, 0, &emc_lock);
> +
> +       clk = tegra_clk_register_mc("mc", "emc_mux", clk_base + CLK_SOURCE_EMC,
> +                                   &emc_lock);
> +       clks[TEGRA124_CLK_MC] = clk;
>
>         /* cml0 */
>         clk = clk_register_gate(NULL, "cml0", "pll_e", 0, clk_base + PLLE_AUX,
> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
> index dace2b1b5ae6..41272dcc9e22 100644
> --- a/drivers/clk/tegra/clk-tegra20.c
> +++ b/drivers/clk/tegra/clk-tegra20.c
> @@ -140,6 +140,8 @@ static struct cpu_clk_suspend_context {
>  static void __iomem *clk_base;
>  static void __iomem *pmc_base;
>
> +static DEFINE_SPINLOCK(emc_lock);
> +
>  #define TEGRA_INIT_DATA_MUX(_name, _parents, _offset,  \
>                             _clk_num, _gate_flags, _clk_id)     \
>         TEGRA_INIT_DATA(_name, NULL, NULL, _parents, _offset,   \
> @@ -819,11 +821,15 @@ static void __init tegra20_periph_clk_init(void)
>                                ARRAY_SIZE(mux_pllmcp_clkm),
>                                CLK_SET_RATE_NO_REPARENT,
>                                clk_base + CLK_SOURCE_EMC,
> -                              30, 2, 0, NULL);
> +                              30, 2, 0, &emc_lock);
>         clk = tegra_clk_register_periph_gate("emc", "emc_mux", 0, clk_base, 0,
>                                     57, periph_clk_enb_refcnt);
>         clks[TEGRA20_CLK_EMC] = clk;
>
> +       clk = tegra_clk_register_mc("mc", "emc_mux", clk_base + CLK_SOURCE_EMC,
> +                                   &emc_lock);
> +       clks[TEGRA20_CLK_MC] = clk;
> +
>         /* dsi */
>         clk = tegra_clk_register_periph_gate("dsi", "pll_d", 0, clk_base, 0,
>                                     48, periph_clk_enb_refcnt);
> diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c
> index 5bbacd01094f..4b9d8bd3d0bf 100644
> --- a/drivers/clk/tegra/clk-tegra30.c
> +++ b/drivers/clk/tegra/clk-tegra30.c
> @@ -177,6 +177,7 @@ static unsigned long input_freq;
>
>  static DEFINE_SPINLOCK(cml_lock);
>  static DEFINE_SPINLOCK(pll_d_lock);
> +static DEFINE_SPINLOCK(emc_lock);
>
>  #define TEGRA_INIT_DATA_MUX(_name, _parents, _offset,  \
>                             _clk_num, _gate_flags, _clk_id)     \
> @@ -1157,11 +1158,15 @@ static void __init tegra30_periph_clk_init(void)
>                                ARRAY_SIZE(mux_pllmcp_clkm),
>                                CLK_SET_RATE_NO_REPARENT,
>                                clk_base + CLK_SOURCE_EMC,
> -                              30, 2, 0, NULL);
> +                              30, 2, 0, &emc_lock);
>         clk = tegra_clk_register_periph_gate("emc", "emc_mux", 0, clk_base, 0,
>                                     57, periph_clk_enb_refcnt);
>         clks[TEGRA30_CLK_EMC] = clk;
>
> +       clk = tegra_clk_register_mc("mc", "emc_mux", clk_base + CLK_SOURCE_EMC,
> +                                   &emc_lock);
> +       clks[TEGRA30_CLK_MC] = clk;
> +
>         /* cml0 */
>         clk = clk_register_gate(NULL, "cml0", "pll_e", 0, clk_base + PLLE_AUX,
>                                 0, 0, &cml_lock);
> diff --git a/drivers/clk/tegra/clk.h b/drivers/clk/tegra/clk.h
> index 16ec8d6bb87f..4e458aa8d45c 100644
> --- a/drivers/clk/tegra/clk.h
> +++ b/drivers/clk/tegra/clk.h
> @@ -86,6 +86,8 @@ struct clk *tegra_clk_register_divider(const char *name,
>                 const char *parent_name, void __iomem *reg,
>                 unsigned long flags, u8 clk_divider_flags, u8 shift, u8 width,
>                 u8 frac_width, spinlock_t *lock);
> +struct clk *tegra_clk_register_mc(const char *name, const char *parent_name,
> +                                 void __iomem *reg, spinlock_t *lock);
>
>  /*
>   * Tegra PLL:
> diff --git a/include/dt-bindings/clock/tegra114-car.h b/include/dt-bindings/clock/tegra114-car.h
> index fc12621fb432..534c03f8ad72 100644
> --- a/include/dt-bindings/clock/tegra114-car.h
> +++ b/include/dt-bindings/clock/tegra114-car.h
> @@ -49,7 +49,7 @@
>  #define TEGRA114_CLK_I2S0 30
>  /* 31 */
>
> -/* 32 */
> +#define TEGRA114_CLK_MC 32
>  /* 33 */
>  #define TEGRA114_CLK_APBDMA 34
>  /* 35 */
> diff --git a/include/dt-bindings/clock/tegra124-car.h b/include/dt-bindings/clock/tegra124-car.h
> index 6bac637fd635..af9bc9a3ddbc 100644
> --- a/include/dt-bindings/clock/tegra124-car.h
> +++ b/include/dt-bindings/clock/tegra124-car.h
> @@ -48,7 +48,7 @@
>  #define TEGRA124_CLK_I2S0 30
>  /* 31 */
>
> -/* 32 */
> +#define TEGRA124_CLK_MC 32
>  /* 33 */
>  #define TEGRA124_CLK_APBDMA 34
>  /* 35 */
> diff --git a/include/dt-bindings/clock/tegra20-car.h b/include/dt-bindings/clock/tegra20-car.h
> index 9406207cfac8..04500b243a4d 100644
> --- a/include/dt-bindings/clock/tegra20-car.h
> +++ b/include/dt-bindings/clock/tegra20-car.h
> @@ -49,7 +49,7 @@
>  /* 30 */
>  #define TEGRA20_CLK_CACHE2 31
>
> -#define TEGRA20_CLK_MEM 32
> +#define TEGRA20_CLK_MC 32
>  #define TEGRA20_CLK_AHBDMA 33
>  #define TEGRA20_CLK_APBDMA 34
>  /* 35 */
> --
> 2.1.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alexandre Courbot Oct. 30, 2014, 10:03 a.m. UTC | #4

> diff --git a/drivers/memory/tegra/tegra124-mc.c b/drivers/memory/tegra/tegra124-mc.c
...
> +static const struct tegra_smmu_swgroup tegra124_swgroups[] = {
> +	{ .swgroup = TEGRA_SWGROUP_DC,        .reg = 0x240 },
> +	{ .swgroup = TEGRA_SWGROUP_DCB,       .reg = 0x244 },
> +	{ .swgroup = TEGRA_SWGROUP_AFI,       .reg = 0x238 },
> +	{ .swgroup = TEGRA_SWGROUP_AVPC,      .reg = 0x23c },
> +	{ .swgroup = TEGRA_SWGROUP_HDA,       .reg = 0x254 },
> +	{ .swgroup = TEGRA_SWGROUP_HC,        .reg = 0x250 },
> +	{ .swgroup = TEGRA_SWGROUP_MSENC,     .reg = 0x264 },
> +	{ .swgroup = TEGRA_SWGROUP_PPCS,      .reg = 0x270 },
> +	{ .swgroup = TEGRA_SWGROUP_SATA,      .reg = 0x274 },
> +	{ .swgroup = TEGRA_SWGROUP_VDE,       .reg = 0x27c },
> +	{ .swgroup = TEGRA_SWGROUP_ISP2,      .reg = 0x258 },
> +	{ .swgroup = TEGRA_SWGROUP_XUSB_HOST, .reg = 0x288 },
> +	{ .swgroup = TEGRA_SWGROUP_XUSB_DEV,  .reg = 0x28c },
> +	{ .swgroup = TEGRA_SWGROUP_ISP2B,     .reg = 0xaa4 },
> +	{ .swgroup = TEGRA_SWGROUP_TSEC,      .reg = 0x294 },
> +	{ .swgroup = TEGRA_SWGROUP_A9AVP,     .reg = 0x290 },
> +	{ .swgroup = TEGRA_SWGROUP_GPU,       .reg = 0xaa8 },

I had to change the .reg of TEGRA_SWGROUP_GPU to 0xaac to get the IOMMU 
to work with GK20A. The reason is still not completely clear to me, but 
if you look at the TRM you see that 0xaa8 is basically constant, with 
the SMMU translation bit hardcoded to DISABLE (and the ASID field being 
meaningless in that case). However right after that register you have a 
functional one named GPUB instead of GPU, and this one is fully 
writeable (and has the expected effect).

I will try to get more information about the why of this, but for now 
this setting is what works for me.

Terje Bergstrom Oct. 30, 2014, 10:18 a.m. UTC | #5

On 30.10.2014 12:03, Alexandre Courbot wrote:
> I had to change the .reg of TEGRA_SWGROUP_GPU to 0xaac to get the IOMMU
> to work with GK20A. The reason is still not completely clear to me, but
> if you look at the TRM you see that 0xaa8 is basically constant, with
> the SMMU translation bit hardcoded to DISABLE (and the ASID field being
> meaningless in that case). However right after that register you have a
> functional one named GPUB instead of GPU, and this one is fully
> writeable (and has the expected effect).

GPU has two SW group IDs, because it accesses memory both with and
without translation. The bit 34 in addresses in f.ex. PTE chooses
between the two.

GPU is hard-wired to disable translation. For GPUB translation can be
enabled.

Terje

Alexandre Courbot Oct. 30, 2014, 10:22 a.m. UTC | #6

On Thu, Oct 30, 2014 at 7:18 PM, Terje Bergström <[e-mail]> 
wrote:
 > On 30.10.2014 12:03, Alexandre Courbot wrote:
 >> I had to change the .reg of TEGRA_SWGROUP_GPU to 0xaac to get the IOMMU
 >> to work with GK20A. The reason is still not completely clear to me, but
 >> if you look at the TRM you see that 0xaa8 is basically constant, with
 >> the SMMU translation bit hardcoded to DISABLE (and the ASID field being
 >> meaningless in that case). However right after that register you have a
 >> functional one named GPUB instead of GPU, and this one is fully
 >> writeable (and has the expected effect).
 >
 > GPU has two SW group IDs, because it accesses memory both with and
 > without translation. The bit 34 in addresses in f.ex. PTE chooses
 > between the two.

Indeed, to enable SMMU translation I have to program 0xaac correctly 
*and* set the bit 34 of every address that needs to go through the SMMU.

 > GPU is hard-wired to disable translation. For GPUB translation can be
 > enabled.

So should I understand that the GPU group is for addresses without bit 
34 set (hence forcibly disabled) while GPUB is used when that bit is 
set? Or is it something else?

Terje Bergstrom Oct. 30, 2014, 11:04 a.m. UTC | #7

On 30.10.2014 12:22, Alexandre Courbot wrote:
> So should I understand that the GPU group is for addresses without bit
> 34 set (hence forcibly disabled) while GPUB is used when that bit is
> set? Or is it something else?

That's exactly correct. And only GPUB can be programmed to be SMMU
translated.

Terje

Alexandre Courbot Oct. 30, 2014, 1:35 p.m. UTC | #8

On 10/30/2014 08:04 PM, Terje Bergström wrote:
> On 30.10.2014 12:22, Alexandre Courbot wrote:
>> So should I understand that the GPU group is for addresses without bit
>> 34 set (hence forcibly disabled) while GPUB is used when that bit is
>> set? Or is it something else?
>
> That's exactly correct. And only GPUB can be programmed to be SMMU
> translated.

Great, thanks for confirming!

Thierry, how do you want to address this? We could change the register 
for the GPU group, or (maybe preferable if we want to reflect the actual 
hardware state) add the GPUB group. I don't know if that would be easy 
though since we would have the problem of the gpusrd and gpuswr clients 
ownership (seems like they would belong to both groups?)

Terje Bergstrom Oct. 30, 2014, 1:47 p.m. UTC | #9

On 30.10.2014 15:35, Alexandre Courbot wrote:
> Great, thanks for confirming!
> 
> Thierry, how do you want to address this? We could change the register
> for the GPU group, or (maybe preferable if we want to reflect the actual
> hardware state) add the GPUB group. I don't know if that would be easy
> though since we would have the problem of the gpusrd and gpuswr clients
> ownership (seems like they would belong to both groups?)

gpusrd and gpuswr are client IDs for GPU reads and writes on MC. GPU and
GPUB are SW group IDs for SMMU. There's no 1:1 or hierarchical mapping.

Terje

Thierry Reding Oct. 30, 2014, 2:56 p.m. UTC | #10

On Thu, Oct 30, 2014 at 03:47:59PM +0200, Terje Bergström wrote:
> On 30.10.2014 15:35, Alexandre Courbot wrote:
> > Great, thanks for confirming!
> > 
> > Thierry, how do you want to address this? We could change the register
> > for the GPU group, or (maybe preferable if we want to reflect the actual
> > hardware state) add the GPUB group. I don't know if that would be easy
> > though since we would have the problem of the gpusrd and gpuswr clients
> > ownership (seems like they would belong to both groups?)
> 
> gpusrd and gpuswr are client IDs for GPU reads and writes on MC. GPU and
> GPUB are SW group IDs for SMMU. There's no 1:1 or hierarchical mapping.

Since the GPU client ID is effectively useless for purposes of IOMMU
translation I'd lean towards just keeping the existing TEGRA_SWGROUP_GPU
and update the register to point to MC_SMMU_GPUB_ASID_0.

Thierry

Thierry Reding Oct. 30, 2014, 2:57 p.m. UTC | #11

On Mon, Oct 20, 2014 at 01:02:45PM +0200, Tomeu Vizoso wrote:
> On 13 October 2014 12:33, Thierry Reding <[e-mail]> wrote:
[...]
> > +struct clk *tegra_clk_register_mc(const char *name, const char *parent_name,
> > +                                 void __iomem *reg, spinlock_t *lock)
> > +{
> > +       return clk_register_divider_table(NULL, "mc", "emc_mux", 0, reg,
> 
> Should take name and parent_name from the passed args?
> 
> With that change:
> 
> Reviewed-By: Tomeu Vizoso <[e-mail]>

Good catch.

Thanks,
Thierry

Thierry Reding Oct. 30, 2014, 3:08 p.m. UTC | #12

On Wed, Oct 15, 2014 at 03:09:30PM -0700, Olof Johansson wrote:
> Hi,
> 
> Oh, a few more comments:
> 
> On Mon, Oct 13, 2014 at 3:33 AM, Thierry Reding
> <[e-mail]> wrote:
> 
> > diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
> > index c32d31981be3..1c932e7e7b8d 100644
> > --- a/drivers/memory/Makefile
> > +++ b/drivers/memory/Makefile
> > @@ -12,4 +12,5 @@ obj-$(CONFIG_FSL_CORENET_CF)  += fsl-corenet-cf.o
> >  obj-$(CONFIG_FSL_IFC)          += fsl_ifc.o
> >  obj-$(CONFIG_MVEBU_DEVBUS)     += mvebu-devbus.o
> >  obj-$(CONFIG_TEGRA20_MC)       += tegra20-mc.o
> > -obj-$(CONFIG_TEGRA30_MC)       += tegra30-mc.o
> > +
> > +obj-$(CONFIG_ARCH_TEGRA)       += tegra/
> > diff --git a/drivers/memory/tegra/Makefile b/drivers/memory/tegra/Makefile
> > new file mode 100644
> > index 000000000000..51b9e8fcde1b
> > --- /dev/null
> > +++ b/drivers/memory/tegra/Makefile
> > @@ -0,0 +1,5 @@
> > +obj-y                                   = tegra-mc.o
> > +obj-$(CONFIG_ARCH_TEGRA_3x_SOC)                += tegra30-mc.o
> > +obj-$(CONFIG_ARCH_TEGRA_114_SOC)       += tegra114-mc.o
> > +obj-$(CONFIG_ARCH_TEGRA_124_SOC)       += tegra124-mc.o
> > +obj-$(CONFIG_ARCH_TEGRA_132_SOC)       += tegra124-mc.o
> 
> You'll need a Kconfig and not just a makefile -- there are definitely
> dependencies on this driver (IOMMU in particular).

This is handled within the tegra-mc driver by only setting up the IOMMU
when TEGRA_IOMMU_SMMU is enabled. That config option remains in place.

> Also, the problem of having a global enable bit that is only under
> control of TrustZone FW is a big problem -- if the bit is not set, the
> driver will not work (and the machine will crash).
> 
> I think you'll need to come up with a way to detect that in the
> driver. I don't have a good idea of how it can be done though.

I don't think I ever got back to you on this. We discussed this
internally and it seems like there's no way to detect this properly, so
the best suggestion so far was to make it a requirement on the secure
firmware to enable IOMMU or not. Since there's no way for the kernel to
detect whether IOMMU was enabled or not, I think the firmware would
equally have to adjust the SMMU's device tree node's status property
appropriately.

Stephen, does that accurately reflect what we had discussed?

Thierry

Thierry Reding Oct. 30, 2014, 3:32 p.m. UTC | #13

On Wed, Oct 15, 2014 at 03:05:36PM -0700, Olof Johansson wrote:
> Hi,
> 
> On Mon, Oct 13, 2014 at 3:33 AM, Thierry Reding
> <[e-mail]> wrote:
> [...]
> > diff --git a/drivers/memory/tegra/tegra-mc.c b/drivers/memory/tegra/tegra-mc.c
> > new file mode 100644
> > index 000000000000..0f0c8be096d0
> > --- /dev/null
> > +++ b/drivers/memory/tegra/tegra-mc.c
> > @@ -0,0 +1,1061 @@
> > +/*
> > + * Copyright (C) 2014 NVIDIA CORPORATION.  All rights reserved.
> > + *
> > + * 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.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/iommu.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> 
> You need a linux/mm.h in here (on 64-bit).

Can you show what build error this fixes? I don't see any build failures
(after fixing up the obvious ones you pointed out below).

> > diff --git a/drivers/memory/tegra/tegra124-mc.c b/drivers/memory/tegra/tegra124-mc.c
> > new file mode 100644
> > index 000000000000..db31c96fc288
> > --- /dev/null
> > +++ b/drivers/memory/tegra/tegra124-mc.c
> 
> [...]
> 
> 
> > @@ -0,0 +1,1028 @@
> > +/*
> > + * Copyright (C) 2014 NVIDIA CORPORATION.  All rights reserved.
> > + *
> > + * 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.
> > + */
> > +
> > +#include <linux/of.h>
> > +#include <linux/mm.h>
> > +
> > +#include <asm/cacheflush.h>
> > +
> > +#include <dt-bindings/memory/tegra124-mc.h>
> > +
> > +#include "tegra-mc.h"
> > +
> > +static const struct tegra_mc_client tegra124_mc_clients[] = {
> > +       {
> > +               .id = 0x00,
> > +               .name = "ptcr",
> > +               .swgroup = TEGRA_SWGROUP_PTC,
> > +       }, {
> > +               .id = 0x01,
> > +               .name = "display0a",
> > +               .swgroup = TEGRA_SWGROUP_DC,
> > +               .smmu = {
> > +                       .reg = 0x228,
> > +                       .bit = 1,
> > +               },
> > +               .latency = {
> > +                       .reg = 0x2e8,
> > +                       .shift = 0,
> > +                       .mask = 0xff,
> > +                       .def = 0xc2,
> > +               },
> > +       }, {
> 
> These are very verbose tables. Having a macro for the initializers
> could help density a lot.

I've tried to use macros here, but I find that it hurts readability:

	...
	}, {
		TEGRA_MC_CLIENT(0x01, "display0a", TEGRA_SWGROUP_DC),
		TEGRA_MC_SMMU_ENABLE(0x228, 1),
		TEGRA_MC_LATENCY(0x2e8, 0, 0xff, 0xc2),
	}, {
	...

The original is more readable because it immediately gives you the
context, whereas with the macros you need to look up what the parameters
refer to.

> > +#ifdef CONFIG_ARCH_TEGRA_132_SOC
> > +static void tegra132_flush_dcache(struct page *page, unsigned long offset,
> > +                                 size_t size)
> > +{
> > +       void *virt = page_address(page) + offset;
> > +
> > +       __flush_dcache_area(virt, size);
> > +}
> > +
> > +static const struct tegra_smmu_ops tegra132_smmu_ops = {
> > +       .flush_dcache = tegra132_flush_dcache,
> > +};
> > +
> > +static const struct tegra_smmu_soc tegra132_smmu_soc = {
> > +       .groups = tegra124_smmu_groups,
> > +       .num_groups = ARRAY_SIZE(tegra124_smmu_groups),
> > +       .clients = tegra124_mc_clients,
> > +       .num_clients = ARRAY_SIZE(tegra124_mc_clients),
> > +       .swgroups = tegra124_swgroups,
> > +       .num_swgroups = ARRAY_SIZE(tegra124_swgroups),
> > +       .supports_round_robin_arbitration = true,
> > +       .supports_request_limit = true,
> > +       .num_address_bits = 34,
> > +       .num_asids = 128,
> > +       .ops = &tegra132_smmu_ops,
> > +};
> > +
> > +const struct tegra_mc_soc tegra132_mc_soc = {
> > +       .clients = tegra124_mc_clients,
> > +       .num_clients = ARRAY_SIZE(tegra124_mc_clients),
> > +       .atom_size = 32,
> > +       .smmu = &tegra132_smmu_soc,
> > +};
> > +#endif /* CONFIG_ARCH_TEGRA_132_SOC */
> 
> 
> This won't compile -- several of the tegra132_smmu_soc members are no
> longer valid. In particular:
> 
> groups
> num_groups

Fixed.

> supports_round_robin_arbitration
> supports_request_limit

In the version that I have these are still part of the tegra_smmu_soc
structure.

I've been thinking of extracting the Tegra132 changes into a separate
patch that can be applied once we have basic Tegra132 SoC support. It
feels wrong to merge Tegra132 SMMU support if there's no support in
arch/arm64 for the SoC yet. Though if nobody else thinks that's a
problem that's fine with me too.

Thierry

Thierry Reding Oct. 31, 2014, 1:27 p.m. UTC | #14

On Thu, Oct 30, 2014 at 04:08:41PM +0100, Thierry Reding wrote:
> On Wed, Oct 15, 2014 at 03:09:30PM -0700, Olof Johansson wrote:
> > Hi,
> > 
> > Oh, a few more comments:
> > 
> > On Mon, Oct 13, 2014 at 3:33 AM, Thierry Reding
> > <[e-mail]> wrote:
> > 
> > > diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
> > > index c32d31981be3..1c932e7e7b8d 100644
> > > --- a/drivers/memory/Makefile
> > > +++ b/drivers/memory/Makefile
> > > @@ -12,4 +12,5 @@ obj-$(CONFIG_FSL_CORENET_CF)  += fsl-corenet-cf.o
> > >  obj-$(CONFIG_FSL_IFC)          += fsl_ifc.o
> > >  obj-$(CONFIG_MVEBU_DEVBUS)     += mvebu-devbus.o
> > >  obj-$(CONFIG_TEGRA20_MC)       += tegra20-mc.o
> > > -obj-$(CONFIG_TEGRA30_MC)       += tegra30-mc.o
> > > +
> > > +obj-$(CONFIG_ARCH_TEGRA)       += tegra/
> > > diff --git a/drivers/memory/tegra/Makefile b/drivers/memory/tegra/Makefile
> > > new file mode 100644
> > > index 000000000000..51b9e8fcde1b
> > > --- /dev/null
> > > +++ b/drivers/memory/tegra/Makefile
> > > @@ -0,0 +1,5 @@
> > > +obj-y                                   = tegra-mc.o
> > > +obj-$(CONFIG_ARCH_TEGRA_3x_SOC)                += tegra30-mc.o
> > > +obj-$(CONFIG_ARCH_TEGRA_114_SOC)       += tegra114-mc.o
> > > +obj-$(CONFIG_ARCH_TEGRA_124_SOC)       += tegra124-mc.o
> > > +obj-$(CONFIG_ARCH_TEGRA_132_SOC)       += tegra124-mc.o
> > 
> > You'll need a Kconfig and not just a makefile -- there are definitely
> > dependencies on this driver (IOMMU in particular).
> 
> This is handled within the tegra-mc driver by only setting up the IOMMU
> when TEGRA_IOMMU_SMMU is enabled. That config option remains in place.
> 
> > Also, the problem of having a global enable bit that is only under
> > control of TrustZone FW is a big problem -- if the bit is not set, the
> > driver will not work (and the machine will crash).
> > 
> > I think you'll need to come up with a way to detect that in the
> > driver. I don't have a good idea of how it can be done though.
> 
> I don't think I ever got back to you on this. We discussed this
> internally and it seems like there's no way to detect this properly, so
> the best suggestion so far was to make it a requirement on the secure
> firmware to enable IOMMU or not. Since there's no way for the kernel to
> detect whether IOMMU was enabled or not, I think the firmware would
> equally have to adjust the SMMU's device tree node's status property
> appropriately.

The other option would be for the firmware not to touch the SMMU device
tree node and the kernel simply assuming that if it's running in non-
secure mode then there must be secure firmware and it has enabled the
SMMU. Enabling the SMMU would become part of the contract between
firmware and kernel, much like locking the VPR is required to get the
GPU to work.

Those are really the only two choices we have.

Thierry

Alexandre Courbot Nov. 1, 2014, 5:38 a.m. UTC | #15

On Fri, Oct 31, 2014 at 10:27 PM, Thierry Reding
<[e-mail]> wrote:
> On Thu, Oct 30, 2014 at 04:08:41PM +0100, Thierry Reding wrote:
>> On Wed, Oct 15, 2014 at 03:09:30PM -0700, Olof Johansson wrote:
>> > Hi,
>> >
>> > Oh, a few more comments:
>> >
>> > On Mon, Oct 13, 2014 at 3:33 AM, Thierry Reding
>> > <[e-mail]> wrote:
>> >
>> > > diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
>> > > index c32d31981be3..1c932e7e7b8d 100644
>> > > --- a/drivers/memory/Makefile
>> > > +++ b/drivers/memory/Makefile
>> > > @@ -12,4 +12,5 @@ obj-$(CONFIG_FSL_CORENET_CF)  += fsl-corenet-cf.o
>> > >  obj-$(CONFIG_FSL_IFC)          += fsl_ifc.o
>> > >  obj-$(CONFIG_MVEBU_DEVBUS)     += mvebu-devbus.o
>> > >  obj-$(CONFIG_TEGRA20_MC)       += tegra20-mc.o
>> > > -obj-$(CONFIG_TEGRA30_MC)       += tegra30-mc.o
>> > > +
>> > > +obj-$(CONFIG_ARCH_TEGRA)       += tegra/
>> > > diff --git a/drivers/memory/tegra/Makefile b/drivers/memory/tegra/Makefile
>> > > new file mode 100644
>> > > index 000000000000..51b9e8fcde1b
>> > > --- /dev/null
>> > > +++ b/drivers/memory/tegra/Makefile
>> > > @@ -0,0 +1,5 @@
>> > > +obj-y                                   = tegra-mc.o
>> > > +obj-$(CONFIG_ARCH_TEGRA_3x_SOC)                += tegra30-mc.o
>> > > +obj-$(CONFIG_ARCH_TEGRA_114_SOC)       += tegra114-mc.o
>> > > +obj-$(CONFIG_ARCH_TEGRA_124_SOC)       += tegra124-mc.o
>> > > +obj-$(CONFIG_ARCH_TEGRA_132_SOC)       += tegra124-mc.o
>> >
>> > You'll need a Kconfig and not just a makefile -- there are definitely
>> > dependencies on this driver (IOMMU in particular).
>>
>> This is handled within the tegra-mc driver by only setting up the IOMMU
>> when TEGRA_IOMMU_SMMU is enabled. That config option remains in place.
>>
>> > Also, the problem of having a global enable bit that is only under
>> > control of TrustZone FW is a big problem -- if the bit is not set, the
>> > driver will not work (and the machine will crash).
>> >
>> > I think you'll need to come up with a way to detect that in the
>> > driver. I don't have a good idea of how it can be done though.
>>
>> I don't think I ever got back to you on this. We discussed this
>> internally and it seems like there's no way to detect this properly, so
>> the best suggestion so far was to make it a requirement on the secure
>> firmware to enable IOMMU or not. Since there's no way for the kernel to
>> detect whether IOMMU was enabled or not, I think the firmware would
>> equally have to adjust the SMMU's device tree node's status property
>> appropriately.
>
> The other option would be for the firmware not to touch the SMMU device
> tree node and the kernel simply assuming that if it's running in non-
> secure mode then there must be secure firmware and it has enabled the
> SMMU. Enabling the SMMU would become part of the contract between
> firmware and kernel, much like locking the VPR is required to get the
> GPU to work.
>
> Those are really the only two choices we have.

We got the exact same problem with GPU and VPR registers, and it seems
like the approach we will be taking here is to have the
firmware/bootloader do whatever is needed to get the GPU working and
enable the DT node once it did. IOW, the kernel will never touch
protected registers, and will not freeze if the hardware is not
properly set up.

It would be nice for consistency if the same approach can be taken
with the IOMMU. OTOH we will need to make sure that all these
initialization contracts are clearly documented somewhere. Maybe a
comment in the DTS to explain what is expected from the firmware to
enable such nodes would be a good idea, too.

Thierry Reding Nov. 3, 2014, 8:22 a.m. UTC | #16

On Sat, Nov 01, 2014 at 02:38:26PM +0900, Alexandre Courbot wrote:
> On Fri, Oct 31, 2014 at 10:27 PM, Thierry Reding
> <[e-mail]> wrote:
> > On Thu, Oct 30, 2014 at 04:08:41PM +0100, Thierry Reding wrote:
> >> On Wed, Oct 15, 2014 at 03:09:30PM -0700, Olof Johansson wrote:
> >> > Hi,
> >> >
> >> > Oh, a few more comments:
> >> >
> >> > On Mon, Oct 13, 2014 at 3:33 AM, Thierry Reding
> >> > <[e-mail]> wrote:
> >> >
> >> > > diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
> >> > > index c32d31981be3..1c932e7e7b8d 100644
> >> > > --- a/drivers/memory/Makefile
> >> > > +++ b/drivers/memory/Makefile
> >> > > @@ -12,4 +12,5 @@ obj-$(CONFIG_FSL_CORENET_CF)  += fsl-corenet-cf.o
> >> > >  obj-$(CONFIG_FSL_IFC)          += fsl_ifc.o
> >> > >  obj-$(CONFIG_MVEBU_DEVBUS)     += mvebu-devbus.o
> >> > >  obj-$(CONFIG_TEGRA20_MC)       += tegra20-mc.o
> >> > > -obj-$(CONFIG_TEGRA30_MC)       += tegra30-mc.o
> >> > > +
> >> > > +obj-$(CONFIG_ARCH_TEGRA)       += tegra/
> >> > > diff --git a/drivers/memory/tegra/Makefile b/drivers/memory/tegra/Makefile
> >> > > new file mode 100644
> >> > > index 000000000000..51b9e8fcde1b
> >> > > --- /dev/null
> >> > > +++ b/drivers/memory/tegra/Makefile
> >> > > @@ -0,0 +1,5 @@
> >> > > +obj-y                                   = tegra-mc.o
> >> > > +obj-$(CONFIG_ARCH_TEGRA_3x_SOC)                += tegra30-mc.o
> >> > > +obj-$(CONFIG_ARCH_TEGRA_114_SOC)       += tegra114-mc.o
> >> > > +obj-$(CONFIG_ARCH_TEGRA_124_SOC)       += tegra124-mc.o
> >> > > +obj-$(CONFIG_ARCH_TEGRA_132_SOC)       += tegra124-mc.o
> >> >
> >> > You'll need a Kconfig and not just a makefile -- there are definitely
> >> > dependencies on this driver (IOMMU in particular).
> >>
> >> This is handled within the tegra-mc driver by only setting up the IOMMU
> >> when TEGRA_IOMMU_SMMU is enabled. That config option remains in place.
> >>
> >> > Also, the problem of having a global enable bit that is only under
> >> > control of TrustZone FW is a big problem -- if the bit is not set, the
> >> > driver will not work (and the machine will crash).
> >> >
> >> > I think you'll need to come up with a way to detect that in the
> >> > driver. I don't have a good idea of how it can be done though.
> >>
> >> I don't think I ever got back to you on this. We discussed this
> >> internally and it seems like there's no way to detect this properly, so
> >> the best suggestion so far was to make it a requirement on the secure
> >> firmware to enable IOMMU or not. Since there's no way for the kernel to
> >> detect whether IOMMU was enabled or not, I think the firmware would
> >> equally have to adjust the SMMU's device tree node's status property
> >> appropriately.
> >
> > The other option would be for the firmware not to touch the SMMU device
> > tree node and the kernel simply assuming that if it's running in non-
> > secure mode then there must be secure firmware and it has enabled the
> > SMMU. Enabling the SMMU would become part of the contract between
> > firmware and kernel, much like locking the VPR is required to get the
> > GPU to work.
> >
> > Those are really the only two choices we have.
> 
> We got the exact same problem with GPU and VPR registers, and it seems
> like the approach we will be taking here is to have the
> firmware/bootloader do whatever is needed to get the GPU working and
> enable the DT node once it did. IOW, the kernel will never touch
> protected registers, and will not freeze if the hardware is not
> properly set up.

I think the situation is slightly different. For the SMMU we still have
the option to enable translations when running in secure mode with code
that's pretty trivial to have in the IOMMU driver (it's just a single
bit that gets written to a register). And when the IOMMU driver does
that everything will work just fine.

So I'm thinking that a workable alternative to what we've done for VPR
would be to just always enable translations in the SMMU driver (that
operation will simply be discarded in non-secure mode) and assume that
it'll work. So the contract would be that running in secure mode the
kernel sets everything up and when running in non-secure mode the kernel
will assume that firmware set everything up already.

Requiring firmware to change the device node's status to "okay" seems
rather restrictive since it would have to do that even if it boots the
kernel in secure mode.

> It would be nice for consistency if the same approach can be taken
> with the IOMMU. OTOH we will need to make sure that all these
> initialization contracts are clearly documented somewhere. Maybe a
> comment in the DTS to explain what is expected from the firmware to
> enable such nodes would be a good idea, too.

The device tree binding seems like a good place to document this.

Thierry

Alexandre Courbot Nov. 3, 2014, 8:40 a.m. UTC | #17

On Mon, Nov 3, 2014 at 5:22 PM, Thierry Reding <[e-mail]> wrote:
> On Sat, Nov 01, 2014 at 02:38:26PM +0900, Alexandre Courbot wrote:
>> On Fri, Oct 31, 2014 at 10:27 PM, Thierry Reding
>> <[e-mail]> wrote:
>> > On Thu, Oct 30, 2014 at 04:08:41PM +0100, Thierry Reding wrote:
>> >> On Wed, Oct 15, 2014 at 03:09:30PM -0700, Olof Johansson wrote:
>> >> > Hi,
>> >> >
>> >> > Oh, a few more comments:
>> >> >
>> >> > On Mon, Oct 13, 2014 at 3:33 AM, Thierry Reding
>> >> > <[e-mail]> wrote:
>> >> >
>> >> > > diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
>> >> > > index c32d31981be3..1c932e7e7b8d 100644
>> >> > > --- a/drivers/memory/Makefile
>> >> > > +++ b/drivers/memory/Makefile
>> >> > > @@ -12,4 +12,5 @@ obj-$(CONFIG_FSL_CORENET_CF)  += fsl-corenet-cf.o
>> >> > >  obj-$(CONFIG_FSL_IFC)          += fsl_ifc.o
>> >> > >  obj-$(CONFIG_MVEBU_DEVBUS)     += mvebu-devbus.o
>> >> > >  obj-$(CONFIG_TEGRA20_MC)       += tegra20-mc.o
>> >> > > -obj-$(CONFIG_TEGRA30_MC)       += tegra30-mc.o
>> >> > > +
>> >> > > +obj-$(CONFIG_ARCH_TEGRA)       += tegra/
>> >> > > diff --git a/drivers/memory/tegra/Makefile b/drivers/memory/tegra/Makefile
>> >> > > new file mode 100644
>> >> > > index 000000000000..51b9e8fcde1b
>> >> > > --- /dev/null
>> >> > > +++ b/drivers/memory/tegra/Makefile
>> >> > > @@ -0,0 +1,5 @@
>> >> > > +obj-y                                   = tegra-mc.o
>> >> > > +obj-$(CONFIG_ARCH_TEGRA_3x_SOC)                += tegra30-mc.o
>> >> > > +obj-$(CONFIG_ARCH_TEGRA_114_SOC)       += tegra114-mc.o
>> >> > > +obj-$(CONFIG_ARCH_TEGRA_124_SOC)       += tegra124-mc.o
>> >> > > +obj-$(CONFIG_ARCH_TEGRA_132_SOC)       += tegra124-mc.o
>> >> >
>> >> > You'll need a Kconfig and not just a makefile -- there are definitely
>> >> > dependencies on this driver (IOMMU in particular).
>> >>
>> >> This is handled within the tegra-mc driver by only setting up the IOMMU
>> >> when TEGRA_IOMMU_SMMU is enabled. That config option remains in place.
>> >>
>> >> > Also, the problem of having a global enable bit that is only under
>> >> > control of TrustZone FW is a big problem -- if the bit is not set, the
>> >> > driver will not work (and the machine will crash).
>> >> >
>> >> > I think you'll need to come up with a way to detect that in the
>> >> > driver. I don't have a good idea of how it can be done though.
>> >>
>> >> I don't think I ever got back to you on this. We discussed this
>> >> internally and it seems like there's no way to detect this properly, so
>> >> the best suggestion so far was to make it a requirement on the secure
>> >> firmware to enable IOMMU or not. Since there's no way for the kernel to
>> >> detect whether IOMMU was enabled or not, I think the firmware would
>> >> equally have to adjust the SMMU's device tree node's status property
>> >> appropriately.
>> >
>> > The other option would be for the firmware not to touch the SMMU device
>> > tree node and the kernel simply assuming that if it's running in non-
>> > secure mode then there must be secure firmware and it has enabled the
>> > SMMU. Enabling the SMMU would become part of the contract between
>> > firmware and kernel, much like locking the VPR is required to get the
>> > GPU to work.
>> >
>> > Those are really the only two choices we have.
>>
>> We got the exact same problem with GPU and VPR registers, and it seems
>> like the approach we will be taking here is to have the
>> firmware/bootloader do whatever is needed to get the GPU working and
>> enable the DT node once it did. IOW, the kernel will never touch
>> protected registers, and will not freeze if the hardware is not
>> properly set up.
>
> I think the situation is slightly different. For the SMMU we still have
> the option to enable translations when running in secure mode with code
> that's pretty trivial to have in the IOMMU driver (it's just a single
> bit that gets written to a register). And when the IOMMU driver does
> that everything will work just fine.
>
> So I'm thinking that a workable alternative to what we've done for VPR
> would be to just always enable translations in the SMMU driver (that
> operation will simply be discarded in non-secure mode) and assume that
> it'll work. So the contract would be that running in secure mode the
> kernel sets everything up and when running in non-secure mode the kernel
> will assume that firmware set everything up already.
>
> Requiring firmware to change the device node's status to "okay" seems
> rather restrictive since it would have to do that even if it boots the
> kernel in secure mode.

Sounds good to me, as long as the kernel can always run using the same
code in both modes.

>
>> It would be nice for consistency if the same approach can be taken
>> with the IOMMU. OTOH we will need to make sure that all these
>> initialization contracts are clearly documented somewhere. Maybe a
>> comment in the DTS to explain what is expected from the firmware to
>> enable such nodes would be a good idea, too.
>
> The device tree binding seems like a good place to document this.

Indeed.

Patch

diff --git a/drivers/clk/tegra/clk-divider.c b/drivers/clk/tegra/clk-divider.c
index 290f9c1a3749..84e1b3c1fb2a 100644
--- a/drivers/clk/tegra/clk-divider.c
+++ b/drivers/clk/tegra/clk-divider.c
@@ -185,3 +185,16 @@  struct clk *tegra_clk_register_divider(const char *name,
 
 	return clk;
 }
+
+static const struct clk_div_table mc_div_table[] = {
+	{ .val = 0, .div = 2 },
+	{ .val = 1, .div = 1 },
+	{ .val = 0, .div = 0 },
+};
+
+struct clk *tegra_clk_register_mc(const char *name, const char *parent_name,
+				  void __iomem *reg, spinlock_t *lock)
+{
+	return clk_register_divider_table(NULL, "mc", "emc_mux", 0, reg,
+					  16, 1, 0, mc_div_table, lock);
+}
diff --git a/drivers/clk/tegra/clk-tegra114.c b/drivers/clk/tegra/clk-tegra114.c
index f760f31d05c4..0b03d2cf7264 100644
--- a/drivers/clk/tegra/clk-tegra114.c
+++ b/drivers/clk/tegra/clk-tegra114.c
@@ -173,6 +173,7 @@  static DEFINE_SPINLOCK(pll_d_lock);
 static DEFINE_SPINLOCK(pll_d2_lock);
 static DEFINE_SPINLOCK(pll_u_lock);
 static DEFINE_SPINLOCK(pll_re_lock);
+static DEFINE_SPINLOCK(emc_lock);
 
 static struct div_nmp pllxc_nmp = {
 	.divm_shift = 0,
@@ -1228,7 +1229,11 @@  static __init void tegra114_periph_clk_init(void __iomem *clk_base,
 			       ARRAY_SIZE(mux_pllmcp_clkm),
 			       CLK_SET_RATE_NO_REPARENT,
 			       clk_base + CLK_SOURCE_EMC,
-			       29, 3, 0, NULL);
+			       29, 3, 0, &emc_lock);
+
+	clk = tegra_clk_register_mc("mc", "emc_mux", clk_base + CLK_SOURCE_EMC,
+				    &emc_lock);
+	clks[TEGRA114_CLK_MC] = clk;
 
 	for (i = 0; i < ARRAY_SIZE(tegra_periph_clk_list); i++) {
 		data = &tegra_periph_clk_list[i];
diff --git a/drivers/clk/tegra/clk-tegra124.c b/drivers/clk/tegra/clk-tegra124.c
index e3a85842ce0c..f5f9baca7bb6 100644
--- a/drivers/clk/tegra/clk-tegra124.c
+++ b/drivers/clk/tegra/clk-tegra124.c
@@ -132,6 +132,7 @@  static DEFINE_SPINLOCK(pll_d2_lock);
 static DEFINE_SPINLOCK(pll_e_lock);
 static DEFINE_SPINLOCK(pll_re_lock);
 static DEFINE_SPINLOCK(pll_u_lock);
+static DEFINE_SPINLOCK(emc_lock);
 
 /* possible OSC frequencies in Hz */
 static unsigned long tegra124_input_freq[] = {
@@ -1127,7 +1128,11 @@  static __init void tegra124_periph_clk_init(void __iomem *clk_base,
 	clk = clk_register_mux(NULL, "emc_mux", mux_pllmcp_clkm,
 			       ARRAY_SIZE(mux_pllmcp_clkm), 0,
 			       clk_base + CLK_SOURCE_EMC,
-			       29, 3, 0, NULL);
+			       29, 3, 0, &emc_lock);
+
+	clk = tegra_clk_register_mc("mc", "emc_mux", clk_base + CLK_SOURCE_EMC,
+				    &emc_lock);
+	clks[TEGRA124_CLK_MC] = clk;
 
 	/* cml0 */
 	clk = clk_register_gate(NULL, "cml0", "pll_e", 0, clk_base + PLLE_AUX,
diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
index dace2b1b5ae6..41272dcc9e22 100644
--- a/drivers/clk/tegra/clk-tegra20.c
+++ b/drivers/clk/tegra/clk-tegra20.c
@@ -140,6 +140,8 @@  static struct cpu_clk_suspend_context {
 static void __iomem *clk_base;
 static void __iomem *pmc_base;
 
+static DEFINE_SPINLOCK(emc_lock);
+
 #define TEGRA_INIT_DATA_MUX(_name, _parents, _offset,	\
 			    _clk_num, _gate_flags, _clk_id)	\
 	TEGRA_INIT_DATA(_name, NULL, NULL, _parents, _offset,	\
@@ -819,11 +821,15 @@  static void __init tegra20_periph_clk_init(void)
 			       ARRAY_SIZE(mux_pllmcp_clkm),
 			       CLK_SET_RATE_NO_REPARENT,
 			       clk_base + CLK_SOURCE_EMC,
-			       30, 2, 0, NULL);
+			       30, 2, 0, &emc_lock);
 	clk = tegra_clk_register_periph_gate("emc", "emc_mux", 0, clk_base, 0,
 				    57, periph_clk_enb_refcnt);
 	clks[TEGRA20_CLK_EMC] = clk;
 
+	clk = tegra_clk_register_mc("mc", "emc_mux", clk_base + CLK_SOURCE_EMC,
+				    &emc_lock);
+	clks[TEGRA20_CLK_MC] = clk;
+
 	/* dsi */
 	clk = tegra_clk_register_periph_gate("dsi", "pll_d", 0, clk_base, 0,
 				    48, periph_clk_enb_refcnt);
diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c
index 5bbacd01094f..4b9d8bd3d0bf 100644
--- a/drivers/clk/tegra/clk-tegra30.c
+++ b/drivers/clk/tegra/clk-tegra30.c
@@ -177,6 +177,7 @@  static unsigned long input_freq;
 
 static DEFINE_SPINLOCK(cml_lock);
 static DEFINE_SPINLOCK(pll_d_lock);
+static DEFINE_SPINLOCK(emc_lock);
 
 #define TEGRA_INIT_DATA_MUX(_name, _parents, _offset,	\
 			    _clk_num, _gate_flags, _clk_id)	\
@@ -1157,11 +1158,15 @@  static void __init tegra30_periph_clk_init(void)
 			       ARRAY_SIZE(mux_pllmcp_clkm),
 			       CLK_SET_RATE_NO_REPARENT,
 			       clk_base + CLK_SOURCE_EMC,
-			       30, 2, 0, NULL);
+			       30, 2, 0, &emc_lock);
 	clk = tegra_clk_register_periph_gate("emc", "emc_mux", 0, clk_base, 0,
 				    57, periph_clk_enb_refcnt);
 	clks[TEGRA30_CLK_EMC] = clk;
 
+	clk = tegra_clk_register_mc("mc", "emc_mux", clk_base + CLK_SOURCE_EMC,
+				    &emc_lock);
+	clks[TEGRA30_CLK_MC] = clk;
+
 	/* cml0 */
 	clk = clk_register_gate(NULL, "cml0", "pll_e", 0, clk_base + PLLE_AUX,
 				0, 0, &cml_lock);
diff --git a/drivers/clk/tegra/clk.h b/drivers/clk/tegra/clk.h
index 16ec8d6bb87f..4e458aa8d45c 100644
--- a/drivers/clk/tegra/clk.h
+++ b/drivers/clk/tegra/clk.h
@@ -86,6 +86,8 @@  struct clk *tegra_clk_register_divider(const char *name,
 		const char *parent_name, void __iomem *reg,
 		unsigned long flags, u8 clk_divider_flags, u8 shift, u8 width,
 		u8 frac_width, spinlock_t *lock);
+struct clk *tegra_clk_register_mc(const char *name, const char *parent_name,
+				  void __iomem *reg, spinlock_t *lock);
 
 /*
  * Tegra PLL:
diff --git a/include/dt-bindings/clock/tegra114-car.h b/include/dt-bindings/clock/tegra114-car.h
index fc12621fb432..534c03f8ad72 100644
--- a/include/dt-bindings/clock/tegra114-car.h
+++ b/include/dt-bindings/clock/tegra114-car.h
@@ -49,7 +49,7 @@ 
 #define TEGRA114_CLK_I2S0 30
 /* 31 */
 
-/* 32 */
+#define TEGRA114_CLK_MC 32
 /* 33 */
 #define TEGRA114_CLK_APBDMA 34
 /* 35 */
diff --git a/include/dt-bindings/clock/tegra124-car.h b/include/dt-bindings/clock/tegra124-car.h
index 6bac637fd635..af9bc9a3ddbc 100644
--- a/include/dt-bindings/clock/tegra124-car.h
+++ b/include/dt-bindings/clock/tegra124-car.h
@@ -48,7 +48,7 @@ 
 #define TEGRA124_CLK_I2S0 30
 /* 31 */
 
-/* 32 */
+#define TEGRA124_CLK_MC 32
 /* 33 */
 #define TEGRA124_CLK_APBDMA 34
 /* 35 */
diff --git a/include/dt-bindings/clock/tegra20-car.h b/include/dt-bindings/clock/tegra20-car.h
index 9406207cfac8..04500b243a4d 100644
--- a/include/dt-bindings/clock/tegra20-car.h
+++ b/include/dt-bindings/clock/tegra20-car.h
@@ -49,7 +49,7 @@ 
 /* 30 */
 #define TEGRA20_CLK_CACHE2 31
 
-#define TEGRA20_CLK_MEM 32
+#define TEGRA20_CLK_MC 32
 #define TEGRA20_CLK_AHBDMA 33
 #define TEGRA20_CLK_APBDMA 34
 /* 35 */