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

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

[Shenhar, Talel]

Hi,

Want to consult on a topic that involve both drivers/memory and 
drivers/edac.

* We want to introduce driver that reads DDR controller RAS register and 
notify for ECC errors by using EDAC MC API found in drivers/edac.
* We also want to have a capability to dynamically change DDR refresh 
rate based on thermal values (best to be done in drivers/memory ?).

The pain point here is that both capabilities are controlled from the 
DDR controller.
This create issue while using 
devm_platform_ioremap_resource*->devm_request_mem_region which prevent 
two mapping of same area.

It seems to be expected problem as we have 2 "framework" (edac and 
memory) split while both aim for same HW unit.
What is the recommended way to face such conflicts?

We had several concept in mind but would love to get your point of view 
first.

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

Your recommendation shall be appreciated!

Thanks,
Talel.

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

[Krzysztof Kozlowski]

On 02/01/2023 13:17, Shenhar, Talel wrote:
> Hi,
> 
> Want to consult on a topic that involve both drivers/memory and 
> drivers/edac.
> 
> * We want to introduce driver that reads DDR controller RAS register and 
> notify for ECC errors by using EDAC MC API found in drivers/edac.
> * We also want to have a capability to dynamically change DDR refresh 
> rate based on thermal values (best to be done in drivers/memory ?).
> 
> The pain point here is that both capabilities are controlled from the 
> DDR controller.
> This create issue while using 
> devm_platform_ioremap_resource*->devm_request_mem_region which prevent 
> two mapping of same area.

This could be avoided but the true problem is that you have two devices
for same memory mapping. Devicetree does not allow it and it points to
some wrong hardware representation in DTS.

> 
> It seems to be expected problem as we have 2 "framework" (edac and 
> memory) split while both aim for same HW unit.
> What is the recommended way to face such conflicts?

You now mix Devicetree and Linux drivers. You can have same IO address
space used by multiple drivers, even though it is not always good
approach (concurrent and conflicting change of same settings).

HW description is irrelevant to this.

> 
> We had several concept in mind but would love to get your point of view 
> first.

Describe hardware accurately and completely. This solves all the
problems, doesn't it? Linux drivers do not depend on it and you can make
it differently.

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

Best regards,
Krzysztof

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

[Shenhar, Talel]

On 1/2/2023 2:47 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 13:17, Shenhar, Talel wrote:
>> Hi,
>>
>> Want to consult on a topic that involve both drivers/memory and
>> drivers/edac.
>>
>> * We want to introduce driver that reads DDR controller RAS register and
>> notify for ECC errors by using EDAC MC API found in drivers/edac.
>> * We also want to have a capability to dynamically change DDR refresh
>> rate based on thermal values (best to be done in drivers/memory ?).
>>
>> The pain point here is that both capabilities are controlled from the
>> DDR controller.
>> This create issue while using
>> devm_platform_ioremap_resource*->devm_request_mem_region which prevent
>> two mapping of same area.
> This could be avoided but the true problem is that you have two devices
> for same memory mapping. Devicetree does not allow it and it points to
> some wrong hardware representation in DTS.
>
>> It seems to be expected problem as we have 2 "framework" (edac and
>> memory) split while both aim for same HW unit.
>> What is the recommended way to face such conflicts?
> You now mix Devicetree and Linux drivers. You can have same IO address
> space used by multiple drivers, even though it is not always good
> approach (concurrent and conflicting change of same settings).
>
> HW description is irrelevant to this.
>
>> We had several concept in mind but would love to get your point of view
>> first.
> Describe hardware accurately and completely. This solves all the
> problems, doesn't it? Linux drivers do not depend on it and you can make
> it differently.

Describing the hardware accurately and completely means to have multiple 
reg property in the device-tree, right?

That way we will split the HW description to smaller bits rather then 
just big "ddrc", and that shall allow us to have two drivers and each 
one will get its own share of the split, right?

That was the intent of solution 1 below.

>
>> 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?

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)

>
> Best regards,
> Krzysztof
>

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

[Krzysztof Kozlowski]

On 02/01/2023 14:44, Shenhar, Talel wrote:
> 
> On 1/2/2023 2:47 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 13:17, Shenhar, Talel wrote:
>>> Hi,
>>>
>>> Want to consult on a topic that involve both drivers/memory and
>>> drivers/edac.
>>>
>>> * We want to introduce driver that reads DDR controller RAS register and
>>> notify for ECC errors by using EDAC MC API found in drivers/edac.
>>> * We also want to have a capability to dynamically change DDR refresh
>>> rate based on thermal values (best to be done in drivers/memory ?).
>>>
>>> The pain point here is that both capabilities are controlled from the
>>> DDR controller.
>>> This create issue while using
>>> devm_platform_ioremap_resource*->devm_request_mem_region which prevent
>>> two mapping of same area.
>> This could be avoided but the true problem is that you have two devices
>> for same memory mapping. Devicetree does not allow it and it points to
>> some wrong hardware representation in DTS.
>>
>>> It seems to be expected problem as we have 2 "framework" (edac and
>>> memory) split while both aim for same HW unit.
>>> What is the recommended way to face such conflicts?
>> You now mix Devicetree and Linux drivers. You can have same IO address
>> space used by multiple drivers, even though it is not always good
>> approach (concurrent and conflicting change of same settings).
>>
>> HW description is irrelevant to this.
>>
>>> We had several concept in mind but would love to get your point of view
>>> first.
>> Describe hardware accurately and completely. This solves all the
>> problems, doesn't it? Linux drivers do not depend on it and you can make
>> it differently.
> 
> Describing the hardware accurately and completely means to have multiple 
> reg property in the device-tree, right?

