Re: [PATCH 1/9] memory: tegra: Move internal data structures into separate header

[Dmitry Osipenko <digetx@gmail.com>]

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;