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.
Best Regards,
Ben Chuang
