On 4/09/19 3:58 AM, Ben Chuang wrote:
> On Tue, Sep 3, 2019 at 6:05 AM Andy Shevchenko
> <andy.shevchenko@xxxxxxxxx> wrote:
>>
>> On Fri, Aug 30, 2019 at 5:28 AM Ben Chuang <benchuanggli@xxxxxxxxx> wrote:
>>>
>>> From: Ben Chuang <ben.chuang@xxxxxxxxxxxxxxxxxxx>
>>>
>>> Add support for the GL9750 and GL9755 chipsets.
>>>
>>> Enable v4 mode and wait 5ms after set 1.8V signal enable for GL9750/
>>> GL9755. Fix the value of SDHCI_MAX_CURRENT register and use the vendor
>>> tuning flow for GL9750.
>>>
>>
>>> Signed-off-by: Ben Chuang <ben.chuang@xxxxxxxxxxxxxxxxxxx>
>>
>> Usually last one for latest developer / submitter goes on.
>>
>>> Co-developed-by: Michael K Johnson <johnsonm@xxxxxxxxx>
>>> Signed-off-by: Michael K Johnson <johnsonm@xxxxxxxxx>
>>
>>
>>> +#define GLI_MAX_TUNING_LOOP 40
>>
>>
>>> +static void gli_set_9750(struct sdhci_host *host)
>>> +{
>>> + u32 driving_value = 0;
>>> + u32 pll_value = 0;
>>> + u32 sw_ctrl_value = 0;
>>> + u32 misc_value = 0;
>>> + u32 parameter_value = 0;
>>> + u32 control_value = 0;
>>
>>> +
>>
>> Redundant blank line.
>>
>>> + u16 ctrl2 = 0;
>>
>> Do you need these all assignments above?
>>
>>> + driving_value = sdhci_readl(host, SDHCI_GLI_9750_DRIVING);
>>> + pll_value = sdhci_readl(host, SDHCI_GLI_9750_PLL);
>>> + sw_ctrl_value = sdhci_readl(host, SDHCI_GLI_9750_SW_CTRL);
>>> + misc_value = sdhci_readl(host, SDHCI_GLI_9750_MISC);
>>> + parameter_value = sdhci_readl(host, SDHCI_GLI_9750_TUNING_PARAMETERS);
>>> + control_value = sdhci_readl(host, SDHCI_GLI_9750_TUNING_CONTROL);
>>
>>
>>
>>> +
>>> + udelay(1);
>>
>> This misses the answer to question why. Why this is needed and why
>> timeout is this long?
>>
>>> +
>>> + gl9750_wt_off(host);
>>> +}
>>
>>> +static int __sdhci_execute_tuning_9750(struct sdhci_host *host, u32 opcode)
>>> +{
>>> + int i;
>>
>>> + int rx_inv = 0;
>>
>> Duplicate assignment.
>>
>>> +
>>> + for (rx_inv = 0; rx_inv < 2; rx_inv++) {
>>
>>> + if (rx_inv & 0x1)
>>> + gli_set_9750_rx_inv(host, true);
>>> + else
>>> + gli_set_9750_rx_inv(host, false);
>>
>> gli_set_...(host, !!rx_inv);
>>
>>> +
>>> + sdhci_start_tuning(host);
>>> +
>>> + for (i = 0; i < GLI_MAX_TUNING_LOOP; i++) {
>>> + u16 ctrl;
>>> +
>>> + sdhci_send_tuning(host, opcode);
>>> +
>>> + if (!host->tuning_done) {
>>
>>> + if (rx_inv == 1) {
>>
>> It's an invariant to the loop. So, you may do this check after outter loop.
>>
>>> + pr_info("%s: Tuning timeout, falling back to fixed sampling clock\n",
>>> + mmc_hostname(host->mmc));
>>
>>> + sdhci_abort_tuning(host, opcode);
>>
>> It will also de-duplicates this call.
>>
>>> + return -ETIMEDOUT;
>>> + }
>>> + sdhci_abort_tuning(host, opcode);
>>> + break;
>>> + }
>>
>>> + }
>>> + }
>>> +
>>> + pr_info("%s: Tuning failed, falling back to fixed sampling clock\n",
>>> + mmc_hostname(host->mmc));
>>> + sdhci_reset_tuning(host);
>>> + return -EAGAIN;
>>> +}
>>
>>> +static void sdhci_gli_voltage_switch(struct sdhci_host *host)
>>> +{
>>
>> Any comment why?
>>
>>> + usleep_range(5000, 5500);
>>> +}
>>
>>> +static u32 sdhci_gl9750_readl(struct sdhci_host *host, int reg)
>>> +{
>>> + u32 value;
>>> +
>>> + value = readl(host->ioaddr + reg);
>>
>>> + if (unlikely(reg == SDHCI_MAX_CURRENT)) {
>>> + if (!(value & 0xff))
>>> + value |= 0xc8;
>>> + }
>>
>> if (a) {
>> if (b) {
>> ...
>> }
>> }
>>
>> is the same as
>>
>> if (a && b) {
>> ...
>> }
>>
>>> + return value;
>>> +}
>>
>>> +#define PCI_DEVICE_ID_GLI_9755 0x9755
>>> +#define PCI_DEVICE_ID_GLI_9750 0x9750
>>
>> --
>> With Best Regards,
>> Andy Shevchenko
>
> Hi, Andy,
>
> Thank you for your comments to make the code better.
> Waiting to see if Adrian has any other comments.
Nope! :-)
Please go ahead and address Andy's comments.
