Re: [PATCH v7 1/9] dt-bindings: rockchip-sfc: Bindings for Rockchip serial flash controller

[Ezequiel Garcia <ezequiel@xxxxxxxxxxxxxxxxxxxx>]

Hi all,

On Thu, 10 Jun 2021 at 00:04, Kever Yang <kever.yang@xxxxxxxxxxxxxx> wrote:
>
> Hi Rob,
>
> On 2021/6/10 上午10:43, Rob Herring wrote:
> > On Wed, Jun 09, 2021 at 10:04:04PM +0800, Jon Lin wrote:
> >> From: Chris Morgan <macromorgan@xxxxxxxxxxx>
> >>
> >> Add bindings for the Rockchip serial flash controller. New device
> >> specific parameter of rockchip,sfc-no-dma included in documentation.
> >>
> >> Signed-off-by: Chris Morgan <macromorgan@xxxxxxxxxxx>
> >> Signed-off-by: Jon Lin <jon.lin@xxxxxxxxxxxxxx>
> >> ---
> >>
> >> Changes in v7:
> >> - Fix up the sclk_sfc parent error in rk3036
> >> - Unify to "rockchip,sfc" compatible id because all the feature update
> >>    will have a new IP version, so the driver is used for the SFC IP in
> >>    all SoCs
> >> - Change to use node "sfc" to name the SFC pinctrl group
> >> - Add subnode reg property check
> >> - Add rockchip_sfc_adjust_op_size to workaround in CMD + DUMMY case
> >> - Limit max_iosize to 32KB
> >>
> >> Changes in v6:
> >> - Add support in device trees for rv1126(Declared in series 5 but not
> >>    submitted)
> >> - Change to use "clk_sfc" "hclk_sfc" as clock lable, since it does not
> >>    affect interpretation and has been widely used
> >> - Support sfc tx_dual, tx_quad(Declared in series 5 but not submitted)
> >> - Simplify the code, such as remove "rockchip_sfc_register_all"(Declared
> >>    in series 5 but not submitted)
> >> - Support SFC ver4 ver5(Declared in series 5 but not submitted)
> >> - Add author Chris Morgan and Jon Lin to spi-rockchip-sfc.c
> >> - Change to use devm_spi_alloc_master and spi_unregister_master
> >>
> >> Changes in v5:
> >> - Add support in device trees for rv1126
> >> - Support sfc tx_dual, tx_quad
> >> - Simplify the code, such as remove "rockchip_sfc_register_all"
> >> - Support SFC ver4 ver5
> >>
> >> Changes in v4:
> >> - Changing patch back to an "RFC". An engineer from Rockchip
> >>    reached out to me to let me know they are working on this patch for
> >>    upstream, I am submitting this v4 for the community to see however
> >>    I expect Jon Lin (jon.lin@xxxxxxxxxxxxxx) will submit new patches
> >>    soon and these are the ones we should pursue for mainlining. Jon's
> >>    patch series should include support for more hardware than this
> >>    series.
> >> - Clean up documentation more and ensure it is correct per
> >>    make dt_binding_check.
> >> - Add support in device trees for rk3036, rk3308, and rv1108.
> >> - Add ahb clock (hclk_sfc) support for rk3036.
> >> - Change rockchip_sfc_wait_fifo_ready() to use a switch statement.
> >> - Change IRQ code to only mark IRQ as handled if it handles the
> >>    specific IRQ (DMA transfer finish) it is supposed to handle.
> >>
> >> Changes in v3:
> >> - Changed the name of the clocks to sfc/ahb (from clk-sfc/clk-hsfc).
> >> - Changed the compatible string from rockchip,sfc to
> >>    rockchip,rk3036-sfc. A quick glance at the datasheets suggests this
> >>    driver should work for the PX30, RK180x, RK3036, RK312x, RK3308 and
> >>    RV1108 SoCs, and possibly more. However, I am currently only able
> >>    to test this on a PX30 (an RK3326). The technical reference manuals
> >>    appear to list the same registers for each device.
> >> - Corrected devicetree documentation for formatting and to note these
> >>    changes.
> >> - Replaced the maintainer with Heiko Stuebner and myself, as we will
> >>    take ownership of this going forward.
> >> - Noted that the device (per the reference manual) supports 4 CS, but
> >>    I am only able to test a single CS (CS 0).
> >> - Reordered patches to comply with upstream rules.
> >>
> >> Changes in v2:
> >> - Reimplemented driver using spi-mem subsystem.
> >> - Removed power management code as I couldn't get it working properly.
> >> - Added device tree bindings for Odroid Go Advance.
> >>
> >> Changes in v1:
> >> hanges made in this new series versus the v8 of the old series:
> >> - Added function to read spi-rx-bus-width from device tree, in the
> >>    event that the SPI chip supports 4x mode but only has 2 pins
> >>    wired (such as the Odroid Go Advance).
> >> - Changed device tree documentation from txt to yaml format.
> >> - Made "reset" message a dev_dbg from a dev_info.
> >> - Changed read and write fifo functions to remove redundant checks.
> >> - Changed the write and read from relaxed to non-relaxed when
> >>    starting the DMA transfer or reading the DMA IRQ.
> >> - Changed from dma_coerce_mask_and_coherent to just
> >>    dma_set_mask_and_coherent.
> >> - Changed name of get_if_type to rockchip_sfc_get_if_type.
> >>
> >>   .../devicetree/bindings/spi/rockchip-sfc.yaml | 88 +++++++++++++++++++
> >>   1 file changed, 88 insertions(+)
> >>   create mode 100644 Documentation/devicetree/bindings/spi/rockchip-sfc.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/spi/rockchip-sfc.yaml b/Documentation/devicetree/bindings/spi/rockchip-sfc.yaml
> >> new file mode 100644
> >> index 000000000000..42e4198e92af
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/spi/rockchip-sfc.yaml
> >> @@ -0,0 +1,88 @@
> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/spi/rockchip-sfc.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Rockchip Serial Flash Controller (SFC)
> >> +
> >> +maintainers:
> >> +  - Heiko Stuebner <heiko@xxxxxxxxx>
> >> +  - Chris Morgan <macromorgan@xxxxxxxxxxx>
> >> +
> >> +allOf:
> >> +  - $ref: spi-controller.yaml#
> >> +
> >> +properties:
> >> +  compatible:
> >> +    oneOf:
> >> +      - const: rockchip,sfc
> > Use 'enum' instead of oneOf+const.
> >
> > You need an SoC specific compatible.
>
>
> The rockchip sfc controller is a standalone IP with version register,
> and the driver can
>
> handle all the feature difference inside the IP, so we would like to use
> a more generic
>
> compatible name instead of bind to any of SoC name. So can we use
> "rockchip,sfc"
>
> like "snps,designware-spi", which is a generic one, instead of an SoC
> specific compatible?
>

IIUC, the way this works is along these lines:

* The SFC driver can only care for the rockchip,sfc compatible string
and, if suitable, use the IP version register mentioned by Kever [1].

* The bindings doc specifies both the SoC-specific and the generic one
with:

      - items:
          - enum:
              - rockchip,px30-sfc
          - const: rockchip,sfc

* The device tree lists both as well:

compatible = "rockchip,px30-sfc", "rockchip,sfc";

This can apply to all IP cores really; and will allow some
compatibility between the downstream/vendor device tree
and upstream.

This scheme is indeed more convoluted than just
picking any SoC name for the compatible string, and
use that compatible string for all the SoCs (given they
are all compatible, again as per [1]).

IOW, you only have "rockchip,px30-sfc" in the bindings,
in the devicetree files and in the driver.

[1] https://lkml.org/lkml/2021/6/8/2030

Thanks!
Ezequiel