by Conor Dooley

[permalink] [raw]

Subject: Re: [PATCH v5 02/12] dt-bindings: mtd: brcmnand: Add WP pin connection property

On Wed, Feb 07, 2024 at 12:22:47PM -0800, William Zhang wrote:
> Add brcm,wp-not-connected property to have an option for disabling this
> feature on broadband board design that does not connect WP pin.
>
> Signed-off-by: William Zhang <[email protected]>

Reviewed-by: Conor Dooley <[email protected]>

Thanks,
Conor.

Attachments:

(No filename) (348.00 B)
signature.asc (235.00 B)
Download all attachments

2024-02-09 17:44:44

2024-02-20 10:28:45

by Miquel Raynal

[permalink] [raw]

Subject: Re: [PATCH v5 11/12] mtd: rawnand: brcmnand: exec_op helper functions return type fixes

[email protected] wrote on Wed, 7 Feb 2024 12:22:56 -0800:

> From: David Regan <[email protected]>
>
> fix return type for exec_op reset and status detect helper functions
>
> Reported-by: Dan Carpenter <[email protected]>
> Closes: http://lists.infradead.org/pipermail/linux-mtd/2023-December/102423.html
> Fixes: 3c8260ce7663 ("mtd: rawnand: brcmnand: exec_op implementation")
> Signed-off-by: David Regan <[email protected]>
> Signed-off-by: William Zhang <[email protected]>
> Reviewed-by: William Zhang <[email protected]>

By the way, a Cc: stable tag might be useful as otherwise you may leak
an error code in a status value (which is a bug). And move this patch
first in your series so we're sure it does not conflict with any of the
other patches and can be backported more easily.

Thanks,
Miquèl

2024-02-21 01:11:58

On 2/21/24 16:40, Florian Fainelli wrote:
> On 2/21/24 16:37, Florian Fainelli wrote:
>> From: Florian Fainelli <[email protected]>
>>
>> On Wed, 7 Feb 2024 12:22:51 -0800, William Zhang
>> <[email protected]> wrote:
>>> Enable the nand controller and add WP pin connection property in actual
>>> board dts as they are board level properties now that they are disabled
>>> and moved out from SoC dtsi.
>>>
>>> Also remove the unnecessary brcm,nand-has-wp property from AC5300 board.
>>> This property is only needed for some old controller that this board
>>> does not apply.
>>>
>>> Signed-off-by: William Zhang <[email protected]>
>>> ---
>>
>> Applied to
>> https://github.com/Broadcom/stblinux/commits/devicetree/next, thanks!
>> --
>> Florian
>
> Actually no, cannot apply this just yet since this depends upon patch #5.
Correct. Please wait for v6.

Attachments:

smime.p7s (4.11 kB)
S/MIME Cryptographic Signature

2024-02-22 08:19:35

Hi Rafal,

