On Mon. 25 Jan 2021 at 20:43, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> wrote:
>
> On 1/25/21 12:01 PM, Vincent MAILHOL wrote:
> >>>> +/* CTL/BTR Register Bits */
> >>>> +#define MEN_Z192_CTL0_INITRQ BIT(0)
> >>>> +#define MEN_Z192_CTL0_SLPRQ BIT(1)
> >>>> +#define MEN_Z192_CTL1_INITAK BIT(8)
> >>>> +#define MEN_Z192_CTL1_SLPAK BIT(9)
> >>>> +#define MEN_Z192_CTL1_LISTEN BIT(12)
> >>>> +#define MEN_Z192_CTL1_LOOPB BIT(13)
> >>>> +#define MEN_Z192_CTL1_CANE BIT(15)
> >>>> +#define MEN_Z192_BTR0_BRP GENMASK(21, 16)
> >>>> +#define MEN_Z192_BTR0_SJW GENMASK(23, 22)
> >>>> +#define MEN_Z192_BTR1_TSEG1 GENMASK(27, 24)
> >>>> +#define MEN_Z192_BTR1_TSEG2 GENMASK(30, 28)
> >>>> +#define MEN_Z192_BTR1_SAMP BIT(31)
> >>>
> >>> Please use only one space between the #define and the value.
> >> I had tried that before, but that makes the code difficult to read. The other drivers in mainline have also used more than one space.
> >
> > When I sent my first patch, I received the same comment asking me
> > to remove extra spaces. I am not here to argue which one is the
> > most readable. I just recommend you to do as such because it is
> > the prefered style here.
>
> IMHO one space only pays out, once you add something to the driver, which
> doesn't fit into the current indention level.
>
> [...]
>
> >>>> +struct men_z192_cf_buf {
> >>>> + u32 can_id;
> >>>> + u32 data[2];
> >>>
> >>> Is it possible to make it u8 data[CAN_MAX_DLEN]?
> >> The core has two 32-bit registers for data, therefore array of two 32-bit
> >> elements has been used.
> >
> > First thing, u32 is wrong, it should be either __le32 or __be32
> > to make your code portable.
>
> You should describe the memory/packet/etc... layout of the hardware here, so if
> it's organized in 32 bit values, use them here.
>
> If you have to use __le32, __be32 or u32 depends on how the HW is connected to
> your linux box. If it's USB you have to use the le or be variants.
>
> For memory mapped IO with readl/writel and friend, according to:
>
> > https://elixir.bootlin.com/linux/latest/source/include/asm-generic/io.h#L141
>
> {read,write}{b,w,l,q}() access little endian memory and return result in native
> endianness.
ACK. I missed that point. Now the u32[] type makes sense.
> > The data field of struct can_frame is an array of bytes. You have
> > to make a choice which is either casting cf->data to
> > __be32[] (what you did) or make your data field a u8[].
> >
> > If I am correct, u8[] and __be32[] has the same memory
> > layout. Thus I think it is more readable to declare it your data
> > field as a u8[] despite how it is declared on the
> > controller (and you can add a comment to reflect that).
>
> See above, that should describe your controller.
ACK
> [...]
>
> >>>> +static int men_z192_xmit(struct sk_buff *skb, struct net_device *ndev)
> >>>> +{
> >>>> + struct can_frame *cf = (struct can_frame *)skb->data;
> >>>> + struct men_z192 *priv = netdev_priv(ndev);
> >>>> + struct men_z192_regs __iomem *regs = priv->regs;
> >>>> + struct men_z192_cf_buf __iomem *cf_buf;
> >>>> + int status;
> >>>> + u32 id;
> >>>> + u16 id_11_bits;
> >>>> + u32 id_18_bits;
> >>>> +
> >>>> + if (can_dropped_invalid_skb(ndev, skb))
> >>>> + return NETDEV_TX_OK;
> >>>> +
> >>>> + status = readl(®s->rx_tx_sts);
> >>>> +
> >>>> + if (FIELD_GET(MEN_Z192_TX_BUF_CNT, status) == MEN_Z192_MAX_BUF_LVL) {
> >>>> + netif_stop_queue(ndev);
> >>>> + netdev_err(ndev, "not enough space in TX buffer\n");
> >>>> +
> >>>> + return NETDEV_TX_BUSY;
> >>>> + }
> >>>
> >>> Could you stop the queue in advance to prevent returning NETDEV_TX_BUSY?
> >> sorry, didn'get you. You mean before checking buffer count for max level? The queue is to be stopped when there are no transmission buffers left in the hardawre, right?
> >> And since now buffer is full, means NETDEV_TX_BUSY?
> >
> > Please refer to: https://www.kernel.org/doc/html/latest//networking/driver.html
>
> In short:
> Once your TX-buffer is full, call netif_stop_queue(). This stops the stack to
> call your xmit function. Onc you've successfully send a CAN frame, call
> netif_start_queue().
>
> Have a look at the mcp251xfd driver, how to implement that race-free. You can
> check if xmit is called with full buffer as a safety net and reutrn TX_BUSY, but
> if that happens your driver is racy.
>
> >>>> +
> >>>> + cf_buf = priv->dev_base + MEN_Z192_TX_BUF_START;
> >>>> +
> >>>> + if (cf->can_id & CAN_EFF_FLAG) {
> >>>> + /* Extended frame */
> >>>> + id = cf->can_id & CAN_EFF_MASK;
> >>>> + id_11_bits = FIELD_GET(MEN_Z192_CAN_ID_11_BITS, id);
> >>>> + id_18_bits = FIELD_GET(MEN_Z192_CAN_ID_18_BITS, id);
> >>>> +
> >>>> + id = FIELD_PREP(MEN_Z192_CFBUF_ID_11_BITS, id_11_bits) |
> >>>> + MEN_Z192_CFBUF_SRR |
> >>>> + MEN_Z192_CFBUF_IDE |
> >>>> + FIELD_PREP(MEN_Z192_CFBUF_ID_18_BITS, id_18_bits);
> >>>> +
> >>>> + if (cf->can_id & CAN_RTR_FLAG)
> >>>> + id |= MEN_Z192_CFBUF_E_RTR;
> >>>> + } else {
> >>>> + /* Standard frame */
> >>>> + id = FIELD_PREP(MEN_Z192_CFBUF_ID_11_BITS, cf->can_id);
> >>>> +
> >>>> + if (cf->can_id & CAN_RTR_FLAG)
> >>>> + id |= MEN_Z192_CFBUF_S_RTR;
> >>>> + }
> >>>> +
> >>>> + writel(id, &cf_buf->can_id);
> >>>> + writel(cf->can_dlc, &cf_buf->length);
> >>>> +
> >>>> + if (!(cf->can_id & CAN_RTR_FLAG) && cf->can_dlc) {
> >>>> + writel(be32_to_cpup((__be32 *)(cf->data)), &cf_buf->data[0]);
> >>>> + if (cf->can_dlc > 4)
> >>>> + writel(be32_to_cpup((__be32 *)(cf->data + 4)),
> >>>> + &cf_buf->data[1]);
> >>>
> >>> Why do you cast cf->data to __be32? Shouldn't the cf_buf->data
> >>> which should be of type __be32 instead (or as commented above,
> >>> make cf_buf->data an array of bytes).
>
> cf->data is in __be32 format. (As Vincent said above, an an u8 array for 4
> members has the same mem layout as a __be32).
ACK
> >> wanted to write 4 bytes with correct byte ordering. When function
> >> be32_to_cpup is used for that purpose, it's parameter has to be of type
> >> __be32, which is why casting is done. As already said, there are two 32-bit
> >> registers for storing entire 8-byte CAN data, so it is realized with an
> >> array of 32-bit type with length of 2.
>
> > be32_to_cpup is to convert endianness from the device to the cpu.
> >
> > As commented above, the type of your data[] is wrong.
>
> I don't think so, as writel assumes a __le32 HW-register converted to native
> endianess.
ACK. In the light of Marc's explanation, Abhijeet's original code looks fine.
> > Also, did you test your code on a big endian architecture?
>
> Marc
>
> --
> Pengutronix e.K. | Marc Kleine-Budde |
> Embedded Linux | https://www.pengutronix.de |
> Vertretung West/Dortmund | Phone: +49-231-2826-924 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
>
