| Message ID | mailman.140269.1729592657.1280.openwrt-devel@lists.openwrt.org |
|---|---|
| State | RFC |
| Headers | show |
| Series | [RFC,1/4] realtek: rtl8231 driver: dts option for GPIO state | expand |
The sender domain has a DMARC Reject/Quarantine policy which disallows sending mailing list messages using the original "From" header. To mitigate this problem, the original message has been wrapped automatically by the mailing list software. Evan Jobling via openwrt-devel <openwrt-devel@lists.openwrt.org> writes: > This is due to the patch fixing output drivers, > due to untrusted bootloader. > i.e. This is by setting all GPIO's to input. > 3810e897295cb66bc45a62bc2c0b70a01004fe3b > "realtek: ensure output drivers are enabled in RTL8231" Are we sure that commit shouldn't be reverted? Why is it be better to set all pins to some arbitrary state than leaving them in whatever state the bootloader (or we, before reboot) set them to? Not sure I understand the purpose of the 3810e897295c commit... Why is it necessary to set all pins to input for output drivers to be enabled? And if some stupid boot loader pulls the SoC reset to 0, then why not fix that specific bug instead of messing with every device out there? Setting all pins to input is not guaranteed to be a safe operation, as you point out. In addition to your fan problem, I'm thinking about the PoE enable pin on the Zyxel GS1900-10HP (and possibly other devices). If we had a boot loader that didn't touch this output pin, then it would be nice to be able to load the driver without toggling it too. Don't think your patch will be sufficient. It will still toggle the pins briefly, won't it? Bjørn
The sender domain has a DMARC Reject/Quarantine policy which disallows sending mailing list messages using the original "From" header. To mitigate this problem, the original message has been wrapped automatically by the mailing list software. Firstly, thanks for the feedback! > Are we sure that commit shouldn't be reverted? Why is it be better to > set all pins to some arbitrary state than leaving them in whatever state > the bootloader (or we, before reboot) set them to? It's a great point that another option was reverting that commit. If someone could elaborate on more context for that patch it would be great. I didn't want to break existing devices. I guess another valid option is make that patch opt out in device tree? (Or opt in, and see which targets are affected/who complains/when something breaks?) I think this issue is more how to handle existing configurations. An option is update the device compat for all targets? I think ensuring we aren't going to get unexpected behaviour is reasonable. If output drivers don't work we can't trust what's on the rtl8231 GPIO's? That's also the SFP's and other I2C devices I think? I think it's mostly an issue for things that aren't bi-directional though? i.e. what happens if we try to toggle a GPIO and we think the GPIO output drivers are working and they aren't? Fan example: we think we're controlling the fans but we aren't i.e. only have open loop control, at least in the JG922A example. I guess another option is just reset and ensure output driver state on just the pins we care about? > In addition to your fan problem, I'm thinking about the PoE enable pin > on the Zyxel GS1900-10HP (and possibly other devices). If we had a boot > loader that didn't touch this output pin, then it would be nice to be > able to load the driver without toggling it too. Don't think your patch > will be sufficient. It will still toggle the pins briefly, won't it? You are absolutely correct that this still toggles the pins. When I was testing my larger, by register method I was so slow that the fans "blipped". At the moment the fans don't blip for JG922A with this by pin implementation. But because this method is by pin and looping it will cause issues even with fan speed if you sequenced the array in a particular fashion. I can see the impact on a device that is rebooting but perhaps you want the PoE to still be enabled through a reboot. I had a (working.....) implementation that did lots of bitwise math and getting masks to do it by register. I was at the stage where I was doing to do my own review. Didn't want to review my extra code before requesting feedback. I lost interest as my code "worked" haha. Plus then there's just another path that I wasn't planning on using. My justification is that using the existing per pin functions reduces the amount of code/bitwise arithmetic added that is just another workaround. One could extend the code to do it by register and not touch specific things if they're in a "good" state later. My point regarding "good" state is below though: If the output driver isn't working whilst it's in output mode, then how do you know when reading from the rtl8231? i.e. if it's in output mode the output driver is functioning? Once you don't trust your initial state it's just that, you need a "known good" state to work from. i.e. My question is how do you determine the rtl8231 GPIO output driver state is "good"? You would need to toggle the GPIO and see what happens anyway? Cheers, Evan.
diff --git a/target/linux/realtek/files-6.6/drivers/gpio/gpio-rtl8231.c b/target/linux/realtek/files-6.6/drivers/gpio/gpio-rtl8231.c index 2821591a97..3bb5a1cb91 100644 --- a/target/linux/realtek/files-6.6/drivers/gpio/gpio-rtl8231.c +++ b/target/linux/realtek/files-6.6/drivers/gpio/gpio-rtl8231.c @@ -250,9 +250,14 @@ void rtl8231_gpio_set(struct gpio_chip *gc, unsigned int offset, int value) rtl8231_pin_set(gpios, offset, value); } -int rtl8231_init(struct rtl8231_gpios *gpios) +int rtl8231_init(struct rtl8231_gpios *gpios, struct device_node *np ) { u32 ret; + /*return variables for GPIO init*/ + int err; + int count; + /* local variables for gpio output init */ + u32 gpio, data; pr_info("%s called, MDIO bus ID: %d\n", __func__, gpios->smi_bus_id); @@ -278,6 +283,50 @@ int rtl8231_init(struct rtl8231_gpios *gpios) rtl8231_write(gpios, RTL8231_GPIO_DIR(16), 0xffff); rtl8231_write(gpios, RTL8231_GPIO_PIN_SEL(32), 0x03ff); + /* Get/set default GPIO state from device tree as input/output driver + initialisation above resets GPIO configuration from bootloader. + Important to keep default fan behaviour in some switches. + Note we don't handle inverted logic GPIO's.*/ + if ( (count = of_property_count_u32_elems(np, "gpio-output-pin-state") ) > 0 ) { + pr_info("%s, MDIO bus ID: %d: Set GPIO output initial state per pin\n", __func__, gpios->smi_bus_id); + /*we are going in pairs as we have an array which contains gpio number, gpio val. + The GPIO we want as output (0-36), and whether we want it to be high or low. + We should have less than 36 pairs of u32's and the number should be even. + i.e. we have a pair of u32's per gpio.*/ + if (count > 36*2) + return -EINVAL; + if (count % 2) + return -EINVAL; + + /*setup for array indexing*/ + count = count / 2; + + /*Loop through our array of gpio, val. + we should have count to be less that 36*/ + for (u16 i = 0; i < count; i++) { + if ( (err = of_property_read_u32_index(np, "gpio-output-pin-state", i*2, &gpio)) ) + return err; + if ( (err = of_property_read_u32_index(np, "gpio-output-pin-state", i*2+1, &data)) ) + return err; + + if (gpio > 36) + return -EINVAL; + + if (data > 1) + return -EINVAL; + + //set pin to output + if( (err = rtl8231_pin_dir(gpios, gpio, 0)) ) + return err; + //set pin level + if( (err = rtl8231_pin_set(gpios, gpio, data)) ) + return err; + } + } + else { + pr_info("%s, MDIO bus ID: %d: no gpio initial state, all GPIO's are forced input\n", __func__, gpios->smi_bus_id); + } + /* Set LED_Start to enable drivers for output mode */ rtl8231_write(gpios, RTL8231_LED_FUNC0, 1 << 1); @@ -327,7 +376,7 @@ static int rtl8231_gpio_probe(struct platform_device *pdev) return err; } - err = rtl8231_init(gpios); + err = rtl8231_init(gpios, np); if (err) { dev_err(dev, "no device found at bus address %d\n", gpios->smi_bus_id); return err;