Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Ulf,

On 2016/11/29 19:11, Ulf Hansson wrote:
> [...]
> 
>

Sorry that I didn't make myself clear.

Our host PHY delay line consists of hundreds of sampling points.
Each sampling point represents a different phase shift.

In lower speed mode, our host driver will scan the delay line.
It will select and test multiple sampling points, other than testing
only single sampling point.

If a sampling point fails to transfer cmd/data, our host driver will
move to test next sampling point, until we find out a group of 
 successful
sampling points which can transfer cmd/data. At last we will select
a perfect one from them.
>>>
>>> Ahh, I see. Unfortunate, this is going to be very hard to implement 
>>> properly.
>>>
>>> The main problem is that the host driver has *no* knowledge about the
>>> internal state of the card, as that is the responsibility of the mmc
>>> core to keep track of.
>>>
>>> If the host driver would send a command during every update of the
>>> "ios" setting, from ->set_ios(), for sure it would lead to commands
>>> being sent that are "forbidden" in the current internal state of the
>>> card.
>>> This would lead to that the card initialization sequence fails,
>>> because the card may move to an unknown internal state and the mmc
>>> core would have no knowledge about what happened.
>>>
>>
>>Yes. In theory, host layer should not initiate a command by itself.
>>
>>We assume that bus is idle and card is stable in Tran state, when core 
>> layer
>>asks host to switch "ios".
> 
> Understand, but this is a wrong assumption. The card may very well in
> another state than Tran state.
> 

   Could you please provide an example that card might not be in Tran state?
   It seems that card should be in Tran state after CMD6 succeed.
   If CMD6 fails, mmc driver will not execute ios setting. Thus ->set_ios()
   will not be called.

>>Besides, we only select the commands which is valid in the whole 
>> procedure,
>>such as CMD8 for eMMC.
>>Those test commands are actually like read operations to card registers.
>>The card will return to Tran state even if transfer fails. It is also easy
>>for host to recover.
> 
> For example, I would recommend you to investigate in detail the
> sequence for when a CMD6 command is sent to the card.
> The host must *not* start sending commands from ->set_ios() during a
> CMD6 sequence. For example a CMD8 is not allowed.
> 
> Moreover, due to this, I wonder if it is even possible to get this HW
> to work properly.
> 

   In my very own opinion, ->set_ios() is only executed after CMD6 sequence
   succeeds, based on current mmc.c/sd.c/sdio.c.
   I personally think that it should not interfere CMD6 sequence.

   I'm afraid that HW cannot help and SW driver has to take care of this.

>>
>>> Hmm..
>>>
>>> Can you specify, *exactly*, under which "ios updates" you need to
>>> verify updated PHY setting changes by sending a cmd/data? Also, please
>>> specify if it's enough to only test the CMD line or also DATA lines.
>>>
>>
>>When one of the three parameters in below changes, our host driver needs
>>to adjust PHY in lower speed mode.
>>1. Speed Mode (timing): like legacy mode --> HS DDR
>>2. Bus Clock: like 400KHz --> 50MHz
>>3. Bus Width: like 1-bit --> 4-bit/8-bit
>>
>>For eMMC, we use CMD8 to test sampling point.
>>For SD, we use CMD13.
>>For SDIO, currently CMD52 is used to read a register from CCCR.
>>Those commands in above are all valid during the whole procedure to switch
>>to high speed mode from legacy mode.
>>
>>It is the best case if the test command can transfer both on CMD and DAT 
>> lines.
>>CMD8 for eMMC can test both CMD line and DAT lines. CMD13 and CMD52 only 
>> test
>>CMD line. We might use ACMD51 for SD and CMD53 for SDIO later thus DAT 
>> lines
>>are also under test.
> 
> Thanks for sharing these details!
> 
> So, if possible, I would recommend you to discuss these issues with
> some of the HW designers. Perhaps you can figure out an alternative
> method of confirming/testing PHY setting changes? Sending commands to
> the card just doesn't work well for all cases.
> 

   Thanks a lot for you patience.

   Actually, we, including HW engineers, have been working on this for
   a very long time. We also test a lot on many actual products. It is
   quiet stable in real use scenarios.

   I know it is still not good enough. It seems to be impossible to find
   another practical and reliable solution, based on our tests.
   Could you please provide some suggestions thus we can try our best to 
improve it
   to meet your requirement?

   Thank you.

Best regards,
Hu Ziji

> Kind regards
> Uffe
> 

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ulf Hansson]

[...]


>>>
>>>Sorry that I didn't make myself clear.
>>>
>>>Our host PHY delay line consists of hundreds of sampling points.
>>>Each sampling point represents a different phase shift.
>>>
>>>In lower speed mode, our host driver will scan the delay line.
>>>It will select and test multiple sampling points, other than testing
>>>only single sampling point.
>>>
>>>If a sampling point fails to transfer cmd/data, our host driver will
>>>move to test next sampling point, until we find out a group of successful
>>>sampling points which can transfer cmd/data. At last we will select
>>>a perfect one from them.
>>
>> Ahh, I see. Unfortunate, this is going to be very hard to implement properly.
>>
>> The main problem is that the host driver has *no* knowledge about the
>> internal state of the card, as that is the responsibility of the mmc
>> core to keep track of.
>>
>> If the host driver would send a command during every update of the
>> "ios" setting, from ->set_ios(), for sure it would lead to commands
>> being sent that are "forbidden" in the current internal state of the
>> card.
>> This would lead to that the card initialization sequence fails,
>> because the card may move to an unknown internal state and the mmc
>> core would have no knowledge about what happened.
>>
>
>Yes. In theory, host layer should not initiate a command by itself.
>
>We assume that bus is idle and card is stable in Tran state, when core 
> layer
>asks host to switch "ios".

Understand, but this is a wrong assumption. The card may very well in
another state than Tran state.

>Besides, we only select the commands which is valid in the whole procedure,
>such as CMD8 for eMMC.
>Those test commands are actually like read operations to card registers.
>The card will return to Tran state even if transfer fails. It is also easy
>for host to recover.

For example, I would recommend you to investigate in detail the
sequence for when a CMD6 command is sent to the card.
The host must *not* start sending commands from ->set_ios() during a
CMD6 sequence. For example a CMD8 is not allowed.

Moreover, due to this, I wonder if it is even possible to get this HW
to work properly.

>
>> Hmm..
>>
>> Can you specify, *exactly*, under which "ios updates" you need to
>> verify updated PHY setting changes by sending a cmd/data? Also, please
>> specify if it's enough to only test the CMD line or also DATA lines.
>>
>
>When one of the three parameters in below changes, our host driver needs
>to adjust PHY in lower speed mode.
>1. Speed Mode (timing): like legacy mode --> HS DDR
>2. Bus Clock: like 400KHz --> 50MHz
>3. Bus Width: like 1-bit --> 4-bit/8-bit
>
>For eMMC, we use CMD8 to test sampling point.
>For SD, we use CMD13.
>For SDIO, currently CMD52 is used to read a register from CCCR.
>Those commands in above are all valid during the whole procedure to switch
>to high speed mode from legacy mode.
>
>It is the best case if the test command can transfer both on CMD and DAT 
> lines.
>CMD8 for eMMC can test both CMD line and DAT lines. CMD13 and CMD52 only 
> test
>CMD line. We might use ACMD51 for SD and CMD53 for SDIO later thus DAT 
> lines
>are also under test.

Thanks for sharing these details!

So, if possible, I would recommend you to discuss these issues with
some of the HW designers. Perhaps you can figure out an alternative
method of confirming/testing PHY setting changes? Sending commands to
the card just doesn't work well for all cases.

Kind regards
Uffe

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Ulf,

On 2016/11/29 15:49, Ulf Hansson wrote:
> On 29 November 2016 at 03:53, Ziji Hu  wrote:
>> Hi Ulf,
>>
>> On 2016/11/28 23:16, Ulf Hansson wrote:
>>> On 28 November 2016 at 12:38, Ziji Hu  wrote:
 Hi Ulf,

 On 2016/11/28 19:13, Ulf Hansson wrote:
>>
>> As you suggest, I replace mmc_wait_for_cmd() with mmc_send_tuning(), 
>> to
>> send commands for testing current sampling point set in our host PHY.
>>
>> According to my test result, it shows that mmc_send_tuning() can 
>> only support
>> tuning command (CMD21/CMD19).
>> As a result, we cannot use mmc_send_tuning() when card is in the 
>> speed modes
>> which doesn't support tuning, such as eMMC HS SDR, eMMC HS DRR and
>> SD SDR 12/SDR25/DDR50. Card will not response to tuning commands in 
>> those
>> speed modes.
>>
>> Could you please provide suggestions for the speed mode in which 
>> tuning is
>> not available?
>>
>
> Normally the mmc host driver shouldn't have to care about what the
> card supports, as that is the responsibility of the mmc core to
> manage.
>
> The host should only need to implement the ->execute_tuning() ops,
> which gets called when the card supports tuning (CMD19/21). Does it
> make sense?
>
I think it is irrelevant to tuning procedure.

Our host requires to adjust PHY setting after each time ios setting
(SDCLK/bus width/speed mode) is changed.
The simplified sequence is:
mmc change ios --> mmc_set_ios() --> ->set_ios() --> after 
 sdhci_set_ios(),
adjust PHY setting.
During PHY setting adjustment, out host driver has to send commands to
test current sampling point. Tuning is another independent step.
>>>
>>> For those speed modes (or other ios changes) that *don't* requires
>>> tuning, then what will you do when you send the command to confirm the
>>> change of PHY setting and it fails?
>>>
>>> My assumption is that you will fail anyway, by propagating the error
>>> to the mmc core. At least that what was my understanding from your
>>> earlier replies, right!?
>>>
>>> Then, I think there are no point having the host driver sending a
>>> command to confirm the PHY settings, as the mmc core will anyway
>>> discover if something goes wrong when the next command is sent.
>>>
>>> Please correct me if I am wrong!
>>>
>>
>>Sorry that I didn't make myself clear.
>>
>>Our host PHY delay line consists of hundreds of sampling points.
>>Each sampling point represents a different phase shift.
>>
>>In lower speed mode, our host driver will scan the delay line.
>>It will select and test multiple sampling points, other than testing
>>only single sampling point.
>>
>>If a sampling point fails to transfer cmd/data, our host driver will
>>move to test next sampling point, until we find out a group of successful
>>sampling points which can transfer cmd/data. At last we will select
>>a perfect one from them.
> 
> Ahh, I see. Unfortunate, this is going to be very hard to implement properly.
> 
> The main problem is that the host driver has *no* knowledge about the
> internal state of the card, as that is the responsibility of the mmc
> core to keep track of.
> 
> If the host driver would send a command during every update of the
> "ios" setting, from ->set_ios(), for sure it would lead to commands
> being sent that are "forbidden" in the current internal state of the
> card.
> This would lead to that the card initialization sequence fails,
> because the card may move to an unknown internal state and the mmc
> core would have no knowledge about what happened.
> 

   Yes. In theory, host layer should not initiate a command by itself.

   We assume that bus is idle and card is stable in Tran state, when core layer
   asks host to switch "ios".
   Besides, we only select the commands which is valid in the whole procedure,
   such as CMD8 for eMMC.
   Those test commands are actually like read operations to card registers.
   The card will return to Tran state even if transfer fails. It is also easy
   for host to recover.  

> Hmm..
> 
> Can you specify, *exactly*, under which "ios updates" you need to
> verify updated PHY setting changes by sending a cmd/data? Also, please
> specify if it's enough to only test the CMD line or also DATA lines.
> 

   When one of the three parameters in below changes, our host driver needs
   to adjust PHY in lower speed mode.
   1. Speed Mode (timing): like legacy mode --> HS DDR
   2. Bus Clock: like 400KHz --> 50MHz
   3. Bus Width: like 1-bit --> 4-bit/8-bit

   For eMMC, we use CMD8 to test sampling point.
   For SD, we use CMD13.
   For SDIO, currently CMD52 is used to read a register from CCCR.
   Those commands in above are all valid during the whole procedure to switch
   to high speed mode from legacy mode.

   It is the best case if th

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ulf Hansson]

On 29 November 2016 at 03:53, Ziji Hu  wrote:
> Hi Ulf,
>
> On 2016/11/28 23:16, Ulf Hansson wrote:
>> On 28 November 2016 at 12:38, Ziji Hu  wrote:
>>> Hi Ulf,
>>>
>>> On 2016/11/28 19:13, Ulf Hansson wrote:
>
> As you suggest, I replace mmc_wait_for_cmd() with mmc_send_tuning(), 
> to
> send commands for testing current sampling point set in our host PHY.
>
> According to my test result, it shows that mmc_send_tuning() can only 
> support
> tuning command (CMD21/CMD19).
> As a result, we cannot use mmc_send_tuning() when card is in the 
> speed modes
> which doesn't support tuning, such as eMMC HS SDR, eMMC HS DRR and
> SD SDR 12/SDR25/DDR50. Card will not response to tuning commands in 
> those
> speed modes.
>
> Could you please provide suggestions for the speed mode in which 
> tuning is
> not available?
>

 Normally the mmc host driver shouldn't have to care about what the
 card supports, as that is the responsibility of the mmc core to
 manage.

 The host should only need to implement the ->execute_tuning() ops,
 which gets called when the card supports tuning (CMD19/21). Does it
 make sense?

>>>I think it is irrelevant to tuning procedure.
>>>
>>>Our host requires to adjust PHY setting after each time ios setting
>>>(SDCLK/bus width/speed mode) is changed.
>>>The simplified sequence is:
>>>mmc change ios --> mmc_set_ios() --> ->set_ios() --> after 
>>> sdhci_set_ios(),
>>>adjust PHY setting.
>>>During PHY setting adjustment, out host driver has to send commands to
>>>test current sampling point. Tuning is another independent step.
>>
>> For those speed modes (or other ios changes) that *don't* requires
>> tuning, then what will you do when you send the command to confirm the
>> change of PHY setting and it fails?
>>
>> My assumption is that you will fail anyway, by propagating the error
>> to the mmc core. At least that what was my understanding from your
>> earlier replies, right!?
>>
>> Then, I think there are no point having the host driver sending a
>> command to confirm the PHY settings, as the mmc core will anyway
>> discover if something goes wrong when the next command is sent.
>>
>> Please correct me if I am wrong!
>>
>
>Sorry that I didn't make myself clear.
>
>Our host PHY delay line consists of hundreds of sampling points.
>Each sampling point represents a different phase shift.
>
>In lower speed mode, our host driver will scan the delay line.
>It will select and test multiple sampling points, other than testing
>only single sampling point.
>
>If a sampling point fails to transfer cmd/data, our host driver will
>move to test next sampling point, until we find out a group of successful
>sampling points which can transfer cmd/data. At last we will select
>a perfect one from them.

Ahh, I see. Unfortunate, this is going to be very hard to implement properly.

The main problem is that the host driver has *no* knowledge about the
internal state of the card, as that is the responsibility of the mmc
core to keep track of.

If the host driver would send a command during every update of the
"ios" setting, from ->set_ios(), for sure it would lead to commands
being sent that are "forbidden" in the current internal state of the
card.
This would lead to that the card initialization sequence fails,
because the card may move to an unknown internal state and the mmc
core would have no knowledge about what happened.

Hmm..

Can you specify, *exactly*, under which "ios updates" you need to
verify updated PHY setting changes by sending a cmd/data? Also, please
specify if it's enough to only test the CMD line or also DATA lines.

Kind regards
Uffe

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Ulf,

On 2016/11/28 23:16, Ulf Hansson wrote:
> On 28 November 2016 at 12:38, Ziji Hu  wrote:
>> Hi Ulf,
>>
>> On 2016/11/28 19:13, Ulf Hansson wrote:

 As you suggest, I replace mmc_wait_for_cmd() with mmc_send_tuning(), to
 send commands for testing current sampling point set in our host PHY.

 According to my test result, it shows that mmc_send_tuning() can only 
 support
 tuning command (CMD21/CMD19).
 As a result, we cannot use mmc_send_tuning() when card is in the speed 
 modes
 which doesn't support tuning, such as eMMC HS SDR, eMMC HS DRR and
 SD SDR 12/SDR25/DDR50. Card will not response to tuning commands in 
 those
 speed modes.

 Could you please provide suggestions for the speed mode in which 
 tuning is
 not available?

>>>
>>> Normally the mmc host driver shouldn't have to care about what the
>>> card supports, as that is the responsibility of the mmc core to
>>> manage.
>>>
>>> The host should only need to implement the ->execute_tuning() ops,
>>> which gets called when the card supports tuning (CMD19/21). Does it
>>> make sense?
>>>
>>I think it is irrelevant to tuning procedure.
>>
>>Our host requires to adjust PHY setting after each time ios setting
>>(SDCLK/bus width/speed mode) is changed.
>>The simplified sequence is:
>>mmc change ios --> mmc_set_ios() --> ->set_ios() --> after 
>> sdhci_set_ios(),
>>adjust PHY setting.
>>During PHY setting adjustment, out host driver has to send commands to
>>test current sampling point. Tuning is another independent step.
> 
> For those speed modes (or other ios changes) that *don't* requires
> tuning, then what will you do when you send the command to confirm the
> change of PHY setting and it fails?
> 
> My assumption is that you will fail anyway, by propagating the error
> to the mmc core. At least that what was my understanding from your
> earlier replies, right!?
> 
> Then, I think there are no point having the host driver sending a
> command to confirm the PHY settings, as the mmc core will anyway
> discover if something goes wrong when the next command is sent.
> 
> Please correct me if I am wrong!
>

   Sorry that I didn't make myself clear.

   Our host PHY delay line consists of hundreds of sampling points.
   Each sampling point represents a different phase shift.

   In lower speed mode, our host driver will scan the delay line.
   It will select and test multiple sampling points, other than testing
   only single sampling point.

   If a sampling point fails to transfer cmd/data, our host driver will
   move to test next sampling point, until we find out a group of successful
   sampling points which can transfer cmd/data. At last we will select
   a perfect one from them.

   Thank you.

Best regards,
Hu Ziji
 
>>
>>Thus our host needs a valid command in PHY setting adjustment. Tuning 
>> command
>>can be borrowed to complete this task in SD SDR50. But for other speed 
>> mode,
>>we have to find out a valid command.
> 
> I thought we agreed on this wasn't necessary? Please see my upper response.
> 
> Kind regards
> Uffe
> 

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ulf Hansson]

On 28 November 2016 at 12:38, Ziji Hu  wrote:
> Hi Ulf,
>
> On 2016/11/28 19:13, Ulf Hansson wrote:
>>>
>>> As you suggest, I replace mmc_wait_for_cmd() with mmc_send_tuning(), to
>>> send commands for testing current sampling point set in our host PHY.
>>>
>>> According to my test result, it shows that mmc_send_tuning() can only 
>>> support
>>> tuning command (CMD21/CMD19).
>>> As a result, we cannot use mmc_send_tuning() when card is in the speed 
>>> modes
>>> which doesn't support tuning, such as eMMC HS SDR, eMMC HS DRR and
>>> SD SDR 12/SDR25/DDR50. Card will not response to tuning commands in 
>>> those
>>> speed modes.
>>>
>>> Could you please provide suggestions for the speed mode in which tuning 
>>> is
>>> not available?
>>>
>>
>> Normally the mmc host driver shouldn't have to care about what the
>> card supports, as that is the responsibility of the mmc core to
>> manage.
>>
>> The host should only need to implement the ->execute_tuning() ops,
>> which gets called when the card supports tuning (CMD19/21). Does it
>> make sense?
>>
>I think it is irrelevant to tuning procedure.
>
>Our host requires to adjust PHY setting after each time ios setting
>(SDCLK/bus width/speed mode) is changed.
>The simplified sequence is:
>mmc change ios --> mmc_set_ios() --> ->set_ios() --> after sdhci_set_ios(),
>adjust PHY setting.
>During PHY setting adjustment, out host driver has to send commands to
>test current sampling point. Tuning is another independent step.

For those speed modes (or other ios changes) that *don't* requires
tuning, then what will you do when you send the command to confirm the
change of PHY setting and it fails?

My assumption is that you will fail anyway, by propagating the error
to the mmc core. At least that what was my understanding from your
earlier replies, right!?

Then, I think there are no point having the host driver sending a
command to confirm the PHY settings, as the mmc core will anyway
discover if something goes wrong when the next command is sent.

Please correct me if I am wrong!

>
>Thus our host needs a valid command in PHY setting adjustment. Tuning 
> command
>can be borrowed to complete this task in SD SDR50. But for other speed 
> mode,
>we have to find out a valid command.

I thought we agreed on this wasn't necessary? Please see my upper response.

Kind regards
Uffe

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Ulf,

On 2016/11/28 19:13, Ulf Hansson wrote:
>>
>> As you suggest, I replace mmc_wait_for_cmd() with mmc_send_tuning(), to
>> send commands for testing current sampling point set in our host PHY.
>>
>> According to my test result, it shows that mmc_send_tuning() can only 
>> support
>> tuning command (CMD21/CMD19).
>> As a result, we cannot use mmc_send_tuning() when card is in the speed 
>> modes
>> which doesn't support tuning, such as eMMC HS SDR, eMMC HS DRR and
>> SD SDR 12/SDR25/DDR50. Card will not response to tuning commands in those
>> speed modes.
>>
>> Could you please provide suggestions for the speed mode in which tuning 
>> is
>> not available?
>>
> 
> Normally the mmc host driver shouldn't have to care about what the
> card supports, as that is the responsibility of the mmc core to
> manage.
> 
> The host should only need to implement the ->execute_tuning() ops,
> which gets called when the card supports tuning (CMD19/21). Does it
> make sense?
> 
   I think it is irrelevant to tuning procedure.

   Our host requires to adjust PHY setting after each time ios setting
   (SDCLK/bus width/speed mode) is changed.
   The simplified sequence is:
   mmc change ios --> mmc_set_ios() --> ->set_ios() --> after sdhci_set_ios(),
   adjust PHY setting.
   During PHY setting adjustment, out host driver has to send commands to
   test current sampling point. Tuning is another independent step.

   Thus our host needs a valid command in PHY setting adjustment. Tuning command
   can be borrowed to complete this task in SD SDR50. But for other speed mode,
   we have to find out a valid command.

   Any suggestion please?

   Thank you.

Best regards,
Hu Ziji

> Kind regards
> Uffe
> 

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ulf Hansson]

>
> As you suggest, I replace mmc_wait_for_cmd() with mmc_send_tuning(), to
> send commands for testing current sampling point set in our host PHY.
>
> According to my test result, it shows that mmc_send_tuning() can only 
> support
> tuning command (CMD21/CMD19).
> As a result, we cannot use mmc_send_tuning() when card is in the speed 
> modes
> which doesn't support tuning, such as eMMC HS SDR, eMMC HS DRR and
> SD SDR 12/SDR25/DDR50. Card will not response to tuning commands in those
> speed modes.
>
> Could you please provide suggestions for the speed mode in which tuning is
> not available?
>

Normally the mmc host driver shouldn't have to care about what the
card supports, as that is the responsibility of the mmc core to
manage.

The host should only need to implement the ->execute_tuning() ops,
which gets called when the card supports tuning (CMD19/21). Does it
make sense?

Kind regards
Uffe

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ulf Hansson]

[...]

>
> Could you please tell me the requirement of "op_code" parameter in
> mmc_send_tuning()?
> According to mmc_send_tuning(),it seems that tuning command(CMD19/CMD21)
> is required. Thus device will not response mmc_send_tuning() if current
> speed mode doesn't support tuning command.
> Please correct me if I am wrong.
>

When the mmc core decides it's time to execute tuning, it invokes the
->execute_tuning() host ops, which has the "opcode" as a parameter.
You should be able to use it when calling mmc_send_tuning().

[...]

Kind regards
Uffe

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Ulf,

On 2016/11/24 23:37, Ziji Hu wrote:
> Hi Ulf,
> 
> On 2016/11/24 22:33, Ulf Hansson wrote:

>>>
>>>As result, our SDHC driver has to implement the functionality to
>>>send commands and check the results, in host layer.
>>>If directly calling mmc_wait_for_cmd() is improper, could you please
>>>give us some suggestions?
>>>
>>>For eMMC, CMD8 is used to test current sampling point set in PHY.
>>
>> Try to use mmc_send_tuning().
>>
> 
> Could you please tell me the requirement of "op_code" parameter in
> mmc_send_tuning()?
> According to mmc_send_tuning(),it seems that tuning command(CMD19/CMD21)
> is required. Thus device will not response mmc_send_tuning() if current
> speed mode doesn't support tuning command.
> Please correct me if I am wrong.
>

As you suggest, I replace mmc_wait_for_cmd() with mmc_send_tuning(), to
send commands for testing current sampling point set in our host PHY.

According to my test result, it shows that mmc_send_tuning() can only 
support
tuning command (CMD21/CMD19).
As a result, we cannot use mmc_send_tuning() when card is in the speed modes
which doesn't support tuning, such as eMMC HS SDR, eMMC HS DRR and
SD SDR 12/SDR25/DDR50. Card will not response to tuning commands in those
speed modes.

Could you please provide suggestions for the speed mode in which tuning is
not available?

Thank you.

Best regards,
Hu Ziji

