| Message ID | 20200923151511.3842150-1-luzmaximilian@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add support for Microsoft Surface System Aggregator Module | expand |
On Wed, Sep 23, 2020 at 5:15 PM Maximilian Luz <luzmaximilian@gmail.com> wrote: > > Hello, > > The Surface System Aggregator Module (we'll refer to it as Surface > Aggregator or SAM below) is an embedded controller (EC) found on various > Microsoft Surface devices. Specifically, all 4th and later generation > Surface devices, i.e. Surface Pro 4, Surface Book 1 and later, with the > exception of the Surface Go series and the Surface Duo. Notably, it > seems like this EC can also be found on the ARM-based Surface Pro X [1]. I think this should go to drivers/platform/x86 or drivers/platform/surface/ along with other laptop vendor specific code rather than drivers/misc/. I'll have a look at the code myself, but I'd prefer to have the maintainers for the other laptop drivers review this properly. Arnd
On 9/23/20 5:30 PM, Arnd Bergmann wrote: > On Wed, Sep 23, 2020 at 5:15 PM Maximilian Luz <luzmaximilian@gmail.com> wrote: >> >> Hello, >> >> The Surface System Aggregator Module (we'll refer to it as Surface >> Aggregator or SAM below) is an embedded controller (EC) found on various >> Microsoft Surface devices. Specifically, all 4th and later generation >> Surface devices, i.e. Surface Pro 4, Surface Book 1 and later, with the >> exception of the Surface Go series and the Surface Duo. Notably, it >> seems like this EC can also be found on the ARM-based Surface Pro X [1]. > > I think this should go to drivers/platform/x86 or drivers/platform/surface/ > along with other laptop vendor specific code rather than drivers/misc/. I initially had this under drivers/platform/x86. There are two main reasons I changed that: First, I think it's a bit too big for platform/x86 given that it basically introduces a new subsystem. At this point it's really less of "a couple of odd devices here and there" and more of a bus-type thing. Second, with the possibility of future support for ARM devices (Pro X, Pro X 2 which is rumored to come out soon), I thought that platform/x86 would not be a good fit. I'd be happy to move this to platform/surface though, if that's considered a better fit and you're okay with me adding that. Would make sense given that there's already a platform/chrome, which, as far as I can tell, also seems to be mainly focused on EC support. > I'll have a look at the code myself, but I'd prefer to have the maintainers > for the other laptop drivers review this properly. Thanks! I'll CC them for the next version. Regards, Max
On Wed, Sep 23, 2020 at 5:43 PM Maximilian Luz <luzmaximilian@gmail.com> wrote: > > On 9/23/20 5:30 PM, Arnd Bergmann wrote: > > On Wed, Sep 23, 2020 at 5:15 PM Maximilian Luz <luzmaximilian@gmail.com> wrote: > >> > >> Hello, > >> > >> The Surface System Aggregator Module (we'll refer to it as Surface > >> Aggregator or SAM below) is an embedded controller (EC) found on various > >> Microsoft Surface devices. Specifically, all 4th and later generation > >> Surface devices, i.e. Surface Pro 4, Surface Book 1 and later, with the > >> exception of the Surface Go series and the Surface Duo. Notably, it > >> seems like this EC can also be found on the ARM-based Surface Pro X [1]. > > > > I think this should go to drivers/platform/x86 or drivers/platform/surface/ > > along with other laptop vendor specific code rather than drivers/misc/. > > I initially had this under drivers/platform/x86. There are two main > reasons I changed that: First, I think it's a bit too big for > platform/x86 given that it basically introduces a new subsystem. At this > point it's really less of "a couple of odd devices here and there" and > more of a bus-type thing. Second, with the possibility of future support > for ARM devices (Pro X, Pro X 2 which is rumored to come out soon), I > thought that platform/x86 would not be a good fit. I don't see that as a strong reason against it. As you write yourself, the driver won't work on the arm machines without major changes anyway, and even if it does, it fits much better with the rest of it. If you are worried about the size of the directory, drivers/platform/x86/surface/ would also work. > I'd be happy to move this to platform/surface though, if that's > considered a better fit and you're okay with me adding that. Would make > sense given that there's already a platform/chrome, which, as far as I > can tell, also seems to be mainly focused on EC support. Yes, I think the main question is how much overlap you see functionally between this driver and the others in drivers/platform/x86. Arnd
On 9/23/20 9:43 PM, Arnd Bergmann wrote: > On Wed, Sep 23, 2020 at 5:43 PM Maximilian Luz <luzmaximilian@gmail.com> wrote: >> >> On 9/23/20 5:30 PM, Arnd Bergmann wrote: >>> On Wed, Sep 23, 2020 at 5:15 PM Maximilian Luz <luzmaximilian@gmail.com> wrote: >>>> >>>> Hello, >>>> >>>> The Surface System Aggregator Module (we'll refer to it as Surface >>>> Aggregator or SAM below) is an embedded controller (EC) found on various >>>> Microsoft Surface devices. Specifically, all 4th and later generation >>>> Surface devices, i.e. Surface Pro 4, Surface Book 1 and later, with the >>>> exception of the Surface Go series and the Surface Duo. Notably, it >>>> seems like this EC can also be found on the ARM-based Surface Pro X [1]. >>> >>> I think this should go to drivers/platform/x86 or drivers/platform/surface/ >>> along with other laptop vendor specific code rather than drivers/misc/. >> >> I initially had this under drivers/platform/x86. There are two main >> reasons I changed that: First, I think it's a bit too big for >> platform/x86 given that it basically introduces a new subsystem. At this >> point it's really less of "a couple of odd devices here and there" and >> more of a bus-type thing. Second, with the possibility of future support >> for ARM devices (Pro X, Pro X 2 which is rumored to come out soon), I >> thought that platform/x86 would not be a good fit. > > I don't see that as a strong reason against it. As you write yourself, the > driver won't work on the arm machines without major changes anyway, > and even if it does, it fits much better with the rest of it. Sorry, I should have written that a bit more clearly. I don't see any reason why these drivers would not work on an ARM device such as the Pro X right now, assuming that it boots via ACPI and the serial device it loads against is fully functional. The reason (at least as far as I know) it currently hasn't been tested is that a) there aren't a lot of people around attempting to run Linux on the currently only ARM device with that and b) it's currently blocked by a reason unrelated to this driver itself, specifically that the serial controller isn't being set up and thus the core driver doesn't have a device it can attach to. My information may be outdated though and is pretty much exclusively based on https://github.com/Sonicadvance1/linux/issues/7. > If you are worried about the size of the directory, > drivers/platform/x86/surface/ > would also work. This was the alternative I'd have considered without ARM devices. >> I'd be happy to move this to platform/surface though, if that's >> considered a better fit and you're okay with me adding that. Would make >> sense given that there's already a platform/chrome, which, as far as I >> can tell, also seems to be mainly focused on EC support. > > Yes, I think the main question is how much overlap you see functionally > between this driver and the others in drivers/platform/x86. I think that the Pro X likely won't be the last ARM Surface device with a SAM EC. Further, the subsystem is going to grow, and platform/x86 seems more like a collection of, if at all, loosely connected drivers, which might give off the wrong impression. In my mind, this is just a bit more comparable to platform/chrome than the rest of platform/x86. I don't think I'm really qualified to make the decision on that though, that's just my opinion. Here's an overview of other drivers that I hopefully at some point get in good enough shape, which are part of this subsystem/dependent on the EC API introduced here: - A device registry / device hub for devices that are connected to the EC but can't be detected via ACPI. - A dedicated battery driver for 7th generation devices (where the battery isn't hanled via the ACPI shim). - A driver properly handling clipboard detachment on the Surface Books. - A driver for HID input/transport on the Surface Laptops and Surface Book 3. - A driver for allowing users to set the performance/cooling mode via sysfs. - Possibly a driver improving hot-plug handling of the discrete GPU in the Surface Book base. And also some stuff that hasn't been written yet: - A dedicated driver for temperature sensors handled via the EC on 7th generation devices (also handled via the ACPI shim on previous generations). - Possibly a driver for real-time-clock access on 7th generation devices (it has yet to be tested if that interface is still around/required on those devices; that's also a thing handled via the ACPI shim on previous generations). I doubt that those client drivers will be exclusive to x86, and I could see (current and future) ARM devices using SAM based battery, keyboard/HID, and performance mode drivers (which will likely also require the device registry, because for some reason MS doesn't want to describe those devices in ACPI on the newer generations any more...). All of those should work as-is on ARM (or at least after the corresponding device entries have been added to the device registry), modulo bugs of course. I hope this all gives a better overview of the form this may eventually take on and helps you in your decision. I'd be completely happy to move it to either, platform/surface or platform/x86/surface, whatever the consensus is. I'd very much like to keep the client drivers all contained to one sub-directory, though, and not scattered all over platform/x86/surface_*.c. Again that's more of a personal preference though :) Thanks, Max
On Thu, Sep 24, 2020 at 1:28 AM Maximilian Luz <luzmaximilian@gmail.com> wrote: > On 9/23/20 9:43 PM, Arnd Bergmann wrote: > > On Wed, Sep 23, 2020 at 5:43 PM Maximilian Luz <luzmaximilian@gmail.com> wrote: > >> > >> On 9/23/20 5:30 PM, Arnd Bergmann wrote: > >>> On Wed, Sep 23, 2020 at 5:15 PM Maximilian Luz <luzmaximilian@gmail.com> wrote: > >>>> > >>>> Hello, > >>>> > >>>> The Surface System Aggregator Module (we'll refer to it as Surface > >>>> Aggregator or SAM below) is an embedded controller (EC) found on various > >>>> Microsoft Surface devices. Specifically, all 4th and later generation > >>>> Surface devices, i.e. Surface Pro 4, Surface Book 1 and later, with the > >>>> exception of the Surface Go series and the Surface Duo. Notably, it > >>>> seems like this EC can also be found on the ARM-based Surface Pro X [1]. > >>> > >>> I think this should go to drivers/platform/x86 or drivers/platform/surface/ > >>> along with other laptop vendor specific code rather than drivers/misc/. > >> > >> I initially had this under drivers/platform/x86. There are two main > >> reasons I changed that: First, I think it's a bit too big for > >> platform/x86 given that it basically introduces a new subsystem. At this > >> point it's really less of "a couple of odd devices here and there" and > >> more of a bus-type thing. Second, with the possibility of future support > >> for ARM devices (Pro X, Pro X 2 which is rumored to come out soon), I > >> thought that platform/x86 would not be a good fit. > > > > I don't see that as a strong reason against it. As you write yourself, the > > driver won't work on the arm machines without major changes anyway, > > and even if it does, it fits much better with the rest of it. > > Sorry, I should have written that a bit more clearly. I don't see any > reason why these drivers would not work on an ARM device such as the Pro > X right now, assuming that it boots via ACPI and the serial device it > loads against is fully functional. As I understand, the dialect of ACPI used on the snapdragon laptops is not really compatible with the subset expected by the kernel, so you'd be more likely to run those laptops with a device tree description of the hardware instead (if at all). Making the driver talk to the hardware directly instead of going through AML likely requires more refactoring. > >> I'd be happy to move this to platform/surface though, if that's > >> considered a better fit and you're okay with me adding that. Would make > >> sense given that there's already a platform/chrome, which, as far as I > >> can tell, also seems to be mainly focused on EC support. > > > > Yes, I think the main question is how much overlap you see functionally > > between this driver and the others in drivers/platform/x86. > > I think that the Pro X likely won't be the last ARM Surface device with > a SAM EC. Further, the subsystem is going to grow, and platform/x86 > seems more like a collection of, if at all, loosely connected drivers, > which might give off the wrong impression. In my mind, this is just a > bit more comparable to platform/chrome than the rest of platform/x86. I > don't think I'm really qualified to make the decision on that though, > that's just my opinion. I would ask the drivers/platform/x86 maintainers for an opinion here, they are probably best qualified to make that decision. I don't really mind either way, for me this is more about who is responsible as a subsystem maintainer than whether these are technically x86 or not. > Here's an overview of other drivers that I hopefully at some point get > in good enough shape, which are part of this subsystem/dependent on the > EC API introduced here: > > - A device registry / device hub for devices that are connected to the > EC but can't be detected via ACPI. > > - A dedicated battery driver for 7th generation devices (where the > battery isn't hanled via the ACPI shim). > > - A driver properly handling clipboard detachment on the Surface Books. > > - A driver for HID input/transport on the Surface Laptops and Surface > Book 3. > > - A driver for allowing users to set the performance/cooling mode via > sysfs. > > - Possibly a driver improving hot-plug handling of the discrete GPU in > the Surface Book base. Note that drivers that connect to the bus typically don't live in the same subdirectory as the driver that operates the bus. E.g. the battery driver would go into drivers/power/supply and the input would go into drivers/input/ or drivers/hid. Arnd
On Wed, Sep 23, 2020 at 6:32 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Wed, Sep 23, 2020 at 5:15 PM Maximilian Luz <luzmaximilian@gmail.com> wrote: > > > > Hello, > > > > The Surface System Aggregator Module (we'll refer to it as Surface > > Aggregator or SAM below) is an embedded controller (EC) found on various > > Microsoft Surface devices. Specifically, all 4th and later generation > > Surface devices, i.e. Surface Pro 4, Surface Book 1 and later, with the > > exception of the Surface Go series and the Surface Duo. Notably, it > > seems like this EC can also be found on the ARM-based Surface Pro X [1]. > > I think this should go to drivers/platform/x86 or drivers/platform/surface/ > along with other laptop vendor specific code rather than drivers/misc/. +1 here. drivers/platform/surface is a good place to start. And you may begin with moving a few Surface drivers out of PDx86 to the new folder.
On 9/24/20 10:26 AM, Arnd Bergmann wrote: > On Thu, Sep 24, 2020 at 1:28 AM Maximilian Luz <luzmaximilian@gmail.com> wrote: >> On 9/23/20 9:43 PM, Arnd Bergmann wrote: >>> On Wed, Sep 23, 2020 at 5:43 PM Maximilian Luz <luzmaximilian@gmail.com> wrote: >>>> >>>> On 9/23/20 5:30 PM, Arnd Bergmann wrote: >>>>> On Wed, Sep 23, 2020 at 5:15 PM Maximilian Luz <luzmaximilian@gmail.com> wrote: >>>>>> >>>>>> Hello, >>>>>> >>>>>> The Surface System Aggregator Module (we'll refer to it as Surface >>>>>> Aggregator or SAM below) is an embedded controller (EC) found on various >>>>>> Microsoft Surface devices. Specifically, all 4th and later generation >>>>>> Surface devices, i.e. Surface Pro 4, Surface Book 1 and later, with the >>>>>> exception of the Surface Go series and the Surface Duo. Notably, it >>>>>> seems like this EC can also be found on the ARM-based Surface Pro X [1]. >>>>> >>>>> I think this should go to drivers/platform/x86 or drivers/platform/surface/ >>>>> along with other laptop vendor specific code rather than drivers/misc/. >>>> >>>> I initially had this under drivers/platform/x86. There are two main >>>> reasons I changed that: First, I think it's a bit too big for >>>> platform/x86 given that it basically introduces a new subsystem. At this >>>> point it's really less of "a couple of odd devices here and there" and >>>> more of a bus-type thing. Second, with the possibility of future support >>>> for ARM devices (Pro X, Pro X 2 which is rumored to come out soon), I >>>> thought that platform/x86 would not be a good fit. >>> >>> I don't see that as a strong reason against it. As you write yourself, the >>> driver won't work on the arm machines without major changes anyway, >>> and even if it does, it fits much better with the rest of it. >> >> Sorry, I should have written that a bit more clearly. I don't see any >> reason why these drivers would not work on an ARM device such as the Pro >> X right now, assuming that it boots via ACPI and the serial device it >> loads against is fully functional. > > As I understand, the dialect of ACPI used on the snapdragon laptops > is not really compatible with the subset expected by the kernel, so > you'd be more likely to run those laptops with a device tree description > of the hardware instead (if at all). > > Making the driver talk to the hardware directly instead of going through > AML likely requires more refactoring. Oh, I did not know that! Thanks! >>>> I'd be happy to move this to platform/surface though, if that's >>>> considered a better fit and you're okay with me adding that. Would make >>>> sense given that there's already a platform/chrome, which, as far as I >>>> can tell, also seems to be mainly focused on EC support. >>> >>> Yes, I think the main question is how much overlap you see functionally >>> between this driver and the others in drivers/platform/x86. >> >> I think that the Pro X likely won't be the last ARM Surface device with >> a SAM EC. Further, the subsystem is going to grow, and platform/x86 >> seems more like a collection of, if at all, loosely connected drivers, >> which might give off the wrong impression. In my mind, this is just a >> bit more comparable to platform/chrome than the rest of platform/x86. I >> don't think I'm really qualified to make the decision on that though, >> that's just my opinion. > > I would ask the drivers/platform/x86 maintainers for an opinion here, > they are probably best qualified to make that decision. > > I don't really mind either way, for me this is more about who is > responsible as a subsystem maintainer than whether these are > technically x86 or not. I see, okay. I'll ask them and CC them on the next submission. >> Here's an overview of other drivers that I hopefully at some point get >> in good enough shape, which are part of this subsystem/dependent on the >> EC API introduced here: >> >> - A device registry / device hub for devices that are connected to the >> EC but can't be detected via ACPI. >> >> - A dedicated battery driver for 7th generation devices (where the >> battery isn't hanled via the ACPI shim). >> >> - A driver properly handling clipboard detachment on the Surface Books. >> >> - A driver for HID input/transport on the Surface Laptops and Surface >> Book 3. >> >> - A driver for allowing users to set the performance/cooling mode via >> sysfs. >> >> - Possibly a driver improving hot-plug handling of the discrete GPU in >> the Surface Book base. > > Note that drivers that connect to the bus typically don't live in the > same subdirectory as the driver that operates the bus. E.g. the > battery driver would go into drivers/power/supply and the input > would go into drivers/input/ or drivers/hid. Right. I wonder if this also holds for devices that are directly dependent on a special platform though? It could make sense to have them under plaform/surface rather than in the individual subsystems as they are only ever going to be used on this platform. On the other hand, one could argue that having them in the subsystem directories is better for maintainability. Thanks, Max
On 9/24/20 10:30 AM, Andy Shevchenko wrote: > On Wed, Sep 23, 2020 at 6:32 PM Arnd Bergmann <arnd@arndb.de> wrote: >> >> On Wed, Sep 23, 2020 at 5:15 PM Maximilian Luz <luzmaximilian@gmail.com> wrote: >>> >>> Hello, >>> >>> The Surface System Aggregator Module (we'll refer to it as Surface >>> Aggregator or SAM below) is an embedded controller (EC) found on various >>> Microsoft Surface devices. Specifically, all 4th and later generation >>> Surface devices, i.e. Surface Pro 4, Surface Book 1 and later, with the >>> exception of the Surface Go series and the Surface Duo. Notably, it >>> seems like this EC can also be found on the ARM-based Surface Pro X [1]. >> >> I think this should go to drivers/platform/x86 or drivers/platform/surface/ >> along with other laptop vendor specific code rather than drivers/misc/. > > +1 here. drivers/platform/surface is a good place to start. > And you may begin with moving a few Surface drivers out of PDx86 to > the new folder. Perfect, thanks! I'll draft up a patch series over the weekend. A couple questions regarding structure and maintenance: - Should I CC the platform-driver-x86 list on future submissions to drivers/platform/surface? I.e. is this something you would want to review if it doesn't touch the drivers/platform/x86 directory? - How would you want the layout to be, specifically regarding to the surface-aggregator stuff? My suggestion would be simply: drivers/platform/surface/ surface_aggregator/ Kconfig Makefile core.c controller.c ... (all core stuff built into the surface_aggregator module) Kconfig Makefile surface_aggregator_debugfs.c surface_acpi_notify.c surface_*.c (any other surface platform driver as well as drivers dependent on surface_aggregator) - Regarding future things like HID transport driver, battery/AC driver: Submit them to drivers/platform/surface or to their respective subsystem directories? Thanks, Max
On Thu, Sep 24, 2020 at 8:59 PM Maximilian Luz <luzmaximilian@gmail.com> wrote: > On 9/24/20 10:26 AM, Arnd Bergmann wrote: > > On Thu, Sep 24, 2020 at 1:28 AM Maximilian Luz <luzmaximilian@gmail.com> wrote: > > Note that drivers that connect to the bus typically don't live in the > > same subdirectory as the driver that operates the bus. E.g. the > > battery driver would go into drivers/power/supply and the input > > would go into drivers/input/ or drivers/hid. > > Right. I wonder if this also holds for devices that are directly > dependent on a special platform though? It could make sense to have them > under plaform/surface rather than in the individual subsystems as they > are only ever going to be used on this platform. On the other hand, one > could argue that having them in the subsystem directories is better for > maintainability. Yes, absolutely. The subsystem maintainers are the ones that are most qualified of reviewing code that uses their subsystem, regardless of which bus is used underneath the device, and having all drivers for a subsystem in one place makes it much easier to refactor them all at once in case the internal interfaces are changed or common bugs are found in multiple drivers. Arnd
On 9/24/20 9:38 PM, Arnd Bergmann wrote: > On Thu, Sep 24, 2020 at 8:59 PM Maximilian Luz <luzmaximilian@gmail.com> wrote: >> On 9/24/20 10:26 AM, Arnd Bergmann wrote: >>> On Thu, Sep 24, 2020 at 1:28 AM Maximilian Luz <luzmaximilian@gmail.com> wrote: > >>> Note that drivers that connect to the bus typically don't live in the >>> same subdirectory as the driver that operates the bus. E.g. the >>> battery driver would go into drivers/power/supply and the input >>> would go into drivers/input/ or drivers/hid. >> >> Right. I wonder if this also holds for devices that are directly >> dependent on a special platform though? It could make sense to have them >> under plaform/surface rather than in the individual subsystems as they >> are only ever going to be used on this platform. On the other hand, one >> could argue that having them in the subsystem directories is better for >> maintainability. > > Yes, absolutely. The subsystem maintainers are the ones that are > most qualified of reviewing code that uses their subsystem, regardless > of which bus is used underneath the device, and having all drivers > for a subsystem in one place makes it much easier to refactor them > all at once in case the internal interfaces are changed or common bugs > are found in multiple drivers. Got it. Thank you for bearing with me and answering all my (probably a bit silly) questions! I really appreciate it! Regards, Max
On Thu, Sep 24, 2020 at 10:38 PM Arnd Bergmann <arnd@arndb.de> wrote: > On Thu, Sep 24, 2020 at 8:59 PM Maximilian Luz <luzmaximilian@gmail.com> wrote: > > On 9/24/20 10:26 AM, Arnd Bergmann wrote: > > > On Thu, Sep 24, 2020 at 1:28 AM Maximilian Luz <luzmaximilian@gmail.com> wrote: > > > > Note that drivers that connect to the bus typically don't live in the > > > same subdirectory as the driver that operates the bus. E.g. the > > > battery driver would go into drivers/power/supply and the input > > > would go into drivers/input/ or drivers/hid. > > > > Right. I wonder if this also holds for devices that are directly > > dependent on a special platform though? It could make sense to have them > > under plaform/surface rather than in the individual subsystems as they > > are only ever going to be used on this platform. On the other hand, one > > could argue that having them in the subsystem directories is better for > > maintainability. > > Yes, absolutely. The subsystem maintainers are the ones that are > most qualified of reviewing code that uses their subsystem, regardless > of which bus is used underneath the device, and having all drivers > for a subsystem in one place makes it much easier to refactor them > all at once in case the internal interfaces are changed or common bugs > are found in multiple drivers. The problem is that some of the drivers are mostly reincarnation of board files due to the platform being Windows-oriented with badly written ACPI tables / firmware as a whole (which means a lot of quirks are required).
On Thu, Sep 24, 2020 at 10:17 PM Maximilian Luz <luzmaximilian@gmail.com> wrote: > On 9/24/20 10:30 AM, Andy Shevchenko wrote: > > On Wed, Sep 23, 2020 at 6:32 PM Arnd Bergmann <arnd@arndb.de> wrote: > >> On Wed, Sep 23, 2020 at 5:15 PM Maximilian Luz <luzmaximilian@gmail.com> wrote: ... > >> I think this should go to drivers/platform/x86 or drivers/platform/surface/ > >> along with other laptop vendor specific code rather than drivers/misc/. > > > > +1 here. drivers/platform/surface is a good place to start. > > And you may begin with moving a few Surface drivers out of PDx86 to > > the new folder. > > Perfect, thanks! I'll draft up a patch series over the weekend. > > A couple questions regarding structure and maintenance: > > - Should I CC the platform-driver-x86 list on future submissions to > drivers/platform/surface? I.e. is this something you would want to > review if it doesn't touch the drivers/platform/x86 directory? Include PDx86 mailing list to the list of that. Current SURFACE* drivers have per driver record in MAINTAINERS IIRC. So, update them as well if needed. > - How would you want the layout to be, specifically regarding to the > surface-aggregator stuff? My suggestion would be simply: > > drivers/platform/surface/ > surface_aggregator/ Don't repeat parts of the path, the aggregator is enough as a folder name, but the driver of course should be in its own namespace ('surface'). > Kconfig > Makefile > core.c > controller.c > ... (all core stuff built into the surface_aggregator module) > Kconfig > Makefile > surface_aggregator_debugfs.c (Not sure why it's not a part of aggregator folder) > surface_acpi_notify.c > surface_*.c (any other surface platform driver as well > as drivers dependent on surface_aggregator) > > - Regarding future things like HID transport driver, battery/AC driver: > Submit them to drivers/platform/surface or to their respective > subsystem directories? Respective subsystem _if_ it is a subsystem related driver and not kinda board file. Use common sense and existing examples.
On 9/25/20 4:58 PM, Andy Shevchenko wrote: > On Thu, Sep 24, 2020 at 10:17 PM Maximilian Luz <luzmaximilian@gmail.com> wrote: >> On 9/24/20 10:30 AM, Andy Shevchenko wrote: >>> On Wed, Sep 23, 2020 at 6:32 PM Arnd Bergmann <arnd@arndb.de> wrote: >>>> On Wed, Sep 23, 2020 at 5:15 PM Maximilian Luz <luzmaximilian@gmail.com> wrote: > > ... > >>>> I think this should go to drivers/platform/x86 or drivers/platform/surface/ >>>> along with other laptop vendor specific code rather than drivers/misc/. >>> >>> +1 here. drivers/platform/surface is a good place to start. >>> And you may begin with moving a few Surface drivers out of PDx86 to >>> the new folder. >> >> Perfect, thanks! I'll draft up a patch series over the weekend. >> >> A couple questions regarding structure and maintenance: >> >> - Should I CC the platform-driver-x86 list on future submissions to >> drivers/platform/surface? I.e. is this something you would want to >> review if it doesn't touch the drivers/platform/x86 directory? > > Include PDx86 mailing list to the list of that. Current SURFACE* > drivers have per driver record in MAINTAINERS IIRC. So, update them as > well if needed. Will do. >> - How would you want the layout to be, specifically regarding to the >> surface-aggregator stuff? My suggestion would be simply: >> >> drivers/platform/surface/ > >> surface_aggregator/ > > Don't repeat parts of the path, the aggregator is enough as a folder > name, but the driver of course should be in its own namespace > ('surface'). Okay. >> Kconfig >> Makefile >> core.c >> controller.c >> ... (all core stuff built into the surface_aggregator module) >> Kconfig >> Makefile > >> surface_aggregator_debugfs.c > > (Not sure why it's not a part of aggregator folder) I kind of thought of the aggregator folder to contain only files that build the core module. surface_aggregator_debugfs is intended as separate module, to be loaded when needed. So I'd consider it a client driver to the aggregator in the same way that surface_acpi_notify is. Let me know if you still want me to move this into the aggregator folder though. Personally, I just feel that that might lead to a bit of confusion, specifically the idea that it's built into the core when it's not. >> surface_acpi_notify.c >> surface_*.c (any other surface platform driver as well >> as drivers dependent on surface_aggregator) >> >> - Regarding future things like HID transport driver, battery/AC driver: >> Submit them to drivers/platform/surface or to their respective >> subsystem directories? > > Respective subsystem _if_ it is a subsystem related driver and not > kinda board file. Use common sense and existing examples. Right, thank you! Regards, Max
Hello, The Surface System Aggregator Module (we'll refer to it as Surface Aggregator or SAM below) is an embedded controller (EC) found on various Microsoft Surface devices. Specifically, all 4th and later generation Surface devices, i.e. Surface Pro 4, Surface Book 1 and later, with the exception of the Surface Go series and the Surface Duo. Notably, it seems like this EC can also be found on the ARM-based Surface Pro X [1]. Functionality provided by this EC depends on the Surface model and can (roughly) be broken down by their generations: Starting with 5th generation devices (Surface Pro 2017/5, Surface Book 2, Surface Laptop 1, and later), the EC provides battery and thermal readings, as well as access to the real-time clock. On 5th and 6th generations, these features, specifically, are provided via the ACPI interface of the EC, referred to as Surface ACPI Notify (SAN), i.e. they act as standard ACPI devices of that type, but require a driver translating requests written to an ACPI operation region to requests to the EC. On 7th generation devices, the ACPI interface is (largely) gone, and has been replaced with custom battery and thermal drivers, directly querying the EC. Additionally, HID keyboard and touchpad input for Surface models with these devices built in can be handled via the EC: On the Surface Laptops 1 and 2, this includes only the keyboard, while on the Surface Laptop 3 and Book 3, this includes both touchpad and keyboard. In this case, actual input is provided as HID data and the EC connection acts as HID transport, thus requiring a special transport driver for those devices to work. Further, all known devices (5th and later generations) also support changing of performance/cooling modes, which can influence cooling capabilities of the device (e.g. prefer silent operation over performance), and may influence power limits (e.g. of the discrete GPU found on Surface Books). While this constitutes all major functionality, some more device specific functionality is also handled by the EC. For example, on the Surface Books, the EC handles detaching of the clipboard (i.e. the upper part with screen and CPU) from the base (the lower part with keyboard and optional discrete GPU) and can influence its behavior (i.e. it provides an interface via which detachment can be requested, aborted, or confirmed). It can also be used to detect if there has been a base attached to the clipboard, and if so what type. This patch-series adds the basis for supporting this EC and the features provided by it, by, first, implementing a communication driver providing a fundamental API for client drivers, handling specific aspects of the EC. Additionally, it builds on top of that to provide a dedicated bus and device type to better manage EC clients (and break it down pseudo- device-wise), especially in the case when these client devices are not described in ACPI, i.e. cannot be discovered by conventional means. Furthermore, it provides support for debugging and prototyping via an optional DebugFS interface, and, lastly, also support for the aforementioned ACPI interface, allowing ACPI to communicate with the EC directly. This series only addresses 5th and later generation Surface models as the communication interface has changed substantially from 4th to 5th generation, and the 4th generation interface has not been reverse- engineered yet. Specifically, the underlying transport has been changed from HID feature and input-/output-reports to serial communication via an UART and a custom protocol. Support for 4th generation devices may be added in the future, but as currently not much is known about 4th generation SAM, it yet remains to be seen if this can happen as addition to this subsystem, or if it will be easier to implement this as separate platform driver. Especially as the 4th generation EC does not seem to provide much of the functionality found on 5th and later generations (e.g. no battery status reporting, no thermal reporting, ..., we assume it's just clipboard detachment on the Surface Book 1 and performance mode setting). In more detail, this series adds a driver for the Surface Serial Hub (SSH), the 5th- and later-generation communication channel to the EC, a pseudo-device and driver exposing a DebugFS interface that can be used to communicate with the EC from user-space, intended for debugging, testing, and prototyping, as well as a driver for the Surface ACPI Notify (SAN) device, i.e. the interface between ACPI and EC. Some more details on those can be found on the individual commit messages. This series, apart from the SAN and DebugFS drivers, does not add any client drivers. This will be handled via future patches once the core has been accepted (and the other client drivers have been cleaned up a bit). On the top level, EC communication via the SSH driver can be broken down into requests (sent from host to EC), corresponding responses (sent from EC to host, associated with and triggered by a request), and events (sent from EC to host without association ot a request). The SSH driver manages all communication (i.e. matches responses to requests, enables and disables events, and manages event handlers/notifiers installed by client drivers). On the lower levels, SSH communication is packet-based, and described in more detail in the documentation added in this series (specifically ssh.rst). This set of modules and drivers has been developed out of tree at [2] and used/tested in the kernel we provide at [3] pretty much since its beginnings. It has been developed by reverse-engineering the SSH protocol, mostly through the ACPI interface, communication dumps obtained from listening in on Windows, and deduction. So things may be wrong. There have been some attempts at reverse-engineering existing drivers, which also gave a bit of insight for development, however, I haven't gotten very far on that front beyond some more higher-level concepts and detecting a couple of new EC commands/confirming the functionality of already known commands. Driver and module names have been chosen to align with Windows driver names, some field, vairable, and concept names have been chosen to align with ACPI code (or at least with what I think some of the more cryptic names could mean and make sense in the respective context, e.g. IID -> Instance ID, TC -> Target Category). While I consider this submission complete, I've decided to submit this as RFC first, mainly due to its size and it being my first submission on this scale. Any feedback, review, comment, question, etc. is much appreciated. This patch-set can also be found at the following respository and reference, if you prefer to look at a kernel tree instead of these emails: https://github.com/linux-surface/kernel tags/s/surface-aggregator/rfc-v1 Thanks, Max [1]: The Surface Pro X is, however, currently considered unsupported due to a lack of test candidates and, as it seems, general lack of Linux support on other parts. AFAIK there is an issue preventing serial devices from being registered, on which the core driver in this series is build on, thus there is no way to even test that at this point. I'd be happy to work out any issues regarding SAM on the Pro X at some point in the future, provided someone can/wants to actually test it. [2]: https://github.com/linux-surface/surface-aggregator-module [3]: https://github.com/linux-surface/linux-surface Maximilian Luz (9): misc: Add Surface Aggregator subsystem surface_aggregator: Add control packet allocation chaching surface_aggregator: Add event item allocation chaching surface_aggregator: Add trace points surface_aggregator: Add error injection capabilities surface_aggregator: Add dedicated bus and device type docs: driver-api: Add Surface Aggregator subsystem documentation surface_aggregator: Add DebugFS interface surface_aggregator: Add Surface ACPI Notify client driver Documentation/driver-api/index.rst | 1 + .../surface_aggregator/client-api.rst | 38 + .../driver-api/surface_aggregator/client.rst | 394 +++ .../surface_aggregator/clients/dbgdev.rst | 130 + .../surface_aggregator/clients/index.rst | 21 + .../surface_aggregator/clients/san.rst | 44 + .../driver-api/surface_aggregator/index.rst | 21 + .../surface_aggregator/internal-api.rst | 67 + .../surface_aggregator/internal.rst | 50 + .../surface_aggregator/overview.rst | 76 + .../driver-api/surface_aggregator/ssh.rst | 343 +++ MAINTAINERS | 10 + drivers/misc/Kconfig | 1 + drivers/misc/Makefile | 1 + drivers/misc/surface_aggregator/Kconfig | 65 + drivers/misc/surface_aggregator/Makefile | 17 + drivers/misc/surface_aggregator/bus.c | 419 +++ drivers/misc/surface_aggregator/bus.h | 22 + .../misc/surface_aggregator/clients/Kconfig | 38 + .../misc/surface_aggregator/clients/Makefile | 4 + .../clients/surface_acpi_notify.c | 882 ++++++ .../clients/surface_aggregator_debugfs.c | 281 ++ drivers/misc/surface_aggregator/controller.c | 2505 +++++++++++++++++ drivers/misc/surface_aggregator/controller.h | 283 ++ drivers/misc/surface_aggregator/core.c | 821 ++++++ drivers/misc/surface_aggregator/ssh_msgb.h | 196 ++ .../surface_aggregator/ssh_packet_layer.c | 2002 +++++++++++++ .../surface_aggregator/ssh_packet_layer.h | 170 ++ drivers/misc/surface_aggregator/ssh_parser.c | 224 ++ drivers/misc/surface_aggregator/ssh_parser.h | 152 + .../surface_aggregator/ssh_request_layer.c | 1249 ++++++++ .../surface_aggregator/ssh_request_layer.h | 137 + drivers/misc/surface_aggregator/trace.h | 621 ++++ include/linux/mod_devicetable.h | 18 + include/linux/surface_acpi_notify.h | 37 + include/linux/surface_aggregator/controller.h | 812 ++++++ include/linux/surface_aggregator/device.h | 408 +++ include/linux/surface_aggregator/serial_hub.h | 657 +++++ scripts/mod/devicetable-offsets.c | 8 + scripts/mod/file2alias.c | 23 + 40 files changed, 13248 insertions(+) create mode 100644 Documentation/driver-api/surface_aggregator/client-api.rst create mode 100644 Documentation/driver-api/surface_aggregator/client.rst create mode 100644 Documentation/driver-api/surface_aggregator/clients/dbgdev.rst create mode 100644 Documentation/driver-api/surface_aggregator/clients/index.rst create mode 100644 Documentation/driver-api/surface_aggregator/clients/san.rst create mode 100644 Documentation/driver-api/surface_aggregator/index.rst create mode 100644 Documentation/driver-api/surface_aggregator/internal-api.rst create mode 100644 Documentation/driver-api/surface_aggregator/internal.rst create mode 100644 Documentation/driver-api/surface_aggregator/overview.rst create mode 100644 Documentation/driver-api/surface_aggregator/ssh.rst create mode 100644 drivers/misc/surface_aggregator/Kconfig create mode 100644 drivers/misc/surface_aggregator/Makefile create mode 100644 drivers/misc/surface_aggregator/bus.c create mode 100644 drivers/misc/surface_aggregator/bus.h create mode 100644 drivers/misc/surface_aggregator/clients/Kconfig create mode 100644 drivers/misc/surface_aggregator/clients/Makefile create mode 100644 drivers/misc/surface_aggregator/clients/surface_acpi_notify.c create mode 100644 drivers/misc/surface_aggregator/clients/surface_aggregator_debugfs.c create mode 100644 drivers/misc/surface_aggregator/controller.c create mode 100644 drivers/misc/surface_aggregator/controller.h create mode 100644 drivers/misc/surface_aggregator/core.c create mode 100644 drivers/misc/surface_aggregator/ssh_msgb.h create mode 100644 drivers/misc/surface_aggregator/ssh_packet_layer.c create mode 100644 drivers/misc/surface_aggregator/ssh_packet_layer.h create mode 100644 drivers/misc/surface_aggregator/ssh_parser.c create mode 100644 drivers/misc/surface_aggregator/ssh_parser.h create mode 100644 drivers/misc/surface_aggregator/ssh_request_layer.c create mode 100644 drivers/misc/surface_aggregator/ssh_request_layer.h create mode 100644 drivers/misc/surface_aggregator/trace.h create mode 100644 include/linux/surface_acpi_notify.h create mode 100644 include/linux/surface_aggregator/controller.h create mode 100644 include/linux/surface_aggregator/device.h create mode 100644 include/linux/surface_aggregator/serial_hub.h