Hurricos / realtek-poe

7 stars 10 forks source link

Support for TI TPS23861-based switches #27

Open andyboeh opened 2 years ago

andyboeh commented 2 years ago

I own a TP-Link TL-SG2452P switch (RTL8393M) that incorporates 12x TI TPS23861 PSE controllers. While there is a hwmon driver for this IC, it does not initialize it. A PR adding this capability was turned down by the maintainers as not being a hardware monitoring device.

As an intermediate solution, until a proper kernel driver comes up, I created a modified version of realtek-poe, realtek-poe-ti. It provides the same interface and uses the same configuration file structure, but uses i2c to talk to the PSE controllers. Further, it provides a mapping between the PSE controller channels and the physical LAN ports. This code is available at my github repository: https://github.com/andyboeh/openwrt-packages/tree/master/utils/realtek-poe-ti

It would be nice to have one unified project for managing different kinds of PSE controllers. One solution would be splitting realtek-poe into several backends:

What do you think? Other suggestions?

mrnuke commented 2 years ago

The end-goal for a PoE implementation (including realtek-poe) is to be a kernel driver for the PSE. That's a challenging goal with the current broadcom protocol because (a) It's a protocol instead of a register set, (b) it has device-specific variations, and (c) it's not documented by the vendor. More than half of realtek-poe code is dealing with the protocol, matching commands to replies, and handling unusual cases.

TPS23861 is a great choice for a kernel driver because it's just a set of registers easily covered with regmap. You don't have the complexity, and the register set is well documented. And you can use the plethora of existing userspace tools to do the monitoring and control, such as collectd-sensors.

I saw Guenter's criticism of the kernel patchset. I think it's a simple fix. Implement a power_supply or regulator with a hwmon component. Try searcing for hwmon_device_register through the kernel source tree to see examples.

I think moving the TPS32861 implementation to userspace is a sub-optimal idea. Think of a user that just flashed openwrt. They find the kmod-tps32861 package and install it, but that doesn't work. Then they learn of the userspace demon, and install that. Of course that doesn't work because the addresses are reserved by the kernel driver. Eventually,, said user needs to struggle with config files, unbinding kernel drivers, and the like.

Splitting realtek-poe into backends

We sort of need to do that already. We have a couple of variations

The problem with I2C

The kernel doesn't offer an asynchronous I2C API. For the entire duration of the transfer, the thread is doing nothing but waiting on the kernel. That's problematic when you fire a bunch of I2C transfers in sequence, and don't return control to your main loop. The program can't respond to other events, such as ubus requests until the transfers are finished. The converse of that, returning to main loop between each request greatly increases complexity.

Even something as innocent as firing 10 transfers with a 100 ms timeout can put you out of business for one whole second. This is one of the concerns that #22 did not address.

Should realtek-poe support TPS32861?

Other than a couple of ubus calls, how much code in realtek-poe is actually useful? You could solve all the issues I described, we merge the code, and everyone's happy, right? I feel that's a lot more work than just writing a kernel driver with the correct subsystem.

There isn't anything preventing realtek-poe from having a sysfs backend and pulling in data from /sys/class/hwmon, and /sys/class/power/supply.

Closing remarks

AVOID REINVENTING KERNEL WHEELS IN USERSPACE.

Documentation/admin-guide/gpio/sysfs.rst

andyboeh commented 2 years ago

While I fully agree with your points, I'm not going to do any kernel development work. I lack the experience, the time and resources to do so. However, I needed a solution now (my TL-SG2452P needs to be back in service in about a week) and adding userspace packages is a much quicker task.

I doubt that the average user is aware of the various PoE ICs that could be inside of the switch. Instead, they would check the Wiki or, ideally, either install realtek-poe or have it shipped with the default image.

What I can do, however, is refactoring realtek-poe so that it supports different backends. In the long run, we can swap out the i2c/tps backends with sysfs-based backends.

andyboeh commented 2 years ago

I just found out about poemgr - what is the situation with realtek-poe vs. poemgr? poemgr already contains a driver for i2c-based PSE chips, so maybe the TPS is better located there?

mrnuke commented 2 years ago

At some point, I had a poemgr port for my engenius switch (https://github.com/mrnuke/poemgr/commits/unreal-tek-poe). poemgr is really simple, which I like. It doesn't have to monitor ubus, so making blocking syscalls is generally not an issue.

It's a one-shot and done deal, so you don't need to worry about a daemon crashing or hanging.

Have you considered using the TPS23861 broadcast address and configuring ports in /etc/rc.local ? Here's a config from my TL-SG2008P:

i2cset -y 0 0x30 0x12 0xff
echo 1 > /sys/class/hwmon/hwmon0/in0_enable
echo 1 > /sys/class/hwmon/hwmon0/in1_enable
echo 1 > /sys/class/hwmon/hwmon0/in2_enable
echo 1 > /sys/class/hwmon/hwmon0/in3_enable
andyboeh commented 2 years ago

Thanks for the clarification regarding poemgr!

I never tried it, although it could work: According to the datasheet, a write operation to all ICs using the broadcast address is possible. That actually means that realtek-poe would only need to initialize the ICs using the broadcast address, then it doesn't even need to know the individual addresses and could monitor and control the controllers using a sysfs interface.

EDIT: Indeed, it does work! Seems like a nice way to get rid of most of the i2c transactions in realtek-poe, a few limitations:

  1. I would do the broadcast in realtek-poe during init
  2. It's not possible to configure PoE+ or to (properly, i.e. without debugfs) get the PoE class
  3. If I've understood correctly how the hwmon driver works, then it updates current, temperature and voltage values on sysfs access - which means that the kernel has to do the i2c transfer in the background and on demand, resulting in a similar delay as with the userspace implementation.