Hi Mark,
Thank you for your review.
> On Wed, Sep 16, 2020 at 12:17:55PM +0300, Viorel Suman (OSS) wrote:
> > +static int fsl_xcvr_load_firmware(struct fsl_xcvr *xcvr) {
> > + struct device *dev = &xcvr->pdev->dev;
> > + const struct firmware *fw;
> > + int ret = 0, rem, off, out, page = 0, size = FSL_XCVR_REG_OFFSET;
> > + u32 mask, val;
> > +
> > + ret = request_firmware(&fw, xcvr->fw_name, dev);
> > + if (ret) {
> > + dev_err(dev, "failed to request firmware.\n");
> > + return ret;
> > + }
> > +
> > + rem = fw->size;
>
> It would be good to see some explicit validation of the image size, at least
> printing an error message if the image is bigger than can be loaded. The code
> should be safe in that it won't overflow the device region it's writing to but it
> feels like it'd be better to tell people if we spot a problem rather than just silently
> truncating the file.
Make sense, will improve this part in the next version.
> > +static irqreturn_t irq0_isr(int irq, void *devid) {
> > + struct fsl_xcvr *xcvr = (struct fsl_xcvr *)devid;
> > + struct device *dev = &xcvr->pdev->dev;
> > + struct regmap *regmap = xcvr->regmap;
> > + void __iomem *reg_ctrl, *reg_buff;
> > + u32 isr, val, i;
> > +
> > + regmap_read(regmap, FSL_XCVR_EXT_ISR, &isr);
> > + regmap_write(regmap, FSL_XCVR_EXT_ISR_CLR, isr);
>
> This will unconditionally clear any interrupts, even those we don't understand - it
> might be better to only clear bits that are supported so the IRQ core can
> complain if there's something unexpected showing up.
The ARM core registers itself in "fsl_xcvr_prepare" (the code below) just for a subset of all supported interrupts:
=====
ret = regmap_update_bits(xcvr->regmap, FSL_XCVR_EXT_IER0,
FSL_XCVR_IRQ_EARC_ALL, FSL_XCVR_IRQ_EARC_ALL);
=====
FSL_XCVR_IRQ_EARC_ALL - this mask represents all the interrupts we are interested in and we handle in interrupt handler,
But this is just a subset of all interrupts the M0+ core is able to assert. Not very intuitive, I think I need to reword it somehow.
> > + if (isr & FSL_XCVR_IRQ_FIFO_UOFL_ERR)
> > + dev_dbg(dev, "RX/TX FIFO full/empty\n");
>
> Should this be dev_err()?
The interrupt may be asserted right before DMA starts to fill the TX FIFO if I recall correctly.
I've added it just to debug the IP behavior, will check and change it to err it in next version if it is the case.
> > +static irqreturn_t irq1_isr(int irq, void *devid) {
> > + struct fsl_xcvr *xcvr = (struct fsl_xcvr *)devid;
> > + struct device *dev = &xcvr->pdev->dev;
> > +
> > + dev_dbg(dev, "irq[1]: %d\n", irq);
> > +
> > + return IRQ_HANDLED;
> > +}
>
> Is there any value in even requesting this and irq2 given the lack of meaningful
> handling?
No, will remove it in v2.
Thank you,
Viorel
