Device driver configuration and driver_data distinction.


Marcus Shawcroft <marcus.shawcroft@...>
 

Hi,

The DEVICE_INIT() macro and associated driver framework provides for
each driver instance to have two data structures, the config structure
and the driver_data structure.

Documentation in docs/drivers/drivers.rst indicates that the
configuration structure is for read only, build time configured data,
while the driver_data structure is for dynamic state.

This design make sense from the perspective that the read only config
data *could* be const and placed in flash only, hence lowering
pressure on sram.

However, it is not possible to make a drivers config structure const
because include/device.h defines struct device_config with a non const
*config_info, hence passing a const config structure to DEVICE_INIT()
generates a diagnostic/error.

I can see no obvious reason why we do not define config_info as const
paving the way for device config information to move to flash.

The infrastructure around config_info was landed in:
commit c9ac95a43aa46e0714a8db10d4d8f940c952acaf
Author: Dirk Brandewie <dirk.j.brandewie(a)intel.com>

config_info was non const in this original commit.

Can anybody provide insight into why config_info should not be const?
Was this an oversight, or, by design, in the original implementation?

Cheers
/Marcus


Tomasz Bursztyka
 

Hi Marcus,

I believe we did not make it const at the beginning due to PCI
enumeration on Galileo.
This was early days, and things changed then.

Actually there have been plans to move it to const, at least I remember
hearing such idea
from Daniel Kalowsky, for the same exact reasons you noted, it might
even be in the git history,
somewhere. But we were too busy to follow up at that time.

So this would indeed be very welcome, if you want to propose a patch for it.
Note, however, that some device drivers may be using such configuration
structure wrongly.
(aka: accessing and changing its data). So you would probably need to
change things there as well.

Tomasz

Hi,

The DEVICE_INIT() macro and associated driver framework provides for
each driver instance to have two data structures, the config structure
and the driver_data structure.

Documentation in docs/drivers/drivers.rst indicates that the
configuration structure is for read only, build time configured data,
while the driver_data structure is for dynamic state.

This design make sense from the perspective that the read only config
data *could* be const and placed in flash only, hence lowering
pressure on sram.

However, it is not possible to make a drivers config structure const
because include/device.h defines struct device_config with a non const
*config_info, hence passing a const config structure to DEVICE_INIT()
generates a diagnostic/error.

I can see no obvious reason why we do not define config_info as const
paving the way for device config information to move to flash.

The infrastructure around config_info was landed in:
commit c9ac95a43aa46e0714a8db10d4d8f940c952acaf
Author: Dirk Brandewie <dirk.j.brandewie(a)intel.com>

config_info was non const in this original commit.

Can anybody provide insight into why config_info should not be const?
Was this an oversight, or, by design, in the original implementation?

Cheers
/Marcus


Jon Medhurst (Tixy) <tixy@...>
 

On Thu, 2016-10-06 at 11:18 +0100, Marcus Shawcroft wrote:
The DEVICE_INIT() macro and associated driver framework provides for
each driver instance to have two data structures, the config structure
and the driver_data structure.

Documentation in docs/drivers/drivers.rst indicates that the
configuration structure is for read only, build time configured data,
while the driver_data structure is for dynamic state.

This design make sense from the perspective that the read only config
data *could* be const and placed in flash only, hence lowering
pressure on sram.

However, it is not possible to make a drivers config structure const
because include/device.h defines struct device_config with a non const
*config_info, hence passing a const config structure to DEVICE_INIT()
generates a diagnostic/error.
I too wondered the same, as my first instinct was to declare these const
along with other structures like the various API function tables and
strings used for device names. But the compiler soon alerted me to the
fact that Zephy APIs seem to have an allergy to 'const' :-)

I don't know if the widespread use of sections and linker scripts
actually ends up putting a lot of these things into read-only memory
anyway, or if these all really do end up wasting RAM.

--
Tixy


Andy Ross
 

Marcus Shawcroft wrote (on Thursday, October 06, 2016 3:18AM):
This design make sense from the perspective that the read only config
data *could* be const and placed in flash only, hence lowering
pressure on sram.

However, it is not possible to make a drivers config structure const
because include/device.h defines struct device_config with a non const
*config_info
Actually it goes into the ROM data section anyway, despite the lack of
const. This is controlled in device.h by declaring the device_config
struct to be in the "devconfig.init" section. This then is placed by
the linker script (one per arhicecture right now, sigh) into the
ROMable region, right before the compiler-managed .rodata (which of
course *is* the stuff that depends on const).

The lack of const is (well, should be) just a missed opportunity for
compiler warnings. But AFAICT it's wrong and should be fixed.

Andy


Marcus Shawcroft <marcus.shawcroft@...>
 

On 6 October 2016 at 15:58, Andy Ross <andrew.j.ross(a)intel.com> wrote:
Marcus Shawcroft wrote (on Thursday, October 06, 2016 3:18AM):
This design make sense from the perspective that the read only config
data *could* be const and placed in flash only, hence lowering
pressure on sram.

