| Message ID | 20190704090205.19400-1-fe@dev.tdt.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | Update pcengines-apuv2 platform device | expand |
On Thu, Jul 4, 2019 at 12:02 PM Florian Eckert <fe@dev.tdt.de> wrote: > > This patchset adds the following changes to this pcengines-apuv2 > platform device. > Before doing anything to this driver, what is the plan for previously upstreamed: drivers/leds/leds-apu.c arch/x86/platform/geode/alix.c ? > * Add mpcie reset gpio export > * Add legacy leds gpio definitions > * Update gpio buttion definitions > > Florian Eckert (3): > platform/x86/pcengines-apuv2: add mpcie reset gpio export > platform/x86/pcengines-apuv2: add legacy leds gpio definitions > platform//x86/pcengines-apuv2: update gpio button definition > > drivers/platform/x86/pcengines-apuv2.c | 32 ++++++++++++++++++++++++++++---- > 1 file changed, 28 insertions(+), 4 deletions(-) > > -- > 2.11.0 >
Hello Andy, thanks for feedback >> This patchset adds the following changes to this pcengines-apuv2 >> platform device. >> > > Before doing anything to this driver, what is the plan for previously > upstreamed: > > drivers/leds/leds-apu.c I think we can remove the related APU2/APU3 code stuff from this driver. The recently added pcengines-apuv2 driver does *not* support the APU1. So I think we need the related APU1 stuff if we still want to support this board. > arch/x86/platform/geode/alix.c I think this is not related because this is a different platform driver. Maybe we should move them to drivers/platform/x86? Regards Florian
On Thu, Jul 4, 2019 at 5:00 PM Florian Eckert <fe@dev.tdt.de> wrote: > > Hello Andy, > > thanks for feedback > > >> This patchset adds the following changes to this pcengines-apuv2 > >> platform device. > >> > > > > Before doing anything to this driver, what is the plan for previously > > upstreamed: > > > > drivers/leds/leds-apu.c > > I think we can remove the related APU2/APU3 code stuff from this driver. > The recently added pcengines-apuv2 driver does *not* support the APU1. > So I think we need the related APU1 stuff if we still want to support > this board. So, I would like to see some unification (since it's material for v5.4 cycle anyway, we have time). > > arch/x86/platform/geode/alix.c > > I think this is not related because this is a different platform driver. > Maybe we should move them to drivers/platform/x86? You mentioned somewhere ALIx, can you elaborate if these are platforms of the same family (PC engines)? Looking into the code, I think we may unify all three under umbrella of one driver if the above is true. -- With Best Regards, Andy Shevchenko
Hello Andy >> >> This patchset adds the following changes to this pcengines-apuv2 >> >> platform device. >> >> >> > >> > Before doing anything to this driver, what is the plan for previously >> > upstreamed: >> > >> > drivers/leds/leds-apu.c >> >> I think we can remove the related APU2/APU3 code stuff from this >> driver. >> The recently added pcengines-apuv2 driver does *not* support the APU1. >> So I think we need the related APU1 stuff if we still want to support >> this board. > > So, I would like to see some unification (since it's material for v5.4 > cycle anyway, we have time). A few thoughts and information about your suggestion to unify this. APU1 (PC-Engines) CPU "AMD G series T40E APU": This is also an old design and is not recommend for new design (deprecated). Also not many were produced and are in the field. See https://pcengines.ch/apu.htm Platform-Device (LEDs, Button): I have no platform device description found in the linux sources. So the GPIO button should not work. LEDs-Driver: Only the LEDs should work with this device driver. This is shared additonal with new APU2/APU3. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/leds/leds-apu.c I think we should remove the APU2/APU3 stuff. This will now be handled by the new gpio-amd-fch.c / pcengines-apuv2.c kombination. APU2/APU3/APU4 (PC-Engines) CPU "AMD Embedded G series GX-412TC": This is the newest design and is recommend for new products. See https://pcengines.ch/apu2.htm GPIO-Driver: The following driver is responsible for the GPIO export and handling https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpio-amd-fch.c Platform-Device (LEDs, Button): This Platform description is only valid for APU2/APU3 and not for APU1. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/x86/pcengines-apuv2.c LEDs-Driver: We have an additional device only for LEDs this works for APU1/APU2/APU3. I think we should remove the APU2/APU3 LEDs from the leds-apu device as mentioned above. So this device supports only the APU1 LEDs. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/leds/leds-apu.c We could extend and/or rename the pcengienes-apuv2 device to support also APU3 and the newest APU4. The APU2 does only have LEDs Button and the MPCIE2 reset lines see my patch. The APU3 does have an additional the simswap pin. So the current pcengines-apuv2 platform is from my point of view wrong. We should change this to the following layout and add the legacy GPIO numbering. This are the following GPIOs: APU2: LED1 LED2 LED3 BUTTON MPCIE2 MPCIE3 APU3: LED1 LED2 LED3 BUTTON MPCIE2 MPCIE3 SIMSWAP APU4: TODO >> > arch/x86/platform/geode/alix.c >> >> I think this is not related because this is a different platform >> driver. >> Maybe we should move them to drivers/platform/x86? > > You mentioned somewhere ALIx, can you elaborate if these are platforms > of the same family (PC engines)? > > Looking into the code, I think we may unify all three under umbrella > of one driver if the above is true. ALIX (PC-Engines) CPU "AMD Geode LX": This is an old design we have already in use and is not recommend for new design (deprecated) https://pcengines.ch/alix.htm GPIO-Driver: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpio-cs5535.c Platform-Device (LEDs, button): https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/platform/geode/alix.c I think we should leave the driver as it is because this is a different design and has nothing to do with the PUs. The only thing I can imagine is to move the platform device to "drivers/platform/x86", but this is cosmetic. I have only mentioned the alix board to explain why I think that we should change the APU key code from the GPIO button to unify this. With Best Regards, Florian
On 05.07.19 13:36, Florian Eckert wrote: Hi, > APU1 (PC-Engines) CPU "AMD G series T40E APU":> This is also an old design and is not recommend for new design> (deprecated).> Also not many were produced and are in the field.> See https://pcengines.ch/apu.htm Yes, and I haven't been able to get one yet, so I dropped my original plan of supporting this once. The gpio device is very different. > Platform-Device (LEDs, Button):> I have no platform device description found in the linux sources.> So the GPIO button should not work. Talking about the old driver ? It only supports the three front LEDs directly and conflicts w/ anything else that wants to support other GPIOs. > Platform-Device (LEDs, Button): > This Platform description is only valid for APU2/APU3 and not for APU1. > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/x86/pcengines-apuv2.c Correct. If we really wanna support APUv1, that most likely become a completely separate driver, as the chipsets are very different. > LEDs-Driver: > We have an additional device only for LEDs this works for APU1/APU2/APU3. > I think we should remove the APU2/APU3 LEDs from the leds-apu device as > mentioned above. > So this device supports only the APU1 LEDs. ACK. > We could extend and/or rename the pcengienes-apuv2 device to support > also APU3 and the newest APU4. APUv3 already is supported. APUv4 again is pretty different, and I don't have one yet. (if anybody convices pcengines to give me some boards, I'll add support for them :)) > The APU3 does have an additional the simswap pin. Already planned to add it, but not yet sorted into which subsystem it actually belongs to. > We should change this to the following layout and add the legacy GPIO > numbering. First we'll have to sort out where the new pins actually belong into. (maybe power management / regulators for the reset pins ?) > ALIX (PC-Engines) CPU "AMD Geode LX": > This is an old design we have already in use and is not recommend for > new design (deprecated) > https://pcengines.ch/alix.htm And it's an entirely different chipset, not related at all to apu. > I think we should leave the driver as it is because this is a different > design and has nothing to do with the PUs. Yes. --mtx
On 04.07.19 15:39, Andy Shevchenko wrote: > On Thu, Jul 4, 2019 at 12:02 PM Florian Eckert <fe@dev.tdt.de> wrote: >> >> This patchset adds the following changes to this pcengines-apuv2 >> platform device. >> > > Before doing anything to this driver, what is the plan for previously > upstreamed: > > drivers/leds/leds-apu.c Only supports the three front LEDs, nothing else. (we've got more gpios that are not LEDs, eg. the front button, simsw, ...) > arch/x86/platform/geode/alix.c completely unrelated - very different chipset. --mtx
On 2019-07-08 21:45, Enrico Weigelt, metux IT consult wrote: > On 04.07.19 15:39, Andy Shevchenko wrote: >> On Thu, Jul 4, 2019 at 12:02 PM Florian Eckert <fe@dev.tdt.de> wrote: >>> >>> This patchset adds the following changes to this pcengines-apuv2 >>> platform device. >>> >> >> Before doing anything to this driver, what is the plan for previously >> upstreamed: >> >> drivers/leds/leds-apu.c > > Only supports the three front LEDs, nothing else. (we've got more gpios > that are not LEDs, eg. the front button, simsw, ...) > >> arch/x86/platform/geode/alix.c > > completely unrelated - very different chipset. > > > --mtx I'm going to sum it all what we have ALIX family boards (https://www.pcengines.ch/alix.htm): CPU -> AMD Geode LX CPU Stays as it is different because it has a different CPU APU family boards (https://www.pcengines.ch/apu.htm): CPU -> AMD G series T40E APU Remove the related APU2 family stuff from the LEDs driver https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/leds/leds-apu.c this will be handled in the future by the platform device https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/x86/pcengines-apuv2.c The other GPIOs are not supported by this platform. Only LEDs are supported for now. APU2 family boards (https://www.pcengines.ch/apu2.htm): CPU -> AMD Embedded G series GX-412TC Add the additional mpcie reset pins and add additional board descriptions to https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/x86/pcengines-apuv2.c?h=v5.2#n61 so we can distinguish between the APU2,APU3 and APU4 boards of the APU2 board family. My research in the pcengines documentation shows the following GPIO pins for the individual boards which we can support. APU2: front-led1 front-led2 front-led3 front-button mpcie2_reset mpcie3_reset APU3: front-led1 front-led2 front-led3 front-button mpcie2_reset mpcie3_reset simswap APU4: front-led1 front-led2 front-led3 front-button mpcie2_reset mpcie3_reset Until now we support aAPU2 and APU3 and treat it the same way. But the APU2 does not have a simswap. Kind regards Florian
On Wed, Jul 10, 2019 at 3:54 PM Florian Eckert <fe@dev.tdt.de> wrote: > > On 2019-07-08 21:45, Enrico Weigelt, metux IT consult wrote: > > On 04.07.19 15:39, Andy Shevchenko wrote: > >> On Thu, Jul 4, 2019 at 12:02 PM Florian Eckert <fe@dev.tdt.de> wrote: > >>> > >>> This patchset adds the following changes to this pcengines-apuv2 > >>> platform device. Guys, I'm lost with this series. So, for now I dropped them from queue, if needed, please resend a new version.
On 25.07.19 19:49, Andy Shevchenko wrote: Hi, > On Wed, Jul 10, 2019 at 3:54 PM Florian Eckert <fe@dev.tdt.de> wrote: >> >> On 2019-07-08 21:45, Enrico Weigelt, metux IT consult wrote: >>> On 04.07.19 15:39, Andy Shevchenko wrote: >>>> On Thu, Jul 4, 2019 at 12:02 PM Florian Eckert <fe@dev.tdt.de> wrote: >>>>> >>>>> This patchset adds the following changes to this pcengines-apuv2 >>>>> platform device. > > Guys, I'm lost with this series. > So, for now I dropped them from queue, if needed, please resend a new version. @Andy: please take my patch for the keycode change, which I sent separately. (Florian's version also added raw gpio numnber). @Florian: could you resend your patch for the reset pin addition ? --mtx