>>>
> +
> +   return err;
> +}
> +
> +static int __xenon_sdio_delay_adj_test(struct mmc_card *card)
> +{
> +   struct mmc_command cmd = {0};
> +   int err;
> +
> +   cmd.opcode = SD_IO_RW_DIRECT;
> +   cmd.flags = MMC_RSP_R5 | MMC_CMD_AC;
> +
> +   err = mmc_wait_for_cmd(card->host, &cmd, 0);
> +   if (err)
> +   return err;
> +
> +   if (cmd.resp[0] & R5_ERROR)
> +   return -EIO;
> +   if (cmd.resp[0] & R5_FUNCTION_NUMBER)
> +   return -EINVAL;
> +   if (cmd.resp[0] & R5_OUT_OF_RANGE)
> +   return -ERANGE;
> +   return 0;

 No thanks! MMC/SD/SDIO protocol code belongs in the core.

>>>For SDIO, SD_IO_RW_DIRECT command is sent to test current sampling point
>>>in PHY.
>>>Please help provide some suggestion to implement the command transfer.
>>
>> Again, I think mmc_send_tuning() should be possible for you to use.
>>
>> [...]
>>
> +   if (mmc->card)
> +   card = mmc->card;
> +   else
> +   /*
> +* Only valid during initialization
> +* before mmc->card is set
> +*/
> +   card = priv->card_candidate;
> +   if (unlikely(!card)) {
> +   dev_warn(mmc_dev(mmc), "card is not present\n");
> +   return -EINVAL;
> +   }

 That your host need to hold a copy of the card pointer, tells me that
 something is not really correct.

 I might be wrong, if this turns out to be a special case, but I doubt
 it. Although, if it *is* a special such case, we shall most likely try
 to extend the the mmc core layer instead of adding all these hacks in
 your host driver.

>>> This card pointer copies the temporary structure mmc_card
>>> used in mmc_init_card(), mmc_sd_init_card() and mmc_sdio_init_card().
>>> Since we call mmc_wait_for_cmd() to send test commands, we need a copy
>>> of that temporary mmc_card here in our host driver.
>>
>> I see, thanks for clarifying.
>>
>>>
>>> During PHY setting in card initialization, mmc_host->card is not updated
>>> yet with that temporary mmc_card. Thus we are not able to directly use
>>> mmc_host->card. Instead, this card pointer is introduced to enable
>>> mmc_wait_for_cmd().
>>>
>>> If we can improve our host driver to send test commands without 
>>> mmc_card,
>>> this card pointer can be removed.
>>> Could you please share your opinion please?
>>
>> The mmc_send_tuning() API takes the mmc_host as parameter. If you
>> convert to that, perhaps you would be able to remove the need to hold
>> the card pointer.
>>
>> BTW, the reason why mmc_send_tuning() doesn't take the card as a
>> parameter, is exactly those you just described above.
>>
>Got it.
>Thanks a lot for the information.
> 
>Thank you for the great help.
> 
> Best regards,
> Hu Ziji
> 
>> [...]
>>
>> Kind regards
>> Uffe
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Ulf,

On 2016/11/24 22:33, Ulf Hansson wrote:
> [...]
> 
>>>

 +
 +static int __xenon_emmc_delay_adj_test(struct mmc_card *card)
 +{
 +   int err;
 +   u8 *ext_csd = NULL;
 +
 +   err = mmc_get_ext_csd(card, &ext_csd);
 +   kfree(ext_csd);
>>>
>>> Why do you read the ext csd here?
>>>
>>I would like to simply introduce the PHY setting of our SDHC.
>>The target of the PHY setting is to achieve a perfect sampling
>>point for transfers, during card initialization.
> 
> Okay, so the phy is involved when running the tuning sequence.
> 
Actually, all the transfers pass our host PHY.

>>
>>For HS200/HS400/SDR104 whose SDCLK is more than 50MHz, SDHC HW
>>will search for this sampling point with DLL's help.
> 
> Apologize for my ignorance, but what is a "DLL" in this case?
> 
   DLL is Delay-locked Loop. It is a HW module similar to PLL.

>>
>>For other speed mode whose SDLCK is less than or equals to 50MHz,
>>SW has to scan the PHY delay line to find out this perfect sampling
>>point. Our driver sends a command to verify a sampling point
>>in current environment.
> 
> Ahh, okay! I guess the important part here is to not only send a
> command, but also to make sure data becomes transferred on the DAT
> lines, as to confirm your tuning sequence!?

   Yes.
   It is the best if the test command can transfer on DAT lines.

> 
> In cases of HS200/HS400/SDR104 you should be able to use the
> mmc_send_tuning() API, don't you think?

   For HS200/HS400/SDR104, we finally call sdhci_execute_tuning() to
   execute tuning. Those test commands are not used.
   In HS200/HS400/SDR104, HW will provide our host driver with suitable
   tuning step. Our host driver set the tuning step in SDHCI register and
   then start standard tuning sequence. The tuning step value provided
   by our host HW will enhance tuning. 

> 
> For the other cases (lower speed modes) which cards doesn't support
> the tuning command, perhaps you can just assume the PHY scan succeeded
> and then allow to core to continue with the card initialization
> sequence? Or do you foresee any issues with that? My point is that, if
> it will fail - it will fail anyway.

  Usually, our host driver will always successfully scan and select a
  perfect sampling point.
  If driver cannot find any suitable sampling point, it is likely that
  transfers will also fail after init. But usually it is a issue, caused by
  incorrect setting on boards/SOC/other PHY parameters, especially in 
development.
  We will fix the issue and then scan will succeed in final product.

> 
>>
>>As result, our SDHC driver has to implement the functionality to
>>send commands and check the results, in host layer.
>>If directly calling mmc_wait_for_cmd() is improper, could you please
>>give us some suggestions?
>>
>>For eMMC, CMD8 is used to test current sampling point set in PHY.
> 
> Try to use mmc_send_tuning().
> 

Could you please tell me the requirement of "op_code" parameter in
mmc_send_tuning()?
According to mmc_send_tuning(),it seems that tuning command(CMD19/CMD21)
is required. Thus device will not response mmc_send_tuning() if current
speed mode doesn't support tuning command.
Please correct me if I am wrong.

>>
 +
 +   return err;
 +}
 +
 +static int __xenon_sdio_delay_adj_test(struct mmc_card *card)
 +{
 +   struct mmc_command cmd = {0};
 +   int err;
 +
 +   cmd.opcode = SD_IO_RW_DIRECT;
 +   cmd.flags = MMC_RSP_R5 | MMC_CMD_AC;
 +
 +   err = mmc_wait_for_cmd(card->host, &cmd, 0);
 +   if (err)
 +   return err;
 +
 +   if (cmd.resp[0] & R5_ERROR)
 +   return -EIO;
 +   if (cmd.resp[0] & R5_FUNCTION_NUMBER)
 +   return -EINVAL;
 +   if (cmd.resp[0] & R5_OUT_OF_RANGE)
 +   return -ERANGE;
 +   return 0;
>>>
>>> No thanks! MMC/SD/SDIO protocol code belongs in the core.
>>>
>>For SDIO, SD_IO_RW_DIRECT command is sent to test current sampling point
>>in PHY.
>>Please help provide some suggestion to implement the command transfer.
> 
> Again, I think mmc_send_tuning() should be possible for you to use.
> 
> [...]
> 
 +   if (mmc->card)
 +   card = mmc->card;
 +   else
 +   /*
 +* Only valid during initialization
 +* before mmc->card is set
 +*/
 +   card = priv->card_candidate;
 +   if (unlikely(!card)) {
 +   dev_warn(mmc_dev(mmc), "card is not present\n");
 +   return -EINVAL;
 +   }
>>>
>>> That your host need to hold a copy of the card pointer, tells me that
>>> something is not really correct.
>>>
>>> I might be wrong, if this turns out to be a special case, but I doubt
>>> it. Althou

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ulf Hansson]

[...]

>>
>>>
>>> +
>>> +static int __xenon_emmc_delay_adj_test(struct mmc_card *card)
>>> +{
>>> +   int err;
>>> +   u8 *ext_csd = NULL;
>>> +
>>> +   err = mmc_get_ext_csd(card, &ext_csd);
>>> +   kfree(ext_csd);
>>
>> Why do you read the ext csd here?
>>
>I would like to simply introduce the PHY setting of our SDHC.
>The target of the PHY setting is to achieve a perfect sampling
>point for transfers, during card initialization.

Okay, so the phy is involved when running the tuning sequence.

>
>For HS200/HS400/SDR104 whose SDCLK is more than 50MHz, SDHC HW
>will search for this sampling point with DLL's help.

Apologize for my ignorance, but what is a "DLL" in this case?

>
>For other speed mode whose SDLCK is less than or equals to 50MHz,
>SW has to scan the PHY delay line to find out this perfect sampling
>point. Our driver sends a command to verify a sampling point
>in current environment.

Ahh, okay! I guess the important part here is to not only send a
command, but also to make sure data becomes transferred on the DAT
lines, as to confirm your tuning sequence!?

In cases of HS200/HS400/SDR104 you should be able to use the
mmc_send_tuning() API, don't you think?

For the other cases (lower speed modes) which cards doesn't support
the tuning command, perhaps you can just assume the PHY scan succeeded
and then allow to core to continue with the card initialization
sequence? Or do you foresee any issues with that? My point is that, if
it will fail - it will fail anyway.

>
>As result, our SDHC driver has to implement the functionality to
>send commands and check the results, in host layer.
>If directly calling mmc_wait_for_cmd() is improper, could you please
>give us some suggestions?
>
>For eMMC, CMD8 is used to test current sampling point set in PHY.

Try to use mmc_send_tuning().

>
>>> +
>>> +   return err;
>>> +}
>>> +
>>> +static int __xenon_sdio_delay_adj_test(struct mmc_card *card)
>>> +{
>>> +   struct mmc_command cmd = {0};
>>> +   int err;
>>> +
>>> +   cmd.opcode = SD_IO_RW_DIRECT;
>>> +   cmd.flags = MMC_RSP_R5 | MMC_CMD_AC;
>>> +
>>> +   err = mmc_wait_for_cmd(card->host, &cmd, 0);
>>> +   if (err)
>>> +   return err;
>>> +
>>> +   if (cmd.resp[0] & R5_ERROR)
>>> +   return -EIO;
>>> +   if (cmd.resp[0] & R5_FUNCTION_NUMBER)
>>> +   return -EINVAL;
>>> +   if (cmd.resp[0] & R5_OUT_OF_RANGE)
>>> +   return -ERANGE;
>>> +   return 0;
>>
>> No thanks! MMC/SD/SDIO protocol code belongs in the core.
>>
>For SDIO, SD_IO_RW_DIRECT command is sent to test current sampling point
>in PHY.
>Please help provide some suggestion to implement the command transfer.

Again, I think mmc_send_tuning() should be possible for you to use.

[...]

>>> +   if (mmc->card)
>>> +   card = mmc->card;
>>> +   else
>>> +   /*
>>> +* Only valid during initialization
>>> +* before mmc->card is set
>>> +*/
>>> +   card = priv->card_candidate;
>>> +   if (unlikely(!card)) {
>>> +   dev_warn(mmc_dev(mmc), "card is not present\n");
>>> +   return -EINVAL;
>>> +   }
>>
>> That your host need to hold a copy of the card pointer, tells me that
>> something is not really correct.
>>
>> I might be wrong, if this turns out to be a special case, but I doubt
>> it. Although, if it *is* a special such case, we shall most likely try
>> to extend the the mmc core layer instead of adding all these hacks in
>> your host driver.
>>
> This card pointer copies the temporary structure mmc_card
> used in mmc_init_card(), mmc_sd_init_card() and mmc_sdio_init_card().
> Since we call mmc_wait_for_cmd() to send test commands, we need a copy
> of that temporary mmc_card here in our host driver.

I see, thanks for clarifying.

>
> During PHY setting in card initialization, mmc_host->card is not updated
> yet with that temporary mmc_card. Thus we are not able to directly use
> mmc_host->card. Instead, this card pointer is introduced to enable
> mmc_wait_for_cmd().
>
> If we can improve our host driver to send test commands without mmc_card,
> this card pointer can be removed.
> Could you please share your opinion please?

The mmc_send_tuning() API takes the mmc_host as parameter. If you
convert to that, perhaps you would be able to remove the need to hold
the card pointer.

BTW, the reason why mmc_send_tuning() doesn't take the card as a
parameter, is exactly those you just described above.

[...]

Kind regards
Uffe

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Ulf,

   Thanks a lot for the review.

On 2016/11/24 19:37, Ulf Hansson wrote:
> On 31 October 2016 at 12:09, Gregory CLEMENT
>  wrote:
>> From: Ziji Hu 
>>
>> Marvell Xenon eMMC/SD/SDIO Host Controller contains PHY.
>> Three types of PHYs are supported.
>>
>> Add support to multiple types of PHYs init and configuration.
>> Add register definitions of PHYs.
>>
>> Signed-off-by: Hu Ziji 
>> Signed-off-by: Gregory CLEMENT 
>> ---
>>  MAINTAINERS|2 +-
>>  drivers/mmc/host/Makefile  |2 +-
>>  drivers/mmc/host/sdhci-xenon-phy.c | 1181 +-
>>  drivers/mmc/host/sdhci-xenon-phy.h |  157 -
>>  drivers/mmc/host/sdhci-xenon.c |4 +-
>>  drivers/mmc/host/sdhci-xenon.h |   17 +-
>>  6 files changed, 1361 insertions(+), 2 deletions(-)
>>  create mode 100644 drivers/mmc/host/sdhci-xenon-phy.c
>>  create mode 100644 drivers/mmc/host/sdhci-xenon-phy.h
> 
> Can you please consider to split this up somehow!? It would make it
> easier to review...
> 
Sure. I will try to split them into smaller pieces.

> Anyway, allow me to provide some initial feedback, particularly around
> those things that Adrian and you requested for my input.
> 
> [...]
> 
>>
>> +
>> +static int __xenon_emmc_delay_adj_test(struct mmc_card *card)
>> +{
>> +   int err;
>> +   u8 *ext_csd = NULL;
>> +
>> +   err = mmc_get_ext_csd(card, &ext_csd);
>> +   kfree(ext_csd);
> 
> Why do you read the ext csd here?
> 
   I would like to simply introduce the PHY setting of our SDHC.
   The target of the PHY setting is to achieve a perfect sampling
   point for transfers, during card initialization.

   For HS200/HS400/SDR104 whose SDCLK is more than 50MHz, SDHC HW
   will search for this sampling point with DLL's help.

   For other speed mode whose SDLCK is less than or equals to 50MHz,
   SW has to scan the PHY delay line to find out this perfect sampling
   point. Our driver sends a command to verify a sampling point
   in current environment.

   As result, our SDHC driver has to implement the functionality to
   send commands and check the results, in host layer.
   If directly calling mmc_wait_for_cmd() is improper, could you please
   give us some suggestions?

   For eMMC, CMD8 is used to test current sampling point set in PHY.

>> +
>> +   return err;
>> +}
>> +
>> +static int __xenon_sdio_delay_adj_test(struct mmc_card *card)
>> +{
>> +   struct mmc_command cmd = {0};
>> +   int err;
>> +
>> +   cmd.opcode = SD_IO_RW_DIRECT;
>> +   cmd.flags = MMC_RSP_R5 | MMC_CMD_AC;
>> +
>> +   err = mmc_wait_for_cmd(card->host, &cmd, 0);
>> +   if (err)
>> +   return err;
>> +
>> +   if (cmd.resp[0] & R5_ERROR)
>> +   return -EIO;
>> +   if (cmd.resp[0] & R5_FUNCTION_NUMBER)
>> +   return -EINVAL;
>> +   if (cmd.resp[0] & R5_OUT_OF_RANGE)
>> +   return -ERANGE;
>> +   return 0;
> 
> No thanks! MMC/SD/SDIO protocol code belongs in the core.
> 
   For SDIO, SD_IO_RW_DIRECT command is sent to test current sampling point
   in PHY.
   Please help provide some suggestion to implement the command transfer.

>> +}
>> +
>> +static int __xenon_sd_delay_adj_test(struct mmc_card *card)
>> +{
>> +   struct mmc_command cmd = {0};
>> +   int err;
>> +
>> +   cmd.opcode = MMC_SEND_STATUS;
>> +   cmd.arg = card->rca << 16;
>> +   cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
>> +
>> +   err = mmc_wait_for_cmd(card->host, &cmd, 0);
>> +   return err;
> 
> No thanks! MMC/SD/SDIO protocol code belongs in the core.
> 
>> +}
>> +
> 
> [...]
> 
>> +int xenon_phy_adj(struct sdhci_host *host, struct mmc_ios *ios)
>> +{
>> +   struct mmc_host *mmc = host->mmc;
>> +   struct mmc_card *card;
>> +   int ret = 0;
>> +   struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +   struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> +
>> +   if (!host->clock) {
>> +   priv->clock = 0;
>> +   return 0;
>> +   }
>> +
>> +   /*
>> +* The timing, frequency or bus width is changed,
>> +* better to set eMMC PHY based on current setting
>> +* and adjust Xenon SDHC delay.
>> +*/
>> +   if ((host->clock == priv->clock) &&
>> +   (ios->bus_width == priv->bus_width) &&
>> +   (ios->timing == priv->timing))
>> +   return 0;
>> +
>> +   xenon_phy_set(host, ios->timing);
>> +
>> +   /* Update the record */
>> +   priv->bus_width = ios->bus_width;
>> +   /* Temp stage from HS200 to HS400 */
>> +   if (((priv->timing == MMC_TIMING_MMC_HS200) &&
>> +(ios->timing == MMC_TIMING_MMC_HS)) ||
>> +   ((ios->timing == MMC_TIMING_MMC_HS) &&
>> +(priv->clock > host->clock))) {
>> +   priv->timing = ios->timing;
>> +   priv->clock = host->clock;
>> +   return 0;
>> +   }
>> +   /*
>> +  

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ulf Hansson]

On 31 October 2016 at 12:09, Gregory CLEMENT
 wrote:
> From: Ziji Hu 
>
> Marvell Xenon eMMC/SD/SDIO Host Controller contains PHY.
> Three types of PHYs are supported.
>
> Add support to multiple types of PHYs init and configuration.
> Add register definitions of PHYs.
>
> Signed-off-by: Hu Ziji 
> Signed-off-by: Gregory CLEMENT 
> ---
>  MAINTAINERS|2 +-
>  drivers/mmc/host/Makefile  |2 +-
>  drivers/mmc/host/sdhci-xenon-phy.c | 1181 +-
>  drivers/mmc/host/sdhci-xenon-phy.h |  157 -
>  drivers/mmc/host/sdhci-xenon.c |4 +-
>  drivers/mmc/host/sdhci-xenon.h |   17 +-
>  6 files changed, 1361 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/mmc/host/sdhci-xenon-phy.c
>  create mode 100644 drivers/mmc/host/sdhci-xenon-phy.h

Can you please consider to split this up somehow!? It would make it
easier to review...

Anyway, allow me to provide some initial feedback, particularly around
those things that Adrian and you requested for my input.

[...]

>
> +
> +static int __xenon_emmc_delay_adj_test(struct mmc_card *card)
> +{
> +   int err;
> +   u8 *ext_csd = NULL;
> +
> +   err = mmc_get_ext_csd(card, &ext_csd);
> +   kfree(ext_csd);

Why do you read the ext csd here?

> +
> +   return err;
> +}
> +
> +static int __xenon_sdio_delay_adj_test(struct mmc_card *card)
> +{
> +   struct mmc_command cmd = {0};
> +   int err;
> +
> +   cmd.opcode = SD_IO_RW_DIRECT;
> +   cmd.flags = MMC_RSP_R5 | MMC_CMD_AC;
> +
> +   err = mmc_wait_for_cmd(card->host, &cmd, 0);
> +   if (err)
> +   return err;
> +
> +   if (cmd.resp[0] & R5_ERROR)
> +   return -EIO;
> +   if (cmd.resp[0] & R5_FUNCTION_NUMBER)
> +   return -EINVAL;
> +   if (cmd.resp[0] & R5_OUT_OF_RANGE)
> +   return -ERANGE;
> +   return 0;

No thanks! MMC/SD/SDIO protocol code belongs in the core.

> +}
> +
> +static int __xenon_sd_delay_adj_test(struct mmc_card *card)
> +{
> +   struct mmc_command cmd = {0};
> +   int err;
> +
> +   cmd.opcode = MMC_SEND_STATUS;
> +   cmd.arg = card->rca << 16;
> +   cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
> +
> +   err = mmc_wait_for_cmd(card->host, &cmd, 0);
> +   return err;

No thanks! MMC/SD/SDIO protocol code belongs in the core.

> +}
> +

[...]

> +int xenon_phy_adj(struct sdhci_host *host, struct mmc_ios *ios)
> +{
> +   struct mmc_host *mmc = host->mmc;
> +   struct mmc_card *card;
> +   int ret = 0;
> +   struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +   struct sdhci_xenon_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +
> +   if (!host->clock) {
> +   priv->clock = 0;
> +   return 0;
> +   }
> +
> +   /*
> +* The timing, frequency or bus width is changed,
> +* better to set eMMC PHY based on current setting
> +* and adjust Xenon SDHC delay.
> +*/
> +   if ((host->clock == priv->clock) &&
> +   (ios->bus_width == priv->bus_width) &&
> +   (ios->timing == priv->timing))
> +   return 0;
> +
> +   xenon_phy_set(host, ios->timing);
> +
> +   /* Update the record */
> +   priv->bus_width = ios->bus_width;
> +   /* Temp stage from HS200 to HS400 */
> +   if (((priv->timing == MMC_TIMING_MMC_HS200) &&
> +(ios->timing == MMC_TIMING_MMC_HS)) ||
> +   ((ios->timing == MMC_TIMING_MMC_HS) &&
> +(priv->clock > host->clock))) {
> +   priv->timing = ios->timing;
> +   priv->clock = host->clock;
> +   return 0;
> +   }
> +   /*
> +* Skip temp stages from HS400 t0 HS200:
> +* from 200MHz to 52MHz in HS400
> +* from HS400 to HS DDR in 52MHz
> +* from HS DDR to HS in 52MHz
> +* from HS to HS200 in 52MHz
> +*/
> +   if (((priv->timing == MMC_TIMING_MMC_HS400) &&
> +((host->clock == MMC_HIGH_52_MAX_DTR) ||
> + (ios->timing == MMC_TIMING_MMC_DDR52))) ||
> +   ((priv->timing == MMC_TIMING_MMC_DDR52) &&
> +(ios->timing == MMC_TIMING_MMC_HS)) ||
> +   ((ios->timing == MMC_TIMING_MMC_HS200) &&
> +(ios->clock == MMC_HIGH_52_MAX_DTR))) {
> +   priv->timing = ios->timing;
> +   priv->clock = host->clock;
> +   return 0;
> +   }
> +   priv->timing = ios->timing;
> +   priv->clock = host->clock;
> +
> +   /* Legacy mode is a special case */
> +   if (ios->timing == MMC_TIMING_LEGACY)
> +   return 0;
> +
> +   if (mmc->card)
> +   card = mmc->card;
> +   else
> +   /*
> +* Only valid during initialization
> +* before mmc->card is set
> +*/
> +   card = priv->card_candidate;
> +   if (unlikely(!card)) {
> +   dev_warn(mmc_de

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Arnd Bergmann]

On Thursday, November 24, 2016 6:57:18 PM CET Ziji Hu wrote:
> > 
> > Please explain in the changelog why this is not a generic
> > phy driver (or three of them).
> > 
> Actually we tried to put the PHY code into Linux PHY framework.
> But it cannot fit in Linux common PHY framework.
> 
> Our Xenon SDHC PHY register is a part of Xenon SDHC register set.
> Besides, during MMC initialization, MMC sequence has to call several 
> PHY functions to complete timing setting.
> In those PHY setting functions, they have to access SDHC register and 
> know current MMC setting, such as bus width, clock frequency and speed mode.
> As a result, we have to implement PHY under MMC directory.
> 

Ok, that makes sense, just put the same text in the changelog comment.

Arnd

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Arnd,

On 2016/11/24 17:56, Arnd Bergmann wrote:
> On Monday, October 31, 2016 12:09:56 PM CET Gregory CLEMENT wrote:
>> From: Ziji Hu 
>>
>> Marvell Xenon eMMC/SD/SDIO Host Controller contains PHY.
>> Three types of PHYs are supported.
>>
>> Add support to multiple types of PHYs init and configuration.
>> Add register definitions of PHYs.
>>
>> Signed-off-by: Hu Ziji 
>> Signed-off-by: Gregory CLEMENT 
>>
> 
> Please explain in the changelog why this is not a generic
> phy driver (or three of them).
> 
Actually we tried to put the PHY code into Linux PHY framework.
But it cannot fit in Linux common PHY framework.

Our Xenon SDHC PHY register is a part of Xenon SDHC register set.
Besides, during MMC initialization, MMC sequence has to call several 
PHY functions to complete timing setting.
In those PHY setting functions, they have to access SDHC register and 
know current MMC setting, such as bus width, clock frequency and speed mode.
As a result, we have to implement PHY under MMC directory.

Thank you.

Best regards,
Hu Ziji

>   Arnd
> 

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Arnd Bergmann]

On Monday, October 31, 2016 12:09:56 PM CET Gregory CLEMENT wrote:
> From: Ziji Hu 
> 
> Marvell Xenon eMMC/SD/SDIO Host Controller contains PHY.
> Three types of PHYs are supported.
> 
> Add support to multiple types of PHYs init and configuration.
> Add register definitions of PHYs.
> 
> Signed-off-by: Hu Ziji 
> Signed-off-by: Gregory CLEMENT 
> 

Please explain in the changelog why this is not a generic
phy driver (or three of them).

Arnd

[PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Gregory CLEMENT]

From: Ziji Hu 

Marvell Xenon eMMC/SD/SDIO Host Controller contains PHY.
Three types of PHYs are supported.

Add support to multiple types of PHYs init and configuration.
Add register definitions of PHYs.

Signed-off-by: Hu Ziji 
Signed-off-by: Gregory CLEMENT 
---
 MAINTAINERS|2 +-
 drivers/mmc/host/Makefile  |2 +-
 drivers/mmc/host/sdhci-xenon-phy.c | 1181 +-
 drivers/mmc/host/sdhci-xenon-phy.h |  157 -
 drivers/mmc/host/sdhci-xenon.c |4 +-
 drivers/mmc/host/sdhci-xenon.h |   17 +-
 6 files changed, 1361 insertions(+), 2 deletions(-)
 create mode 100644 drivers/mmc/host/sdhci-xenon-phy.c
 create mode 100644 drivers/mmc/host/sdhci-xenon-phy.h

diff --git a/MAINTAINERS b/MAINTAINERS
index d92f4175574b..bb33286aeb48 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7608,7 +7608,7 @@ MARVELL XENON MMC/SD/SDIO HOST CONTROLLER DRIVER
 M: Ziji Hu 
 L: linux-...@vger.kernel.org
 S: Supported
-F: drivers/mmc/host/sdhci-xenon.*
+F: drivers/mmc/host/sdhci-xenon*
 F: Documentation/devicetree/bindings/mmc/marvell,xenon-sdhci.txt
 
 MATROX FRAMEBUFFER DRIVER
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 75eaf743486c..4f2854556ff7 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -82,4 +82,4 @@ ifeq ($(CONFIG_CB710_DEBUG),y)
 endif
 
 obj-$(CONFIG_MMC_SDHCI_XENON)  += sdhci-xenon-driver.o
-sdhci-xenon-driver-y   += sdhci-xenon.o
+sdhci-xenon-driver-y   += sdhci-xenon.o sdhci-xenon-phy.o
diff --git a/drivers/mmc/host/sdhci-xenon-phy.c 
b/drivers/mmc/host/sdhci-xenon-phy.c
new file mode 100644
index ..af32f8842e0b
--- /dev/null
+++ b/drivers/mmc/host/sdhci-xenon-phy.c
@@ -0,0 +1,1181 @@
+/*
+ * PHY support for Xenon SDHC
+ *
+ * Copyright (C) 2016 Marvell, All Rights Reserved.
+ *
+ * Author: Hu Ziji 
+ * Date:   2016-8-24
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "sdhci.h"
+#include "sdhci-pltfm.h"
+#include "sdhci-xenon.h"
+
+static const char * const phy_types[] = {
+   "sdh phy",
+   "emmc 5.0 phy",
+   "emmc 5.1 phy"
+};
+
+enum phy_type_enum {
+   SDH_PHY,
+   EMMC_5_0_PHY,
+   EMMC_5_1_PHY,
+   NR_PHY_TYPES
+};
+
+struct soc_pad_ctrl_table {
+   const char *soc;
+   void (*set_soc_pad)(struct sdhci_host *host,
+   unsigned char signal_voltage);
+};
+
+struct soc_pad_ctrl {
+   /* Register address of SOC PHY PAD ctrl */
+   void __iomem*reg;
+   /* SOC PHY PAD ctrl type */
+   enum soc_pad_ctrl_type pad_type;
+   /* SOC specific operation to set SOC PHY PAD */
+   void (*set_soc_pad)(struct sdhci_host *host,
+   unsigned char signal_voltage);
+};
+
+static struct xenon_emmc_phy_regs  xenon_emmc_5_0_phy_regs = {
+   .timing_adj = EMMC_5_0_PHY_TIMING_ADJUST,
+   .func_ctrl  = EMMC_5_0_PHY_FUNC_CONTROL,
+   .pad_ctrl   = EMMC_5_0_PHY_PAD_CONTROL,
+   .pad_ctrl2  = EMMC_5_0_PHY_PAD_CONTROL2,
+   .dll_ctrl   = EMMC_5_0_PHY_DLL_CONTROL,
+   .logic_timing_adj = EMMC_5_0_PHY_LOGIC_TIMING_ADJUST,
+   .delay_mask = EMMC_5_0_PHY_FIXED_DELAY_MASK,
+   .dll_update = DLL_UPDATE_STROBE_5_0,
+};
+
+static struct xenon_emmc_phy_regs  xenon_emmc_5_1_phy_regs = {
+   .timing_adj = EMMC_PHY_TIMING_ADJUST,
+   .func_ctrl  = EMMC_PHY_FUNC_CONTROL,
+   .pad_ctrl   = EMMC_PHY_PAD_CONTROL,
+   .pad_ctrl2  = EMMC_PHY_PAD_CONTROL2,
+   .dll_ctrl   = EMMC_PHY_DLL_CONTROL,
+   .logic_timing_adj = EMMC_PHY_LOGIC_TIMING_ADJUST,
+   .delay_mask = EMMC_PHY_FIXED_DELAY_MASK,
+   .dll_update = DLL_UPDATE,
+};
+
+static int xenon_delay_adj_test(struct mmc_card *card);
+
+/*
+ * eMMC PHY configuration and operations
+ */
+struct emmc_phy_params {
+   boolslow_mode;
+
+   u8  znr;
+   u8  zpr;
+
+   /* Nr of consecutive Sampling Points of a Valid Sampling Window */
+   u8  nr_tun_times;
+   /* Divider for calculating Tuning Step */
+   u8  tun_step_divider;
+
+   struct soc_pad_ctrl pad_ctrl;
+};
+
+static void xenon_emmc_phy_strobe_delay_adj(struct sdhci_host *host,
+   struct mmc_card *card);
+static int xenon_emmc_phy_fix_sampl_delay_adj(struct sdhci_host *host,
+ struct mmc_card *card);
+static void xenon_emmc_phy_set(struct sdhci_host *host,
+  unsigned char timing);
+static void xenon_emmc_set_soc_pad(struct sdhci_host *host,
+  unsigned char signal_voltage);
+
+static const struct xenon_phy_ops

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Adrian Hunter]

On 18/10/16 15:04, Ziji Hu wrote:
> Hi Adrian,
> 
> On 2016/10/17 15:55, Adrian Hunter wrote:
>> On 12/10/16 15:17, Ziji Hu wrote:
>>> Hi Adrian,
>>>
>>> On 2016/10/11 20:39, Adrian Hunter wrote:
> 
> +static int __xenon_emmc_delay_adj_test(struct mmc_card *card)
> +{
> + int err;
> + u8 *ext_csd = NULL;
> +
> + err = mmc_get_ext_csd(card, &ext_csd);
> + kfree(ext_csd);
> +
> + return err;
> +}
> +
> +static int __xenon_sdio_delay_adj_test(struct mmc_card *card)
> +{
> + struct mmc_command cmd = {0};
> + int err;
> +
> + cmd.opcode = SD_IO_RW_DIRECT;
> + cmd.flags = MMC_RSP_R5 | MMC_CMD_AC;
> +
> + err = mmc_wait_for_cmd(card->host, &cmd, 0);
> + if (err)
> + return err;
> +
> + if (cmd.resp[0] & R5_ERROR)
> + return -EIO;
> + if (cmd.resp[0] & R5_FUNCTION_NUMBER)
> + return -EINVAL;
> + if (cmd.resp[0] & R5_OUT_OF_RANGE)
> + return -ERANGE;
> + return 0;
> +}
> +
> +static int __xenon_sd_delay_adj_test(struct mmc_card *card)
> +{
> + struct mmc_command cmd = {0};
> + int err;
> +
> + cmd.opcode = MMC_SEND_STATUS;
> + cmd.arg = card->rca << 16;
> + cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
> +
> + err = mmc_wait_for_cmd(card->host, &cmd, 0);
> + return err;
> +}
> +
> +static int xenon_delay_adj_test(struct mmc_card *card)
> +{
> + WARN_ON(!card);
> + WARN_ON(!card->host);
> +
> + if (mmc_card_mmc(card))
> + return __xenon_emmc_delay_adj_test(card);
> + else if (mmc_card_sd(card))
> + return __xenon_sd_delay_adj_test(card);
> + else if (mmc_card_sdio(card))
> + return __xenon_sdio_delay_adj_test(card);
> + else
> + return -EINVAL;
> +}

 So you are issuing commands from the ->set_ios() callback.  I would want to
 get Ulf's OK for that before going further.

>>> Yes, you are correct.
>>> In some speed mode, Xenon SDHC has to send a series of transfers to 
>>> search for a perfect sampling point in PHY delay line.
>>> It is like tuning process.
>>>
 One thing: you will need to ensure you don't trigger get HS400 re-tuning
 because it will call back into ->set_ios().

>>> Could you please make the term "HS400 re-tuning" more detailed?
>>> In current MMC driver, "HS400 re-tuning" will go back to HS200, execute 
>>> HS200 tuning and come back to HS400.
>>> I'm sure our Xenon SDHC will not execute it.
>>
>> Currently, re-tuning is automatically enabled whenever tuning is executed,
>> and then re-tuning will be done periodically or after CRC errors.  The
>> function to disable re-tuning mmc_retune_disable() is not exported, however
>> if you have you are determining the sampling point your own way, you could
>> simply not implement ->execute_tuning() and then there would be no tuning
>> and no re-tuning.
>>
> 
>   It is a little complex in our Xenon SDHC.
>   For the speed mode which requests tuning, such as SDR104 and HS200, our 
> driver will execute standard tuning as spec requires.
>   However, for those speed mode in which tuning is not requested in spec, 
> our driver has to issues commands to search for the best sampling point.
>   
>   It seems that HS400 re-tuning/tuning is disabled by default. Is it 
> correct?

No, it is enabled by default - there is currently no periodic re-tuning  for
HS400 but CRC errors or runtime suspend / resume will cause re-tuning.

> 
>>>
>>> However, in coming eMMC 5.2, there is a real HS400 re-tuning, in which 
>>> tuning can be directly executed in HS400 mode.
>>> Our Xenon SDHC will neither trigger this HS400 re-tuning.
>>> But since so far there is no such feature in MMC driver, I cannot give 
>>> you a 100% guarantee now.
>>>
 And you have the problem that you need to get a reference to the card 
 before
 the card device has been added.  As I wrote in response to the previous
 patch, you should get Ulf's help with that too.

>>> Sure.
>>> I will get card_candidate solved at first.
>>> Thank you again for your review and help.
>>>
>>> Thank you.
>>>
>>> Best regards,
>>> Hu Ziji

>>>
>>
> 

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Adrian,

On 2016/10/17 15:55, Adrian Hunter wrote:
> On 12/10/16 15:17, Ziji Hu wrote:
>> Hi Adrian,
>>
>> On 2016/10/11 20:39, Adrian Hunter wrote:

 +static int __xenon_emmc_delay_adj_test(struct mmc_card *card)
 +{
 +  int err;
 +  u8 *ext_csd = NULL;
 +
 +  err = mmc_get_ext_csd(card, &ext_csd);
 +  kfree(ext_csd);
 +
 +  return err;
 +}
 +
 +static int __xenon_sdio_delay_adj_test(struct mmc_card *card)
 +{
 +  struct mmc_command cmd = {0};
 +  int err;
 +
 +  cmd.opcode = SD_IO_RW_DIRECT;
 +  cmd.flags = MMC_RSP_R5 | MMC_CMD_AC;
 +
 +  err = mmc_wait_for_cmd(card->host, &cmd, 0);
 +  if (err)
 +  return err;
 +
 +  if (cmd.resp[0] & R5_ERROR)
 +  return -EIO;
 +  if (cmd.resp[0] & R5_FUNCTION_NUMBER)
 +  return -EINVAL;
 +  if (cmd.resp[0] & R5_OUT_OF_RANGE)
 +  return -ERANGE;
 +  return 0;
 +}
 +
 +static int __xenon_sd_delay_adj_test(struct mmc_card *card)
 +{
 +  struct mmc_command cmd = {0};
 +  int err;
 +
 +  cmd.opcode = MMC_SEND_STATUS;
 +  cmd.arg = card->rca << 16;
 +  cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
 +
 +  err = mmc_wait_for_cmd(card->host, &cmd, 0);
 +  return err;
 +}
 +
 +static int xenon_delay_adj_test(struct mmc_card *card)
 +{
 +  WARN_ON(!card);
 +  WARN_ON(!card->host);
 +
 +  if (mmc_card_mmc(card))
 +  return __xenon_emmc_delay_adj_test(card);
 +  else if (mmc_card_sd(card))
 +  return __xenon_sd_delay_adj_test(card);
 +  else if (mmc_card_sdio(card))
 +  return __xenon_sdio_delay_adj_test(card);
 +  else
 +  return -EINVAL;
 +}
>>>
>>> So you are issuing commands from the ->set_ios() callback.  I would want to
>>> get Ulf's OK for that before going further.
>>>
>>  Yes, you are correct.
>>  In some speed mode, Xenon SDHC has to send a series of transfers to 
>> search for a perfect sampling point in PHY delay line.
>>  It is like tuning process.
>>
>>> One thing: you will need to ensure you don't trigger get HS400 re-tuning
>>> because it will call back into ->set_ios().
>>>
>>  Could you please make the term "HS400 re-tuning" more detailed?
>>  In current MMC driver, "HS400 re-tuning" will go back to HS200, execute 
>> HS200 tuning and come back to HS400.
>>  I'm sure our Xenon SDHC will not execute it.
> 
> Currently, re-tuning is automatically enabled whenever tuning is executed,
> and then re-tuning will be done periodically or after CRC errors.  The
> function to disable re-tuning mmc_retune_disable() is not exported, however
> if you have you are determining the sampling point your own way, you could
> simply not implement ->execute_tuning() and then there would be no tuning
> and no re-tuning.
>

It is a little complex in our Xenon SDHC.
For the speed mode which requests tuning, such as SDR104 and HS200, our 
driver will execute standard tuning as spec requires.
However, for those speed mode in which tuning is not requested in spec, 
our driver has to issues commands to search for the best sampling point.

It seems that HS400 re-tuning/tuning is disabled by default. Is it 
correct?

>>
>>  However, in coming eMMC 5.2, there is a real HS400 re-tuning, in which 
>> tuning can be directly executed in HS400 mode.
>>  Our Xenon SDHC will neither trigger this HS400 re-tuning.
>>  But since so far there is no such feature in MMC driver, I cannot give 
>> you a 100% guarantee now.
>>
>>> And you have the problem that you need to get a reference to the card before
>>> the card device has been added.  As I wrote in response to the previous
>>> patch, you should get Ulf's help with that too.
>>>
>>  Sure.
>>  I will get card_candidate solved at first.
>>  Thank you again for your review and help.
>>
>>  Thank you.
>>
>> Best regards,
>> Hu Ziji
>>>
>>
> 

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Adrian Hunter]