However, it is not possible to make a drivers config structure const
because include/device.h defines struct device_config with a non const
*config_info
Actually it goes into the ROM data section anyway, despite the lack of
const. This is controlled in device.h by declaring the device_config
struct to be in the "devconfig.init" section. This then is placed by
the linker script (one per arhicecture right now, sigh) into the
ROMable region, right before the compiler-managed .rodata (which of
course *is* the stuff that depends on const).

The lack of const is (well, should be) just a missed opportunity for
compiler warnings. But AFAICT it's wrong and should be fixed.
Andy, I think we are talking about to different structures, given:

static struct gpio_k64_config gpio_k64_E_cfg = {
.gpio_base_addr = GPIO_K64_E_BASE_ADDR,
.port_base_addr = PORT_K64_E_BASE_ADDR,
};
DEVICE_AND_API_INIT(gpio_k64_E, CONFIG_GPIO_K64_E_DEV_NAME, gpio_k64_E_init,
&gpio_data_E, &gpio_k64_E_cfg,
SECONDARY, CONFIG_KERNEL_INIT_PRIORITY_DEFAULT,
&gpio_k64_drv_api_funcs);


The gpio_k64_E_cfg structure is placed in the data section and copied
to RAM from FLASH at startup.

2000007c l O datas 00000008 gpio_k64_E_cfg

Making in const (and dealing with the *config_info discussed above), gives:
00003b20 l O rodata 00000008 gpio_k64_E_cfg

Cheers
/Marcus


Andy Ross
 

Marcus Shawcroft wrote (on Thursday, October 06, 2016 8:26AM):
Andy, I think we are talking about to different structures
Ah, gotcha. Yeah, you're totally right. The only trick I can see
with merging this would be verifying that no existing drivers are
accidentally exploiting that missing const to mutate their config
structs.

Andy


Marcus Shawcroft <marcus.shawcroft@...>
 

On 6 October 2016 at 12:06, Tomasz Bursztyka
<tomasz.bursztyka(a)linux.intel.com> wrote:
Hi Marcus,

I believe we did not make it const at the beginning due to PCI enumeration
on Galileo.
This was early days, and things changed then.

Actually there have been plans to move it to const, at least I remember
hearing such idea
from Daniel Kalowsky, for the same exact reasons you noted, it might even be
in the git history,
somewhere. But we were too busy to follow up at that time.

So this would indeed be very welcome, if you want to propose a patch for it.
Note, however, that some device drivers may be using such configuration
structure wrongly.
(aka: accessing and changing its data). So you would probably need to change
things there as well.
Hi,

Thanks for the background information.

Switching to const config_info will require the following:
1) Update ~50 drivers with an explicit const in any pointers they
construct when accessing their config_info structure. This change is
safe to make even when struct device {} has a a non const *config_info
2) Update a small subset of the above 50 to make their config
structures static. Not strictly necessary, but seems like the right
thing to do for all those drivers that are self contained within one
file.
3) Update ~12 drivers that currently have RW data within their RO
config data structure. This should be a straight forward mechanical
move of each RW object within config to the corresponding driver_data
structure.
4) Update ~12 drivers to put explict const in any pointers they
construct when accessing config_info
5) Update device {} to make config_info const.
6) Update all ~60 odd drivers to add const to their config structure
definitions.

All of this is mechanical, most trivial, some slightly more involved.
The hard part is likely to be figuring out how to make sure everything
modified gets compiled / tested as appropriate. Each of these steps
can be executed individually and incrementally without breaking the
tree.

3) Is not as difficult as it might first appear since each driver
instance has a RO config structure and a RW driver data structure, it
is essentially a trivial exercise of moving objects from one structure
to the other.

Sound sane?

Cheers
/Marcus


Tomasz Bursztyka
 

Hi Marcus,


Sound sane?
ACK from me at least.

Tomasz


Benjamin Walsh <benjamin.walsh@...>
 

On Thu, Oct 06, 2016 at 07:04:42PM +0200, Tomasz Bursztyka wrote:
Hi Marcus,


Sound sane?
ACK from me at least.
Seconded.


Iván Briano <ivan.briano at intel.com...>
 

On Thu, 06 Oct 2016 13:22:31 -0400, Benjamin Walsh wrote:
On Thu, Oct 06, 2016 at 07:04:42PM +0200, Tomasz Bursztyka wrote:
Hi Marcus,


Sound sane?
ACK from me at least.
Seconded.
Thirded.


Dan Kalowsky
 

On Thu, Oct 6, 2016 at 4:06 AM, Tomasz Bursztyka <
tomasz.bursztyka(a)linux.intel.com> wrote:

Hi Marcus,

I believe we did not make it const at the beginning due to PCI enumeration
on Galileo.
This was early days, and things changed then.
Pretty much this was the reason.


Actually there have been plans to move it to const, at least I remember
hearing such idea
from Daniel Kalowsky, for the same exact reasons you noted, it might even
be in the git history,
somewhere. But we were too busy to follow up at that time.
I believe we even did a move that changed it to const and then ran into an
issue with a specific compiler we were to support and reverted out the
change. Might have only been in a local topic branch though.

--
"Do you expect me to talk?"
"No Mr. Bond, I expect you to die."