Not necessarily. It means to describe the devices, the hardware, not
drivers.

> 
> That way we will split the HW description to smaller bits rather then 
> just big "ddrc", and that shall allow us to have two drivers and each 
> one will get its own share of the split, right?

Not necessarily. If you have one hardware, why would you split it? It's
only one memory controller, not two.

> That was the intent of solution 1 below.
> 
>>
>>> 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

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

[Shenhar, Talel]

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.

e.g. the EDAC driver needs register 0,1,2,4,5 and refresh-rate needs 
register 3.

If we would only have EDAC driver than we would do IO address mapping 
from 0 with size 5 (not caring mapping register 3 even that its not used).

However, with the other driver (refresh rate) that need register 3 we am 
facing a problem.

So looking for the best solution here.

I don't think this is a problem that is specific to drivers/edac and to 
drivers/memory, however, due to the nature of those two libraries this 
conflict is more expected.


>

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

[Krzysztof Kozlowski]

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

> 
> e.g. the EDAC driver needs register 0,1,2,4,5 and refresh-rate needs 
> register 3.

I don't think there is EDAC and "refresh-rate" hardwares. There is
memory controller.

> 
> If we would only have EDAC driver than we would do IO address mapping 
> from 0 with size 5 (not caring mapping register 3 even that its not used).
> 
> However, with the other driver (refresh rate) that need register 3 we am 
> facing a problem.
> 
> So looking for the best solution here.
> 
> I don't think this is a problem that is specific to drivers/edac and to 
> drivers/memory, however, due to the nature of those two libraries this 
> conflict is more expected.

All these problems look like started from wrong hardware description, so
not sure if it is worth fixing something where the basis is already not
correct.

Best regards,
Krzysztof

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

[Shenhar, Talel]

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.

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.


If option 1 is chosen, then we shall have 2 drivers with same reg 
description.

For that case, they can both remap the whole space or each one can try 
to map only the section it needs.

If option 2 is chosen, then each driver can use DT to know exactly what 
it needs to map and do it in safer manner.


>
>> e.g. the EDAC driver needs register 0,1,2,4,5 and refresh-rate needs
>> register 3.
> I don't think there is EDAC and "refresh-rate" hardwares. There is
> memory controller.
>
>> If we would only have EDAC driver than we would do IO address mapping
>> from 0 with size 5 (not caring mapping register 3 even that its not used).
>>
>> However, with the other driver (refresh rate) that need register 3 we am
>> facing a problem.
>>
>> So looking for the best solution here.
>>
>> I don't think this is a problem that is specific to drivers/edac and to
>> drivers/memory, however, due to the nature of those two libraries this
>> conflict is more expected.
> All these problems look like started from wrong hardware description, so
> not sure if it is worth fixing something where the basis is already not
> correct.
>
> Best regards,
> Krzysztof
>

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

[Krzysztof Kozlowski]

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.

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

You did not Cc relevant here mailing addresses (DT and Rob), so this
discussion might miss their feedback.

How the drivers map IO address space is independent question and should
not determine the hardware description. You want to say that hardware
changes depending on OS? One OS means hardware is like that and on other
OS it's different?

> 
> 
> If option 1 is chosen, then we shall have 2 drivers with same reg 
> description.

Drivers are not related to DT bindings and DTS. Two different things.

> 
> For that case, they can both remap the whole space or each one can try 
> to map only the section it needs.
> 
> If option 2 is chosen, then each driver can use DT to know exactly what 
> it needs to map and do it in safer manner.




Best regards,
Krzysztof

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

[Shenhar, Talel]

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.

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

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

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


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

>
> You did not Cc relevant here mailing addresses (DT and Rob), so this
> discussion might miss their feedback.
>
> How the drivers map IO address space is independent question and should
> not determine the hardware description. You want to say that hardware
> changes depending on OS? One OS means hardware is like that and on other
> OS it's different?
>
>>
>> If option 1 is chosen, then we shall have 2 drivers with same reg
>> description.
> Drivers are not related to DT bindings and DTS. Two different things.
>
>> For that case, they can both remap the whole space or each one can try
>> to map only the section it needs.
>>
>> If option 2 is chosen, then each driver can use DT to know exactly what
>> it needs to map and do it in safer manner.
>
>
>
> Best regards,
> Krzysztof
>

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

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

