Hi Thanks for reply. We make some proxy layer in linux /sys. So for example we have directories: /sys/resets /sys/routers etc. So if user wants to get to know what is the reset reason he doesn't use devmem 0xfff...., he just read the file in /sys/resets/.. If user wants to know some routing path he doesn't read registers via devmem, he only reads some file in /sys/routers which gives him output. So this is in order to facilitate reading/writing to registers. Instead of searching in documentation concrete registers users have some helper proxy layer in sysfs. And for each directory in /sys there is separate kernel module. Division is based on some functionalities like resets, routing etc. And the problem is should this 6th kernel module perform role of getting access to all registers or only to parts which are used by more than one kernel module? Now I am using second approach but when I am looking at the code it looks terrible. For example #define for register X and function to access it, is in module A, for egister Y in module B, for register Z in 6th kernel module. There is mess I think and maybe better approach is to put all API to 6th kernel module? wt., 16.06.2020, 15:27 użytkownik Greg KH <greg@kroah.com> napisał:

On Tue, Jun 16, 2020 at 03:18:02PM +0200, Tomek The Messenger wrote:

...

Hi I am in the middle of implementation of various external kernel modules. All kernel modules touch some board specific registers. For example let's say we have 3 groups of register areas: #define REG1_ADDR (0xFDEA7000) #define REG1 _SIZE (0x3C0) #define REG2_ADDR (0xFDEC4000) #define REG2 _SIZE (0xFF8) #define REG3_ADDR (0xFDEA6000) #define REG3_SIZE (0x80) And let's say we have 5 kernel modules: A B C D E

Kernel module A touches only one 32-bit register X from REG1 space and a lot of registers from REG2 space. Kernel modules B,C,D touches various registers from REG1 and REG2 space but not this one which kernel module A touches. Kernel module E is the only one who touches registers from REG3 space. No other kernel module touches REG3 space.

If I would implement the code in C++ in user space I would create dynamic library for access to REG1,REG2,REG3 register space. Each 32-bit register would have his own separate function to read,write to register. And then each user space process A-E would be linked against this library during compilation. When programming in kernel I don't know what I should do. Should I implement 6th external kernel module which implements all operations on access to registers REG1,REG2,REG3??? There will be a lot of EXPORT_SYMBOL_GPL. Or maybe this approach is NOK, I mean it is good but I should only place in 6th helper kernel module functions which are used by more than one kernel module. So for example functions for getting access to REG3 address space or reg X from REG1 space should be not there. They should be only in kernel module code A/E. Currently this is code which I implemented. In 6th helper kernel module there are only registers which are used by more than one kernel module. But I am not sure if it is good approach. Maybe there should be there all registers access?

Could anybody help how to organize code here.

Why is this so many different modules? Why not just make it one?

thanks,

greg k-h

OK, so I had to wrongly described something because in my company this is popular approach to use /sys as some abstract layer to get access to registers. So maybe in other words. Let's say You have many socs. In one soc reset_reason can be gotten from devmem 0xfff.... In other soc reset_reason can be gotten through i2c read from some device. Apart from reset_reason You have many other functionalities like kernel boot source (tftp, spi). And let's assume you are tester. It is hard for those people to remember addresses, remembering and checking all the time how to read reset_reason or routing, then extract value from proper bits. Instead of raw addresses they have interface in /sys in the form of files. For example they read cat /sys/resetreg/reason as output they got in string format for example COLD_REBOOT. They read: /sys/bootreg/source As output they got in string format TFTP. And that is on each soc. This is good that registers, the way of access is hidden under interface. It is easier and faster to debug anything with such approach even for developers not only testers. And there is separate kernel module which implements read/write access to each of directory in /sys. So we use here linux API: kobj = kobject_create_and_add("bootreg", NULL); sysfs_create_groups(kobj, ...) So hope that You have now good understanding. I cannot show the code as I don't if I am allowed. I am about half an year in kernel development team, so sometimes I have some doubts. And now my doubts are where to put API for read/write operations as I mentioned above. On Wed, Jun 17, 2020 at 10:55 AM Greg KH <greg@kroah.com> wrote:

On Wed, Jun 17, 2020 at 09:58:03AM +0200, Tomek The Messenger wrote:

...

Hi Thanks for reply. We make some proxy layer in linux /sys. So for example we have directories: /sys/resets /sys/routers

Really? That's a total abuse of sysfs, don't do that.

Seriously, that's not ok at all.

...

etc. So if user wants to get to know what is the reset reason he doesn't use devmem 0xfff...., he just read the file in /sys/resets/.. If user wants to know some routing path he doesn't read registers via devmem, he only reads some file in /sys/routers which gives him output. So this is in order to facilitate reading/writing to registers. Instead of searching in documentation concrete registers users have some helper proxy layer in sysfs. And for each directory in /sys there is separate kernel module. Division is based on some functionalities like resets, routing etc. And the problem is should this 6th kernel module perform role of getting access to all registers or only to parts which are used by more than one kernel module? Now I am using second approach but when I am looking at the code it looks terrible. For example #define for register X and function to access it, is in module A, for egister Y in module B, for register Z in 6th kernel module. There is mess I think and maybe better approach is to put all API to 6th kernel module?

I really do not understand at all, sorry. Why are you using sysfs for something that it is not designed for? And sysfs directory structure has nothing to do with kernel module names/structures.

Do you have a pointer to your code somewhere so that we can review it and suggest the correct apis you should be using instead?

thanks,

greg k-h

On Wed, Jun 17, 2020 at 11:17:28AM +0200, Tomek The Messenger wrote:

OK, so I had to wrongly described something because in my company this is popular approach to use /sys as some abstract layer to get access to registers.

