On Friday, April 14, 2023 10:19 PM, Jesper Dangaard Brouer <jbrouer@xxxxxxxxxx> wrote: >On 14/04/2023 14.32, David Laight wrote: >> From: Song, Yoong Siang >>> Sent: 14 April 2023 12:16 >> ... >>>> I have checked Foxville manual for SRRCTL (Split and Replication >>>> Receive >>>> Control) register and below GENMASKs looks correct. >>>> >>>>> -#define IGC_SRRCTL_BSIZEPKT_SHIFT 10 /* Shift _right_ */ >>>>> -#define IGC_SRRCTL_BSIZEHDRSIZE_SHIFT 2 /* Shift _left_ */ >>>>> +#define IGC_SRRCTL_BSIZEPKT_MASK GENMASK(6, 0) >>>>> +#define IGC_SRRCTL_BSIZEPKT_SHIFT 10 /* Shift _right_ */ >>>> >>>> Shift due to 1 KB resolution of BSIZEPKT (manual field BSIZEPACKET) >>> >>> Ya, 1K = BIT(10), so need to shift right 10 bits. >> >> I bet the code would be easier to read if it did 'value / 1024u'. >> The object code will be (much) the same. > >I agree. Code becomes more readable for humans and machine code will be the >same. > >>>>> +#define IGC_SRRCTL_BSIZEHDRSIZE_MASK GENMASK(13, 8) >>>>> +#define IGC_SRRCTL_BSIZEHDRSIZE_SHIFT 2 /* Shift _left_ */ >>>> >>>> This shift is suspicious, but as you inherited it I guess it works. >>>> I did the math, and it happens to work, knowing (from manual) value >>>> is in 64 bytes resolution. >>> >>> It is in 64 = BIT(6) resolution, so need to shift right 6 bits. >>> But it start on 8th bit, so need to shift left 8 bits. >>> Thus, total = shift left 2 bits. >>> >>> I didnt put the explanation into the header file because it is too >>> lengthy and user can know from databook. > >Well, users usually don't have access to the databook (Programming >Interface) PDF. Personally I have it, but I had to go though a lot of red-tape to >get it (under Red Hat NDA). > > >>> >>> How do you feel on the necessary of explaining the shifting logic? >> >> Not everyone trying to grok the code will have the manual. >> Even writing (8 - 6) will help. >> Or (I think) if the value is in bits 13-8 in units of 64 then just: >> ((value >> 8) & 0x1f) * 64 >> gcc will do a single shift right and a mask 9at some point). >> You might want some defines, but if they aren't used much just >> comments that refer to the names in the manual/datasheet can be >> enough. >> > >After Alexander Lobakin opened my eyes for GENMASK, FIELD_PREP and >FIELD_GET, I find that easier to read and work-with these kind of register value >manipulations, see[1] include/linux/bitfield.h. It will also detect if the assigned >value exceeds the mask (like David code handled via mask). (thx Alex) > > [1] >https://elixir.bootlin.com/linux/v6.3-rc6/source/include/linux/bitfield.h#L14 > >So, instead of: > srrctl |= IGC_RX_HDR_LEN << IGC_SRRCTL_BSIZEHDRSIZE_SHIFT; > >I would write > > /* BSIZEHDR value in 64 bytes resolution */ > srrctl |= FIELD_PREP(IGC_SRRCTL_BSIZEHDRSIZE_MASK, (IGC_RX_HDR_LEN / 64)); > >--Jesper Thanks David and Jesper for the comments. I agree to make the code more human readable. Will refactor the code and send out v3 for review. Thanks & Regards Siang
