Hi Alex,
Thanks a lot for the inputs. I've incorporated a portion of your suggestions
and kept remaining ones post upstream.
On Wed, Jan 05, 2022 at 06:22:25PM -0600, Alex Elder wrote:
> On 12/2/21 5:35 AM, Manivannan Sadhasivam wrote:
> > Move the common MHI definitions in host "internal.h" to "common.h" so
> > that the endpoint code can make use of them. This also avoids
> > duplicating the definitions in the endpoint stack.
> >
> > Still, the MHI register definitions are not moved since the offsets
> > vary between host and endpoint.
> >
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > ---
> > drivers/bus/mhi/common.h | 182 ++++++++++++++++++++++++++++++++
> > drivers/bus/mhi/host/internal.h | 154 +--------------------------
> > 2 files changed, 183 insertions(+), 153 deletions(-)
> > create mode 100644 drivers/bus/mhi/common.h
> >
[...]
> > +
> > +#define EV_CTX_RESERVED_MASK GENMASK(7, 0)
> > +#define EV_CTX_INTMODC_MASK GENMASK(15, 8)
> > +#define EV_CTX_INTMODC_SHIFT 8
> > +#define EV_CTX_INTMODT_MASK GENMASK(31, 16)
> > +#define EV_CTX_INTMODT_SHIFT 16
> > +struct mhi_event_ctxt {
>
> These fields should all be explicitly marked as little endian.
> It so happens Intel and ARM use that, but defining them as
> simple unsigned values is not correct for an external interface.
>
> This comment applies to the command and channel context structures
> also.
>
Ack
> > + __u32 intmod;
> > + __u32 ertype;
> > + __u32 msivec;
> > +
>
> I think you can just define the entire struct as __packed
> and __aligned(4) rather than defining all of these fields
> with those attributes.
>
This was suggested by Arnd during the MHI host review. He preferred adding the
aligned parameter only for members that require them.
> > + __u64 rbase __packed __aligned(4);
> > + __u64 rlen __packed __aligned(4);
> > + __u64 rp __packed __aligned(4);
> > + __u64 wp __packed __aligned(4);
> > +};
> > +
> > +#define CHAN_CTX_CHSTATE_MASK GENMASK(7, 0)
> > +#define CHAN_CTX_CHSTATE_SHIFT 0
>
> Please eliminate all the _SHIFT definitions like this,
> where you are already defining the corresponding _MASK.
> The _SHIFT is redundant (and could lead to error, and
> takes up extra space).
>
> You are using bitfield operations (like FIELD_GET()) in
> at least some places already. Use them consistently
> throughout the driver. Those macros simplify the code
> and obviate the need for any shift definitions.
>
Ack.
Thanks,
Mani
