Hi Marc,
thx for your comments.
M. Sc.
Abhijeet Badurkar
Software Engineering
Business Unit Computing Systems, duagon
duagon Germany GmbH
Neuwieder Straße 1-7
90411 Nürnberg
Deutschland
Phone +49 911 99 33 5 - 219
www.duagon.com
duagon Germany GmbH - Geschäftsführer: Dr. Michael Goldbach - Mathias Kamolz - Kalina Scott - Handelsregister/Trade Register AG Nürnberg HRB 5540
This message and/or attachments may be privileged or confidential. If you are not the intended recipient, you are hereby notified that you have received this transmittal in error; any review, dissemination, or copying is strictly prohibited. If you received this transmittal in error, please notify us immediately by reply and immediately delete this message and all its attachments. Thank you.
On 25.01.21 12:43, Marc Kleine-Budde wrote:
> * PGP Signed by an unknown key
>
> 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.
ok
>
> [...]
>
>>>>> +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.
ok, will look into this
>
>>>>> +
>>>>> + 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
>
--
Abhijeet Badurkar - Software Engineer
