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

From: William Zhang <william.zhang@broadcom.com>
To: Conor Dooley <conor@kernel.org>
Cc: David Regan <dregan@broadcom.com>,
	dregan@mail.com, miquel.raynal@bootlin.com, richard@nod.at,
	vigneshr@ti.com, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
	computersforpeace@gmail.com, kdasu.kdev@gmail.com,
	linux-mtd@lists.infradead.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, joel.peshkin@broadcom.com,
	tomer.yacoby@broadcom.com, dan.beygelman@broadcom.com,
	anand.gore@broadcom.com, kursad.oney@broadcom.com,
	florian.fainelli@broadcom.com, rafal@milecki.pl,
	bcm-kernel-feedback-list@broadcom.com, andre.przywara@arm.com,
	baruch@tkos.co.il, linux-arm-kernel@lists.infradead.org,
	dan.carpenter@linaro.org
Subject: Re: [PATCH v3 01/10] dt-bindings: mtd: brcmnand: Updates for bcmbca SoCs
Date: Fri, 26 Jan 2024 10:09:22 -0800	[thread overview]
Message-ID: <9af23e54-a484-44b7-bf88-0ade194ab74e@broadcom.com> (raw)
In-Reply-To: <20240126-armadillo-clean-e3509ed23ed5@spud>


[-- Attachment #1.1: Type: text/plain, Size: 4404 bytes --]


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@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?
> 
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 #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]

[-- Attachment #2: Type: text/plain, Size: 144 bytes --]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/