On 16.06.2022 13:21:52, Rasmus Villemoes wrote:
> > The ecspi or the qspi?
>
> ecspi
OK.
> >> A logic analyzer shows that the spi communication works (mostly) fine
> >> [1], up until mcp251xfd_chip_ecc_init(). In there, it seems that the
> >> regmap_raw_write() ends up trying to do byte-by-byte transfers - i.e.,
> >> the chip select is only asserted for 8 clock cycles, then deasserted,
> >> and asserted again for the next byte.
> >
> > What about the other transfers done before the
> > mcp251xfd_chip_ecc_init()? Do the look correct?
>
> Yes. For example the very first write done from mcp251xfd_chip_start()
> -> mcp251xfd_chip_softreset{,_do}() -> mcp251xfd_chip_clock_enable()
> looks like https://ibb.co/xMbdsSq , and the MOSI signal looks right
> (0xa = crc write to 0xe00 aka OSC, 4 value bytes, the 0x00000060 value
> in little endian, and the crc partly off-screen).
That looks perfect.
[...]
> >> Obviously, that means that the RAM doesn't get initialized, because
> >> all the device sees is a bunch of aborted spi transactions. I can't
> >> really figure out why this happens, maybe I'm missing some setting in
> >> DT or elsewhere; currently I have
> >
> > Can you show me your SPI host controller node, too?
>
> Yes, it's essentially the one from imx8mp.dtsi, i.e.
>
> ecspi1: spi@30820000 {
> #address-cells = <1>;
> #size-cells = <0>;
> compatible = "fsl,imx8mp-ecspi", "fsl,imx6ul-ecspi";
> reg = <0x30820000 0x10000>;
> interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
> clocks = <&clk IMX8MP_CLK_ECSPI1_ROOT>,
> <&clk IMX8MP_CLK_ECSPI1_ROOT>;
> clock-names = "ipg", "per";
> assigned-clock-rates = <80000000>;
> assigned-clocks = <&clk IMX8MP_CLK_ECSPI1>;
> assigned-clock-parents = <&clk IMX8MP_SYS_PLL1_800M>;
> dmas = <&sdma1 0 7 1>, <&sdma1 1 7 2>;
> dma-names = "rx", "tx";
> status = "disabled";
> };
>
> with this in the board .dts:
>
> &ecspi1 {
> status = "okay";
> pinctrl-names = "default";
> pinctrl-0 = <&pinctrl_ecspi1>;
> cs-gpios = <&gpio5 9 GPIO_ACTIVE_LOW>;
>
> CAN_C: spi@0 {
> reg = <0>;
> compatible = "microchip,mcp2518fd";
> /* clocks = <&cdce937 6>; */
> clocks = <&clk_40MHz>;
> interrupts-extended = <&gpio1 6 IRQ_TYPE_LEVEL_LOW>;
> spi-max-frequency = <10000000>;
> pinctrl-names = "default";
> pinctrl-0 = <&pinctrl_CAN_C>;
> };
> };
>
> It's currently (for bad reasons) the nxp kernel, hence the
> fsl,imx6ul-ecspi compatible, but changing that to the one in mainline,
> fsl,imx51-ecspi, doesn't change the symptoms.
The imx8mp.dtsi enables DMA for the SPI host controller.
> >> CAN_C: spi@0 {
> >> reg = <0>;
> >> compatible = "microchip,mcp2518fd";
> >> clocks = <&clk_40MHz>;
> >> interrupts-extended = <&gpio1 6 IRQ_TYPE_LEVEL_LOW>;
> >> spi-max-frequency = <17000000>;
> >> pinctrl-names = "default";
> >> pinctrl-0 = <&pinctrl_CAN_C>;
> >> };
> >>
> >> with the pinctrl being for the irq gpio. I have also tried lowering
> >> spi-max-frequency to 10MHz (currently it ends up effectively using
> >> 16MHz),
> >
> > The driver uses 85% of clock/2 or spi-max-frequency, whatever is lower.
>
> Yes; using 10MHz was just an attempt to see if using an even lower
> frequency than the maximum implied by the errata (and implemented in the
> driver) would make a difference.
>
> >> [1] One thing I have noticed is that we probably want to do
> >>
> >> - xfer[1].len = sizeof(dev_id);
> >> + xfer[1].len = sizeof(*dev_id);
> >
> > Fixed. Do you want to appear as Reported-by in the patch?
>
> Yes please.
Updated.
> >> But: the reading doesn't seem to work; it's as if the device doesn't
> >> drive MISO at all at this point, because when I configure the pin with
> >> a weak internal pull-up, I get 0xffffffff, while if I use pinmux
> >> settings with a weak pull-down I get 0 - which is the "correct"
> >> answer, but probably not for the right reason.
> >
> > The device id is read after detecting the chip and the
> > mcp251xfd_chip_ecc_init(). Does the chip detection work properly?
>
> It appears so, adding a print of the osc variable after the
> regmap_read() in mcp251xfd_register_chip_detect() gives
>
> mcp251xfd_register_chip_detect: osc = 0x00000468
>
> so the LPMEN bit seems to have been set (and the other bits show
> expected values). And it's true that the dev_id is read after this,
> through mcp251xfd_register_done(), which is why I'm puzzled that it
> doesn't seem to work.
>
> Are you sure about the ecc_init() part? AFAICT that's only called from
> chip_start, which in turn is only called when the device is brought
> up.
Yes, you're right!
> >> I'm on a 5.10.y kernel, but I don't see any commits related to this
> >> since then (other than the error handling in
> >> mcp251xfd_register_get_dev_id()).
> >
> > Which kernel are you using exactly? Mainline or freescale/nxp
> > downstream?
>
> Currently NXP, lf-5.10.72-2.2.0 aka a68e31b63f86. I'll see if I can
> manage to make a mainline kernel boot.
v5.10 is old, you will not get the best mcp251xfd performance out of it.
There are several performance improvements on the SPI framework, the
spi-imx driver and the mcp251xfd driver with a recent kernel.
> > Have you enabled DMA on SPI?
>
> Not explicitly, but nor have I done anything to disable/not enable it,
> so I'm not really sure what the right answer is. Is that a CONFIG_ knob
> or module/kernel parameter?
You can use "/delete-property/ dmas;" and "/delete-property/ dma-names;"
in your board dts on the &ecspi1 node, the module parameter "use_dma=0",
or the kernel command line "spi-imx.use_dma=0" to disable DMA.
The SPI host driver uses PIO for smaller transfers, but switches to DMA
mode for bigger ones (IIRC > 64 bytes). The clearing of the memory
definitely falls into the big transfer category. The SPI DMA mode is
broken on various kernel variants (downstream, mainline), kernel
versions, SoC variants and used SDMA firmware versions. For the
mcp251xfd driver DMA is also slower. So switch of DMA and try again.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Attachment:
signature.asc
Description: PGP signature
