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

[Witold Sadowski]

> 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@xxxxxxxxxxx>
> > > > ---
> > > >  .../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?

Yes, by HW overlay I meant actual HW modification. I called it overlay,
as it is not modifying xSPI block itself, but it is rather build around
it. As I don't have better word for it now, I will continue to use it.
With that approach we can still have full functionality of xSPI block
(like memory transfers), and additional features(like SPI full-duplex
operations).
Regarding "internal SoC structure" - I meant physical parameters of silicon,
Signal propagation times inside block etc. That won't be changed if we have
Two different SoC, bud made in same technology. 

> 
> 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.

We can have SoC A, B with common xSPI block, and both of them can share
Same dtb node with compatible property "marvell,cn10k,xspi-nor" for
example. I don't think it will be beneficial to have different compatible
name for each different SoC, for example "marvell,t98,xspi-nor", if all
other parts will be the same. Or am I not correct?

> 
> Cheers,
> Conor.

Regards
Witek