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

From: William Zhang
Date: Fri Jan 26 2024 - 13:09:35 EST




On 1/26/24 08:46, Conor Dooley wrote:
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@xxxxxxxxxxxx>

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?

Yes that is a bit in the controller reg to disable the feature by the driver even the chip have the WP pin connected.

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 ;)

Agree it is more on the software for that particular mode.

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.

Well if binding only say 3 possible value, you can not use this property for other value of course. DTS binding check will fail. But agree there is no check on this in the driver side for any integer property.

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.

That's fine. I will just change to the boolean.

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


Thanks,
Conor.

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature