Re: [PATCHv3 2/3] spi: Add spi driver for Socionext Synquacer platform

[Jassi Brar <jassisinghbrar@xxxxxxxxx>]

On Fri, Feb 9, 2018 at 2:12 AM, Ard Biesheuvel
<ard.biesheuvel@xxxxxxxxxx> wrote:
> On 19 January 2018 at 10:37,  <jassisinghbrar@xxxxxxxxx> wrote:
> > From: Jassi Brar <jaswinder.singh@xxxxxxxxxx>
> >
> > This patch adds support for controller found on synquacer platforms.
> >
> > Signed-off-by: Jassi Brar <jaswinder.singh@xxxxxxxxxx>
>
> I added the following nodes to my SynQuacer boards:
>
>     clk_fip006_spi: spi_ihclk {
>         compatible = "fixed-clock";
>         #clock-cells = <0>;
>         clock-frequency = <125000000>;
>     };
>
>     spi@54810000 {
>         compatible = "socionext,synquacer-spi";
>         reg = <0x0 0x54810000 0x0 0x1000>;
>         clocks = <&clk_fip006_spi>;
>         clock-names = "iHCLK";
>         socionext,use-rtm;
>         socionext,set-aces;
>         #address-cells = <1>;
>         #size-cells = <0>;
>
>         tpm@0 {
>             reg = <0x0>;
>             compatible = "infineon,slb9670";
>             spi-max-frequency = <22500000>;
>         };
>     };
>
> and I end up with the following splat:
>
> [    5.741886] Unable to handle kernel paging request at virtual
> address fffffffffffffffe
> [    5.741889] Mem abort info:
> [    5.741891]   ESR = 0x96000004
> [    5.741895]   Exception class = DABT (current EL), IL = 32 bits
> [    5.741898]   SET = 0, FnV = 0
> [    5.741899]   EA = 0, S1PTW = 0
> [    5.741901] Data abort info:
> [    5.741903]   ISV = 0, ISS = 0x00000004
> [    5.741905]   CM = 0, WnR = 0
> [    5.741910] swapper pgtable: 4k pages, 48-bit VAs, pgd = 0000000042134b9d
> [    5.741913] [fffffffffffffffe] *pgd=0000000000000000
> [    5.741920] Internal error: Oops: 96000004 [#1] SMP
> [    5.741924] Modules linked in: efivars(+) gpio_keys(+)
> spi_synquacer(+) efivarfs ip_tables x_tables autofs4 ext4 crc16
> mbcache jbd2 fscrypto sd_mod ahci xhci_pci libahci xhci_hcd libata
> usbcore realtek scsi_mod netsec of_mdio fixed_phy libphy gpio_mb86s7x
> [    5.741974] CPU: 18 PID: 389 Comm: systemd-udevd Not tainted 4.15.0+ #1
> [    5.741976] Hardware name: Socionext Developer Box (DT)
> [    5.741981] pstate: a0000005 (NzCv daif -PAN -UAO)
> [    5.741995] pc : clk_prepare+0x1c/0x60
> [    5.742007] lr : synquacer_spi_probe+0xe8/0x290 [spi_synquacer]
> [    5.742008] sp : ffff00000d5739e0
> [    5.742011] x29: ffff00000d5739e0 x28: ffff01911d855000
> [    5.742016] x27: ffff3dda0e962000 x26: 0000000000000000
> [    5.742021] x25: ffff01911d8542d0 x24: 0000000000000010
> [    5.742026] x23: ffffbed27ffed5c8 x22: ffffbed25b7cb400
> [    5.742031] x21: fffffffffffffffe x20: ffffbed25658b000
> [    5.742036] x19: fffffffffffffffe x18: ffffffffffffffff
> [    5.742041] x17: 0000ffff90f5ce58 x16: ffff3dda0e3cb818
> [    5.742046] x15: ffff3dda0ee59c08 x14: ffffffffffffffff
> [    5.742051] x13: 0000000000000028 x12: 0101010101010101
> [    5.742055] x11: 0000000000000038 x10: 0101010101010101
> [    5.742060] x9 : 0000000000000002 x8 : 7f7f7f7f7f7f7f7f
> [    5.742065] x7 : ff6c73712c647274 x6 : 0000000000000080
> [    5.742070] x5 : 0000000000000000 x4 : 8000000000000000
> [    5.742075] x3 : 0000000000000000 x2 : 0000000000000000
> [    5.742079] x1 : 0000000000000001 x0 : ffff01911d852778
> [    5.742086] Process systemd-udevd (pid: 389, stack limit =
> 0x00000000cdd89d3b)
> [    5.742088] Call trace:
> [    5.742093]  clk_prepare+0x1c/0x60
> [    5.742101]  synquacer_spi_probe+0xe8/0x290 [spi_synquacer]
> [    5.742109]  platform_drv_probe+0x60/0xc8
> [    5.742114]  driver_probe_device+0x2dc/0x480
> [    5.742119]  __driver_attach+0x124/0x128
> [    5.742124]  bus_for_each_dev+0x78/0xe0
> [    5.742128]  driver_attach+0x30/0x40
> [    5.742132]  bus_add_driver+0x1f8/0x2b0
> [    5.742136]  driver_register+0x68/0x100
> [    5.742141]  __platform_driver_register+0x54/0x60
> [    5.742148]  synquacer_spi_driver_init+0x1c/0x1000 [spi_synquacer]
> [    5.742154]  do_one_initcall+0x5c/0x168
> [    5.742161]  do_init_module+0x64/0x1e0
> [    5.742167]  load_module+0x1ed0/0x2198
> [    5.742172]  SyS_finit_module+0x128/0x140
> [    5.742176]  __sys_trace_return+0x0/0x4
> [    5.742183] Code: aa0003f3 aa1e03e0 d503201f b4000193 (f9400273)
> [    5.742188] ---[ end trace 831278301b1eda70 ]---
>
>
> It looks like the call
>
> clk_prepare_enable(sspi->clk[IPCLK]);
>
> is passing the ERR() value of devm_clk_get() rather than NULL. Adding
> 'if (!IS_ERR())' fixes it for me.
>
I had thought the clk_xyz() calls would immediately return if invalid
clock was provided, but I see they return only if the passed clock is
a NULL pointer, and not when its ERR_PTR. I think a better option
would be to make it NULL if devm_clk_get returns ERR_PTR.

Stephen, does it make sense to also catch the ERR_PTR in
clk_enable/prepare?   Like clk_disable, clk_unprepare already does.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html