Date:   Tue, 19 Sep 2023 11:38:33 +0000
From:   <Parthiban.Veerasooran@...rochip.com>
To:     <andrew@...n.ch>
CC:     <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
        <pabeni@...hat.com>, <robh+dt@...nel.org>,
        <krzysztof.kozlowski+dt@...aro.org>, <conor+dt@...nel.org>,
        <corbet@....net>, <Steen.Hegelund@...rochip.com>,
        <rdunlap@...radead.org>, <horms@...nel.org>,
        <casper.casan@...il.com>, <netdev@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-doc@...r.kernel.org>, <Horatiu.Vultur@...rochip.com>,
        <Woojung.Huh@...rochip.com>, <Nicolas.Ferre@...rochip.com>,
        <UNGLinuxDriver@...rochip.com>, <Thorsten.Kummermehr@...rochip.com>
Subject: Re: [RFC PATCH net-next 1/6] net: ethernet: implement OPEN Alliance
 control transaction interface

Hi Andrew,

On 13/09/23 7:41 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> +static bool oa_tc6_get_parity(u32 p)
>> +{
>> +     bool parity = true;
>> +
>> +     /* This function returns an odd parity bit */
>> +     while (p) {
>> +             parity = !parity;
>> +             p = p & (p - 1);
>> +     }
>> +     return parity;
> 
> Please take a look around and see if you can find another
> implementation in the kernel which can be used. If not, you could copy/paste:
> 
> https://elixir.bootlin.com/linux/latest/source/lib/bch.c#L348
> 
> which is probably more efficient.
Sure, will check out it.
> 
>> +static void oa_tc6_prepare_ctrl_buf(struct oa_tc6 *tc6, u32 addr, u32 val[],
>> +                                 u8 len, bool wnr, u8 *buf, bool ctrl_prot)
>> +{
>> +     u32 hdr;
>> +
>> +     /* Prepare the control header with the required details */
>> +     hdr = FIELD_PREP(CTRL_HDR_DNC, 0) |
>> +           FIELD_PREP(CTRL_HDR_WNR, wnr) |
>> +           FIELD_PREP(CTRL_HDR_AID, 0) |
>> +           FIELD_PREP(CTRL_HDR_MMS, addr >> 16) |
>> +           FIELD_PREP(CTRL_HDR_ADDR, addr) |
>> +           FIELD_PREP(CTRL_HDR_LEN, len - 1);
>> +     hdr |= FIELD_PREP(CTRL_HDR_P, oa_tc6_get_parity(hdr));
>> +     *(u32 *)buf = cpu_to_be32(hdr);
>> +
>> +     if (wnr) {
> 
> What does wnr mean? Maybe give it a more meaningful name, unless it is
> actually something in the standard. Kerneldoc would also help.
Ah, it is "write not read". Shall I name it as "write_not_read" ?
> 
>> +static int oa_tc6_check_control(struct oa_tc6 *tc6, u8 *ptx, u8 *prx, u8 len,
>> +                             bool wnr, bool ctrl_prot)
>> +{
>> +     /* 1st 4 bytes of rx chunk data can be discarded */
>> +     u32 rx_hdr = *(u32 *)&prx[TC6_HDR_SIZE];
>> +     u32 tx_hdr = *(u32 *)ptx;
>> +     u32 rx_data_complement;
>> +     u32 tx_data;
>> +     u32 rx_data;
>> +     u16 pos1;
>> +     u16 pos2;
>> +
>> +     /* If tx hdr and echoed hdr are not equal then there might be an issue
>> +      * with the connection between SPI host and MAC-PHY. Here this case is
>> +      * considered as MAC-PHY is not connected.
> 
> I could understand ENODEV on the first transaction during probe. But
> after that -EIO seems more appropriate. I've also seen USB use -EPROTO
> to indicate a protocol error, which a corrupt message would be.
Ah ok, then in this case I will consider -EIO.
> 
>> +int oa_tc6_perform_ctrl(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len,
>> +                     bool wnr, bool ctrl_prot)
>> +{
>> +     u8 *tx_buf;
>> +     u8 *rx_buf;
>> +     u16 size;
>> +     u16 pos;
>> +     int ret;
>> +
>> +     if (ctrl_prot)
>> +             size = (TC6_HDR_SIZE * 2) + (len * (TC6_HDR_SIZE * 2));
>> +     else
>> +             size = (TC6_HDR_SIZE * 2) + (len * TC6_HDR_SIZE);
> 
> Do you have an idea how big the biggest control message is? Rather
> than allocate these buffers for every transaction, maybe allocate
> maximum size buffers one at startup and keep them in tc6? That will
> reduce overhead and simplify the code.
Ok, as per OA spec, up to 128 consecutive registers read or write can be 
possible. So the maximum possible size would be 1032. As you suggested 
will allocate this size of memory in the startup.
> 
>> +struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
>> +{
>> +     struct oa_tc6 *tc6;
>> +
>> +     if (!spi)
>> +             return NULL;
> 
> This is defensive programming which is generally not liked. You cannot
> do anything without an SPI device, so just assume it is passed, and if
> not, let is explode later and the driver write will quickly fix there
> broken code.
Ah yes, will remove this check.
> 
>> +
>> +     tc6 = kzalloc(sizeof(*tc6), GFP_KERNEL);
>> +     if (!tc6)
>> +             return NULL;
>> +
>> +     tc6->spi = spi;
>> +
>> +     return tc6;
>> +}
>> +EXPORT_SYMBOL_GPL(oa_tc6_init);
>> +
>> +void oa_tc6_deinit(struct oa_tc6 *tc6)
>> +{
>> +     kfree(tc6);
>> +}
>> +EXPORT_SYMBOL_GPL(oa_tc6_deinit);
> 
> Maybe consider a devm_ API to make the MAC driver simpler.
Sorry I don't get your point. Could you please explain bit more?

Best Regards,
Parthiban V

> 
>        Andrew
> 

Powered by blists - more mailing lists