Re: [PATCH v3 01/10] dt-bindings: mtd: brcmnand: Updates for bcmbca SoCs

[Conor Dooley <conor@kernel.org>]

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

On Thu, Jan 25, 2024 at 05:55:29PM -0800, William Zhang wrote:
> On 1/25/24 11:51, Conor Dooley wrote:
> > On Wed, Jan 24, 2024 at 07:01:48PM -0800, William Zhang wrote:
> > > On 1/24/24 09:24, Conor Dooley wrote:
> > > > On Tue, Jan 23, 2024 at 07:04:49PM -0800, David Regan wrote:
> > > > > From: William Zhang <william.zhang@broadcom.com>

> > > And the reason I keep it as int is because there could be a potential value
> > > of 2 for another mode that the current driver could set the wp_on to.
> > 
> > Can you explain, in driver independent terms, what this other mode has
> > to do with how the WP is connected?
> > Either you've got the standard scenario where apparently "NAND_WPb" and
> > "WP_L" are connected and another situation where they are not.
> > Is there another pin configuration in addition to that, that this 3rd
> > mode represents?
> > 
> The 3rd mode is WP pin connected but wp feature is disabled in the
> controller.

What does "disabled" mean? The controller itself is not capable of using
the WP pin because of hardware modifications? Or there's a bit in a
register that disables it and that bit has been set by software?

If it is a hardware difference for that controller, why does it not have
a dedicated compatible? If it's a software decision, then it shouldn't
be in the DT in the first place ;)

We've gone back here a bunch trying to figure stuff out, and you give me
a vague one sentence answer. Please.

> > > I
> > > don't see it is being used in BCMBCA product but I am not sure about other
> > > SoC family. So I don't want to completely close the door here just in case.
> > 
> > If you ever need it, you could have a "brcm,wp-in-wacky-configuration"
> > property that indicates that the hardware is configured in that way
> > (providing that this hardware configuration is not just "NAND_WPb" and
> > "WP_L" pins connected. The property can very easily be made mutually
> > exclusive with "brcm,wp-not-connected" iff it ever comes to be.
> Yes we could have a new property but if we can have a single int property to
> cover all, IMHO it is better to just have one property as these three modes
> are related. I would really like to have it as an int property to keep
> things simple.

It is "better" because it is easier for you perhaps, but ripe for abuse.

> But if you think this is absolutely against the dts rule,  I will switch to
> the "brcm,wp-not-connected" boolean property as you suggested.

I'll answer this when you describe the mode better, right now I cannot
really comment, but I have not yet seen a justification for the non-flag
version of the property. Even if there's a justification for documenting
that other mode in the DT, I'm likely to still think boolean properties
are a better fit.

> > > > > If ecc
> > > > > +          strength and spare area size are set by nand-ecc-strength
> > > > > +          and brcm,nand-oob-sector-size in the dts, these settings
> > > > > +          have precedence and override this flag.
> > > > 
> > > > Again, IMO, this is not for the binding to decide. That is a decision
> > > > for the operating system to make.
> > > > 
> > > Again this is just for historic reason and BCMBCA as late player does not
> > > want to break any existing dts setting.  So I thought it is useful to
> > > explain here.  I can remove this text if you don't think it should be in the
> > > binding document.
> > 
> > I don't, at least not in that form. I think it is reasonable to explain
> > why these values are better though.
> I understand the decision is for OS/driver to make. If we were to write
> another driver for other OS, the same precedence will apply too so the
> binding works the same way across all the OS. So I am not sure what better
> explanation or form I can put up here. I am open to any suggestion or I just
> delete it.

I would just delete it tbh.

Thanks,
Conor.

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