| Message ID | 20201107081420.60325-1-damien.lemoal@wdc.com |
|---|---|
| Headers | show |
| Series | RISC-V Kendryte K210 support improvments | expand |
On Sat, Nov 07, 2020 at 05:13:48PM +0900, Damien Le Moal wrote: > The first patch of this series is a fix of the device tree parsing code. > Without this fix, a warning is generated when parsing Designware gpio > controller nodes. > The following 6 patches are fixes and improvements to the Designware SPI > driver to enable support for the MMC-spi slot found on all K210 boards. > Pathes 8 to 13 are various fixes for riscv arch code and riscv > dependent devices. Of not here is patch 11 which fix system call > execution in the no MMU case, and patch 13 which simplifies DTB builtin > handling. > The following patches 14 to 25 implement and document idevice tree > mapping of several drivers for the K210 SoC: clock driver, reset > controller and pin function management (pinctrl). > Patches 26 to 31 update the existing K210 device tree and device new > device tree files for several K210 based boards: the MAIX Bit, > MAIXSUINO, MAIX Dock and MAIX Go boards from SiPeed and the KD233 > development board from Kendryte. Please don't mix unrelated changes into a single series like this - patch serieses this big are generally something to be avoided at the best of times since they're a bit overwhelming in people's inboxes and when unrelated changes are put in a single series it makes dependencies between patches unclear which can hold things up. It is better to send the changes for each subsystem separately, it makes life easier all round.
On Sat, Nov 07, 2020 at 08:52:24AM -0500, Sean Anderson wrote: > I think if it is detectable at runtime it should be, instead of relying > on compatible strings. That way causes less future grief to anyone > porting a device possibly using DFS_32. Yes, runtime enumeration is generally preferred. Having the compatible string is nice in case some quirks are discoverd but for things that can be enumerated there's less that can go wrong if we do so.
On 11/9/20 9:33 AM, Sean Anderson wrote: > On 11/9/20 9:25 AM, Serge Semin wrote: >> Hello Damien, >> Thanks for your patches. My comments are below. >> >> On Sat, Nov 07, 2020 at 05:13:50PM +0900, Damien Le Moal wrote: >>> Synopsis DesignWare DW_apb_ssi version 4 defines a 32-bit layout of >>> the ctrlr0 register for SPI masters. The layout of ctrlr0 is: >>> >>> | 31 .. 23 | 22 .. 21 | 20 .. 16 | >>> | other stuff | spi_frf | dfs_32 | >>> >>> | 15 .. 10 | 9 .. 8 | 7 .. 6 | 5 .. 4 | 3 .. 0 | >>> | other stuff | tmod | mode | frf | dfs | >>> >> >>> Th main difference of this layout with the 16-bits version is the data >> ^ >> | >> e >> >>> frame format field which resides in bits 16..20 instead of bits 3..0. >>> >> >> Are you sure they have been moved from [0, 3] to [16, 20]? I don't have the >> manual for the 4.0x version of the core, but according to this patch: >> https://patchwork.kernel.org/project/spi-devel-general/patch/1575907443-26377-7-git-send-email-wan.ahmad.zainie.wan.mohamad@intel.com/ >> it has been ok to use the lowest four bits for DFS setting. Is the commit >> message misleading there? > > This commit message is a truncated version of [1]. Importantly, DFS is > valid when SSI_MAX_XFER_SIZE=16. When it =32, then DFS_32 must be used > (since DFS is constant 0xF). Since SSI_MAX_XFER_SIZE is a synthesis s/0xF/0x0/ > parameter, there exist devices where DFS must be used, and also where > DFS_32 must be used. > > [1] https://patchwork.ozlabs.org/project/uboot/patch/20201016225755.302659-10-seanga2@gmail.com/ > > --Sean > >> >>> Introduce the DW SPI capability flag DW_SPI_CAP_DFS_32 to let a >>> platform signal that this layout is in use. Modify >>> dw_spi_update_config() to test this capability flag to set the data >>> frame format field at the correct register location. >>> >>> Suggested-by: Sean Anderson <seanga2@gmail.com> >>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> >>> --- >>> drivers/spi/spi-dw-core.c | 8 ++++++-- >>> drivers/spi/spi-dw.h | 9 +++++++++ >>> 2 files changed, 15 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c >>> index 2e50cc0a9291..841c85247f01 100644 >>> --- a/drivers/spi/spi-dw-core.c >>> +++ b/drivers/spi/spi-dw-core.c >>> @@ -311,8 +311,12 @@ void dw_spi_update_config(struct dw_spi *dws, struct spi_device *spi, >>> u32 speed_hz; >>> u16 clk_div; >>> >> >>> - /* CTRLR0[ 4/3: 0] Data Frame Size */ >>> - cr0 |= (cfg->dfs - 1); >>> + if (!(dws->caps & DW_SPI_CAP_DFS_32)) >>> + /* CTRLR0[ 4/3: 0] Data Frame Size */ >>> + cr0 |= (cfg->dfs - 1); >>> + else >>> + /* CTRLR0[20: 16] Data Frame Size */ >>> + cr0 |= (cfg->dfs - 1) << DWC_APB_CTRLR0_32_DFS_OFFSET; >> >> If you extend the dfs field from four to five bits, then >> controller->bits_per_word_mask field should be properly updated too. >> >> Alas it hasn't been done for the DWC_ssi version of the core. So I suppose it >> should be fixed for the both of them. >> >> Just for the record. There are very handy macros for setting and getting bit fields >> to/from a variable. This is a good place to use them instead of manually >> shifting and defining the offsets. The macros are defined in linux/bitfield.h . >> Alas this driver hasn't been converted to using them. So I won't insist on using >> them here. But I hope someone will fix it sometime in future... > > I second that request. > >> -Sergey >> >>> >>> if (!(dws->caps & DW_SPI_CAP_DWC_SSI)) >>> /* CTRLR0[ 9:8] Transfer Mode */ >>> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h >>> index faf40cb66498..48a11a51a407 100644 >>> --- a/drivers/spi/spi-dw.h >>> +++ b/drivers/spi/spi-dw.h >>> @@ -9,6 +9,7 @@ >>> #include <linux/io.h> >>> #include <linux/scatterlist.h> >>> #include <linux/spi/spi-mem.h> >>> +#include <linux/bitfield.h> >>> >>> /* Register offsets */ >>> #define DW_SPI_CTRLR0 0x00 >>> @@ -72,6 +73,13 @@ >>> #define DWC_SSI_CTRLR0_FRF_OFFSET 6 >>> #define DWC_SSI_CTRLR0_DFS_OFFSET 0 >>> >>> +/* >>> + * Bit fields in CTRLR0 for DWC_apb_ssi v4 32-bits ctrlr0. >>> + * Based on DW_apb_ssi Databook v4.02a. >>> + */ >>> +#define DWC_APB_CTRLR0_32_DFS_OFFSET 16 >>> +#define DWC_APB_CTRLR0_32_DFS_MASK GENMASK(20, 16) >>> + >>> /* >>> * For Keem Bay, CTRLR0[31] is used to select controller mode. >>> * 0: SSI is slave >>> @@ -121,6 +129,7 @@ enum dw_ssi_type { >>> #define DW_SPI_CAP_CS_OVERRIDE BIT(0) >>> #define DW_SPI_CAP_KEEMBAY_MST BIT(1) >>> #define DW_SPI_CAP_DWC_SSI BIT(2) >>> +#define DW_SPI_CAP_DFS_32 BIT(3) >>> >>> /* Slave spi_transfer/spi_mem_op related */ >>> struct dw_spi_cfg { >>> -- >>> 2.28.0 >>> >
On Mon, Nov 9, 2020 at 4:40 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Nov 9, 2020 at 4:34 PM Sean Anderson <seanga2@gmail.com> wrote: > > On 11/9/20 9:25 AM, Serge Semin wrote: > > > On Sat, Nov 07, 2020 at 05:13:50PM +0900, Damien Le Moal wrote: > > ... > > > > Are you sure they have been moved from [0, 3] to [16, 20]? I don't have the > > > manual for the 4.0x version of the core, but according to this patch: > > > https://patchwork.kernel.org/project/spi-devel-general/patch/1575907443-26377-7-git-send-email-wan.ahmad.zainie.wan.mohamad@intel.com/ > > > it has been ok to use the lowest four bits for DFS setting. Is the commit > > > message misleading there? > > > > This commit message is a truncated version of [1]. > > I don't see how they are related. For DW_ssi v1.x DFS is always for transfers up to 32-bit. > > Importantly, DFS is > > valid when SSI_MAX_XFER_SIZE=16. When it =32, then DFS_32 must be used > > (since DFS is constant 0xF). Since SSI_MAX_XFER_SIZE is a synthesis > > parameter, there exist devices where DFS must be used, and also where > > DFS_32 must be used. > > > > [1] https://patchwork.ozlabs.org/project/uboot/patch/20201016225755.302659-10-seanga2@gmail.com/
On 11/9/20 9:41 AM, Andy Shevchenko wrote: > On Mon, Nov 9, 2020 at 4:40 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: >> >> On Mon, Nov 9, 2020 at 4:34 PM Sean Anderson <seanga2@gmail.com> wrote: >>> On 11/9/20 9:25 AM, Serge Semin wrote: >>>> On Sat, Nov 07, 2020 at 05:13:50PM +0900, Damien Le Moal wrote: >> >> ... >> >>>> Are you sure they have been moved from [0, 3] to [16, 20]? I don't have the >>>> manual for the 4.0x version of the core, but according to this patch: >>>> https://patchwork.kernel.org/project/spi-devel-general/patch/1575907443-26377-7-git-send-email-wan.ahmad.zainie.wan.mohamad@intel.com/ >>>> it has been ok to use the lowest four bits for DFS setting. Is the commit >>>> message misleading there? >>> >>> This commit message is a truncated version of [1]. >> >> I don't see how they are related. I think this commit message is specifically taken from v3 of that patch [2]. However, I had not gotten a chance to have a look at the datasheet at that point, so it is a bit misleading (e.g. showing dfs for devices with SSI_MAX_XFER_SIZE=32, even though it is all zeros for those devices). In any case, the diagram is taken from that patch. [2] https://patchwork.ozlabs.org/project/uboot/patch/20200914153503.91983-7-seanga2@gmail.com/ > > For DW_ssi v1.x DFS is always for transfers up to 32-bit. Do you mean DWC_ssi? > >>> Importantly, DFS is >>> valid when SSI_MAX_XFER_SIZE=16. When it =32, then DFS_32 must be used >>> (since DFS is constant 0xF). Since SSI_MAX_XFER_SIZE is a synthesis >>> parameter, there exist devices where DFS must be used, and also where >>> DFS_32 must be used. >>> >>> [1] https://patchwork.ozlabs.org/project/uboot/patch/20201016225755.302659-10-seanga2@gmail.com/ > > >
On Sat, Nov 07, 2020 at 05:13:49PM +0900, Damien Le Moal wrote: > The DesignWare GPIO driver gpio-dwapb ("snps,dw-apb-gpio" or > "apm,xgene-gpio-v2" compatible string) defines the property > "snps,nr-gpios" for the user to specify the number of GPIOs available > on a port. The "-gpios" suffix of this property name ends up being > interpreted as a cell reference when properties are parsed in > of_link_to_suppliers(), leading to error messages such as: > > OF: /soc/bus@50200000/gpio-controller@50200000/gpio-port@0: could not > find phandle > > Fix this by manually defining a parse_gpios() function which ignores > this property, skipping the search for the supplier and thus avoiding > the device tree parsing error. That's why I have introduced the "ngpios" property support and marked the "snps,nr-gpios" as deprecated here: https://lkml.org/lkml/2020/7/22/1298 to encourage the later one from being used in favor of the first one. So I suggest for you to convert your dts'es (if you have ones) to using the "ngpios" property anyway. > > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > --- > drivers/of/property.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/of/property.c b/drivers/of/property.c > index 408a7b5f06a9..d16111c0d6da 100644 > --- a/drivers/of/property.c > +++ b/drivers/of/property.c > @@ -1308,7 +1308,6 @@ DEFINE_SIMPLE_PROP(pinctrl7, "pinctrl-7", NULL) > DEFINE_SIMPLE_PROP(pinctrl8, "pinctrl-8", NULL) > DEFINE_SUFFIX_PROP(regulators, "-supply", NULL) > DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells") > -DEFINE_SUFFIX_PROP(gpios, "-gpios", "#gpio-cells") > > static struct device_node *parse_iommu_maps(struct device_node *np, > const char *prop_name, int index) > @@ -1319,6 +1318,22 @@ static struct device_node *parse_iommu_maps(struct device_node *np, > return of_parse_phandle(np, prop_name, (index * 4) + 1); > } > > +static struct device_node *parse_gpios(struct device_node *np, > + const char *prop_name, int index) > +{ > + /* > + * Quirck for the DesignWare gpio-dwapb GPIO driver which defines ^ | Quirk? > + * the "snps,nr-gpios" property to indicate the total number of GPIOs > + * available. As this conflict with "xx-gpios" reference properties, > + * ignore it. > + */ > + if (strcmp(prop_name, "snps,nr-gpios") == 0) > + return NULL; > + > + return parse_suffix_prop_cells(np, prop_name, index, > + "-gpios", "#gpio-cells"); > +} > + Personally I'd prefer to convert all the dts-es to using the "ngpios' instead of the vendor-specific property. That's why I haven't fixed the problem the way you suggest in the first place, to encourage people to send the patches with such fixes. Anyway it's up to the OF-subsystem maintainers to decide whether to accept this quirk. -Sergey > static const struct supplier_bindings of_supplier_bindings[] = { > { .parse_prop = parse_clocks, }, > { .parse_prop = parse_interconnects, }, > -- > 2.28.0 >
On Sat, Nov 7, 2020 at 10:14 AM Damien Le Moal <damien.lemoal@wdc.com> wrote: > @@ -1308,7 +1308,6 @@ DEFINE_SIMPLE_PROP(pinctrl7, "pinctrl-7", NULL) > DEFINE_SIMPLE_PROP(pinctrl8, "pinctrl-8", NULL) > DEFINE_SUFFIX_PROP(regulators, "-supply", NULL) > DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells") > -DEFINE_SUFFIX_PROP(gpios, "-gpios", "#gpio-cells") Sorry, but the above doesn't sound right to me. It's a generic code and you may imagine how many systems you broke by this change.
On Sat, 07 Nov 2020 17:14:12 +0900, Damien Le Moal wrote: > Document the device tree bindings for the Kendryte K210 SoC Fully > Programmable IO Array (FPIOA) pinctrl driver in > Documentation/devicetree/bindings/pinctrl/kendryte,k210-fpioa.yaml > > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > --- > .../bindings/pinctrl/kendryte,k210-fpioa.yaml | 106 ++++++++++++++++++ > 1 file changed, 106 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pinctrl/kendryte,k210-fpioa.yaml > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pinctrl/kendryte,k210-fpioa.yaml: properties:clocks:minItems: False schema does not allow 2 /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pinctrl/kendryte,k210-fpioa.yaml: properties:clocks:maxItems: False schema does not allow 2 /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pinctrl/kendryte,k210-fpioa.yaml: properties:clock-names:minItems: False schema does not allow 2 /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pinctrl/kendryte,k210-fpioa.yaml: properties:clock-names:maxItems: False schema does not allow 2 /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pinctrl/kendryte,k210-fpioa.yaml: ignoring, error in schema: properties: clocks: minItems warning: no schema found in file: ./Documentation/devicetree/bindings/pinctrl/kendryte,k210-fpioa.yaml Documentation/devicetree/bindings/pinctrl/kendryte,k210-fpioa.example.dts:19:18: fatal error: dt-bindings/pinctrl/k210-pinctrl.h: No such file or directory 19 | #include <dt-bindings/pinctrl/k210-pinctrl.h> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ compilation terminated. make[1]: *** [scripts/Makefile.lib:342: Documentation/devicetree/bindings/pinctrl/kendryte,k210-fpioa.example.dt.yaml] Error 1 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:1364: dt_binding_check] Error 2 See https://patchwork.ozlabs.org/patch/1396081 The base for the patch is generally the last rc1. Any dependencies should be noted. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
On Sat, 07 Nov 2020 17:14:13 +0900, Damien Le Moal wrote: > Document the device tree bindings for the Kendryte K210 SoC reset > controller driver in > Documentation/devicetree/bindings/reset/kendryte,k210-rst.yaml. > > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > --- > .../bindings/reset/kendryte,k210-rst.yaml | 78 +++++++++++++++++++ > 1 file changed, 78 insertions(+) > create mode 100644 Documentation/devicetree/bindings/reset/kendryte,k210-rst.yaml > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: ./Documentation/devicetree/bindings/reset/kendryte,k210-rst.yaml:24:9: [warning] wrong indentation: expected 10 but found 8 (indentation) dtschema/dtc warnings/errors: Documentation/devicetree/bindings/reset/kendryte,k210-rst.example.dts:19:18: fatal error: dt-bindings/mfd/k210-sysctl.h: No such file or directory 19 | #include <dt-bindings/mfd/k210-sysctl.h> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ compilation terminated. make[1]: *** [scripts/Makefile.lib:342: Documentation/devicetree/bindings/reset/kendryte,k210-rst.example.dt.yaml] Error 1 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:1364: dt_binding_check] Error 2 See https://patchwork.ozlabs.org/patch/1396082 The base for the patch is generally the last rc1. Any dependencies should be noted. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
Hello Andy, On Mon, Nov 09, 2020 at 05:14:21PM +0200, Andy Shevchenko wrote: > On Sat, Nov 7, 2020 at 10:14 AM Damien Le Moal <damien.lemoal@wdc.com> wrote: > > > @@ -1308,7 +1308,6 @@ DEFINE_SIMPLE_PROP(pinctrl7, "pinctrl-7", NULL) > > DEFINE_SIMPLE_PROP(pinctrl8, "pinctrl-8", NULL) > > DEFINE_SUFFIX_PROP(regulators, "-supply", NULL) > > DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells") > > -DEFINE_SUFFIX_PROP(gpios, "-gpios", "#gpio-cells") > > Sorry, but the above doesn't sound right to me. > It's a generic code and you may imagine how many systems you broke by > this change. Damien replaced the macro above with the code below (your removed it from your message): +static struct device_node *parse_gpios(struct device_node *np, + const char *prop_name, int index) +{ + /* + * Quirck for the DesignWare gpio-dwapb GPIO driver which defines + * the "snps,nr-gpios" property to indicate the total number of GPIOs + * available. As this conflict with "xx-gpios" reference properties, + * ignore it. + */ + if (strcmp(prop_name, "snps,nr-gpios") == 0) + return NULL; + + return parse_suffix_prop_cells(np, prop_name, index, + "-gpios", "#gpio-cells"); +} So AFAICS removing the macro shouldn't cause any problem. My concern was whether the quirk has been really needed. As I said the "snps,nr-gpios" property has been marked as deprecated in favor of the standard "ngpios" one. Due to the problem noted by Damien any deprecated property utilization will cause the DW APB SSI DT-nodes probe malfunction. That though implicitly but is supposed to encourage people to provide fixes for the dts-files with the deprecated property replaced with "ngpios". On the other hand an encouragement based on breaking the kernel doesn't seem a good solution. So as I see it either we should accept the solution provided by Damien, or replace it with a series of fixes for all dts-es with DW APB SSI DT-node defined. I suggest to hear the OF-subsystem maintainers out what solution would they prefer. -Sergey > > -- > With Best Regards, > Andy Shevchenko
Hello Andy, On Mon, Nov 09, 2020 at 04:36:51PM +0200, Andy Shevchenko wrote: > On Mon, Nov 9, 2020 at 4:25 PM Serge Semin <fancer.lancer@gmail.com> wrote: > > > > Hello Damien, > > Thanks for your patches. My comments are below. > > > > On Sat, Nov 07, 2020 at 05:13:50PM +0900, Damien Le Moal wrote: > > > Synopsis DesignWare DW_apb_ssi version 4 defines a 32-bit layout of > > > the ctrlr0 register for SPI masters. The layout of ctrlr0 is: > > > > > > | 31 .. 23 | 22 .. 21 | 20 .. 16 | > > > | other stuff | spi_frf | dfs_32 | > > > > > > | 15 .. 10 | 9 .. 8 | 7 .. 6 | 5 .. 4 | 3 .. 0 | > > > | other stuff | tmod | mode | frf | dfs | > > > > > > > > Th main difference of this layout with the 16-bits version is the data > > ^ > > | > > e > > > > > frame format field which resides in bits 16..20 instead of bits 3..0. > > > > > > > Are you sure they have been moved from [0, 3] to [16, 20]? I don't have the > > manual for the 4.0x version of the core, but according to this patch: > > https://patchwork.kernel.org/project/spi-devel-general/patch/1575907443-26377-7-git-send-email-wan.ahmad.zainie.wan.mohamad@intel.com/ > > it has been ok to use the lowest four bits for DFS setting. Is the commit > > message misleading there? > > 20:16 DFS_32 > Data Frame Size in 32-bit transfer size mode. Used to select > the data frame size in 32-bit transfer mode. These bits are > only valid when SSI_MAX_XFER_SIZE is configured to 32. > When the data frame size is programmed to be less than 32 > bits, the receive data are automatically right-justified by the > receive logic, with the upper bits of the receive FIFO zero- > padded. > > 3:0 DFS > Data Frame Size. > This register field is only valid when SSI_MAX_XFER_SIZE > is configured to 16. If SSI_MAX_XFER_SIZE is configured to > 32, then writing to this field will not have any effect. > > As far as I can tell it's an extension to the existing one (which one > in use depends on the SSI configuration). > > > The comment you are referring to is about DW_ssi v1.x (vs. DW_apb_ssi v4.x). Ok. Thanks for clarifying this. Now I see the way it's working. -Sergey > > > -- > With Best Regards, > Andy Shevchenko
On Sat, Nov 07, 2020 at 05:13:54PM +0900, Damien Le Moal wrote: > The DW SPI master of the Kendryte K210 RISC-V SoC uses the 32-bits > ctrlr0 register format. This SoC is also quite slow and gets significant > SD card performance improvements from using no-delay polled transfers. > Add the dw_spi_k210_init() function tied to the > "canaan,kendryte-k210-spi" compatible string to set the > DW_SPI_CAP_DFS_32 and DW_SPI_CAP_POLL_NODELAY DW SPI capability fields > for this SoC. > > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > --- > drivers/spi/spi-dw-mmio.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c > index 3f1bc384cb45..a00def6c5b39 100644 > --- a/drivers/spi/spi-dw-mmio.c > +++ b/drivers/spi/spi-dw-mmio.c > @@ -223,6 +223,14 @@ static int dw_spi_keembay_init(struct platform_device *pdev, > return 0; > } > > +static int dw_spi_k210_init(struct platform_device *pdev, > + struct dw_spi_mmio *dwsmmio) > +{ > + dwsmmio->dws.caps = DW_SPI_CAP_DFS_32 | DW_SPI_CAP_POLL_NODELAY; > + > + return 0; > +} > + > static int dw_spi_mmio_probe(struct platform_device *pdev) > { > int (*init_func)(struct platform_device *pdev, > @@ -340,6 +348,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = { > { .compatible = "snps,dwc-ssi-1.01a", .data = dw_spi_dwc_ssi_init}, > { .compatible = "intel,keembay-ssi", .data = dw_spi_keembay_init}, > { .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init}, > + { .compatible = "canaan,kendryte-k210-spi", .data = dw_spi_k210_init}, Other than the comments from Sean and Mark regarding the DFS_32 feature runtime detectability, I couldn't find a patch with adding the new new compatible string into the DW APB SSI DT schema. Have I missed it? If I haven't could you add one to the next version of the series? -Sergey > { /* end of table */} > }; > MODULE_DEVICE_TABLE(of, dw_spi_mmio_of_match); > -- > 2.28.0 >
On 2020/11/10 6:22, Serge Semin wrote: > On Sat, Nov 07, 2020 at 05:13:54PM +0900, Damien Le Moal wrote: >> The DW SPI master of the Kendryte K210 RISC-V SoC uses the 32-bits >> ctrlr0 register format. This SoC is also quite slow and gets significant >> SD card performance improvements from using no-delay polled transfers. >> Add the dw_spi_k210_init() function tied to the >> "canaan,kendryte-k210-spi" compatible string to set the >> DW_SPI_CAP_DFS_32 and DW_SPI_CAP_POLL_NODELAY DW SPI capability fields >> for this SoC. >> >> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> >> --- >> drivers/spi/spi-dw-mmio.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c >> index 3f1bc384cb45..a00def6c5b39 100644 >> --- a/drivers/spi/spi-dw-mmio.c >> +++ b/drivers/spi/spi-dw-mmio.c >> @@ -223,6 +223,14 @@ static int dw_spi_keembay_init(struct platform_device *pdev, >> return 0; >> } >> >> +static int dw_spi_k210_init(struct platform_device *pdev, >> + struct dw_spi_mmio *dwsmmio) >> +{ >> + dwsmmio->dws.caps = DW_SPI_CAP_DFS_32 | DW_SPI_CAP_POLL_NODELAY; >> + >> + return 0; >> +} >> + >> static int dw_spi_mmio_probe(struct platform_device *pdev) >> { >> int (*init_func)(struct platform_device *pdev, >> @@ -340,6 +348,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = { >> { .compatible = "snps,dwc-ssi-1.01a", .data = dw_spi_dwc_ssi_init}, >> { .compatible = "intel,keembay-ssi", .data = dw_spi_keembay_init}, >> { .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init}, > >> + { .compatible = "canaan,kendryte-k210-spi", .data = dw_spi_k210_init}, > > Other than the comments from Sean and Mark regarding the DFS_32 > feature runtime detectability, I couldn't find a patch with adding the > new new compatible string into the DW APB SSI DT schema. Have I missed > it? If I haven't could you add one to the next version of the series? Yes, I will. I forgot to change the DW DT binding doc for this. I did add a patch for the "polling" property but forgot the compatible string. In any case, I think that this new compatible string change can be dropped by switching to automatically detecting the DFS32 and using a different solution than the polling property change I sent for the RX fifo overflow problem. I am still going through all the emails trying to understand what to try next to avoid the polling "hack". Thanks ! > > -Sergey > >> { /* end of table */} >> }; >> MODULE_DEVICE_TABLE(of, dw_spi_mmio_of_match); >> -- >> 2.28.0 >> >
On Mon, Nov 09, 2020 at 09:39:19PM +0000, Damien Le Moal wrote: > On 2020/11/10 6:22, Serge Semin wrote: > > On Sat, Nov 07, 2020 at 05:13:54PM +0900, Damien Le Moal wrote: > >> The DW SPI master of the Kendryte K210 RISC-V SoC uses the 32-bits > >> ctrlr0 register format. This SoC is also quite slow and gets significant > >> SD card performance improvements from using no-delay polled transfers. > >> Add the dw_spi_k210_init() function tied to the > >> "canaan,kendryte-k210-spi" compatible string to set the > >> DW_SPI_CAP_DFS_32 and DW_SPI_CAP_POLL_NODELAY DW SPI capability fields > >> for this SoC. > >> > >> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > >> --- > >> drivers/spi/spi-dw-mmio.c | 9 +++++++++ > >> 1 file changed, 9 insertions(+) > >> > >> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c > >> index 3f1bc384cb45..a00def6c5b39 100644 > >> --- a/drivers/spi/spi-dw-mmio.c > >> +++ b/drivers/spi/spi-dw-mmio.c > >> @@ -223,6 +223,14 @@ static int dw_spi_keembay_init(struct platform_device *pdev, > >> return 0; > >> } > >> > >> +static int dw_spi_k210_init(struct platform_device *pdev, > >> + struct dw_spi_mmio *dwsmmio) > >> +{ > >> + dwsmmio->dws.caps = DW_SPI_CAP_DFS_32 | DW_SPI_CAP_POLL_NODELAY; > >> + > >> + return 0; > >> +} > >> + > >> static int dw_spi_mmio_probe(struct platform_device *pdev) > >> { > >> int (*init_func)(struct platform_device *pdev, > >> @@ -340,6 +348,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = { > >> { .compatible = "snps,dwc-ssi-1.01a", .data = dw_spi_dwc_ssi_init}, > >> { .compatible = "intel,keembay-ssi", .data = dw_spi_keembay_init}, > >> { .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init}, > > > >> + { .compatible = "canaan,kendryte-k210-spi", .data = dw_spi_k210_init}, > > > > Other than the comments from Sean and Mark regarding the DFS_32 > > feature runtime detectability, I couldn't find a patch with adding the > > new new compatible string into the DW APB SSI DT schema. Have I missed > > it? If I haven't could you add one to the next version of the series? > > Yes, I will. I forgot to change the DW DT binding doc for this. I did add a > patch for the "polling" property but forgot the compatible string. > > In any case, I think that this new compatible string change can be dropped by > switching to automatically detecting the DFS32 and using a different solution > than the polling property change I sent for the RX fifo overflow problem. No, new SoC needs new compatible string. Especially if a new vendor. > > I am still going through all the emails trying to understand what to try next to > avoid the polling "hack". Use compatible. Rob
On Sat, Nov 07, 2020 at 05:14:03PM +0900, Damien Le Moal wrote: > Introduce the dt-bindings file include/dt-bindings/mfd/k210_sysctl.h to > define the offset of all registers of the K210 system controller. We generally don't have defines for registers in DT. > > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > --- > include/dt-bindings/mfd/k210-sysctl.h | 41 +++++++++++++++++++++++++++ > 1 file changed, 41 insertions(+) > create mode 100644 include/dt-bindings/mfd/k210-sysctl.h > > diff --git a/include/dt-bindings/mfd/k210-sysctl.h b/include/dt-bindings/mfd/k210-sysctl.h > new file mode 100644 > index 000000000000..5cc386d3c9ca > --- /dev/null > +++ b/include/dt-bindings/mfd/k210-sysctl.h > @@ -0,0 +1,41 @@ > +/* SPDX-License-Identifier: GPL-2.0+ */ > +/* > + * Copyright (C) 2020 Sean Anderson <seanga2@gmail.com> > + * Copyright (c) 2020 Western Digital Corporation or its affiliates. > + */ > +#ifndef MFD_K210_SYSCTL_H > +#define MFD_K210_SYSCTL_H > + > +/* > + * Kendryte K210 SoC system controller registers offsets. > + * Taken from Kendryte SDK (kendryte-standalone-sdk). > + */ > +#define K210_SYSCTL_GIT_ID 0x00 /* Git short commit id */ > +#define K210_SYSCTL_UART_BAUD 0x04 /* Default UARTHS baud rate */ > +#define K210_SYSCTL_PLL0 0x08 /* PLL0 controller */ > +#define K210_SYSCTL_PLL1 0x0C /* PLL1 controller */ > +#define K210_SYSCTL_PLL2 0x10 /* PLL2 controller */ > +#define K210_SYSCTL_PLL_LOCK 0x18 /* PLL lock tester */ > +#define K210_SYSCTL_ROM_ERROR 0x1C /* AXI ROM detector */ > +#define K210_SYSCTL_SEL0 0x20 /* Clock select controller 0 */ > +#define K210_SYSCTL_SEL1 0x24 /* Clock select controller 1 */ > +#define K210_SYSCTL_EN_CENT 0x28 /* Central clock enable */ > +#define K210_SYSCTL_EN_PERI 0x2C /* Peripheral clock enable */ > +#define K210_SYSCTL_SOFT_RESET 0x30 /* Soft reset ctrl */ > +#define K210_SYSCTL_PERI_RESET 0x34 /* Peripheral reset controller */ > +#define K210_SYSCTL_THR0 0x38 /* Clock threshold controller 0 */ > +#define K210_SYSCTL_THR1 0x3C /* Clock threshold controller 1 */ > +#define K210_SYSCTL_THR2 0x40 /* Clock threshold controller 2 */ > +#define K210_SYSCTL_THR3 0x44 /* Clock threshold controller 3 */ > +#define K210_SYSCTL_THR4 0x48 /* Clock threshold controller 4 */ > +#define K210_SYSCTL_THR5 0x4C /* Clock threshold controller 5 */ > +#define K210_SYSCTL_THR6 0x50 /* Clock threshold controller 6 */ > +#define K210_SYSCTL_MISC 0x54 /* Miscellaneous controller */ > +#define K210_SYSCTL_PERI 0x58 /* Peripheral controller */ > +#define K210_SYSCTL_SPI_SLEEP 0x5C /* SPI sleep controller */ > +#define K210_SYSCTL_RESET_STAT 0x60 /* Reset source status */ > +#define K210_SYSCTL_DMA_SEL0 0x64 /* DMA handshake selector 0 */ > +#define K210_SYSCTL_DMA_SEL1 0x68 /* DMA handshake selector 1 */ > +#define K210_SYSCTL_POWER_SEL 0x6C /* IO Power Mode Select controller */ > + > +#endif /* MFD_K210_SYSCTL_H */ > -- > 2.28.0 >
On 2020/11/10 6:55, Rob Herring wrote: > On Mon, Nov 09, 2020 at 09:39:19PM +0000, Damien Le Moal wrote: >> On 2020/11/10 6:22, Serge Semin wrote: >>> On Sat, Nov 07, 2020 at 05:13:54PM +0900, Damien Le Moal wrote: >>>> The DW SPI master of the Kendryte K210 RISC-V SoC uses the 32-bits >>>> ctrlr0 register format. This SoC is also quite slow and gets significant >>>> SD card performance improvements from using no-delay polled transfers. >>>> Add the dw_spi_k210_init() function tied to the >>>> "canaan,kendryte-k210-spi" compatible string to set the >>>> DW_SPI_CAP_DFS_32 and DW_SPI_CAP_POLL_NODELAY DW SPI capability fields >>>> for this SoC. >>>> >>>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> >>>> --- >>>> drivers/spi/spi-dw-mmio.c | 9 +++++++++ >>>> 1 file changed, 9 insertions(+) >>>> >>>> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c >>>> index 3f1bc384cb45..a00def6c5b39 100644 >>>> --- a/drivers/spi/spi-dw-mmio.c >>>> +++ b/drivers/spi/spi-dw-mmio.c >>>> @@ -223,6 +223,14 @@ static int dw_spi_keembay_init(struct platform_device *pdev, >>>> return 0; >>>> } >>>> >>>> +static int dw_spi_k210_init(struct platform_device *pdev, >>>> + struct dw_spi_mmio *dwsmmio) >>>> +{ >>>> + dwsmmio->dws.caps = DW_SPI_CAP_DFS_32 | DW_SPI_CAP_POLL_NODELAY; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> static int dw_spi_mmio_probe(struct platform_device *pdev) >>>> { >>>> int (*init_func)(struct platform_device *pdev, >>>> @@ -340,6 +348,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = { >>>> { .compatible = "snps,dwc-ssi-1.01a", .data = dw_spi_dwc_ssi_init}, >>>> { .compatible = "intel,keembay-ssi", .data = dw_spi_keembay_init}, >>>> { .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init}, >>> >>>> + { .compatible = "canaan,kendryte-k210-spi", .data = dw_spi_k210_init}, >>> >>> Other than the comments from Sean and Mark regarding the DFS_32 >>> feature runtime detectability, I couldn't find a patch with adding the >>> new new compatible string into the DW APB SSI DT schema. Have I missed >>> it? If I haven't could you add one to the next version of the series? >> >> Yes, I will. I forgot to change the DW DT binding doc for this. I did add a >> patch for the "polling" property but forgot the compatible string. >> >> In any case, I think that this new compatible string change can be dropped by >> switching to automatically detecting the DFS32 and using a different solution >> than the polling property change I sent for the RX fifo overflow problem. > > No, new SoC needs new compatible string. Especially if a new vendor. My apologies for the bad wording: I meant to say the change to the list of compatible strings that the DW SPI support would not be needed. So from the DW SPI point of view, there would be no new compatible string to add/document. > >> >> I am still going through all the emails trying to understand what to try next to >> avoid the polling "hack". > > Use compatible. Yes, that is what this patch used. Again, I think there is a chance this change can be dropped. > > Rob >
On 11/9/20 4:59 PM, Rob Herring wrote: > On Sat, Nov 07, 2020 at 05:14:03PM +0900, Damien Le Moal wrote: >> Introduce the dt-bindings file include/dt-bindings/mfd/k210_sysctl.h to >> define the offset of all registers of the K210 system controller. > > We generally don't have defines for registers in DT. At least K210_SYSCTL_SOFT_RESET is necessary for the syscon-reboot driver. --Sean > >> >> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> >> --- >> include/dt-bindings/mfd/k210-sysctl.h | 41 +++++++++++++++++++++++++++ >> 1 file changed, 41 insertions(+) >> create mode 100644 include/dt-bindings/mfd/k210-sysctl.h >> >> diff --git a/include/dt-bindings/mfd/k210-sysctl.h b/include/dt-bindings/mfd/k210-sysctl.h >> new file mode 100644 >> index 000000000000..5cc386d3c9ca >> --- /dev/null >> +++ b/include/dt-bindings/mfd/k210-sysctl.h >> @@ -0,0 +1,41 @@ >> +/* SPDX-License-Identifier: GPL-2.0+ */ >> +/* >> + * Copyright (C) 2020 Sean Anderson <seanga2@gmail.com> >> + * Copyright (c) 2020 Western Digital Corporation or its affiliates. >> + */ >> +#ifndef MFD_K210_SYSCTL_H >> +#define MFD_K210_SYSCTL_H >> + >> +/* >> + * Kendryte K210 SoC system controller registers offsets. >> + * Taken from Kendryte SDK (kendryte-standalone-sdk). >> + */ >> +#define K210_SYSCTL_GIT_ID 0x00 /* Git short commit id */ >> +#define K210_SYSCTL_UART_BAUD 0x04 /* Default UARTHS baud rate */ >> +#define K210_SYSCTL_PLL0 0x08 /* PLL0 controller */ >> +#define K210_SYSCTL_PLL1 0x0C /* PLL1 controller */ >> +#define K210_SYSCTL_PLL2 0x10 /* PLL2 controller */ >> +#define K210_SYSCTL_PLL_LOCK 0x18 /* PLL lock tester */ >> +#define K210_SYSCTL_ROM_ERROR 0x1C /* AXI ROM detector */ >> +#define K210_SYSCTL_SEL0 0x20 /* Clock select controller 0 */ >> +#define K210_SYSCTL_SEL1 0x24 /* Clock select controller 1 */ >> +#define K210_SYSCTL_EN_CENT 0x28 /* Central clock enable */ >> +#define K210_SYSCTL_EN_PERI 0x2C /* Peripheral clock enable */ >> +#define K210_SYSCTL_SOFT_RESET 0x30 /* Soft reset ctrl */ >> +#define K210_SYSCTL_PERI_RESET 0x34 /* Peripheral reset controller */ >> +#define K210_SYSCTL_THR0 0x38 /* Clock threshold controller 0 */ >> +#define K210_SYSCTL_THR1 0x3C /* Clock threshold controller 1 */ >> +#define K210_SYSCTL_THR2 0x40 /* Clock threshold controller 2 */ >> +#define K210_SYSCTL_THR3 0x44 /* Clock threshold controller 3 */ >> +#define K210_SYSCTL_THR4 0x48 /* Clock threshold controller 4 */ >> +#define K210_SYSCTL_THR5 0x4C /* Clock threshold controller 5 */ >> +#define K210_SYSCTL_THR6 0x50 /* Clock threshold controller 6 */ >> +#define K210_SYSCTL_MISC 0x54 /* Miscellaneous controller */ >> +#define K210_SYSCTL_PERI 0x58 /* Peripheral controller */ >> +#define K210_SYSCTL_SPI_SLEEP 0x5C /* SPI sleep controller */ >> +#define K210_SYSCTL_RESET_STAT 0x60 /* Reset source status */ >> +#define K210_SYSCTL_DMA_SEL0 0x64 /* DMA handshake selector 0 */ >> +#define K210_SYSCTL_DMA_SEL1 0x68 /* DMA handshake selector 1 */ >> +#define K210_SYSCTL_POWER_SEL 0x6C /* IO Power Mode Select controller */ >> + >> +#endif /* MFD_K210_SYSCTL_H */ >> -- >> 2.28.0 >>
On Mon, Nov 9, 2020 at 4:00 PM Damien Le Moal <Damien.LeMoal@wdc.com> wrote: > > On 2020/11/10 6:55, Rob Herring wrote: > > On Mon, Nov 09, 2020 at 09:39:19PM +0000, Damien Le Moal wrote: > >> On 2020/11/10 6:22, Serge Semin wrote: > >>> On Sat, Nov 07, 2020 at 05:13:54PM +0900, Damien Le Moal wrote: > >>>> The DW SPI master of the Kendryte K210 RISC-V SoC uses the 32-bits > >>>> ctrlr0 register format. This SoC is also quite slow and gets significant > >>>> SD card performance improvements from using no-delay polled transfers. > >>>> Add the dw_spi_k210_init() function tied to the > >>>> "canaan,kendryte-k210-spi" compatible string to set the > >>>> DW_SPI_CAP_DFS_32 and DW_SPI_CAP_POLL_NODELAY DW SPI capability fields > >>>> for this SoC. > >>>> > >>>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > >>>> --- > >>>> drivers/spi/spi-dw-mmio.c | 9 +++++++++ > >>>> 1 file changed, 9 insertions(+) > >>>> > >>>> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c > >>>> index 3f1bc384cb45..a00def6c5b39 100644 > >>>> --- a/drivers/spi/spi-dw-mmio.c > >>>> +++ b/drivers/spi/spi-dw-mmio.c > >>>> @@ -223,6 +223,14 @@ static int dw_spi_keembay_init(struct platform_device *pdev, > >>>> return 0; > >>>> } > >>>> > >>>> +static int dw_spi_k210_init(struct platform_device *pdev, > >>>> + struct dw_spi_mmio *dwsmmio) > >>>> +{ > >>>> + dwsmmio->dws.caps = DW_SPI_CAP_DFS_32 | DW_SPI_CAP_POLL_NODELAY; > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > >>>> static int dw_spi_mmio_probe(struct platform_device *pdev) > >>>> { > >>>> int (*init_func)(struct platform_device *pdev, > >>>> @@ -340,6 +348,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = { > >>>> { .compatible = "snps,dwc-ssi-1.01a", .data = dw_spi_dwc_ssi_init}, > >>>> { .compatible = "intel,keembay-ssi", .data = dw_spi_keembay_init}, > >>>> { .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init}, > >>> > >>>> + { .compatible = "canaan,kendryte-k210-spi", .data = dw_spi_k210_init}, > >>> > >>> Other than the comments from Sean and Mark regarding the DFS_32 > >>> feature runtime detectability, I couldn't find a patch with adding the > >>> new new compatible string into the DW APB SSI DT schema. Have I missed > >>> it? If I haven't could you add one to the next version of the series? > >> > >> Yes, I will. I forgot to change the DW DT binding doc for this. I did add a > >> patch for the "polling" property but forgot the compatible string. > >> > >> In any case, I think that this new compatible string change can be dropped by > >> switching to automatically detecting the DFS32 and using a different solution > >> than the polling property change I sent for the RX fifo overflow problem. > > > > No, new SoC needs new compatible string. Especially if a new vendor. > > My apologies for the bad wording: I meant to say the change to the list of > compatible strings that the DW SPI support would not be needed. So from the DW > SPI point of view, there would be no new compatible string to add/document. No, there is a need for a new compatible string to add/document. You might not need it in the driver if you have a fallback. Compatible strings should be SoC specific so you can handle quirks without a DT change. Otherwise, it's a never ending stream of new properties and DT updates. > >> I am still going through all the emails trying to understand what to try next to > >> avoid the polling "hack". > > > > Use compatible. > > Yes, that is what this patch used. Again, I think there is a chance this change > can be dropped. Looks to me like it used a 'polling' property... Rob
On 2020/11/10 8:08, Rob Herring wrote: > On Mon, Nov 9, 2020 at 4:00 PM Damien Le Moal <Damien.LeMoal@wdc.com> wrote: >> >> On 2020/11/10 6:55, Rob Herring wrote: >>> On Mon, Nov 09, 2020 at 09:39:19PM +0000, Damien Le Moal wrote: >>>> On 2020/11/10 6:22, Serge Semin wrote: >>>>> On Sat, Nov 07, 2020 at 05:13:54PM +0900, Damien Le Moal wrote: >>>>>> The DW SPI master of the Kendryte K210 RISC-V SoC uses the 32-bits >>>>>> ctrlr0 register format. This SoC is also quite slow and gets significant >>>>>> SD card performance improvements from using no-delay polled transfers. >>>>>> Add the dw_spi_k210_init() function tied to the >>>>>> "canaan,kendryte-k210-spi" compatible string to set the >>>>>> DW_SPI_CAP_DFS_32 and DW_SPI_CAP_POLL_NODELAY DW SPI capability fields >>>>>> for this SoC. >>>>>> >>>>>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> >>>>>> --- >>>>>> drivers/spi/spi-dw-mmio.c | 9 +++++++++ >>>>>> 1 file changed, 9 insertions(+) >>>>>> >>>>>> diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c >>>>>> index 3f1bc384cb45..a00def6c5b39 100644 >>>>>> --- a/drivers/spi/spi-dw-mmio.c >>>>>> +++ b/drivers/spi/spi-dw-mmio.c >>>>>> @@ -223,6 +223,14 @@ static int dw_spi_keembay_init(struct platform_device *pdev, >>>>>> return 0; >>>>>> } >>>>>> >>>>>> +static int dw_spi_k210_init(struct platform_device *pdev, >>>>>> + struct dw_spi_mmio *dwsmmio) >>>>>> +{ >>>>>> + dwsmmio->dws.caps = DW_SPI_CAP_DFS_32 | DW_SPI_CAP_POLL_NODELAY; >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> static int dw_spi_mmio_probe(struct platform_device *pdev) >>>>>> { >>>>>> int (*init_func)(struct platform_device *pdev, >>>>>> @@ -340,6 +348,7 @@ static const struct of_device_id dw_spi_mmio_of_match[] = { >>>>>> { .compatible = "snps,dwc-ssi-1.01a", .data = dw_spi_dwc_ssi_init}, >>>>>> { .compatible = "intel,keembay-ssi", .data = dw_spi_keembay_init}, >>>>>> { .compatible = "microchip,sparx5-spi", dw_spi_mscc_sparx5_init}, >>>>> >>>>>> + { .compatible = "canaan,kendryte-k210-spi", .data = dw_spi_k210_init}, >>>>> >>>>> Other than the comments from Sean and Mark regarding the DFS_32 >>>>> feature runtime detectability, I couldn't find a patch with adding the >>>>> new new compatible string into the DW APB SSI DT schema. Have I missed >>>>> it? If I haven't could you add one to the next version of the series? >>>> >>>> Yes, I will. I forgot to change the DW DT binding doc for this. I did add a >>>> patch for the "polling" property but forgot the compatible string. >>>> >>>> In any case, I think that this new compatible string change can be dropped by >>>> switching to automatically detecting the DFS32 and using a different solution >>>> than the polling property change I sent for the RX fifo overflow problem. >>> >>> No, new SoC needs new compatible string. Especially if a new vendor. >> >> My apologies for the bad wording: I meant to say the change to the list of >> compatible strings that the DW SPI support would not be needed. So from the DW >> SPI point of view, there would be no new compatible string to add/document. > > No, there is a need for a new compatible string to add/document. You > might not need it in the driver if you have a fallback. > > Compatible strings should be SoC specific so you can handle quirks > without a DT change. Otherwise, it's a never ending stream of new > properties and DT updates. Ah. OK. I get it now. Thanks for clarifying. So I will keep the new compatible string (renamed with proper vendor name instead of brand) and document it. > >>>> I am still going through all the emails trying to understand what to try next to >>>> avoid the polling "hack". >>> >>> Use compatible. >> >> Yes, that is what this patch used. Again, I think there is a chance this change >> can be dropped. > > Looks to me like it used a 'polling' property... I hope to be able to get rid of this change if a proper solution can be found to the transfer speed problem I am seeing. > > Rob >
On Mon, Nov 9, 2020 at 9:45 AM Sean Anderson <seanga2@gmail.com> wrote: > > On 11/9/20 10:36 AM, Rob Herring wrote: > > On Sat, Nov 07, 2020 at 05:14:12PM +0900, Damien Le Moal wrote: > >> Document the device tree bindings for the Kendryte K210 SoC Fully > >> Programmable IO Array (FPIOA) pinctrl driver in > >> Documentation/devicetree/bindings/pinctrl/kendryte,k210-fpioa.yaml > >> > >> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > >> --- > >> .../bindings/pinctrl/kendryte,k210-fpioa.yaml | 106 ++++++++++++++++++ > >> 1 file changed, 106 insertions(+) > >> create mode 100644 Documentation/devicetree/bindings/pinctrl/kendryte,k210-fpioa.yaml > >> > >> diff --git a/Documentation/devicetree/bindings/pinctrl/kendryte,k210-fpioa.yaml b/Documentation/devicetree/bindings/pinctrl/kendryte,k210-fpioa.yaml > >> new file mode 100644 > >> index 000000000000..8730add88ee0 > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/pinctrl/kendryte,k210-fpioa.yaml > >> @@ -0,0 +1,106 @@ > >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > >> +%YAML 1.2 > >> +--- > >> +$id: http://devicetree.org/schemas/pinctrl/kendryte,k210-fpioa.yaml# > >> +$schema: http://devicetree.org/meta-schemas/core.yaml# > >> + > >> +title: Kendryte K210 FPIOA (Fully Programmable IO Array) Device Tree Bindings > >> + > >> +maintainers: > >> + - Damien Le Moal <damien.lemoal@wdc.com> > >> + > >> +description: > >> + The Kendryte K210 SoC Fully Programmable IO Array controller allows assiging > >> + any of 256 possible functions to any of 48 IO pins. Pin function configuration > >> + is performed on a per-pin basis. > >> + > >> +properties: > >> + compatible: > >> + const: kendryte,k210-fpioa > >> + > >> + reg: > >> + description: FPIOA controller register space base address and size > >> + > >> + clocks: > >> + minItems: 2 > >> + maxItems: 2 > > > > Can drop these. Implied by 'items' length. > > > >> + items: > >> + - description: Controller reference clock source > >> + - description: APB interface clock source > >> + > >> + clock-names: > >> + minItems: 2 > >> + maxItems: 2 > >> + items: > >> + - const: ref > >> + - const: pclk > >> + > >> + resets: > >> + maxItems: 1 > >> + > >> + kendryte,sysctl: > >> + minItems: 1 > >> + maxItems: 1 > >> + $ref: /schemas/types.yaml#/definitions/phandle-array > >> + description: | > >> + phandle to the system controller node > >> + > >> + kendryte,power-offset: > >> + minItems: 1 > >> + maxItems: 1 > >> + $ref: /schemas/types.yaml#/definitions/uint32 > >> + description: | > >> + Offset of the power domain control register of the system controller. > > > > Sounds like you should be using power-domains binding. > > This is for pin power domains. E.g. pins 0-5 can be set to 1V8 or 3V3 logic levels. Okay, please make that clear in the description. You can combine the above 2 properties into one which is a phandle+offset. Rob
On 2020/11/11 23:32, Rob Herring wrote: > On Mon, Nov 9, 2020 at 9:45 AM Sean Anderson <seanga2@gmail.com> wrote: >> >> On 11/9/20 10:36 AM, Rob Herring wrote: >>> On Sat, Nov 07, 2020 at 05:14:12PM +0900, Damien Le Moal wrote: >>>> Document the device tree bindings for the Kendryte K210 SoC Fully >>>> Programmable IO Array (FPIOA) pinctrl driver in >>>> Documentation/devicetree/bindings/pinctrl/kendryte,k210-fpioa.yaml >>>> >>>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> >>>> --- >>>> .../bindings/pinctrl/kendryte,k210-fpioa.yaml | 106 ++++++++++++++++++ >>>> 1 file changed, 106 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/pinctrl/kendryte,k210-fpioa.yaml >>>> >>>> diff --git a/Documentation/devicetree/bindings/pinctrl/kendryte,k210-fpioa.yaml b/Documentation/devicetree/bindings/pinctrl/kendryte,k210-fpioa.yaml >>>> new file mode 100644 >>>> index 000000000000..8730add88ee0 >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/pinctrl/kendryte,k210-fpioa.yaml >>>> @@ -0,0 +1,106 @@ >>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>>> +%YAML 1.2 >>>> +--- >>>> +$id: http://devicetree.org/schemas/pinctrl/kendryte,k210-fpioa.yaml# >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>> + >>>> +title: Kendryte K210 FPIOA (Fully Programmable IO Array) Device Tree Bindings >>>> + >>>> +maintainers: >>>> + - Damien Le Moal <damien.lemoal@wdc.com> >>>> + >>>> +description: >>>> + The Kendryte K210 SoC Fully Programmable IO Array controller allows assiging >>>> + any of 256 possible functions to any of 48 IO pins. Pin function configuration >>>> + is performed on a per-pin basis. >>>> + >>>> +properties: >>>> + compatible: >>>> + const: kendryte,k210-fpioa >>>> + >>>> + reg: >>>> + description: FPIOA controller register space base address and size >>>> + >>>> + clocks: >>>> + minItems: 2 >>>> + maxItems: 2 >>> >>> Can drop these. Implied by 'items' length. >>> >>>> + items: >>>> + - description: Controller reference clock source >>>> + - description: APB interface clock source >>>> + >>>> + clock-names: >>>> + minItems: 2 >>>> + maxItems: 2 >>>> + items: >>>> + - const: ref >>>> + - const: pclk >>>> + >>>> + resets: >>>> + maxItems: 1 >>>> + >>>> + kendryte,sysctl: >>>> + minItems: 1 >>>> + maxItems: 1 >>>> + $ref: /schemas/types.yaml#/definitions/phandle-array >>>> + description: | >>>> + phandle to the system controller node >>>> + >>>> + kendryte,power-offset: >>>> + minItems: 1 >>>> + maxItems: 1 >>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>> + description: | >>>> + Offset of the power domain control register of the system controller. >>> >>> Sounds like you should be using power-domains binding. >> >> This is for pin power domains. E.g. pins 0-5 can be set to 1V8 or 3V3 logic levels. > > Okay, please make that clear in the description. You can combine the > above 2 properties into one which is a phandle+offset. Could you point me to an example of such property ? I am not sure how to do that so an example would help. Thanks. > > Rob >
On Wed, 2020-11-11 at 15:06 +0000, Damien Le Moal wrote: > On 2020/11/11 23:32, Rob Herring wrote: > > On Mon, Nov 9, 2020 at 9:45 AM Sean Anderson <seanga2@gmail.com> wrote: > > > > > > On 11/9/20 10:36 AM, Rob Herring wrote: > > > > On Sat, Nov 07, 2020 at 05:14:12PM +0900, Damien Le Moal wrote: > > > > > Document the device tree bindings for the Kendryte K210 SoC Fully > > > > > Programmable IO Array (FPIOA) pinctrl driver in > > > > > Documentation/devicetree/bindings/pinctrl/kendryte,k210-fpioa.yaml > > > > > > > > > > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > > > > > --- > > > > > .../bindings/pinctrl/kendryte,k210-fpioa.yaml | 106 ++++++++++++++++++ > > > > > 1 file changed, 106 insertions(+) > > > > > create mode 100644 Documentation/devicetree/bindings/pinctrl/kendryte,k210-fpioa.yaml > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/pinctrl/kendryte,k210-fpioa.yaml b/Documentation/devicetree/bindings/pinctrl/kendryte,k210-fpioa.yaml > > > > > new file mode 100644 > > > > > index 000000000000..8730add88ee0 > > > > > --- /dev/null > > > > > +++ b/Documentation/devicetree/bindings/pinctrl/kendryte,k210-fpioa.yaml > > > > > @@ -0,0 +1,106 @@ > > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > > > +%YAML 1.2 > > > > > +--- > > > > > +$id: http://devicetree.org/schemas/pinctrl/kendryte,k210-fpioa.yaml# > > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > > + > > > > > +title: Kendryte K210 FPIOA (Fully Programmable IO Array) Device Tree Bindings > > > > > + > > > > > +maintainers: > > > > > + - Damien Le Moal <damien.lemoal@wdc.com> > > > > > + > > > > > +description: > > > > > + The Kendryte K210 SoC Fully Programmable IO Array controller allows assiging > > > > > + any of 256 possible functions to any of 48 IO pins. Pin function configuration > > > > > + is performed on a per-pin basis. > > > > > + > > > > > +properties: > > > > > + compatible: > > > > > + const: kendryte,k210-fpioa > > > > > + > > > > > + reg: > > > > > + description: FPIOA controller register space base address and size > > > > > + > > > > > + clocks: > > > > > + minItems: 2 > > > > > + maxItems: 2 > > > > > > > > Can drop these. Implied by 'items' length. > > > > > > > > > + items: > > > > > + - description: Controller reference clock source > > > > > + - description: APB interface clock source > > > > > + > > > > > + clock-names: > > > > > + minItems: 2 > > > > > + maxItems: 2 > > > > > + items: > > > > > + - const: ref > > > > > + - const: pclk > > > > > + > > > > > + resets: > > > > > + maxItems: 1 > > > > > + > > > > > + kendryte,sysctl: > > > > > + minItems: 1 > > > > > + maxItems: 1 > > > > > + $ref: /schemas/types.yaml#/definitions/phandle-array > > > > > + description: | > > > > > + phandle to the system controller node > > > > > + > > > > > + kendryte,power-offset: > > > > > + minItems: 1 > > > > > + maxItems: 1 > > > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > > > + description: | > > > > > + Offset of the power domain control register of the system controller. > > > > > > > > Sounds like you should be using power-domains binding. > > > > > > This is for pin power domains. E.g. pins 0-5 can be set to 1V8 or 3V3 logic levels. > > > > Okay, please make that clear in the description. You can combine the > > above 2 properties into one which is a phandle+offset. > > Could you point me to an example of such property ? I am not sure how to do that > so an example would help. Thanks. Please ignore. Found what I need. Thanks. > > > > > Rob > > > > -- Damien Le Moal Western Digital
On 2020/11/13 16:31, Stephen Boyd wrote: > Quoting Damien Le Moal (2020-11-07 00:13:56) >> If of_clk_init() is not called in time_init(), clock providers defined >> in the system device tree are not initialized, resulting in failures for >> other devices to initialize due to missing clocks. >> Similarly to other architectures and to the default kernel time_init() >> implementation, call of_clk_init() before executing timer_probe() in >> time_init(). > > Do you have timers that need clks to be running or queryable this early? > This of_clk_init() call is made here when architectures need to call > things like clk_get_rate() to figure out some clk frequency for their > clockevent or clocksource. It is OK to have this call here, I'm just > curious if this is actually necessary vs. delaying it to later. > I think the clocks could be initialized later, but at least the CLINT will depend on one of the clocks, same for the CPU frequency information. So need checking. What this patch fixes is not the need for a super early initialization though, it is that _nothing_ was being initialized without it: the clock driver probe function was never called with the current riscv time_init() as is. I looked at other architectures and at the default kernel time_init(), and mimicked what was done, that is, added of_clk_init(). Is there any other way to make sure that the needed clock drivers are initialized ?
Quoting Damien Le Moal (2020-11-12 23:40:17) > On 2020/11/13 16:31, Stephen Boyd wrote: > > Quoting Damien Le Moal (2020-11-07 00:13:56) > >> If of_clk_init() is not called in time_init(), clock providers defined > >> in the system device tree are not initialized, resulting in failures for > >> other devices to initialize due to missing clocks. > >> Similarly to other architectures and to the default kernel time_init() > >> implementation, call of_clk_init() before executing timer_probe() in > >> time_init(). > > > > Do you have timers that need clks to be running or queryable this early? > > This of_clk_init() call is made here when architectures need to call > > things like clk_get_rate() to figure out some clk frequency for their > > clockevent or clocksource. It is OK to have this call here, I'm just > > curious if this is actually necessary vs. delaying it to later. > > > > I think the clocks could be initialized later, but at least the CLINT will > depend on one of the clocks, same for the CPU frequency information. So need > checking. > > What this patch fixes is not the need for a super early initialization though, > it is that _nothing_ was being initialized without it: the clock driver probe > function was never called with the current riscv time_init() as is. I looked at > other architectures and at the default kernel time_init(), and mimicked what was > done, that is, added of_clk_init(). Is there any other way to make sure that the > needed clock drivers are initialized ? > Yes it's fine. Just the commit text reads as "If of_clk_init() is not called in time_init() then nothing works" which is true but made me wonder if it was because it needed to be super early or not. The commit text could be a little clearer here. We don't have any good solution for a fallback to call of_clk_init() somewhere later. I do wonder if we should generalize this though and call of_clk_init() from start_kernel() directly via some Kconfig that architectures select if they need it for their timer and then move it to an initcall if architectures don't select the config. Or throw it into the of_platform_default_populate_init() hook if the architecture doesn't need to call it early.
On 2020/11/13 16:53, Stephen Boyd wrote: > Quoting Damien Le Moal (2020-11-12 23:40:17) >> On 2020/11/13 16:31, Stephen Boyd wrote: >>> Quoting Damien Le Moal (2020-11-07 00:13:56) >>>> If of_clk_init() is not called in time_init(), clock providers defined >>>> in the system device tree are not initialized, resulting in failures for >>>> other devices to initialize due to missing clocks. >>>> Similarly to other architectures and to the default kernel time_init() >>>> implementation, call of_clk_init() before executing timer_probe() in >>>> time_init(). >>> >>> Do you have timers that need clks to be running or queryable this early? >>> This of_clk_init() call is made here when architectures need to call >>> things like clk_get_rate() to figure out some clk frequency for their >>> clockevent or clocksource. It is OK to have this call here, I'm just >>> curious if this is actually necessary vs. delaying it to later. >>> >> >> I think the clocks could be initialized later, but at least the CLINT will >> depend on one of the clocks, same for the CPU frequency information. So need >> checking. >> >> What this patch fixes is not the need for a super early initialization though, >> it is that _nothing_ was being initialized without it: the clock driver probe >> function was never called with the current riscv time_init() as is. I looked at >> other architectures and at the default kernel time_init(), and mimicked what was >> done, that is, added of_clk_init(). Is there any other way to make sure that the >> needed clock drivers are initialized ? >> > > Yes it's fine. Just the commit text reads as "If of_clk_init() is not > called in time_init() then nothing works" which is true but made me > wonder if it was because it needed to be super early or not. The commit > text could be a little clearer here. OK. I will clarify the commit message in V2. Working on it now. > We don't have any good solution for a fallback to call of_clk_init() > somewhere later. I do wonder if we should generalize this though and > call of_clk_init() from start_kernel() directly via some Kconfig that > architectures select if they need it for their timer and then move it to > an initcall if architectures don't select the config. Or throw it into > the of_platform_default_populate_init() hook if the architecture doesn't > need to call it early. This last idea seems reasonable and probably the easiest. And I think it could be done unconditionally even if the arch calls of_clk_init() early as the already populated clock provider nodes would not be initialized again.
Quoting Damien Le Moal (2020-11-12 23:57:19) > On 2020/11/13 16:53, Stephen Boyd wrote: > > > > Yes it's fine. Just the commit text reads as "If of_clk_init() is not > > called in time_init() then nothing works" which is true but made me > > wonder if it was because it needed to be super early or not. The commit > > text could be a little clearer here. > > OK. I will clarify the commit message in V2. Working on it now. Thanks! > > > We don't have any good solution for a fallback to call of_clk_init() > > somewhere later. I do wonder if we should generalize this though and > > call of_clk_init() from start_kernel() directly via some Kconfig that > > architectures select if they need it for their timer and then move it to > > an initcall if architectures don't select the config. Or throw it into > > the of_platform_default_populate_init() hook if the architecture doesn't > > need to call it early. > > This last idea seems reasonable and probably the easiest. And I think it could > be done unconditionally even if the arch calls of_clk_init() early as the > already populated clock provider nodes would not be initialized again. > Also of_clk_init() is for the CLK_OF_DECLARE() users and if they can wait to platform bus population time then they could be converted to regular old platform device drivers. Maybe the problem is the clk driver you're using is only using CLK_OF_DECLARE() and not registering a platform driver?
On 2020/11/13 17:11, Stephen Boyd wrote: > Quoting Damien Le Moal (2020-11-12 23:57:19) >> On 2020/11/13 16:53, Stephen Boyd wrote: >>> >>> Yes it's fine. Just the commit text reads as "If of_clk_init() is not >>> called in time_init() then nothing works" which is true but made me >>> wonder if it was because it needed to be super early or not. The commit >>> text could be a little clearer here. >> >> OK. I will clarify the commit message in V2. Working on it now. > > Thanks! > >> >>> We don't have any good solution for a fallback to call of_clk_init() >>> somewhere later. I do wonder if we should generalize this though and >>> call of_clk_init() from start_kernel() directly via some Kconfig that >>> architectures select if they need it for their timer and then move it to >>> an initcall if architectures don't select the config. Or throw it into >>> the of_platform_default_populate_init() hook if the architecture doesn't >>> need to call it early. >> >> This last idea seems reasonable and probably the easiest. And I think it could >> be done unconditionally even if the arch calls of_clk_init() early as the >> already populated clock provider nodes would not be initialized again. >> > > Also of_clk_init() is for the CLK_OF_DECLARE() users and if they can > wait to platform bus population time then they could be converted to > regular old platform device drivers. Maybe the problem is the clk driver > you're using is only using CLK_OF_DECLARE() and not registering a > platform driver? Yep, correct, that is what I did. SO yes, indeed, if I where to use a regular platform_driver, then the of_clk_init() change would not be needed. For the clock driver, I followed the pattern used by most other drivers with the majority I think using CLK_OF_DECLARE(). I did see some using the platform driver declaration though. I could spend time trying to figure out if I can get away without using CLK_OF_DECLARE(), but since I think other riscv board clock driver are arriving, we may as well keep that of_clk_init() change, no ?
Quoting Damien Le Moal (2020-11-13 00:23:52) > On 2020/11/13 17:11, Stephen Boyd wrote: > > > > Also of_clk_init() is for the CLK_OF_DECLARE() users and if they can > > wait to platform bus population time then they could be converted to > > regular old platform device drivers. Maybe the problem is the clk driver > > you're using is only using CLK_OF_DECLARE() and not registering a > > platform driver? > > Yep, correct, that is what I did. SO yes, indeed, if I where to use a regular > platform_driver, then the of_clk_init() change would not be needed. > > For the clock driver, I followed the pattern used by most other drivers with the > majority I think using CLK_OF_DECLARE(). I did see some using the platform > driver declaration though. > > I could spend time trying to figure out if I can get away without using > CLK_OF_DECLARE(), but since I think other riscv board clock driver are arriving, > we may as well keep that of_clk_init() change, no ? > Sure keeping of_clk_init() in riscv architecture code is fine. It would be good to use a platform driver though and avoid the use of CLK_OF_DECLARE() so we don't burden ourselves with more code that registers clks early unnecessarily.
On Mon, Nov 16, 2020 at 07:30:15AM +0000, Damien Le Moal wrote: > On 2020/11/10 2:45, Serge Semin wrote: > > Hello Andy, > > > > On Mon, Nov 09, 2020 at 05:14:21PM +0200, Andy Shevchenko wrote: > >> On Sat, Nov 7, 2020 at 10:14 AM Damien Le Moal <damien.lemoal@wdc.com> wrote: > >> > >>> @@ -1308,7 +1308,6 @@ DEFINE_SIMPLE_PROP(pinctrl7, "pinctrl-7", NULL) > >>> DEFINE_SIMPLE_PROP(pinctrl8, "pinctrl-8", NULL) > >>> DEFINE_SUFFIX_PROP(regulators, "-supply", NULL) > >>> DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells") > >>> -DEFINE_SUFFIX_PROP(gpios, "-gpios", "#gpio-cells") > >> > >> Sorry, but the above doesn't sound right to me. > >> It's a generic code and you may imagine how many systems you broke by > >> this change. > > > > Damien replaced the macro above with the code below (your removed it from your > > message): > > > > +static struct device_node *parse_gpios(struct device_node *np, > > + const char *prop_name, int index) > > +{ > > + /* > > + * Quirck for the DesignWare gpio-dwapb GPIO driver which defines > > + * the "snps,nr-gpios" property to indicate the total number of GPIOs > > + * available. As this conflict with "xx-gpios" reference properties, > > + * ignore it. > > + */ > > + if (strcmp(prop_name, "snps,nr-gpios") == 0) > > + return NULL; > > + > > + return parse_suffix_prop_cells(np, prop_name, index, > > + "-gpios", "#gpio-cells"); > > +} > > > > So AFAICS removing the macro shouldn't cause any problem. > > > > My concern was whether the quirk has been really needed. As I said the > > "snps,nr-gpios" property has been marked as deprecated in favor of the standard > > "ngpios" one. Due to the problem noted by Damien any deprecated property > > utilization will cause the DW APB SSI DT-nodes probe malfunction. That > > though implicitly but is supposed to encourage people to provide fixes for > > the dts-files with the deprecated property replaced with "ngpios". > > > > On the other hand an encouragement based on breaking the kernel doesn't seem a > > good solution. So as I see it either we should accept the solution provided by > > Damien, or replace it with a series of fixes for all dts-es with DW APB SSI > > DT-node defined. I suggest to hear the OF-subsystem maintainers out what > > solution would they prefer. > > As Rob mentioned, there are still a lot of DTS out there using "snps,nr-gpios", > so I think the fix is needed, Yes. > albeit with an added warning as Rob suggested so > that board maintainers can notice and update their DT. Yes. > And I can send a patch > for the DW gpio apb driver to first try the default "ngpios" property, and if it > is not defined, fallback to the legacy "snps,nr-gpios". With that, these new > RISC-V boards will not add another use case of the deprecated "snsps,nr-gpios". > Does that sound like a good plan ? It has already been added in 5.10: https://elixir.bootlin.com/linux/v5.10-rc4/source/drivers/gpio/gpio-dwapb.c#L585 so there is no need in sending a patch for the gpio-dwapb.c driver. -Sergey > > > > > > -Sergey > > > >> > >> -- > >> With Best Regards, > >> Andy Shevchenko > > > > > -- > Damien Le Moal > Western Digital Research
Hi Damien, Sean, On Mon, Nov 9, 2020 at 4:46 PM Sean Anderson <seanga2@gmail.com> wrote: > On 11/9/20 10:36 AM, Rob Herring wrote: > > On Sat, Nov 07, 2020 at 05:14:12PM +0900, Damien Le Moal wrote: > >> Document the device tree bindings for the Kendryte K210 SoC Fully > >> Programmable IO Array (FPIOA) pinctrl driver in > >> Documentation/devicetree/bindings/pinctrl/kendryte,k210-fpioa.yaml > >> > >> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/pinctrl/kendryte,k210-fpioa.yaml > >> + kendryte,power-offset: > >> + minItems: 1 > >> + maxItems: 1 > >> + $ref: /schemas/types.yaml#/definitions/uint32 > >> + description: | > >> + Offset of the power domain control register of the system controller. > > > > Sounds like you should be using power-domains binding. > > This is for pin power domains. E.g. pins 0-5 can be set to 1V8 or 3V3 logic levels. Which brings to my attention the power-source property is not documented below... > >> + The value should be the macro K210_SYSCTL_POWER_SEL defined in > >> + dt-bindings/mfd/k210-sysctl.h. > >> + > >> +patternProperties: > >> + '^.*$': > >> + if: > >> + type: object > >> + then: As the driver supports e.g. bias and drive-strength, these should be documented here, too. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds