Message-ID: <20231215061159.hc2pkdm3pwvdbyys@vireshk-i7>
Date: Fri, 15 Dec 2023 11:41:59 +0530
From: Viresh Kumar <viresh.kumar@...aro.org>
To: Harald Mommer <harald.mommer@...nsynergy.com>,
	"Michael S. Tsirkin" <mst@...hat.com>,
	Cornelia Huck <cohuck@...hat.com>
Cc: virtio-dev@...ts.oasis-open.org, Haixu Cui <quic_haixcui@...cinc.com>,
	Mark Brown <broonie@...nel.org>, linux-spi@...r.kernel.org,
	linux-kernel@...r.kernel.org, Harald.Mommer@...il.com,
	quic_ztu@...cinc.com, Matti Moell <Matti.Moell@...nsynergy.com>,
	Mikhail Golubev <Mikhail.Golubev@...nsynergy.com>,
	Alex Bennée <alex.bennee@...aro.org>,
	Vincent Guittot <vincent.guittot@...aro.org>
Subject: Re: [virtio-dev] [RFC PATCH v1 3/3] SPI: Add virtio SPI driver (V4
 draft specification).

+ Michael and Cornelia

Hello Harald,

On 14-12-23, 17:52, Harald Mommer wrote:
> On 13.12.23 10:53, Viresh Kumar wrote:
> > On 27-10-23, 18:10, Harald Mommer wrote:
> > > +	struct virtqueue *vqs[VIRTIO_SPI_QUEUE_COUNT];
> > There is no need to make this configurable since the specification
> > fixes it to 1. You can simplify this a bit.
> Yes, i2c makes it "struct virtqueue *vq;" and this makes also sense here.

Please add a blank line before and after your responses. It becomes very
difficult otherwise and one may end up missing some of the comments,
especially with bottom posting.

> > > +	/*
> > > +	 * Simple implementation: Process message by message and wait for each
> > > +	 * message to be completed by the device side.
> > > +	 */
> > And why not send all the messages once and speed this thing up ? Just
> > like how I2C does it.
> 
> Because this is more complicated when I looked into i2c. First I wanted to
> have a working driver which can be delivered to our internal project. This
> internal project has no sophisticated performance requirements. Better to
> have something for the internal project when you have to deliver to them
> instead of having nothing at all because you wanted initially too much
> nobody asked for.
> 
> To minimize risk and not to extend the scope of my existing tickets too much
> I will address your comment not by a code change but by a comment in the
> code. Nothing is broken, this is an optimization only.

I understand that the current implementation is good enough for your
use case. But you are trying to upstream this and there are more users
who will end up using it. Qualcomm is one for example, who is
upstreaming the spec.

I think the changes required shouldn't be huge and can be taken care
of easily, unless there is a big blocker. Given that I2C already
implements it that way, I think it would be easier to do it now from
the first implementation itself, since it isn't already merged.

Please see if that can be done, or I will write an incremental patch
for this.

> > > +#ifdef DEBUG
> > Drop all temporary things please.
> 
> This is an RFC patch, not an integration request. Until the spec is accepted
> by OASIS the driver is code under development tracking a moving target and
> will have to remain RFC. Removing debug aids is something to be addressed
> before we are going for non-RFC.

Since this is up for review, I would have liked to see a version
closer to what is supposed to be merged. It doesn't harm to keep all
such changes in a separate commit which you don't post to LKML. That
makes it easier for reviewers to go through stuff without getting
distracted with small things.

> > > +static void virtio_spi_read_config(struct virtio_device *vdev)
> > > +{
> > > +	struct spi_master *master = dev_get_drvdata(&vdev->dev);
> > > +	struct virtio_spi_priv *priv = vdev->priv;
> > > +	u16 bus_num;
> > > +	u16 cs_max_number;
> > > +	u8 support_delays;
> > > +
> > > +	bus_num = virtio_cread16(vdev,
> > > +				 offsetof(struct virtio_spi_config, bus_num));
> > > +	cs_max_number = virtio_cread16(vdev, offsetof(struct virtio_spi_config,
> > > +						      chip_select_max_number));
> > > +	support_delays =
> > > +		virtio_cread16(vdev, offsetof(struct virtio_spi_config,
> > > +					      cs_timing_setting_enable));
> > Instead of reading values separately, you can also read the entire
> > configuration structure in a single call to virtio_cread_bytes.
> 
> Cannot.
> 
> Virtio spec 4.2.2.2 Driver Requirements: MMIO Device Register Layout
> 
> ...
> For the device­-specific configuration space, the driver MUST use 8 bit wide
> accesses for 8 bit wide fields, 16 bit wide and aligned accesses for 16 bit
> wide fields and 32 bit wide and aligned accesses for 32 and 64 bit wide
> fields.

Fair point. I wonder if other implementations have got it wrong now.

Michael / Cornelia,

What's the right thing to do here ? I am not sure why that
driver-normative was even required for MMIO.

> > > +static int virtio_spi_probe(struct virtio_device *vdev)
> > > +{
> > > +	struct virtio_spi_priv *priv;
> > > +	struct spi_master *master;
> > > +	int err;
> > > +	u16 csi;
> > > +
> > > +	err = -ENOMEM;

I was only suggesting to use:

int err = -ENOMEM;

:)

> > Why not do it with the definition itself ?
> 
> Replaced in the meantime all definitions containing the word "master" by the
> word controller when they existed so I'm not using the compatibility layer
> containing preprocessor definitions any more. For some reason there is no
> function spi_alloc_controller() so this here is the only one which cannot be
> replaced. It's already the function.
> 
> Update: In newer kernels there is now an identical spi_alloc_host(). Why now
> host instead of controller?

IIUC, both master/slave are called controllers here.

And with below commit:

commit b8d3b056a78d ("spi: introduce new helpers with using modern naming")

Master => Host
Slave => Target

I guess eventually Master and Slave interfaces will be removed, and we
are in the transition phase for now.

> I get mad.A lot of people still use
> spi_alloc_master().But this is a new driver so a change to spi_alloc_host()
> is most probably required.

Right.

> > > +	master = spi_alloc_master(&vdev->dev, sizeof(struct virtio_spi_priv));
> > sizeof(*priv)
> 
> Some people do it this way, others the other way.

Documentation/process/coding-style.rst

14) Allocating memory

...

	p = kmalloc(sizeof(*p), ...);

The alternative form where struct name is spelled out hurts readability and
introduces an opportunity for a bug when the pointer variable type is changed
but the corresponding sizeof that is passed to a memory allocator is not.

> > and there is a devm_* variant too that you can use.
> Not that I'm aware of now.There is spi_alloc_host() as alternative
> frequently used now.

Yeah, that also has a devm_* variant.

Thanks.

-- 
viresh

Powered by blists - more mailing lists