On 12/10/16 15:17, Ziji Hu wrote:
> Hi Adrian,
> 
> On 2016/10/11 20:39, Adrian Hunter wrote:
>> On 07/10/16 18:22, Gregory CLEMENT wrote:
>>> From: Ziji Hu 
>>>
>>> Marvell Xenon eMMC/SD/SDIO Host Controller contains PHY.
>>> Three types of PHYs are supported.
>>>
>>> Add support to multiple types of PHYs init and configuration.
>>> Add register definitions of PHYs.
>>>
>>> Signed-off-by: Hu Ziji 
>>> Reviewed-by: Gregory CLEMENT 
>>> Signed-off-by: Gregory CLEMENT 
>>> ---
>>>  MAINTAINERS|1 +-
>>>  drivers/mmc/host/Makefile  |2 +-
>>>  drivers/mmc/host/sdhci-xenon-phy.c | 1141 +-
>>>  drivers/mmc/host/sdhci-xenon-phy.h |  157 -
>>>  drivers/mmc/host/sdhci-xenon.c |4 +-
>>>  drivers/mmc/host/sdhci-xenon.h |   17 +-
>>>  6 files changed, 1321 insertions(+), 1 deletion(-)
>>>  create mode 100644 drivers/mmc/host/sdhci-xenon-phy.c
>>>  create mode 100644 drivers/mmc/host/sdhci-xenon-phy.h
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 859420e5dfd3..b5673c2ee5f2 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -7583,6 +7583,7 @@ M:Ziji Hu 
>>>  L: linux-...@vger.kernel.org
>>>  S: Supported
>>>  F: drivers/mmc/host/sdhci-xenon.*
>>> +F: drivers/mmc/host/sdhci-xenon-phy.*
>>>  F: Documentation/devicetree/bindings/mmc/marvell,sdhci-xenon.txt
>>>  
>>>  MATROX FRAMEBUFFER DRIVER
>>> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
>>> index 75eaf743486c..4f2854556ff7 100644
>>> --- a/drivers/mmc/host/Makefile
>>> +++ b/drivers/mmc/host/Makefile
>>> @@ -82,4 +82,4 @@ ifeq ($(CONFIG_CB710_DEBUG),y)
>>>  endif
>>>  
>>>  obj-$(CONFIG_MMC_SDHCI_XENON)  += sdhci-xenon-driver.o
>>> -sdhci-xenon-driver-y   += sdhci-xenon.o
>>> +sdhci-xenon-driver-y   += sdhci-xenon.o sdhci-xenon-phy.o
>>> diff --git a/drivers/mmc/host/sdhci-xenon-phy.c 
>>> b/drivers/mmc/host/sdhci-xenon-phy.c
>>> new file mode 100644
>>> index ..4eb8fea1bec9
>>> --- /dev/null
>>> +++ b/drivers/mmc/host/sdhci-xenon-phy.c
>>
>> 
>>
>>> +static int __xenon_emmc_delay_adj_test(struct mmc_card *card)
>>> +{
>>> +   int err;
>>> +   u8 *ext_csd = NULL;
>>> +
>>> +   err = mmc_get_ext_csd(card, &ext_csd);
>>> +   kfree(ext_csd);
>>> +
>>> +   return err;
>>> +}
>>> +
>>> +static int __xenon_sdio_delay_adj_test(struct mmc_card *card)
>>> +{
>>> +   struct mmc_command cmd = {0};
>>> +   int err;
>>> +
>>> +   cmd.opcode = SD_IO_RW_DIRECT;
>>> +   cmd.flags = MMC_RSP_R5 | MMC_CMD_AC;
>>> +
>>> +   err = mmc_wait_for_cmd(card->host, &cmd, 0);
>>> +   if (err)
>>> +   return err;
>>> +
>>> +   if (cmd.resp[0] & R5_ERROR)
>>> +   return -EIO;
>>> +   if (cmd.resp[0] & R5_FUNCTION_NUMBER)
>>> +   return -EINVAL;
>>> +   if (cmd.resp[0] & R5_OUT_OF_RANGE)
>>> +   return -ERANGE;
>>> +   return 0;
>>> +}
>>> +
>>> +static int __xenon_sd_delay_adj_test(struct mmc_card *card)
>>> +{
>>> +   struct mmc_command cmd = {0};
>>> +   int err;
>>> +
>>> +   cmd.opcode = MMC_SEND_STATUS;
>>> +   cmd.arg = card->rca << 16;
>>> +   cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
>>> +
>>> +   err = mmc_wait_for_cmd(card->host, &cmd, 0);
>>> +   return err;
>>> +}
>>> +
>>> +static int xenon_delay_adj_test(struct mmc_card *card)
>>> +{
>>> +   WARN_ON(!card);
>>> +   WARN_ON(!card->host);
>>> +
>>> +   if (mmc_card_mmc(card))
>>> +   return __xenon_emmc_delay_adj_test(card);
>>> +   else if (mmc_card_sd(card))
>>> +   return __xenon_sd_delay_adj_test(card);
>>> +   else if (mmc_card_sdio(card))
>>> +   return __xenon_sdio_delay_adj_test(card);
>>> +   else
>>> +   return -EINVAL;
>>> +}
>>
>> So you are issuing commands from the ->set_ios() callback.  I would want to
>> get Ulf's OK for that before going further.
>>
>   Yes, you are correct.
>   In some speed mode, Xenon SDHC has to send a series of transfers to 
> search for a perfect sampling point in PHY delay line.
>   It is like tuning process.
> 
>> One thing: you will need to ensure you don't trigger get HS400 re-tuning
>> because it will call back into ->set_ios().
>>
>   Could you please make the term "HS400 re-tuning" more detailed?
>   In current MMC driver, "HS400 re-tuning" will go back to HS200, execute 
> HS200 tuning and come back to HS400.
>   I'm sure our Xenon SDHC will not execute it.

Currently, re-tuning is automatically enabled whenever tuning is executed,
and then re-tuning will be done periodically or after CRC errors.  The
function to disable re-tuning mmc_retune_disable() is not exported, however
if you have you are determining the sampling point your own way, you could
simply not implement ->execute_tuning() and then there would be no tuning
and no re-tuning.

> 
>   However, in coming eMMC 5.2, there is a real HS400 re-tuning, in which 
> tuning can be directly executed in HS400 mode.
>   Our Xenon SDHC will neither trigger this HS400 re-tuning.
>   B

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Adrian,

On 2016/10/11 20:39, Adrian Hunter wrote:
> On 07/10/16 18:22, Gregory CLEMENT wrote:
>> From: Ziji Hu 
>>
>> Marvell Xenon eMMC/SD/SDIO Host Controller contains PHY.
>> Three types of PHYs are supported.
>>
>> Add support to multiple types of PHYs init and configuration.
>> Add register definitions of PHYs.
>>
>> Signed-off-by: Hu Ziji 
>> Reviewed-by: Gregory CLEMENT 
>> Signed-off-by: Gregory CLEMENT 
>> ---
>>  MAINTAINERS|1 +-
>>  drivers/mmc/host/Makefile  |2 +-
>>  drivers/mmc/host/sdhci-xenon-phy.c | 1141 +-
>>  drivers/mmc/host/sdhci-xenon-phy.h |  157 -
>>  drivers/mmc/host/sdhci-xenon.c |4 +-
>>  drivers/mmc/host/sdhci-xenon.h |   17 +-
>>  6 files changed, 1321 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/mmc/host/sdhci-xenon-phy.c
>>  create mode 100644 drivers/mmc/host/sdhci-xenon-phy.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 859420e5dfd3..b5673c2ee5f2 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -7583,6 +7583,7 @@ M: Ziji Hu 
>>  L:  linux-...@vger.kernel.org
>>  S:  Supported
>>  F:  drivers/mmc/host/sdhci-xenon.*
>> +F:  drivers/mmc/host/sdhci-xenon-phy.*
>>  F:  Documentation/devicetree/bindings/mmc/marvell,sdhci-xenon.txt
>>  
>>  MATROX FRAMEBUFFER DRIVER
>> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
>> index 75eaf743486c..4f2854556ff7 100644
>> --- a/drivers/mmc/host/Makefile
>> +++ b/drivers/mmc/host/Makefile
>> @@ -82,4 +82,4 @@ ifeq ($(CONFIG_CB710_DEBUG),y)
>>  endif
>>  
>>  obj-$(CONFIG_MMC_SDHCI_XENON)   += sdhci-xenon-driver.o
>> -sdhci-xenon-driver-y+= sdhci-xenon.o
>> +sdhci-xenon-driver-y+= sdhci-xenon.o sdhci-xenon-phy.o
>> diff --git a/drivers/mmc/host/sdhci-xenon-phy.c 
>> b/drivers/mmc/host/sdhci-xenon-phy.c
>> new file mode 100644
>> index ..4eb8fea1bec9
>> --- /dev/null
>> +++ b/drivers/mmc/host/sdhci-xenon-phy.c
> 
> 
> 
>> +static int __xenon_emmc_delay_adj_test(struct mmc_card *card)
>> +{
>> +int err;
>> +u8 *ext_csd = NULL;
>> +
>> +err = mmc_get_ext_csd(card, &ext_csd);
>> +kfree(ext_csd);
>> +
>> +return err;
>> +}
>> +
>> +static int __xenon_sdio_delay_adj_test(struct mmc_card *card)
>> +{
>> +struct mmc_command cmd = {0};
>> +int err;
>> +
>> +cmd.opcode = SD_IO_RW_DIRECT;
>> +cmd.flags = MMC_RSP_R5 | MMC_CMD_AC;
>> +
>> +err = mmc_wait_for_cmd(card->host, &cmd, 0);
>> +if (err)
>> +return err;
>> +
>> +if (cmd.resp[0] & R5_ERROR)
>> +return -EIO;
>> +if (cmd.resp[0] & R5_FUNCTION_NUMBER)
>> +return -EINVAL;
>> +if (cmd.resp[0] & R5_OUT_OF_RANGE)
>> +return -ERANGE;
>> +return 0;
>> +}
>> +
>> +static int __xenon_sd_delay_adj_test(struct mmc_card *card)
>> +{
>> +struct mmc_command cmd = {0};
>> +int err;
>> +
>> +cmd.opcode = MMC_SEND_STATUS;
>> +cmd.arg = card->rca << 16;
>> +cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
>> +
>> +err = mmc_wait_for_cmd(card->host, &cmd, 0);
>> +return err;
>> +}
>> +
>> +static int xenon_delay_adj_test(struct mmc_card *card)
>> +{
>> +WARN_ON(!card);
>> +WARN_ON(!card->host);
>> +
>> +if (mmc_card_mmc(card))
>> +return __xenon_emmc_delay_adj_test(card);
>> +else if (mmc_card_sd(card))
>> +return __xenon_sd_delay_adj_test(card);
>> +else if (mmc_card_sdio(card))
>> +return __xenon_sdio_delay_adj_test(card);
>> +else
>> +return -EINVAL;
>> +}
> 
> So you are issuing commands from the ->set_ios() callback.  I would want to
> get Ulf's OK for that before going further.
> 
Yes, you are correct.
In some speed mode, Xenon SDHC has to send a series of transfers to 
search for a perfect sampling point in PHY delay line.
It is like tuning process.

> One thing: you will need to ensure you don't trigger get HS400 re-tuning
> because it will call back into ->set_ios().
> 
Could you please make the term "HS400 re-tuning" more detailed?
In current MMC driver, "HS400 re-tuning" will go back to HS200, execute 
HS200 tuning and come back to HS400.
I'm sure our Xenon SDHC will not execute it.

However, in coming eMMC 5.2, there is a real HS400 re-tuning, in which 
tuning can be directly executed in HS400 mode.
Our Xenon SDHC will neither trigger this HS400 re-tuning.
But since so far there is no such feature in MMC driver, I cannot give 
you a 100% guarantee now.

> And you have the problem that you need to get a reference to the card before
> the card device has been added.  As I wrote in response to the previous
> patch, you should get Ulf's help with that too.
> 
Sure.
I will get card_candidate solved at first.
Thank you again for your review and help.

Thank you.

Best regards,
Hu Ziji
> 

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Adrian Hunter]

On 07/10/16 18:22, Gregory CLEMENT wrote:
> From: Ziji Hu 
> 
> Marvell Xenon eMMC/SD/SDIO Host Controller contains PHY.
> Three types of PHYs are supported.
> 
> Add support to multiple types of PHYs init and configuration.
> Add register definitions of PHYs.
> 
> Signed-off-by: Hu Ziji 
> Reviewed-by: Gregory CLEMENT 
> Signed-off-by: Gregory CLEMENT 
> ---
>  MAINTAINERS|1 +-
>  drivers/mmc/host/Makefile  |2 +-
>  drivers/mmc/host/sdhci-xenon-phy.c | 1141 +-
>  drivers/mmc/host/sdhci-xenon-phy.h |  157 -
>  drivers/mmc/host/sdhci-xenon.c |4 +-
>  drivers/mmc/host/sdhci-xenon.h |   17 +-
>  6 files changed, 1321 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/mmc/host/sdhci-xenon-phy.c
>  create mode 100644 drivers/mmc/host/sdhci-xenon-phy.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 859420e5dfd3..b5673c2ee5f2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7583,6 +7583,7 @@ M:  Ziji Hu 
>  L:   linux-...@vger.kernel.org
>  S:   Supported
>  F:   drivers/mmc/host/sdhci-xenon.*
> +F:   drivers/mmc/host/sdhci-xenon-phy.*
>  F:   Documentation/devicetree/bindings/mmc/marvell,sdhci-xenon.txt
>  
>  MATROX FRAMEBUFFER DRIVER
> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> index 75eaf743486c..4f2854556ff7 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -82,4 +82,4 @@ ifeq ($(CONFIG_CB710_DEBUG),y)
>  endif
>  
>  obj-$(CONFIG_MMC_SDHCI_XENON)+= sdhci-xenon-driver.o
> -sdhci-xenon-driver-y += sdhci-xenon.o
> +sdhci-xenon-driver-y += sdhci-xenon.o sdhci-xenon-phy.o
> diff --git a/drivers/mmc/host/sdhci-xenon-phy.c 
> b/drivers/mmc/host/sdhci-xenon-phy.c
> new file mode 100644
> index ..4eb8fea1bec9
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-xenon-phy.c



> +static int __xenon_emmc_delay_adj_test(struct mmc_card *card)
> +{
> + int err;
> + u8 *ext_csd = NULL;
> +
> + err = mmc_get_ext_csd(card, &ext_csd);
> + kfree(ext_csd);
> +
> + return err;
> +}
> +
> +static int __xenon_sdio_delay_adj_test(struct mmc_card *card)
> +{
> + struct mmc_command cmd = {0};
> + int err;
> +
> + cmd.opcode = SD_IO_RW_DIRECT;
> + cmd.flags = MMC_RSP_R5 | MMC_CMD_AC;
> +
> + err = mmc_wait_for_cmd(card->host, &cmd, 0);
> + if (err)
> + return err;
> +
> + if (cmd.resp[0] & R5_ERROR)
> + return -EIO;
> + if (cmd.resp[0] & R5_FUNCTION_NUMBER)
> + return -EINVAL;
> + if (cmd.resp[0] & R5_OUT_OF_RANGE)
> + return -ERANGE;
> + return 0;
> +}
> +
> +static int __xenon_sd_delay_adj_test(struct mmc_card *card)
> +{
> + struct mmc_command cmd = {0};
> + int err;
> +
> + cmd.opcode = MMC_SEND_STATUS;
> + cmd.arg = card->rca << 16;
> + cmd.flags = MMC_RSP_R1 | MMC_CMD_AC;
> +
> + err = mmc_wait_for_cmd(card->host, &cmd, 0);
> + return err;
> +}
> +
> +static int xenon_delay_adj_test(struct mmc_card *card)
> +{
> + WARN_ON(!card);
> + WARN_ON(!card->host);
> +
> + if (mmc_card_mmc(card))
> + return __xenon_emmc_delay_adj_test(card);
> + else if (mmc_card_sd(card))
> + return __xenon_sd_delay_adj_test(card);
> + else if (mmc_card_sdio(card))
> + return __xenon_sdio_delay_adj_test(card);
> + else
> + return -EINVAL;
> +}

So you are issuing commands from the ->set_ios() callback.  I would want to
get Ulf's OK for that before going further.

One thing: you will need to ensure you don't trigger get HS400 re-tuning
because it will call back into ->set_ios().

And you have the problem that you need to get a reference to the card before
the card device has been added.  As I wrote in response to the previous
patch, you should get Ulf's help with that too.

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Shawn Lin]

在 2016/10/8 17:28, Ziji Hu 写道:

Hi Shawn,

On 2016/10/8 10:44, Shawn Lin wrote:

在 2016/10/7 23:22, Gregory CLEMENT 写道:

From: Ziji Hu 

Marvell Xenon eMMC/SD/SDIO Host Controller contains PHY.
Three types of PHYs are supported.

Add support to multiple types of PHYs init and configuration.
Add register definitions of PHYs.

Signed-off-by: Hu Ziji 
Reviewed-by: Gregory CLEMENT 
Signed-off-by: Gregory CLEMENT 
---
 MAINTAINERS|1 +-
 drivers/mmc/host/Makefile  |2 +-
 drivers/mmc/host/sdhci-xenon-phy.c | 1141 +-
 drivers/mmc/host/sdhci-xenon-phy.h |  157 -
 drivers/mmc/host/sdhci-xenon.c |4 +-
 drivers/mmc/host/sdhci-xenon.h |   17 +-
 6 files changed, 1321 insertions(+), 1 deletion(-)
 create mode 100644 drivers/mmc/host/sdhci-xenon-phy.c
 create mode 100644 drivers/mmc/host/sdhci-xenon-phy.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 859420e5dfd3..b5673c2ee5f2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7583,6 +7583,7 @@ M:Ziji Hu 
 L:linux-...@vger.kernel.org
 S:Supported
 F:drivers/mmc/host/sdhci-xenon.*
+F:drivers/mmc/host/sdhci-xenon-phy.*


drivers/mmc/host/sdhci-xenon* shoube enough


 F:Documentation/devicetree/bindings/mmc/marvell,sdhci-xenon.txt

 MATROX FRAMEBUFFER DRIVER
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 75eaf743486c..4f2854556ff7 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -82,4 +82,4 @@ ifeq ($(CONFIG_CB710_DEBUG),y)
 endif

 obj-$(CONFIG_MMC_SDHCI_XENON)+= sdhci-xenon-driver.o
-sdhci-xenon-driver-y+= sdhci-xenon.o
+sdhci-xenon-driver-y+= sdhci-xenon.o sdhci-xenon-phy.o
diff --git a/drivers/mmc/host/sdhci-xenon-phy.c 
b/drivers/mmc/host/sdhci-xenon-phy.c
new file mode 100644
index ..4eb8fea1bec9
--- /dev/null
+++ b/drivers/mmc/host/sdhci-xenon-phy.c


Well, it's legit to use phy API and move your phy
operations to PHY subsystem. :)



Actually we tried to put the PHY code into Linux PHY framework.
But it cannot fit in Linux common PHY framework.



Indeed, it seems you need much intercation between the phy and host,
but the phy APIs are not so rich. :)


Our Xenon SDHC PHY register is a part of Xenon SDHC register set.
Besides, during MMC initialization, MMC sequence has to call several PHY 
functions to complete timing setting.
In those PHY setting functions, they have to access SDHC register and know 
current MMC setting, such as bus width, clock frequency and speed mode.
As a result, we have to implement PHY under MMC directory.

Thank you.

Best regards,
Hu Ziji


@@ -0,0 +1,1141 @@
+/*
+ * PHY support for Xenon SDHC
+ *
+ * Copyright (C) 2016 Marvell, All Rights Reserved.
+ *
+ * Author:Hu Ziji 
+ * Date:2016-8-24
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "sdhci.h"
+#include "sdhci-pltfm.h"
+#include "sdhci-xenon.h"
+
+static const char * const phy_types[] = {
+"sdh phy",
+"emmc 5.0 phy",
+"emmc 5.1 phy"
+};
+
+enum phy_type_enum {
+SDH_PHY,
+EMMC_5_0_PHY,
+EMMC_5_1_PHY,
+NR_PHY_TYPES
+};
+
+struct soc_pad_ctrl_table {
+const char *soc;
+void (*set_soc_pad)(struct sdhci_host *host,
+unsigned char signal_voltage);
+};
+
+struct soc_pad_ctrl {
+/* Register address of SOC PHY PAD ctrl */
+void __iomem*reg;
+/* SOC PHY PAD ctrl type */
+enum soc_pad_ctrl_type pad_type;
+/* SOC specific operation to set SOC PHY PAD */
+void (*set_soc_pad)(struct sdhci_host *host,
+unsigned char signal_voltage);
+};
+
+static struct xenon_emmc_phy_regs  xenon_emmc_5_0_phy_regs = {
+.timing_adj= EMMC_5_0_PHY_TIMING_ADJUST,
+.func_ctrl= EMMC_5_0_PHY_FUNC_CONTROL,
+.pad_ctrl= EMMC_5_0_PHY_PAD_CONTROL,
+.pad_ctrl2= EMMC_5_0_PHY_PAD_CONTROL2,
+.dll_ctrl= EMMC_5_0_PHY_DLL_CONTROL,
+.logic_timing_adj = EMMC_5_0_PHY_LOGIC_TIMING_ADJUST,
+.delay_mask= EMMC_5_0_PHY_FIXED_DELAY_MASK,
+.dll_update= DLL_UPDATE_STROBE_5_0,
+};
+
+static struct xenon_emmc_phy_regs  xenon_emmc_5_1_phy_regs = {
+.timing_adj= EMMC_PHY_TIMING_ADJUST,
+.func_ctrl= EMMC_PHY_FUNC_CONTROL,
+.pad_ctrl= EMMC_PHY_PAD_CONTROL,
+.pad_ctrl2= EMMC_PHY_PAD_CONTROL2,
+.dll_ctrl= EMMC_PHY_DLL_CONTROL,
+.logic_timing_adj = EMMC_PHY_LOGIC_TIMING_ADJUST,
+.delay_mask= EMMC_PHY_FIXED_DELAY_MASK,
+.dll_update= DLL_UPDATE,
+};
+
+static int xenon_delay_adj_test(struct mmc_card *card);
+
+/*
+ * eMMC PHY configuration and operations
+ */
+struct emmc_phy_params {
+boolslow_mode;
+
+u8znr;
+u8zpr;
+
+/* Nr of consecutive Sampling Points of

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Ziji Hu]

Hi Shawn,

On 2016/10/8 10:44, Shawn Lin wrote:
> 在 2016/10/7 23:22, Gregory CLEMENT 写道:
>> From: Ziji Hu 
>>
>> Marvell Xenon eMMC/SD/SDIO Host Controller contains PHY.
>> Three types of PHYs are supported.
>>
>> Add support to multiple types of PHYs init and configuration.
>> Add register definitions of PHYs.
>>
>> Signed-off-by: Hu Ziji 
>> Reviewed-by: Gregory CLEMENT 
>> Signed-off-by: Gregory CLEMENT 
>> ---
>>  MAINTAINERS|1 +-
>>  drivers/mmc/host/Makefile  |2 +-
>>  drivers/mmc/host/sdhci-xenon-phy.c | 1141 +-
>>  drivers/mmc/host/sdhci-xenon-phy.h |  157 -
>>  drivers/mmc/host/sdhci-xenon.c |4 +-
>>  drivers/mmc/host/sdhci-xenon.h |   17 +-
>>  6 files changed, 1321 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/mmc/host/sdhci-xenon-phy.c
>>  create mode 100644 drivers/mmc/host/sdhci-xenon-phy.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 859420e5dfd3..b5673c2ee5f2 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -7583,6 +7583,7 @@ M:Ziji Hu 
>>  L:linux-...@vger.kernel.org
>>  S:Supported
>>  F:drivers/mmc/host/sdhci-xenon.*
>> +F:drivers/mmc/host/sdhci-xenon-phy.*
> 
> drivers/mmc/host/sdhci-xenon* shoube enough
> 
>>  F:Documentation/devicetree/bindings/mmc/marvell,sdhci-xenon.txt
>>
>>  MATROX FRAMEBUFFER DRIVER
>> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
>> index 75eaf743486c..4f2854556ff7 100644
>> --- a/drivers/mmc/host/Makefile
>> +++ b/drivers/mmc/host/Makefile
>> @@ -82,4 +82,4 @@ ifeq ($(CONFIG_CB710_DEBUG),y)
>>  endif
>>
>>  obj-$(CONFIG_MMC_SDHCI_XENON)+= sdhci-xenon-driver.o
>> -sdhci-xenon-driver-y+= sdhci-xenon.o
>> +sdhci-xenon-driver-y+= sdhci-xenon.o sdhci-xenon-phy.o
>> diff --git a/drivers/mmc/host/sdhci-xenon-phy.c 
>> b/drivers/mmc/host/sdhci-xenon-phy.c
>> new file mode 100644
>> index ..4eb8fea1bec9
>> --- /dev/null
>> +++ b/drivers/mmc/host/sdhci-xenon-phy.c
> 
> Well, it's legit to use phy API and move your phy
> operations to PHY subsystem. :)
> 

Actually we tried to put the PHY code into Linux PHY framework.
But it cannot fit in Linux common PHY framework.

Our Xenon SDHC PHY register is a part of Xenon SDHC register set.
Besides, during MMC initialization, MMC sequence has to call several PHY 
functions to complete timing setting.
In those PHY setting functions, they have to access SDHC register and know 
current MMC setting, such as bus width, clock frequency and speed mode.
As a result, we have to implement PHY under MMC directory.

Thank you.

Best regards,
Hu Ziji

>> @@ -0,0 +1,1141 @@
>> +/*
>> + * PHY support for Xenon SDHC
>> + *
>> + * Copyright (C) 2016 Marvell, All Rights Reserved.
>> + *
>> + * Author:Hu Ziji 
>> + * Date:2016-8-24
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation version 2.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#include "sdhci.h"
>> +#include "sdhci-pltfm.h"
>> +#include "sdhci-xenon.h"
>> +
>> +static const char * const phy_types[] = {
>> +"sdh phy",
>> +"emmc 5.0 phy",
>> +"emmc 5.1 phy"
>> +};
>> +
>> +enum phy_type_enum {
>> +SDH_PHY,
>> +EMMC_5_0_PHY,
>> +EMMC_5_1_PHY,
>> +NR_PHY_TYPES
>> +};
>> +
>> +struct soc_pad_ctrl_table {
>> +const char *soc;
>> +void (*set_soc_pad)(struct sdhci_host *host,
>> +unsigned char signal_voltage);
>> +};
>> +
>> +struct soc_pad_ctrl {
>> +/* Register address of SOC PHY PAD ctrl */
>> +void __iomem*reg;
>> +/* SOC PHY PAD ctrl type */
>> +enum soc_pad_ctrl_type pad_type;
>> +/* SOC specific operation to set SOC PHY PAD */
>> +void (*set_soc_pad)(struct sdhci_host *host,
>> +unsigned char signal_voltage);
>> +};
>> +
>> +static struct xenon_emmc_phy_regs  xenon_emmc_5_0_phy_regs = {
>> +.timing_adj= EMMC_5_0_PHY_TIMING_ADJUST,
>> +.func_ctrl= EMMC_5_0_PHY_FUNC_CONTROL,
>> +.pad_ctrl= EMMC_5_0_PHY_PAD_CONTROL,
>> +.pad_ctrl2= EMMC_5_0_PHY_PAD_CONTROL2,
>> +.dll_ctrl= EMMC_5_0_PHY_DLL_CONTROL,
>> +.logic_timing_adj = EMMC_5_0_PHY_LOGIC_TIMING_ADJUST,
>> +.delay_mask= EMMC_5_0_PHY_FIXED_DELAY_MASK,
>> +.dll_update= DLL_UPDATE_STROBE_5_0,
>> +};
>> +
>> +static struct xenon_emmc_phy_regs  xenon_emmc_5_1_phy_regs = {
>> +.timing_adj= EMMC_PHY_TIMING_ADJUST,
>> +.func_ctrl= EMMC_PHY_FUNC_CONTROL,
>> +.pad_ctrl= EMMC_PHY_PAD_CONTROL,
>> +.pad_ctrl2= EMMC_PHY_PAD_CONTROL2,
>> +.dll_ctrl= EMMC_PHY_DLL_CONTROL,
>> +.logic_timing_adj = EMMC_PHY_LOGIC_TIMING_ADJUST,
>> +.delay_mask= EMMC_PHY_FIXED_DELAY_MASK,
>> +.dll_update= DLL_UPDATE,
>> +};