On 2/22/24 00:27, Rafał Miłecki wrote:
> On 2024-02-07 21:22, William Zhang wrote:
>> diff --git a/arch/arm64/boot/dts/broadcom/bcmbca/bcm4908.dtsi
>> b/arch/arm64/boot/dts/broadcom/bcmbca/bcm4908.dtsi
>> index 2f124b027bbf..336016e334d9 100644
>> --- a/arch/arm64/boot/dts/broadcom/bcmbca/bcm4908.dtsi
>> +++ b/arch/arm64/boot/dts/broadcom/bcmbca/bcm4908.dtsi
>> @@ -589,7 +589,7 @@ hsspi: spi@1000 {
>>              status = "disabled";
>>          };
>>
>> -        nand-controller@1800 {
>> +        nand_controller: nand-controller@1800 {
>>              #address-cells = <1>;
>>              #size-cells = <0>;
>>              compatible = "brcm,nand-bcm63138", "brcm,brcmnand-v7.1",
>> "brcm,brcmnand";
>> @@ -597,7 +597,7 @@ nand-controller@1800 {
>>              reg-names = "nand", "nand-int-base";
>>              interrupts = <GIC_SPI 37 IRQ_TYPE_LEVEL_HIGH>;
>>              interrupt-names = "nand_ctlrdy";
>> -            status = "okay";
>> +            status = "disabled";
>>
>>              nandcs: nand@0 {
>>                  compatible = "brcm,nandcs";
>
> You disable NAND controller here breaking support for all dependant
> devices.
>
> I see you fix it afterwards in PATCH 06/12 but it's still a poor way of
> handling such change and this issue can be hit e.g. during git bisect.
>
> Also: given that 99% of BCM4908 devices use NAND controller can we just
> save ourselves efforts and keep it enabled in .dtsi?
>
I was trying to fix this according to the dts convention per
maintainers' requirement(only enable the node in the board dts, not soc
include). We have 100% of BCMBCA devices use strap for ecc but we still
have to put the property in board dts.
I understand if someone try to bisect the commits, it may hit the
problem but this will be very obvious that controller is not even shown
in the boot up so hopefully it is easy to figure out. Anyway if
maintainer(Minque and/or anyone from device tree side) is okay with your
request, I can change it back to enable in the dtsi.

Attachments:

smime.p7s (4.11 kB)
S/MIME Cryptographic Signature

2024-02-23 03:35:29

by William Zhang

[permalink] [raw]

Subject: Re: [PATCH v5 09/12] mtd: rawnand: brcmnand: Add support for getting ecc setting from strap

Hi Miquel,

On 2/20/24 01:53, Miquel Raynal wrote:
> Hi William,
>
> [email protected] wrote on Wed, 7 Feb 2024 12:22:54 -0800:
>
>> BCMBCA broadband SoC based board design does not specify ecc setting in
>> dts but rather use the SoC NAND strap info to obtain the ecc strength
>> and spare area size setting. Add brcm,nand-ecc-use-strap dts propety for
>> this purpose and update driver to support this option.
>>
>> The generic nand ecc settings still take precedence over this flag. For
>> example, if nand-ecc-strength is set in the dts, the driver ignores the
>> strap setting and falls back to original behavior. This makes sure that
>> the existing BCMBCA board dts still works the old way even the strap
>> flag is set in the BCMBCA chip dtsi.
>>
>> Signed-off-by: William Zhang <[email protected]>
>> Reviewed-by: David Regan <[email protected]>
>>
>> ---
>>
>> Changes in v5: None
>> Changes in v4:
>> - Update the comments for ecc setting selection
>>
>> Changes in v3: None
>> Changes in v2:
>> - Minor cosmetic fixes
>>
>> drivers/mtd/nand/raw/brcmnand/brcmnand.c | 83 ++++++++++++++++++++++--
>> 1 file changed, 76 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> index 73fdf7ce21aa..efeee9e80213 100644
>> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c
>> @@ -1038,6 +1038,19 @@ static inline int brcmnand_sector_1k_shift(struct brcmnand_controller *ctrl)
>> return -1;
>> }
>>
>> +static int brcmnand_get_sector_size_1k(struct brcmnand_host *host)
>> +{
>> + struct brcmnand_controller *ctrl = host->ctrl;
>> + int shift = brcmnand_sector_1k_shift(ctrl);
>> + u16 acc_control_offs = brcmnand_cs_offset(ctrl, host->cs,
>> + BRCMNAND_CS_ACC_CONTROL);
>> +
>> + if (shift < 0)
>> + return 0;
>> +
>> + return (nand_readreg(ctrl, acc_control_offs) >> shift) & 0x1;
>
> What is this & 0x1 ? If you return a yes/no value, please make this
> function return a bool. Also, please use intermediate steps to clarify
> what you do.
>
> sector_1k_bit = ...;
> acc = nand_readreg();
> return acc & BIT(sector_1k_bit);
>
> Or something like that.
>
This sector 1k bit is just single bit field. But we do want to return
negative for error condition and 0 for 512 sector and 1 for 1K sector
size. Will add intermediate steps.

>> +}
>> +
>> static void brcmnand_set_sector_size_1k(struct brcmnand_host *host, int val)
>> {
>> struct brcmnand_controller *ctrl = host->ctrl;
>> @@ -1055,6 +1068,38 @@ static void brcmnand_set_sector_size_1k(struct brcmnand_host *host, int val)
>> nand_writereg(ctrl, acc_control_offs, tmp);
>> }
>>
>> +static int brcmnand_get_spare_size(struct brcmnand_host *host)
>> +{
>> + struct brcmnand_controller *ctrl = host->ctrl;
>> + u16 acc_control_offs = brcmnand_cs_offset(ctrl, host->cs,
>> + BRCMNAND_CS_ACC_CONTROL);
>> + u32 acc = nand_readreg(ctrl, acc_control_offs);
>> +
>> + return (acc & brcmnand_spare_area_mask(ctrl));
>> +}
>> +
>> +static int brcmnand_get_ecc_strength(struct brcmnand_host *host)
>
> _from_strap
>
>> +{
>> + struct brcmnand_controller *ctrl = host->ctrl;
>> + u16 acc_control_offs = brcmnand_cs_offset(ctrl, host->cs,
>> + BRCMNAND_CS_ACC_CONTROL);
>> + int sector_size_1k = brcmnand_get_sector_size_1k(host);
>> + int spare_area_size, ecc_level, ecc_strength;
>> + u32 acc;
>> +
>> + spare_area_size = brcmnand_get_spare_size(host);
>> + acc = nand_readreg(ctrl, acc_control_offs);
>> + ecc_level = (acc & brcmnand_ecc_level_mask(ctrl)) >> ctrl->ecc_level_shift;
>
> Please use FIELD_PREP/FIELD_GET.
>
These macros do not work here as the mask is not constant.

>> + if (sector_size_1k)
>> + ecc_strength = ecc_level * 2;
>> + else if (spare_area_size == 16 && ecc_level == 15)
>> + ecc_strength = 1; /* hamming */
>> + else
>> + ecc_strength = ecc_level;
>> +
>> + return ecc_strength;
>> +}
>> +
>> /***********************************************************************
>> * CS_NAND_SELECT
>> ***********************************************************************/
>> @@ -2622,19 +2667,43 @@ static int brcmnand_setup_dev(struct brcmnand_host *host)
>> nanddev_get_memorg(&chip->base);
>> struct brcmnand_controller *ctrl = host->ctrl;
>> struct brcmnand_cfg *cfg = &host->hwcfg;
>> - char msg[128];
>> + struct device_node *np = nand_get_flash_node(chip);
>> u32 offs, tmp, oob_sector;
>> - int ret;
>> + int ret, sector_size_1k = 0;
>> + bool use_strap = false;
>> + char msg[128];
>>
>> memset(cfg, 0, sizeof(*cfg));
>> + use_strap = of_property_read_bool(np, "brcm,nand-ecc-use-strap");
>>
>> - ret = of_property_read_u32(nand_get_flash_node(chip),
>> - "brcm,nand-oob-sector-size",
>> + /*
>> + * Set ECC size and strength based on hw configuration from strap
>> + * if brcm,nand-ecc-use-strap is set. However if nand-ecc-strength
>> + * is set, its value will be used and ignore the strap setting.
>
> Please error out in this case. It's one or the other, not both.
>
Will update.

>> + */
>> + if (chip->ecc.strength)
>> + use_strap = 0;
>> +
>> + if (use_strap) {
>> + chip->ecc.strength = brcmnand_get_ecc_strength(host);
>> + sector_size_1k = brcmnand_get_sector_size_1k(host);
>> + if (chip->ecc.size == 0) {
>> + if (sector_size_1k < 0)
>> + chip->ecc.size = 512;
>> + else
>> + chip->ecc.size = 512 << sector_size_1k;
>> + }
>
> I'd instead make a function named brcmnand_get_ecc_settings() with the
> chip->ecc parameter, so you can directly fill the entries without
> getting another time the sector_size_1k thing.
>
> Strength and step size are tightly linked, it does make sense to derive
> them both at the same time.
>
Will update.

>> + }
>> +
>> + ret = of_property_read_u32(np, "brcm,nand-oob-sector-size",
>> &oob_sector);
>> if (ret) {
>> - /* Use detected size */
>> - cfg->spare_area_size = mtd->oobsize /
>> - (mtd->writesize >> FC_SHIFT);
>> + if (use_strap)
>> + cfg->spare_area_size = brcmnand_get_spare_size(host);
>> + else
>> + /* Use detected size */
>> + cfg->spare_area_size = mtd->oobsize /
>> + (mtd->writesize >> FC_SHIFT);
>> } else {
>> cfg->spare_area_size = oob_sector;
>> }
>
>
> Thanks,
> Miquèl

Attachments:

smime.p7s (4.11 kB)
S/MIME Cryptographic Signature