| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHp75VfZ4GEKVvd2ZyR2wxHRPNej+_KbQja8KvucYGsTWrQsXw@mail.gmail.com>
Date: Mon, 1 Dec 2025 04:25:57 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Crescent Hsieh <crescentcy.hsieh@...a.com>
Cc: gregkh@...uxfoundation.org, jirislaby@...nel.org,
ilpo.jarvinen@...ux.intel.com, linux-kernel@...r.kernel.org,
linux-serial@...r.kernel.org
Subject: Re: [PATCH v1 26/31] serial: 8250_mxpcie: add basic CPLD helper functions
On Sun, Nov 30, 2025 at 12:45 PM Crescent Hsieh
<crescentcy.hsieh@...a.com> wrote:
>
> Introduce a set of helper functions to access the on-board CPLD on
> Moxa PCIe serial devices through the GPIO I/O space. These helpers
> cover:
>
> - Initializing the CPLD-related GPIO pins to a safe default state
> - Enabling/disabling the CPLD chip select
> - Switching between read/write and address/data modes
> - Performing single-byte read and write transactions using GPIO
> bit-banging, with simple delay and retry logic
>
> These functions do not affect the UART datapath and are not yet used
> by the driver. They are added as a preparation step for follow-up
> patches that will implement more complex CPLD-based features.
...
> +static void mxpcie8250_cpld_read(resource_size_t iobar_addr, u8 addr, u8 *data)
> +{
> + u8 saved_state, new_state;
> + u8 samples[MOXA_CPLD_RETRY_CNT], votes[MOXA_CPLD_RETRY_CNT];
> + int i, j;
> +
> + /* Perform multiple read attempts with majority voting */
Why?! Is the HW so buggy?
> + for (i = 0; i < MOXA_CPLD_RETRY_CNT; i++) {
> + /* Set read/write pin to read state */
> + mxpcie8250_cpld_set_direction(iobar_addr, MOXA_CPLD_READ);
> + /* Set address/data bus pins to output for address phase */
> + mxpcie8250_gpio_set_direction(iobar_addr, MOXA_GPIO_PIN0, MOXA_GPIO_STATE_OUTPUT);
> + mxpcie8250_gpio_set_direction(iobar_addr, MOXA_GPIO_PIN1, MOXA_GPIO_STATE_OUTPUT);
> + mxpcie8250_gpio_set_direction(iobar_addr, MOXA_GPIO_PIN2, MOXA_GPIO_STATE_OUTPUT);
> + mxpcie8250_gpio_set_direction(iobar_addr, MOXA_GPIO_PIN3, MOXA_GPIO_STATE_OUTPUT);
> + mxpcie8250_gpio_set_direction(iobar_addr, MOXA_GPIO_PIN4, MOXA_GPIO_STATE_OUTPUT);
> + /* Backup current GPIO output state */
> + mxpcie8250_gpio_get_all(iobar_addr, &saved_state, MOXA_GPIO_OUTPUT);
> + /* Prepare address to GPIO bus */
> + new_state = saved_state & MOXA_CPLD_CTRL_MASK;
> + new_state |= (addr & MOXA_CPLD_DATA_MASK);
> + /* Output address to GPIO bus */
> + mxpcie8250_gpio_set_all(iobar_addr, new_state, MOXA_GPIO_OUTPUT);
> + /* Switch to address mode (address/data pin) */
> + mxpcie8250_cpld_set_mode(iobar_addr, MOXA_CPLD_ADDRESS);
> + /* Enable CPLD by pulling chip select pin low*/
MIssed space before */.
> + mxpcie8250_cpld_enable(iobar_addr);
> + /* Wait for CPLD timing (about 70 ns) */
> + mdelay(1);
Why atomic?
And 70ns is more than 10x times less than 1ms!
> + /* Switch to data mode (address/data pin) */
> + mxpcie8250_cpld_set_mode(iobar_addr, MOXA_CPLD_DATA);
> + /* Set address/data bus pins to input for data phase */
> + mxpcie8250_gpio_set_direction(iobar_addr, MOXA_GPIO_PIN0, MOXA_GPIO_STATE_INPUT);
> + mxpcie8250_gpio_set_direction(iobar_addr, MOXA_GPIO_PIN1, MOXA_GPIO_STATE_INPUT);
> + mxpcie8250_gpio_set_direction(iobar_addr, MOXA_GPIO_PIN2, MOXA_GPIO_STATE_INPUT);
> + mxpcie8250_gpio_set_direction(iobar_addr, MOXA_GPIO_PIN3, MOXA_GPIO_STATE_INPUT);
> + mxpcie8250_gpio_set_direction(iobar_addr, MOXA_GPIO_PIN4, MOXA_GPIO_STATE_INPUT);
> + /* Wait for CPLD timing (about 70 ns) */
> + mdelay(1);
Ditto.
> + /* Read data bus pins */
> + mxpcie8250_gpio_get_all(iobar_addr, data, MOXA_GPIO_INPUT);
> + *data &= MOXA_CPLD_DATA_MASK;
> + /* No need to restore read/write pin (defaults to read); disable CPLD */
> + mxpcie8250_cpld_disable(iobar_addr);
> + /* Store read value for voting */
> + samples[i] = *data;
> + votes[i] = 0;
> +
> + for (j = i - 1; j >= 0; j--) {
j = i
while (j--) {
is much easier to understand.
However, the "votes" approach has to be explained "Why?". Bugs in HW?
Other stuff?
> + if (samples[j] == samples[i])
> + votes[i]++;
> + }
> + /* Perform majority voting to select stable value */
> + if (votes[i] >= (MOXA_CPLD_RETRY_CNT / 2))
> + break;
> + }
> +}
> +
> +/**
> + * mxpcie8250_cpld_write() - Write a byte to the CPLD at a specified address
> + * @iobar_addr: The base address of the GPIO I/O region
> + * @addr: Address in the CPLD to write to
> + * @data: Data byte to write
> + *
> + * Writes a single byte of data to the CPLD at the given address using
> + * GPIO-based communication. Includes verification with optional retry.
> + */
> +static void mxpcie8250_cpld_write(resource_size_t iobar_addr, u8 addr, u8 data)
> +{
> + u8 saved_state, new_state, verify_data;
> + int retry_cnt;
> +
> + for (retry_cnt = 0; retry_cnt < MOXA_CPLD_RETRY_CNT; retry_cnt++) {
> + /* Set read/write pin to write state */
> + mxpcie8250_cpld_set_direction(iobar_addr, MOXA_CPLD_WRITE);
> + /* Set data bus pins to output for address phase */
> + mxpcie8250_gpio_set_direction(iobar_addr, MOXA_GPIO_PIN0, MOXA_GPIO_STATE_OUTPUT);
> + mxpcie8250_gpio_set_direction(iobar_addr, MOXA_GPIO_PIN1, MOXA_GPIO_STATE_OUTPUT);
> + mxpcie8250_gpio_set_direction(iobar_addr, MOXA_GPIO_PIN2, MOXA_GPIO_STATE_OUTPUT);
> + mxpcie8250_gpio_set_direction(iobar_addr, MOXA_GPIO_PIN3, MOXA_GPIO_STATE_OUTPUT);
> + mxpcie8250_gpio_set_direction(iobar_addr, MOXA_GPIO_PIN4, MOXA_GPIO_STATE_OUTPUT);
This block is repeated at least three times (just with a different
direction), make it a helper and call as many times as you need.
> + /* Backup current GPIO output state */
> + mxpcie8250_gpio_get_all(iobar_addr, &saved_state, MOXA_GPIO_OUTPUT);
> + /* Prepare bus value with address bits */
> + new_state = saved_state & MOXA_CPLD_CTRL_MASK;
> + new_state |= (addr & MOXA_CPLD_DATA_MASK);
> + /* Output address to GPIO bus */
> + mxpcie8250_gpio_set_all(iobar_addr, new_state, MOXA_GPIO_OUTPUT);
> + /* Switch to address mode (address/data pin)*/
> + mxpcie8250_cpld_set_mode(iobar_addr, MOXA_CPLD_ADDRESS);
> + /* Enable CPLD by pulling chip select pin low */
> + mxpcie8250_cpld_enable(iobar_addr);
> + /* Wait for CPLD timing (about 70 ns) */
> + mdelay(1);
See above.
> + /* Set data bus pins to output for data phase */
> + mxpcie8250_gpio_set_direction(iobar_addr, MOXA_GPIO_PIN0, MOXA_GPIO_STATE_OUTPUT);
> + mxpcie8250_gpio_set_direction(iobar_addr, MOXA_GPIO_PIN1, MOXA_GPIO_STATE_OUTPUT);
> + mxpcie8250_gpio_set_direction(iobar_addr, MOXA_GPIO_PIN2, MOXA_GPIO_STATE_OUTPUT);
> + mxpcie8250_gpio_set_direction(iobar_addr, MOXA_GPIO_PIN3, MOXA_GPIO_STATE_OUTPUT);
> + mxpcie8250_gpio_set_direction(iobar_addr, MOXA_GPIO_PIN4, MOXA_GPIO_STATE_OUTPUT);
See above.
> + /* Switch to data mode (address/data pin) */
> + mxpcie8250_cpld_set_mode(iobar_addr, MOXA_CPLD_DATA);
> + /* Backup current GPIO output state */
> + mxpcie8250_gpio_get_all(iobar_addr, &saved_state, MOXA_GPIO_OUTPUT);
> + /* Prepare bus value with data bits */
> + new_state = saved_state & MOXA_CPLD_CTRL_MASK;
> + new_state |= (data & MOXA_CPLD_DATA_MASK);
> + /* Output data to GPIO bus */
> + mxpcie8250_gpio_set_all(iobar_addr, new_state, MOXA_GPIO_OUTPUT);
> + /* Wait for CPLD timing (about 70 ns) */
> + mdelay(1);
See above
> + /* Disable CPLD by releasing chip select pin */
> + mxpcie8250_cpld_disable(iobar_addr);
> +
> + if (addr & MOXA_CPLD_SET_STATE_BASE) {
> + mxpcie8250_cpld_read(iobar_addr, ((addr & ~MOXA_CPLD_SET_STATE_BASE) | MOXA_CPLD_GET_STATE_BASE), &verify_data);
> +
> + if (verify_data == data)
> + break;
> + }
> + }
> +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists