RE: [patch v21 2/4] drivers: jtag: Add Aspeed SoC 24xx and 25xx families JTAG master driver

[Oleksandr Shamray]

Hi Andy. 
Thanks for review.

Please read my answers inline.

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@xxxxxxxxx]
> Sent: 16 ÐÐÑ 2018 Ð. 0:00
> To: Oleksandr Shamray <oleksandrs@xxxxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Arnd Bergmann 
> <arnd@xxxxxxxx>; Linux Kernel Mailing List 
> <linux-kernel@xxxxxxxxxxxxxxx>; linux-arm Mailing List 
> <linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>; devicetree 
> <devicetree@xxxxxxxxxxxxxxx>; openbmc@xxxxxxxxxxxxxxxx; Joel Stanley 
> <joel@xxxxxxxxx>; JiÅÃ PÃrko <jiri@xxxxxxxxxxx>; Tobias Klauser 
> <tklauser@xxxxxxxxxx>; open list:SERIAL DRIVERS <linux- 
> serial@xxxxxxxxxxxxxxx>; Vadim Pasternak <vadimp@xxxxxxxxxxxx>; 
> system-sw-low-level <system-sw-low-level@xxxxxxxxxxxx>; Rob Herring 
> <robh+dt@xxxxxxxxxx>; openocd-devel-owner@xxxxxxxxxxxxxxxxxxxxx; 
> linux- api@xxxxxxxxxxxxxxx; David S. Miller <davem@xxxxxxxxxxxxx>; 
> Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>; Jiri Pirko 
> <jiri@xxxxxxxxxxxx>
> Subject: Re: [patch v21 2/4] drivers: jtag: Add Aspeed SoC 24xx and 
> 25xx families JTAG master driver
> 
> On Tue, May 15, 2018 at 5:21 PM, Oleksandr Shamray 
> <oleksandrs@xxxxxxxxxxxx> wrote:
> > Driver adds support of Aspeed 2500/2400 series SOC JTAG master
> controller.
> >
> > Driver implements the following jtag ops:
> > - freq_get;
> > - freq_set;
> > - status_get;
> > - idle;
> > - xfer;
> >
> > It has been tested on Mellanox system with BMC equipped with Aspeed
> > 2520 SoC for programming CPLD devices.
> 
> > +#define ASPEED_JTAG_DATA               0x00
> > +#define ASPEED_JTAG_INST               0x04
> > +#define ASPEED_JTAG_CTRL               0x08
> > +#define ASPEED_JTAG_ISR                        0x0C
> > +#define ASPEED_JTAG_SW                 0x10
> > +#define ASPEED_JTAG_TCK                        0x14
> > +#define ASPEED_JTAG_EC                 0x18
> > +
> > +#define ASPEED_JTAG_DATA_MSB           0x01
> > +#define ASPEED_JTAG_DATA_CHUNK_SIZE    0x20
> 
> 
> > +#define ASPEED_JTAG_IOUT_LEN(len)      (ASPEED_JTAG_CTL_ENG_EN |\
> > +                                        ASPEED_JTAG_CTL_ENG_OUT_EN 
> > +|\
> > +
> > +ASPEED_JTAG_CTL_INST_LEN(len))
> 
> Better to read
> 
> #define MY_COOL_CONST_OR_MACRO(xxx) \
>  ...
> 
> > +#define ASPEED_JTAG_DOUT_LEN(len)      (ASPEED_JTAG_CTL_ENG_EN
> |\
> > +                                        ASPEED_JTAG_CTL_ENG_OUT_EN 
> > + |\
> > +
> > +ASPEED_JTAG_CTL_DATA_LEN(len))
> 
> Ditto.

Ok. Changed to:
#define ASPEED_JTAG_IOUT_LEN(len) \
                               (ASPEED_JTAG_CTL_ENG_EN | \
                               ASPEED_JTAG_CTL_ENG_OUT_EN | \
                               ASPEED_JTAG_CTL_INST_LEN(len))

#define ASPEED_JTAG_DOUT_LEN(len) \
                               (ASPEED_JTAG_CTL_ENG_EN | \
                               ASPEED_JTAG_CTL_ENG_OUT_EN | \
                               ASPEED_JTAG_CTL_DATA_LEN(len))


> 
> > +static char *end_status_str[] = {"idle", "ir pause", "drpause"};
> 
> > +static int aspeed_jtag_freq_set(struct jtag *jtag, u32 freq) {
> > +       struct aspeed_jtag *aspeed_jtag = jtag_priv(jtag);
> > +       unsigned long apb_frq;
> > +       u32 tck_val;
> > +       u16 div;
> > +
> > +       apb_frq = clk_get_rate(aspeed_jtag->pclk);
> 
> > +       div = (apb_frq % freq == 0) ? (apb_frq / freq) - 1 : 
> > + (apb_frq / freq);
> 
> Isn't it the same as
> 
> div = (apb_frq - 1) / freq;
> 
> ?
> 

Seems it is same. Thanks.

> > +       tck_val = aspeed_jtag_read(aspeed_jtag, ASPEED_JTAG_TCK);
> > +       aspeed_jtag_write(aspeed_jtag,
> > +                         (tck_val & ASPEED_JTAG_TCK_DIVISOR_MASK) | div,
> > +                         ASPEED_JTAG_TCK);
> > +       return 0;
> > +}
> 
> > +static void aspeed_jtag_sw_delay(struct aspeed_jtag *aspeed_jtag, 
> > +int
> > +cnt) {
> > +       int i;
> > +
> > +       for (i = 0; i < cnt; i++)
> > +               aspeed_jtag_read(aspeed_jtag, ASPEED_JTAG_SW);
> 
> Isn't it readsl() (or how it's called, I don't remember).
> 

No, readsl reads data into buffer. But in this place read used for make 
software delay. 
Aspeed jtag driver supports 2 modes:
1 - hw mode with the hardware controlled JTAG states
and pins
2 - with software controlled pins. 
This part of code used in sw-mode and generates delay for JTAG bit-bang .
I will change it to ndelay().

> > +}
> 
> > +static void aspeed_jtag_wait_instruction_pause(struct aspeed_jtag
> > +*aspeed_jtag) {
> > +       wait_event_interruptible(aspeed_jtag->jtag_wq, aspeed_jtag->flag &
> > +                                ASPEED_JTAG_ISR_INST_PAUSE);
> 
> In such cases I prefer to see a new line with a parameter in full.
> Check all places.
> 
> > +       aspeed_jtag->flag &= ~ASPEED_JTAG_ISR_INST_PAUSE; }
> 
> > +static void aspeed_jtag_sm_cycle(struct aspeed_jtag *aspeed_jtag, 
> > +const
> u8 *tms,
> > +                                int len) {
> > +       int i;
> > +
> > +       for (i = 0; i < len; i++)
> > +               aspeed_jtag_tck_cycle(aspeed_jtag, tms[i], 0); }
> > +
> > +static void aspeed_jtag_run_test_idle_sw(struct aspeed_jtag
> *aspeed_jtag,
> > +                                        struct jtag_run_test_idle
> > +*runtest) {
> > +       static const u8 sm_pause_irpause[] = {1, 1, 1, 1, 0, 1, 0};
> > +       static const u8 sm_pause_drpause[] = {1, 1, 1, 0, 1, 0};
> > +       static const u8 sm_idle_irpause[] = {1, 1, 0, 1, 0};
> > +       static const u8 sm_idle_drpause[] = {1, 0, 1, 0};
> > +       static const u8 sm_pause_idle[] = {1, 1, 0};
> > +       int i;
> > +
> > +       /* SW mode from idle/pause-> to pause/idle */
> > +       if (runtest->reset) {
> > +               for (i = 0; i < ASPEED_JTAG_RESET_CNTR; i++)
> > +                       aspeed_jtag_tck_cycle(aspeed_jtag, 1, 0);
> > +       }
> 
> I would rather split this big switch to a few helper functions per 
> each status of surrounding switch.
> 

Ok.
Will do it.


> > +
> > +       /* Stay on IDLE for at least  TCK cycle */
> > +       for (i = 0; i < runtest->tck; i++)
> > +               aspeed_jtag_tck_cycle(aspeed_jtag, 0, 0); }
> 
> 
> > +/**
> > + * aspeed_jtag_run_test_idle:
> > + * JTAG reset: generates at least 9 TMS high and 1 TMS low to force
> > + * devices into Run-Test/Idle State.
> > + */
> 
> It's rather broken kernel doc.

Deleted.

> 
> > +               aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_CTL_ENG_EN |
> > +                                 ASPEED_JTAG_CTL_ENG_OUT_EN |
> > +                                 ASPEED_JTAG_CTL_FORCE_TMS, 
> > + ASPEED_JTAG_CTRL);
> 
> > +               aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_EC_GO_IDLE,
> > +                                 ASPEED_JTAG_EC);
> 
> > +       aspeed_jtag_write(aspeed_jtag, ASPEED_JTAG_SW_MODE_EN |
> > +                         ASPEED_JTAG_SW_MODE_TDIO, ASPEED_JTAG_SW);
> 
> Here you have permutations of flag some of which are repeatetive in 
> the code. Perhaps make additional definitions instead.
> Check other similar places.
> 

Ok. Will add defined for repeated flags

> 
> > +       char          tdo;
> 
> Indentation.

Ok.

> 
> > +       if (xfer->direction == JTAG_READ_XFER)
> > +               tdi = UINT_MAX;
> > +       else
> > +               tdi = data[index];
> 
> > +                       if (xfer->direction == JTAG_READ_XFER)
> > +                               tdi = UINT_MAX;
> > +                       else
> > +                               tdi = data[index];
> 
> Take your time to think how the above duplication can be avoided.
> 

In both cases data[] is different, so I should check it twice, but I will 
change it to, macro like:

#define ASPEED_JTAG_GET_TDI(direction, data) \
                              (direction == JTAG_READ_XFER) ? UNIT_MAX : data

> > +               }
> > +       }
> > +
> > +       tdo = aspeed_jtag_tck_cycle(aspeed_jtag, 1, tdi &
> ASPEED_JTAG_DATA_MSB);
> > +       data[index] |= tdo << (shift_bits % 
> > +ASPEED_JTAG_DATA_CHUNK_SIZE); }
> 
> 
> > +       if (endstate != JTAG_STATE_IDLE) {
> 
> Why not to use positive check?
> 

Will restructure to have positive check
if (endstate == JTAG_STATE_IDLE) {
...
} else {
...
} 

> > +       int i;
> > +
> > +       for (i = 0; i <= xfer->length / BITS_PER_BYTE; i++) {
> > +               pos += snprintf(&dbg_str[pos], sizeof(dbg_str) - pos,
> > +                               "0x%02x ", xfer_data[i]);
> > +       }
> 
> Oh, NO! Consider reading printk-formats (for %*ph) and other 
> documentation about available APIs.

Ok.

> 
> > +       if (!(aspeed_jtag->mode & JTAG_XFER_HW_MODE)) {
> > +               /* SW mode */
> 
> This is rather too complex to be in one function.
> 

Will split to separate functions.

> > +       } else {
> 
> > +               /* hw mode */
> > +               aspeed_jtag_write(aspeed_jtag, 0, ASPEED_JTAG_SW);
> > +               aspeed_jtag_xfer_hw(aspeed_jtag, xfer, data);
> 
> For symmetry it might be another function.
> 
> > +       }
> 
> > +       dev_dbg(aspeed_jtag->dev, "status %x\n", status);
> 
> Perhaps someone should become familiar with tracepoints?
> 
> > +               dev_err(aspeed_jtag->dev, "irq status:%x\n",
> > +                       status);
> 
> 
> Huh, really?! SPAM.


I will review and delete redundant debug messages.

> 
> (I would drop it completely, though you may use ratelimited variant)
> 
> > +               ret = IRQ_NONE;
> > +       }
> > +       return ret;
> > +}
> 
> > +       clk_prepare_enable(aspeed_jtag->pclk);
> 
> This might fail.

Will add error check

> 
> > +       dev_dbg(&pdev->dev, "IRQ %d.\n", aspeed_jtag->irq);
> 
> Noise even for debug.

Agree.

> 
> > +       err = jtag_register(jtag);
> 
> Perhaps we might have devm_ variant of this. Check how SPI framework 
> deal with a such.
> 

Jtag driver uses miscdevice and related  misc_register and misc_deregister 
calls for creation and destruction. There is no device object prior 
to call to misc_register, which could be used in devm_jtag_register.

> > +static int aspeed_jtag_remove(struct platform_device *pdev) {
> 
> > +       struct jtag *jtag;
> > +
> > +       jtag = platform_get_drvdata(pdev);
> 
> Usually we put this on one line

+

> 
> > +       aspeed_jtag_deinit(pdev, jtag_priv(jtag));
> > +       jtag_unregister(jtag);
> > +       jtag_free(jtag);
> > +       return 0;
> > +}
> 
> 
> --
> With Best Regards,
> Andy Shevchenko

Best Regards,
Oleksandr Shamray