Re: [PATCH v4 09/15] memory: tegra: Squash tegra20-mc into common tegra-mc driver

[Dmitry Osipenko <digetx@gmail.com>]

On 27.04.2018 13:24, Thierry Reding wrote:
> On Fri, Apr 27, 2018 at 01:13:47PM +0300, Dmitry Osipenko wrote:
>> On 27.04.2018 12:34, Thierry Reding wrote:
>>> On Mon, Apr 09, 2018 at 10:28:31PM +0300, Dmitry Osipenko wrote:
>>> [...]
>>>> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
>>> [...]
>>>> +#define MC_GART_ERROR_REQ		0x30
>>>> +#define MC_DECERR_EMEM_OTHERS_STATUS	0x58
>>>> +#define MC_SECURITY_VIOLATION_STATUS	0x74
>>> [...]
>>>> diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h
>>> [...]
>>>> @@ -21,19 +21,30 @@
>>>>  #define MC_INT_INVALID_SMMU_PAGE (1 << 10)
>>>>  #define MC_INT_ARBITRATION_EMEM (1 << 9)
>>>>  #define MC_INT_SECURITY_VIOLATION (1 << 8)
>>>> +#define MC_INT_INVALID_GART_PAGE (1 << 7)
>>>>  #define MC_INT_DECERR_EMEM (1 << 6)
>>>>  
>>>>  static inline u32 mc_readl(struct tegra_mc *mc, unsigned long offset)
>>>>  {
>>>> +	if (mc->regs2 && offset >= 0x24)
>>>> +		return readl(mc->regs2 + offset - 0x3c);
>>>
>>> I'm still not sure how this is supposed to work. If we pass in
>>> MC_GART_ERROR_REQ as offset into mc_readl(), then the condition above
>>> will be true (0x30 >= 0x24) but then the new offset will be computed
>>> and we end up with:
>>>
>>> 	return readl(mc->regs2 + 0x30 - 0x3c);
>>>
>>> which means we'll be adding a negative offset (or rather a very large
>>> offset because it will wrap around).
>>
>> Indeed! Thank you for pointing at it again, now I see the issue. It probably
>> works because actual registers mapping is aligned to page(?) size and adding the
>> large offset with wraparound is equal to subtraction.
>>
>> That register belongs to the GART and we can't simply move interrupt handling to
>> the GART driver because status register is within the MC in device tree. We can
>> omit reading of MC_GART_ERROR_REQ and simply report GART page fault for the
>> starter and then reorganize drivers by making MC driver MFD and GART its
>> sub-device, what do you think?
> 
> Sounds like a good idea. Can you send a fix on top of this that I can
> squash into this when applying?

Sure.

> As for integrating GART with MC, I'd prefer something that doesn't use
> MFD but rather does something similar to what we have for the SMMU. I
> think that's simpler to do and has less boilerplate. I think it's also
> warranted because the MC and GART are very tightly coupled, so an MFD
> would be slightly over-engineered, in my opinion.

Okay, I'll recap how SMMU is integrated with MC and then come up with something.