Re: [EXTERNAL] Re: [PATCH v3 2/5] spi: cadence: Add MRVL overlay bindings documentation for Cadence XSPI

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3905 bytes --]

On Mon, Apr 29, 2024 at 02:47:23PM +0000, Witold Sadowski wrote:
> > ----------------------------------------------------------------------
> > On Wed, Apr 17, 2024 at 06:13:49PM -0700, Witold Sadowski wrote:
> > > Add new bindings for v2 Marvell xSPI overlay:
> > > mrvl,xspi-nor  compatible string
> > > New compatible string to distinguish between orginal and modified xSPI
> > > block
> > >
> > > PHY configuration registers
> > > Allow to change orginal xSPI PHY configuration values. If not set, and
> > > Marvell overlay is enabled, safe defaults will be written into xSPI
> > > PHY
> > >
> > > Optional base for xfer register set
> > > Additional reg field to allocate xSPI Marvell overlay XFER block
> > >
> > > Signed-off-by: Witold Sadowski <wsadowski@marvell.com>
> > > ---
> > >  .../devicetree/bindings/spi/cdns,xspi.yaml    | 92 ++++++++++++++++++-
> > >  1 file changed, 91 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/spi/cdns,xspi.yaml
> > > b/Documentation/devicetree/bindings/spi/cdns,xspi.yaml
> > > index eb0f92468185..0e608245b136 100644
> > > --- a/Documentation/devicetree/bindings/spi/cdns,xspi.yaml
> > > +++ b/Documentation/devicetree/bindings/spi/cdns,xspi.yaml
> > > @@ -20,23 +20,82 @@ allOf:
> > >
> > >  properties:
> > >    compatible:
> > > -    const: cdns,xspi-nor
> > > +    oneOf:
> > > +      - description: Vanilla Cadence xSPI controller
> > > +        items:
> > > +          - const: cdns,xspi-nor
> > 
> > The "items: isn't required here is it? Can't you just have
> >     oneOf:
> >       - description: Vanilla Cadence xSPI controller
> >         const: cdns,xspi-nor
> >       - description: Cadence xSPI controller with v2 Marvell overlay
> >         const: mrvl,xspi-nor
> > if you don't want to use an enum?
> 
> It works without items, but I will try also with enums.
> 
> > 
> > > +      - description: Cadence xSPI controller with v2 Marvell overlay
> > > +        items:
> > > +          - const: mrvl,xspi-nor
> > 
> > 
> > "mrvl" is deprecated, please use "marvell". You're also missing a soc-
> > specific compatible here, I doubt there's only going to be one device from
> > marvell with an xspi controller ever.
> 
> The intention is to add overlay on top of existing IP block to gain some
> More features from it. So if there will be different SoC with same xSPI
> IP, we can simply use that property, as internal SoC structure will be the same.
> On the other hand, if there will be used different IP to handle SPI operations
> It should use different driver. Also, I do not expect that new version of the
> Overlay will be developed to handle different IP.

I'm struggling to understand what you mean here by "overlay". Ordinarily
I'd expect someone to meant a dt-overlay, but you're talking about IP
blocks, so this sounds like hardware modifications.
I am also a bit confused by the claim that the "internal SoC structure
will be the same". Usually different SoCs have different internal
structures, even when they re-use IP cores. If they have the same internal
structure then they're not really different SoCs, just different
packages! I think what you're saying here is that you intend using the
"mrvl,xspi-nor" compatible for multiple SoCs that all contain the same
modified versions of the Cadence IP, not different packages for the same
SoC?

Confusing wording aside, using the same generic compatible for different
SoCs is what I trying to avoid. I don't mind there being a fallback
compatible that's generic, but I want to see specific compatibles here
for the individual SoCs.

If you did actually mean that only the packaging is different between
the devices, then I don't think you need specific compatibles for each
different package, but you should have one for the SoC itself IMO.

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]