| Message ID | 20210325130332.778208-2-thierry.reding@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | arm64: tegra: Prevent early SMMU faults | expand |
25.03.2021 16:03, Thierry Reding пишет: > From: Thierry Reding <treding@nvidia.com> > > From Tegra20 through Tegra210, either the GART or SMMU drivers need > access to the internals of the memory controller driver because they are > tightly coupled (in fact, the GART and SMMU are part of the memory > controller). On later chips, a separate hardware block implements the > SMMU functionality, so this is no longer needed. However, we still want > to reuse some of the existing infrastructure on later chips, so split > the memory controller internals into a separate header file to avoid > conflicts with the implementation on newer chips. > > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > drivers/iommu/tegra-gart.c | 2 +- > drivers/iommu/tegra-smmu.c | 2 +- > drivers/memory/tegra/mc.h | 2 +- > drivers/memory/tegra/tegra186.c | 12 ++++--- > include/soc/tegra/mc-internal.h | 62 +++++++++++++++++++++++++++++++++ > include/soc/tegra/mc.h | 50 -------------------------- > 6 files changed, 72 insertions(+), 58 deletions(-) > create mode 100644 include/soc/tegra/mc-internal.h What about to make T186 to re-use the existing tegra_mc struct? Seems there is nothing special in that struct which doesn't fit for the newer SoCs. Please notice that both SMMU and GART are already optional and all the SoC differences are specified within the tegra_mc_soc. It looks to me that this could be a much nicer and cleaner variant.
On Thu, Mar 25, 2021 at 06:12:51PM +0300, Dmitry Osipenko wrote: > 25.03.2021 16:03, Thierry Reding пишет: > > From: Thierry Reding <treding@nvidia.com> > > > > From Tegra20 through Tegra210, either the GART or SMMU drivers need > > access to the internals of the memory controller driver because they are > > tightly coupled (in fact, the GART and SMMU are part of the memory > > controller). On later chips, a separate hardware block implements the > > SMMU functionality, so this is no longer needed. However, we still want > > to reuse some of the existing infrastructure on later chips, so split > > the memory controller internals into a separate header file to avoid > > conflicts with the implementation on newer chips. > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > --- > > drivers/iommu/tegra-gart.c | 2 +- > > drivers/iommu/tegra-smmu.c | 2 +- > > drivers/memory/tegra/mc.h | 2 +- > > drivers/memory/tegra/tegra186.c | 12 ++++--- > > include/soc/tegra/mc-internal.h | 62 +++++++++++++++++++++++++++++++++ > > include/soc/tegra/mc.h | 50 -------------------------- > > 6 files changed, 72 insertions(+), 58 deletions(-) > > create mode 100644 include/soc/tegra/mc-internal.h > > What about to make T186 to re-use the existing tegra_mc struct? Seems > there is nothing special in that struct which doesn't fit for the newer > SoCs. Please notice that both SMMU and GART are already optional and all > the SoC differences are specified within the tegra_mc_soc. It looks to > me that this could be a much nicer and cleaner variant. The problem is that much of the interesting bits in tegra_mc_soc are basically incompatible between the two. For instance the tegra_mc_client and tegra186_mc_client structures, while they have the same purpose, have completely different content. I didn't see a way to unify that without overly complicating things by making half of the fields basically optional on one or the other SoC generation. Maybe one option would be to split tegra_mc into a tegra_mc_common and then derive tegra_mc and tegra186_mc from that. That way we could share the common bits while still letting the chip-specific differences be handled separately. Thierry
25.03.2021 18:52, Thierry Reding пишет: > On Thu, Mar 25, 2021 at 06:12:51PM +0300, Dmitry Osipenko wrote: >> 25.03.2021 16:03, Thierry Reding пишет: >>> From: Thierry Reding <treding@nvidia.com> >>> >>> From Tegra20 through Tegra210, either the GART or SMMU drivers need >>> access to the internals of the memory controller driver because they are >>> tightly coupled (in fact, the GART and SMMU are part of the memory >>> controller). On later chips, a separate hardware block implements the >>> SMMU functionality, so this is no longer needed. However, we still want >>> to reuse some of the existing infrastructure on later chips, so split >>> the memory controller internals into a separate header file to avoid >>> conflicts with the implementation on newer chips. >>> >>> Signed-off-by: Thierry Reding <treding@nvidia.com> >>> --- >>> drivers/iommu/tegra-gart.c | 2 +- >>> drivers/iommu/tegra-smmu.c | 2 +- >>> drivers/memory/tegra/mc.h | 2 +- >>> drivers/memory/tegra/tegra186.c | 12 ++++--- >>> include/soc/tegra/mc-internal.h | 62 +++++++++++++++++++++++++++++++++ >>> include/soc/tegra/mc.h | 50 -------------------------- >>> 6 files changed, 72 insertions(+), 58 deletions(-) >>> create mode 100644 include/soc/tegra/mc-internal.h >> >> What about to make T186 to re-use the existing tegra_mc struct? Seems >> there is nothing special in that struct which doesn't fit for the newer >> SoCs. Please notice that both SMMU and GART are already optional and all >> the SoC differences are specified within the tegra_mc_soc. It looks to >> me that this could be a much nicer and cleaner variant. > > The problem is that much of the interesting bits in tegra_mc_soc are > basically incompatible between the two. For instance the tegra_mc_client > and tegra186_mc_client structures, while they have the same purpose, > have completely different content. I didn't see a way to unify that > without overly complicating things by making half of the fields > basically optional on one or the other SoC generation. The additional fields aren't problem for T20, which doesn't need most of the fields. I'd try to go with the additional fields for now and see how it will look like, if it will be bothering too much, then we may consider to refactor the drivers more thoroughly (later on, in a separate series), with a better/nicer separation and taking into account a potential modularization support by the MC drivers. Using a union for the exclusive fields also could work, although always need to be extra careful with the unions. > Maybe one option would be to split tegra_mc into a tegra_mc_common and > then derive tegra_mc and tegra186_mc from that. That way we could share > the common bits while still letting the chip-specific differences be > handled separately. But isn't tegra_mc already a superset of tegra186_mc? I think the tegra186_mc_client is the main difference here.
25.03.2021 19:11, Dmitry Osipenko пишет: > 25.03.2021 18:52, Thierry Reding пишет: >> On Thu, Mar 25, 2021 at 06:12:51PM +0300, Dmitry Osipenko wrote: >>> 25.03.2021 16:03, Thierry Reding пишет: >>>> From: Thierry Reding <treding@nvidia.com> >>>> >>>> From Tegra20 through Tegra210, either the GART or SMMU drivers need >>>> access to the internals of the memory controller driver because they are >>>> tightly coupled (in fact, the GART and SMMU are part of the memory >>>> controller). On later chips, a separate hardware block implements the >>>> SMMU functionality, so this is no longer needed. However, we still want >>>> to reuse some of the existing infrastructure on later chips, so split >>>> the memory controller internals into a separate header file to avoid >>>> conflicts with the implementation on newer chips. >>>> >>>> Signed-off-by: Thierry Reding <treding@nvidia.com> >>>> --- >>>> drivers/iommu/tegra-gart.c | 2 +- >>>> drivers/iommu/tegra-smmu.c | 2 +- >>>> drivers/memory/tegra/mc.h | 2 +- >>>> drivers/memory/tegra/tegra186.c | 12 ++++--- >>>> include/soc/tegra/mc-internal.h | 62 +++++++++++++++++++++++++++++++++ >>>> include/soc/tegra/mc.h | 50 -------------------------- >>>> 6 files changed, 72 insertions(+), 58 deletions(-) >>>> create mode 100644 include/soc/tegra/mc-internal.h >>> >>> What about to make T186 to re-use the existing tegra_mc struct? Seems >>> there is nothing special in that struct which doesn't fit for the newer >>> SoCs. Please notice that both SMMU and GART are already optional and all >>> the SoC differences are specified within the tegra_mc_soc. It looks to >>> me that this could be a much nicer and cleaner variant. >> >> The problem is that much of the interesting bits in tegra_mc_soc are >> basically incompatible between the two. For instance the tegra_mc_client >> and tegra186_mc_client structures, while they have the same purpose, >> have completely different content. I didn't see a way to unify that >> without overly complicating things by making half of the fields >> basically optional on one or the other SoC generation. > > The additional fields aren't problem for T20, which doesn't need most of > the fields. I'd try to go with the additional fields for now and see how > it will look like, if it will be bothering too much, then we may > consider to refactor the drivers more thoroughly (later on, in a > separate series), with a better/nicer separation and taking into account > a potential modularization support by the MC drivers. > > Using a union for the exclusive fields also could work, although always > need to be extra careful with the unions. > >> Maybe one option would be to split tegra_mc into a tegra_mc_common and >> then derive tegra_mc and tegra186_mc from that. That way we could share >> the common bits while still letting the chip-specific differences be >> handled separately. > > But isn't tegra_mc already a superset of tegra186_mc? I think the > tegra186_mc_client is the main difference here. > Another thing we could do is to optimize the size of tegra_mc_client, but not sure whether it's worthwhile to care about extra ~3KB of data. This slims down tegra_mc_client by two times: diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c index edea9b2b406e..1d652bfc6b44 100644 --- a/drivers/memory/tegra/mc.c +++ b/drivers/memory/tegra/mc.c @@ -317,11 +317,11 @@ static int tegra_mc_setup_latency_allowance(struct tegra_mc *mc) /* write latency allowance defaults */ for (i = 0; i < mc->soc->num_clients; i++) { const struct tegra_mc_la *la = &mc->soc->clients[i].la; - u32 value; + u32 value, la_mask = la->mask, la_def = la->def; value = mc_readl(mc, la->reg); - value &= ~(la->mask << la->shift); - value |= (la->def & la->mask) << la->shift; + value &= ~(la_mask << la->shift); + value |= (la_def & la_mask) << la->shift; mc_writel(mc, value, la->reg); } diff --git a/drivers/memory/tegra/tegra30.c b/drivers/memory/tegra/tegra30.c index 46332fa82d10..ecf05484d656 100644 --- a/drivers/memory/tegra/tegra30.c +++ b/drivers/memory/tegra/tegra30.c @@ -1157,7 +1157,7 @@ static void tegra30_mc_tune_client_latency(struct tegra_mc *mc, u32 arb_tolerance_compensation_nsec, arb_tolerance_compensation_div; const struct tegra_mc_la *la = &client->la; unsigned int fifo_size = client->fifo_size; - u32 arb_nsec, la_ticks, value; + u32 arb_nsec, la_ticks, value, la_mask; /* see 18.4.1 Client Configuration in Tegra3 TRM v03p */ if (bandwidth_mbytes_sec) @@ -1214,11 +1214,12 @@ static void tegra30_mc_tune_client_latency(struct tegra_mc *mc, * client may wait in the EMEM arbiter before it becomes a high-priority * request. */ + la_mask = la->mask; la_ticks = arb_nsec / mc->tick; - la_ticks = min(la_ticks, la->mask); + la_ticks = min(la_ticks, la_mask); value = mc_readl(mc, la->reg); - value &= ~(la->mask << la->shift); + value &= ~(la_mask << la->shift); value |= la_ticks << la->shift; mc_writel(mc, value, la->reg); } diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h index d2fbe6a8b25b..e7a994d16c8e 100644 --- a/include/soc/tegra/mc.h +++ b/include/soc/tegra/mc.h @@ -18,8 +18,8 @@ struct device; struct page; struct tegra_smmu_enable { - unsigned int reg; - unsigned int bit; + u16 reg; + u8 bit; }; struct tegra_mc_timing { @@ -30,22 +30,22 @@ struct tegra_mc_timing { /* latency allowance */ struct tegra_mc_la { - unsigned int reg; - unsigned int shift; - unsigned int mask; - unsigned int def; + u16 reg; + u8 shift; + u8 mask; + u8 def; }; struct tegra_mc_client { - unsigned int id; const char *name; - unsigned int swgroup; - unsigned int fifo_size; + u8 id; + u8 swgroup; + u16 fifo_size; struct tegra_smmu_enable smmu; struct tegra_mc_la la; -}; +} __packed; struct tegra_smmu_swgroup { const char *name;
diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c index 6f130e51f072..716185234b2a 100644 --- a/drivers/iommu/tegra-gart.c +++ b/drivers/iommu/tegra-gart.c @@ -17,7 +17,7 @@ #include <linux/spinlock.h> #include <linux/vmalloc.h> -#include <soc/tegra/mc.h> +#include <soc/tegra/mc-internal.h> #define GART_REG_BASE 0x24 #define GART_CONFIG (0x24 - GART_REG_BASE) diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c index 602aab98c079..fdb798c62596 100644 --- a/drivers/iommu/tegra-smmu.c +++ b/drivers/iommu/tegra-smmu.c @@ -17,7 +17,7 @@ #include <linux/dma-mapping.h> #include <soc/tegra/ahb.h> -#include <soc/tegra/mc.h> +#include <soc/tegra/mc-internal.h> struct tegra_smmu_group { struct list_head list; diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h index 1ee34f0da4f7..116bf68325b7 100644 --- a/drivers/memory/tegra/mc.h +++ b/drivers/memory/tegra/mc.h @@ -10,7 +10,7 @@ #include <linux/io.h> #include <linux/types.h> -#include <soc/tegra/mc.h> +#include <soc/tegra/mc-internal.h> #define MC_INTSTATUS 0x00 #define MC_INTMASK 0x04 diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c index e25c954dde2e..aa676c45650b 100644 --- a/drivers/memory/tegra/tegra186.c +++ b/drivers/memory/tegra/tegra186.c @@ -9,6 +9,8 @@ #include <linux/of_device.h> #include <linux/platform_device.h> +#include <soc/tegra/mc.h> + #if defined(CONFIG_ARCH_TEGRA_186_SOC) #include <dt-bindings/memory/tegra186-mc.h> #endif @@ -31,14 +33,14 @@ struct tegra186_mc_soc { unsigned int num_clients; }; -struct tegra186_mc { +struct tegra_mc { struct device *dev; void __iomem *regs; const struct tegra186_mc_soc *soc; }; -static void tegra186_mc_program_sid(struct tegra186_mc *mc) +static void tegra186_mc_program_sid(struct tegra_mc *mc) { unsigned int i; @@ -1523,8 +1525,8 @@ static const struct tegra186_mc_soc tegra194_mc_soc = { static int tegra186_mc_probe(struct platform_device *pdev) { - struct tegra186_mc *mc; struct resource *res; + struct tegra_mc *mc; int err; mc = devm_kzalloc(&pdev->dev, sizeof(*mc), GFP_KERNEL); @@ -1552,7 +1554,7 @@ static int tegra186_mc_probe(struct platform_device *pdev) static int tegra186_mc_remove(struct platform_device *pdev) { - struct tegra186_mc *mc = platform_get_drvdata(pdev); + struct tegra_mc *mc = platform_get_drvdata(pdev); of_platform_depopulate(mc->dev); @@ -1577,7 +1579,7 @@ static int __maybe_unused tegra186_mc_suspend(struct device *dev) static int __maybe_unused tegra186_mc_resume(struct device *dev) { - struct tegra186_mc *mc = dev_get_drvdata(dev); + struct tegra_mc *mc = dev_get_drvdata(dev); tegra186_mc_program_sid(mc); diff --git a/include/soc/tegra/mc-internal.h b/include/soc/tegra/mc-internal.h new file mode 100644 index 000000000000..4f327695d58c --- /dev/null +++ b/include/soc/tegra/mc-internal.h @@ -0,0 +1,62 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Copyright (C) 2014 NVIDIA Corporation + * Copyright (C) 2021 NVIDIA Corporation + */ + +#ifndef __SOC_TEGRA_MC_INTERNAL_H__ +#define __SOC_TEGRA_MC_INTERNAL_H__ + +#include <soc/tegra/mc.h> + +struct tegra_mc_soc { + const struct tegra_mc_client *clients; + unsigned int num_clients; + + const unsigned long *emem_regs; + unsigned int num_emem_regs; + + unsigned int num_address_bits; + unsigned int atom_size; + + u8 client_id_mask; + + const struct tegra_smmu_soc *smmu; + + u32 intmask; + + const struct tegra_mc_reset_ops *reset_ops; + const struct tegra_mc_reset *resets; + unsigned int num_resets; + + const struct tegra_mc_icc_ops *icc_ops; + + int (*init)(struct tegra_mc *mc); +}; + +struct tegra_mc { + struct device *dev; + struct tegra_smmu *smmu; + struct gart_device *gart; + void __iomem *regs; + struct clk *clk; + int irq; + + const struct tegra_mc_soc *soc; + unsigned long tick; + + struct tegra_mc_timing *timings; + unsigned int num_timings; + + struct reset_controller_dev reset; + + struct icc_provider provider; + + spinlock_t lock; + + struct { + struct dentry *root; + } debugfs; +}; + +#endif /* __SOC_TEGRA_MC_INTERNAL_H__ */ diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h index d2fbe6a8b25b..7be8441c6e9e 100644 --- a/include/soc/tegra/mc.h +++ b/include/soc/tegra/mc.h @@ -155,56 +155,6 @@ struct tegra_mc_icc_ops { void *data); }; -struct tegra_mc_soc { - const struct tegra_mc_client *clients; - unsigned int num_clients; - - const unsigned long *emem_regs; - unsigned int num_emem_regs; - - unsigned int num_address_bits; - unsigned int atom_size; - - u8 client_id_mask; - - const struct tegra_smmu_soc *smmu; - - u32 intmask; - - const struct tegra_mc_reset_ops *reset_ops; - const struct tegra_mc_reset *resets; - unsigned int num_resets; - - const struct tegra_mc_icc_ops *icc_ops; - - int (*init)(struct tegra_mc *mc); -}; - -struct tegra_mc { - struct device *dev; - struct tegra_smmu *smmu; - struct gart_device *gart; - void __iomem *regs; - struct clk *clk; - int irq; - - const struct tegra_mc_soc *soc; - unsigned long tick; - - struct tegra_mc_timing *timings; - unsigned int num_timings; - - struct reset_controller_dev reset; - - struct icc_provider provider; - - spinlock_t lock; - - struct { - struct dentry *root; - } debugfs; -}; - int tegra_mc_write_emem_configuration(struct tegra_mc *mc, unsigned long rate); unsigned int tegra_mc_get_emem_device_count(struct tegra_mc *mc);