RE: [PATCH 4/6] spi: Add SPIBSC driver

From: Chris Brandt <Chris.Brandt@renesas.com>
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Mark Brown <broonie@kernel.org>, Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	linux-spi <linux-spi@vger.kernel.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>,
	Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
	linux-clk <linux-clk@vger.kernel.org>,
	Mason Yang <masonccyang@mxic.com.tw>,
	Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Subject: RE: [PATCH 4/6] spi: Add SPIBSC driver
Date: Wed, 4 Dec 2019 22:12:57 +0000	[thread overview]
Message-ID: <TY1PR01MB1562B1CBD17676A559EBA5718A5D0@TY1PR01MB1562.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CAMuHMdVAZOa7OmT0s=RsVJsny9NujDzpdg4T+QoUXGe0kJjOTw@mail.gmail.com>

Hi Geert,

Thank you for your review and suggestions!

On Tue, Dec 3, 2019, Geert Uytterhoeven wrote:
> > +static int spibsc_wait_trans_completion(struct spibsc_priv *sbsc) {
> > +       int t = 256 * 100000;
> > +
> > +       while (t--) {
> > +               if (spibsc_read(sbsc, CMNSR) & CMNSR_TEND)
> > +                       return 0;
> > +               ndelay(1);
> > +       }
> 
> So this may busy loop for up to 25.6 ms? Oops...
> 
> Can you use the interrupt (SPIHF) instead, signalling a completion?

RZ/A1 doesn't have any interrupts. And I think the interrupts in RZ/A2 
only work for HyperFlash (not QSPI).

But, 25.6ms is way too long!
I'll assume the slowest clock to be 1MHz and the max FIFO transfer of 4 bytes.
That's 32us, so that's what I'll use.

> > +               if (t->cs_change) {
> > +                       dev_err(sbsc->dev, "cs_change not supported");
> > +                       return -EIO;
> > +               }
> > +       }
> 
> If you would do the checks above in .prepare_message() (like e.g. rspi
> does)...

OK, Thanks. :)

> > +       case 6:
> > +               dropr |= DROPR_OPD0(tx_data[5]);
> > +               opde |= (1 << 0);
> 
> Both checkpatch and gcc tell you to add fallthrough coments.

OK, fixed.

> ... can't you just use .transfer_one(), instead of duplicating the logic from
> spi_transfer_one_message()?
> Or is there some special detail that's not obvious to the casual reviewer?

I guess .transfer_one() could be used if I kept shoving more stuff into 
.prepare_message().

But, the screwy thing about this controller is that messages that are 
'Tx only' are processed differently then 'Tx then Rx' messages and not 
like a traditional SPI controller.
By implementing both conditions in .spi_transfer_one_message(), it makes
that more clear for the next person in my opinion because you can see 
that sometimes we have to work with the entire message as a whole, not 
just individual pieces.

> > +static const struct of_device_id of_spibsc_match[] = {
> > +       { .compatible = "renesas,spibsc"},
> > +       { .compatible = "renesas,r7s72100-spibsc", .data = (void
> *)HAS_SPBCR},
> > +       { .compatible = "renesas,r7s9210-spibsc"},
> 
> Do you need to match against all 3 in the driver?
> Does SPIBSC work on RZ/A1 when not setting HAS_SPBCR?
> If not, the fallback to "renesas,spibsc" is not valid for RZ/A1.

OK, I dropped "renesas,spibsc".
I like having individual SoC names anyway.

Chris