| Message ID | 20200305191925.959494-3-seanga2@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | riscv: Add SPI support for Kendryte K210 | expand |
On 3/5/20 8:19 PM, Sean Anderson wrote: > Some devices have different layouts for the fields in CTRL0 (e.g. the > Kendryte K210). Allow this layout to be configurable from the device tree. > The documentation has been taken from Linux. > > Signed-off-by: Sean Anderson <seanga2 at gmail.com> > Reviewed-by: Simon Glass <sjg at chromium.org> Can't you just have different compatible string for each SoC and derive the various fields based on that compatible string, instead of describing all the register bitfields in DT ? What does Linux do ?
On 3/21/20 9:51 PM, Marek Vasut wrote: > On 3/5/20 8:19 PM, Sean Anderson wrote: >> Some devices have different layouts for the fields in CTRL0 (e.g. the >> Kendryte K210). Allow this layout to be configurable from the device tree. >> The documentation has been taken from Linux. >> >> Signed-off-by: Sean Anderson <seanga2 at gmail.com> >> Reviewed-by: Simon Glass <sjg at chromium.org> > > Can't you just have different compatible string for each SoC and derive > the various fields based on that compatible string, instead of > describing all the register bitfields in DT ? > > What does Linux do ? > Linux only supports socfpga boards. I don't know if there is any rhyme/reason to the shifting around of these fields. It is possible to add several compatible strings like "kendryte,k210-spi3". I chose this method because the bitfields are different for spi0 and spi1, spi2, and spi3. If there are other incompatibilities discovered, then it may make more sense to use different strings. Another option could have been to use the DW_SPI_VERSION field to detect different controllers, but it is the same among all the controllers on the K210. --Sean
On 3/22/20 3:36 AM, Sean Anderson wrote: > On 3/21/20 9:51 PM, Marek Vasut wrote: >> On 3/5/20 8:19 PM, Sean Anderson wrote: >>> Some devices have different layouts for the fields in CTRL0 (e.g. the >>> Kendryte K210). Allow this layout to be configurable from the device tree. >>> The documentation has been taken from Linux. >>> >>> Signed-off-by: Sean Anderson <seanga2 at gmail.com> >>> Reviewed-by: Simon Glass <sjg at chromium.org> >> >> Can't you just have different compatible string for each SoC and derive >> the various fields based on that compatible string, instead of >> describing all the register bitfields in DT ? >> >> What does Linux do ? >> > > Linux only supports socfpga boards. I don't know if there is any > rhyme/reason to the shifting around of these fields. Could be a different revision of the IP. This is usually handled by using SoC-specific compatible strings, see e.g. Linux Documentation/devicetree/bindings/usb/renesas,usb3-peri.yaml You don't want to encode register layout in the DT. > It is possible to > add several compatible strings like "kendryte,k210-spi3". Why ? > I chose this > method because the bitfields are different for spi0 and spi1, spi2, and > spi3. If there are other incompatibilities discovered, then it may make > more sense to use different strings. Another option could have been to > use the DW_SPI_VERSION field to detect different controllers, but it is > the same among all the controllers on the K210. The controllers on the same SoC have different register layout ?
On 3/21/20 11:04 PM, Marek Vasut wrote: > On 3/22/20 3:36 AM, Sean Anderson wrote: >> On 3/21/20 9:51 PM, Marek Vasut wrote: >>> On 3/5/20 8:19 PM, Sean Anderson wrote: >>>> Some devices have different layouts for the fields in CTRL0 (e.g. the >>>> Kendryte K210). Allow this layout to be configurable from the device tree. >>>> The documentation has been taken from Linux. >>>> >>>> Signed-off-by: Sean Anderson <seanga2 at gmail.com> >>>> Reviewed-by: Simon Glass <sjg at chromium.org> >>> >>> Can't you just have different compatible string for each SoC and derive >>> the various fields based on that compatible string, instead of >>> describing all the register bitfields in DT ? >>> >>> What does Linux do ? >>> >> >> Linux only supports socfpga boards. I don't know if there is any >> rhyme/reason to the shifting around of these fields. > > Could be a different revision of the IP. This is usually handled by > using SoC-specific compatible strings, see e.g. Linux > Documentation/devicetree/bindings/usb/renesas,usb3-peri.yaml > > You don't want to encode register layout in the DT. Ok, then I think adding compatible strings would be the cleanest. > >> It is possible to >> add several compatible strings like "kendryte,k210-spi3". > > Why ? See below. > >> I chose this >> method because the bitfields are different for spi0 and spi1, spi2, and >> spi3. If there are other incompatibilities discovered, then it may make >> more sense to use different strings. Another option could have been to >> use the DW_SPI_VERSION field to detect different controllers, but it is >> the same among all the controllers on the K210. > > The controllers on the same SoC have different register layout ? > Yup! Don't ask me why. --Sean
On 3/22/20 4:08 AM, Sean Anderson wrote: > On 3/21/20 11:04 PM, Marek Vasut wrote: >> On 3/22/20 3:36 AM, Sean Anderson wrote: >>> On 3/21/20 9:51 PM, Marek Vasut wrote: >>>> On 3/5/20 8:19 PM, Sean Anderson wrote: >>>>> Some devices have different layouts for the fields in CTRL0 (e.g. the >>>>> Kendryte K210). Allow this layout to be configurable from the device tree. >>>>> The documentation has been taken from Linux. >>>>> >>>>> Signed-off-by: Sean Anderson <seanga2 at gmail.com> >>>>> Reviewed-by: Simon Glass <sjg at chromium.org> >>>> >>>> Can't you just have different compatible string for each SoC and derive >>>> the various fields based on that compatible string, instead of >>>> describing all the register bitfields in DT ? >>>> >>>> What does Linux do ? >>>> >>> >>> Linux only supports socfpga boards. I don't know if there is any >>> rhyme/reason to the shifting around of these fields. >> >> Could be a different revision of the IP. This is usually handled by >> using SoC-specific compatible strings, see e.g. Linux >> Documentation/devicetree/bindings/usb/renesas,usb3-peri.yaml >> >> You don't want to encode register layout in the DT. > > Ok, then I think adding compatible strings would be the cleanest. > >> >>> It is possible to >>> add several compatible strings like "kendryte,k210-spi3". >> >> Why ? > > See below. > >> >>> I chose this >>> method because the bitfields are different for spi0 and spi1, spi2, and >>> spi3. If there are other incompatibilities discovered, then it may make >>> more sense to use different strings. Another option could have been to >>> use the DW_SPI_VERSION field to detect different controllers, but it is >>> the same among all the controllers on the K210. >> >> The controllers on the same SoC have different register layout ? >> > > Yup! > > Don't ask me why. Now that is truly an odd design. Is there a datasheet for this SoC ? You might be able to somehow enumerate those controllers in DT and derive their layout from that enumeration or somesuch I think.
On 3/21/20 11:13 PM, Marek Vasut wrote: > On 3/22/20 4:08 AM, Sean Anderson wrote: >> On 3/21/20 11:04 PM, Marek Vasut wrote: >>> Could be a different revision of the IP. This is usually handled by >>> using SoC-specific compatible strings, see e.g. Linux >>> Documentation/devicetree/bindings/usb/renesas,usb3-peri.yaml >>> >>> You don't want to encode register layout in the DT. >> >> Ok, then I think adding compatible strings would be the cleanest. >> >>> >>>> It is possible to >>>> add several compatible strings like "kendryte,k210-spi3". >>> >>> Why ? >> >> See below. >> >>> >>>> I chose this >>>> method because the bitfields are different for spi0 and spi1, spi2, and >>>> spi3. If there are other incompatibilities discovered, then it may make >>>> more sense to use different strings. Another option could have been to >>>> use the DW_SPI_VERSION field to detect different controllers, but it is >>>> the same among all the controllers on the K210. >>> >>> The controllers on the same SoC have different register layout ? >>> >> >> Yup! >> >> Don't ask me why. > > Now that is truly an odd design. Is there a datasheet for this SoC ? Nope. Everything has been done referencing their sdk. In hindsight, porting this board was a poor decision because of all the hoops I've had to jump through due to the absence of documentation. Unfortunately, it's probably the only budget risc-v board with S-mode until the end of the year or so. > You might be able to somehow enumerate those controllers in DT and > derive their layout from that enumeration or somesuch I think. So like derive it from the sequence number? I'd rather do this in a more standard way, if possible. --Sean
On 3/22/20 4:33 AM, Sean Anderson wrote: > On 3/21/20 11:13 PM, Marek Vasut wrote: >> On 3/22/20 4:08 AM, Sean Anderson wrote: >>> On 3/21/20 11:04 PM, Marek Vasut wrote: >>>> Could be a different revision of the IP. This is usually handled by >>>> using SoC-specific compatible strings, see e.g. Linux >>>> Documentation/devicetree/bindings/usb/renesas,usb3-peri.yaml >>>> >>>> You don't want to encode register layout in the DT. >>> >>> Ok, then I think adding compatible strings would be the cleanest. >>> >>>> >>>>> It is possible to >>>>> add several compatible strings like "kendryte,k210-spi3". >>>> >>>> Why ? >>> >>> See below. >>> >>>> >>>>> I chose this >>>>> method because the bitfields are different for spi0 and spi1, spi2, and >>>>> spi3. If there are other incompatibilities discovered, then it may make >>>>> more sense to use different strings. Another option could have been to >>>>> use the DW_SPI_VERSION field to detect different controllers, but it is >>>>> the same among all the controllers on the K210. >>>> >>>> The controllers on the same SoC have different register layout ? >>>> >>> >>> Yup! >>> >>> Don't ask me why. >> >> Now that is truly an odd design. Is there a datasheet for this SoC ? > > Nope. Everything has been done referencing their sdk. > > In hindsight, porting this board was a poor decision because of all the > hoops I've had to jump through due to the absence of documentation. > Unfortunately, it's probably the only budget risc-v board with S-mode > until the end of the year or so. > >> You might be able to somehow enumerate those controllers in DT and >> derive their layout from that enumeration or somesuch I think. > > So like derive it from the sequence number? I'd rather do this in a more > standard way, if possible. I would rather like to know why do the controllers have different register layout.
diff --git a/doc/device-tree-bindings/spi/snps,dw-apb-ssi.txt b/doc/device-tree-bindings/spi/snps,dw-apb-ssi.txt new file mode 100644 index 0000000000..4b6152f6b7 --- /dev/null +++ b/doc/device-tree-bindings/spi/snps,dw-apb-ssi.txt @@ -0,0 +1,43 @@ +Synopsys DesignWare AMBA 2.0 Synchronous Serial Interface. + +Required properties: +- compatible : "snps,dw-apb-ssi" +- reg : The register base for the controller. For "mscc,<soc>-spi", a second + register set is required (named ICPU_CFG:SPI_MST) +- #address-cells : <1>, as required by generic SPI binding. +- #size-cells : <0>, also as required by generic SPI binding. +- clocks : phandles for the clocks, see the description of clock-names below. + The phandle for the "ssi_clk" is required. The phandle for the "pclk" clock + is optional. If a single clock is specified but no clock-name, it is the + "ssi_clk" clock. If both clocks are listed, the "ssi_clk" must be first. + +Optional properties: +- clock-names : Contains the names of the clocks: + "ssi_clk", for the core clock used to generate the external SPI clock. + "pclk", the interface clock, required for register access. +- cs-gpios : Specifies the gpio pins to be used for chipselects. +- num-cs : The number of chipselects. If omitted, this will default to 4. +- reg-io-width : The I/O register width (in bytes) implemented by this + device. Supported values are 2 or 4 (the default). +- snps,dfs-offset The offset in bits of the DFS field in CTRL0, defaulting to 0 +- snps,frf-offset The offset in bits of the FRF field in CTRL0, defaulting to 4 +- snps,tmod-offset The offset in bits of the tmode field in CTRL0, defaulting + to 6 +- snps,mode-offset The offset in bits of the work mode field in CTRL0, + defaulting to 8 + +Child nodes as per the generic SPI binding. + +Example: + + spi at fff00000 { + compatible = "snps,dw-apb-ssi"; + reg = <0xfff00000 0x1000>; + interrupts = <0 154 4>; + #address-cells = <1>; + #size-cells = <0>; + clocks = <&spi_m_clk>; + num-cs = <2>; + cs-gpios = <&gpio0 13 0>, + <&gpio0 14 0>; + }; diff --git a/drivers/spi/designware_spi.c b/drivers/spi/designware_spi.c index 2dc16736a3..6e1c289297 100644 --- a/drivers/spi/designware_spi.c +++ b/drivers/spi/designware_spi.c @@ -3,6 +3,7 @@ * Designware master SPI core controller driver * * Copyright (C) 2014 Stefan Roese <sr at denx.de> + * Copyright (C) 2020 Sean Anderson <seanga2 at gmail.com> * * Very loosely based on the Linux driver: * drivers/spi/spi-dw.c, which is: @@ -51,20 +52,14 @@ #define DW_SPI_DR 0x60 /* Bit fields in CTRLR0 */ -#define SPI_DFS_OFFSET 0 - -#define SPI_FRF_OFFSET 4 #define SPI_FRF_SPI 0x0 #define SPI_FRF_SSP 0x1 #define SPI_FRF_MICROWIRE 0x2 #define SPI_FRF_RESV 0x3 -#define SPI_MODE_OFFSET 6 -#define SPI_SCPH_OFFSET 6 -#define SPI_SCOL_OFFSET 7 +#define SPI_MODE_SCPH 0x1 +#define SPI_MODE_SCOL 0x2 -#define SPI_TMOD_OFFSET 8 -#define SPI_TMOD_MASK (0x3 << SPI_TMOD_OFFSET) #define SPI_TMOD_TR 0x0 /* xmit & recv */ #define SPI_TMOD_TO 0x1 /* xmit only */ #define SPI_TMOD_RO 0x2 /* recv only */ @@ -89,6 +84,12 @@ struct dw_spi_platdata { s32 frequency; /* Default clock frequency, -1 for none */ void __iomem *regs; + + /* Offsets in CTRL0 */ + u8 dfs_off; + u8 frf_off; + u8 tmod_off; + u8 mode_off; }; struct dw_spi_priv { @@ -115,6 +116,15 @@ struct dw_spi_priv { struct reset_ctl_bulk resets; }; +static inline u32 GEN_CTRL0(struct dw_spi_priv *priv, + struct dw_spi_platdata *plat) +{ + return ((priv->bits_per_word - 1) << plat->dfs_off | + (priv->type << plat->frf_off) | + (priv->mode << plat->mode_off) | + (priv->tmode << plat->tmod_off)); +} + static inline u32 dw_read(struct dw_spi_priv *priv, u32 offset) { return __raw_readl(priv->regs + offset); @@ -160,6 +170,10 @@ static int dw_spi_ofdata_to_platdata(struct udevice *bus) /* Use 500KHz as a suitable default */ plat->frequency = dev_read_u32_default(bus, "spi-max-frequency", 500000); + plat->dfs_off = dev_read_u32_default(bus, "snps,dfs-offset", 0); + plat->frf_off = dev_read_u32_default(bus, "snps,frf-offset", 4); + plat->mode_off = dev_read_u32_default(bus, "snps,mode-offset", 6); + plat->tmod_off = dev_read_u32_default(bus, "snps,tmod-offset", 8); debug("%s: regs=%p max-frequency=%d\n", __func__, plat->regs, plat->frequency); @@ -388,6 +402,7 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen, const void *dout, void *din, unsigned long flags) { struct udevice *bus = dev->parent; + struct dw_spi_platdata *plat = dev_get_platdata(bus); struct dw_spi_priv *priv = dev_get_priv(bus); const u8 *tx = dout; u8 *rx = din; @@ -406,10 +421,6 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen, if (flags & SPI_XFER_BEGIN) external_cs_manage(dev, false); - cr0 = (priv->bits_per_word - 1) | (priv->type << SPI_FRF_OFFSET) | - (priv->mode << SPI_MODE_OFFSET) | - (priv->tmode << SPI_TMOD_OFFSET); - if (rx && tx) priv->tmode = SPI_TMOD_TR; else if (rx) @@ -421,8 +432,7 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen, */ priv->tmode = SPI_TMOD_TR; - cr0 &= ~SPI_TMOD_MASK; - cr0 |= (priv->tmode << SPI_TMOD_OFFSET); + cr0 = GEN_CTRL0(priv, plat); priv->len = bitlen >> 3; debug("%s: rx=%p tx=%p len=%d [bytes]\n", __func__, rx, tx, priv->len); @@ -476,7 +486,7 @@ static int dw_spi_xfer(struct udevice *dev, unsigned int bitlen, static int dw_spi_set_speed(struct udevice *bus, uint speed) { - struct dw_spi_platdata *plat = bus->platdata; + struct dw_spi_platdata *plat = dev_get_platdata(bus); struct dw_spi_priv *priv = dev_get_priv(bus); u16 clk_div;