Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <e516d27b-922a-dbbe-e0da-143eb2ab31d8@linaro.org>
Date:   Fri, 18 Feb 2022 09:47:44 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH v3 12/25] bus: mhi: ep: Add support for ring management
Content-Language: en-US
To:     Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc:     mhi@lists.linux.dev, quic_hemantk@quicinc.com,
        quic_bbhatt@quicinc.com, quic_jhugo@quicinc.com,
        vinod.koul@linaro.org, bjorn.andersson@linaro.org,
        dmitry.baryshkov@linaro.org, quic_vbadigan@quicinc.com,
        quic_cang@quicinc.com, quic_skananth@quicinc.com,
        linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20220212182117.49438-1-manivannan.sadhasivam@linaro.org>
 <20220212182117.49438-13-manivannan.sadhasivam@linaro.org>
 <20504c14-6df5-83d1-53e9-85c682a5a3c7@linaro.org>
 <20220218080704.GD11964@workstation> <20220218152327.GA11639@thinkpad>
From:   Alex Elder <elder@linaro.org>
In-Reply-To: <20220218152327.GA11639@thinkpad>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2/18/22 9:23 AM, Manivannan Sadhasivam wrote:
>>>
>>> I'm pretty sure I mentioned this before...  I don't really like these
>>> "DWORD" macros that simply write compute register values to write
>>> out to the TREs.  A TRE is a structure, not a set of registers.  And
>>> a whole TRE can be written or read in a single ARM instruction in
>>> some cases--but most likely you need to define it as a structure
>>> for that to happen.
>>>
>>> struct mhi_tre {
>>> 	__le64 addr;
>>> 	__le16 len_opcode
>>> 	__le16 reserved;
>>> 	__le32 flags;
>>> };
>> Changing the TRE structure requires changes to both host and endpoint
>> stack. So I'll tackle this as an improvement later.
>>
>> Added to TODO list.
> Just did a comparision w/ IPA code and I convinced myself that this conversion
> should happen now itself. So please ignore my above comment.

This might not be that much work, but if it is, I somewhat
apologize for that.  Still, I believe the code will be better
as a result, so I'm not *that* sorry.

If you do this though, I would recommend you do it as a
separate, prerequisite bit of work.  Your series is too
long, and making it longer by adding this will just delay
*everything* a bit more.  So, I'd advise updating the
existing host code this way first, then adapt your patch
series to do things the new way.

Alternatively, do this later (as you earlier said you would),
and don't delay this series any more.  If it works, it works,
and you can always improve it in the future.

And now that your series is getting closer to golden, maybe
you can break it into a few smaller series?  I don't know,
that also can lead to some confusion, so I won't strongly
advocate that.  But it's something to consider for future
work regardless.

					-Alex