Re: RFC on drivers/memory vs drivers/edac memory mapping for DDR Controller

[Krzysztof Kozlowski]

On 03/01/2023 14:47, Shenhar, Talel wrote:
> 
> On 1/3/2023 3:23 PM, Krzysztof Kozlowski wrote:
>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>>
>>
>>
>> On 03/01/2023 14:12, Shenhar, Talel wrote:
>>> On 1/2/2023 6:25 PM, Krzysztof Kozlowski wrote:
>>>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>>>>
>>>>
>>>>
>>>> On 02/01/2023 17:21, Shenhar, Talel wrote:
>>>>> On 1/2/2023 3:59 PM, Krzysztof Kozlowski wrote:
>>>>>> On 02/01/2023 14:44, Shenhar, Talel wrote:
>>>>>>> On 1/2/2023 2:47 PM, Krzysztof Kozlowski wrote:
>>>>>>>> On 02/01/2023 13:17, Shenhar, Talel wrote:
>>>>>>>>
>>>>>>>>> Things we had in mind:
>>>>>>>>> 1) map more specific region to avoid conflict (we don't need the same
>>>>>>>>> registers on both entity so if we do very specific multiple mapping this
>>>>>>>>> shall be resolved)
>>>>>>>>> 2) use other kernel API for mapping that doesn't do request_mem_region
>>>>>>>>> (or use the reserve only for one of them)
>>>>>>>>> 3) have single driver (edac mc) handle also the refresh rate
>>>>>>>>> 4) export edac_mc.h and have the drivers/memory have all the needed code
>>>>>>>>> to do both edac and refresh rate under drivers/memory
>>>>>>>> None of these address the core problem - possibly inaccurate hardware
>>>>>>>> description...
>>>>>>> Can you elaborate on this inaccurate hardware description?
>>>>>> I explained - using same IO address suggests you used Linux driver
>>>>>> structure in your hardware description. I assume we talk here about
>>>>>> Devicetree. If not, that's quite different case... then I guess ACPI,
>>>>>> which I do not care - I am not it's maintainer.
>>>>>>
>>>>>>> Also, I'd like to write down my understanding of your response from above:
>>>>>>>
>>>>>>> it seems you see as possible solution both using different API that
>>>>>>> allow overlapping (solution 2) and also for splitting the IO address
>>>>>>> space to finer pieces to achieve full HW description (solution 1)
>>>>>> No. Sorry, we probably talk about two different things.
>>>>>>
>>>>>> You started writing that you have a hardware described as one IO address
>>>>>> space and now have a problem developing drivers for it.
>>>>>>
>>>>>> The driver model for this is entirely different problem than problem of
>>>>>> accurate hardware description. Whether you described HW correct or not,
>>>>>> I don't know. You did not provide any details here, like DTS or bindings
>>>>>> (if we talk about Devicetree).
>>>>>>
>>>>>> Having multiple drivers using similar resources is already solved many
>>>>>> times (MFD, syscon).
>>>>>>
>>>>>> Whether the solution is correct or not is one more (third) topic: poking
>>>>>> to same IO address space from two different drivers is error-prone. This
>>>>>> one is solvable with splitting IO address space.
>>>>>>
>>>>>> Best regards,
>>>>>> Krzysztof
>>>>> You are right.
>>>>>
>>>>> Let me elaborate on this.
>>>>>
>>>>> We will write down the hardware description via device tree.
>>>>>
>>>>> Then we will write the driver which will honor that binding.
>>>>>
>>>>> So the question is what is the best practice there assuming there is no
>>>>> shared registers however there is overlapping.
>>>> The correct solution is to describe hardware. The hardware is memory
>>>> controller. There is no hardware called "scaller of memory controller".
>>>> There is no hardware called "EDAC" because that's purely a Linux term.
>>>>
>>>> Your DTS should accurately describe the hardware, not drivers. Then
>>>> drivers can do whatever they want with it - have safe, non-concurrent
>>>> access or keep poking same registers and break things...
>>> The way the HW shall be described in DT is tightly coupled to the way
>>> the drivers will work on mapping the IO addresses.
>> No, that's not true and such DT description will get probably Rob's or
>> mine comments. The HW shall be described without tying to one, specific
>> driver implementation. Otherwise why do you make it tightly coupled to
>> Linux, but ignore BSD, firmware and bootloaders?
>>
>> Don't tightly couple DT with your drivers.
> 
> But of course its true :)
> 
> binding document define the reg property for drivers that do registers 
> access.
> 
> If you define it one way or the other that shall change the driver 
> mapping policy/method.

Read again my message. Binding is not coupled with drivers. Binding is
coupled with hardware because it describes the DTS written for hardware,
not for drivers.

I did not say that binding will not affect the drivers. I said that
design of binding is somehow independent (at least partially) from
drivers and once you answer to question "how the hardware looks?"
several problems dissapear. You create here some questions and problems
because you have inaccurate hardware description (e.g. two devices -
some fake "EDAC" and frequency scaler device...).

> 
>>
>>> There are 3 ways to describe the HW as far as I see it from address
>>> point of view: (actually 2 as option 3 is not really sane)
>>>
>>> 1) one big chunk of registers
>>>
>>> 2) smaller chunk of registers aiming to have each chunk describe a
>>> subset of the HW capablity (e.g. RAS, e.g. Refresh-rate, ...)
>>>
>>> 3) describe each register with its name
>>>
>>> Each option dictate how driver shall map the address space.
>> Again, the driver does not matter. You have one device, describe
>> properly one device. DT is not to describe drivers.
> 
> The way drivers are being probed is based on compatible found in DT.
> 
> So you only get one platform driver probe per device described in DT.

No, that's not true. I also gave you hints to solve it - MFD, simple-mfd
etc.

> 
> If we have single device described in DT then we won't have two distinct 
> platform drivers getting probe.

No, again not true. You can have infinite number of drivers getting
probed as result of one device node in DTS.

> 
> (We could not have them platform driver and have them as regular module 
> which go and look "manually" for that device... but that looks too hacky).
> 
> 
> As we do consider two distinct drivers the idea was to have two devices 
> described in DT. One gets the registers subset it want while the other 
> get the registers it want.

I repeated it many, many times. Describe the hardware. Now you again
ignore all my comments and mention that you need two devices because you
have two drivers.

I repeated it way too many times, so the last:
Most likely you have one hardware, so there is one device in DTS.
Describe in DTS the hardware, not the driver model/binding/problems you
have.

> 
> 
> So how would you have the DT described and how would driver/s look like 
> for cases that the unit registers are split interchangeably?

What do you mean by "split interchangeably"? I understood there is *one*
hardware device, not two. One memory controller. Do you want to say you
have two independent memory controllers for the same IO address space?

Anyway, reviewing real patches takes priority of reviewing imaginary
solutions, so please send patches. Otherwise it's end of discussion to
me. If you still have questions, please go back to what I repeated many
times - describe hardware in DT, not the drivers.

Best regards,
Krzysztof