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@xxxxxxxxxx>
>>>>
>>>> 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@xxxxxxxxxx>
>>>> ---
>>>> 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;
