Re: [PATCH V3] virtio-gpio: Add the device specification

[Viresh Kumar <viresh.kumar@linaro.org>]

On 10-06-21, 23:45, Linus Walleij wrote:
> Hi Viresh!
> 
> I'm not any expert with virtio but I know a few things about GPIO.

And I am an expert at none of those :)

> On Tue, Jun 8, 2021 at 1:32 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> 
> > +\subsection{Feature bits}\label{sec:Device Types / GPIO Device / Feature bits}
> > +
> > +\begin{description}
> > +\item[VIRTIO_GPIO_F_IRQ (0)] The device supports interrupts on GPIO lines.
> > +\end{description}
> 
> How many bits are these?

From code, I see this to be 64. And from spec this is what I see:

Feature bits are allocated as follows:

0 to 23
        Feature bits for the specific device type
24 to 37
        Feature bits reserved for extensions to the queue and feature negotiation mechanisms
38 and above
        Feature bits reserved for future extensions.

I guess only 24 are for us here ? But that should be enough, isn't it
?

> Features I can already see will sooner or later be requested are:
> 
> - Setting and getting several GPIO lines at the same time
>   (reflecing get/set_multiple() in the GPIO subsystem)
> 
> - Line configuration, i.e. all these +/-:
>   (include/linux/pinctrl/pinconf-generic.h)
>         PIN_CONFIG_BIAS_BUS_HOLD,
>         PIN_CONFIG_BIAS_DISABLE,
>         PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
>         PIN_CONFIG_BIAS_PULL_DOWN,
>         PIN_CONFIG_BIAS_PULL_PIN_DEFAULT,
>         PIN_CONFIG_BIAS_PULL_UP,
>         PIN_CONFIG_DRIVE_OPEN_DRAIN,
>         PIN_CONFIG_DRIVE_OPEN_SOURCE,
>         PIN_CONFIG_DRIVE_PUSH_PULL,
>         PIN_CONFIG_DRIVE_STRENGTH,
>         PIN_CONFIG_DRIVE_STRENGTH_UA,
>         PIN_CONFIG_INPUT_DEBOUNCE,
>         PIN_CONFIG_INPUT_ENABLE,
>         PIN_CONFIG_INPUT_SCHMITT,
>         PIN_CONFIG_INPUT_SCHMITT_ENABLE,
>         PIN_CONFIG_MODE_LOW_POWER,
>         PIN_CONFIG_MODE_PWM,
>         PIN_CONFIG_OUTPUT_ENABLE,
>         PIN_CONFIG_OUTPUT,
>         PIN_CONFIG_PERSIST_STATE,
>         PIN_CONFIG_POWER_SOURCE,
>         PIN_CONFIG_SLEEP_HARDWARE_STATE,
>         PIN_CONFIG_SLEW_RATE,
>         PIN_CONFIG_SKEW_DELAY,
> 
> People always want features. So there better be lots of bits
> to signal this or that feature, plus the requirement that all other
> bits MUST be zero for now.

I think that is already implied by the specification in general. If
the protocol doesn't use any feature bits, those bits must be zero.

> > +\item[\field{gpio_names}] is a stream of \field{ngpio} null-terminated strings,
> > +    where each string corresponds to a GPIO line.
> 
> Add:
> "each and every name MUST be unique for this GPIO Device and
> names MUST not be NULL"

Sure.

> IMO else you will get problems.
> Example: "gpio0", "gpio1", "gpio2"  ... "gpioN" is fine.
> More descriptive names can be used but they MUST be unique.
> 
> > +#define VIRTIO_GPIO_REQ_ACTIVATE                0x01
> > +#define VIRTIO_GPIO_REQ_DEACTIVATE              0x02
> > +#define VIRTIO_GPIO_REQ_GET_DIRECTION           0x03
> > +#define VIRTIO_GPIO_REQ_DIRECTION_IN            0x04
> > +#define VIRTIO_GPIO_REQ_DIRECTION_OUT           0x05
> > +#define VIRTIO_GPIO_REQ_GET_VALUE               0x06
> > +#define VIRTIO_GPIO_REQ_SET_VALUE               0x07
> > +#define VIRTIO_GPIO_REQ_IRQ_TYPE                0x08
> > +#define VIRTIO_GPIO_REQ_IRQ_MASK                0x09
> > +#define VIRTIO_GPIO_REQ_IRQ_UNMASK              0x0a
> > +#define VIRTIO_GPIO_IRQ_EVENT                   0x0b
> 
> As per review of the Linux driver I think a VIRTIO_GPIO_REQ_ACK
> message should exist with the semantics that the host MUST not
> send any new VIRTIO_GPIO_IRQ_EVENT:s for the same IRQ
> (other IRQs are fine) until it has recieved a VIRTIO_GPIO_REQ_ACK
> back for the IRQ (the IRQ number must be passed with the ACK).

Actually if you look at the specification, it is already there. I did
add that the guest needs to send a response message back on the
"interrupt" virtqueue to make end of the message initiated at host.

I just didn't use it in kernel driver :)

> This makes things
> feasible for transport to the other operating system, otherwise
> the recieveing OS must essentially hold local IRQs disabled
> until it gets a chance to send VIRTIO_GPIO_REQ_IRQ_MASK
> to mask this IRQ.

Yes, I will make this completely synchronous. Only one at a time.

-- 
viresh