Re: [PATCH 2/5] spi: implement companion-spi driver

[Oleksij Rempel]

Hi Mark,

On Thu, Jun 07, 2018 at 02:58:24PM +0000, Jonas Mark (BT-FIR/ENG1) wrote:
> Hello Andy,
> 
> Thank you very much for your feedback.
> 
> > > +       /*TODO: support mutiple packets in one write in future*/
> > 
> > Hmm... Either fix this, or remove comment?
> 
> Agreed, we will manage ideas for future changes at a different place.
> 
> > > +       if (copy_from_user(p.data, buf, sizeof(p)) == 0) {
> > > +               if (is_can_type(&p))
> > > +                       return -EINVAL;
> > > +       } else {
> > > +               dev_info(parent, "copy from user not succeed in one call\n");
> > 
> > Shouldn't it return immediately?
> 
> Yes, it should. Will be changed.
> 
> > 
> > > +       }
> > 
> > > +
> > > +       error = qm_io_txq_in(&priv->pm.qm, buf, count, &copied);
> > 
> > ...what the point to call if we got garbage from user space?
> 
> Will be changed with the code above.
> 
> > > +       if (!error) {
> > 
> > The usual pattern is to check for errors first.
> 
> Understood, will be changed.
> 
> > > +               wake_up_interruptible(&priv->wait);
> > > +               priv->pm.stats.io_tx++;
> > > +               return copied;
> > > +       } else {
> > > +               priv->pm.stats.io_tx_overflows++;
> > > +       }
> > > +       return error;
> > > +}
> > 
> > > +       ... qm_io_rxq_out(&priv->pm.qm, buf, count, &copied);
> > 
> > > +       ... pm_can_data_tx(&priv->pm, port, prio, cf);
> > 
> > Oh, oh, oh.
> > 
> > These namespaces are too generic, moreover pm is kinda occupied by
> > power management. You bring a lot of confusion here, I think.
> 
> We agree and we started thinking about better names. We will send a proposal.
> 
> > > +       err = pm_can_get_txq_status(pm, port);
> > > +       if (!err) {
> > 
> > if (err)
> >  return err;
> 
> Yes, will be changed.
> 
> > > +       }
> > > +       return err;
> > 
> > 
> > > +       int                        ret, value;
> > > +
> > > +       ret = sscanf(buf, "%d", &value);
> > > +       if (ret != 1) {
> > 
> > > +       }
> > 
> > You have to be consistent in your code.
> > 
> > I've already noticed
> > 
> > err
> > error
> > 
> > and now
> > 
> > ret
> > 
> > Choose one and stick with it.
> 
> Yes, will be changed.
> 
> > Also check your entire patch series' code for consistency.
> 
> We will have a look.
> 
> > > +static DEVICE_ATTR(dump_packets, S_IRUGO | S_IWUSR,
> > > +                   show_dump_packets, store_dump_packets);
> > 
> > We have helpers, like DEVICE_ATTR_RW().
> 
> Will be changed.
> 
> > > +               ret  = snprintf(buf + pos, PAGE_SIZE - pos,
> > > +                               "\ntx: %u, rx: %u, err: %u\n\n",
> > > +                               total,
> > > +                               priv->pm.stats.can_rx_overflows[i],
> > > +                               priv->pm.stats.can_err_overflows[i]);
> > 
> > Please, avoid leading '\n'.
> 
> We think we will stick to the existing. Otherweise we would have to
> insert another pair of sprint() and pos += ret. Is it worth that?
> 
> > 
> > > +       gpio_set_value(priv->cs_gpios, priv->cs_gpios_assert);
> > 
> > > +       gpio_set_value(priv->cs_gpios, !priv->cs_gpios_assert);
> > 
> > Can you switch to GPIO descriptor API?
> 
> Yes, we are working on it.
> 
> > > +       unsigned int count = READY_POLL_US / READY_POLL_US_GRAN;
> > > +       while (count--) {
> > 
> > For counting like this better to have
> > do {
> > } while (--count);
> > 
> > The rationale, reader at first glance will know that the loop will
> > iterate at least once.
> 
> Agreed, will be changed.
> 
> > > +               if (slave_is_not_busy(priv))
> > > +                       return 0;
> > > +
> > 
> > > +               udelay(READY_POLL_US_GRAN);
> > 
> > Should it be atomic?
> > Can it use read_poll_* type of helpers instead?
> 
> Yes, it shall be atomic because we need to reduce the latency at
> detecting the de-assertion of the busy signal. We accept that this will
> cost CPU time.
> 
> In case the Companion itself is very busy and does not reply quickly we
> are have second polling loop below which sleeps longer and is non-atomic.

I can confirm, this make huge impact on protocol performance. And this
protocol is not really the speed runner. 

> > Above comments applicable to entire code you have.
> 
> We will look at it.
> 
> > > +static void companion_spi_cpu_to_be32(char *buf)
> > > +{
> > > +       u32 *buf32 = (u32*)buf;
> > > +       int  i;
> > > +
> > > +       for (i = 0; i < (BCP_PACKET_SIZE / sizeof(u32)); i++, buf32++)
> > > +               *buf32 = cpu_to_be32(*buf32);
> > > +}
> > 
> > This entire function should be replaced by one of the helpers from
> > include/linux/byteorder/generic.h
> > 
> > I guess cpu_to_be32_array() is a right one.
> 
> Correct. We are testing the driver against 4.14 and that function is not
> available there. It will be changed later.
> 
> > > +static void companion_spi_be32_to_cpu(char *buf)
> > > +{
> > > +       u32 *buf32 = (u32*)buf;
> > > +       int  i;
> > > +
> > > +       for (i = 0; i < (BCP_PACKET_SIZE / sizeof(u32)); i++, buf32++)
> > > +               *buf32 = be32_to_cpu(*buf32);
> > > +}
> > 
> > Ditto.
> > 
> > Recommendation: check your code against existing helpers.
> 
> Yes, every kernel release brings new helpers. We will have to learn how
> to keep track of the additions.
> 
> > > +               p = (const struct companion_packet*)transfer->tx_buf;
> > 
> > > +       companion_spi_cpu_to_be32((char*)transfer->tx_buf);
> > 
> > If tx_buf is type of void * all these castings are redundant.
> 
> The type is const void*. So the first cast is superfluous, the second
> is not.
> 
> > Also looking at the function, did you consider to use whatever SPI
> > core provides, like CS handling, messages handling and so on?
> 
> SPI CS has to be done manually in our case because we need to wait
> until the Companion signals that it is ready for the transfer.
> 
> Do you have concrete proposals regarding messages handling?

you can send dummy message to set CS.
+	struct spi_transfer t = {
+		.len = 0,
+		.cs_change = 1,
+	};

+	/* send dummy to trigger CS */
+	reinit_completion(&priv->fc_complete);
+	spi_sync_locked(spi, &m);

see my ancient unfinished code:
https://patchwork.kernel.org/patch/9418511/

In general, this part of the code, can be provided as separate driver
which should be called as "SPI 5wire protocol". 3 wires for data, 1 -
chip select, 1 - busy state. Since, the slave cant fit to normal SPI
requirements, and can't be ready within predictable time, busy signal is
needed. Providing this as separate driver or part of SPI framework
should reduce the code for many different drivers.

The actual datagram on top of your spi companion should go to
separate driver. There are similar (almost identical designs)

can :+
char:+
dw:  +
gpio:+--->some_multi_end_mux_protocol--->spi_5wire_protocol->spi--->

In my knowledge, only data format on top of spi_5wire_protocol is
different. See my notes for similar topics:
https://github.com/olerem/icc
https://github.com/olerem/spi-5wire

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Attachment: signature.asc
Description: PGP signature