Page MenuHomeFreeBSD

Add ACPI GPIO Controller driver for AMD Platforms
ClosedPublic

Authored by rajfbsd_gmail.com on Aug 23 2018, 1:10 PM.

Details

Summary

This is the driver for ACPI attached GPIO controller in AMD Platforms. This driver is basically a port of "drivers/pinctrl/pinctrl-amd.c" driver in Linux. This patch has the basic functions of GPIO, leaving interrupt related support.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Hi Gonzo, Did you get a chance to look at this?

gonzo added a comment.Sep 2 2018, 5:37 AM

Hi Gonzo, Did you get a chance to look at this?

Not yet. HEAD is in freeze for 12.0 release anyway, so it can't be committed at the moment. I'll look into it next week.

Hi Gonzo,

Sorry to trouble again. Did you get a chance to look at this? From the release schedule, seems FreeBSD 12.0 is scheduled for release in Nov 2018. By any chance, could this patch be taken before 12.0 release?

gonzo added inline comments.Sep 17 2018, 10:18 PM
sys/dev/amdgpio/amdgpio.c
164 ↗(On Diff #47163)

This line is redundant. The value is overwritten by the next line.

186 ↗(On Diff #47163)

This one is also redundant.

362 ↗(On Diff #47163)

sc_bank_prefix is assigned but never used. I think since there is just one bank it's better to remove this field altogether.

sys/dev/amdgpio/amdgpio.h
116 ↗(On Diff #47163)

All static variables should be moved to C file from the header. There is no reason to keep it in .h file as far as I can see.

Hi Gonzo,
Sorry to trouble again. Did you get a chance to look at this? From the release schedule, seems FreeBSD 12.0 is scheduled for release in Nov 2018. By any chance, could this patch be taken before 12.0 release?

No worries, sorry for the delay. Unfortunately, this driver can't make it to 12.0 since the HEAD branch is in code freeze starting from August 24th. During the code freeze, only bugfixes are allowed to be committed with explicit approval by the release engineering team. Technically new features should be avoided even during the code slush but it's less strictly enforced comparing to the code freeze. Details about the release process can be found here: https://www.freebsd.org/releases/12.0R/schedule.html

Once the code is cleaned-up and code freeze is over I'll commit it to the HEAD (which will be 13-CURRENT at that point) and MFC to older branches.

rajfbsd_gmail.com marked 4 inline comments as done.
  1. Removed static keywords from pin grouping definitions in H file.
  2. Moved static struct "resource_spec" to C file.
  3. Removed unwanted static declaration of probe, attach and detach in H file.
  4. Removed redundant file in C file

Hi Gonzo,

Please find attached the new revision for this patch. Also, please look at the inline comments and let me know if you have any feedback.

One question, I have used tabs for spacing in H file. When I look at the file in vim, things are aligned. But here(in phabricator), I see misalignment in macros definition. Is this normal with phabricator?

Thanks.

sys/dev/amdgpio/amdgpio.c
362 ↗(On Diff #47163)

This driver has 4 banks (please see sc_nbanks above). There is some plan to implement a "show" routine later for this driver, which shows the pin details bank wise. That's why I have the definitions for sc_banks and sc_bank_prefix. But, if you insist this being removed for now (since it's not used now), I will remove it.

We need to implement functions to configure gpio pins as interrupt line. For that, we need the INTRNG framework support for amd64 platforms. Since, it's not available now, we haven't done it now. Once this is implemented, we will have the "show" routine as well.

sys/dev/amdgpio/amdgpio.h
116 ↗(On Diff #47163)

Originally intent is to keep functions and pin group structure definitions seperately in C and H files respectively. So, removed the static keywords from the header file. But,

a) Removed the unwanted static declaration of probe, attach and detach function in H file and made appropriate changes in C file (in attach routine, not calling detach in case of failure)

b) Moved "resource_spec" definition alone to C file (as done in most of the drivers)

Thanks for taking time to review the first patch and clarifying on the release process.

gonzo added a comment.Sep 19 2018, 8:37 PM

One question, I have used tabs for spacing in H file. When I look at the file in vim, things are aligned. But here(in phabricator), I see misalignment in macros definition. Is this normal with phabricator?

I think sometimes phabricator misaligns things but in this case, there are mixes of spaces/tabs in header file that violate style(9). I'll post comments separately. Thanks for pointing it out.

sys/dev/amdgpio/amdgpio.h
116 ↗(On Diff #47163)

I misunderstood the intention then, sorry. My line of thinking was: if it's a static variable and used only in one compilation unit, it's better to be moved to C file because it's naturally belongs there - like a resources definition and internal methods declarations. It wasn't the "static" keyword I objected to, it was the location of the data itself. But if you want to keep pins information in the header file as a kind of constant for code readability purpose then the original usage of the "static" keyword was correct and my comment was wrong. Please add "static" back to kernzp_pins, kernzp_groups, and *_pins constants.

gonzo added inline comments.Sep 19 2018, 8:45 PM
sys/dev/amdgpio/amdgpio.h
28 ↗(On Diff #48164)

style(9) requires tab character after the #define. There are several instances of #define<space> in this file

41 ↗(On Diff #48164)

Values of AMD_GPIO_PINS_BANK0..AMD_GPIO_PINS_BANK4 are aligned using mix of spaces and tabs, should be just tabs

60 ↗(On Diff #48164)

Values in this section are aligned only by spaces, should be tabs.

111 ↗(On Diff #48164)

fields indented using four spaces, should be single tab instead.

258 ↗(On Diff #48164)

There is a tab character after every "}," in this array. There should be no trailing spaces.

There are also several instances of trailing spaces/tabs in C file too

trasz added a subscriber: trasz.Sep 20 2018, 8:09 PM

Hi Gonzo,
Sorry to trouble again. Did you get a chance to look at this? From the release schedule, seems FreeBSD 12.0 is scheduled for release in Nov 2018. By any chance, could this patch be taken before 12.0 release?

No worries, sorry for the delay. Unfortunately, this driver can't make it to 12.0 since the HEAD branch is in code freeze starting from August 24th. During the code freeze, only bugfixes are allowed to be committed with explicit approval by the release engineering team. Technically new features should be avoided even during the code slush but it's less strictly enforced comparing to the code freeze. Details about the release process can be found here: https://www.freebsd.org/releases/12.0R/schedule.html
Once the code is cleaned-up and code freeze is over I'll commit it to the HEAD (which will be 13-CURRENT at that point) and MFC to older branches.

To be honest, the current code freeze seems quite... flexible. As in, there's certainly some new stuff coming in, and from my understanding adding a new driver (as opposed to modifying something that's already there) is rather unlikely to break something. Maybe just send an email to re@ and see what they say?

rajfbsd_gmail.com marked 6 inline comments as done.
  1. Added the static keywords to those mentioned structures in H file, as the intent is code readability
  2. Cleared the unwanted whitespaces/tabs and followed style(9) format for indentation in both C and H file.

Hi Olexsandr,

Addressed all your comments. Also, taken care of the whitespace and tabs formatting. Please see if anything against the style guidelines still. Thanks for your time again.

rajfbsd_gmail.com marked an inline comment as done.Sep 21 2018, 10:54 AM

Hi Oleksandr,

Any other comments on this?

gonzo added a comment.Oct 4 2018, 7:02 PM

Hi Oleksandr,
Any other comments on this?

Hi Rajesh,

The code looks fine now. One thing though, it's disconnected from the build. You might want to take a look at sys/modules/Makefile and search for _bytgpio string to see how you can make it x86_64 only

gonzo added a comment.Oct 4 2018, 7:38 PM

Sorry for delays with answers

To be honest, the current code freeze seems quite... flexible. As in, there's certainly some new stuff coming in, and from my understanding adding a new driver (as opposed to modifying something that's already there) is rather unlikely to break something. Maybe just send an email to re@ and see what they say?

Yes, and IMHO this flexibility is not a very good thing (speaking from personal experience as a part-time RE at $WORK). I don't want to submit more new feature changes to re@ that is strictly necessary and so far I can't find pro arguments for this driver. As far as I understand this driver is going to be used as a building block for other components that are going to use it either as a control method for peripherals or as an interrupt source. If I am wrong and there are readily available immediate use cases, for instance, AMD-based embedded platforms where GPIO can be used from the userland or AMD-based laptops where gpioctl can be used directly to enable certain functionality that is otherwise unavailable I'll bring the patch up to re@ for the consideration. Otherwise, I'd rather wait for the code thaw and submit it to -HEAD with further MFCs to -12 and -11, it will be available in 12.1 in a year or so.

Rajesh, if you're familiar with the solutions available on the market could you share such use cases so I could argue for the inclusion of the driver?

Thanks

Hi Oleksandr,

Sorry about the delayed response.

Addressed your comments regarding build in "sys/modules/Makefile" and "sys/conf/files.amd64". With these changes, tried compiling the kernel with and without "gpio" and "amdgpio" in the custom kernel config file. When "amdgpio" is added as part of kernel config, faced some compilation issue because of the "ofw" related header file inclusions. So, removed that, as that is unnecessary here. With all these changes, things goes fine.

Also, made the following minor changes. Sorry about having these after all the review. But I feel it's worth it.

  1. Added a check for "Output" capability of the pin "pin_set" and "pin_toggle" routines in order to make writing to "Input" pin Invalid.
  2. Made minor change to "pin name" setting in "amdgpio_attach" to have appropriate bank number in the name.

Sorry for delays with answers

To be honest, the current code freeze seems quite... flexible. As in, there's certainly some new stuff coming in, and from my understanding adding a new driver (as opposed to modifying something that's already there) is rather unlikely to break something. Maybe just send an email to re@ and see what they say?

Yes, and IMHO this flexibility is not a very good thing (speaking from personal experience as a part-time RE at $WORK). I don't want to submit more new feature changes to re@ that is strictly necessary and so far I can't find pro arguments for this driver. As far as I understand this driver is going to be used as a building block for other components that are going to use it either as a control method for peripherals or as an interrupt source. If I am wrong and there are readily available immediate use cases, for instance, AMD-based embedded platforms where GPIO can be used from the userland or AMD-based laptops where gpioctl can be used directly to enable certain functionality that is otherwise unavailable I'll bring the patch up to re@ for the consideration. Otherwise, I'd rather wait for the code thaw and submit it to -HEAD with further MFCs to -12 and -11, it will be available in 12.1 in a year or so.
Rajesh, if you're familiar with the solutions available on the market could you share such use cases so I could argue for the inclusion of the driver?
Thanks

Hi Oleksandr,

Typical use cases we have come across so far is to use it as a interrupt source for few devices and LED functions. For now, we have just enabled the driver for basic IO operations. It would be good if we have this included in 12.0, but if it's really not good to have new feature/driver sent to @re at this point, we can still manage with MFC to stable/12 and stable/11.

gonzo added a comment.Oct 11 2018, 7:54 PM

This is the only change left I believe

sys/modules/Makefile
742 ↗(On Diff #48898)

The list of the conditional modules is sorted alphabetically, this line should go right after the .if ${MACHINE_CPUARCH} == "amd64"

Hi Oleksandr,

Addressed the comment about alphabetical order in "sys/modules/Makefile". Seems, "sys/conf/files.amd64" also needs the modules to be listed in alphabetical order. Corrected that as well.

gonzo accepted this revision.Oct 18 2018, 7:37 AM

Looks good. I'll commit it once the freeze is over

This revision is now accepted and ready to land.Oct 18 2018, 7:37 AM
This revision was automatically updated to reflect the committed changes.