| Message ID | 20220801094255.664548-2-nagasuresh.relli@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for Microchip QSPI controller | expand |
Hey Suresh, On 01/08/2022 10:42, Naga Sureshkumar Relli wrote: > Add compatible string for Microchip CoreQSPI controller. > > Signed-off-by: Naga Sureshkumar Relli <nagasuresh.relli@microchip.com> > --- > .../devicetree/bindings/spi/microchip,mpfs-spi.yaml | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml > index 7326c0a28d16..b65f4e070796 100644 > --- a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml > +++ b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml > @@ -14,9 +14,13 @@ allOf: > > properties: > compatible: > - enum: > - - microchip,mpfs-spi > - - microchip,mpfs-qspi > + oneOf: > + - description: Microchip's Polarfire SoC SPI controller. ^ (This is a capital F btw) > + const: microchip,mpfs-spi > + - description: Microchip's Polarfire SoC QSPI controller. > + const: microchip,mpfs-qspi > + - description: Microchip's FPGA QSPI controller. > + const: microchip,coreqspi-rtl-v2 I am not sure that this is the correct "hierarchy". coreQSPI has a subset of the registers of the "hard" QSPI & the same driver works for both at the moment. The "hard" QSPI is based on the FPGA core, so I think this should be changed to something like the following: properties: compatible: oneOf: - description: Microchip's PolarFire SoC QSPI controller items: - const: microchip,mpfs-qspi - const: microchip,coreqspi-rtl-v2 - description: Microchip's fabric based QSPI IP core const: microchip,coreqspi-rtl-v2 - description: Microchip's PolarFire SoC SPI controller const: microchip,mpfs-spi Unrelated to this patch, but a diff --git a/MAINTAINERS b/MAINTAINERS index f4202a19faa1..887bfee5c7af 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -17526,6 +17526,7 @@ F: drivers/pci/controller/pcie-microchip-host.c F: drivers/rtc/rtc-mpfs.c F: drivers/soc/microchip/ F: drivers/spi/spi-microchip-core.c +F: drivers/spi/spi-microchip-core-qspi.c F: drivers/usb/musb/mpfs.c F: include/soc/microchip/mpfs.h Would be nice too. Thanks, Conor.
Hi Conor, Thanks for your review. On Mon, Aug 1, 2022 at 3:54 PM <Conor.Dooley@microchip.com> wrote: > > Hey Suresh, > > On 01/08/2022 10:42, Naga Sureshkumar Relli wrote: > > Add compatible string for Microchip CoreQSPI controller. > > > > Signed-off-by: Naga Sureshkumar Relli <nagasuresh.relli@microchip.com> > > --- > > .../devicetree/bindings/spi/microchip,mpfs-spi.yaml | 10 +++++++--- > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml > > index 7326c0a28d16..b65f4e070796 100644 > > --- a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml > > +++ b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml > > @@ -14,9 +14,13 @@ allOf: > > > > properties: > > compatible: > > - enum: > > - - microchip,mpfs-spi > > - - microchip,mpfs-qspi > > + oneOf: > > + - description: Microchip's Polarfire SoC SPI controller. > ^ > (This is a capital F btw) > > > + const: microchip,mpfs-spi > > + - description: Microchip's Polarfire SoC QSPI controller. > > + const: microchip,mpfs-qspi > > + - description: Microchip's FPGA QSPI controller. > > + const: microchip,coreqspi-rtl-v2 > > I am not sure that this is the correct "hierarchy". coreQSPI has a > subset of the registers of the "hard" QSPI & the same driver works > for both at the moment. The "hard" QSPI is based on the FPGA core, > so I think this should be changed to something like the following: I have added each element for each controller separately. but the below one hierarchy explains clearly about the cores. I will update the bindings. > > properties: > compatible: > oneOf: > - description: Microchip's PolarFire SoC QSPI controller > items: > - const: microchip,mpfs-qspi > - const: microchip,coreqspi-rtl-v2 > - description: Microchip's fabric based QSPI IP core > const: microchip,coreqspi-rtl-v2 > - description: Microchip's PolarFire SoC SPI controller > const: microchip,mpfs-spi > > Unrelated to this patch, but a > > diff --git a/MAINTAINERS b/MAINTAINERS > index f4202a19faa1..887bfee5c7af 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -17526,6 +17526,7 @@ F: drivers/pci/controller/pcie-microchip-host.c > F: drivers/rtc/rtc-mpfs.c > F: drivers/soc/microchip/ > F: drivers/spi/spi-microchip-core.c > +F: drivers/spi/spi-microchip-core-qspi.c > F: drivers/usb/musb/mpfs.c > F: include/soc/microchip/mpfs.h > > Would be nice too. Ok. Thanks, Naga Sureshkumar Relli. > > Thanks, > Conor.
On 01/08/2022 11:42, Naga Sureshkumar Relli wrote: > Add compatible string for Microchip CoreQSPI controller. > > Signed-off-by: Naga Sureshkumar Relli <nagasuresh.relli@microchip.com> > --- > .../devicetree/bindings/spi/microchip,mpfs-spi.yaml | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml > index 7326c0a28d16..b65f4e070796 100644 > --- a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml > +++ b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml > @@ -14,9 +14,13 @@ allOf: > > properties: > compatible: > - enum: > - - microchip,mpfs-spi > - - microchip,mpfs-qspi > + oneOf: > + - description: Microchip's Polarfire SoC SPI controller. > + const: microchip,mpfs-spi > + - description: Microchip's Polarfire SoC QSPI controller. Useless descriptions - they repeat compatible. Just keep it as enum and skip descriptions. What value do they bring? > + const: microchip,mpfs-qspi > + - description: Microchip's FPGA QSPI controller. > + const: microchip,coreqspi-rtl-v2 Best regards, Krzysztof
On Tue, Aug 02, 2022 at 10:52:25AM +0200, Krzysztof Kozlowski wrote: > On 01/08/2022 11:42, Naga Sureshkumar Relli wrote: > > - enum: > > - - microchip,mpfs-spi > > - - microchip,mpfs-qspi > > + oneOf: > > + - description: Microchip's Polarfire SoC SPI controller. > > + const: microchip,mpfs-spi > > + - description: Microchip's Polarfire SoC QSPI controller. > Useless descriptions - they repeat compatible. Just keep it as enum and > skip descriptions. What value do they bring? Someone not familiar with the full Microchip product line might not be aware of the expansion of mpfs, it's not blindingly obvious.
On 02/08/2022 15:13, Mark Brown wrote: > On Tue, Aug 02, 2022 at 10:52:25AM +0200, Krzysztof Kozlowski wrote: >> On 01/08/2022 11:42, Naga Sureshkumar Relli wrote: > >>> - enum: >>> - - microchip,mpfs-spi >>> - - microchip,mpfs-qspi >>> + oneOf: >>> + - description: Microchip's Polarfire SoC SPI controller. >>> + const: microchip,mpfs-spi >>> + - description: Microchip's Polarfire SoC QSPI controller. > >> Useless descriptions - they repeat compatible. Just keep it as enum and >> skip descriptions. What value do they bring? > > Someone not familiar with the full Microchip product line might not be > aware of the expansion of mpfs, it's not blindingly obvious. Then it should be explained in title/description of the binding, not in compatible. This is the usual way of providing some text description, not for each compatible by repeating the compatible text. Best regards, Krzysztof
Hi Krzysztof, On Wed, Aug 3, 2022 at 11:42 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 02/08/2022 15:13, Mark Brown wrote: > > On Tue, Aug 02, 2022 at 10:52:25AM +0200, Krzysztof Kozlowski wrote: > >> On 01/08/2022 11:42, Naga Sureshkumar Relli wrote: > > > >>> - enum: > >>> - - microchip,mpfs-spi > >>> - - microchip,mpfs-qspi > >>> + oneOf: > >>> + - description: Microchip's Polarfire SoC SPI controller. > >>> + const: microchip,mpfs-spi > >>> + - description: Microchip's Polarfire SoC QSPI controller. > > > >> Useless descriptions - they repeat compatible. Just keep it as enum and > >> skip descriptions. What value do they bring? > > > > Someone not familiar with the full Microchip product line might not be > > aware of the expansion of mpfs, it's not blindingly obvious. > > Then it should be explained in title/description of the binding, not in > compatible. This is the usual way of providing some text description, > not for each compatible by repeating the compatible text. Ok. In the next version I will update the bindings like below -title: Microchip MPFS {Q,}SPI Controller Device Tree Bindings +title: Microchip FPGA {Q,}SPI Controllers + +description: + SPI and QSPI controllers on the Microchip PolarFire SoC and they are based + on the "soft" fabric IP cores. oneOf: - items: + - const: microchip,mpfs-qspi + - const: microchip,coreqspi-rtl-v2 + - const: microchip,coreqspi-rtl-v2 - const: microchip,mpfs-spi Is that ok? Thanks, Naga Sureshkumar Relli. > > Best regards, > Krzysztof
On 03/08/2022 08:59, naga sureshkumar wrote: > Hi Krzysztof, > > On Wed, Aug 3, 2022 at 11:42 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 02/08/2022 15:13, Mark Brown wrote: >>> On Tue, Aug 02, 2022 at 10:52:25AM +0200, Krzysztof Kozlowski wrote: >>>> On 01/08/2022 11:42, Naga Sureshkumar Relli wrote: >>> >>>>> - enum: >>>>> - - microchip,mpfs-spi >>>>> - - microchip,mpfs-qspi >>>>> + oneOf: >>>>> + - description: Microchip's Polarfire SoC SPI controller. >>>>> + const: microchip,mpfs-spi >>>>> + - description: Microchip's Polarfire SoC QSPI controller. >>> >>>> Useless descriptions - they repeat compatible. Just keep it as enum and >>>> skip descriptions. What value do they bring? >>> >>> Someone not familiar with the full Microchip product line might not be >>> aware of the expansion of mpfs, it's not blindingly obvious. >> >> Then it should be explained in title/description of the binding, not in >> compatible. This is the usual way of providing some text description, >> not for each compatible by repeating the compatible text. > Ok. In the next version I will update the bindings like below > > -title: Microchip MPFS {Q,}SPI Controller Device Tree Bindings > +title: Microchip FPGA {Q,}SPI Controllers > + > +description: > + SPI and QSPI controllers on the Microchip PolarFire SoC and they are based > + on the "soft" fabric IP cores. > oneOf: > - items: > + - const: microchip,mpfs-qspi > + - const: microchip,coreqspi-rtl-v2 This piece should have its own explanation and preferably in its own commit. As I mentioned before, you change existing compatible so it should be explained. Best regards, Krzysztof
On Wed, Aug 03, 2022 at 08:11:03AM +0200, Krzysztof Kozlowski wrote: > On 02/08/2022 15:13, Mark Brown wrote: > > On Tue, Aug 02, 2022 at 10:52:25AM +0200, Krzysztof Kozlowski wrote: > >> On 01/08/2022 11:42, Naga Sureshkumar Relli wrote: > >>> + oneOf: > >>> + - description: Microchip's Polarfire SoC SPI controller. > >>> + const: microchip,mpfs-spi > >>> + - description: Microchip's Polarfire SoC QSPI controller. > >> Useless descriptions - they repeat compatible. Just keep it as enum and > >> skip descriptions. What value do they bring? > > Someone not familiar with the full Microchip product line might not be > > aware of the expansion of mpfs, it's not blindingly obvious. > Then it should be explained in title/description of the binding, not in > compatible. This is the usual way of providing some text description, > not for each compatible by repeating the compatible text. I'm not convinced this is a useful rule to try to enforce, and I'm not sure how well it will work if the same IP is used in several different places. It's not clear to me what the benefit is intended to be.
On 03/08/2022 15:29, Mark Brown wrote: > On Wed, Aug 03, 2022 at 08:11:03AM +0200, Krzysztof Kozlowski wrote: >> On 02/08/2022 15:13, Mark Brown wrote: >>> On Tue, Aug 02, 2022 at 10:52:25AM +0200, Krzysztof Kozlowski wrote: >>>> On 01/08/2022 11:42, Naga Sureshkumar Relli wrote: > >>>>> + oneOf: >>>>> + - description: Microchip's Polarfire SoC SPI controller. >>>>> + const: microchip,mpfs-spi >>>>> + - description: Microchip's Polarfire SoC QSPI controller. > >>>> Useless descriptions - they repeat compatible. Just keep it as enum and >>>> skip descriptions. What value do they bring? > >>> Someone not familiar with the full Microchip product line might not be >>> aware of the expansion of mpfs, it's not blindingly obvious. > >> Then it should be explained in title/description of the binding, not in >> compatible. This is the usual way of providing some text description, >> not for each compatible by repeating the compatible text. > > I'm not convinced this is a useful rule to try to enforce, and I'm not > sure how well it will work if the same IP is used in several different > places. It's not clear to me what the benefit is intended to be. First, the description here is really not adding any useful information. "description: Microchip's Polarfire SoC SPI controller." Microchip - already in comaptible SPI controller - already in compatible and in device description The only useful piece could be extending pfs to Polarfire SoC. And now imagine every binding doing the same, adding such acronym-explanations in every compatible list. Basically we loose easy to read, compare, analyze and check for errors enum: enum - microchip,mpfs-spi - microchip,mpfs-qspi - microchip,coreqspi-rtl-v2 - microchip,mpfs-some-more-spi - microchip,mpfs-even-newer-spi into double-sized oneOf with additional descriptions each one explaining "mpfs". oneOf: - description: Microchip's Polarfire SoC SPI controller. const: microchip,mpfs-spi - description: Microchip's Polarfire SoC QSPI controller. const: microchip,mpfs-qspi - description: Microchip's FPGA QSPI controller. const: microchip,coreqspi-rtl-v2 - description: Microchip's Polarfire SoC some-more SPI controller. const: microchip,mpfs-some-more-spi - description: Microchip's Polarfire SoC even newer SPI controller. const: microchip,mpfs-even-newer-spi Why do you need to explain "mpfs" more than once? Why explaining these are SPI or QSPI controllers? It's obvious from compatible. Just keep it simple and small. We all have too much code to look at... Best regards, Krzysztof
On 04/08/2022 13:31, Krzysztof Kozlowski wrote: >> I'm not convinced this is a useful rule to try to enforce, and I'm not >> sure how well it will work if the same IP is used in several different >> places. It's not clear to me what the benefit is intended to be. > > First, the description here is really not adding any useful information. > > "description: Microchip's Polarfire SoC SPI controller." > Microchip - already in comaptible > SPI controller - already in compatible and in device description > > The only useful piece could be extending pfs to Polarfire SoC. > > And now imagine every binding doing the same, adding such > acronym-explanations in every compatible list. Basically we loose easy > to read, compare, analyze and check for errors enum: > enum > - microchip,mpfs-spi > - microchip,mpfs-qspi > - microchip,coreqspi-rtl-v2 > - microchip,mpfs-some-more-spi > - microchip,mpfs-even-newer-spi > > into double-sized oneOf with additional descriptions each one explaining > "mpfs". > > oneOf: > - description: Microchip's Polarfire SoC SPI controller. > const: microchip,mpfs-spi > - description: Microchip's Polarfire SoC QSPI controller. > const: microchip,mpfs-qspi > - description: Microchip's FPGA QSPI controller. > const: microchip,coreqspi-rtl-v2 Just to be more specific - this one description actually brings useful information (FPGA)... This can be easily added as a comment, if anyone finds it useful: enum - microchip,mpfs-spi - microchip,mpfs-qspi - microchip,coreqspi-rtl-v2 # FPGA QSPI Best regards, Krzysztof
On Thu, Aug 04, 2022 at 01:31:34PM +0200, Krzysztof Kozlowski wrote: > On 03/08/2022 15:29, Mark Brown wrote: > > On Wed, Aug 03, 2022 at 08:11:03AM +0200, Krzysztof Kozlowski wrote: > >> Then it should be explained in title/description of the binding, not in > >> compatible. This is the usual way of providing some text description, > >> not for each compatible by repeating the compatible text. > > I'm not convinced this is a useful rule to try to enforce, and I'm not > > sure how well it will work if the same IP is used in several different > > places. It's not clear to me what the benefit is intended to be. > First, the description here is really not adding any useful information. > "description: Microchip's Polarfire SoC SPI controller." > Microchip - already in comaptible > SPI controller - already in compatible and in device description > The only useful piece could be extending pfs to Polarfire SoC. That was the main bit I was thinking of TBH. > into double-sized oneOf with additional descriptions each one explaining > "mpfs". > oneOf: > - description: Microchip's Polarfire SoC SPI controller. > const: microchip,mpfs-spi If the YAML binding format is hard to read when people add comments that does seem like something that should be addressed on the format side rather than the binding side TBH. > Just keep it simple and small. We all have too much code to look at... Excessively clear documentation hasn't typically been our issue.
diff --git a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml index 7326c0a28d16..b65f4e070796 100644 --- a/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml +++ b/Documentation/devicetree/bindings/spi/microchip,mpfs-spi.yaml @@ -14,9 +14,13 @@ allOf: properties: compatible: - enum: - - microchip,mpfs-spi - - microchip,mpfs-qspi + oneOf: + - description: Microchip's Polarfire SoC SPI controller. + const: microchip,mpfs-spi + - description: Microchip's Polarfire SoC QSPI controller. + const: microchip,mpfs-qspi + - description: Microchip's FPGA QSPI controller. + const: microchip,coreqspi-rtl-v2 reg: maxItems: 1
Add compatible string for Microchip CoreQSPI controller. Signed-off-by: Naga Sureshkumar Relli <nagasuresh.relli@microchip.com> --- .../devicetree/bindings/spi/microchip,mpfs-spi.yaml | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)