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.
> 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.
[...]
>>>> +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).
>> 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.
> 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 |
Attachment:
signature.asc
Description: OpenPGP digital signature