Re: [PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Shawn Lin]

在 2016/10/7 23:22, Gregory CLEMENT 写道:

From: Ziji Hu 

Marvell Xenon eMMC/SD/SDIO Host Controller contains PHY.
Three types of PHYs are supported.

Add support to multiple types of PHYs init and configuration.
Add register definitions of PHYs.

Signed-off-by: Hu Ziji 
Reviewed-by: Gregory CLEMENT 
Signed-off-by: Gregory CLEMENT 
---
 MAINTAINERS|1 +-
 drivers/mmc/host/Makefile  |2 +-
 drivers/mmc/host/sdhci-xenon-phy.c | 1141 +-
 drivers/mmc/host/sdhci-xenon-phy.h |  157 -
 drivers/mmc/host/sdhci-xenon.c |4 +-
 drivers/mmc/host/sdhci-xenon.h |   17 +-
 6 files changed, 1321 insertions(+), 1 deletion(-)
 create mode 100644 drivers/mmc/host/sdhci-xenon-phy.c
 create mode 100644 drivers/mmc/host/sdhci-xenon-phy.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 859420e5dfd3..b5673c2ee5f2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7583,6 +7583,7 @@ M:Ziji Hu 
 L: linux-...@vger.kernel.org
 S: Supported
 F: drivers/mmc/host/sdhci-xenon.*
+F: drivers/mmc/host/sdhci-xenon-phy.*


drivers/mmc/host/sdhci-xenon* shoube enough


 F: Documentation/devicetree/bindings/mmc/marvell,sdhci-xenon.txt

 MATROX FRAMEBUFFER DRIVER
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 75eaf743486c..4f2854556ff7 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -82,4 +82,4 @@ ifeq ($(CONFIG_CB710_DEBUG),y)
 endif

 obj-$(CONFIG_MMC_SDHCI_XENON)  += sdhci-xenon-driver.o
-sdhci-xenon-driver-y   += sdhci-xenon.o
+sdhci-xenon-driver-y   += sdhci-xenon.o sdhci-xenon-phy.o
diff --git a/drivers/mmc/host/sdhci-xenon-phy.c 
b/drivers/mmc/host/sdhci-xenon-phy.c
new file mode 100644
index ..4eb8fea1bec9
--- /dev/null
+++ b/drivers/mmc/host/sdhci-xenon-phy.c


Well, it's legit to use phy API and move your phy
operations to PHY subsystem. :)


@@ -0,0 +1,1141 @@
+/*
+ * PHY support for Xenon SDHC
+ *
+ * Copyright (C) 2016 Marvell, All Rights Reserved.
+ *
+ * Author: Hu Ziji 
+ * Date:   2016-8-24
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "sdhci.h"
+#include "sdhci-pltfm.h"
+#include "sdhci-xenon.h"
+
+static const char * const phy_types[] = {
+   "sdh phy",
+   "emmc 5.0 phy",
+   "emmc 5.1 phy"
+};
+
+enum phy_type_enum {
+   SDH_PHY,
+   EMMC_5_0_PHY,
+   EMMC_5_1_PHY,
+   NR_PHY_TYPES
+};
+
+struct soc_pad_ctrl_table {
+   const char *soc;
+   void (*set_soc_pad)(struct sdhci_host *host,
+   unsigned char signal_voltage);
+};
+
+struct soc_pad_ctrl {
+   /* Register address of SOC PHY PAD ctrl */
+   void __iomem*reg;
+   /* SOC PHY PAD ctrl type */
+   enum soc_pad_ctrl_type pad_type;
+   /* SOC specific operation to set SOC PHY PAD */
+   void (*set_soc_pad)(struct sdhci_host *host,
+   unsigned char signal_voltage);
+};
+
+static struct xenon_emmc_phy_regs  xenon_emmc_5_0_phy_regs = {
+   .timing_adj = EMMC_5_0_PHY_TIMING_ADJUST,
+   .func_ctrl  = EMMC_5_0_PHY_FUNC_CONTROL,
+   .pad_ctrl   = EMMC_5_0_PHY_PAD_CONTROL,
+   .pad_ctrl2  = EMMC_5_0_PHY_PAD_CONTROL2,
+   .dll_ctrl   = EMMC_5_0_PHY_DLL_CONTROL,
+   .logic_timing_adj = EMMC_5_0_PHY_LOGIC_TIMING_ADJUST,
+   .delay_mask = EMMC_5_0_PHY_FIXED_DELAY_MASK,
+   .dll_update = DLL_UPDATE_STROBE_5_0,
+};
+
+static struct xenon_emmc_phy_regs  xenon_emmc_5_1_phy_regs = {
+   .timing_adj = EMMC_PHY_TIMING_ADJUST,
+   .func_ctrl  = EMMC_PHY_FUNC_CONTROL,
+   .pad_ctrl   = EMMC_PHY_PAD_CONTROL,
+   .pad_ctrl2  = EMMC_PHY_PAD_CONTROL2,
+   .dll_ctrl   = EMMC_PHY_DLL_CONTROL,
+   .logic_timing_adj = EMMC_PHY_LOGIC_TIMING_ADJUST,
+   .delay_mask = EMMC_PHY_FIXED_DELAY_MASK,
+   .dll_update = DLL_UPDATE,
+};
+
+static int xenon_delay_adj_test(struct mmc_card *card);
+
+/*
+ * eMMC PHY configuration and operations
+ */
+struct emmc_phy_params {
+   boolslow_mode;
+
+   u8  znr;
+   u8  zpr;
+
+   /* Nr of consecutive Sampling Points of a Valid Sampling Window */
+   u8  nr_tun_times;
+   /* Divider for calculating Tuning Step */
+   u8  tun_step_divider;
+
+   struct soc_pad_ctrl pad_ctrl;
+};
+
+static void xenon_emmc_phy_strobe_delay_adj(struct sdhci_host *host,
+   struct mmc_card *card);
+static int xenon_emmc_phy_fix_sampl_delay_adj(struct sdhci_host *host,
+ struct mmc_card *card);
+static void xenon_emmc_phy_set(struct sdhci_host *host,
+  unsigned char timing);
+st

[PATCH 7/10] mmc: sdhci-xenon: Add support to PHYs of Marvell Xenon SDHC

[Gregory CLEMENT]

From: Ziji Hu 

Marvell Xenon eMMC/SD/SDIO Host Controller contains PHY.
Three types of PHYs are supported.

Add support to multiple types of PHYs init and configuration.
Add register definitions of PHYs.

Signed-off-by: Hu Ziji 
Reviewed-by: Gregory CLEMENT 
Signed-off-by: Gregory CLEMENT 
---
 MAINTAINERS|1 +-
 drivers/mmc/host/Makefile  |2 +-
 drivers/mmc/host/sdhci-xenon-phy.c | 1141 +-
 drivers/mmc/host/sdhci-xenon-phy.h |  157 -
 drivers/mmc/host/sdhci-xenon.c |4 +-
 drivers/mmc/host/sdhci-xenon.h |   17 +-
 6 files changed, 1321 insertions(+), 1 deletion(-)
 create mode 100644 drivers/mmc/host/sdhci-xenon-phy.c
 create mode 100644 drivers/mmc/host/sdhci-xenon-phy.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 859420e5dfd3..b5673c2ee5f2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7583,6 +7583,7 @@ M:Ziji Hu 
 L: linux-...@vger.kernel.org
 S: Supported
 F: drivers/mmc/host/sdhci-xenon.*
+F: drivers/mmc/host/sdhci-xenon-phy.*
 F: Documentation/devicetree/bindings/mmc/marvell,sdhci-xenon.txt
 
 MATROX FRAMEBUFFER DRIVER
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 75eaf743486c..4f2854556ff7 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -82,4 +82,4 @@ ifeq ($(CONFIG_CB710_DEBUG),y)
 endif
 
 obj-$(CONFIG_MMC_SDHCI_XENON)  += sdhci-xenon-driver.o
-sdhci-xenon-driver-y   += sdhci-xenon.o
+sdhci-xenon-driver-y   += sdhci-xenon.o sdhci-xenon-phy.o
diff --git a/drivers/mmc/host/sdhci-xenon-phy.c 
b/drivers/mmc/host/sdhci-xenon-phy.c
new file mode 100644
index ..4eb8fea1bec9
--- /dev/null
+++ b/drivers/mmc/host/sdhci-xenon-phy.c
@@ -0,0 +1,1141 @@
+/*
+ * PHY support for Xenon SDHC
+ *
+ * Copyright (C) 2016 Marvell, All Rights Reserved.
+ *
+ * Author: Hu Ziji 
+ * Date:   2016-8-24
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "sdhci.h"
+#include "sdhci-pltfm.h"
+#include "sdhci-xenon.h"
+
+static const char * const phy_types[] = {
+   "sdh phy",
+   "emmc 5.0 phy",
+   "emmc 5.1 phy"
+};
+
+enum phy_type_enum {
+   SDH_PHY,
+   EMMC_5_0_PHY,
+   EMMC_5_1_PHY,
+   NR_PHY_TYPES
+};
+
+struct soc_pad_ctrl_table {
+   const char *soc;
+   void (*set_soc_pad)(struct sdhci_host *host,
+   unsigned char signal_voltage);
+};
+
+struct soc_pad_ctrl {
+   /* Register address of SOC PHY PAD ctrl */
+   void __iomem*reg;
+   /* SOC PHY PAD ctrl type */
+   enum soc_pad_ctrl_type pad_type;
+   /* SOC specific operation to set SOC PHY PAD */
+   void (*set_soc_pad)(struct sdhci_host *host,
+   unsigned char signal_voltage);
+};
+
+static struct xenon_emmc_phy_regs  xenon_emmc_5_0_phy_regs = {
+   .timing_adj = EMMC_5_0_PHY_TIMING_ADJUST,
+   .func_ctrl  = EMMC_5_0_PHY_FUNC_CONTROL,
+   .pad_ctrl   = EMMC_5_0_PHY_PAD_CONTROL,
+   .pad_ctrl2  = EMMC_5_0_PHY_PAD_CONTROL2,
+   .dll_ctrl   = EMMC_5_0_PHY_DLL_CONTROL,
+   .logic_timing_adj = EMMC_5_0_PHY_LOGIC_TIMING_ADJUST,
+   .delay_mask = EMMC_5_0_PHY_FIXED_DELAY_MASK,
+   .dll_update = DLL_UPDATE_STROBE_5_0,
+};
+
+static struct xenon_emmc_phy_regs  xenon_emmc_5_1_phy_regs = {
+   .timing_adj = EMMC_PHY_TIMING_ADJUST,
+   .func_ctrl  = EMMC_PHY_FUNC_CONTROL,
+   .pad_ctrl   = EMMC_PHY_PAD_CONTROL,
+   .pad_ctrl2  = EMMC_PHY_PAD_CONTROL2,
+   .dll_ctrl   = EMMC_PHY_DLL_CONTROL,
+   .logic_timing_adj = EMMC_PHY_LOGIC_TIMING_ADJUST,
+   .delay_mask = EMMC_PHY_FIXED_DELAY_MASK,
+   .dll_update = DLL_UPDATE,
+};
+
+static int xenon_delay_adj_test(struct mmc_card *card);
+
+/*
+ * eMMC PHY configuration and operations
+ */
+struct emmc_phy_params {
+   boolslow_mode;
+
+   u8  znr;
+   u8  zpr;
+
+   /* Nr of consecutive Sampling Points of a Valid Sampling Window */
+   u8  nr_tun_times;
+   /* Divider for calculating Tuning Step */
+   u8  tun_step_divider;
+
+   struct soc_pad_ctrl pad_ctrl;
+};
+
+static void xenon_emmc_phy_strobe_delay_adj(struct sdhci_host *host,
+   struct mmc_card *card);
+static int xenon_emmc_phy_fix_sampl_delay_adj(struct sdhci_host *host,
+ struct mmc_card *card);
+static void xenon_emmc_phy_set(struct sdhci_host *host,
+  unsigned char timing);
+static void xenon_emmc_set_soc_pad(struct sdhci_host *host,
+  unsigned char signal_voltage);
+
+static const struct xenon_phy_ops emmc_phy_ops