25.03.2021 19:11, Dmitry Osipenko пишет:

25.03.2021 18:52, Thierry Reding пишет:

quoted

On Thu, Mar 25, 2021 at 06:12:51PM +0300, Dmitry Osipenko wrote:

quoted

25.03.2021 16:03, Thierry Reding пишет:

quoted

From: Thierry Reding <redacted>

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 <redacted>
---
 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.

quoted

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;

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel