Re: [PATCH 6/7] mmc: Implement content of UHS-II card initialization functions

[Lai Jason <jasonlai.genesyslogic@gmail.com>]

On Wed, Dec 15, 2021 at 4:29 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Fri, 3 Dec 2021 at 11:51, Jason Lai <jasonlai.genesyslogic@gmail.com> wrote:
> >
> > From: Jason Lai <jason.lai@genesyslogic.com.tw>
> >
> > UHS-II card initialization flow is divided into 2 categories: PHY & Card.
> > Part 1 - PHY Initialization:
> >          Every host controller may need their own avtivation operation
> >          to establish LINK between controller and card. So we add a new
> >          member function(uhs2_detect_init) in struct mmc_host_ops for host
> >          controller use.
> > Part 2 - Card Initialization:
> >          This part can be divided into 6 substeps.
> >          1. Send UHS-II CCMD DEVICE_INIT to card.
> >          2. Send UHS-II CCMD ENUMERATE to card.
> >          3. Send UHS-II Native Read CCMD to obtain capabilities in CFG_REG
> >             of card.
> >          4. Host compares capabilities of host controller and  card,
> >             then write the negotiated values to Setting field in CFG_REG
> >             of card through UHS-II Native Write CCMD.
> >          5. Switch host controller's clock to Range B if it is supported
> >             by both host controller and card.
> >          6. Execute legacy SD initialization flow.
> > Part 3 - Provide a function to tranaform legacy SD command packet into
> >          UHS-II SD-TRAN DCMD packet.
> >
> > Most of the code added above came from Intel's original patch[1].
> >
> > [1]
> > https://patchwork.kernel.org/project/linux-mmc/patch/1419672479-30852-2-
> > git-send-email-yi.y.sun@intel.com/
> >
> > Signed-off-by: Jason Lai <jason.lai@genesyslogic.com.tw>
> > ---
> >  drivers/mmc/core/sd_uhs2.c | 831 ++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 810 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/mmc/core/sd_uhs2.c b/drivers/mmc/core/sd_uhs2.c
> > index 24aa51a6d..d13283d54 100644
> > --- a/drivers/mmc/core/sd_uhs2.c
> > +++ b/drivers/mmc/core/sd_uhs2.c
> > @@ -3,6 +3,7 @@

[...]

> > +/**
> > + * sd_uhs2_cmd_assemble() - build up UHS-II command packet which is embeded in
> > + *                          mmc_command structure
> > + * @cmd:       MMC command to executed
> > + * @uhs2_cmd:  UHS2 command corresponded to MMC command
> > + * @header:    Header field of UHS-II command cxpacket
> > + * @arg:       Argument field of UHS-II command packet
> > + * @payload:   Payload field of UHS-II command packet
> > + * @plen:      Payload length
> > + * @resp:      Response buffer is allocated by caller and it is used to keep
> > + *              the response of CM-TRAN command. For SD-TRAN command, uhs2_resp
> > + *              should be null and SD-TRAN command response should be stored in
> > + *              resp of mmc_command.
> > + * @resp_len:  Response buffer length
> > + *
> > + * Different from legacy SD command, there defined several types of packets
> > + * in UHS-II, which include CM-TRAN and SD-TRAN. These packets are then
> > + * transformed to differential signals and transmitted/received between
> > + * transceiver and receiver.
> > + *
> > + * The UHS-II packet structure(uhs2_cmd) are added in structure mmc_command
> > + * and be filled according to the inputed parameters provided by caller.
> > + *
> > + * For mmc requests, call this function before issuing command to SD card.
> > + * Host controller specific request function will send packet in uhs2_cmd
> > + * to UHS-II SD card.
> > + *
>
> I think we discussed the above functions description earlier. The
> above description is just way too overwhelming. What the function does
> is that fills out the uhs2_cmd data structure, that's it.
>
> As this is a static function, I wouldn't mind that you simply drop the
> entire function description above. Or try to make the description
> short and clear.

I try to make the function description more briefly:
/**
 * sd_uhs2_cmd_assemble() - build up UHS-II command packet which is embeded in
 *                          mmc_command structure
 * @cmd: MMC command to executed
 * @uhs2_cmd: UHS2 command corresponded to MMC command
 * @header: Header field of UHS-II command cxpacket
 * @arg: Argument field of UHS-II command packet
 * @payload: Payload field of UHS-II command packet
 * @plen: Payload length
 * @resp: Response buffer is allocated by caller and it is used to keep
 *              the response of CM-TRAN command. For SD-TRAN command, uhs2_resp
 *              should be null and SD-TRAN command response should be stored in
 *              resp of mmc_command.
 * @resp_len: Response buffer length
 *
 * The uhs2_command structure contains message packets which are transmited/
 * received on UHS-II bus. This function fills in the contents of uhs2_command
 * structure and embededs UHS2 command into mmc_command structure, which is used
 * in legacy SD operation functions.
 *
 */
What do you think?

kind regards,
Jason Lai

> [...]
>
> I have stopped the review at this point, let's see if we can conclude
> on the way forward around my comments on the parts above, to start
> with.
>
> Kind regards
> Uffe