Please do not do that. There are valid kernel apis to grant access to registers easily, and creating random files and directories in the root of sysfs is not only the incorrect way of doing it, you are making more work for yourself to do so, adding more code that is not needed. Do you have a pointer to the code so that we can provide help with showing the "correct" way to do all of this? thanks, greg k-h

On Wed, 17 Jun 2020 12:39:08 +0200, Tomek The Messenger said:

OK, I will ask if it is possible to share the code.

Well... if you planned to ship that code and that hardware outside your organization, it's going to have to be GPLv2-compatible.

Hello Greg and all, Thus wrote Greg KH (greg@kroah.com):

Please do not do that. There are valid kernel apis to grant access to registers easily,

the most simple case would be a "reset reason" register within the chip's address space. A hand-crafted driver would ioremap the region and implement a sysfs show method that reads the register. What do you recommend for providing read access to such a register via sysfs? Is this a job for the userspace I/O (UIO) subsystem? Thanks, Martin

On Wed, 17 Jun 2020 14:48:36 +0200, Tomek The Messenger said:

This is the case about which Martin write shortly. Then let's assume on another soc reset reason is not stored in chip's address space memory mapped to address 0xfff.... but it is accessed via some spi operation. On another soc reset reason is still memory mapped but to different address 0xfff... And then let's assume there are 30-40 such functionalities like reset_reason.

Or you could hire hardware engineers who don't create designs that look like they were done by crack-addicted howler monkeys, but instead did sane designs that were easier to program. You know, like being consistent with whether the registers are SPI or not, and so on.

If we have one unique sysfs interface then it is easier for everyone: tester, developer to proceed with such device, testing or debugging.

No - not really. Because if you're mapping 3 or 4 SOC resets reasons to one thing, you then have to write code that says "If it's SoC 1, then a bit 12 being set was a memory error, but if it's SOC 2 bit 12 means a PCI bus reset and a memory parity error is bit 17, but a memory ECC error is bit 4." And if the meaning of the reset register is different, a lot of *other* things are probably different too, which is an argument for just having separate drivers for each SoC.

If we have one unique sysfs interface then it is easier for everyone: tester, developer to proceed with such device, testing or debugging.

...

No - not really. Because if you're mapping 3 or 4 SOC resets reasons to one thing, you then have to write code that says "If it's SoC 1, then a bit 12 being set was a memory error, but if it's SOC 2 bit 12 means a PCI bus reset and a memory parity error is bit 17, but a memory ECC error is bit 4."

...

And if the meaning of the reset register is different, a lot of *other* things are probably different too, which is an argument for just having separate drivers for each SoC.

Yes separate drivers but the actions do by them is to create the same files in sysfs. So if somebody use intel, texas instrument or xilinx soc then in order to read reset reason or other stuff he will use sysfs. So this will look likes: /sys/resetreg - 3 separate drivers plus one core driver where there is common part /sys/otherfunctionality- as Above Apart from above 3 drivers where there is api to access registers from ti, Intel, xilinx. This was original question if place here all api reads/writes or only common between sysfs kernel modules. ... /sys/functionalityN -> separate driver for intel, TI, Xilinx On Wed, Jun 17, 2020 at 3:34 PM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:

Hello,

On 6/17/20 2:48 PM, Tomek The Messenger wrote:

...

This is the case about which Martin write shortly. Then let's assume on another soc reset reason is not stored in chip's address space memory mapped to address 0xfff.... but it is accessed via some spi operation. On another soc reset reason is still memory mapped but to different address 0xfff... And then let's assume there are 30-40 such functionalities like reset_reason. If we have one unique sysfs interface then it is easier for everyone: tester, developer to proceed with such device, testing or debugging. You don't waste time to read documentation each time, checking what address is/how to read each functionality. You have this hidden in kernel modules code and can access via cat/echo to /sys.

It also gets more interesting when you have multiple reset reasons in the same system, e.g. SoC power control differentiates between POR, external and Watchdog reset and Watchdog OTOH differentiates between watchdog reset and regular reset and then you might have an external PMIC with an integrated watchdog that overrides both.

In projects I've been involved in, the bootloader did the boot reason computation[1] and was the one to act on it, so communication to the kernel wasn't of high importance. It would be great to have:

1) an upstream device tree binding for reset reason communication from bootloader to Linux 2) a mainline User API to query the reset reason 3) a mainline in-kernel API for drivers to set reset reasons

Watchdog drivers already have a WDIOF_CARDRESET flag that could be folded into this. If you feel up to the task and go along with upstreaming it, you'll save yourself (and future colleagues who need to debug locking problems between your module and mainline kernel drivers) a lot of hassle.

[1]: https://barebox.org/doc/latest/user/reset-reason.html

Cheers Ahmad

...

On Wed, Jun 17, 2020 at 2:36 PM Martin Kaiser <lists@kaiser.cx> wrote:

...

Hello Greg and all,

Thus wrote Greg KH (greg@kroah.com):

...

Please do not do that. There are valid kernel apis to grant access to registers easily,

the most simple case would be a "reset reason" register within the chip's address space. A hand-crafted driver would ioremap the region and implement a sysfs show method that reads the register.

What do you recommend for providing read access to such a register via sysfs? Is this a job for the userspace I/O (UIO) subsystem?

Thanks, Martin

_______________________________________________ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

_______________________________________________ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

-- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

On Wed, 17 Jun 2020 16:26:49 +0200, Tomek The Messenger said:

Yes separate drivers but the actions do by them is to create the same files in sysfs. So if somebody use intel, texas instrument or xilinx soc then in order to read reset reason or other stuff he will use sysfs. So this will look likes: /sys/resetreg - 3 separate drivers plus one core driver where there is common part

That should be /sys/something/something/resetreg where the somethings put it in the same place in the /sys tree as all the other sysfs entries for your devices.