Marcus Shawcroft <marcus.shawcroft@...>
 

On 6 October 2016 at 19:41, Dan Kalowsky <dank(a)deadmime.org> wrote:


On Thu, Oct 6, 2016 at 4:06 AM, Tomasz Bursztyka
<tomasz.bursztyka(a)linux.intel.com> wrote:

Hi Marcus,

I believe we did not make it const at the beginning due to PCI enumeration
on Galileo.
This was early days, and things changed then.

Pretty much this was the reason.


Actually there have been plans to move it to const, at least I remember
hearing such idea
from Daniel Kalowsky, for the same exact reasons you noted, it might even
be in the git history,
somewhere. But we were too busy to follow up at that time.

I believe we even did a move that changed it to const and then ran into an
issue with a specific compiler we were to support and reverted out the
change. Might have only been in a local topic branch though.
PCI enumeration currently updates the pci_dev object in the various
config structures along with base_addr and irq_num, I propose to move
these objects from the config struct to the runtime struct.

Cheers
/Marcus


--
"Do you expect me to talk?"
"No Mr. Bond, I expect you to die."


Tomasz Bursztyka
 

Hi Dan,


On Thu, Oct 6, 2016 at 4:06 AM, Tomasz Bursztyka
<tomasz.bursztyka(a)linux.intel.com
<mailto:tomasz.bursztyka(a)linux.intel.com>> wrote:

Hi Marcus,

I believe we did not make it const at the beginning due to PCI
enumeration on Galileo.
This was early days, and things changed then.


Pretty much this was the reason.

Actually there have been plans to move it to const, at least I
remember hearing such idea
from Daniel Kalowsky, for the same exact reasons you noted, it
might even be in the git history,
somewhere. But we were too busy to follow up at that time.


I believe we even did a move that changed it to const and then ran
into an issue with a specific compiler we were to support and reverted
out the change. Might have only been in a local topic branch though.
Thanks for the precision!
(Let's hope early stage of iamcu support was the culprit)

Tomasz


Marcus Shawcroft <marcus.shawcroft@...>
 

Hi,

On 6 October 2016 at 17:55, Marcus Shawcroft <marcus.shawcroft(a)gmail.com> wrote:

Switching to const config_info will require the following:
1) Update ~50 drivers with an explicit const in any pointers they
construct when accessing their config_info structure. This change is
safe to make even when struct device {} has a a non const *config_info
2) Update a small subset of the above 50 to make their config
structures static. Not strictly necessary, but seems like the right
thing to do for all those drivers that are self contained within one
file.
3) Update ~12 drivers that currently have RW data within their RO
config data structure. This should be a straight forward mechanical
move of each RW object within config to the corresponding driver_data
structure.
4) Update ~12 drivers to put explict const in any pointers they
construct when accessing config_info
All of the patches to achieve the above are all now on gerrit, most
have been merged already.

.. with the exception of the DW PWM driver, which so far as I can tell
does not compile for any board at this point in time,
https://jira.zephyrproject.org/browse/ZEP-1040. Is there a board that
this driver should work for?

5) Update device {} to make config_info const
Once the above patches have been merged I'll post this one line patch.
Once this patch is applied all new drivers will need to consume their
config_info via a const * or they will get a build error.

6) Update all ~60 odd drivers to add const to their config structure
definitions.
Followed by these patches, 1 per driver, these patches are trivial,
they literally just add const to each definition of a drivers config
structure.

Cheers
/Marcus


Tomasz Bursztyka
 

Hi Marcus,

All of the patches to achieve the above are all now on gerrit, most
have been merged already.

.. with the exception of the DW PWM driver, which so far as I can tell
does not compile for any board at this point in time,
https://jira.zephyrproject.org/browse/ZEP-1040. Is there a board that
this driver should work for?
Currently no.
However, if any pure ARC based boards come in, this might be found useful.

for instance
https://www.synopsys.com/dw/ipdir.php?ds=arc_em_starter_kit

Uses spi_dw, i2c_dw etc..

Tomasz


Francois Bedard <Francois.Bedard@...>
 

Hi,

The ARC EM Starter Kit board support is already in Zephyr as of release 1.4 (added by Chuck Jordan) but it doesn't use DW PWM at the moment.

Francois

-----Original Message-----
From: Tomasz Bursztyka [mailto:tomasz.bursztyka(a)linux.intel.com]
Sent: Tuesday, October 11, 2016 4:07 AM
To: devel(a)lists.zephyrproject.org
Subject: [devel] Re: Re: Re: Device driver configuration and driver_data distinction.

Hi Marcus,

All of the patches to achieve the above are all now on gerrit, most
have been merged already.

.. with the exception of the DW PWM driver, which so far as I can tell
does not compile for any board at this point in time,
https://jira.zephyrproject.org/browse/ZEP-1040. Is there a board that
this driver should work for?
Currently no.
However, if any pure ARC based boards come in, this might be found useful.

for instance
https://www.synopsys.com/dw/ipdir.php?ds=arc_em_starter_kit

Uses spi_dw, i2c_dw etc..

Tomasz