[Krzysztof Kozlowski]

On 03/01/2023 14:47, Shenhar, Talel wrote:
> So how would you have the DT described and how would driver/s look like 
> for cases that the unit registers are split interchangeably?
> 
>>
>> You did not Cc relevant here mailing addresses (DT and Rob), so this
>> discussion might miss their feedback.
>>
>> How the drivers map IO address space is independent question and should
>> not determine the hardware description. You want to say that hardware
>> changes depending on OS? One OS means hardware is like that and on other
>> OS it's different?

BTW, you nicely skipped points of my email which are a bit
inconvenient,e.g. how you want to tie DTS and bindings to one specific
driver implementation and ignore the rest...

Best regards,
Krzysztof

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

[Shenhar, Talel]

On 1/3/2023 4:24 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:47, Shenhar, Talel wrote:
>> So how would you have the DT described and how would driver/s look like
>> for cases that the unit registers are split interchangeably?
>>
>>> You did not Cc relevant here mailing addresses (DT and Rob), so this
>>> discussion might miss their feedback.
>>>
>>> How the drivers map IO address space is independent question and should
>>> not determine the hardware description. You want to say that hardware
>>> changes depending on OS? One OS means hardware is like that and on other
>>> OS it's different?
> BTW, you nicely skipped points of my email which are a bit
> inconvenient,e.g. how you want to tie DTS and bindings to one specific
> driver implementation and ignore the rest...

Sorry missed that.

Rob would probably be relevant for this topic as well, however, I didn't 
want to focus on DT for this topic.

Seems your question is what I am actually asking...

However your answer keep getting back to do "full hardware description 
in dt and that it doesn't relate to driver" but does not give concrete 
response hence I fail to get the answer or direction I am looking for.

The solution I previously gave show different possibility for describing 
the HW and the impact on the drivers and wonder what is the best path.


For now I think this is the path I shall take which is option 1:

"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)"

Which seems to be what you are trying to say by give more complete HW 
description.


>
> Best regards,
> Krzysztof
>

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

[Borislav Petkov]

On Mon, Jan 02, 2023 at 02:17:24PM +0200, Shenhar, Talel wrote:
> * We want to introduce driver that reads DDR controller RAS register and
> notify for ECC errors by using EDAC MC API found in drivers/edac.
> * We also want to have a capability to dynamically change DDR refresh rate
> based on thermal values (best to be done in drivers/memory ?).

Is there any particular reason to want to report the errors through EDAC?

Or can't you simply read the RAS register in your memory driver and dump error
info from there so that you have a single driver that does it all?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Shenhar, Talel]

On 1/2/2023 3:43 PM, Borislav Petkov 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 Mon, Jan 02, 2023 at 02:17:24PM +0200, Shenhar, Talel wrote:
>> * We want to introduce driver that reads DDR controller RAS register and
>> notify for ECC errors by using EDAC MC API found in drivers/edac.
>> * We also want to have a capability to dynamically change DDR refresh rate
>> based on thermal values (best to be done in drivers/memory ?).
> Is there any particular reason to want to report the errors through EDAC?
>
> Or can't you simply read the RAS register in your memory driver and dump error
> info from there so that you have a single driver that does it all?

Doesn't it go against the MC EDAC concept...?

Reinventing the wheel is something that usually doesn't end well. (I 
could probably list them but guess that as the EDAC maintainer you can 
do it better than me :)  )

I would probably consider the other way around - take the refresh-rate 
driver inside the MC driver as the refresh-rate does not use any 
"memory" framework under drivers/memory.

>
> --
> Regards/Gruss,
>      Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette

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

[Borislav Petkov]

On Mon, Jan 02, 2023 at 06:14:04PM +0200, Shenhar, Talel wrote:
> Doesn't it go against the MC EDAC concept...?

You mean the concept of a glorified error reporter? :-)

> Reinventing the wheel is something that usually doesn't end well. (I could
> probably list them but guess that as the EDAC maintainer you can do it
> better than me :)  )

You mean EDAC maintainer because no one else is willing to do it?

See, I don't mind if errors get reported through EDAC but the EDAC "facilities"
are just a reporting mechanism and memory controller layout detection glue.
Yeah, yeah, it can set scrub rate and so on in some drivers but it really is
just that. Oh, and some EDAC drivers provide a simple interrupt handler when the
hw reports errors with a special interrupt.

But, if in your case we get to end up in some weird resources sharing scheme,
then you don't really need the design overhead and you can simply printk the
errors from the other driver.

> I would probably consider the other way around - take the refresh-rate
> driver inside the MC driver as the refresh-rate does not use any "memory"
> framework under drivers/memory.

That is also possible.

The x86 EDAC drivers do get to change settings in the memory controller as a
result of RAS actions because there nothing else "owns" that memory controller.

But I have no clue what your hw does so...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette